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(