audit fixes (P3): clear patient context and account-scoped browser state at every account transition - #2619
Merged
Conversation
…n (M4) Defect: the Prescribing patient profile (age, renal/hepatic function, QTc, pregnancy, allergy classes, current medications) lived in sessionStorage under `clinical-kb-patient-profile` and nothing removed it except closing the tab. The sign-out, markSessionExpired and user-id-change handlers cleared recent queries, the answer thread and the signed-URL cache but not the profile. Trigger: shared workstation — clinician A enters a patient's physiology on /medications and signs out (or the session expires); clinician B signs in in the same tab and every medication surface evaluates B's prescribing alerts against A's patient, with the Patient details pill pre-filled. Fix: `clearPatientProfile()` in patient-profile-storage.ts removes the key and dispatches the store's own change event so mounted subscribers (which cache the parsed snapshot by raw string) re-read an empty profile. client.tsx now clears every account-scoped browser store through one `clearAccountScopedBrowserState` helper on sign-out, session expiry and signed-in user change. Clearing on markSessionExpired also wipes an in-progress patient context when a transient refresh failure is reported as expiry — accepted, because a stale profile surviving to the next sign-in is the worse failure. The initial signed-out boot path is unchanged. docs/codebase-index.md's patient-context sentence now says so instead of "cleared on tab close". Proof: tests/patient-profile-storage-account-transitions.dom.test.tsx drives AuthProvider through all three transitions and asserts the key is gone, the subscriber was notified and the snapshot is empty (red before, green after). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
… (M10)
Defect: the browser-side favourites snapshot parser only accepted timestamps
with exactly 0 or 3 fractional-second digits. The API route passes Supabase
timestamptz columns (`created_at`, `pinned_at`, `last_opened_at`, `updated_at`)
through untouched, and Postgres serialises `now()` at microsecond precision with
trailing zeros trimmed (`2026-06-27T14:10:20.550361+00:00`). The server's Zod
check is precision-agnostic, so the payload passed the server and was rejected
by the client, which nulled the whole snapshot.
Trigger: any authenticated live session with at least one favourite or set
whose stored timestamp does not happen to trim to exactly three digits — the
norm, not the exception — so the Favourites hub, mode-menu Favourites,
saved-state toggles and Continue/Recent rails showed the load-error state.
Fix: the fraction group is now 1-9 digits (`(?:\.\d{1,9})?`); the
`Number.isFinite(Date.parse(value))` guard and the exact-keys checks are
unchanged, so the parser still fails closed on non-ISO values.
Proof: tests/favourites-contract.test.ts "accepts PostgREST timestamptz
precision on the client" feeds a snapshot with 6-, 4-, 2- and 1-digit
fractions and a `+00:00` offset (null before, round-trips after) and pins four
non-ISO shapes as still rejected.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…on (L2) Defect: `database:favourites:last-opened-v1` and `database:favourites:pinned-v1` are global localStorage keys (90-day TTL) with no owner id, and nothing removed them: sign-out, session expiry and user change cleared recent queries, the answer thread and the signed-URL cache but left both keys and the module's memoised caches in place. Trigger: shared workstation — clinician A opens and pins items in the Favourites library and signs out; clinician B signs in in the same browser and the library is ordered by A's usage, showing which guideline/registry items A opened and when (item ids and times only, no patient data). Fix: `clearFavouritesStorage()` in favourites-storage.ts removes both keys, drops the in-memory caches (a `storage` event only fires in other tabs) and notifies subscribers; the auth provider's account-transition helper calls it beside clearRecentQueries(), matching the recent-queries behaviour rather than owner-scoping the keys (the read sites take no owner id). Trade-off, accepted: a clinician who signs out and back in starts from the default pins. Proof: tests/favourites.test.ts "clears both unscoped localStorage keys and the in-memory caches" (TypeError before — no such function) and the L2 block of tests/patient-profile-storage-account-transitions.dom.test.tsx, which drives AuthProvider through all three transitions (keys survived before, gone after). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
Defect: the activation wizard's draft (`caring-contacts:plan-draft` in sessionStorage, carrying the patient's name and mobile from stage 3 on) had three documented clearing paths — tab close, activation, discard — and none of them is the app's account boundary. signOut, markSessionExpired and the user-id-change handler in the auth provider never touched it, and the workspace has no sign-out of its own. Trigger: a coordinator signs out mid-draft and hands the same tab to a colleague, who signs in and opens the same referral; the previous patient's details are restored into the wizard. sessionStorage survives both the sign-out and the next sign-in. Mitigated today only by the production 404 the open P2 item exists to remove — Medium on activation. Fix: plan-draft.ts exports `clearCaringContactsBrowserState()` (the module's fourth documented way the draft goes away; a separate name so future workspace stores are added there, not to the auth provider). The auth provider's shared account-transition helper calls it through a lazy `import()` so the global shell does not carry the workspace's bundle (schedule, message rules, synthetic contacts); the removal therefore lands a tick after the transition, and a failed chunk load is swallowed. Proof: the L6 block of tests/patient-profile-storage-account-transitions.dom.test.tsx — the seam removes the key, nulls the snapshot and notifies the wizard; all three AuthProvider transitions leave the key null (draft survived before, gone after). The wizard's own tests/caring-contacts-plan-draft.dom.test.tsx, including its storage-API directory scan, still passes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
Defect: SENSITIVE_KEY used `\bquery\b` and `\banswer\b`. `_` and letters are word characters, so the bare keys were redacted but `queryText`, `query_text`, `rawQuery`, `normalizedQuery`, `question`, `answerText` and `answer_text` were not — the most natural spellings of a clinical query or answer field passed straight through the layer whose header promises that clinical query/answer content never reaches the log. Trigger: any log call that puts clinical query or answer text under one of those keys. No call site does so today and the Sentry bridge has its own allowlist, so nothing leaks now; the defect is that the redaction layer did not deliver the call-site-independent protection it promises. Fix: `query`, `question` and `answer` are now unanchored substrings, like every other term in the pattern. Over-redacting a `queryMode`-style label is the accepted cost and is pinned in the test. Proof: tests/logger.test.ts "redacts camelCase and snake_case query / answer / question keys" (seven keys passed through before, all `[redacted]` after; `status` and `requestId` stay readable). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
Defect: `ignoreErrors` in src/sentry.server.config.ts used substring matchers
(`/404/`, `/Not Found/i`, `/Cannot find module/i`, bare "NotFoundError",
"NotFound", "BotAccessDenied", "RateLimitedError"). Sentry applies the list
before `beforeSend` as substring / `.test()` matches against the exception
value, `${type}: ${value}` and the event message, so any error whose message
merely contained "404" or "not found" — a Supabase Storage 404 during upload
cleanup, a provider 404 for a retired resource, a future error type containing
the fragment — produced no Sentry event at all.
Trigger: any server exception carrying those words outside the intended
Next.js / framework not-found shapes. Observability gap only; operators saw
Railway logs and nothing else.
Fix: every entry is now a start-anchored regex naming one intended shape —
the Next.js `notFound()` digests (`NEXT_HTTP_ERROR_FALLBACK;404`, and the
legacy `NEXT_NOT_FOUND`, which the old list did not cover), the exact
"Request failed with status code 404" text, `NotFound`/`NotFoundError` as the
error type or "Not Found" as the whole value, "Cannot find module " at the
start, the unchanged `SyntaxError: Unexpected token <`, and this app's own
`BotAccessDenied` / `RateLimitedError` anchored on the type. No string entries
remain. docs/error-tracking.md is owned by another package and is not updated
here.
Proof: tests/sentry-ignore-errors.test.ts reads the live list from the mocked
`Sentry.init` call and evaluates it with `@sentry/core`'s own
`stringMatchesSomePattern` over the SDK's possible-message shapes: eight
intended shapes stay ignored, five genuine errors are no longer dropped
("Object not found" was, before), and every entry is a `^`-anchored RegExp.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…L2, L6) The L2/L6 fixes made src/lib/supabase/client.tsx import two component modules (one static, one dynamic import()), which tests/lib-layering.test.ts forbids: "keeps src/lib independent from UI components" went red. Both clears now go through src/lib/account-scoped-browser-state.ts. It owns the three raw storage keys (favourites last-opened and pinned, the Caring Contacts plan draft), removes them synchronously at every account transition, then dispatches one window event. favourites-storage.ts and plan-draft.ts import their key constants from it (components -> lib, the permitted direction), re-export them so existing importers keep one name, and subscribe to the event at module load to drop their memoised caches and notify their React subscribers. Removing the raw keys in lib, not via a listener registry, is what closes the full-reload hole for L6: after a navigation the wizard module is not loaded, so a listener it would have registered does not exist, but its sessionStorage key still does. The lazy import() and its tick of delay are gone; the clear is synchronous with the transition again. Tests: the account-transitions DOM file gains four cases for the seam (keys removed before any subscriber runs; exactly one event per clear; component caches dropped and subscribers notified through the event alone; neither lib file imports "@/components/"), and the L6 transition cases now assert synchronously and check the wizard subscriber was told. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ot reviewed, not gated) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…h (M4, L2, L6) @supabase/auth-js emits SIGNED_IN for a valid *stored* session while `_recoverAndRefresh` recovers it during boot, and `initialize()` flushes that queued event to every subscriber the moment `initializePromise` settles — before this provider's own `Promise.all([getUser(), getSession()])` round-trip returns. `publishedUserIdRef` is only written after that fetch, so on an ordinary reload of a signed-in tab the handler saw null -> "user-a", called it an account switch and ran `clearAccountScopedBrowserState()`. That destroyed exactly the stores this package added to that helper and whose contract is to survive a refresh: the Caring Contacts plan draft (a patient's name and mobile, held in the browser precisely so a refresh does not lose it), the patient physiology profile, and the favourites pinned / last-opened keys. Because it raced React's registration of the listener, the loss was intermittent rather than deterministic. `initialSessionPublishedRef` records whether `initializeSession` has decided the initial state yet; the handler now clears only once it has. The gate covers the whole helper, not just the three stores this package added — a reload should not wipe the recent-query list, the answer thread or the signed-URL cache either, and on a fresh page load those caches are empty anyway. No leak path opens. Sign-out and session expiry null `publishedUserIdRef` themselves, so a later sign-in as a different user still clears; the two initialize failure branches set the ref too, so a boot that could not be verified keeps the pre-existing conservative behaviour of clearing on the next SIGNED_IN. Three new cases pin all of that: the boot replay while `getUser` is still pending leaves all three stores intact, a different user signing in right after that replay still clears them, and an unverifiable boot followed by a sign-in still clears. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #15613 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
marked this pull request as ready for review
September 4, 2026 16:16
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_fba8f041-d019-4880-9ebb-3419f5f67ac1) |
…lper Main added clearOnCallEntryCache() at each of the three account-transition call sites while this branch was replacing those three sites with one clearAccountScopedBrowserState() helper, so every hunk conflicted. Resolved as a union rather than by picking a side: the helper form is kept (its whole point is that adding a store means adding it to one list, not to three call sites), and main's On Call clear is added to that list, so it still runs on sign-out, on an account switch and on session expiry. Both imports are kept. No clear that either side performed is lost. Verified: tsc --noEmit -p tsconfig.typecheck.json clean; 17 auth/on-call test files, 176 passed (176). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_58a4ea8d-7724-4abc-bd77-8e641a4101d4) |
BigSimmo
enabled auto-merge (squash)
September 4, 2026 18:31
…rts directly check:knip failed on this branch with "Unlisted dependencies (1): @sentry/core tests/sentry-ignore-errors.test.ts", which is why Static PR checks was red — it was red before the main merge too, so it is this package's own failure and not a merge artefact. The L8 test imports stringMatchesSomePattern from @sentry/core so that it pins the ignoreErrors list against Sentry's real matching implementation rather than a reimplementation of it. @sentry/core is a direct dependency of the declared @sentry/nextjs and so is always installed, but it was never declared here, and @sentry/nextjs does not re-export the helper. Declaring it is the honest fix; adding it to a knip ignore list would hide a real unlisted import, and rewriting the test to match patterns itself would drop the property that makes it worth having. Ownership note: package.json and package-lock.json are otherwise owned by the P8a package (PR #2634) in this remediation programme. This is one added line in each, in the alphabetical devDependencies list and its lock entry, disjoint from P8a's overrides/allowScripts/scripts edits. Verified: check:knip clean, check:installed-lock-parity clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…aring it here Reverts the devDependency declaration added a commit ago. Declaring it was the honest fix for the unlisted import, but it turned this PR into a dependency-change PR, which flips CI's dependency audit from advisory to blocking — and that audit is red on main today for two unrelated high advisories (browserslist and fast-uri, both fixed by open Dependabot PRs #2568, #2570 and #2616). A documentation-and-privacy package should not be gated on those. Instead the import is allowed narrowly in knip.json. @sentry/core is a direct dependency of the declared @sentry/nextjs, so it is always installed; the import is in one test that pins the ignoreErrors list against Sentry's real matching implementation. Scoped to that one package name, so every other unlisted import still fails the gate. Remove this entry once @sentry/core is declared properly — that belongs in the P8a package (PR #2634), which owns package.json and package-lock.json and is already a dependency-change PR. Verified: check:knip clean; package.json and package-lock.json byte-identical to the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_36d77ee9-9faa-492b-85bf-21b08f9c10dc) |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_31e81c89-7659-4ada-9407-62fe1968c3d5) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit remediation package P3 — Session and patient-context privacy, from
docs/audit/full-repository-audit-2026-09-02.md(PR #2573). Findings fixed:M4,M10,L2,L6,L1,L8.Signing out did not clear several stores that hold patient-adjacent data in the browser. This clears them at all three account transitions — sign-out, session expiry, and a different user signing in:
M4)L6)L2)Also: the log redaction pattern only matched the bare words
query,answerandquestion, soqueryText,query_text,rawQuery,normalizedQueryandanswer_textpassed through unredacted (L1). The Sentry ignore list used substring matchers, which silently discarded genuine server errors; those are now anchored (L8). And the favourites timestamp parser rejected PostgreSQL's microsecond precision (.550361+00:00), nulling the whole snapshot (M10).The defect the reviewer caught, and how it was fixed
The first implementation cleared on any
SIGNED_INevent whose user id differed from the last published one. But the Supabase auth library replaysSIGNED_INduring boot for a valid stored session, and flushes it to subscribers before thegetUser()round-trip resolves. So on an ordinary page reload of a signed-in tab, the handler sawnull → "user-a"and wiped all three stores — including the Caring Contacts draft, whose entire contract is that a refresh does not lose it. Being a race with React's effect registration, the loss was intermittent, which is worse than deterministic.Reproduced before fixing:
Tests 1 failed | 15 passed (16)—expected { ageYears: null } to match { ageYears: 82 }. The mechanism was confirmed directly in@supabase/auth-js(GoTrueClient.js_recoverAndRefreshemitsSIGNED_IN;initialize()flushes the queued event onceinitializePromisesettles).Fixed with an
initialSessionPublishedRefset whereinitializeSessionpublishes the initial state — including both failure branches, so an unverifiable boot keeps the old conservative clear rather than opening a leak. The clear now runs only when that ref is set and the user id genuinely changed. The explicit sign-out and session-expiry paths still clear and null the ref, so a later sign-in as a different user is covered.A second reviewer round also blocked the first attempt for importing
@/components/...insidesrc/lib/, which the committedtests/lib-layering.test.tsgate refuses. The clears now route through a lib-side seam; that gate is green.Every finding was mutation-tested
Each fix was reverted individually and its tests watched to go red — not merely asserted:
M4— removeclearPatientProfile()→ 5 cases redM10— revert the parser → 1 redL2— remove the lib clear → 3 DOM cases redL6— same mutation → 3 draft cases redL1— revertlogger.ts→ 1 redL8— revertsentry.server.config.ts→ 3 redRAG impact: none — no retrieval, ranking, ordering or selection surface touched.
Verification
npm run verify:pr-local—- completed: check:runtime, check:installed-lock-parity, format:changed, check:diff-integrity, sitemap:check, check:repo-awareness-snapshot, docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:branch-review-ledger, check:outstanding-issues, check:ledger-write-discipline, lint, typecheck, test, build, check:rag:fixtures, check:medication-interactions, check:medication-lexicon-report·- failed: (none)·- not reached: (none)·Test Files 1129 passed | 1 skipped (1130)·Tests 14985 passed | 2 expected fail | 3 skipped (14990)npm run check:diff-integrity—[diff-integrity] PASS — 5 changed test file(s), 15 -> 32 test case(s), against base 51ddfcd83.No test deleted, skipped or weakened; the case count more than doubled.tsc --noEmit -p tsconfig.typecheck.jsonclean;eslint --max-warnings 0exit 0; Prettier clean.Verification not run:
npm run verify:ui— browser proof left to CI; the pinned Chromium is not installed in this container.Verification not run:
npm run verify:release— no release or handoff confidence is claimed.Verification not run: provider-backed gates — nothing here reaches OpenAI, Supabase, Railway or Sentry at run time; all work was offline.
Note:
origin/mainwas merged in (not rebased) after the gate ran, to pick up #2610. The merge was clean with no overlap against this package's files, and the four primary suites were re-run afterwards:Test Files 4 passed (4) / Tests 27 passed (27).Risk and rollout
markSessionExpiredalso wipes an in-progress patient context on a transient refresh failure. That is the conservative direction.SIGNED_OUT(browser back-forward cache or tab discard) and is then reloaded under user B keeps its per-tabsessionStorage— the profile and the plan draft. Closing that needs an owner stamp written into the stores themselves, which is outside this package's owned files and the reviewer's prescribed change. Recommend tracking it as a follow-up.Clinical Governance Preflight
unchanged; no citation or answer path touched.
none introduced; patient-identifiable browser state is narrowed, not expanded — the plan draft holding a patient name and mobile is now cleared at every account boundary.
Clinical KB Database(sjrfecxgysukkwxsowpy)no Supabase env value, migration target or configured project changes.
unchanged; all changes are client-side storage and logging.
unchanged.
unchanged.
reviewed; no decision-support behaviour changes — this is privacy hygiene at the session boundary.
Notes
src/lib/favourites-client-contract.ts, the plan draft insrc/components/caring-contacts/workspace/plan-wizard/plan-draft.ts, and onlysrc/sentry.server.config.tscarries an ignore list.docs/error-tracking.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Generated by Claude Code
Note
Medium Risk
Centralizes identity-bound local state clearing at auth boundaries (privacy-critical) with a subtle boot vs. switch distinction; user-visible loss of in-tab drafts/profile on sign-out/expiry is intentional.
Overview
Audit remediation for session and patient-context privacy: sign-out, session expiry, and user switch now wipe browser state that could leak between clinicians on a shared workstation.
The auth provider funnels those transitions through one
clearAccountScopedBrowserStatepath that clears the prescribing patient profile, answer thread, recent queries, signed URLs, on-call cache, and a new lib seamaccount-scoped-browser-state.tsthat removes favourites pins/last-opened and the Caring Contacts plan draft keys without importing component modules. Component stores subscribe toACCOUNT_TRANSITION_EVENTso in-memory caches refresh in the same tab.A boot-time guard (
initialSessionPublishedRef) stops Supabase’s replayedSIGNED_INduring page load from being treated as an account switch, so refresh-surviving data (draft, profile, favourites) is not cleared intermittently on reload.Separate fixes: log redaction now catches
queryText,answer_text, and similar keys; favourites snapshot parsing accepts Postgres microsecond ISO timestamps; SentryignoreErrorsuses start-anchored regexes so real 404/storage errors are not dropped. Docs and tests were expanded accordingly.Reviewed by Cursor Bugbot for commit 2888b08. Configure here.