Skip to content

Commit eb19278

Browse files
dcramercodex
andauthored
fix(cloudflare): Show requested OAuth redirect URI (#930)
Fix the OAuth approval dialog so it only shows the redirect URI for the active authorization request. Clients with multiple registered callbacks were rendering one warning box per URI, which made the destination prompt noisy and misleading. **Redirect URI Rendering** The authorize route now passes the validated request redirect URI into the approval dialog. The dialog renders that URI and only falls back to the client's single registered redirect URI when no explicit request URI is present. **Regression Coverage** Add an authorize-route regression test for a client with multiple registered redirect URIs and assert that only the requested callback is displayed. Co-authored-by: Codex CLI Agent <noreply@openai.com>
1 parent 8174594 commit eb19278

3 files changed

Lines changed: 50 additions & 14 deletions

File tree

packages/mcp-cloudflare/src/server/lib/approval-dialog.ts

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,10 @@ export interface ApprovalDialogOptions {
199199
organizationSlug: string;
200200
projectSlug: string | null;
201201
} | null;
202+
/**
203+
* The specific redirect URI selected for this authorization request.
204+
*/
205+
redirectUri?: string | null;
202206
/**
203207
* Arbitrary state data to pass through the approval flow
204208
* Will be encoded in the form and returned when approval is complete
@@ -275,7 +279,7 @@ export async function renderApprovalDialog(
275279
request: Request,
276280
options: ApprovalDialogOptions,
277281
): Promise<Response> {
278-
const { client, server, scope, state, cookieSecret } = options;
282+
const { client, server, scope, redirectUri, state, cookieSecret } = options;
279283

280284
// Use static skill definitions bundled at build time
281285
const skills: SkillDefinition[] = skillDefinitions as SkillDefinition[];
@@ -317,25 +321,23 @@ export async function renderApprovalDialog(
317321
? sanitizeHtml(sanitizeHrefUrl(client.tosUri))
318322
: "";
319323

320-
// Get redirect URIs
321-
const redirectUris =
322-
client?.redirectUris && client.redirectUris.length > 0
323-
? client.redirectUris.map((uri) => sanitizeHtml(uri))
324-
: [];
324+
const redirectWarningUri =
325+
typeof redirectUri === "string" && redirectUri.length > 0
326+
? sanitizeHtml(redirectUri)
327+
: client?.redirectUris?.length === 1
328+
? sanitizeHtml(client.redirectUris[0])
329+
: null;
325330

326-
// Generate redirect URI warnings
327-
const redirectWarningsHtml = redirectUris
328-
.map((uri) => {
329-
return `
331+
const redirectWarningsHtml = redirectWarningUri
332+
? `
330333
<div class="redirect-warning">
331-
<div class="redirect-uri-display">${uri}</div>
334+
<div class="redirect-uri-display">${redirectWarningUri}</div>
332335
<div class="redirect-warning-text">
333336
After approval, you will be redirected to this URL. Only approve if you recognize and trust this destination.
334337
</div>
335338
</div>
336-
`;
337-
})
338-
.join("");
339+
`
340+
: "";
339341

340342
const scopeHtml = scope
341343
? `

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,39 @@ describe("oauth authorize routes", () => {
5858
expect(html).toContain("<form");
5959
expect(html).toContain('name="state"');
6060
});
61+
62+
it("renders only the requested redirect URI when the client has multiple registered URIs", async () => {
63+
mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({
64+
clientId: "test-client",
65+
redirectUri: "https://example.com/requested-callback",
66+
scope: ["read"],
67+
state: "orig",
68+
});
69+
mockOAuthProvider.lookupClient.mockResolvedValueOnce({
70+
clientId: "test-client",
71+
clientName: "Test Client",
72+
redirectUris: [
73+
"https://example.com/requested-callback",
74+
"https://example.com/another-callback",
75+
"https://example.com/fallback-callback",
76+
],
77+
tokenEndpointAuthMethod: "client_secret_basic",
78+
});
79+
80+
const request = new Request("http://localhost/oauth/authorize", {
81+
method: "GET",
82+
});
83+
const response = await app.fetch(request, testEnv as Env);
84+
85+
expect(response.status).toBe(200);
86+
const html = await response.text();
87+
expect(html).toContain("https://example.com/requested-callback");
88+
expect(html).not.toContain("https://example.com/another-callback");
89+
expect(html).not.toContain("https://example.com/fallback-callback");
90+
expect(
91+
html.match(/After approval, you will be redirected to this URL\./g),
92+
).toHaveLength(1);
93+
});
6194
});
6295

6396
describe("POST /oauth/authorize", () => {

packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ export default new Hono<{ Bindings: Env }>()
165165
name: "Sentry MCP",
166166
},
167167
scope: approvalScope,
168+
redirectUri: oauthReqInfoWithResource.redirectUri,
168169
state: { oauthReqInfo: oauthReqInfoWithResource },
169170
cookieSecret: c.env.COOKIE_SECRET,
170171
});

0 commit comments

Comments
 (0)