improvement(mship): sweep spent OAuth chat attempts out of localStorage - #6467
improvement(mship): sweep spent OAuth chat attempts out of localStorage#6467waleedlatif1 wants to merge 3 commits into
Conversation
Attempt records were written on every chat OAuth connect and never removed, so they accumulated in the store for the life of the browser profile. Reads already refuse an attempt past the 15m cutoff, so the residue was inert — but unbounded. Collect them on create instead: resolved records go at the read cutoff, and still-pending ones only after a 24h abandoned grace. The grace matters — readOAuthChatAttempt applies no age gate, so a consent window parked past 15m can still publish a verdict, and sweeping on age alone (what the common OIDC clients do) would drop that record and strand the row on 'pending'.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Retention (24h) is intentionally longer than the existing 15m lookup cutoff ( Pruning runs in Reviewed by Cursor Bugbot for commit 1652f4e. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThe PR adds bounded retention for OAuth chat-attempt records while preserving pending and recently resolved flows.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/credentials/oauth-chat-attempt.ts | Adds guarded localStorage pruning and resolution timestamps while retaining active OAuth attempt behavior. |
| apps/sim/lib/credentials/oauth-chat-attempt.test.ts | Adds comprehensive sweep coverage, including an adjacent-record case that resolves the prior review finding. |
Reviews (3): Last reviewed commit: "fix(mship): retain attempt records for a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… was requested Review round 1 findings. A pending attempt held by the 24h grace is by definition older than the 15m cutoff, so the moment a late verdict landed on it the record became instantly sweepable and the next create erased it. Every chip re-reads its row on the event create dispatches, so a connected chip dropped straight back to unset -- the grace period was defeating its own purpose. Resolved records now age from resolvedAt, giving the UI the full window to observe a verdict however late it arrives. Legacy records without the field fall back to requestedAt. Also makes the multi-record sweep test a real guard: its records are written directly so they occupy consecutive storage slots. The earlier version interleaved latest-pointers between them, which masked the index shift -- a remove-during-scan regression passed it.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d5a22f4. Configure here.
The 15m resolved cutoff was still short enough to pull a record out from under a mounted row. A chip recomputes itself from storage on every attempt event, and on the reconnect path connectedFromWorkspaceChange is forced false, so connected collapses to exactly the stored status. Any connect click 15m after a reconnect swept that record -- via the CustomEvent in the same tab or the storage event these removals now fire in others -- and reverted the row from Connected. Non-reconnect rows kept the label but silently lost their lock. OAUTH_CHAT_ATTEMPT_MAX_AGE_MS governs what a lookup honours, not how long a record has readers. Retention is now one flat window measured from last activity, which also drops the status branch.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1652f4e. Configure here.
1 participant
Summary
localStorageon every click and never removed one, so they accumulated for the life of the browser profile. Reads already refuse an attempt past the 15m cutoff, so the residue was inert — just unbounded.readOAuthChatAttemptapplies no age gate, so a consent window parked past 15m can still publish a verdict throughsetOAuthChatAttemptStatus. Sweeping on age alone — whatoidc-client-tsclearStaleStatedoes — would drop that record and strand the chip on "pending" with no event to correct it. There is a test that fails under the age-only version.localStorageis index-addressed, so removing mid-scan shifts every later entry down a slot and skips it.Context: CodeQL flagged this file for
js/clear-text-storage-of-sensitive-data(alert #471). That was a false positive and is dismissed — the write stores a random correlation nonce, and the cited flows carry credential row IDs, not tokens (GET /api/credentialswhitelists columns, never joinsaccount, omitsencryptedServiceAccountKey). This PR does not change the flagged line; it fixes the real storage-hygiene gap found while checking the alert.Type of Change
Testing
bun run type-checkclean.lib/credentialssuite 310/310 pass, including 4 new sweep tests. Verified each new test fails without the change it guards — the sweep tests against a no-op prune, and the pending-guard test against an age-only sweep.Checklist