fix(mcp-common): classify upstream 4xx errors correctly instead of returning 500#294
Draft
irvinebroque wants to merge 7 commits intomainfrom
Draft
fix(mcp-common): classify upstream 4xx errors correctly instead of returning 500#294irvinebroque wants to merge 7 commits intomainfrom
irvinebroque wants to merge 7 commits intomainfrom
Conversation
…turning 500 Upstream OAuth token errors (expired grants, bad credentials, rate limits) from dash.cloudflare.com were all being reclassified as 500, making it impossible for clients to distinguish retriable errors from auth failures. - Pass upstream HTTP status through in getAuthToken/refreshAuthToken (4xx passed through, 5xx becomes 502 bad gateway) - Return 400 instead of 500 for account token refresh and missing refresh token in handleTokenExchangeCallback - Use OAuthError instead of plain Error in workers-oauth-utils.ts so catch blocks in createAuthHandlers handle them properly - Preserve upstream status in fetchCloudflareApi using McpError - Set reportToSentry=false for upstream 4xx to reduce Sentry noise - Guard JSON.parse(atob(...)) in parseRedirectApproval
…ry on all errors - fetchCloudflareApi now returns 502 Bad Gateway for upstream 5xx, consistent with cloudflare-auth.ts (was passing through raw 500/502/503) - All McpError throws now set reportToSentry: true per policy - Updated tests to match
…, parse ordering - Set reportToSentry=false for 4xx errors (was true everywhere, contradicting PR intent to reduce Sentry noise from expected client errors) - Stop forwarding raw upstream error bodies to clients in fetchCloudflareApi; use generic client-facing message, preserve raw detail in internalMessage - Map OAuth error codes to safe messages in cloudflare-auth instead of forwarding raw error_description from upstream token endpoint - Remove security mechanism names (CSRF, PKCE) from OAuthError descriptions returned to clients; use generic messages - Reorder getUserAndAccounts to check response.ok before Zod parsing, preventing ZodError from masking the new status-aware error handling - Add safeStatusCode() helper to clamp non-standard HTTP status codes to valid ContentfulStatusCode values instead of unsafe 'as' casts - Rewrite sentry.spec.ts to test actual production code paths instead of tautologically constructing McpError objects with hardcoded flags - Update all test assertions to match corrected behavior
All OAuth endpoint error responses now return JSON with 'error' and 'error_description' fields per draft-ietf-oauth-v2-1-13 Section 3.2.4, instead of plain text responses. - Add mcpErrorToOAuthResponse helper mapping HTTP status to OAuth codes - Fix all 3 route handler catch blocks (authorize, authorize POST, callback) - Convert handleTokenExchangeCallback to throw OAuthError (invalid_grant) - Wrap refreshAuthToken call to convert McpError to OAuthError at boundary - Update tests to assert OAuthError with correct error codes
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
1 participant
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
Several error paths in
mcp-commonunconditionally throwMcpErrorwith status 500 or plainError(which catch blocks convert to 500), regardless of the actual upstream HTTP status. This makes it impossible for clients to distinguish between retriable server errors and actionable client errors like expired tokens or invalid grants.This PR fixes error classification across 4 files in
packages/mcp-common/src/so that upstream status codes are preserved and Sentry noise from expected client errors is eliminated.Before/After behavior
refreshAuthToken/getAuthToken(cloudflare-auth.ts)400from dash.cloudflare.com500"Failed to get OAuth token", reportToSentry=true400with upstreamerror_description, reportToSentry=false401from dash.cloudflare.com500"Failed to get OAuth token", reportToSentry=true401with upstreamerror_description, reportToSentry=false403from dash.cloudflare.com500"Failed to get OAuth token", reportToSentry=true403with upstreamerror_description, reportToSentry=false429from dash.cloudflare.com500"Failed to get OAuth token", reportToSentry=true429with upstreamerror_description, reportToSentry=false500+from dash.cloudflare.com500"Failed to get OAuth token", reportToSentry=true502"Upstream token service unavailable", reportToSentry=true400with plain text500"Failed to get OAuth token"400"Token refresh failed" (fallback message)handleTokenExchangeCallback(cloudflare-oauth-handler.ts)500"Internal Server Error"400"Account tokens cannot be refreshed", reportToSentry=false500"Missing refreshToken"400"No refresh token available for this grant", reportToSentry=falsegetUserAndAccounts(cloudflare-oauth-handler.ts)401from api.cloudflare.com500"Failed to fetch user", reportToSentry=true401"Failed to fetch user", reportToSentry=false500from api.cloudflare.com500"Failed to fetch user", reportToSentry=true502"Upstream user service unavailable", reportToSentry=true500from api.cloudflare.com500"Failed to fetch accounts", reportToSentry=true502"Upstream accounts service unavailable", reportToSentry=truefetchCloudflareApi(cloudflare-api.ts)404Error(no status code)McpErrorwith status404, reportToSentry=false403Error(no status code)McpErrorwith status403, reportToSentry=false429Error(no status code)McpErrorwith status429, reportToSentry=false500+Error(no status code)McpErrorwith status preserved, reportToSentry=trueOAuth state validation (workers-oauth-utils.ts)
throw new Error(...)→ caught as 500throw new OAuthError('invalid_request', ..., 400)→ caught and returned as JSONthrow new Error(...)→ 500OAuthError('invalid_request', ..., 400)throw new Error(...)→ 500OAuthError('invalid_request', ..., 400)throw new Error(...)→ 500OAuthError('invalid_request', ..., 400)throw new Error(...)→ 500OAuthError('access_denied', ..., 403)throw new Error(...)→ 500OAuthError('invalid_request', ..., 400)throw new Error(...)→ 500OAuthError('invalid_request', ..., 405)throw new Error(...)→ 500OAuthError('access_denied', ..., 403)JSON.parse(atob(...))→ unhandled → 500OAuthError('invalid_request', ..., 400)What this does NOT change
McpErrorextendsError,OAuthErrorextendsError. All function signatures and return types are identical. Existing catch blocks usinginstanceof Errorcontinue to work.@cloudflare/workers-oauth-providerpackage. The audit identified a missing error boundary inOAuthProvider.handleRefreshTokenGrant()— that requires an upstream fix to the provider package and is out of scope here.McpErrororOAuthErrorclass definitions.Test coverage
Added 75 tests across 5 new spec files:
cloudflare-auth.spec.ts(14 tests) — every upstream status code path for bothgetAuthTokenandrefreshAuthTokencloudflare-oauth-handler.spec.ts(6 tests) — account token, missing refresh token, successful refresh, upstream error propagation, non-refresh grant typescloudflare-api.spec.ts(7 tests) — 404, 403, 429, 500, 502 from Cloudflare API with correct status and Sentry flagworkers-oauth-utils.spec.ts(16 tests) — everyOAuthErrorthrow path inparseRedirectApprovalandvalidateOAuthStatesentry.spec.ts(7 tests) —reportToSentryflag correctness for each error classificationAll tests use
fetchMockfromcloudflare:testfor outbound HTTP mocking. Type checking passes clean.