From 797e122cb1181931e6ec1fe2369c7c17dbf1fc9e Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Tue, 3 Nov 2026 11:45:12 +0100 Subject: [PATCH] fix(mcp): refresh with the app's redirect URI, cancel sign-in during the lock wait Tokens from a Client ID Metadata Document sign-in through an https callback route were refreshed with the loopback redirect URI, which the documents do not list, so every expiry needed a browser sign-in. createMcpAuthProvider takes the app's redirectUrl and startMcpSignIn passes its own. startMcpSignIn's wait for the refresh lock now ends when the signal aborts. Only McpRefreshLockBusyError (coding-agent maps proper-lockfile's ELOCKED to it) falls back to the browser step; other lock errors fail the sign-in. --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/extensions/mcp/oauth.ts | 33 ++++++---- .../test/mcp-oauth-refresh.test.ts | 47 +++++++++++++- packages/mcp/CHANGELOG.md | 2 +- packages/mcp/src/oauth/auth-provider.ts | 32 +++++++--- packages/mcp/src/oauth/client-settings.ts | 4 +- packages/mcp/src/oauth/errors.ts | 11 ++++ packages/mcp/src/oauth/index.ts | 1 + packages/mcp/src/oauth/sign-in.ts | 56 ++++++++++++----- packages/mcp/test/auth-provider.test.ts | 26 ++++++++ packages/mcp/test/authorization-server.ts | 37 +++++++---- packages/mcp/test/sign-in.test.ts | 62 ++++++++++++++++++- 12 files changed, 257 insertions(+), 56 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ac04acaea..8247666ad 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -9,7 +9,7 @@ ### Fixed -- A failed token refresh of an MCP connection no longer registers an OAuth client or replaces the PKCE verifier of a sign-in in progress, which made that sign-in fail. MCP sign-in now refreshes the stored tokens with the stored client even when the loopback callback port changed, instead of registering a new client and opening the browser. +- A failed token refresh of an MCP connection no longer registers an OAuth client or replaces the PKCE verifier of a sign-in in progress, which made that sign-in fail. MCP sign-in now refreshes the stored tokens with the stored client even when the loopback callback port changed, instead of registering a new client and opening the browser. Ctrl+C in `/mcp` and the `--timeout` of `pi mcp login` now also stop a sign-in while it waits up to 25 seconds for another pi process's token refresh, and a refresh lock that fails for another reason than that wait now fails the sign-in instead of opening the browser. ## [1.5.0] - 2026-11-02 diff --git a/packages/coding-agent/src/extensions/mcp/oauth.ts b/packages/coding-agent/src/extensions/mcp/oauth.ts index b38865282..5a6aba3cd 100644 --- a/packages/coding-agent/src/extensions/mcp/oauth.ts +++ b/packages/coding-agent/src/extensions/mcp/oauth.ts @@ -24,6 +24,7 @@ import { type McpOAuthServerStore, type McpOAuthSettings, type McpOAuthState, + McpRefreshLockBusyError, type OAuthCallbackPage, type OAuthChallenge, startMcpSignIn, @@ -95,23 +96,31 @@ export class McpOAuthCredentialStore { /** * A lock file per server. When the process exits, proper-lockfile removes the locks it holds; when * it is killed, the lock goes stale because it is no longer renewed, and the next process takes it over. + * Throws `McpRefreshLockBusyError` when another process holds the lock for the whole wait. */ private async withRefreshLock(key: string, fn: () => Promise): Promise { if (!this.lockDir) return fn(); mkdirSync(this.lockDir, { recursive: true, mode: 0o700 }); const hash = createHash("sha256").update(key).digest("hex").slice(0, 16); - const release = await lockfile.lock(join(this.lockDir, `mcp-auth-refresh-${hash}`), { - realpath: false, - stale: REFRESH_LOCK_STALE_MS, - retries: { - retries: REFRESH_LOCK_WAIT_MS / REFRESH_LOCK_RETRY_MS, - factor: 1, - minTimeout: REFRESH_LOCK_RETRY_MS, - maxTimeout: REFRESH_LOCK_RETRY_MS, - }, - // The default throws from a timer. A lost lock at worst lets two refreshes overlap. - onCompromised: () => {}, - }); + const release = await lockfile + .lock(join(this.lockDir, `mcp-auth-refresh-${hash}`), { + realpath: false, + stale: REFRESH_LOCK_STALE_MS, + retries: { + retries: REFRESH_LOCK_WAIT_MS / REFRESH_LOCK_RETRY_MS, + factor: 1, + minTimeout: REFRESH_LOCK_RETRY_MS, + maxTimeout: REFRESH_LOCK_RETRY_MS, + }, + // The default throws from a timer. A lost lock at worst lets two refreshes overlap. + onCompromised: () => {}, + }) + .catch((error: unknown) => { + // proper-lockfile reports a lock held by another process as `ELOCKED`. + if (error instanceof Error && "code" in error && error.code === "ELOCKED") + throw new McpRefreshLockBusyError(); + throw error; + }); try { return await fn(); } finally { diff --git a/packages/coding-agent/test/mcp-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-oauth-refresh.test.ts index 4b7c8fe7c..a4e1768e4 100644 --- a/packages/coding-agent/test/mcp-oauth-refresh.test.ts +++ b/packages/coding-agent/test/mcp-oauth-refresh.test.ts @@ -2,7 +2,13 @@ import { createHash } from "node:crypto"; import { mkdtempSync, readdirSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { type McpOAuthSettings, type OAuthChallenge, OAuthIssuerMismatchError } from "@earendil-works/pi-mcp/oauth"; +import { + type McpOAuthServerStore, + type McpOAuthSettings, + McpSignInCancelledError, + type OAuthChallenge, + OAuthIssuerMismatchError, +} from "@earendil-works/pi-mcp/oauth"; import { afterEach, describe, expect, it } from "vitest"; import { InMemoryAuthStorageBackend } from "../src/core/auth-storage.ts"; import { @@ -73,6 +79,45 @@ describe("MCP OAuth refresh", () => { expect(readdirSync(lockDir)).toEqual([]); }); + it("cancels a sign-in that waits for another process's refresh", async () => { + const { server, lockDir, process } = await signedIn(); + let release = () => {}; + const held = process().store.withRefreshLock( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + // The sign-in's store, recording its wait for the lock. + const store = process().store; + let waiting: Promise | undefined; + const recording: McpOAuthServerStore = { + ...store, + withRefreshLock: (fn) => { + const run = store.withRefreshLock(fn); + waiting = run; + return run; + }, + }; + const controller = new AbortController(); + const signIn = signInMcpServer({ + serverUrl: server.url, + store: recording, + settings: {}, + prompt: { showAuthorizationUrl: () => {}, promptForRedirectUrl: async () => undefined }, + signal: controller.signal, + }); + setTimeout(() => controller.abort(), 200); + await expect(signIn).rejects.toBeInstanceOf(McpSignInCancelledError); + + release(); + await held; + // The cancelled sign-in takes the lock once it is free and releases it without refreshing. + await waiting; + expect(server.log.filter((entry) => entry === "token refresh")).toHaveLength(0); + expect(readdirSync(lockDir)).toEqual([]); + }); + it("waits for a running refresh to save the new tokens", async () => { const { server, process } = await signedIn(); server.expireAccessTokens(); diff --git a/packages/mcp/CHANGELOG.md b/packages/mcp/CHANGELOG.md index ece49661a..81327c170 100644 --- a/packages/mcp/CHANGELOG.md +++ b/packages/mcp/CHANGELOG.md @@ -5,7 +5,7 @@ ### Added - Added `PipeTransport`, MCP's stdio transport over a program started by the caller through an `McpPipe` (`write`, `closeStdin`, `close`, `exited`), for example `@earendil-works/pi-env`'s `openProcess()` on another machine. `StdioTransport` and `PipeTransport` share the newline-delimited framing and the stderr tail. -- Added MCP OAuth sign-in in two halves (`@earendil-works/pi-mcp/oauth`), moved from pi's coding agent: `startMcpSignIn()` refreshes the stored tokens or returns the authorization URL, and `completeMcpSignIn()` exchanges the code later, possibly in another request with the same state store. The code comes from a loopback callback server (`listenForLoopbackSignIn()`, `waitForLoopbackSignIn()`), the application's own callback route, or a pasted redirect URL (`responseFromRedirectUrl()`). Client ID Metadata Documents (`clientMetadataDocuments`) and the client name are the caller's. Also added `createMcpAuthProvider()`, which refreshes tokens once across requests and, through `McpOAuthServerStore.withRefreshLock()`, across processes, and the `refreshOnly` option of `authorizeMcp()`, which refreshes without registering a client or replacing a pending authorization's `state` and PKCE verifier. +- Added MCP OAuth sign-in in two halves (`@earendil-works/pi-mcp/oauth`), moved from pi's coding agent: `startMcpSignIn()` refreshes the stored tokens or returns the authorization URL, and `completeMcpSignIn()` exchanges the code later, possibly in another request with the same state store. The code comes from a loopback callback server (`listenForLoopbackSignIn()`, `waitForLoopbackSignIn()`), the application's own callback route, or a pasted redirect URL (`responseFromRedirectUrl()`). Client ID Metadata Documents (`clientMetadataDocuments`) and the client name are the caller's. Aborting a sign-in's `signal` also stops `startMcpSignIn()` while it waits for the refresh lock; a store's `withRefreshLock()` throws `McpRefreshLockBusyError` when another process holds the lock too long, which makes `startMcpSignIn()` skip the refresh, while any other lock failure fails the sign-in. Also added `createMcpAuthProvider()`, which refreshes tokens once across requests and, through `McpOAuthServerStore.withRefreshLock()`, across processes; its `redirectUrl` option names a redirect URI the application signs in with, so tokens from a Client ID Metadata Document that lists only an https callback route can be refreshed. Also added the `refreshOnly` option of `authorizeMcp()`, which refreshes without registering a client or replacing a pending authorization's `state` and PKCE verifier. - Added helpers that let applications name and configure MCP tools like pi's coding agent, moved from it: `createMcpToolName()` (`mcp____`, sanitized, with a hash suffix for long or colliding names) and `mcpNamespace()`; `normalizeMcpInputSchema()`, which makes an input schema an object schema; `limitMcpContent()` and `MCP_OUTPUT_MAX_BYTES`, which cut model-facing text over 20KB in the middle and optionally save the full text through a caller's `McpOutputSaver`; and `validateMcpServerConfig()`, which validates one entry of the `mcpServers` configuration shape (stdio, Streamable HTTP, `oauth`, `enabled`, `description`, `timeout`), returns the entry tagged with its transport, and keeps the application's own fields for it to check afterwards. `isMcpServerOverride()` recognizes entries that only override a server defined elsewhere. ## [1.5.0] - 2026-11-02 diff --git a/packages/mcp/src/oauth/auth-provider.ts b/packages/mcp/src/oauth/auth-provider.ts index 53cfe8b17..91b66f170 100644 --- a/packages/mcp/src/oauth/auth-provider.ts +++ b/packages/mcp/src/oauth/auth-provider.ts @@ -46,9 +46,15 @@ export function createMcpAuthProvider( options: Omit & { settings: () => McpOAuthSettings; onChallenge: (challenge: OAuthChallenge) => void; + /** + * A redirect URI the application signs in with, such as its https callback route. Refreshing tokens + * from a sign-in with a Client ID Metadata Document needs a redirect URI the documents list; a + * refresh never redirects, so any listed one works. Default: the loopback redirect URI of `settings`. + */ + redirectUrl?: string; }, ): McpAuthProvider { - const { store } = options; + const { store, redirectUrl } = options; let refreshing: Promise | undefined; /** Replace `staleToken`, the access token that expired or was rejected. */ @@ -59,7 +65,7 @@ export function createMcpAuthProvider( if (state?.tokens?.access_token !== staleToken) return; if (!state?.tokens?.refresh_token) throw new McpOAuthAuthorizationRequiredError(); const client = { ...options, settings: options.settings() }; - const result = await refreshStoredTokens(client, state, { fetch, challenge }); + const result = await refreshStoredTokens(client, state, { fetch, challenge, redirectUrl }); if (result === "REDIRECT") throw new McpOAuthAuthorizationRequiredError(); }) .finally(() => { @@ -94,20 +100,26 @@ export function createMcpAuthProvider( /** * Refresh the tokens of `state` with the stored client (or the document it signed in with), or report - * `REDIRECT` when that is not possible. Never registers a client or prepares an authorization request, - * so a pending sign-in stays usable. Call while holding the store's refresh lock. Not cancellable: a - * refresh the server answered may have rotated the refresh token, so its answer must be saved. + * `REDIRECT` when that is not possible. `options.redirectUrl` is a redirect URI the application signs in + * with, tried before the loopback ones of the settings and the stored client. Never registers a client or + * prepares an authorization request, so a pending sign-in stays usable. Call while holding the store's + * refresh lock. Not cancellable: a refresh the server answered may have rotated the refresh token, so its + * answer must be saved. */ export async function refreshStoredTokens( client: McpOAuthClientOptions, state: McpOAuthState, - options: { fetch?: McpFetch; challenge?: OAuthChallenge } = {}, + options: { fetch?: McpFetch; challenge?: OAuthChallenge; redirectUrl?: string } = {}, ): Promise { - // Refreshing never redirects the user; the redirect URI only picks the client metadata. + // Refreshing never redirects the user; the redirect URI only decides whether tokens without a stored + // client can be refreshed with a Client ID Metadata Document, so any URI the documents list works. + const candidates = [ + options.redirectUrl, + loopbackSettings(client.settings).fixedRedirectUrl, + registeredRedirectUrls(state.clientInformation)[0], + ].filter((url) => url !== undefined); const redirectUrl = - loopbackSettings(client.settings).fixedRedirectUrl ?? - registeredRedirectUrls(state.clientInformation)[0] ?? - DEFAULT_REDIRECT_URL; + candidates.find((url) => client.clientMetadataDocuments?.listsRedirectUrl(url)) ?? DEFAULT_REDIRECT_URL; return authorizeMcp(createOAuthClientProvider(client, redirectUrl), { serverUrl: client.serverUrl, resourceMetadataUrl: options.challenge?.resourceMetadataUrl, diff --git a/packages/mcp/src/oauth/client-settings.ts b/packages/mcp/src/oauth/client-settings.ts index e20083d49..c8b042d3a 100644 --- a/packages/mcp/src/oauth/client-settings.ts +++ b/packages/mcp/src/oauth/client-settings.ts @@ -64,7 +64,9 @@ export interface McpClientMetadataDocuments { export interface McpOAuthServerStore extends McpOAuthStateStore { /** * Run `fn` while no other process refreshes the server's tokens. Not reentrant: `startMcpSignIn` and - * `createMcpAuthProvider` take it themselves, so never call them from `fn`. + * `createMcpAuthProvider` take it themselves, so never call them from `fn`. Throw + * `McpRefreshLockBusyError` when another process holds the lock for the whole wait: `startMcpSignIn` + * then goes to the browser step. Any other error fails the sign-in or refresh. */ withRefreshLock(fn: () => Promise): Promise; } diff --git a/packages/mcp/src/oauth/errors.ts b/packages/mcp/src/oauth/errors.ts index 4e625e10d..d2ff2347d 100644 --- a/packages/mcp/src/oauth/errors.ts +++ b/packages/mcp/src/oauth/errors.ts @@ -53,3 +53,14 @@ export class McpOAuthAuthorizationRequiredError extends Error { this.name = "McpOAuthAuthorizationRequiredError"; } } + +/** + * Thrown by `McpOAuthServerStore.withRefreshLock()` when another process holds the refresh lock for the + * whole wait. `startMcpSignIn()` then skips the refresh and goes to the browser step. + */ +export class McpRefreshLockBusyError extends Error { + constructor() { + super("Another process is refreshing the MCP server's tokens"); + this.name = "McpRefreshLockBusyError"; + } +} diff --git a/packages/mcp/src/oauth/index.ts b/packages/mcp/src/oauth/index.ts index 6ef8e18ec..6e9242c40 100644 --- a/packages/mcp/src/oauth/index.ts +++ b/packages/mcp/src/oauth/index.ts @@ -22,6 +22,7 @@ export { } from "./discovery.ts"; export { McpOAuthAuthorizationRequiredError, + McpRefreshLockBusyError, OAuthError, OAuthInsecureEndpointError, OAuthIssuerMismatchError, diff --git a/packages/mcp/src/oauth/sign-in.ts b/packages/mcp/src/oauth/sign-in.ts index 2299dcd99..ffae985ef 100644 --- a/packages/mcp/src/oauth/sign-in.ts +++ b/packages/mcp/src/oauth/sign-in.ts @@ -15,6 +15,7 @@ import { registeredRedirectUrls, timedFetch, } from "./client-settings.ts"; +import { McpRefreshLockBusyError } from "./errors.ts"; import { authorizeMcp, type OAuthFlowOptions, stepUpScope } from "./flow.ts"; import type { McpOAuthState } from "./provider.ts"; import type { OAuthChallenge } from "./types.ts"; @@ -29,7 +30,10 @@ export interface McpSignInOptions extends McpOAuthClientOptions { redirectUrl: string; /** The server's challenge that asked for the sign-in; its resource metadata URL and scope apply. */ challenge?: OAuthChallenge; - /** Aborting stops the sign-in with `McpSignInCancelledError`. A refresh already sent still completes. */ + /** + * Aborting stops the sign-in at once with `McpSignInCancelledError`, also while `startMcpSignIn` waits + * for the store's refresh lock. A refresh already sent still completes and saves its tokens. + */ signal?: AbortSignal; } @@ -63,10 +67,11 @@ export class McpSignInCancelledError extends Error { /** * First half of a sign-in. Without a step-up (`insufficient_scope`), it first tries to refresh the stored * tokens with the stored client, whatever `redirectUrl` is (`authorized`). The refresh takes the store's - * refresh lock, so never call this while holding it; when the lock cannot be taken (for example it timed - * out), the refresh is skipped. Otherwise it prepares the browser step (`redirect`): a fresh `state`, and a - * stored client that cannot use `redirectUrl` is dropped with its tokens. Requests to the authorization - * server are time-limited. + * refresh lock, so never call this while holding it; when another process holds the lock for the whole + * wait (`McpRefreshLockBusyError`), the refresh is skipped, and any other failure of the lock fails the + * sign-in. Otherwise it prepares the browser step (`redirect`): a fresh `state`, and a stored client that + * cannot use `redirectUrl` is dropped with its tokens. Requests to the authorization server are + * time-limited. */ export async function startMcpSignIn(options: McpSignInOptions): Promise { if (options.signal?.aborted) throw new McpSignInCancelledError(); @@ -76,25 +81,42 @@ export async function startMcpSignIn(options: McpSignInOptions): Promise startBrowserStep(options, stepUp)); } -/** Refresh the stored tokens, unless `cimd` is to replace a registered client. */ +/** + * Refresh the stored tokens, unless `cimd` is to replace a registered client. The refresh uses + * `redirectUrl` when the tokens came from a Client ID Metadata Document whose documents list it. + */ async function refreshBeforeSignIn(options: McpSignInOptions): Promise<"AUTHORIZED" | "REDIRECT"> { - const { store, settings, challenge } = options; - let locked = false; + const { store, settings, challenge, redirectUrl, signal } = options; + const refreshed = store.withRefreshLock(async () => { + // The lock may arrive after the sign-in was cancelled: return at once, so it is released. + if (signal?.aborted) return "REDIRECT"; + const state = await store.load(); + if (!state?.tokens?.refresh_token) return "REDIRECT"; + if (settings.clientRegistration === "cimd" && state.clientInformation && !settings.clientId) return "REDIRECT"; + return refreshStoredTokens(options, state, { challenge, redirectUrl }); + }); try { - return await store.withRefreshLock(async () => { - locked = true; - const state = await store.load(); - if (!state?.tokens?.refresh_token) return "REDIRECT"; - if (settings.clientRegistration === "cimd" && state.clientInformation && !settings.clientId) return "REDIRECT"; - return refreshStoredTokens(options, state, { challenge }); - }); + return await untilAborted(signal, refreshed); } catch (error) { - // The lock's own failure: another process holds it too long. The user signs in instead. - if (!locked) return "REDIRECT"; + // Another process is refreshing and took too long. The user signs in instead. + if (error instanceof McpRefreshLockBusyError) return "REDIRECT"; throw error; } } +/** + * `promise`, or `McpSignInCancelledError` as soon as `signal` aborts. `promise` keeps running: a refresh + * the server answered must save its tokens. + */ +function untilAborted(signal: AbortSignal | undefined, promise: Promise): Promise { + if (!signal) return promise; + return new Promise((resolve, reject) => { + const abort = () => reject(new McpSignInCancelledError()); + signal.addEventListener("abort", abort, { once: true }); + promise.then(resolve, reject).finally(() => signal.removeEventListener("abort", abort)); + }); +} + async function startBrowserStep(options: McpSignInOptions, stepUp: boolean): Promise { const { store, settings, redirectUrl, challenge } = options; const stored = await store.load(); diff --git a/packages/mcp/test/auth-provider.test.ts b/packages/mcp/test/auth-provider.test.ts index df20bd813..bc3d94da2 100644 --- a/packages/mcp/test/auth-provider.test.ts +++ b/packages/mcp/test/auth-provider.test.ts @@ -94,6 +94,32 @@ describe("createMcpAuthProvider", () => { expect(server.authorizations).toHaveLength(1); }); + it.each(["auto", "cimd"] as const)( + "refreshes the tokens of a document that lists only the redirect URI passed (%s)", + async (clientRegistration) => { + const server = await startAuthorizationServer({ cimd: true, issParameter: true }); + const options = { serverUrl: server.serverUrl, store: memoryStore(), redirectUrl: ROUTE, clientName: "app" }; + const clientMetadataDocuments = { + baseUrl: "https://app.example/oauth", + listsRedirectUrl: (redirectUrl: string) => redirectUrl === ROUTE, + }; + const settings = { clientRegistration }; + await signIn({ ...options, settings, clientMetadataDocuments }); + expect((await options.store.load())?.clientInformation).toBeUndefined(); + + const provider = createMcpAuthProvider({ + ...options, + clientMetadataDocuments, + settings: () => settings, + onChallenge: () => {}, + }); + await provider.onUnauthorized?.(unauthorized(server, "access-1")); + expect(await provider.token()).toBe("access-2"); + expect(server.tokenRequests[1].get("client_id")).toBe("https://app.example/oauth/client.json"); + expect(server.authorizations).toHaveLength(1); + }, + ); + /** Signed in, with a step-up sign-in waiting for the browser, which keeps the tokens meanwhile. */ async function pendingSignIn() { const server = await startAuthorizationServer(); diff --git a/packages/mcp/test/authorization-server.ts b/packages/mcp/test/authorization-server.ts index 1d14a0042..3ed0a2bfa 100644 --- a/packages/mcp/test/authorization-server.ts +++ b/packages/mcp/test/authorization-server.ts @@ -117,29 +117,44 @@ export function memoryStore(): McpOAuthServerStore { /** * An in-memory store that records whether each save happened while the refresh lock was held. With - * `lockFails`, taking the lock fails like a lock that timed out. + * `lockError`, taking the lock fails with it; `hold()` stands for another process holding the lock until + * the returned function releases it. `lockRuns` are the calls of `withRefreshLock`. */ -export function recordingStore(options: { lockFails?: boolean } = {}) { +export function recordingStore(options: { lockError?: Error } = {}) { const memory = new MemoryOAuthStateStore(); const saves: { locked: boolean; accessToken: string | undefined }[] = []; + const lockRuns: Promise[] = []; let locked = false; + let held: Promise | undefined; const store: McpOAuthServerStore = { load: () => memory.load(), save: (state) => { saves.push({ locked, accessToken: state.tokens?.access_token }); memory.save(state); }, - withRefreshLock: async (fn) => { - if (options.lockFails) throw new Error("Lock file is already being held"); - locked = true; - try { - return await fn(); - } finally { - locked = false; - } + withRefreshLock: (fn) => { + const run = (async () => { + await held; + if (options.lockError) throw options.lockError; + locked = true; + try { + return await fn(); + } finally { + locked = false; + } + })(); + lockRuns.push(run.catch(() => undefined)); + return run; }, }; - return { store, saves }; + const hold = () => { + let release = () => {}; + held = new Promise((resolve) => { + release = resolve; + }); + return release; + }; + return { store, saves, lockRuns, hold, isLocked: () => locked }; } /** Follows the authorization URL like a browser, up to the redirect back to the application. */ diff --git a/packages/mcp/test/sign-in.test.ts b/packages/mcp/test/sign-in.test.ts index f5a6d7bd1..9e0c62f5e 100644 --- a/packages/mcp/test/sign-in.test.ts +++ b/packages/mcp/test/sign-in.test.ts @@ -5,6 +5,7 @@ import { type McpAuthorizationResponse, type McpClientMetadataDocuments, type McpOAuthSettings, + McpRefreshLockBusyError, McpSignInCancelledError, type McpSignInOptions, type McpSignInRedirect, @@ -118,15 +119,27 @@ describe("start and complete", () => { ]); }); - it("goes to the browser step when the refresh lock cannot be taken", async () => { + it("goes to the browser step when another process holds the refresh lock for too long", async () => { const server = await startAuthorizationServer(); - const options = signInOptions(server, { store: recordingStore({ lockFails: true }).store }); + const options = signInOptions(server, { + store: recordingStore({ lockError: new McpRefreshLockBusyError() }).store, + }); await signIn(options); expect((await startMcpSignIn(options)).kind).toBe("redirect"); expect(server.tokenRequests.map((request) => request.get("grant_type"))).toEqual(["authorization_code"]); }); + it("fails when the refresh lock fails for another reason", async () => { + const server = await startAuthorizationServer(); + const lockError = new Error("EACCES: permission denied"); + const options = signInOptions(server, { store: recordingStore({ lockError }).store }); + + await expect(startMcpSignIn(options)).rejects.toBe(lockError); + expect(server.registrations).toEqual([]); + expect(server.authorizations).toEqual([]); + }); + it("refuses to complete with another redirect URI than the authorization request", async () => { const server = await startAuthorizationServer({ cimd: true }); const options = signInOptions(server, { redirectUrl: LOOPBACK, clientMetadataDocuments: documents }); @@ -296,6 +309,31 @@ describe("client metadata documents", () => { expect(server.registrations).toMatchObject([{ client_name: "app" }, { client_name: "Other" }]); }); + /** Documents that list only the application's https callback route. */ + const routeDocuments: McpClientMetadataDocuments = { + baseUrl: "https://app.example/oauth", + listsRedirectUrl: (redirectUrl) => redirectUrl === ROUTE, + }; + + it.each(["auto", "cimd"] as const)( + "refreshes the tokens of a document that lists only an https route (%s)", + async (clientRegistration) => { + const server = await startAuthorizationServer({ cimd: true, issParameter: true }); + const options = signInOptions(server, { clientMetadataDocuments: routeDocuments }, { clientRegistration }); + const authorization = await signIn(options); + expect(new URL(authorization.authorizationUrl).searchParams.get("client_id")).toBe( + "https://app.example/oauth/client.json", + ); + expect((await options.store.load())?.clientInformation).toBeUndefined(); + + expect(await startMcpSignIn(options)).toEqual({ kind: "authorized" }); + expect(server.authorizations).toHaveLength(1); + expect(server.tokenRequests[1].get("grant_type")).toBe("refresh_token"); + expect(server.tokenRequests[1].get("client_id")).toBe("https://app.example/oauth/client.json"); + expect((await options.store.load())?.tokens?.access_token).toBe("access-2"); + }, + ); + it("fails with cimd when the server does not support documents", async () => { const server = await startAuthorizationServer({ issParameter: true }); await expect(startMcpSignIn(loopback(server, { clientRegistration: "cimd" }))).rejects.toThrow( @@ -336,6 +374,26 @@ describe("cancellation", () => { await expect(completed).rejects.toBeInstanceOf(McpSignInCancelledError); }); + it("cancels at once while another process holds the refresh lock, and refreshes nothing", async () => { + const server = await startAuthorizationServer(); + const recording = recordingStore(); + const options = signInOptions(server, { store: recording.store }); + await signIn(options); + + const release = recording.hold(); + const controller = new AbortController(); + const started = startMcpSignIn({ ...options, signal: controller.signal }); + setTimeout(() => controller.abort(), 50); + await expect(started).rejects.toBeInstanceOf(McpSignInCancelledError); + + // The lock arrives after the cancellation: nothing runs under it, and it is released. + release(); + await Promise.all(recording.lockRuns); + expect(recording.isLocked()).toBe(false); + expect(server.tokenRequests.map((request) => request.get("grant_type"))).toEqual(["authorization_code"]); + expect(server.authorizations).toHaveLength(1); + }); + it("fails at once with an aborted signal", async () => { const server = await startAuthorizationServer(); await expect(startMcpSignIn({ ...signInOptions(server), signal: AbortSignal.abort() })).rejects.toBeInstanceOf(