Repository navigation
sweepStore's try wraps the whole loop, so one throwing removeItem silently cancels the rest of the session-user purge #5763
Description
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:
function sweepStore(store: Storage | undefined): void {
if (!store) return;
try {
for (const key of Object.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.
Activity
os-zhuang commentedon Aug 23, 2026
Triage:
pm:queue,domain:ui, Bug — same grading logic as #5731, applied as this card anticipates: the remedy restores the #5664 allowlist-sweep invariant (one uncooperative key costs one key, not the rest of the purge) and needs no product decision; reachability-not-demonstrated is acknowledged and does not gate an invariant restoration on the sign-in purge path. Fix shape in the card (narrow the try to the walk, keep the snapshot guarded; partial-failure test of its own). The "should a failed sweep report" question: follow whatever #5731's landed answer is forclear()— read that PR before deciding, and if the answer does not transfer (sign-in path), say so in the PR body rather than silently diverging. Serial note: same file as #5731's in-flight PR (ActiveOrganizationStorage.ts) — land after it or rebase; the ui seat answers fold-or-serial at claim time.Generated by Claude Code
claude commentedon Aug 23, 2026
Claim —
domain:uiexecution seat.session_01EuPCi56cnGyykygi3z9w4mclaude/issue-5763-sweepstore-per-key-tryDispatching to a dev this round (R23). Serial note from triage answered: #5731's PR #5764 has landed (merged 2026-08-23), so
ActiveOrganizationStorage.tsis free — the dev branches from amainthat already contains it and reads its landed answer on the report-or-swallow question rather than serialising behind it.Generated by Claude Code
fix(auth): stop sweepStore aborting the sign-in purge on one throwing…
os-zhuang commentedon Aug 23, 2026
os-dev-report
{ "issue": 5763, "status": "done", "branch": "claude/issue-5763-sweepstore-per-key-try", "pr": "https://github.com/objectstack-ai/objectui/pull/5779", "premise_still_valid": true, "summary": "sweepStore()'s try now wraps only the Object.keys(store) snapshot; each removeItem gets its own try/catch, so one throwing key costs exactly that key instead of aborting the rest of the sign-in allowlist sweep. Unlike ActiveOrganizationStorage.clear() (#5731), the fix does not add read-back-verified quarantine -- sweepStore does not own reads for the keys it walks, so there is nothing to quarantine, and doing so would be the general storage-error-handling refactor the card scopes away from. What is mirrored is the console.warn reporting channel, so a partial sweep is discoverable instead of silent. Found the identical try-wraps-the-whole-loop shape in AuthProvider.tsx's sign-out counterpart (purgeSignedOutClientCaches) while implementing this and filed it separately as #5777 rather than widening this PR.", "tests": "Full commands and pass/fail evidence are in the PR body's Verification table (union run at 8e1409881, clean tree). Summary: `pnpm exec vitest run packages/auth/` -- 24 files, 248 tests passed (was 246 at #5764's merged baseline; +2 new here). `pnpm --filter @object-ui/auth type-check` -- exit 0 (tsc --noEmit and tsc -p tsconfig.test.json). `pnpm --filter @object-ui/auth lint` -- exit 0, 29 warnings/0 errors, same count as #5764's baseline and none in the changed files. `node scripts/check-control-bytes.mjs` / `check-changeset-presence.mjs` / `check-changeset-no-major.mjs` / `check-changeset-fixed.mjs` -- all exit 0. Reverse verification: committed the fix first, then `git checkout origin/main -- packages/auth/src/ActiveOrganizationStorage.ts` and re-ran the new partial-failure test in isolation -- it failed exactly as the card describes (`objectui-favorites`, seeded after the poisoned key, survived because the whole-loop try aborted the walk at the throw). Restored the fix via `git checkout claude/issue-5763-sweepstore-per-key-try -- packages/auth/src/ActiveOrganizationStorage.ts`, confirmed `git status` clean (byte-identical to the committed fix), and re-ran the full package suite green (248 passed). PR's own CI just started (branch pushed ~2 min before this report); reported at draft-PR time per standing rule, CI convergence left to PM/CI.", "open_questions": [], "out_of_scope_findings": [ "filed as #5777: AuthProvider.tsx's purgeSignedOutClientCaches() has the identical try-wraps-the-whole-loop shape on the sign-out path (sessionStorage metadata-seed-cache purge) -- same defect class as #5763, different file/caller, deliberately not fixed here per this card's own scope note" ] }Generated by Claude Code
fix(auth): stop purgeSignedOutClientCaches aborting the sign-out swee…
fix(auth): stop sweepStore aborting the sign-in purge on one throwing…
fix(auth): stop purgeSignedOutClientCaches aborting the sign-out swee…