From 8ce69e9d2b171d173fe4b6b2b6256f1f4411e69d Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 1 Oct 2026 02:05:35 +0200 Subject: [PATCH] fix(mcp): treat empty and null optional OAuth fields as absent Empty or null optional fields in OAuth token and registration responses (scope, refresh_token, id_token, client_secret) failed sign-in, and expires_in: null marked tokens as expired. Also fall back to the server origin when resource metadata has invalid URLs, skip empty scopes when selecting the requested scope, and end pagination at an empty or null nextCursor. closes #10266 --- packages/coding-agent/CHANGELOG.md | 1 + packages/mcp/CHANGELOG.md | 8 +++++++ packages/mcp/src/client.ts | 8 +++---- packages/mcp/src/oauth/discovery.ts | 3 ++- packages/mcp/src/oauth/flow.ts | 3 ++- packages/mcp/src/oauth/types.ts | 16 ++++++++++---- packages/mcp/test/client.test.ts | 2 ++ packages/mcp/test/oauth.test.ts | 34 ++++++++++++++++++++++++----- 8 files changed, 60 insertions(+), 15 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 246989886..c828b49d2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -9,6 +9,7 @@ ### Fixed - Fixed MCP OAuth sign-in accepting an authorization response whose `iss` parameter names another authorization server; the code is now rejected before it is exchanged (RFC 9207). +- Fixed MCP OAuth sign-in failing with `Invalid scope` when the token response contains `"scope": ""`, and similar failures for other empty or `null` optional OAuth fields ([#10266](https://github.com/earendil-works/pi/issues/10266)). ## [0.99.2] - 2026-09-30 diff --git a/packages/mcp/CHANGELOG.md b/packages/mcp/CHANGELOG.md index 21cb8987e..749c81488 100644 --- a/packages/mcp/CHANGELOG.md +++ b/packages/mcp/CHANGELOG.md @@ -7,6 +7,14 @@ - Added an `authorizationServerMetadataUrl` option to `authorizeMcp()` to use a configured authorization server metadata document instead of discovery ([#10172](https://github.com/earendil-works/pi/issues/10172)). - Added an `iss` option to `authorizeMcp()`. The authorization code is only exchanged when `iss` names the flow's authorization server, or is absent and the server's metadata does not set `authorization_response_iss_parameter_supported` (RFC 9207). +### Fixed + +- Fixed OAuth token and client registration responses with an empty or `null` optional field (such as `"scope": ""`) failing with `Invalid scope` and similar errors; such fields are now treated as absent ([#10266](https://github.com/earendil-works/pi/issues/10266)). +- Fixed `expires_in: null` in a token response marking the access token as already expired. +- Fixed invalid URLs in protected resource metadata aborting OAuth discovery instead of falling back to the MCP server's origin. +- Fixed an empty requested scope, from `scope=""` in a `WWW-Authenticate` challenge or an empty `scopes_supported`, overriding the next scope source. +- Fixed list pagination failing with a duplicate cursor error when a server ends pagination with `nextCursor: ""` or `null`. + ## [0.99.2] - 2026-09-30 ### Fixed diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index f8f3d7b79..6199ed682 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -100,10 +100,10 @@ function validateListPage( for (const item of items) { if (!isObject(item) || !isItem(item)) throw invalid(`Invalid entry in MCP ${method} result`); } - if (value.nextCursor !== undefined && typeof value.nextCursor !== "string") { - throw invalid(`Invalid MCP ${method} cursor`); - } - return { items, ...(value.nextCursor === undefined ? {} : { nextCursor: value.nextCursor }) }; + // Some servers end pagination with `null` or `""` instead of omitting the cursor. + const nextCursor = value.nextCursor === null || value.nextCursor === "" ? undefined : value.nextCursor; + if (nextCursor !== undefined && typeof nextCursor !== "string") throw invalid(`Invalid MCP ${method} cursor`); + return { items, ...(nextCursor === undefined ? {} : { nextCursor }) }; } const isTool = (tool: Record) => typeof tool.name === "string" && isObject(tool.inputSchema); diff --git a/packages/mcp/src/oauth/discovery.ts b/packages/mcp/src/oauth/discovery.ts index b16b73ffc..e8d3030a4 100644 --- a/packages/mcp/src/oauth/discovery.ts +++ b/packages/mcp/src/oauth/discovery.ts @@ -32,7 +32,8 @@ function pathSuffix(pathname: string): string { function field(header: string, name: string): string | undefined { const match = header.match(new RegExp(String.raw`(?:^|[,\s])${name}=(?:"([^"]*)"|([^\s,]+))`, "i")); - return match?.[1] ?? match?.[2]; + // An empty value (`scope=""`) carries no information, so it counts as absent. + return match?.[1] || match?.[2] || undefined; } export function parseWwwAuthenticate(header: string | null): OAuthChallenge { diff --git a/packages/mcp/src/oauth/flow.ts b/packages/mcp/src/oauth/flow.ts index 60e51ff44..c92766c87 100644 --- a/packages/mcp/src/oauth/flow.ts +++ b/packages/mcp/src/oauth/flow.ts @@ -292,8 +292,9 @@ async function runFlow(provider: OAuthClientProvider, options: OAuthFlowOptions) } const metadata = discovered.authorizationServerMetadata; const resource = selectResource(options.serverUrl, discovered.resourceMetadata); + // `||`, not `??`: an empty scope (for example from `scopes_supported: []`) falls through to the next source. const scope = - options.scope ?? discovered.resourceMetadata?.scopes_supported?.join(" ") ?? provider.clientMetadata.scope; + options.scope || discovered.resourceMetadata?.scopes_supported?.join(" ") || provider.clientMetadata.scope; let client = await provider.clientInformation(); if (!client) { if (options.authorizationCode) throw new Error("OAuth client information is missing during code exchange"); diff --git a/packages/mcp/src/oauth/types.ts b/packages/mcp/src/oauth/types.ts index 1dfc127ab..d7d83da56 100644 --- a/packages/mcp/src/oauth/types.ts +++ b/packages/mcp/src/oauth/types.ts @@ -102,26 +102,33 @@ function requiredString(value: unknown, name: string): string { return value; } +/** Treats `null` and `""` as absent: servers send them for fields they have no value for, like `scope: ""`. */ +function absent(value: unknown): value is undefined | null | "" { + return value === undefined || value === null || value === ""; +} + function optionalString(value: unknown, name: string): string | undefined { - if (value === undefined) return undefined; + if (absent(value)) return undefined; return requiredString(value, name); } function optionalStrings(value: unknown, name: string): string[] | undefined { - if (value === undefined) return undefined; + if (value === undefined || value === null) return undefined; if (!Array.isArray(value) || value.some((item) => typeof item !== "string")) throw new Error(`Invalid ${name}`); return [...value]; } function safeUrl(value: unknown, name: string): string { const text = requiredString(value, name); + // URL parsing throws a `TypeError`, which discovery reserves for network failures. + if (!URL.canParse(text)) throw new Error(`Invalid ${name}`); const url = new URL(text); if (["javascript:", "data:", "vbscript:"].includes(url.protocol)) throw new Error(`Invalid ${name}`); return text; } function optionalUrl(value: unknown, name: string): string | undefined { - return value === undefined ? undefined : safeUrl(value, name); + return absent(value) ? undefined : safeUrl(value, name); } export function parseProtectedResourceMetadata(value: unknown): OAuthProtectedResourceMetadata { @@ -170,7 +177,8 @@ export function parseAuthorizationServerMetadata(value: unknown): AuthorizationS export function parseOAuthTokens(value: unknown): OAuthTokens { const input = object(value, "OAuth token response"); - const expires = input.expires_in === undefined ? undefined : Number(input.expires_in); + // `Number(null)` is 0, which would mark the token as expired at once. + const expires = absent(input.expires_in) ? undefined : Number(input.expires_in); if (expires !== undefined && !Number.isFinite(expires)) throw new Error("Invalid expires_in"); return compact({ access_token: requiredString(input.access_token, "access_token"), diff --git a/packages/mcp/test/client.test.ts b/packages/mcp/test/client.test.ts index e5dbd8932..9213da6c8 100644 --- a/packages/mcp/test/client.test.ts +++ b/packages/mcp/test/client.test.ts @@ -109,6 +109,8 @@ describe("McpClient", () => { annotations: { readOnlyHint: true }, }, ], + // Some servers end pagination with an empty cursor instead of omitting it. + nextCursor: "", }; }); expect(await client.listTools()).toEqual([ diff --git a/packages/mcp/test/oauth.test.ts b/packages/mcp/test/oauth.test.ts index 9c350fb0a..9b9b7eedb 100644 --- a/packages/mcp/test/oauth.test.ts +++ b/packages/mcp/test/oauth.test.ts @@ -126,7 +126,8 @@ describe("MCP OAuth", () => { if (url.pathname === "/register") { const metadata = JSON.parse(await readBody(request)) as Record; response.writeHead(201, { "content-type": "application/json" }); - response.end(JSON.stringify({ ...metadata, client_id: "test-client" })); + // Empty and null optional fields count as absent (#10266). + response.end(JSON.stringify({ ...metadata, client_id: "test-client", client_secret: "" })); return; } if (url.pathname === "/authorize") { @@ -142,7 +143,14 @@ describe("MCP OAuth", () => { if (params.get("grant_type") === "refresh_token") { refreshes++; response.setHeader("content-type", "application/json"); - response.end(JSON.stringify({ access_token: "refreshed-token", token_type: "Bearer" })); + response.end( + JSON.stringify({ + access_token: "refreshed-token", + token_type: "Bearer", + refresh_token: "", + expires_in: null, + }), + ); return; } const challenge = createHash("sha256") @@ -155,7 +163,12 @@ describe("MCP OAuth", () => { } response.setHeader("content-type", "application/json"); response.end( - JSON.stringify({ access_token: "first-token", refresh_token: "refresh-token", token_type: "Bearer" }), + JSON.stringify({ + access_token: "first-token", + refresh_token: "refresh-token", + token_type: "Bearer", + scope: "", + }), ); return; } @@ -178,7 +191,8 @@ describe("MCP OAuth", () => { if (token !== "Bearer first-token" && token !== "Bearer refreshed-token") { await readBody(request); response.writeHead(401, { - "www-authenticate": `Bearer resource_metadata="${serverOrigin}/.well-known/oauth-protected-resource/mcp", scope="org:read"`, + // An empty scope falls through to the resource metadata's scopes_supported. + "www-authenticate": `Bearer resource_metadata="${serverOrigin}/.well-known/oauth-protected-resource/mcp", scope=""`, }); response.end("Unauthorized"); return; @@ -251,7 +265,11 @@ describe("MCP OAuth", () => { openGetStream: false, }), ); - expect(provider.tokenSet?.access_token).toBe("refreshed-token"); + expect(provider.tokenSet).toEqual({ + access_token: "refreshed-token", + refresh_token: "refresh-token", + token_type: "Bearer", + }); expect(refreshes).toBe(1); await refreshedClient.close(); await callback.close(); @@ -261,6 +279,12 @@ describe("MCP OAuth", () => { const grants: string[] = []; const origin = await listen(async (request, response, serverOrigin) => { const url = new URL(request.url ?? "/", serverOrigin); + if (url.pathname === "/.well-known/oauth-protected-resource/mcp") { + // Invalid resource metadata falls back to the server origin instead of failing discovery. + response.setHeader("content-type", "application/json"); + response.end(JSON.stringify({ resource: `${serverOrigin}/mcp`, authorization_servers: ["not a url"] })); + return; + } if (url.pathname === "/.well-known/oauth-authorization-server") { response.setHeader("content-type", "application/json"); response.end(