Uh oh!
There was an error while loading. Please reload this page.
fix(auth): stop purgeSignedOutClientCaches aborting the sign-out sweep on one throwing removeItem - #5816
Merged
Conversation
…p on one throwing removeItem Same defect class as #5763 (sweepStore), different file and caller: the `try` wrapped the whole loop instead of each `removeItem`, so a key that throws on removal aborted the walk and left every metadata-seed-cache key after it unswept. The surviving entries are the org-scoped seed cache #5198 classifies as a cross-principal disclosure risk on shared browsers, so a partial sweep here is sharper than the sign-in sibling. Mirrors #5763's landed shape (main, ff2d547): `try` now guards only the `Object.keys` snapshot; each `removeItem` gets its own `try`; a partial sweep is reported via `console.warn` naming the unswept keys rather than quarantined, because this function does not own reads for the metadata seed cache (MetadataProvider does) the same way `sweepStore` does not own reads for the keys it walks. Fixes#5777 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Aug 23, 2026
os-zhuang
marked this pull request as ready for review
August 23, 2026 13:41
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fixes#5777
The defect
purgeSignedOutClientCaches()inpackages/auth/src/AuthProvider.tsxwrapped its wholeremoveItemloop in onetry. AremoveItemthat threw on keynaborted the walk, soevery still-unvisited
objectui:metadata:*key after it was never swept, and the failurewas swallowed —
signOut()believed the purge had completed. Same defect class as#5763 (
sweepStoreinActiveOrganizationStorage.ts), different file and caller(sign-out, not sign-in) — filed separately per that card's scope note and fixed here.
Per its own docstring and #5198, the entries this purges are the previous principal's
org-scoped, PERMISSION-FILTERED app list — a cross-principal disclosure risk on a shared
browser when they survive, not mere staleness. A partial sweep here is the sharper half
of the pair the triage comment names.
The fix
Mirrors #5763's landed shape on
main(ff2d54717, merged 07:56:17Z — read there perdispatch, not from that PR's discussion):
trynow guards only theObject.keys(sessionStorage)snapshot (the reason a guardexists at all — partitioned iframes, some privacy modes).
removeItemgets its owntry; one uncooperative key now costs exactly that key.console.warnnaming the unswept keys, mirroringsweepStore's reporting channel — not quarantined the wayActiveOrganizationStorage.clear()(ActiveOrganizationStorage.clear()swallows a failedremoveItem, so a cleared org can stay readable — asymmetry noted, reachability NOT demonstrated #5731) quarantines a key.Whether #5763's report-vs-silent reasoning transfers (dispatch's open question)
It does, and the reasoning is the same on both axes
sweepStore's doc comment gives forchoosing "report, don't quarantine" over
clear()'s read-back quarantine:get()to guard.purgeSignedOutClientCachesdoes not own reads for themetadata seed cache —
MetadataProvider(@object-ui/app-shell) does, per thisfunction's own docstring (the prefix is spelled out rather than imported specifically
because ownership sits on that side of the dependency boundary). There is nothing here
for a quarantine to guard, same as
sweepStorewalking "another package's recentscache, a metadata seed" it does not own reads for either.
sweepStore's caller (SessionUserScope.adopt,sign-in) cannot act on a partial-sweep failure and must not throw. The sign-out caller
is even further foreclosed: the session is already ending when
purgeSignedOutClientCaches()runs in
signOut'sfinallyblock, so there is no decision left to make either.So this PR mirrors
sweepStoreexactly rather than diverging: report viaconsole.warn,never throw, no quarantine layer added (which would have been the general
storage-error-handling refactor both cards' scope notes rule out).
Tests
packages/auth/src/__tests__/signOut-client-cache-purge-5198.test.tsx— two new cases,same pattern as #5763's sibling test in
sessionUserChangePurge-5664.test.tsx:sweeps every other metadata key when one removeItem throws (#5777)— a poisonedkey with real metadata keys on both sides of it (insertion order), asserting the
poisoned key survives, every other metadata key is still removed, a non-matching key
is untouched either way,
ActiveOrganizationStorage.clear()still runs, and exactlyone
console.warnnames the poisoned key.reports nothing when every removal sticks (#5777)— control: the warning is ameasurement of an actual failure, not a constant.
Reverse-verification, on a real commit (per dispatch's ⛔ never against uncommitted
edits): committed the fix (
da392fbbd), thengit checkout HEAD~1 -- packages/auth/src/AuthProvider.tsxto restore the pre-fix whole-loop-
tryshape and re-ran the same test file:The new test fails against the pre-fix shape exactly as expected (the other 5 pass
either way — they don't exercise a throwing
removeItem). Restored the fix viagit checkout claude/issue-5777-signout-purge-per-key-try -- packages/auth/src/AuthProvider.tsx,confirmed
git diff HEAD --statempty (working tree matches the commit), and re-ran:Full package:
pnpm exec vitest run packages/auth/ --maxWorkers=2(run from repo root,per AGENTS.md — never
pnpm --filter/package-cwd, which silently collectsapps/consoleinstead) → 24 files / 251 tests passed, onc0b7876ba(this PR's head).pnpm --filter '@object-ui/auth' type-check(tsc --noEmit && tsc -p tsconfig.test.json,after building the dependency closure first) → clean, on
c0b7876ba.node scripts/check-changeset-presence.mjs,check-control-bytes.mjs,check-type-check-coverage.mjs,check-lint-coverage.mjs→ all ✅ onc0b7876ba.eslinton the two changed files → 0 errors, 3 pre-existing warnings unrelated to thisdiff (React effect
setStatewarnings at lines 300/760/767, outside the edited region).Scope
purgeSignedOutClientCaches()and its own test file only, per the card's scope note —not a general refactor of
AuthProvider.tsxor the storage-error-handling module.Generated by Claude Code
Generated by Claude Code