diff --git a/e2e/mcp-oauth-consent-redirect.spec.ts b/e2e/mcp-oauth-consent-redirect.spec.ts new file mode 100644 index 000000000..8477e2181 --- /dev/null +++ b/e2e/mcp-oauth-consent-redirect.spec.ts @@ -0,0 +1,111 @@ +import { createHash, randomBytes } from "node:crypto"; +import { createServer, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; + +import { expect, test } from "./setup/test"; + +import { STORAGE_STATE_PATH } from "./setup/global-setup"; +import { SSR_PREFETCH_BASE_URL } from "./setup/ssr-prefetch-server"; + +/** + * The MCP consent screen hands the browser back to the connecting client. + * + * After "Allow", the consent form posts to its own origin and the server + * answers with a redirect to the client's `redirect_uri`. Chromium applies the + * consent page's CSP `form-action` to that redirect as well, so a page that + * only allows `'self'` strands the user on the consent screen while the grant + * has already been issued server-side. Only a real browser enforces CSP, which + * is why this is an e2e journey and not a route test. + * + * It runs against the second server, the one started with `APP_URL`: the MCP + * surface fails closed without a configured origin. The callback is a local + * loopback listener, which is also what desktop MCP clients register. + */ +test.describe("MCP OAuth consent", () => { + test.use({ + storageState: STORAGE_STATE_PATH, + baseURL: SSR_PREFETCH_BASE_URL, + }); + + let callback: Server; + let callbackUrl = ""; + const hits: URL[] = []; + + test.beforeAll(async () => { + callback = createServer((req, res) => { + hits.push(new URL(req.url ?? "/", callbackUrl)); + res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); + res.end("Callback

Connected.

"); + }); + await new Promise((resolve) => + callback.listen(0, "127.0.0.1", () => resolve()), + ); + const { port } = callback.address() as AddressInfo; + callbackUrl = `http://127.0.0.1:${port}/callback`; + }); + + test.afterAll(async () => { + await new Promise((resolve) => callback.close(() => resolve())); + }); + + test("Allow lands on the client's callback with a code", async ({ + page, + }, testInfo) => { + test.skip( + testInfo.project.name !== "chromium-desktop", + "The redirect is enforced by the browser engine, not the layout; one Chromium run is the proof.", + ); + + // Register over the page's own connection rather than the shared API + // request pool, which can hand out a socket the server already closed. + await page.goto("/api/version"); + const registration = await page.evaluate(async (redirectUri) => { + const res = await fetch("/api/mcp/oauth/register", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + client_name: "E2E consent client", + redirect_uris: [redirectUri], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code"], + response_types: ["code"], + }), + }); + return { status: res.status, body: await res.json().catch(() => null) }; + }, callbackUrl); + expect(registration.status, JSON.stringify(registration.body)).toBe(201); + const clientId = (registration.body as { client_id: string }).client_id; + + const verifier = randomBytes(32).toString("base64url"); + const challenge = createHash("sha256").update(verifier).digest("base64url"); + const state = randomBytes(8).toString("hex"); + const origin = new URL(SSR_PREFETCH_BASE_URL).origin; + const authorize = new URL("/api/mcp/oauth/authorize", origin); + for (const [key, value] of Object.entries({ + response_type: "code", + client_id: clientId, + redirect_uri: callbackUrl, + code_challenge: challenge, + code_challenge_method: "S256", + scope: "health:read", + state, + resource: `${origin}/mcp`, + })) { + authorize.searchParams.set(key, value); + } + + await page.goto(authorize.toString()); + const allow = page.getByRole("button", { name: "Allow" }); + await expect(allow).toBeVisible(); + await allow.click(); + + await page.waitForURL((url) => url.toString().startsWith(callbackUrl), { + timeout: 15_000, + }); + const hit = hits.at(-1); + expect(hit, "the callback listener was never reached").toBeDefined(); + expect(hit!.searchParams.get("code")).toMatch(/^hlac_/); + expect(hit!.searchParams.get("state")).toBe(state); + expect(hit!.searchParams.get("iss")).toBe(origin); + }); +}); diff --git a/playwright.config.ts b/playwright.config.ts index a09ee667f..3d4e78b96 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -298,10 +298,15 @@ export default defineConfig({ // rebuilt it. The suite could not see it, because with the prefetch // off both sides paint the same skeleton. // - // One extra server on 3100 closes that. Only - // `dashboard-ssr-prefetch-hydration.spec.ts` talks to it, and it - // asserts the one thing the mocked specs cannot: that the page the - // server streamed is the page the browser keeps. + // One extra server on 3100 closes that. It asserts the one thing the + // mocked specs cannot: that the page the server streamed is the page + // the browser keeps (`dashboard-ssr-prefetch-hydration.spec.ts`). + // + // It also carries `APP_URL`, as a real deployment does. The MCP OAuth + // surface fails closed without a configured origin, so the consent + // journey (`mcp-oauth-consent-redirect.spec.ts`) runs here. The shared + // server stays without it, because `APP_URL` also narrows the accepted + // passkey origins and shapes invite links for every other spec. { command: `PORT=${SSR_PREFETCH_PORT} HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, url: `${SSR_PREFETCH_BASE_URL}/api/version`, @@ -313,6 +318,7 @@ export default defineConfig({ ...process.env, NATIVE_CANVAS: "off", DASHBOARD_SSR_PREFETCH: "true", + APP_URL: SSR_PREFETCH_BASE_URL, }, }, // The mail-configured, scheduler-free server, on its own port. diff --git a/src/__tests__/e2e-measurement-gating-guard.test.ts b/src/__tests__/e2e-measurement-gating-guard.test.ts index 407cc7728..79a749806 100644 --- a/src/__tests__/e2e-measurement-gating-guard.test.ts +++ b/src/__tests__/e2e-measurement-gating-guard.test.ts @@ -39,6 +39,11 @@ const ALLOWLIST: ReadonlyArray<{ file: string; test: string; why: string }> = [ test: "the list and the capture form carry no serious accessibility violations", why: "The evaluate after the goto reads no DOM: it issues the seed POST from the page's own origin so the row is written over the browser's socket rather than the runner's shared keep-alive agent, which handed POSTs to sockets the server had already closed. The scan that follows is gated on the reloaded list.", }, + { + file: "mcp-oauth-consent-redirect.spec.ts", + test: "Allow lands on the client's callback with a code", + why: "The evaluate after the goto reads no DOM: the goto only gives the page an origin, and the evaluate registers the OAuth client from it so the POST travels over the browser's socket rather than the runner's shared keep-alive agent. The consent page that follows is gated on the Allow button being visible.", + }, { file: "ipad-viewport.spec.ts", test: "no horizontal overflow and full-width content on", diff --git a/src/__tests__/proxy-mcp-authorize-csp.test.ts b/src/__tests__/proxy-mcp-authorize-csp.test.ts new file mode 100644 index 000000000..6a6cc669b --- /dev/null +++ b/src/__tests__/proxy-mcp-authorize-csp.test.ts @@ -0,0 +1,53 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * The MCP OAuth authorization endpoint owns its CSP: its consent form leads to + * the connecting client's redirect URI, and Chromium applies `form-action` to + * that redirect. Middleware headers override route headers, so the proxy must + * not set a CSP on exactly that path, and must keep setting it everywhere else, + * including the neighbouring OAuth endpoints. + */ + +vi.mock("@/lib/process-type", () => ({ + shouldRunWeb: () => true, +})); + +import { proxy } from "../proxy"; + +beforeEach(() => { + vi.stubEnv("NODE_ENV", "production"); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +function request(pathname: string): NextRequest { + return new NextRequest(`http://localhost${pathname}`); +} + +describe("proxy CSP on the MCP authorize endpoint", () => { + it("leaves the CSP to the route on the authorize path", () => { + const res = proxy( + request("/api/mcp/oauth/authorize?client_id=x&redirect_uri=y"), + ); + expect(res.headers.get("content-security-policy")).toBeNull(); + // Every other security header still applies there. + expect(res.headers.get("x-frame-options")).toBe("DENY"); + expect(res.headers.get("x-content-type-options")).toBe("nosniff"); + }); + + it("keeps the app CSP on neighbouring and lookalike paths", () => { + for (const path of [ + "/api/mcp/oauth/token", + "/api/mcp/oauth/register", + "/api/mcp/oauth/authorize/extra", + "/api/mcp/oauth/authorizex", + "/api/mcp/oauth", + ]) { + const csp = proxy(request(path)).headers.get("content-security-policy"); + expect(csp, path).toContain("form-action 'self';"); + } + }); +}); diff --git a/src/app/api/mcp/oauth/__tests__/authorize.test.ts b/src/app/api/mcp/oauth/__tests__/authorize.test.ts index 9bcc403b3..f302a20c3 100644 --- a/src/app/api/mcp/oauth/__tests__/authorize.test.ts +++ b/src/app/api/mcp/oauth/__tests__/authorize.test.ts @@ -270,7 +270,7 @@ describe("POST /authorize — decision", () => { expect(res.status).toBe(401); }); - it("mints a code and 302s back on allow", async () => { + it("mints a code and 303s back on allow (a form POST is followed with GET)", async () => { signedIn(); const res = await POST( postReq({ @@ -285,7 +285,7 @@ describe("POST /authorize — decision", () => { decision: "allow", }) as never, ); - expect(res.status).toBe(302); + expect(res.status).toBe(303); const loc = res.headers.get("location") ?? ""; expect(loc.startsWith(REDIRECT)).toBe(true); const code = new URL(loc).searchParams.get("code"); @@ -295,7 +295,7 @@ describe("POST /authorize — decision", () => { expect(new URL(loc).searchParams.get("iss")).toBe("https://health.example"); }); - it("302s with access_denied on deny", async () => { + it("303s with access_denied on deny", async () => { signedIn(); const res = await POST( postReq({ @@ -309,10 +309,85 @@ describe("POST /authorize — decision", () => { decision: "deny", }) as never, ); - expect(res.status).toBe(302); + expect(res.status).toBe(303); const loc = res.headers.get("location") ?? ""; expect(new URL(loc).searchParams.get("error")).toBe("access_denied"); // RFC 9207 — the issuer is echoed on the error redirect too. expect(new URL(loc).searchParams.get("iss")).toBe("https://health.example"); }); }); + +describe("authorize CSP — the consent form may lead only to the validated redirect", () => { + function csp(res: Response): string { + return res.headers.get("content-security-policy") ?? ""; + } + + it("allows the registered client's redirect origin on the consent page", async () => { + signedIn(); + const res = await GET(getReq(authorizeUrl()) as never); + expect(res.status).toBe(200); + expect(csp(res)).toContain("form-action 'self' https://claude.ai;"); + expect(csp(res)).toContain("default-src 'none'"); + expect(csp(res)).toContain("frame-ancestors 'none'"); + }); + + it("uses the requested loopback port the client is allowed to vary", async () => { + const loopback = registerDcrClient({ + clientName: "Desktop client", + redirectUris: ["http://127.0.0.1:4000/callback"], + }); + signedIn(); + const res = await GET( + getReq( + authorizeUrl({ + client_id: loopback.clientId, + redirect_uri: "http://127.0.0.1:53682/callback", + }), + ) as never, + ); + expect(res.status).toBe(200); + expect(csp(res)).toContain("form-action 'self' http://127.0.0.1:53682;"); + }); + + it("never names a redirect origin that failed validation", async () => { + signedIn(); + const res = await GET( + getReq( + authorizeUrl({ redirect_uri: "https://evil.example/cb" }), + ) as never, + ); + expect(res.status).toBe(400); + expect(csp(res)).toContain("form-action 'self';"); + expect(csp(res)).not.toContain("evil.example"); + }); + + it("keeps form-action to 'self' on the sign-in prompt", async () => { + const res = await GET(getReq(authorizeUrl()) as never); + expect(res.status).toBe(200); + expect(csp(res)).toContain("form-action 'self';"); + expect(csp(res)).not.toContain("claude.ai"); + }); + + it("sets the policy on the decision redirect and on refusals", async () => { + signedIn(); + const allow = await POST( + postReq({ + response_type: "code", + client_id: CLIENT.clientId, + redirect_uri: REDIRECT, + code_challenge: CHALLENGE, + code_challenge_method: "S256", + scope: "health:read", + resource: RESOURCE, + decision: "allow", + }) as never, + ); + expect(allow.status).toBe(303); + expect(csp(allow)).toContain("form-action 'self';"); + + vi.mocked(isApiGloballyEnabled).mockResolvedValue(false); + const unavailable = await GET(getReq(authorizeUrl()) as never); + expect(unavailable.status).toBe(503); + expect(csp(unavailable)).toContain("default-src 'none'"); + }); +}); diff --git a/src/app/api/mcp/oauth/authorize/route.ts b/src/app/api/mcp/oauth/authorize/route.ts index 04de08df8..3f83189b0 100644 --- a/src/app/api/mcp/oauth/authorize/route.ts +++ b/src/app/api/mcp/oauth/authorize/route.ts @@ -13,7 +13,7 @@ * POST — the consent decision. Requires a session, re-validates every * parameter, and on "Allow" mints a single-use, short-lived, * self-describing authorization code bound to this user + client + - * redirect URI + PKCE challenge + scope + audience, then 302s back. + * redirect URI + PKCE challenge + scope + audience, then 303s back. * * Mandatory gates, every request: PKCE `S256` (a missing / non-S256 challenge is * rejected), audience binding (`resource` MUST equal the canonical `/mcp` URI — @@ -43,6 +43,7 @@ import { import { redirectUriAllowed, resolveClient } from "@/lib/mcp/oauth/clients"; import { isValidChallenge } from "@/lib/mcp/oauth/pkce"; import { signArtifact } from "@/lib/mcp/oauth/artifacts"; +import { withAuthorizeCsp } from "@/lib/mcp/oauth/consent-csp"; export const runtime = "nodejs"; export const dynamic = "force-dynamic"; @@ -269,13 +270,21 @@ function consentOriginAllowed(request: NextRequest): boolean { return true; } +/** + * Send the browser back to the validated redirect URI. 303, not 302: the + * decision arrives as a form POST, and 303 is the status that tells every user + * agent to follow with a GET (RFC 9700, section 4.12). + */ function redirectBack( redirectUri: string, query: Record, ): Response { const url = new URL(redirectUri); for (const [k, v] of Object.entries(query)) url.searchParams.set(k, v); - return Response.redirect(url.toString(), 302); + return new Response(null, { + status: 303, + headers: { Location: url.toString() }, + }); } /** Whether the surface is available (M1 origin pinned + M4 kill-switch off). */ @@ -283,7 +292,26 @@ async function surfaceAvailable(): Promise { return isMcpOriginConfigured() && (await isApiGloballyEnabled()); } +/** + * Every response from this route carries the route's own CSP; the proxy leaves + * this path alone (see `consent-csp.ts`). A response that already set one, the + * consent page with its validated redirect origin, keeps it. + */ +function ensureAuthorizeCsp(response: Response): Response { + return response.headers.has("content-security-policy") + ? response + : withAuthorizeCsp(response); +} + export async function GET(request: NextRequest): Promise { + return ensureAuthorizeCsp(await authorizeGet(request)); +} + +export async function POST(request: NextRequest): Promise { + return ensureAuthorizeCsp(await authorizePost(request)); +} + +async function authorizeGet(request: NextRequest): Promise { return withBackgroundEvent("mcp.oauth.authorize", async () => { if (!(await surfaceAvailable())) { return Response.json( @@ -347,8 +375,11 @@ export async function GET(request: NextRequest): Promise { const accessSummary = writeGranted ? `

This grants read AND write access: the application will be able to read your own health records and log new measurements and mood entries to your account on your behalf. It cannot delete or change existing entries, edit medications, or reach admin functions.

` : `

Scope: ${htmlEscape(v.scope)} — read-only access to your own health records.

`; - return html( - `
+ // The one response whose form may lead off-origin: allow exactly the + // redirect URI `validate()` just matched against the resolved client. + return withAuthorizeCsp( + html( + `

Authorize access

${htmlEscape(v.clientName)} is requesting access to your HealthLog data.

${provenance} @@ -367,11 +398,13 @@ export async function GET(request: NextRequest): Promise {
`, + ), + v.redirectUri, ); }); } -export async function POST(request: NextRequest): Promise { +async function authorizePost(request: NextRequest): Promise { return withBackgroundEvent("mcp.oauth.authorize", async () => { if (!(await surfaceAvailable())) { return Response.json( diff --git a/src/lib/mcp/oauth/__tests__/consent-csp.test.ts b/src/lib/mcp/oauth/__tests__/consent-csp.test.ts new file mode 100644 index 000000000..fec015649 --- /dev/null +++ b/src/lib/mcp/oauth/__tests__/consent-csp.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; + +import { + authorizeCsp, + formActionOrigin, + withAuthorizeCsp, +} from "../consent-csp"; + +describe("authorize CSP", () => { + it("allows only 'self' in form-action without a validated redirect", () => { + const csp = authorizeCsp(); + expect(csp).toContain("form-action 'self';"); + expect(csp).toContain("default-src 'none'"); + expect(csp).toContain("frame-ancestors 'none'"); + expect(csp).toContain("base-uri 'none'"); + expect(csp).not.toMatch(/script-src|style-src|unsafe/); + }); + + it("adds the origin of the validated redirect URI, not its path or query", () => { + const csp = authorizeCsp( + "https://claude.ai/api/mcp/auth_callback?x=1#frag", + ); + expect(csp).toContain("form-action 'self' https://claude.ai;"); + }); + + it("keeps the port of a loopback redirect", () => { + expect(authorizeCsp("http://127.0.0.1:53682/callback")).toContain( + "form-action 'self' http://127.0.0.1:53682;", + ); + }); + + it("adds nothing for a scheme registration never admits", () => { + for (const uri of [ + "javascript:alert(1)", + "data:text/html,hi", + "myapp://callback", + "not a url", + "", + ]) { + expect(formActionOrigin(uri), uri).toBeNull(); + expect(authorizeCsp(uri), uri).toContain("form-action 'self';"); + } + }); + + it("sets the header on responses whose headers are immutable", async () => { + const redirect = Response.redirect("https://claude.ai/cb", 302); + const res = withAuthorizeCsp(redirect); + expect(res.status).toBe(302); + expect(res.headers.get("location")).toBe("https://claude.ai/cb"); + expect(res.headers.get("content-security-policy")).toContain( + "form-action 'self';", + ); + + const json = withAuthorizeCsp(Response.json({ ok: true }, { status: 400 })); + expect(json.status).toBe(400); + expect(await json.json()).toEqual({ ok: true }); + }); +}); diff --git a/src/lib/mcp/oauth/consent-csp.ts b/src/lib/mcp/oauth/consent-csp.ts new file mode 100644 index 000000000..6c7a6c446 --- /dev/null +++ b/src/lib/mcp/oauth/consent-csp.ts @@ -0,0 +1,69 @@ +/** + * Content-Security-Policy for the MCP OAuth authorization endpoint. + * + * The consent screen is a plain HTML form that posts to its own origin; on + * "Allow" or "Deny" the server answers with a redirect to the client's + * `redirect_uri`. Chromium applies the document's `form-action` to every hop of + * that navigation, redirects included, so the app-wide `form-action 'self'` + * strands the user on the consent screen after the grant was already issued. + * + * The route therefore owns the CSP of its own responses (the proxy leaves this + * exact path alone). The consent page allows `form-action` for `'self'` plus + * the ORIGIN of the one redirect URI the route has just validated against the + * resolved client for this request, never a client-supplied value that has not + * passed that check. Every other response from the route gets the strict + * policy with `form-action 'self'`. The pages carry no script, style or + * subresource, so everything else is `'none'`. + */ + +const CSP_REPORT_ENDPOINT = "/api/monitoring/csp-report"; + +/** + * The origin a validated redirect URI may add to `form-action`, or null. + * Registration only admits `https:` and `http:` loopback URIs; anything else + * (or anything unparsable) adds nothing, so a later relaxation of the + * registration rules cannot silently widen this policy. + */ +export function formActionOrigin(redirectUri: string): string | null { + let url: URL; + try { + url = new URL(redirectUri); + } catch { + return null; + } + if (url.protocol !== "https:" && url.protocol !== "http:") return null; + return url.origin; +} + +/** The policy string. Pass the validated redirect URI only on the consent page. */ +export function authorizeCsp(validatedRedirectUri?: string): string { + const extra = validatedRedirectUri + ? formActionOrigin(validatedRedirectUri) + : null; + const formAction = extra ? `'self' ${extra}` : "'self'"; + return [ + "default-src 'none'", + `form-action ${formAction}`, + "frame-ancestors 'none'", + "base-uri 'none'", + `report-uri ${CSP_REPORT_ENDPOINT}`, + ].join("; "); +} + +/** + * Return `response` with the authorize CSP set. A fresh `Response` is built + * because `Response.redirect()` and `Response.json()` can carry immutable + * headers. + */ +export function withAuthorizeCsp( + response: Response, + validatedRedirectUri?: string, +): Response { + const headers = new Headers(response.headers); + headers.set("Content-Security-Policy", authorizeCsp(validatedRedirectUri)); + return new Response(response.body, { + status: response.status, + statusText: response.statusText, + headers, + }); +} diff --git a/src/proxy.ts b/src/proxy.ts index 7eea0ed35..6c7f711ac 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -683,7 +683,18 @@ export function proxy(request: NextRequest) { isDev ? `default-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data: blob:; frame-src 'self' blob:; connect-src 'self'; font-src 'self';` : `default-src 'self'; script-src 'self' 'nonce-${nonce}'; style-src 'self' 'unsafe-inline'; img-src 'self' data: blob:; frame-src 'self' blob:; connect-src 'self'${aiConnectSrc}${withingsConnectSrc}${whoopConnectSrc}${stravaConnectSrc}; font-src 'self'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; worker-src 'self'; report-uri ${cspReportEndpoint}; report-to csp-endpoint;`; - response.headers.set("Content-Security-Policy", csp); + // The MCP OAuth authorization endpoint sets its own CSP. Its consent form + // posts to this origin and the server then redirects to the connecting + // client's `redirect_uri`; Chromium applies `form-action` to that redirect, + // so the app-wide `form-action 'self'` would strand the user after the + // grant. The route allows exactly the redirect origin it validated for the + // request (`src/lib/mcp/oauth/consent-csp.ts`). Middleware headers override + // route headers, so the proxy must leave the header unset on this exact + // path; every other header above still applies. + const isMcpAuthorizeRoute = pathname === "/api/mcp/oauth/authorize"; + if (!isMcpAuthorizeRoute) { + response.headers.set("Content-Security-Policy", csp); + } // Production-only headers. HSTS carries `preload` so the domain stays // eligible for the Chromium preload list — closes the first-visit MITM