From 32d8575f7960e41dc8fcca6764d49fbfeba747ac Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 12 Jul 2026 07:47:07 +0200 Subject: [PATCH] fix(audit): address /code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scheduler _run_changed: count a run as "changed" only on a truthy NUMERIC value — status-string no-op dicts like {"skipped":"disabled"} (Plex off, the default) or {"skipped":"no demo account"} were flooding the trail every interval. - audit.record: truncate target_id to the column width (128) like summary[:255], so an over-long target (e.g. a 128+ char demo email) can't abort the mutation the audit row is committed alongside. - config set/reset: redact URL userinfo (user:pass@) from logged non-secret values — a proxy setting can embed credentials that shouldn't land in the trail. - config set: skip the audit row when a non-secret value didn't actually change (no-op re-save shouldn't add noise); secrets are write-only so always logged. - audit page title (pageMeta): add the missing "audit" case so the browser tab reads "Audit log · Siftlode" instead of falling back to "Feed". --- backend/app/audit.py | 4 +++- backend/app/routes/config.py | 33 ++++++++++++++++++++++++--------- backend/app/scheduler.py | 7 +++++-- frontend/src/lib/pageMeta.ts | 2 ++ 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/backend/app/audit.py b/backend/app/audit.py index 485bc81..9c564c1 100644 --- a/backend/app/audit.py +++ b/backend/app/audit.py @@ -64,7 +64,9 @@ def record( actor_id=actor_id, action=action, target_type=target_type, - target_id=None if target_id is None else str(target_id), + # Truncate to the column widths so an over-long target/summary can never abort the + # mutation this row is committed alongside (an audit row must not break what it observes). + target_id=None if target_id is None else str(target_id)[:128], summary=(summary or None) and summary[:255], before=before, after=after, diff --git a/backend/app/routes/config.py b/backend/app/routes/config.py index 38b5c09..9e90a10 100644 --- a/backend/app/routes/config.py +++ b/backend/app/routes/config.py @@ -1,5 +1,7 @@ """Admin Configuration page API: read/edit the DB-overridable operational settings defined in app.sysconfig. Secret values (e.g. SMTP password) are write-only — never returned.""" +import re + from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.orm import Session @@ -13,6 +15,15 @@ from app.routes.admin import admin_user router = APIRouter(prefix="/api/admin/config", tags=["admin"]) +# Strip URL userinfo (user:pass@) so a non-secret setting that happens to embed credentials — +# e.g. an authenticated egress proxy http://user:pass@host:port — never lands plaintext in the +# audit trail. Non-secret values are shown live in the UI, but the log shouldn't retain creds. +_URL_USERINFO = re.compile(r"(://)[^/@\s]+@") + + +def _redact(v): + return _URL_USERINFO.sub(r"\1***@", v) if isinstance(v, str) else v + @router.get("") def get_config(_: User = Depends(admin_user), db: Session = Depends(get_db)) -> dict: @@ -41,15 +52,19 @@ def set_config( sysconfig.set_value(db, key, payload["value"]) except (TypeError, ValueError) as exc: raise HTTPException(status_code=400, detail=str(exc) or "Invalid value") - # Never log a secret's value — only that it was changed, by whom. + # Never log a secret's value — only that it was changed, by whom. Non-secret values are logged + # but with URL credentials redacted (a proxy/URL setting can embed user:pass). Skip the row when + # a non-secret value didn't actually change (a no-op re-save shouldn't add trail noise); secrets + # are write-only so we can't compare — always log. new = None if s.secret else sysconfig.get(db, key) - audit.record( - db, admin.id, AuditAction.CONFIG_SET, target_type="config", target_id=key, - summary=f"{key} = (secret updated)" if s.secret else f"{key} = {new}", - before=None if s.secret else {"value": old}, - after=None if s.secret else {"value": new}, - ) - db.commit() + if s.secret or new != old: + audit.record( + db, admin.id, AuditAction.CONFIG_SET, target_type="config", target_id=key, + summary=f"{key} = (secret updated)" if s.secret else f"{key} = {_redact(new)}", + before=None if s.secret else {"value": _redact(old)}, + after=None if s.secret else {"value": _redact(new)}, + ) + db.commit() return sysconfig.describe(db) @@ -67,7 +82,7 @@ def reset_config( audit.record( db, admin.id, AuditAction.CONFIG_RESET, target_type="config", target_id=key, summary=f"{key} → default", - before=None if s.secret else {"value": old}, + before=None if s.secret else {"value": _redact(old)}, ) db.commit() return sysconfig.describe(db) diff --git a/backend/app/scheduler.py b/backend/app/scheduler.py index 62d1599..c08e985 100644 --- a/backend/app/scheduler.py +++ b/backend/app/scheduler.py @@ -130,11 +130,14 @@ def _notify_done(db, actor_id: int, job_id: str, status: str, summary: str | Non def _run_changed(result) -> bool: - """Did an automatic job run actually do something worth an audit row? (No-op polls don't.)""" + """Did an automatic job run actually do something worth an audit row? (No-op polls don't.) + A "change" = a truthy NUMERIC count; status-string dicts like {"skipped": "disabled"} (Plex + off — the default) or {"skipped": "no demo account"} are no-ops, not changes, so they don't + flood the trail every interval.""" if result is None: return False if isinstance(result, dict): - return any(bool(v) for v in result.values()) + return any(isinstance(v, (int, float)) and v for v in result.values()) if isinstance(result, (int, float)): return result != 0 return bool(result) diff --git a/frontend/src/lib/pageMeta.ts b/frontend/src/lib/pageMeta.ts index 250a6e2..834193e 100644 --- a/frontend/src/lib/pageMeta.ts +++ b/frontend/src/lib/pageMeta.ts @@ -26,6 +26,8 @@ export function pageTitleKey(page: Page): string { return "header.configuration"; case "users": return "header.users"; + case "audit": + return "audit.title"; case "settings": return "settings.title"; default: