Repository navigation
Add trusted client reset flow - #161
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new "reset-client" decision to OAuth authorize flow: the frontend detects an "Invalid redirect URI" condition and can request a reset; the backend validates the request, revokes grants for the target client/user, deletes the client record, and returns a JSON success or error response. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend (oauth-authorize)
participant Backend as Backend (oauth-handlers)
participant DB as Database
User->>Frontend: Open authorize page (redirect_uri mismatch)
Frontend->>Frontend: Detect invalidRedirectUriMessage / mismatch
Frontend-->>User: Show "Reset stored connection" UI
User->>Frontend: Click reset button
Frontend->>Backend: POST /authorize with decision='reset-client' and client_id
Backend->>Backend: Validate session & redirect-mismatch eligibility
Backend->>DB: List grants for userId & clientId
Backend->>DB: Revoke matching grants
Backend->>DB: Delete client record
DB-->>Backend: Confirm deletions
Backend->>Backend: Write audit events
Backend-->>Frontend: JSON { ok: true, message? }
Frontend-->>User: Display info / mark resetCompleted
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 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 13, 2026 15:41
Contributor
|
🔎 Preview deployed: https://kody-pr-161.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.
🧹 Nitpick comments (2)
packages/worker/src/oauth-handlers.workers.test.ts (1)
400-404: Consider removing unusedresetReasonform field.The
resetReasonfield is included in the request body, but reviewinghandleResetClientRequestinoauth-handlers.ts, the handler doesn't read this field—it relies solely onerror_descriptionquery param or actual redirect URI mismatch detection viarequestHasRedirectUriMismatch(). The test passes because the redirect URI (https://localhost:8888/callback) doesn't match the registered URI (https://example.com/callback), triggering mismatch detection.This field appears to be vestigial. Consider removing it to avoid confusion about the actual detection mechanism.
♻️ Suggested simplification
body: new URLSearchParams({ decision: 'reset-client', - resetReason: - 'Invalid redirect URI. The redirect URI provided does not match any registered URI for this client.', }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/oauth-handlers.workers.test.ts` around lines 400 - 404, The test includes an unused form field "resetReason" in the POST body; remove that field from the URLSearchParams in oauth-handlers.workers.test.ts so the test relies only on redirect mismatch detection; specifically, edit the test that posts to handleResetClientRequest and delete the resetReason entry (leaving decision: 'reset-client') because handleResetClientRequest and requestHasRedirectUriMismatch() do not read resetReason.packages/worker/client/routes/oauth-authorize.tsx (1)
37-42: Duplicated message constant between frontend and backend.The
invalidRedirectUriMessagestring is duplicated here and inpackages/worker/src/oauth-handlers.ts(line 60-61). If this message ever changes on the backend, the frontend detection will silently break.Consider extracting this to a shared module or at minimum adding a comment noting the dependency.
♻️ Option: Add a clarifying comment
+// NOTE: This message must match the backend constant in oauth-handlers.ts const invalidRedirectUriMessage = 'Invalid redirect URI. The redirect URI provided does not match any registered URI for this client.'🤖 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 37 - 42, The frontend duplicates the backend error string (invalidRedirectUriMessage) and uses isInvalidRedirectUriMessage to detect it, which will break if the backend text changes; extract the constant into a shared module (e.g., a common/constants or oauth/messages export) and import that same symbol into both packages so both sides use the single source of truth, or if extraction isn't feasible immediately, add a clear comment above invalidRedirectUriMessage referencing the backend symbol in oauth-handlers.ts and note the dependency so future changes stay in sync; update isInvalidRedirectUriMessage to compare against the shared import (or retain but annotated) to avoid silent breakage.
🤖 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/client/routes/oauth-authorize.tsx`:
- Around line 37-42: The frontend duplicates the backend error string
(invalidRedirectUriMessage) and uses isInvalidRedirectUriMessage to detect it,
which will break if the backend text changes; extract the constant into a shared
module (e.g., a common/constants or oauth/messages export) and import that same
symbol into both packages so both sides use the single source of truth, or if
extraction isn't feasible immediately, add a clear comment above
invalidRedirectUriMessage referencing the backend symbol in oauth-handlers.ts
and note the dependency so future changes stay in sync; update
isInvalidRedirectUriMessage to compare against the shared import (or retain but
annotated) to avoid silent breakage.
In `@packages/worker/src/oauth-handlers.workers.test.ts`:
- Around line 400-404: The test includes an unused form field "resetReason" in
the POST body; remove that field from the URLSearchParams in
oauth-handlers.workers.test.ts so the test relies only on redirect mismatch
detection; specifically, edit the test that posts to handleResetClientRequest
and delete the resetReason entry (leaving decision: 'reset-client') because
handleResetClientRequest and requestHasRedirectUriMismatch() do not read
resetReason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dbd58a1-cc2e-4114-bfcc-52acd929c686
📒 Files selected for processing (3)
packages/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>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/client/routes/oauth-authorize.tsx (1)
175-179: Minor: Doublehandle.update()call on reset success.When
payload.messageis received,setMessagecallshandle.update(), then thefinallyblock setssubmittingDecision = nulland callshandle.update()again. While functionally correct, you could skip the finally block's update call in this path by clearingsubmittingDecisionbefore returning.🔧 Proposed simplification
if (typeof payload?.message === 'string') { resetCompleted = true + submittingDecision = null setMessage({ type: 'info', text: payload.message }) return }🤖 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 175 - 179, The payload.message branch in the authorize flow sets setMessage({ type: 'info', text: payload.message }) and returns but causes handle.update() to be called twice because the finally block also clears submittingDecision and calls handle.update(); to avoid the double update, clear submittingDecision (set submittingDecision = null) immediately before returning from the branch where typeof payload?.message === 'string' so the finally block can skip re-updating state, leaving handle.update() calls minimal; update the code around the payload.message branch (the setMessage call and return) to null out submittingDecision first.packages/worker/src/oauth-handlers.workers.test.ts (1)
360-415: Consider asserting grant revocation was called.This test verifies client deletion but doesn't assert that grant revocation occurred. Unlike the first test, there's no tracking array for
revokedGrantIds. For completeness, consider adding:🔧 Proposed enhancement
test('reset client also works when the mismatch came from authorize-info loading', async () => { const deletedClientIds = new Array<string>() + const revokedGrantIds = new Array<string>() const userId = await createStableUserIdFromEmail('user@example.com') const helpers = createHelpers({ listUserGrants: async (requestedUserId) => { expect(requestedUserId).toBe(userId) return { items: [ { id: 'grant-1', clientId: 'client-123', userId, scope: ['profile'], metadata: {}, createdAt: 0, }, ], } }, - revokeGrant: async () => undefined, + revokeGrant: async (grantId) => { + revokedGrantIds.push(grantId) + }, deleteClient: async (clientId) => { deletedClientIds.push(clientId) }, }) // ... rest of test ... expect(deletedClientIds).toEqual(['client-123']) + expect(revokedGrantIds).toEqual(['grant-1']) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/oauth-handlers.workers.test.ts` around lines 360 - 415, The test confirms client deletion but doesn't assert that revokeGrant was invoked; update the mock provided to createHelpers to track revoked grant IDs (e.g., add const revokedGrantIds = new Array<string>() and implement revokeGrant: async (grantId) => { revokedGrantIds.push(grantId) }) and then add an expectation after calling handleAuthorizeRequest to assert revokedGrantIds equals ['grant-1']; reference the revokeGrant mock, the createHelpers call, the deletedClientIds array and the handleAuthorizeRequest invocation so the reviewer can locate and verify the added assertion.packages/worker/src/oauth-handlers.ts (1)
224-250: Consider: Grant revocation and client deletion are not atomic.If
helpers.deleteClient(clientId)fails after grants are revoked, the operation partially succeeds. The user would need to retry to delete the client record.This is likely acceptable since:
- Revoked grants are the primary cleanup goal
- A client record without grants is benign
- The error response prompts the user to try again
If stronger consistency is desired, consider reversing the order (delete client first, then revoke grants), though this has its own trade-offs.
🤖 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 224 - 250, The current flow revokes grants (listUserGrantsForClient + helpers.revokeGrant) then calls helpers.deleteClient, which can leave a deleted-grants/remaining-client partial state if delete fails; to make the operation more consistent, reverse the order: call createStableUserIdFromEmail(sessionEmail) then call helpers.deleteClient(clientId) first, then listUserGrantsForClient(userId, clientId) and revoke grants via helpers.revokeGrant; also update the logAuditEvent/action or error handling around helpers.deleteClient to reflect the new sequence and ensure errors are returned if deleteClient fails before revocation.
🤖 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/client/routes/oauth-authorize.tsx`:
- Around line 175-179: The payload.message branch in the authorize flow sets
setMessage({ type: 'info', text: payload.message }) and returns but causes
handle.update() to be called twice because the finally block also clears
submittingDecision and calls handle.update(); to avoid the double update, clear
submittingDecision (set submittingDecision = null) immediately before returning
from the branch where typeof payload?.message === 'string' so the finally block
can skip re-updating state, leaving handle.update() calls minimal; update the
code around the payload.message branch (the setMessage call and return) to null
out submittingDecision first.
In `@packages/worker/src/oauth-handlers.ts`:
- Around line 224-250: The current flow revokes grants (listUserGrantsForClient
+ helpers.revokeGrant) then calls helpers.deleteClient, which can leave a
deleted-grants/remaining-client partial state if delete fails; to make the
operation more consistent, reverse the order: call
createStableUserIdFromEmail(sessionEmail) then call
helpers.deleteClient(clientId) first, then listUserGrantsForClient(userId,
clientId) and revoke grants via helpers.revokeGrant; also update the
logAuditEvent/action or error handling around helpers.deleteClient to reflect
the new sequence and ensure errors are returned if deleteClient fails before
revocation.
In `@packages/worker/src/oauth-handlers.workers.test.ts`:
- Around line 360-415: The test confirms client deletion but doesn't assert that
revokeGrant was invoked; update the mock provided to createHelpers to track
revoked grant IDs (e.g., add const revokedGrantIds = new Array<string>() and
implement revokeGrant: async (grantId) => { revokedGrantIds.push(grantId) }) and
then add an expectation after calling handleAuthorizeRequest to assert
revokedGrantIds equals ['grant-1']; reference the revokeGrant mock, the
createHelpers call, the deletedClientIds array and the handleAuthorizeRequest
invocation so the reviewer can locate and verify the added assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9673df7f-175f-43dc-8a9a-b19d4e1b7807
📒 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 due to trivial changes (1)
- packages/shared/src/oauth-messages.ts
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.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unused import of shared message constant
- Removed the unused
invalidRedirectUriMessageimport fromoauth-handlers.ts.
- Removed the unused
- ✅ Fixed: Unused variable
urlin reset client handler- Deleted the unused
urlvariable inhandleResetClientRequestsince it was never referenced.
- Deleted the unused
Preview (fdc4c3cc80)
diff --git a/packages/shared/src/oauth-messages.ts b/packages/shared/src/oauth-messages.ts
new file mode 100644
--- /dev/null
+++ b/packages/shared/src/oauth-messages.ts
@@ -1,0 +1,2 @@
+export const invalidRedirectUriMessage =
+ 'Invalid redirect URI. The redirect URI provided does not match any registered URI for this client.'
diff --git a/packages/worker/client/routes/oauth-authorize.tsx b/packages/worker/client/routes/oauth-authorize.tsx
--- a/packages/worker/client/routes/oauth-authorize.tsx
+++ b/packages/worker/client/routes/oauth-authorize.tsx
@@ -11,6 +11,7 @@
fieldCss,
fieldLabelCss,
getAlertCardCss,
+ getDangerButtonCss,
getPrimaryButtonCss,
getSecondaryButtonCss,
insetCardCss,
@@ -23,6 +24,7 @@
sectionTitleCss,
stackedPageCss,
} from '#client/styles/style-primitives.ts'
+import { invalidRedirectUriMessage } from '@kody-internal/shared/oauth-messages.ts'
type OAuthAuthorizeInfo = {
client: { id: string; name: string }
@@ -31,7 +33,12 @@
type OAuthAuthorizeStatus = 'idle' | 'loading' | 'ready' | 'error'
type OAuthAuthorizeMessage = { type: 'error' | 'info'; text: string }
+type OAuthAuthorizeDecision = 'approve' | 'deny' | 'reset-client'
+function isInvalidRedirectUriMessage(message: string | null | undefined) {
+ return message === invalidRedirectUriMessage
+}
+
function getSearchParams() {
return typeof window === 'undefined'
? new URLSearchParams()
@@ -42,10 +49,11 @@
let info: OAuthAuthorizeInfo | null = null
let status: OAuthAuthorizeStatus = 'idle'
let message: OAuthAuthorizeMessage | null = null
- let submitting = false
+ let submittingDecision: OAuthAuthorizeDecision | null = null
let lastSearch = ''
let session: SessionInfo | null = null
let sessionStatus: SessionStatus = 'idle'
+ let resetCompleted = false
function setMessage(next: OAuthAuthorizeMessage | null) {
message = next
@@ -116,12 +124,9 @@
handle.update()
}
- async function submitDecision(
- decision: 'approve' | 'deny',
- form?: HTMLFormElement,
- ) {
- if (submitting) return
- submitting = true
+ async function submitDecision(decision: OAuthAuthorizeDecision, form?: HTMLFormElement) {
+ if (submittingDecision) return
+ submittingDecision = decision
handle.update()
try {
@@ -136,7 +141,7 @@
type: 'error',
text: 'Email and password are required.',
})
- submitting = false
+ submittingDecision = null
handle.update()
return
}
@@ -159,7 +164,7 @@
? payload.error
: 'Unable to complete authorization.'
setMessage({ type: 'error', text: errorText })
- submitting = false
+ submittingDecision = null
handle.update()
return
}
@@ -167,6 +172,12 @@
window.location.assign(payload.redirectTo)
return
}
+ if (typeof payload?.message === 'string') {
+ resetCompleted = true
+ submittingDecision = null
+ setMessage({ type: 'info', text: payload.message })
+ return
+ }
setMessage({ type: 'error', text: 'Missing redirect response.' })
} catch {
setMessage({
@@ -174,7 +185,7 @@
text: 'Network error. Please try again.',
})
} finally {
- submitting = false
+ submittingDecision = null
handle.update()
}
}
@@ -204,18 +215,30 @@
const scopes = info?.scopes ?? []
const scopeLabel =
scopes.length > 0 ? scopes.join(', ') : 'No scopes requested.'
+ const queryError = readQueryError()
const sessionEmail = session?.email ?? ''
const isSessionReady = sessionStatus === 'ready'
const isSessionLoading =
sessionStatus === 'loading' || sessionStatus === 'idle'
const isLoggedIn = isSessionReady && Boolean(sessionEmail)
- const actionsDisabled = status !== 'ready' || submitting || isSessionLoading
+ const showResetClientCard =
+ (isInvalidRedirectUriMessage(queryError) ||
+ isInvalidRedirectUriMessage(message?.text)) &&
+ !resetCompleted
+ const actionsDisabled =
+ status !== 'ready' || Boolean(submittingDecision) || isSessionLoading
+ const resetClientDisabled =
+ Boolean(submittingDecision) || isSessionLoading || !isLoggedIn
const formReady = status === 'ready' && !isSessionLoading
- const authorizeLabel = submitting
+ const authorizeLabel = submittingDecision
? 'Submitting...'
: isLoggedIn
? 'Approve connection'
: 'Authorize'
+ const resetClientLabel =
+ submittingDecision === 'reset-client'
+ ? 'Deleting stored client...'
+ : 'Delete stored client'
return (
<section css={pageCss}>
@@ -258,6 +281,30 @@
{message.text}
</p>
) : null}
+ {showResetClientCard ? (
+ <section css={cardCss}>
+ <p css={sectionTitleCss}>Reset stored connection</p>
+ <p css={descriptionCss}>
+ Delete this trusted client's saved registration and grants,
+ then start the connection again from the client to create a
+ fresh record.
+ </p>
+ {isLoggedIn ? (
+ <button
+ type="button"
+ disabled={resetClientDisabled}
+ on={{ click: () => submitDecision('reset-client') }}
+ css={dangerButtonCss}
+ >
+ {resetClientLabel}
+ </button>
+ ) : isSessionReady ? (
+ <p css={descriptionCss}>
+ Sign in first, then delete the stored client record.
+ </p>
+ ) : null}
+ </section>
+ ) : null}
<form
css={{
...cardCss,
@@ -332,3 +379,7 @@
size: 'lg',
weight: 'semibold',
})
+const dangerButtonCss = getDangerButtonCss({
+ size: 'lg',
+ weight: 'semibold',
+})
diff --git a/packages/worker/src/oauth-handlers.ts b/packages/worker/src/oauth-handlers.ts
--- a/packages/worker/src/oauth-handlers.ts
+++ b/packages/worker/src/oauth-handlers.ts
@@ -86,6 +86,75 @@
}
}
+function readClientIdFromAuthorizeRequest(request: Request) {
+ const clientId = new URL(request.url).searchParams.get('client_id')?.trim()
+ return clientId ? clientId : null
+}
+
+function isLoopbackHostname(hostname: string) {
+ return hostname === 'localhost' || hostname === '::1' || hostname.startsWith('127.')
+}
+
+function redirectUriMatchesRegisteredUri(
+ requestUri: string,
+ registeredUris: Array<string>,
+) {
+ return registeredUris.some((registeredUri) => {
+ try {
+ const requestUrl = new URL(requestUri)
+ const registeredUrl = new URL(registeredUri)
+ if (
+ isLoopbackHostname(requestUrl.hostname) &&
+ isLoopbackHostname(registeredUrl.hostname)
+ ) {
+ return (
+ requestUrl.protocol === registeredUrl.protocol &&
+ requestUrl.hostname === registeredUrl.hostname &&
+ requestUrl.pathname === registeredUrl.pathname &&
+ requestUrl.search === registeredUrl.search
+ )
+ }
+ } catch {
+ return false
+ }
+ return requestUri === registeredUri
+ })
+}
+
+async function requestHasRedirectUriMismatch(
+ helpers: OAuthHelpers,
+ request: Request,
+) {
+ const url = new URL(request.url)
+ const clientId = url.searchParams.get('client_id')?.trim()
+ const redirectUri = url.searchParams.get('redirect_uri')?.trim()
+ if (!clientId || !redirectUri) return false
+ const client = await helpers.lookupClient(clientId)
+ if (!client) return false
+ return !redirectUriMatchesRegisteredUri(redirectUri, client.redirectUris)
+}
+
+async function listUserGrantsForClient(
+ helpers: OAuthHelpers,
+ userId: string,
+ clientId: string,
+) {
+ const grants = new Array<{ id: string }>()
+ let cursor: string | undefined
+
+ do {
+ const page = await helpers.listUserGrants(userId, { cursor })
+ for (const grant of page.items) {
+ if (grant.clientId === clientId) {
+ grants.push({ id: grant.id })
+ }
+ }
+ cursor = page.cursor
+ } while (cursor)
+
+ return grants
+}
+
function resolveScopes(requestedScopes: Array<string>) {
if (requestedScopes.length === 0) return oauthScopes
const invalidScopes = requestedScopes.filter(
@@ -99,6 +168,96 @@
return requestedScopes
}
+async function handleResetClientRequest(
+ request: Request,
+ env: Env,
+ helpers: OAuthHelpers,
+ requestIp?: string,
+) {
+ const redirectUriMismatch = await requestHasRedirectUriMismatch(helpers, request)
+ if (!redirectUriMismatch) {
+ return respondAuthorizeError(
+ request,
+ 'Stored client cleanup is only available for redirect URI mismatches.',
+ 400,
+ 'invalid_request',
+ )
+ }
+
+ const clientId = readClientIdFromAuthorizeRequest(request)
+ if (!clientId) {
+ return respondAuthorizeError(
+ request,
+ 'Missing client ID for stored client cleanup.',
+ 400,
+ 'invalid_request',
+ )
+ }
+
+ const { email: sessionEmail, setCookie } = await resolveSessionEmail(request, env)
+ if (!sessionEmail) {
+ void logAuditEvent({
+ category: 'oauth',
+ action: 'reset_client',
+ result: 'failure',
+ ip: requestIp,
+ clientId,
+ reason: 'missing_session',
+ })
+ return respondAuthorizeError(
+ request,
+ 'Sign in before deleting stored client records.',
+ 401,
+ 'unauthorized',
+ )
+ }
+
+ try {
+ const userId = await createStableUserIdFromEmail(sessionEmail)
+ const grants = await listUserGrantsForClient(helpers, userId, clientId)
+ await Promise.all(grants.map((grant) => helpers.revokeGrant(grant.id, userId)))
+ await helpers.deleteClient(clientId)
+ void logAuditEvent({
+ category: 'oauth',
+ action: 'reset_client',
+ result: 'success',
+ email: sessionEmail,
+ ip: requestIp,
+ clientId,
+ })
+ return jsonResponse(
+ {
+ ok: true,
+ message:
+ 'Deleted the stored client records for this connection. Start the connection again from your client to create a fresh trusted client.',
+ },
+ setCookie
+ ? {
+ headers: {
+ 'Set-Cookie': setCookie,
+ },
+ }
+ : undefined,
+ )
+ } catch (error) {
+ void logAuditEvent({
+ category: 'oauth',
+ action: 'reset_client',
+ result: 'failure',
+ email: sessionEmail,
+ ip: requestIp,
+ clientId,
+ reason: error instanceof Error ? error.message : 'unknown_error',
+ })
+ return respondAuthorizeError(
+ request,
+ 'Unable to delete stored client records right now.',
+ 500,
+ 'server_error',
+ )
+ }
+}
+
function createAccessDeniedRedirectUrl(request: AuthRequest) {
if (!request.redirectUri) {
return null
@@ -192,6 +351,12 @@
const requestIp = getRequestIp(request) ?? undefined
const helpers = getOAuthHelpers(env)
+ const formData = await request.formData()
+ const decision = String(formData.get('decision') ?? 'approve')
+ if (decision === 'reset-client') {
+ return handleResetClientRequest(request, env, helpers, requestIp)
+ }
+
const resolution = await resolveAuthRequest(helpers, request)
if ('error' in resolution) {
return respondAuthorizeError(
@@ -201,8 +366,6 @@
}
const { authRequest } = resolution
- const formData = await request.formData()
- const decision = String(formData.get('decision') ?? 'approve')
if (decision === 'deny') {
const redirectTo = createAccessDeniedRedirectUrl(authRequest)
if (!redirectTo) {
diff --git a/packages/worker/src/oauth-handlers.workers.test.ts b/packages/worker/src/oauth-handlers.workers.test.ts
--- a/packages/worker/src/oauth-handlers.workers.test.ts
+++ b/packages/worker/src/oauth-handlers.workers.test.ts
@@ -13,6 +13,7 @@
handleOAuthCallback,
oauthScopes,
} from './oauth-handlers.ts'
+import { createStableUserIdFromEmail } from '#worker/user-id.ts'
const baseAuthRequest: AuthRequest = {
responseType: 'code',
@@ -250,6 +251,173 @@
expect(capturedOptions.scope).toEqual(oauthScopes)
})
+test('reset client deletes matching grants and client registration', async () => {
+ const revokedGrantIds = new Array<string>()
+ const deletedClientIds = new Array<string>()
+ const userId = await createStableUserIdFromEmail('user@example.com')
+ const helpers = createHelpers({
+ parseAuthRequest: async () => {
+ throw new Error(
+ 'Invalid redirect URI. The redirect URI provided does not match any registered URI for this client.',
+ )
+ },
+ listUserGrants: async (requestedUserId) => {
+ expect(requestedUserId).toBe(userId)
+ return {
+ items: [
+ {
+ id: 'grant-1',
+ clientId: 'client-123',
+ userId,
+ scope: ['profile'],
+ metadata: {},
+ createdAt: 0,
+ },
+ {
+ id: 'grant-2',
+ clientId: 'other-client',
+ userId,
+ scope: ['profile'],
+ metadata: {},
+ createdAt: 0,
+ },
+ {
+ id: 'grant-3',
+ clientId: 'client-123',
+ userId,
+ scope: ['email'],
+ metadata: {},
+ createdAt: 0,
+ },
+ ],
+ }
+ },
+ revokeGrant: async (grantId, requestedUserId) => {
+ expect(requestedUserId).toBe(userId)
+ revokedGrantIds.push(grantId)
+ },
+ deleteClient: async (clientId) => {
+ deletedClientIds.push(clientId)
+ },
+ })
+ setAuthSessionSecret(cookieSecret)
+ const cookie = await createAuthCookie(
+ { id: 'session-id', email: 'user@example.com', rememberMe: false },
+ false,
+ )
+
+ const response = await handleAuthorizeRequest(
+ new Request(
+ `https://example.com/oauth/authorize?client_id=client-123&redirect_uri=${encodeURIComponent('https://example.com/invalid')}&error_description=${encodeURIComponent('Invalid redirect URI. The redirect URI provided does not match any registered URI for this client.')}`,
+ {
+ method: 'POST',
+ headers: {
+ Accept: 'application/json',
+ Cookie: cookie,
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
+ body: new URLSearchParams({ decision: 'reset-client' }),
+ },
+ ),
+ createEnv(helpers),
+ )
+
+ expect(response.status).toBe(200)
+ await expect(response.json()).resolves.toEqual({
+ ok: true,
+ message:
+ 'Deleted the stored client records for this connection. Start the connection again from your client to create a fresh trusted client.',
+ })
+ expect(revokedGrantIds).toEqual(['grant-1', 'grant-3'])
+ expect(deletedClientIds).toEqual(['client-123'])
+})
+
+test('reset client is rejected when the request is not a redirect mismatch', async () => {
+ const response = await handleAuthorizeRequest(
+ new Request(
+ `https://example.com/oauth/authorize?client_id=client-123&redirect_uri=${encodeURIComponent('https://example.com/callback')}&error_description=Authorization%20error`,
+ {
+ method: 'POST',
+ headers: {
+ Accept: 'application/json',
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
+ body: new URLSearchParams({ decision: 'reset-client' }),
+ },
+ ),
+ createEnv(createHelpers()),
+ )
+
+ expect(response.status).toBe(400)
+ await expect(response.json()).resolves.toEqual({
+ ok: false,
+ error:
+ 'Stored client cleanup is only available for redirect URI mismatches.',
+ code: 'invalid_request',
+ })
+})
+
+test('reset client also works when the mismatch came from authorize-info loading', async () => {
+ const deletedClientIds = new Array<string>()
+ const revokedGrantIds = new Array<string>()
+ const userId = await createStableUserIdFromEmail('user@example.com')
+ const helpers = createHelpers({
+ listUserGrants: async (requestedUserId) => {
+ expect(requestedUserId).toBe(userId)
+ return {
+ items: [
+ {
+ id: 'grant-1',
+ clientId: 'client-123',
+ userId,
+ scope: ['profile'],
+ metadata: {},
+ createdAt: 0,
+ },
+ ],
+ }
+ },
+ revokeGrant: async (grantId) => {
+ revokedGrantIds.push(grantId)
+ },
+ deleteClient: async (clientId) => {
+ deletedClientIds.push(clientId)
+ },
+ })
+ setAuthSessionSecret(cookieSecret)
+ const cookie = await createAuthCookie(
+ { id: 'session-id', email: 'user@example.com', rememberMe: false },
+ false,
+ )
+
+ const response = await handleAuthorizeRequest(
+ new Request(
+ 'https://example.com/oauth/authorize?client_id=client-123&redirect_uri=https%3A%2F%2Flocalhost%3A8888%2Fcallback',
+ {
+ method: 'POST',
+ headers: {
+ Accept: 'application/json',
+ Cookie: cookie,
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
+ body: new URLSearchParams({
+ decision: 'reset-client',
+ }),
+ },
+ ),
+ createEnv(helpers),
+ )
+
+ expect(response.status).toBe(200)
+ await expect(response.json()).resolves.toEqual({
+ ok: true,
+ message:
+ 'Deleted the stored client records for this connection. Start the connection again from your client to create a fresh trusted client.',
+ })
+ expect(revokedGrantIds).toEqual(['grant-1'])
+ expect(deletedClientIds).toEqual(['client-123'])
+})
+
test('oauth callback page returns SPA shell', async () => {
const response = handleOAuthCallback(
new Request('https://example.com/oauth/callback?code=abc123&state=demo'),You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 471c893. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…/kentcdodds/kody into cursor/trusted-client-reset-9b01 Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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
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
Testing
Summary by CodeRabbit
New Features
Bug Fixes