Skip to content

Commit c1d9f85

Browse files
irvinebroqueirvinebroque
committed
fix(mcp-common): harden error handling — reportToSentry, info leakage, 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
1 parent b39f765 commit c1d9f85

10 files changed

+271
-115
lines changed

‎packages/mcp-common/src/cloudflare-api.spec.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ describe('fetchCloudflareApi', () => {
5151
expect(e).toBeInstanceOf(McpError)
5252
const err = e as McpError
5353
expect(err.code).toBe(404)
54-
expect(err.reportToSentry).toBe(true)
55-
expect(err.message).toContain('Cloudflare API request failed')
54+
expect(err.reportToSentry).toBe(false)
55+
expect(err.message).toBe('Cloudflare API request failed')
56+
expect(err.internalMessage).toContain('Script not found')
5657
}
5758
})
5859

@@ -72,7 +73,7 @@ describe('fetchCloudflareApi', () => {
7273
expect(e).toBeInstanceOf(McpError)
7374
const err = e as McpError
7475
expect(err.code).toBe(403)
75-
expect(err.reportToSentry).toBe(true)
76+
expect(err.reportToSentry).toBe(false)
7677
}
7778
})
7879

@@ -92,7 +93,7 @@ describe('fetchCloudflareApi', () => {
9293
expect(e).toBeInstanceOf(McpError)
9394
const err = e as McpError
9495
expect(err.code).toBe(429)
95-
expect(err.reportToSentry).toBe(true)
96+
expect(err.reportToSentry).toBe(false)
9697
}
9798
})
9899

@@ -140,7 +141,7 @@ describe('fetchCloudflareApi', () => {
140141
}
141142
})
142143

143-
it('preserves error text in the McpError message', async () => {
144+
it('preserves error text in internalMessage (not user-facing message)', async () => {
144145
const errorBody = '{"errors":[{"message":"Worker not found","code":10007}]}'
145146
fetchMock
146147
.get('https://api.cloudflare.com')
@@ -156,7 +157,8 @@ describe('fetchCloudflareApi', () => {
156157
} catch (e) {
157158
expect(e).toBeInstanceOf(McpError)
158159
const err = e as McpError
159-
expect(err.message).toContain('Worker not found')
160+
expect(err.message).toBe('Cloudflare API request failed')
161+
expect(err.internalMessage).toContain('Worker not found')
160162
}
161163
})
162164
})

‎packages/mcp-common/src/cloudflare-api.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
import { Cloudflare } from 'cloudflare'
22
import { env } from 'cloudflare:workers'
33

4-
import { McpError } from './mcp-error'
4+
import { McpError, safeStatusCode } from './mcp-error'
55

6-
import type { ContentfulStatusCode } from 'hono/utils/http-status'
76
import type { z } from 'zod'
87

98
export function getCloudflareClient(apiToken: string) {
@@ -62,10 +61,10 @@ export async function fetchCloudflareApi<T>({
6261
const is5xx = response.status >= 500 && response.status <= 599
6362

6463
throw new McpError(
65-
is5xx ? 'Upstream Cloudflare API unavailable' : `Cloudflare API request failed: ${error}`,
66-
(is5xx ? 502 : response.status) as ContentfulStatusCode,
64+
is5xx ? 'Upstream Cloudflare API unavailable' : 'Cloudflare API request failed',
65+
safeStatusCode(is5xx ? 502 : response.status),
6766
{
68-
reportToSentry: true,
67+
reportToSentry: is5xx,
6968
internalMessage: `Cloudflare API ${response.status}: ${error}`,
7069
}
7170
)

‎packages/mcp-common/src/cloudflare-auth.spec.ts‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,10 @@ describe('getAuthToken', () => {
6767
expect(e).toBeInstanceOf(McpError)
6868
const err = e as McpError
6969
expect(err.code).toBe(400)
70-
expect(err.message).toBe('The authorization code has expired')
71-
expect(err.reportToSentry).toBe(true)
70+
expect(err.message).toBe('Authorization grant is invalid, expired, or revoked')
71+
expect(err.reportToSentry).toBe(false)
7272
expect(err.internalMessage).toContain('Upstream 400')
73+
expect(err.internalMessage).toContain('The authorization code has expired')
7374
}
7475
})
7576

@@ -92,8 +93,8 @@ describe('getAuthToken', () => {
9293
expect(e).toBeInstanceOf(McpError)
9394
const err = e as McpError
9495
expect(err.code).toBe(401)
95-
expect(err.message).toBe('Invalid client credentials')
96-
expect(err.reportToSentry).toBe(true)
96+
expect(err.message).toBe('Client authentication failed')
97+
expect(err.reportToSentry).toBe(false)
9798
}
9899
})
99100

@@ -116,7 +117,8 @@ describe('getAuthToken', () => {
116117
expect(e).toBeInstanceOf(McpError)
117118
const err = e as McpError
118119
expect(err.code).toBe(403)
119-
expect(err.reportToSentry).toBe(true)
120+
expect(err.message).toBe('Access denied')
121+
expect(err.reportToSentry).toBe(false)
120122
}
121123
})
122124

@@ -139,7 +141,7 @@ describe('getAuthToken', () => {
139141
expect(e).toBeInstanceOf(McpError)
140142
const err = e as McpError
141143
expect(err.code).toBe(429)
142-
expect(err.reportToSentry).toBe(true)
144+
expect(err.reportToSentry).toBe(false)
143145
}
144146
})
145147

