Add keep flag to protect snapshots from retention cleanup #148
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-144-keep-snapshots"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a
keepboolean column to snapshots (migration V6) and a toggle endpoint (POST /snapshots/{id}/keep) so events or single frames can be protected from retention cleanup.delete_expired_snapshots) skips kept rows_keep_btn.htmlscaffolding left over from the defunct clips conceptCloses #144
WuMing
Found 1 issue(s). See inline comments below.
@ -436,1 +504,4 @@)@router.post("/snapshots/{snapshot_id}/keep")security [MEDIUM]
New state-changing endpoint has no visible authentication or authorization check (no Depends(auth) or router-level auth in the diff). Any caller who can reach the web UI can toggle keep on arbitrary snapshot IDs (IDOR), potentially bypassing retention cleanup and exhausting storage. Add an authentication/authorization dependency and verify the caller owns or is permitted to modify the snapshot (A01).
WuMing
Found 1 issue(s). See inline comments below.
@ -436,1 +545,4 @@)@router.post("/snapshots/{snapshot_id}/keep")security [MEDIUM]
A01: This state-changing endpoint has no authentication, authorization, or CSRF protection. Any client that can reach the web UI can toggle keep on arbitrary snapshot IDs, including every snapshot in the resolved journey, and may interfere with retention protection. Require an authenticated user and ownership/permission verification before mutating snapshots, and add CSRF protections for state-changing routes.
src/cang/web/routes/snapshots.py🔴 Not actioned — false premise. The endpoint is authenticated:
app.pymounts the snapshots router withapp.include_router(snapshots_router, dependencies=[Depends(require_auth(cfg.web))]), which applies to every route in that router including this one. Unauthenticated calls raiseNotAuthenticatedExceptionand are redirected to/login. The check is intentionally router-level (pre-existing pattern shared by all routes), so it does not appear in this diff. Ownership checks do not apply — cang is a single-user NVR.src/cang/web/routes/snapshots.py🔴 Not actioned — same finding as review 191 (see reply above): authentication is enforced at router level via
Depends(require_auth(cfg.web))inapp.py. CSRF is mitigated by the session cookie being set withSameSite=Lax(auth.py), so cross-site POSTs do not carry the session; single-user NVR, no multi-user authorization model to enforce.