Found while implementing #5731 (ActiveOrganizationStorage.clear() swallowing a failed removal). Same defect class, different function and different callers, so it is filed rather than fixed on that PR. Unassigned.
The shape
packages/auth/src/ActiveOrganizationStorage.ts:
functionsweepStore(store: Storage|undefined): void{if(!store)return;try{for(constkeyofObject.keys(store)){if(DEVICE_SCOPED_KEYS.has(key))continue;store.removeItem(key);}}catch{/* storage unavailable */}}The try wraps the whole loop, not each removal. A removeItem that throws on key n aborts the walk, so keys n+1..end are never swept — and the failure is swallowed, so purgePreviousUserClientState() returns normally and its caller believes the purge completed.
Why it matters
sweepStore is part 3 of the #5664 fix, and its own comment says why that part is the load-bearing one: it is an allowlist sweep precisely so the next un-namespaced key — one nobody has written yet — cannot re-open the cross-user pollution class. A partial sweep is a partial allowlist. Which keys survive depends on Object.keys iteration order, so the residue is arbitrary rather than bounded: the previous user's org id, recents, favourites, or a sessionStorage metadata seed (their permission-filtered app list, which #5198 classifies as a cross-principal disclosure rather than staleness) can all be on the wrong side of the abort.
Both sweepStore call sites are inside purgePreviousUserClientState, which runs on SessionUserScope.adopt — the sign-in path.
Reachability — same honesty as #5731
Not demonstrated. The state needed is Object.keys succeeding while some removeItem throws, which is the same state #5731 could not reach from a browser and for the same reasons. Triage graded #5731 anyway because the remedy restores an invariant and needs no product decision; the identical argument applies here, but that is triage's call and not mine to make.
Worth noting for the grade: a wrapped or proxied localStorage (an extension, a polyfill) is a candidate here in a way it is not everywhere, since this loop calls removeItem many times in a row and only needs one of them to fail.
Possible shape
Move the try inside the loop so one uncooperative key costs one key. Object.keys(store) itself can also throw and needs to stay guarded — that is the reason the outer try exists at all, so it should be kept for the snapshot and narrowed for the walk. Whether a failed sweep should additionally be reported is the same question #5731 answered for clear(), and the answer there may or may not transfer: sweepStore's caller is on the sign-in path.
Scope note
Deliberately not fixed on the #5731 PR. That card is about ActiveOrganizationStorage.clear()'s own removal and its interaction with get()'s read order; this is a different function whose behaviour is pinned by __tests__/sessionUserChangePurge-5664.test.tsx, and changing it means a test of its own for partial-failure behaviour. Keeping the two apart keeps both reviewable.
Blocked-by: none. Related: #5731, #5664, #5198.
Found while implementing #5731 (
ActiveOrganizationStorage.clear()swallowing a failed removal). Same defect class, different function and different callers, so it is filed rather than fixed on that PR. Unassigned.The shape
packages/auth/src/ActiveOrganizationStorage.ts:The
trywraps the whole loop, not each removal. AremoveItemthat throws on key n aborts the walk, so keys n+1..end are never swept — and the failure is swallowed, sopurgePreviousUserClientState()returns normally and its caller believes the purge completed.Why it matters
sweepStoreis part 3 of the #5664 fix, and its own comment says why that part is the load-bearing one: it is an allowlist sweep precisely so the next un-namespaced key — one nobody has written yet — cannot re-open the cross-user pollution class. A partial sweep is a partial allowlist. Which keys survive depends onObject.keysiteration order, so the residue is arbitrary rather than bounded: the previous user's org id, recents, favourites, or asessionStoragemetadata seed (their permission-filtered app list, which #5198 classifies as a cross-principal disclosure rather than staleness) can all be on the wrong side of the abort.Both
sweepStorecall sites are insidepurgePreviousUserClientState, which runs onSessionUserScope.adopt— the sign-in path.Reachability — same honesty as #5731
Not demonstrated. The state needed is
Object.keyssucceeding while someremoveItemthrows, which is the same state #5731 could not reach from a browser and for the same reasons. Triage graded #5731 anyway because the remedy restores an invariant and needs no product decision; the identical argument applies here, but that is triage's call and not mine to make.Worth noting for the grade: a wrapped or proxied
localStorage(an extension, a polyfill) is a candidate here in a way it is not everywhere, since this loop callsremoveItemmany times in a row and only needs one of them to fail.Possible shape
Move the
tryinside the loop so one uncooperative key costs one key.Object.keys(store)itself can also throw and needs to stay guarded — that is the reason the outertryexists at all, so it should be kept for the snapshot and narrowed for the walk. Whether a failed sweep should additionally be reported is the same question #5731 answered forclear(), and the answer there may or may not transfer:sweepStore's caller is on the sign-in path.Scope note
Deliberately not fixed on the #5731 PR. That card is about
ActiveOrganizationStorage.clear()'s own removal and its interaction withget()'s read order; this is a different function whose behaviour is pinned by__tests__/sessionUserChangePurge-5664.test.tsx, and changing it means a test of its own for partial-failure behaviour. Keeping the two apart keeps both reviewable.Blocked-by: none. Related: #5731, #5664, #5198.