From 5806068c26e55feefd1f5875bf05c0419ec5d912 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 1 Oct 2026 11:20:59 +0200 Subject: [PATCH] fix(coding-agent): store MCP OAuth credentials per server name and URL Entries sharing a URL now keep separate accounts. Credentials stored by URL alone move to the first server that loads them. closes #10252 --- packages/coding-agent/CHANGELOG.md | 4 ++ packages/coding-agent/docs/mcp.md | 2 + .../coding-agent/src/extensions/mcp/cli.ts | 4 +- .../coding-agent/src/extensions/mcp/index.ts | 6 +- .../coding-agent/src/extensions/mcp/oauth.ts | 60 +++++++++++++------ .../src/extensions/mcp/runtime.ts | 2 +- .../test/mcp-conformance/client.ts | 2 +- .../test/mcp-oauth-refresh.test.ts | 6 +- .../coding-agent/test/mcp-oauth-store.test.ts | 55 +++++++++++++++++ 9 files changed, 112 insertions(+), 29 deletions(-) create mode 100644 packages/coding-agent/test/mcp-oauth-store.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e0468ed80..cc5258eb5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,10 @@ - Added an `oauth.authServerMetadataUrl` setting for MCP servers that advertise a wrong OAuth authorization server or none. Pi uses the configured metadata document instead of discovery ([#10172](https://github.com/earendil-works/pi/issues/10172)). +### Changed + +- MCP OAuth credentials are now stored per server name and URL, so MCP servers with the same URL can sign in with different accounts. Credentials stored by URL alone move to the first server that uses them ([#10252](https://github.com/earendil-works/pi/issues/10252)). + ### 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). diff --git a/packages/coding-agent/docs/mcp.md b/packages/coding-agent/docs/mcp.md index ef53e14bd..77c727667 100644 --- a/packages/coding-agent/docs/mcp.md +++ b/packages/coding-agent/docs/mcp.md @@ -118,6 +118,8 @@ When the server rejects an unauthenticated connection, `/mcp` shows that it need Pi registers itself with the authorization server, stores tokens in `~/.pi/agent/mcp-auth.json`, and refreshes access tokens when they expire or the server rejects them. If a server later requests additional scope, Pi asks for sign-in again. Signing out deletes the stored credentials. +Credentials belong to a server name and URL. Servers with the same URL under different names, such as one per account, sign in separately; servers with the same name and URL in different `mcp.json` files share one sign-in. + OAuth applies to HTTP servers without an `Authorization` header. For a server that does not support dynamic client registration, configure a registered client: ```json diff --git a/packages/coding-agent/src/extensions/mcp/cli.ts b/packages/coding-agent/src/extensions/mcp/cli.ts index 77aa6bafa..64ce281f2 100644 --- a/packages/coding-agent/src/extensions/mcp/cli.ts +++ b/packages/coding-agent/src/extensions/mcp/cli.ts @@ -236,7 +236,7 @@ export async function runMcpCommand(args: string[], options: McpCommandOptions): return 1; } if (command === "logout") { - const removed = credentials.remove(url); + const removed = credentials.remove(name, url); log(removed ? `Signed out of MCP server "${name}".` : `No stored credentials for MCP server "${name}".`); return 0; } @@ -542,7 +542,7 @@ async function login( try { await signInMcpServer({ serverUrl: url, - store: credentials.forServer(url), + store: credentials.forServer(name, url), settings: connection.oauthSettings(), challenge: connection.challenge, prompt: { diff --git a/packages/coding-agent/src/extensions/mcp/index.ts b/packages/coding-agent/src/extensions/mcp/index.ts index ab5435203..3bb364b3e 100644 --- a/packages/coding-agent/src/extensions/mcp/index.ts +++ b/packages/coding-agent/src/extensions/mcp/index.ts @@ -486,7 +486,7 @@ export function createMcpExtension(options: McpExtensionOptions = {}): Extension const tokensAtSignIn = new Map(); const storedTokens = (connection: McpServerConnection): string => { const url = connection.oauthUrl; - return url && credentials ? JSON.stringify(credentials.tokens(url) ?? null) : "null"; + return url && credentials ? JSON.stringify(credentials.tokens(connection.name, url) ?? null) : "null"; }; const onConnectionChange = (connection: McpServerConnection) => { if (connection.state !== "needs-auth") tokensAtSignIn.delete(connection); @@ -598,7 +598,7 @@ export function createMcpExtension(options: McpExtensionOptions = {}): Extension try { await runtime.signInMcpServer({ serverUrl: url, - store: getCredentials(runtime).forServer(url), + store: getCredentials(runtime).forServer(server.entry.name, url), settings: connection.oauthSettings(), challenge: connection.challenge, prompt, @@ -621,7 +621,7 @@ export function createMcpExtension(options: McpExtensionOptions = {}): Extension const connection = server.connection; const url = connection?.oauthUrl; if (!connection || !url) return false; - const removed = getCredentials(await loadMcpRuntime()).remove(url); + const removed = getCredentials(await loadMcpRuntime()).remove(server.entry.name, url); await connection.signOut(); return removed; }; diff --git a/packages/coding-agent/src/extensions/mcp/oauth.ts b/packages/coding-agent/src/extensions/mcp/oauth.ts index 7d7cd146c..33cd0c604 100644 --- a/packages/coding-agent/src/extensions/mcp/oauth.ts +++ b/packages/coding-agent/src/extensions/mcp/oauth.ts @@ -6,7 +6,7 @@ * `McpOAuthAuthorizationRequiredError`, and the user signs in through `/mcp`, which runs * the authorization code flow (PKCE, dynamic client registration) against a loopback callback. * - * Credentials live in `/mcp-auth.json`, keyed by server URL. + * Credentials live in `/mcp-auth.json`, keyed by server name and URL. */ import { createHash } from "node:crypto"; @@ -31,6 +31,7 @@ import { import lockfile from "proper-lockfile"; import { APP_NAME, getAgentDir } from "../../config.ts"; import { type AuthStorageBackend, FileAuthStorageBackend } from "../../core/auth-storage.ts"; +import { mcpNamespace } from "../../core/mcp-servers.ts"; const CALLBACK_HOST = "127.0.0.1"; const CALLBACK_PATH = "/callback"; @@ -108,6 +109,19 @@ function parseStates(content: string | undefined): StoredStates { return typeof parsed === "object" && parsed !== null && !Array.isArray(parsed) ? (parsed as StoredStates) : {}; } +function serializeStates(states: StoredStates): string { + return `${JSON.stringify(states, null, 2)}\n`; +} + +/** + * Keys of a server's state: by name and URL, so servers sharing a URL keep separate accounts, and the + * legacy key by URL alone, written by older versions. + */ +function storeKeys(name: string, serverUrl: string): { key: string; legacyKey: string } { + const legacyKey = String(new URL(serverUrl)); + return { key: `${mcpNamespace(name)}|${legacyKey}`, legacyKey }; +} + export interface McpOAuthServerStore extends McpOAuthStateStore { /** Run `fn` while no other process refreshes the server's tokens. */ withRefreshLock(fn: () => Promise): Promise; @@ -124,10 +138,18 @@ export class McpOAuthCredentialStore { this.lockDir = backend ? lockDir : getAgentDir(); } - forServer(serverUrl: string): McpOAuthServerStore { - const key = String(new URL(serverUrl)); + forServer(name: string, serverUrl: string): McpOAuthServerStore { + const { key, legacyKey } = storeKeys(name, serverUrl); return { - load: () => this.read()[key], + // The first server to load legacy state takes it over; others with the same URL sign in again. + load: () => + this.backend.withLock((current) => { + const states = parseStates(current); + if (states[key] || !states[legacyKey]) return { result: states[key] }; + states[key] = states[legacyKey]; + delete states[legacyKey]; + return { result: states[key], next: serializeStates(states) }; + }), save: (state) => this.write((states) => { states[key] = state; @@ -163,30 +185,30 @@ export class McpOAuthCredentialStore { } } - /** The stored tokens of a server, for noticing sign-ins done by another process. */ - tokens(serverUrl: string): McpOAuthState["tokens"] { - return this.read()[String(new URL(serverUrl))]?.tokens; + /** The stored tokens of a server, for noticing sign-ins done by another process. Does not take over legacy state. */ + tokens(name: string, serverUrl: string): McpOAuthState["tokens"] { + const { key, legacyKey } = storeKeys(name, serverUrl); + const states = this.backend.withLock((current) => ({ result: parseStates(current) })); + return (states[key] ?? states[legacyKey])?.tokens; } - /** Returns whether credentials were stored for the server. */ - remove(serverUrl: string): boolean { - const key = String(new URL(serverUrl)); - if (!(key in this.read())) return false; - this.write((states) => { - delete states[key]; + /** Returns whether credentials were stored for the server. Removes legacy state the server would take over. */ + remove(name: string, serverUrl: string): boolean { + const { key, legacyKey } = storeKeys(name, serverUrl); + return this.backend.withLock((current) => { + const states = parseStates(current); + const stored = key in states ? key : legacyKey in states ? legacyKey : undefined; + if (!stored) return { result: false }; + delete states[stored]; + return { result: true, next: serializeStates(states) }; }); - return true; - } - - private read(): StoredStates { - return this.backend.withLock((current) => ({ result: parseStates(current) })); } private write(update: (states: StoredStates) => void): void { this.backend.withLock((current) => { const states = parseStates(current); update(states); - return { result: undefined, next: `${JSON.stringify(states, null, 2)}\n` }; + return { result: undefined, next: serializeStates(states) }; }); } } diff --git a/packages/coding-agent/src/extensions/mcp/runtime.ts b/packages/coding-agent/src/extensions/mcp/runtime.ts index b14eae4f5..3828e7eaa 100644 --- a/packages/coding-agent/src/extensions/mcp/runtime.ts +++ b/packages/coding-agent/src/extensions/mcp/runtime.ts @@ -204,7 +204,7 @@ export class McpServerConnection implements McpToolCaller, McpResourceServer { this.authProvider = url ? createMcpAuthProvider({ serverUrl: url, - store: options.credentials.forServer(url), + store: options.credentials.forServer(this.entry.name, url), settings: () => this.oauthSettings(), onChallenge: (challenge) => { this.challenge = challenge; diff --git a/packages/coding-agent/test/mcp-conformance/client.ts b/packages/coding-agent/test/mcp-conformance/client.ts index cea10ced2..636bf62b7 100644 --- a/packages/coding-agent/test/mcp-conformance/client.ts +++ b/packages/coding-agent/test/mcp-conformance/client.ts @@ -151,7 +151,7 @@ async function run(serverUrl: string, scenario: string): Promise { log(`sign-in ${signIns}${connection.challenge?.scope ? ` (scope: ${connection.challenge.scope})` : ""}`); await signInMcpServer({ serverUrl: url, - store: credentials.forServer(url), + store: credentials.forServer(entry.name, url), settings: connection.oauthSettings(), challenge: connection.challenge, prompt: simulatedBrowser(), diff --git a/packages/coding-agent/test/mcp-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-oauth-refresh.test.ts index 5bee26540..574ebeb21 100644 --- a/packages/coding-agent/test/mcp-oauth-refresh.test.ts +++ b/packages/coding-agent/test/mcp-oauth-refresh.test.ts @@ -27,7 +27,7 @@ describe("MCP OAuth refresh", () => { // Stores sharing the credential file and lock directory stand in for separate pi processes. const backend = new InMemoryAuthStorageBackend(); const process = () => { - const store = new McpOAuthCredentialStore(backend, lockDir).forServer(server.url); + const store = new McpOAuthCredentialStore(backend, lockDir).forServer("test", server.url); const provider = createMcpAuthProvider({ serverUrl: server.url, store, @@ -95,7 +95,7 @@ describe("MCP OAuth sign-in", () => { try { await signInMcpServer({ serverUrl: server.url, - store: new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()).forServer(server.url), + store: new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()).forServer("test", server.url), settings: settings(server.url), prompt: { showAuthorizationUrl: (url) => void fetch(url), @@ -115,7 +115,7 @@ describe("MCP OAuth sign-in", () => { it("keeps the granted scope when the server asks for more", async () => { const server = await startOAuthMcpServer(); try { - const store = new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()).forServer(server.url); + const store = new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()).forServer("test", server.url); const opened: URL[] = []; const signInWith = (challenge: OAuthChallenge) => signInMcpServer({ diff --git a/packages/coding-agent/test/mcp-oauth-store.test.ts b/packages/coding-agent/test/mcp-oauth-store.test.ts new file mode 100644 index 000000000..e1ca939f1 --- /dev/null +++ b/packages/coding-agent/test/mcp-oauth-store.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { InMemoryAuthStorageBackend } from "../src/core/auth-storage.ts"; +import { McpOAuthCredentialStore } from "../src/extensions/mcp/oauth.ts"; + +const SERVER_URL = "https://mcp.example.com/mcp"; + +function state(accessToken: string) { + return { serverUrl: SERVER_URL, tokens: { access_token: accessToken, token_type: "Bearer" } }; +} + +function storedKeys(backend: InMemoryAuthStorageBackend): string[] { + return Object.keys(JSON.parse(backend.withLock((current) => ({ result: current ?? "{}" })))); +} + +describe("MCP OAuth credential store", () => { + // https://github.com/earendil-works/pi/issues/10252 + it("keeps separate credentials for servers sharing a SERVER_URL", async () => { + const store = new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()); + await store.forServer("work", SERVER_URL).save(state("work-token")); + await store.forServer("personal", SERVER_URL).save(state("personal-token")); + + expect((await store.forServer("work", SERVER_URL).load())?.tokens?.access_token).toBe("work-token"); + expect((await store.forServer("personal", SERVER_URL).load())?.tokens?.access_token).toBe("personal-token"); + + expect(store.remove("work", SERVER_URL)).toBe(true); + expect(await store.forServer("work", SERVER_URL).load()).toBeUndefined(); + expect(store.tokens("personal", SERVER_URL)?.access_token).toBe("personal-token"); + }); + + it("moves credentials stored by SERVER_URL to the first server that loads them", async () => { + const backend = new InMemoryAuthStorageBackend(); + backend.withLock(() => ({ result: undefined, next: JSON.stringify({ [SERVER_URL]: state("legacy-token") }) })); + const store = new McpOAuthCredentialStore(backend); + + // Reading tokens does not take the legacy state over. + expect(store.tokens("work", SERVER_URL)?.access_token).toBe("legacy-token"); + expect(storedKeys(backend)).toEqual([SERVER_URL]); + + expect((await store.forServer("my_work", SERVER_URL).load())?.tokens?.access_token).toBe("legacy-token"); + // Names differing only in `-` and `_` are the same server. + expect(store.tokens("my-work", SERVER_URL)?.access_token).toBe("legacy-token"); + expect(await store.forServer("personal", SERVER_URL).load()).toBeUndefined(); + expect(storedKeys(backend)).toEqual([`mcp__my_work|${SERVER_URL}`]); + }); + + it("signs out of credentials stored by SERVER_URL", () => { + const backend = new InMemoryAuthStorageBackend(); + backend.withLock(() => ({ result: undefined, next: JSON.stringify({ [SERVER_URL]: state("legacy-token") }) })); + const store = new McpOAuthCredentialStore(backend); + + expect(store.remove("work", SERVER_URL)).toBe(true); + expect(storedKeys(backend)).toEqual([]); + expect(store.remove("work", SERVER_URL)).toBe(false); + }); +});