mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 04:50:59 +00:00
fix(mcp): let the consent screen return to the connecting client
After Allow on the MCP OAuth consent screen, Chromium-based browsers stayed on the page although the grant was issued. The page carried the app-wide CSP form-action 'self', and Chromium applies form-action to the redirect that follows the form post to the client's redirect_uri. The authorize route now owns the CSP of its responses and the proxy leaves that exact path alone. The consent page allows form-action for its own origin plus the origin of the redirect URI the route has just validated against the resolved client, and only https or http origins qualify. Every other response from the route keeps form-action 'self' with default-src 'none'. The redirect after the decision is now 303, which tells every browser to follow a form POST with a GET. Refs #983
This commit is contained in:
1 parent
7b7a77b292
commit
864566d882
6 files changed
+309
-10
No files matched your search
@@ -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';");
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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'");
|
||||
});
|
||||
});
|
||||
@@ -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<string, string>,
|
||||
): 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<boolean> {
|
||||
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<Response> {
|
||||
return ensureAuthorizeCsp(await authorizeGet(request));
|
||||
}
|
||||
|
||||
export async function POST(request: NextRequest): Promise<Response> {
|
||||
return ensureAuthorizeCsp(await authorizePost(request));
|
||||
}
|
||||
|
||||
async function authorizeGet(request: NextRequest): Promise<Response> {
|
||||
return withBackgroundEvent("mcp.oauth.authorize", async () => {
|
||||
if (!(await surfaceAvailable())) {
|
||||
return Response.json(
|
||||
@@ -347,8 +375,11 @@ export async function GET(request: NextRequest): Promise<Response> {
|
||||
const accessSummary = writeGranted
|
||||
? `<p><strong>This grants read AND write access:</strong> the application will be able to read your own health records <strong>and log new measurements and mood entries</strong> to your account on your behalf. It cannot delete or change existing entries, edit medications, or reach admin functions.</p>`
|
||||
: `<p>Scope: <code>${htmlEscape(v.scope)}</code> — read-only access to your own health records.</p>`;
|
||||
return html(
|
||||
`<main>
|
||||
// The one response whose form may lead off-origin: allow exactly the
|
||||
// redirect URI `validate()` just matched against the resolved client.
|
||||
return withAuthorizeCsp(
|
||||
html(
|
||||
`<main>
|
||||
<h1>Authorize access</h1>
|
||||
<p><strong>${htmlEscape(v.clientName)}</strong> is requesting access to your HealthLog data.</p>
|
||||
${provenance}
|
||||
@@ -367,11 +398,13 @@ export async function GET(request: NextRequest): Promise<Response> {
|
||||
<button type="submit" name="decision" value="deny">Deny</button>
|
||||
</form>
|
||||
</main>`,
|
||||
),
|
||||
v.redirectUri,
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
export async function POST(request: NextRequest): Promise<Response> {
|
||||
async function authorizePost(request: NextRequest): Promise<Response> {
|
||||
return withBackgroundEvent("mcp.oauth.authorize", async () => {
|
||||
if (!(await surfaceAvailable())) {
|
||||
return Response.json(
|
||||
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
});
|
||||
}
|
||||
+12
-1
@@ -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
|
||||
|
||||
Reference in new issue
Block a user