Skip to content

Commit 844ced6

Browse files
dcramercodex
andcommitted
fix(cloudflare): keep oauth system failures issue-worthy
Treat generated OAuth request failures and unknown upstream HTTP failures as system-side auth outages so the Cloudflare callback still captures Sentry issues and surfaces event IDs. Co-Authored-By: Codex CLI Agent <noreply@openai.com>
1 parent f51584a commit 844ced6

File tree

3 files changed

+128
-40
lines changed

3 files changed

+128
-40
lines changed

packages/mcp-cloudflare/src/server/oauth/callback.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,39 @@ describe("oauth callback routes", () => {
314314
expect(exchangeCodeForAccessToken).not.toHaveBeenCalled();
315315
});
316316

317+
it("treats invalid_request callback errors as system failures", async () => {
318+
logIssue.mockReturnValue("oauth-invalid-request-event-id");
319+
320+
const testEnv = createTestEnv();
321+
const oauthApp = createTestApp();
322+
const client = await createClient(testEnv);
323+
const cookie = await approveClient(oauthApp, testEnv, client.clientId);
324+
const state = await createSignedCallbackState(client.clientId);
325+
326+
const response = await callCallback(oauthApp, testEnv, {
327+
state,
328+
cookie,
329+
error: "invalid_request",
330+
});
331+
332+
const body = await response.text();
333+
expect(response.status).toBe(502);
334+
expect(body).toContain(
335+
"The authorization request was rejected. Please try again.",
336+
);
337+
expect(body).toContain(
338+
"Event ID:</strong> <code>oauth-invalid-request-event-id</code>",
339+
);
340+
expect(logIssue).toHaveBeenCalledWith(
341+
"[oauth] Upstream authorization callback error",
342+
expect.objectContaining({
343+
loggerScope: ["cloudflare", "oauth", "callback"],
344+
}),
345+
);
346+
expect(logWarn).not.toHaveBeenCalled();
347+
expect(exchangeCodeForAccessToken).not.toHaveBeenCalled();
348+
});
349+
317350
it("renders a safe error page when the callback is missing a code", async () => {
318351
const testEnv = createTestEnv();
319352
const oauthApp = createTestApp();

packages/mcp-cloudflare/src/server/oauth/helpers.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ vi.mock("@sentry/mcp-core/telem/logging", () => ({
1414
import {
1515
createResourceValidationError,
1616
exchangeCodeForAccessToken,
17+
getOAuthFailureDetails,
1718
tokenExchangeCallback,
1819
validateResourceParameter,
1920
} from "./helpers";
@@ -351,6 +352,63 @@ describe("exchangeCodeForAccessToken", () => {
351352
);
352353
expect(logWarn).not.toHaveBeenCalled();
353354
});
355+
356+
it("returns an event ID for unknown upstream http failures", async () => {
357+
logIssue.mockReturnValue("oauth-forbidden-event-id");
358+
vi.spyOn(globalThis, "fetch").mockResolvedValue(
359+
new Response("<title>403</title>403 Forbidden", {
360+
status: 403,
361+
statusText: "Forbidden",
362+
headers: { "Content-Type": "text/html; charset=UTF-8" },
363+
}),
364+
);
365+
366+
const [payload, response] = await exchangeCodeForAccessToken({
367+
client_id: "test-client-id",
368+
client_secret: "test-client-secret",
369+
code: "test-code",
370+
upstream_url: "https://sentry.io/oauth/token",
371+
redirect_uri: "https://mcp.sentry.dev/oauth/callback",
372+
});
373+
374+
expect(payload).toBeNull();
375+
expect(response).not.toBeNull();
376+
expect(response!.status).toBe(502);
377+
378+
const body = await response!.text();
379+
expect(body).toContain(
380+
"There was an internal error authenticating your account. Please try again shortly.",
381+
);
382+
expect(body).toContain(
383+
"Event ID:</strong> <code>oauth-forbidden-event-id</code>",
384+
);
385+
expect(logIssue).toHaveBeenCalledWith(
386+
"[oauth] Failed to exchange code for access token",
387+
expect.objectContaining({
388+
loggerScope: ["cloudflare", "oauth", "callback"],
389+
}),
390+
);
391+
expect(logWarn).not.toHaveBeenCalled();
392+
});
393+
});
394+
395+
describe("getOAuthFailureDetails", () => {
396+
it("treats invalid_scope as a system failure", () => {
397+
expect(getOAuthFailureDetails({ oauthError: "invalid_scope" })).toEqual({
398+
message: "The requested permissions were invalid. Please try again.",
399+
status: 502,
400+
shouldLogIssue: true,
401+
});
402+
});
403+
404+
it("treats unknown upstream http failures as system failures", () => {
405+
expect(getOAuthFailureDetails({ upstreamStatus: 403 })).toEqual({
406+
message:
407+
"There was an internal error authenticating your account. Please try again shortly.",
408+
status: 502,
409+
shouldLogIssue: true,
410+
});
411+
});
354412
});
355413

356414
describe("validateResourceParameter", () => {

packages/mcp-cloudflare/src/server/oauth/helpers.ts

Lines changed: 37 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ export function getOAuthFailureDetails({
3737
status: number;
3838
shouldLogIssue: boolean;
3939
} {
40+
const systemFailure = (message: string, status = 502) => ({
41+
message,
42+
status,
43+
shouldLogIssue: true,
44+
});
45+
4046
switch (oauthError) {
4147
case "access_denied":
4248
return {
@@ -46,25 +52,18 @@ export function getOAuthFailureDetails({
4652
shouldLogIssue: false,
4753
};
4854
case "temporarily_unavailable":
49-
return {
50-
message:
51-
"Sentry OAuth is temporarily unavailable. Please try again shortly.",
52-
status: 503,
53-
shouldLogIssue: true,
54-
};
55+
return systemFailure(
56+
"Sentry OAuth is temporarily unavailable. Please try again shortly.",
57+
503,
58+
);
5559
case "server_error":
56-
return {
57-
message:
58-
"Sentry OAuth encountered an internal error. Please try again.",
59-
status: 502,
60-
shouldLogIssue: true,
61-
};
60+
return systemFailure(
61+
"Sentry OAuth encountered an internal error. Please try again.",
62+
);
6263
case "invalid_request":
63-
return {
64-
message: "The authorization request was rejected. Please try again.",
65-
status: 400,
66-
shouldLogIssue: false,
67-
};
64+
return systemFailure(
65+
"The authorization request was rejected. Please try again.",
66+
);
6867
case "invalid_grant":
6968
return {
7069
message:
@@ -73,36 +72,34 @@ export function getOAuthFailureDetails({
7372
shouldLogIssue: false,
7473
};
7574
case "invalid_scope":
76-
return {
77-
message: "The requested permissions were invalid. Please try again.",
78-
status: 400,
79-
shouldLogIssue: false,
80-
};
75+
return systemFailure(
76+
"The requested permissions were invalid. Please try again.",
77+
);
8178
case "invalid_client":
8279
case "unauthorized_client":
8380
case "unsupported_grant_type":
84-
return {
85-
message:
86-
"There was an internal configuration issue completing authentication. Please try again later.",
87-
status: 500,
88-
shouldLogIssue: true,
89-
};
81+
return systemFailure(
82+
"There was an internal configuration issue completing authentication. Please try again later.",
83+
500,
84+
);
9085
default:
91-
if (typeof upstreamStatus === "number" && upstreamStatus >= 500) {
92-
return {
93-
message:
86+
if (typeof upstreamStatus === "number") {
87+
if (upstreamStatus >= 500) {
88+
return systemFailure(
9489
"There was an internal error authenticating your account. Please try again shortly.",
95-
status: 502,
96-
shouldLogIssue: true,
97-
};
90+
);
91+
}
92+
93+
// Any upstream HTTP failure outside explicit user-correctable OAuth errors
94+
// points to our configuration, the upstream service, or edge protection.
95+
return systemFailure(
96+
"There was an internal error authenticating your account. Please try again shortly.",
97+
);
9898
}
9999

100-
return {
101-
message:
102-
"There was an issue authenticating your account. Please try again.",
103-
status: 400,
104-
shouldLogIssue: false,
105-
};
100+
return systemFailure(
101+
"There was an internal error authenticating your account. Please try again shortly.",
102+
);
106103
}
107104
}
108105

0 commit comments

Comments
 (0)