diff --git a/docs/operations/oauth-signout-playbook.md b/docs/operations/oauth-signout-playbook.md index c3387b7d7..ce55deea9 100644 --- a/docs/operations/oauth-signout-playbook.md +++ b/docs/operations/oauth-signout-playbook.md @@ -107,6 +107,14 @@ Attributes: `OAuth authorization failed: Invalid redirect URI` (both GET and POST `/oauth/authorize`) and `Redirect URI not registered for client on callback` now carry `clientId`, `redirectUri`, `registeredUris`, `clientName` as `extra` fields. +`OAuth authorization request rejected: missing redirect URI` identifies GET +`/oauth/authorize` requests with a non-empty `client_id` and a missing or empty +`redirect_uri`. These return a local 400 before client metadata lookup, with no +redirect or Sentry issue. The log carries `code: invalid_request` and +`reason: missing_redirect_uri`, without the authorization URL. Ask the user to +restart authentication in their client and open the full link. Metadata fetch +failures for complete authorization requests still follow the existing error path. + The OAuth provider itself emits high-volume logs shaped like `OAuth error response: 401 invalid_token - Invalid access token`. Treat those as the provider-level count source; do not add a second high-volume app log for diff --git a/packages/mcp-cloudflare/src/server/oauth/client-metadata.integration.test.ts b/packages/mcp-cloudflare/src/server/oauth/client-metadata.integration.test.ts index 41f3e37ae..1b2c4a840 100644 --- a/packages/mcp-cloudflare/src/server/oauth/client-metadata.integration.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/client-metadata.integration.test.ts @@ -1,6 +1,6 @@ import { createExecutionContext, env } from "cloudflare:test"; import { getOAuthApi } from "@cloudflare/workers-oauth-provider"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { SCOPES } from "../../constants"; import { createPkcePair, PKCE_CODE_CHALLENGE } from "../../test-utils/pkce"; import app from "../app"; @@ -116,6 +116,48 @@ async function exchangeCode( } describe("pre-registered client metadata integration", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it.each([undefined, ""])( + "rejects an incomplete authorization link before fetching metadata: redirect_uri=%j", + async (redirectUri) => { + const fetchSpy = vi.spyOn(globalThis, "fetch"); + const response = await handler.fetch!( + createAuthorizationRequest(`${CODEX_CLIENT_ID}?other-client=1`, { + redirect_uri: redirectUri, + }), + workerEnv, + createExecutionContext(), + ); + + expect(response.status).toBe(400); + expect(response.headers.get("Location")).toBeNull(); + expect(await response.text()).toContain( + "authorization link is incomplete", + ); + expect(fetchSpy).not.toHaveBeenCalled(); + }, + ); + + it("preserves the missing client ID error for an incomplete authorization link", async () => { + const fetchSpy = vi.spyOn(globalThis, "fetch"); + const response = await handler.fetch!( + createAuthorizationRequest(CODEX_CLIENT_ID, { + client_id: undefined, + redirect_uri: undefined, + }), + workerEnv, + createExecutionContext(), + ); + + expect(response.status).toBe(400); + expect(response.headers.get("Location")).toBeNull(); + expect(await response.text()).toBe("client_id is required"); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + it("authorizes Codex and exchanges and refreshes tokens when upstream metadata is blocked", async () => { // The network mock returns 403: a fresh provider without pre-registration // cannot resolve this client, so an existing cache cannot hide the outage. @@ -194,6 +236,17 @@ describe("pre-registered client metadata integration", () => { it("still fetches unregistered CIMD URLs and rejects an upstream 403", async () => { const clientId = `${CODEX_CLIENT_ID}?other-client=1`; + const fetchSpy = vi.spyOn(globalThis, "fetch"); + const authorizationResponse = await handler.fetch!( + createAuthorizationRequest(clientId), + workerEnv, + createExecutionContext(), + ); + expect(authorizationResponse.status).toBe(500); + expect(authorizationResponse.headers.get("Location")).toBeNull(); + expect(await authorizationResponse.text()).toBe("Internal Server Error"); + expect(fetchSpy).toHaveBeenCalled(); + await expect( createOAuthApi().parseAuthRequest(createAuthorizationRequest(clientId)), ).rejects.toThrow("Failed to fetch client metadata: HTTP 403"); diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index 4bdaf1215..491a518a3 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -74,6 +74,23 @@ export default new Hono<{ Bindings: Env }>() * This route initiates the OAuth flow when a user wants to log in. */ .get("/", async (c) => { + const authUrl = new URL(c.req.url); + if ( + authUrl.searchParams.get("client_id") && + !authUrl.searchParams.get("redirect_uri") + ) { + // The provider looks up client metadata before checking redirect_uri. + // Reject incomplete links first so an upstream failure cannot mask them. + logWarn("OAuth authorization request rejected: missing redirect URI", { + loggerScope: ["cloudflare", "oauth", "authorize"], + extra: { code: "invalid_request", reason: "missing_redirect_uri" }, + }); + return c.text( + "The authorization link is incomplete. Restart authentication in your client and open the full link.", + 400, + ); + } + let oauthReqInfo: AuthRequest; try { oauthReqInfo = await c.env.OAUTH_PROVIDER.parseAuthRequest(c.req.raw); @@ -88,7 +105,6 @@ export default new Hono<{ Bindings: Env }>() if (errorMessage.includes("Invalid redirect URI")) { // parseAuthRequest threw before producing a request, so read the // attempted values directly from the query string. - const authUrl = new URL(c.req.url); const attemptedClientId = authUrl.searchParams.get("client_id"); const attemptedRedirectUri = authUrl.searchParams.get("redirect_uri"); let registeredUris: string[] | undefined;