Handle OAuth provider and authorize route exceptions - #638
Conversation
π WalkthroughWalkthroughThis PR adds centralized exception handling for OAuth authorization and token flows in the worker. It introduces error-mapping helpers in ChangesOAuth Exception Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker as index.ts fetch
participant OAuthProvider
participant Sentry
participant Handlers as oauth-handlers.ts
Client->>Worker: request /oauth/authorize or /oauth/token
Worker->>OAuthProvider: forward request
OAuthProvider-->>Worker: throws error
Worker->>Worker: check isOAuthOwnedPath / isMalformedOAuthClientException
alt not OAuth-owned
Worker-->>Client: rethrow error
else OAuth-owned
Worker->>Sentry: captureException
Worker->>Handlers: handleAuthorizeRouteException / createOAuthProviderExceptionResponse
Handlers-->>Worker: standardized error Response
Worker-->>Client: JSON/redirect/HTML error response
end
Possibly related PRs
π₯ Pre-merge checks | β 5β Passed checks (5 passed)
β¨ 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 |
|
π Preview deployed: https://kody-pr-638.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 1
π§Ή Nitpick comments (2)
packages/worker/src/index.ts (2)
407-416: π Maintainability & Code Quality | π΅ Trivial | β‘ Quick winExtract the OAuth discovery path into a shared constant
'/.well-known/oauth-authorization-server'is hardcoded here while the adjacent paths come from shared constants. Moving it into the same shared source would avoid drift if this path changes.π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/index.ts` around lines 407 - 416, The OAuth discovery route is hardcoded inside isOAuthProviderOwnedPath while the neighboring checks already use shared constants. Move '/.well-known/oauth-authorization-server' into the same shared path source as oauthPaths and protectedResourceMetadataPath, then update isOAuthProviderOwnedPath to reference that constant so all OAuth-owned paths stay centralized and consistent.
418-424: π Maintainability & Code Quality | π΅ Trivial | π€ Low valueDocument this version-specific OAuth workaround
isMalformedOAuthClientExceptionrelies on a rawTypeErrormessage from@cloudflare/workers-oauth-provider@0.4.0, so an upstream fix or wording change will silently change token failures back toserver_error. Add a short comment/link to the upstream bug here so the coupling is explicit.π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/index.ts` around lines 418 - 424, Document the version-specific OAuth workaround in isMalformedOAuthClientException by adding a short comment and upstream bug reference near the message check so the dependency on `@cloudflare/workers-oauth-provider`@0.4.0 is explicit. Keep the existing pathname/oauthPaths.token and TypeError message guard, but annotate that this raw error text is intentional and tied to the upstream issue so future wording changes are easy to spot.
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/index.ts`:
- Around line 513-521: The try/catch in the worker request handler still
suppresses Sentry for the recognized malformed-client path, which removes the
telemetry this PR is meant to restore. Update the catch block in the request
flow around oauthProvider.fetch so that Sentry.captureException is still called
for isMalformedOAuthClientException cases, while keeping
createOAuthProviderExceptionResponse(error, url.pathname) unchanged for the
client response; only preserve the rethrow for non-owned paths.
---
Nitpick comments:
In `@packages/worker/src/index.ts`:
- Around line 407-416: The OAuth discovery route is hardcoded inside
isOAuthProviderOwnedPath while the neighboring checks already use shared
constants. Move '/.well-known/oauth-authorization-server' into the same shared
path source as oauthPaths and protectedResourceMetadataPath, then update
isOAuthProviderOwnedPath to reference that constant so all OAuth-owned paths
stay centralized and consistent.
- Around line 418-424: Document the version-specific OAuth workaround in
isMalformedOAuthClientException by adding a short comment and upstream bug
reference near the message check so the dependency on
`@cloudflare/workers-oauth-provider`@0.4.0 is explicit. Keep the existing
pathname/oauthPaths.token and TypeError message guard, but annotate that this
raw error text is intentional and tied to the upstream issue so future wording
changes are easy to spot.
πͺ 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 Plus
Run ID: a3cd90b0-a8a6-4878-b931-c94d2ef2014e
π Files selected for processing (3)
packages/worker/src/index.tspackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.ts
Summary
/oauth/token,/oauth/register, discovery/API routes) so provider exceptions return OAuth JSON errors instead of Worker 1101s./oauth/authorizeand/oauth/authorize-infoboundary so Kody captures route exceptions and returns recoverable OAuth responses.Research notes
/oauth/registerrequests followed by three/oauth/authorize500s at 03:54-03:55 UTC.isValidRedirectUri(..., clientInfo.redirectUris)can still throw on malformed client records in provider-owned token paths.Testing
npx vitest run --project workers-unit packages/worker/src/oauth-handlers.workers.test.ts -t "delegated authorize|provider-owned|dynamic registration|Claude-shaped"npx vitest run --project workers-unit packages/worker/src/oauth-handlers.workers.test.tsnpm run test -- packages/worker/src/oauth-handlers.workers.test.tsnpm run format:checknpm run lintnpm run typechecknpm run validatenpm run test:pushValidate,Deploy Preview Resources,CodeRabbit, andCursor BugbotSystem recap β extends existing primitives (medium risk)
Mode: recap Β· Base:
main@82c720b2Β· Head:386c3f4cClassification: extends β changes
mcp-oautherror handling for provider-owned and delegated authorize routes.Primitives touched
mcp-oauthapp-uimcp-server/mcpresource audienceSystem map
Change flow
Before / after
Summary by CodeRabbit
New Features
/.well-known/oauth-authorization-server.Bug Fixes
Tests