Close the residual gaps the 2026-09-18 audit left open
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
This commit is contained in:
@@ -57,3 +57,44 @@ Fixed in commit "Harden the API and the data layer after a security and performa
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user