@@ -193,7 +195,7 @@ describe('getAuthToken', () => {
193195
const err = e as McpError
194196
expect(err.code).toBe(400)
195197
expect(err.message).toBe('Token exchange failed')
196-
expect(err.reportToSentry).toBe(true)
198+
expect(err.reportToSentry).toBe(false)
197199
}
198200
})
199201
})
@@ -235,9 +237,10 @@ describe('refreshAuthToken', () => {
235237
expect(e).toBeInstanceOf(McpError)
236238
const err = e as McpError
237239
expect(err.code).toBe(400)
238-
expect(err.message).toBe('The refresh token has expired')
239-
expect(err.reportToSentry).toBe(true)
240+
expect(err.message).toBe('Authorization grant is invalid, expired, or revoked')
241+
expect(err.reportToSentry).toBe(false)
240242
expect(err.internalMessage).toContain('Upstream 400')
243+
expect(err.internalMessage).toContain('The refresh token has expired')
241244
}
242245
})
243246

@@ -260,7 +263,7 @@ describe('refreshAuthToken', () => {
260263
expect(e).toBeInstanceOf(McpError)
261264
const err = e as McpError
262265
expect(err.code).toBe(401)
263-
expect(err.reportToSentry).toBe(true)
266+
expect(err.reportToSentry).toBe(false)
264267
}
265268
})
266269

@@ -282,11 +285,11 @@ describe('refreshAuthToken', () => {
282285
}
283286
})
284287

