jacobh2
88197060de
Re-checked every item of the audit's own outcome section against the tree.
Most held, but five items marked fixed had residual gaps — one of them a
live IDOR — and the deferred list was untouched.
Security:
- Analytics IDOR: /api/analytics/{ws}/overview and /projection took
child_id as an unchecked query param, and get_overview called
get_days_left_at_age_4 without workspace_id, so the ownership guard
never ran. Any member could read another workspace's child projection.
Fixed at both layers.
- Scraper: session ids no longer logged at INFO; a persist-callback
failure's raw exception text and the FK maintenance banner's scraped
text no longer reach the poll client.
- Email is now case-normalised at the API boundary, with migration 009
lowercasing existing rows (aborts naming colliding rows rather than
failing opaquely on the UNIQUE constraint).
Frontend correctness:
- usePlanProposal/usePlanSync gained the stale-response guard the other
hooks already had.
- PlanSyncProvider no longer fires POST /plan/sync before auth resolves
off a blindly-read localStorage workspaceId.
- Busy flags are cleared unconditionally across all 16 useLatestRequest
consumers. setBusy(true) was unconditional while the clear was guarded
by isCurrent, so a superseded call stranded its flag forever — under
StrictMode this hung every in-app navigation behind the PageSkeleton.
isCurrent decides which response's data wins, never whether a call
releases the flag it raised.
Performance:
- Calendar no longer blanks on a mutation, and a multi-day save issues N
writes plus one background refetch instead of 2N requests.
- take_pl_on_red_days moved onto GET /auth/context, dropping a redundant
GET /workspace/:id from every cold load.
CI and repo hygiene: least-privilege permissions block, actions pinned to
commit SHAs, npm ci, uv sync --locked, failure-only artifact upload,
dependabot; dead frontend/out.js removed; CSP added to the nginx template
(repeated into the location blocks that set their own add_header, since
nginx does not merge them across levels).
Left deferred by decision: refresh-token revocation, the phase-2 contract
change, M4, M5, L2, and H12's cross-route cache. Details and the smaller
follow-ups browser verification turned up are in the audit report.
Backend 901 passed, coverage 92%. Frontend 1322 tests, coverage
98.17/94.79/92.14/98.17.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PngVF38MLpdkgXcByKfdW6
101 lines
14 KiB
Markdown
101 lines
14 KiB
Markdown
# Security + performance audit — merged findings (2026-09-18)
|
||
|
||
Detail reports: authz.md, backend-perf.md, scraper-config-ops.md, frontend.md (same dir).
|
||
Baseline before fixes: backend 552 passed, ruff clean; frontend 61 files passed, cov 98.75/94.61/92.99/98.75.
|
||
|
||
## CRITICAL
|
||
C1. 14 routes need no auth at all — plan/*, leave-wishes, day-overrides, vacation-days (routers/plan.py:12, leave_wishes.py:12, day_overrides.py:11, vacation_days.py:11). Anonymous read/write/delete of any workspace by id.
|
||
C2. Self-invite into any workspace: POST /api/workspace/{any_ws}/invitations + redeem (workspace.py:159-205, workspace_service.py:228-282). Grants full legitimate membership.
|
||
C3. Real FK personal data committed: captures/04_*, captures/05_* (child name, payout kronor, tax rate, balances), captures/02_* (BankID QR data-code), test.html (idpx login dump). In history since 4e9fb37. No git remote configured.
|
||
|
||
## HIGH
|
||
H1. IDOR on every authenticated workspace/child/personal_data route — analytics, calendar, settings, planned_days, fetch_data (GET latest/history, start), workspace members/rename/remove. Body ids never reconciled with caller. Only workspace_service._get_member exists and is not caller-scoped.
|
||
H2. Scraper: poll/cancel by session_id without owner (fetch_data.py:190-212); session ids logged at INFO (fk_scraper.py:172,198); start targets arbitrary pd/child → data poisoning.
|
||
H3. Scraper debug artifacts (logged-in FK HTML + full screenshot) written to disk by default (config.py:121 scraper_debug_dir="./debug/scraper"; fk_scraper.py:245-270,294). Balances logged at DEBUG (fk_scraper.py:443,458,465,479-484,500); raw page text in ValueError surfaced to poll client (:450,:476,:292). Default log_level="DEBUG" (config.py:111).
|
||
H4. Default JWT secret silently accepted (config.py:106); no jti/iat/revocation; 7-day stateless refresh; is_active never checked; /api/auth/refresh unthrottled.
|
||
H5. No atomicity/isolation in plan apply/sync (planning_service.py:486-513): per-row COMMIT, deletes committed before creates; concurrent syncs corrupt mid-write. Explicit BEGIN on the shared connection is unsafe without a lock.
|
||
H6. Per-row commit cost: 300-row apply 89 ms (macOS) / seconds on Linux fsync vs ~1 ms in one txn. No WAL/synchronous/busy_timeout PRAGMAs (connection.py:186-195).
|
||
H7. argon2 hash/verify block the event loop 22 ms each (auth_service.py:72,84,88).
|
||
H8. No GZipMiddleware; /api/calendar/days ships 183 KB JSON per request, built 3× (calendar_service.py:125, models/calendar.py:63).
|
||
H9. Zero indexes anywhere. UNIQUE autoindexes cover the main list paths; full scans on count_by_parent_type, count_dubbeldagar, get_latest_by_parent, delete_by_wish, leave_wish/child/personal_data by workspace, invitation list, all FK-cascade columns. 14-statement DDL in backend-perf.md §4.
|
||
H10. Frontend: 15-min access token, no 401→refresh→retry (api.ts:523-579, AuthContext.tsx:28-43) → sessions silently die after 15 min; errors render as banners, isAuthenticated stays true.
|
||
H11. Frontend: stale-response race in all 10 hooks (no abort/request-id guard) → wrong child's data shown on fast child switch / year paging.
|
||
H12. Frontend cold-load waterfall: refresh → /auth/context → /workspace ×2 (dup) → 6–12 page requests; Calendar auto-POSTs /plan/sync then refetches.
|
||
|
||
## MEDIUM
|
||
M1. revoke_invitation unscoped (workspace_service.py:274-281); invitation_id never exposed → low exploitability.
|
||
M2. _sessions never purged; QR PNG + result retained for process lifetime (fk_scraper.py:113,358,432).
|
||
M3. OpenAPI docs public in all envs (main.py:81); migration failure swallowed (main.py:61-65).
|
||
M4. Rate limiter per-process, IP-keyed, no proxy trust (rate_limit.py:10).
|
||
M5. N+1 fan-outs: allocation loader 5+C+C·P+P, analytics overview 2+6P (3·(P−1) redundant), tips 6+P+C·(P+1), dismiss_tip recomputes full pipeline. Measured 0.05 ms/query → not a bottleneck today.
|
||
M6. Frontend: logout doesn't clear token if api.logout() throws (AuthContext.tsx:66-70); in-flight WorkspaceContext fetches can repopulate old account's data after logout.
|
||
M7. Frontend: every planned-day mutation blanks the Calendar page with a spinner; multi-day save is N sequential POST+GET.
|
||
M8. Frontend: useScraperFetch setInterval(1000) async callback, no overlap guard, no unmount cleanup.
|
||
M9. localStorage workspaceId fetched blindly (moot once H1 fixed server-side).
|
||
|
||
## LOW
|
||
L1. 40 router sites pass str(exc) to HTTP detail; register 409 echoes email; login timing oracle (no dummy verify); case-sensitive email → duplicate accounts.
|
||
L2. personnummer_enc column never written — dead column.
|
||
L3. CI: no permissions block, tag-pinned actions, npm install in e2e, playwright-report uploaded always, no uv lock --check, no dependabot.
|
||
L4. scripts/demo_plan.py crashes (stale AllocationInput signature, L123).
|
||
L5. Frontend: out.js dead artifact; no CSP; grid memoization ~1–3 ms/render (not worth it); bundle 97 kB gzip single chunk (splitting not yet worthwhile); dead /calendar?date= link (Dashboard.tsx:136).
|
||
|
||
## REFUTED leads
|
||
- Allocator propose (0.9 ms) and holidays json.load (0.4 ms) / get_red_days (<0.01 ms) — not worth to_thread.
|
||
- Refresh-cookie CSRF — mitigated by SameSite=Lax + explicit origin allowlist; only a footgun if cors_origins=["*"].
|
||
- logout delete_cookie attrs — Starlette defaults path="/" on both; cosmetic.
|
||
- DevFetch in prod bundle — tree-shaken (import.meta.env.DEV constant-folded).
|
||
|
||
## Fix scope this run (approved: critical + high)
|
||
Wave 1 (parallel, disjoint files): T1 authz (C1,C2,H1,H2,M1) · T2 auth/config/ops hardening (H4,H7,H8,M3, L1 email echo + dummy verify) · T6 frontend (H10,H11,M6,M8)
|
||
Wave 2 (after T1): T4 DB layer — UoW + PRAGMAs + batching + indexes (H5,H6,H9) · T3 scraper hygiene (H3,M2) · L4 demo_plan fix
|
||
User decision: C3 history purge vs delete-only.
|
||
Deferred to follow-up: H12, M4, M5, M7, M9, L2, L3, L5.
|
||
|
||
## Outcome (2026-09-18)
|
||
Fixed in commit "Harden the API and the data layer after a security and performance audit": C1, C2, C3 (history rewritten with git filter-repo; backup bundle at scratchpad/pre-purge-backup.bundle), H1–H11, M1, M2, M3, M6, M8, L1 (email echo + timing), L4; is_active persisted (migration 011).
|
||
Final gates: backend 632 passed / ruff clean / alembic head OK; frontend 958 tests, typecheck+lint clean, cov 98.77/94.64/92.04/98.77.
|
||
Deferred (follow-up list): H12 cold-load waterfall + no cross-route cache; M4 rate limiter proxy trust / shared store; M5 N+1 query collapsing (0.05 ms/query — not a bottleneck); M7 Calendar spinner on every mutation + sequential multi-day save; M9 (moot server-side); L2 drop personnummer_enc; L3 CI hardening (permissions block, SHA pins, npm ci, failure-only artefact upload, uv lock --check, dependabot); L5 out.js removal, CSP meta, dead /calendar?date= link.
|
||
Phase-2 contract change (not started): drop workspace_id from request bodies and derive it from the caller.
|
||
|
||
## Re-check (2026-09-20)
|
||
Verified every item of 2026-09-18 outcome against the tree. Confirmed intact: C1, C2, C3, H5–H11, M1, M2, M3, M6, M8, L1 (partly), L4. Five residual gaps found inside items marked fixed; deferred list untouched.
|
||
|
||
Fixed this pass:
|
||
- R1 (residual H1) — IDOR on `/api/analytics/{ws}/overview` and `/projection`: `child_id` unchecked query param, analytics_service.get_overview() called get_days_left_at_age_4 without workspace_id (guard at analytics_service.py:240 never ran). Member of workspace A could read workspace B's child projection. Fixed: `scope.check_child(child_id)` in router (matching /age4 route pattern) and `workspace_id=` passed through service.
|
||
- R2 (residual H2) — session ids logged at INFO throughout fk_scraper.py (172,198). All session-id-carrying logger.info() calls downgraded to DEBUG; "scraper session done" kept at INFO without id; debug-artifact warning stays WARNING without id. One WARNING (tier sum mismatch, fk_scraper.py:723) deliberately keeps id.
|
||
- R3 (residual H3) — raw str(exc) from persist-callback failure and FK maintenance banner's scraped text reached poll client. Both replaced with fixed strings; full detail stays in ERROR/DEBUG logs.
|
||
- R4 (residual L1) — case-sensitive email: Foo@x.se and foo@x.se became two accounts. Lowercasing field_validator on RegisterRequest/LoginRequest; migration 009_lowercase_user_email (aborts with named-rows error if two rows would collide, not opaque UNIQUE constraint failure).
|
||
- R5 (residual H11) — usePlanProposal and usePlanSync had no stale-response guard. Both now use shared useLatestRequest helper (as 15 other hooks do).
|
||
- M9 — WorkspaceContext seeded workspaceId from localStorage synchronously; PlanSyncProvider's effect did not depend on auth → cold load POST /api/plan/sync before auth resolved, 401'd, raced AuthContext's own refresh. Effect now gated on isAuthenticated.
|
||
- M7 — every planned-day mutation blanked Calendar behind skeleton; multi-day save issued 2N requests (write+full refetch per date). usePlannedDays gained background/isRefreshing split (like useLeaveWishes) + skipRefresh option; Calendar's multi-day loop now N writes + one trailing background refresh + one bumpPlanVersion in finally (error path refreshes too). runSync's refresh also made background.
|
||
- H12 (partial) — take_pl_on_red_days added to GET /auth/context response; WorkspaceContext.refreshContext now builds workspace state from context alone, drops redundant GET /workspace/:id from every cold load. Workspace.name/createdAt optional (no read-side consumer in src/).
|
||
- L3 — CI hardened: top-level permissions: contents: read; all four actions pinned to commit SHAs; e2e npm install → npm ci; both uv sync → uv sync --locked; playwright-report upload if: always() → if: failure(). New .github/dependabot.yml (github-actions, npm, uv).
|
||
- L5 (partial) — frontend/out.js dead artifact removed + gitignored. CSP added to nginx.conf.template (from production build). Note: nginx does not merge add_header across levels; four security headers repeated in three location blocks (Cache-Control: /assets/, /fonts/, = /index.html) — verified curl against running container all four present on every route. Three pre-existing headers had silently been absent from those routes before.
|
||
|
||
Decisions taken (not oversights):
|
||
- H4 refresh-token revocation (jti/iat) NOT implemented — stateless refresh stands. is_active checked on refresh → disabled account cut off; only per-token revocation missing.
|
||
- Phase-2 contract change (drop workspace_id from bodies) deferred — every route already reconciles body value against caller's scope, defence-in-depth not a live hole.
|
||
- H12 step 2 (cross-route cache) re-deferred: of 7 Dashboard fan-out endpoints only 2 safe to cache today; 5 have same-hook or cross-page writers (usePlannedDays, useLeaveWishes, useWorkspaceMembers/Settings, saveManualFetch) not wired to shared invalidation signal. URL-keyed cache would serve pre-mutation data to mutation's own refetch. If revisited, go endpoint-by-endpoint.
|
||
- Still deferred: M4 (rate limiter per-process/IP-keyed/proxy trust), M5 (N+1 fan-outs, 0.05 ms/query), L2 (drop personnummer_enc), L5 bundle splitting.
|
||
- New follow-up: /auth/context could also carry workspace name/created_at if future feature needs them on cold load.
|
||
|
||
Gates after this pass: backend 901 passed / ruff clean / alembic head OK / coverage 92%. Frontend 1320 tests in 75 files / typecheck+lint clean / coverage 98.16 lines, 94.79 functions, 92.15 branches, 98.16 statements.
|
||
|
||
### Browser verification (2026-09-20) and two defects it caught
|
||
Two rounds of real-browser verification against seeded dev instance caught two bugs the unit suites missed; both fixed + re-verified.
|
||
- M7 incomplete on first landing: `usePlannedDays`' `planVersion` effect called `fetch()` with no options, so mutators' `bumpPlanVersion()` re-ran it as second blocking fetch—2 GETs per save, page blanked on Advanced (direct planned-day) save path. Fixed: effect sole fetcher, mutators only bump, effect backgrounds everything but first load or childId switch.
|
||
- `isLoading` stranded forever on in-app navigation: `useLatestRequest` pattern latent flaw — `setBusy(true)` ran unconditional but `finally { if (isCurrent(requestId)) setBusy(false) }` cleared it only for still-current request, so superseded calls left flag set. React StrictMode double-invokes mount effect; on SPA navigation (childId already known) both invocations fetched—one foreground, one background—leaving `isLoading` true behind PageSkeleton. Fixed in all 16 hooks: unguarded busy-flag clear (`isCurrent` decides which response's DATA wins, never whether call releases flag it raised); plus in-flight dedup in `usePlannedDays`. Regression tests confirmed to fail against pre-fix code.
|
||
|
||
Verified in-browser post-fix: 4 alternating in-app navigations all render without skeleton; 1 GET /api/planned-days per navigation; Advanced single-day save = 1 write + 1 GET, no blanking; 4-day drag-select = 4 writes back-to-back + exactly 1 trailing GET; cold load issues no GET /api/workspace/{id}; red-days flag round-trips through /auth/context and honored by Calendar's free-day filter; SV and EN both clean.
|
||
|
||
New follow-ups found (not fixed):
|
||
- `useCalendarDays` mutates `isFirstPlanVersionRun` ref in effect body; StrictMode double-invoke can fire `background: true` fetch on page with nothing loaded. Unguarded flag clear stops hangs, can still cause transient extra request / brief flash (dev only).
|
||
- `PlanSyncProvider` handles stale `localStorage` workspaceId poorly: after workspace it names is gone, background sync 404s and logs error instead of clearing stale id.
|
||
- Cold load issues `GET /api/workspace/{ws}/members` twice.
|
||
- Cold load issues `POST /api/auth/refresh` twice (consistent with StrictMode double-invoke; dev-only, unconfirmed in prod).
|
||
- `POST /api/plan/sync` fires before `GET /api/auth/context` resolves (safe: auth resolved and server scope-checks; ordering surprise).
|
||
- Child-switch behavior could not be browser-tested: demo seed has one child only.
|
||
|
||
Frontend gates after final fix: 75 files / 1322 tests / 98.17 lines, 94.79 functions, 92.14 branches, 98.17 statements. Branch coverage 0.14 above gate of 92—thin.
|