Handle stale OAuth client reset recovery - #169
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds a client-ID-mismatch reset flow: new message constant, cookie-based verification for reset eligibility, client UI gating for reset and post-reset behavior, server handlers to set/validate the verification cookie and perform guarded reset, and tests covering the new path. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Client as OAuth Authorize Route (client)
participant Server as OAuth Worker (server)
participant Store as Grants/Client Store
Browser->>Client: GET /oauth/authorize (may include error_description)
Client->>Client: readQueryError(), increment activeInfoRequestId
Client->>Server: GET /oauth/authorize-info
Server->>Server: parse request, detect invalidClientIdMismatchMessage
alt mismatch detected
Server->>Browser: 400 { ok:false, error, allowClientReset:true } + Set-Cookie (verification)
Client->>Browser: show "Reset stored connection" UI
Browser->>Server: POST /oauth/reset-client?decision=reset-client (with auth cookie + verification cookie)
Server->>Server: validate verification cookie vs client_id
Server->>Store: list grants, revoke grants, delete stored client
Server-->>Browser: 200 { ok:true, message }
else not mismatch
Server->>Browser: normal authorize-info response
Client->>Browser: show authorize form or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 14, 2026 14:38
Contributor
|
🔎 Preview deployed: https://kody-pr-169.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/client/routes/oauth-authorize.tsx (1)
203-207:⚠️ Potential issue | 🟠 MajorReset
resetCompletedwhen the authorize query changes.This flag is set after a successful reset, but Lines 203-207 keep the same route instance alive across
window.location.searchchanges. A later authorize attempt in the same tab will keepshowAuthorizeFormfalse at Line 227 and hide the approve/deny actions until the page is hard-refreshed.💡 Minimal fix
if (currentSearch !== lastSearch) { lastSearch = currentSearch + resetCompleted = false void loadInfo() }Also applies to: 223-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/oauth-authorize.tsx` around lines 203 - 207, The component keeps the same instance across window.location.search changes (tracked by lastSearch) but doesn't clear the resetCompleted flag, causing showAuthorizeForm to remain false on subsequent authorize queries; update the block that detects search changes (the lastSearch / loadInfo area) to also set resetCompleted = false whenever currentSearch !== lastSearch (and likewise where similar search-change handling exists around lines 223-227), so a new authorize flow will display the authorize form and actions again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/oauth-handlers.ts`:
- Around line 193-196: The current logic uses the raw query error_description
(via readAuthorizeErrorDescription) to decide canResetStoredClient (through
canResetStoredClientForMessage), which is untrusted; replace this by checking
only trusted server-side signals (e.g., redirectUriMismatch OR a validated
provider-reported flag stored in server session/state or a provider-signed
assertion) and stop using queryErrorDescription for reset gating. Concretely,
remove or ignore canResetStoredClientForMessage(queryErrorDescription) when
computing canResetStoredClient and instead consult a trusted boolean (for
example providerReportedStaleClient or session.providerMismatchVerified)
populated after verifying provider responses or server-side callbacks in the
OAuth flow before allowing stored-client reset in the resetStoredClient/reset
path referenced by the reset logic.
---
Outside diff comments:
In `@packages/worker/client/routes/oauth-authorize.tsx`:
- Around line 203-207: The component keeps the same instance across
window.location.search changes (tracked by lastSearch) but doesn't clear the
resetCompleted flag, causing showAuthorizeForm to remain false on subsequent
authorize queries; update the block that detects search changes (the lastSearch
/ loadInfo area) to also set resetCompleted = false whenever currentSearch !==
lastSearch (and likewise where similar search-change handling exists around
lines 223-227), so a new authorize flow will display the authorize form and
actions again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3b62bcd-a856-4c4e-9c38-0168e5cf6492
📒 Files selected for processing (4)
packages/shared/src/oauth-messages.tspackages/worker/client/routes/oauth-authorize.tsxpackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Server trusts client-controlled query param for authorization
- Resetting stored clients now requires a server-confirmed redirect URI mismatch instead of trusting the query error description.
You can send follow-ups to the cloud agent here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit b3cb57d. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/oauth-handlers.ts (1)
116-122: Potential false positive from substring match.Using
includes()to check for the cookie could match unintended cookie names. For example, a cookie namedother_kody_oauth_client_reset=would trigger a false positive.Consider using a more precise check:
🔧 Suggested fix with regex boundary
function requestHasOAuthClientResetVerificationCookie(request: Request) { const cookieHeader = request.headers.get('Cookie') + if (!cookieHeader) return false + const pattern = new RegExp( + `(?:^|;\\s*)${oauthClientResetVerificationCookieName}=`, + ) - return ( - cookieHeader?.includes(`${oauthClientResetVerificationCookieName}=`) ?? - false - ) + return pattern.test(cookieHeader) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/oauth-handlers.ts` around lines 116 - 122, The current requestHasOAuthClientResetVerificationCookie uses a substring includes() check which can false-positive match cookie names that contain the target as a substring; update the function to parse the Cookie header properly and check cookie name equality instead of substring matching — e.g., split request.headers.get('Cookie') on ';', trim each pair, and verify any pair starts with `${oauthClientResetVerificationCookieName}=` (or use a strict regex with cookie name boundaries) so only an exact cookie name match (reference: requestHasOAuthClientResetVerificationCookie and oauthClientResetVerificationCookieName) triggers true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/oauth-handlers.ts`:
- Around line 116-122: The current requestHasOAuthClientResetVerificationCookie
uses a substring includes() check which can false-positive match cookie names
that contain the target as a substring; update the function to parse the Cookie
header properly and check cookie name equality instead of substring matching —
e.g., split request.headers.get('Cookie') on ';', trim each pair, and verify any
pair starts with `${oauthClientResetVerificationCookieName}=` (or use a strict
regex with cookie name boundaries) so only an exact cookie name match
(reference: requestHasOAuthClientResetVerificationCookie and
oauthClientResetVerificationCookieName) triggers true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c7cb903-388f-42c8-b463-0d50fa672f00
📒 Files selected for processing (4)
packages/shared/src/oauth-messages.tspackages/worker/client/routes/oauth-authorize.tsxpackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/oauth-messages.ts
- packages/worker/src/oauth-handlers.workers.test.ts
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
/oauthso browsers send it to both/oauth/authorizeand/oauth/authorize-info, allowing cleanup logic to actually runinvalidRedirectUriMessageexport after confirming it no longer has any callersWalkthrough
Testing
npx vitest run --project workers-unit packages/worker/src/oauth-handlers.workers.test.tsnpm run typechecknpm run buildcurlcookie-jar verification proving the signed reset cookie is issued withPath=/oauthand sent back to/oauth/authorize-infoSummary by CodeRabbit
New Features
Tests