285-
it('uses fallback message when upstream error has no error_description', async () => {
288+
it('uses fallback message when upstream error code is unknown', async () => {
286289
fetchMock
287290
.get('https://dash.cloudflare.com')
288291
.intercept({ path: '/oauth2/token', method: 'POST' })
289-
.reply(400, JSON.stringify({ error: 'invalid_grant' }))
292+
.reply(400, JSON.stringify({ error: 'some_unknown_error' }))
290293

291294
try {
292295
await refreshAuthToken(baseParams)
@@ -296,6 +299,31 @@ describe('refreshAuthToken', () => {
296299
const err = e as McpError
297300
expect(err.code).toBe(400)
298301
expect(err.message).toBe('Token refresh failed')
302+
expect(err.reportToSentry).toBe(false)
303+
}
304+
})
305+
306+
it('maps known error codes to safe messages instead of forwarding error_description', async () => {
307+
fetchMock
308+
.get('https://dash.cloudflare.com')
309+
.intercept({ path: '/oauth2/token', method: 'POST' })
310+
.reply(
311+
400,
312+
JSON.stringify({
313+
error: 'invalid_grant',
314+
error_description: 'Internal: token xyz expired at 2024-01-01',
315+
})
316+
)
317+
318+
try {
319+
await refreshAuthToken(baseParams)
320+
expect.unreachable()
321+
} catch (e) {
322+
expect(e).toBeInstanceOf(McpError)
323+
const err = e as McpError
324+
expect(err.message).toBe('Authorization grant is invalid, expired, or revoked')
325+
// Raw upstream detail preserved in internalMessage only
326+
expect(err.internalMessage).toContain('Internal: token xyz expired')
299327
}
300328
})
301329
})

‎packages/mcp-common/src/cloudflare-auth.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,19 @@
11
import { z } from 'zod'
22

3-
import { McpError } from './mcp-error'
3+
import { McpError, safeStatusCode } from './mcp-error'
44

55
import type { AuthRequest } from '@cloudflare/workers-oauth-provider'
6-
import type { ContentfulStatusCode } from 'hono/utils/http-status'
6+
7+
/** Maps known OAuth error codes to safe client-facing messages */
8+
const SAFE_TOKEN_ERROR_MESSAGES: Record<string, string> = {
9+
invalid_grant: 'Authorization grant is invalid, expired, or revoked',
10+
invalid_client: 'Client authentication failed',
11+
invalid_request: 'Invalid token request',
12+
unauthorized_client: 'Client is not authorized for this grant type',
13+
unsupported_grant_type: 'Unsupported grant type',
14+
invalid_scope: 'Requested scope is invalid',
15+
access_denied: 'Access denied',
16+
}
717

818
// Constants
919
const PKCE_CHARSET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~'
@@ -163,10 +173,10 @@ export async function getAuthToken({
163173

164174
if (resp.status >= 400 && resp.status < 500) {
165175
throw new McpError(
166-
upstreamError.error_description || 'Token exchange failed',
167-
resp.status as ContentfulStatusCode,
176+
SAFE_TOKEN_ERROR_MESSAGES[upstreamError.error || ''] || 'Token exchange failed',
177+
safeStatusCode(resp.status),
168178
{
169-
reportToSentry: true,
179+
reportToSentry: false,
170180
internalMessage: `Upstream ${resp.status}: ${body}`,
171181
}
172182
)
@@ -216,10 +226,10 @@ export async function refreshAuthToken({
216226

217227
if (resp.status >= 400 && resp.status < 500) {
218228
throw new McpError(
219-
upstreamError.error_description || 'Token refresh failed',
220-
resp.status as ContentfulStatusCode,
229+
SAFE_TOKEN_ERROR_MESSAGES[upstreamError.error || ''] || 'Token refresh failed',
230+
safeStatusCode(resp.status),
221231
{
222-
reportToSentry: true,
232+
reportToSentry: false,
223233
internalMessage: `Upstream ${resp.status}: ${body}`,
224234
}
225235
)

‎packages/mcp-common/src/cloudflare-oauth-handler.spec.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ describe('handleTokenExchangeCallback', () => {
5555
const err = e as McpError
5656
expect(err.code).toBe(400)
5757
expect(err.message).toBe('Account tokens cannot be refreshed')
58-
expect(err.reportToSentry).toBe(true)
58+
expect(err.reportToSentry).toBe(false)
5959
}
6060
})
6161
})
@@ -78,7 +78,7 @@ describe('handleTokenExchangeCallback', () => {
7878
const err = e as McpError
7979
expect(err.code).toBe(400)
8080
expect(err.message).toBe('No refresh token available for this grant')
81-
expect(err.reportToSentry).toBe(true)
81+
expect(err.reportToSentry).toBe(false)
8282
}
8383
})
8484
})
@@ -114,8 +114,8 @@ describe('handleTokenExchangeCallback', () => {
114114
describe('propagates upstream errors from refreshAuthToken', () => {
115115
it('propagates McpError 400 from expired upstream refresh token', async () => {
116116
mockRefreshAuthToken.mockRejectedValueOnce(
117-
new McpError('The refresh token has expired', 400, {
118-
reportToSentry: true,
117+
new McpError('Authorization grant is invalid, expired, or revoked', 400, {
118+
reportToSentry: false,
119119
internalMessage: 'Upstream 400: {"error":"invalid_grant"}',
120120
})
121121
)
@@ -135,7 +135,7 @@ describe('handleTokenExchangeCallback', () => {
135135
expect(e).toBeInstanceOf(McpError)
136136
const err = e as McpError
137137
expect(err.code).toBe(400)
138-
expect(err.reportToSentry).toBe(true)
138+
expect(err.reportToSentry).toBe(false)
139139
}
140140
})
141141

‎packages/mcp-common/src/cloudflare-oauth-handler.ts‎

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
getAuthToken,
1010
refreshAuthToken,
1111
} from './cloudflare-auth'
12-
import { McpError } from './mcp-error'
12+
import { McpError, safeStatusCode } from './mcp-error'
1313
import { useSentry } from './sentry'
1414
import { V4Schema } from './v4-api'
1515
import {
@@ -30,7 +30,6 @@ import type {
3030
TokenExchangeCallbackResult,
3131
} from '@cloudflare/workers-oauth-provider'
3232
import type { Context } from 'hono'
33-
import type { ContentfulStatusCode } from 'hono/utils/http-status'
3433
import type { MetricsTracker } from '../../mcp-observability/src'
3534
import type { BaseHonoContext } from './sentry'
3635

@@ -99,35 +98,55 @@ export async function getUserAndAccounts(
9998
}),
10099
])
101100

102-
const { result: user } = V4Schema(UserSchema).parse(await userResponse.json())
101+
// Check response status before parsing to avoid Zod errors on non-V4 error bodies
102+
if (!accountsResponse.ok) {
103+
const status = accountsResponse.status
104+
const is5xx = status >= 500 && status <= 599
105+
throw new McpError(
106+
is5xx ? 'Upstream accounts service unavailable' : 'Failed to fetch accounts',
107+
safeStatusCode(is5xx ? 502 : status),
108+
{
109+
reportToSentry: is5xx,
110+
internalMessage: `Upstream /accounts returned ${status}`,
111+
}
112+
)
113+
}
114+
103115
const { result: accounts } = V4Schema(AccountsSchema).parse(await accountsResponse.json())
104-
if (!user || !userResponse.ok) {
105-
// If accounts is present, then assume that we have an account scoped token
116+
117+
if (!userResponse.ok) {
118+
// If accounts is present, assume we have an account-scoped token
106119
if (accounts !== null) {
107120
return { user: null, accounts }
108121
}
109122
const status = userResponse.status
110123
const is5xx = status >= 500 && status <= 599
111124
throw new McpError(
112125
is5xx ? 'Upstream user service unavailable' : 'Failed to fetch user',
113-
(is5xx ? 502 : status) as ContentfulStatusCode,
126+
safeStatusCode(is5xx ? 502 : status),
114127
{
115-
reportToSentry: true,
128+
reportToSentry: is5xx,
116129
internalMessage: `Upstream /user returned ${status}`,
117130
}
118131
)
119132
}
120-
if (!accounts || !accountsResponse.ok) {
121-
const status = accountsResponse.status
122-
const is5xx = status >= 500 && status <= 599
123-
throw new McpError(
124-
is5xx ? 'Upstream accounts service unavailable' : 'Failed to fetch accounts',
125-
(is5xx ? 502 : status) as ContentfulStatusCode,
126-
{
127-
reportToSentry: true,
128-
internalMessage: `Upstream /accounts returned ${status}`,
129-
}
130-
)
133+
134+
const { result: user } = V4Schema(UserSchema).parse(await userResponse.json())
135+
if (!user) {
136+
// User parse succeeded but result was null — fall back to accounts if available
137+
if (accounts !== null) {
138+
return { user: null, accounts }
139+
}
140+
throw new McpError('Failed to fetch user', 500, {
141+
reportToSentry: true,
142+
internalMessage: 'Upstream /user returned null result with 200 status',
143+
})
144+
}
145+
if (!accounts) {
146+
throw new McpError('Failed to fetch accounts', 500, {
147+
reportToSentry: true,
148+
internalMessage: 'Upstream /accounts returned null result with 200 status',
149+
})
131150
}
132151

133152
return { user, accounts }
@@ -180,12 +199,12 @@ export async function handleTokenExchangeCallback(
180199
if (props.type === 'account_token') {
181200
// Account tokens cannot be refreshed — this is a client error, not a server error
182201
throw new McpError('Account tokens cannot be refreshed', 400, {
183-
reportToSentry: true,
202+
reportToSentry: false,
184203
})
185204
}
186205
if (!props.refreshToken) {
187206
throw new McpError('No refresh token available for this grant', 400, {
188-
reportToSentry: true,
207+
reportToSentry: false,
189208
})
190209
}
191210

0 commit comments

Comments
 (0)