fix(audit): address /code-review findings

- 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".
This commit is contained in:
npeter83 2026-07-12 07:47:07 +02:00
parent 318fdc4812
commit 32d8575f79
4 changed files with 34 additions and 12 deletions

View file

@ -64,7 +64,9 @@ def record(
actor_id=actor_id, actor_id=actor_id,
action=action, action=action,
target_type=target_type, 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], summary=(summary or None) and summary[:255],
before=before, before=before,
after=after, after=after,

View file

@ -1,5 +1,7 @@
"""Admin Configuration page API: read/edit the DB-overridable operational settings defined in """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.""" app.sysconfig. Secret values (e.g. SMTP password) are write-only never returned."""
import re
from fastapi import APIRouter, Depends, HTTPException from fastapi import APIRouter, Depends, HTTPException
from sqlalchemy.orm import Session from sqlalchemy.orm import Session
@ -13,6 +15,15 @@ from app.routes.admin import admin_user
router = APIRouter(prefix="/api/admin/config", tags=["admin"]) 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("") @router.get("")
def get_config(_: User = Depends(admin_user), db: Session = Depends(get_db)) -> dict: 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"]) sysconfig.set_value(db, key, payload["value"])
except (TypeError, ValueError) as exc: except (TypeError, ValueError) as exc:
raise HTTPException(status_code=400, detail=str(exc) or "Invalid value") 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) new = None if s.secret else sysconfig.get(db, key)
audit.record( if s.secret or new != old:
db, admin.id, AuditAction.CONFIG_SET, target_type="config", target_id=key, audit.record(
summary=f"{key} = (secret updated)" if s.secret else f"{key} = {new}", db, admin.id, AuditAction.CONFIG_SET, target_type="config", target_id=key,
before=None if s.secret else {"value": old}, summary=f"{key} = (secret updated)" if s.secret else f"{key} = {_redact(new)}",
after=None if s.secret else {"value": new}, before=None if s.secret else {"value": _redact(old)},
) after=None if s.secret else {"value": _redact(new)},
db.commit() )
db.commit()
return sysconfig.describe(db) return sysconfig.describe(db)
@ -67,7 +82,7 @@ def reset_config(
audit.record( audit.record(
db, admin.id, AuditAction.CONFIG_RESET, target_type="config", target_id=key, db, admin.id, AuditAction.CONFIG_RESET, target_type="config", target_id=key,
summary=f"{key} → default", summary=f"{key} → default",
before=None if s.secret else {"value": old}, before=None if s.secret else {"value": _redact(old)},
) )
db.commit() db.commit()
return sysconfig.describe(db) return sysconfig.describe(db)

View file

@ -130,11 +130,14 @@ def _notify_done(db, actor_id: int, job_id: str, status: str, summary: str | Non
def _run_changed(result) -> bool: 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: if result is None:
return False return False
if isinstance(result, dict): 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)): if isinstance(result, (int, float)):
return result != 0 return result != 0
return bool(result) return bool(result)

View file

@ -26,6 +26,8 @@ export function pageTitleKey(page: Page): string {
return "header.configuration"; return "header.configuration";
case "users": case "users":
return "header.users"; return "header.users";
case "audit":
return "audit.title";
case "settings": case "settings":
return "settings.title"; return "settings.title";
default: default: