From d2f0fc9257fb34a94cf93e4fe455cc160da22941 Mon Sep 17 00:00:00 2001 From: vastsa Date: Fri, 18 Sep 2026 17:05:57 +0800 Subject: [PATCH] fix(desktop): harden remote MCP OAuth after landing Force HTTPS on authorization-server endpoints (loopback excepted), reuse DCR clients only when RFC 8252 portless or exact redirect matches, and consume loopback callbacks only after a matching state. Pass the listed server record through to onAuthorized so a project-level MCP can handshake after login, and unsubscribe the settings OAuth listener on unmount. Refs #556. --- apps/desktop/electron/main/ipc/mcp-ipc.ts | 2 +- apps/desktop/electron/main/mcp-oauth.ts | 337 +++++++++++++----- .../electron/main/services/plugin-services.ts | 11 +- .../src/components/settings/AgentMcpPage.tsx | 22 +- .../test/agent-capability-settings.test.mjs | 9 + apps/desktop/test/mcp-oauth.test.mjs | 314 ++++++++++++++-- docs/adr/0283-remote-mcp-oauth.md | 9 + docs/spec/03-runtime/01-ipc-protocol.md | 4 +- docs/spec/06-delivery/04-e2e-test-plan.md | 2 +- docs/zh-CN/spec/03-runtime/01-ipc-protocol.md | 4 +- .../spec/06-delivery/04-e2e-test-plan.md | 2 +- 11 files changed, 585 insertions(+), 131 deletions(-) diff --git a/apps/desktop/electron/main/ipc/mcp-ipc.ts b/apps/desktop/electron/main/ipc/mcp-ipc.ts index 2ec8c6fef..eedb04b95 100644 --- a/apps/desktop/electron/main/ipc/mcp-ipc.ts +++ b/apps/desktop/electron/main/ipc/mcp-ipc.ts @@ -179,7 +179,7 @@ handle(IPC.invoke.mcpList, async (query: Partial = {}) => throw new Error(`MCP server ${payload.id} is not an HTTP transport server`); } - return oauth.start(server.id, server.url); + return oauth.start(server.id, server.url, server); }, ); diff --git a/apps/desktop/electron/main/mcp-oauth.ts b/apps/desktop/electron/main/mcp-oauth.ts index 2d13dee81..9f3d0544c 100644 --- a/apps/desktop/electron/main/mcp-oauth.ts +++ b/apps/desktop/electron/main/mcp-oauth.ts @@ -1,7 +1,7 @@ import { createHash, randomBytes, randomUUID } from "node:crypto"; import { createServer, type Server } from "node:http"; import type { AddressInfo } from "node:net"; -import type { McpOAuthLoginEvent, McpServerStatus } from "@pi-desktop/shared"; +import type { McpOAuthLoginEvent, McpServerRecord, McpServerStatus } from "@pi-desktop/shared"; export type StoredMcpOAuthToken = { clientId: string; @@ -13,6 +13,7 @@ export type StoredMcpOAuthToken = { expiresAt?: number; scope?: string; resource?: string; + redirectUris?: string[]; }; export type McpOAuthMetadata = { @@ -31,7 +32,7 @@ export type McpOAuthDeps = { fetchImpl?: typeof fetch; createServer?: typeof createServer; log?: (level: "info" | "warn" | "error", message: string, data?: unknown) => void; - onAuthorized?: (serverId: string) => Promise; + onAuthorized?: (serverId: string, record?: McpServerRecord) => Promise; newId?: () => string; }; @@ -57,10 +58,62 @@ export function parseExpiresIn(val: unknown): number | undefined { return undefined; } +/** RFC 8252 loopback redirect without a port, so the AS may accept any ephemeral port. */ +export const LOOPBACK_REDIRECT_PORTLESS = "http://127.0.0.1/callback"; + +export function isLoopbackHostname(hostname: string): boolean { + const host = hostname.replace(/^\[|\]$/g, "").toLowerCase(); + if (host === "localhost" || host === "::1" || host === "0:0:0:0:0:0:0:1") return true; + return /^127(?:\.\d{1,3}){3}$/.test(host); +} + +/** OAuth 2.1: authorization-server endpoints must be HTTPS, except loopback. */ +export function assertTlsProtectedUrl(raw: string, label: string): URL { + let url: URL; + try { + url = new URL(raw); + } catch { + throw new Error(`${label} is not a valid URL`); + } + if (url.protocol === "https:") return url; + if (url.protocol === "http:" && isLoopbackHostname(url.hostname)) return url; + throw new Error(`${label} must use HTTPS (got ${url.protocol}//${url.host})`); +} + +export function loopbackRedirectUri(port: number): string { + return `http://127.0.0.1:${port}/callback`; +} + +export function canReuseDcrClient( + token: StoredMcpOAuthToken | null | undefined, + registrationEndpoint: string | undefined, + exactRedirect: string, +): boolean { + if (!token?.clientId) return false; + if (token.registrationEndpoint !== registrationEndpoint) return false; + const uris = token.redirectUris ?? []; + return uris.includes(LOOPBACK_REDIRECT_PORTLESS) || uris.includes(exactRedirect); +} + +export function preferredLoopbackPort(uris: string[] | undefined): number | undefined { + for (const uri of uris ?? []) { + try { + const url = new URL(uri); + if (url.hostname !== "127.0.0.1" || !url.port) continue; + const port = Number(url.port); + if (Number.isInteger(port) && port > 0 && port <= 65535) return port; + } catch { + continue; + } + } + return undefined; +} + type LoginSession = { loginId: string; serverId: string; serverUrl: string; + record?: McpServerRecord; controller: AbortController; server?: Server; cleanup: () => void; @@ -138,6 +191,9 @@ export class McpOAuthManager { } catch { // Best-effort probe; fallback to standard paths below. } + if (resourceMetadataUrl) { + assertTlsProtectedUrl(resourceMetadataUrl, "resource_metadata"); + } // Step 2: Fallback to standard RFC 9728 paths if not in WWW-Authenticate let prm: Record | null = null; @@ -171,6 +227,7 @@ export class McpOAuthManager { ? (prm.authorization_servers as string[]) : []; const authServer = authServersRaw[0] ?? urlObj.origin; + assertTlsProtectedUrl(authServer, "authorization_server"); const authServerObj = new URL(authServer); // Step 3: Fetch Authorization Server Metadata (RFC 8414) @@ -202,8 +259,13 @@ export class McpOAuthManager { ); } + assertTlsProtectedUrl(authorizationEndpoint, "authorization_endpoint"); + assertTlsProtectedUrl(tokenEndpoint, "token_endpoint"); const registrationEndpoint = typeof asMeta?.registration_endpoint === "string" ? asMeta.registration_endpoint : undefined; + if (registrationEndpoint) { + assertTlsProtectedUrl(registrationEndpoint, "registration_endpoint"); + } const scopesSupported = Array.isArray(asMeta?.scopes_supported) ? (asMeta.scopes_supported as string[]) : Array.isArray(prm?.scopes_supported) @@ -225,16 +287,18 @@ export class McpOAuthManager { */ async registerClient( registrationEndpoint: string, - redirectUri: string, + redirectUris: string | string[], clientName = "PI-Desktop", ): Promise<{ clientId: string; clientSecret?: string }> { + assertTlsProtectedUrl(registrationEndpoint, "registration_endpoint"); + const uris = Array.isArray(redirectUris) ? redirectUris : [redirectUris]; const res = await this.fetch(registrationEndpoint, { method: "POST", redirect: "manual", headers: { "content-type": "application/json" }, body: JSON.stringify({ client_name: clientName, - redirect_uris: [redirectUri], + redirect_uris: uris, grant_types: ["authorization_code", "refresh_token"], response_types: ["code"], token_endpoint_auth_method: "none", @@ -243,9 +307,11 @@ export class McpOAuthManager { if (!res.ok) { const text = await res.text().catch(() => ""); - throw new Error( - `Dynamic client registration failed (${res.status}): ${text.slice(0, 300)}`, - ); + this.deps.log?.("warn", "mcp oauth DCR failed", { + status: res.status, + body: text.slice(0, 300), + }); + throw new Error(`Dynamic client registration failed (HTTP ${res.status})`); } const json = (await res.json()) as Record; @@ -257,12 +323,33 @@ export class McpOAuthManager { return { clientId, clientSecret }; } + private async registerLoopbackClient( + registrationEndpoint: string, + exactRedirect: string, + ): Promise<{ clientId: string; clientSecret?: string; redirectUris: string[] }> { + const portless = [LOOPBACK_REDIRECT_PORTLESS, exactRedirect]; + try { + const registered = await this.registerClient(registrationEndpoint, portless); + return { ...registered, redirectUris: portless }; + } catch (error) { + this.deps.log?.("info", "mcp oauth portless DCR rejected, registering exact redirect", { + message: error instanceof Error ? error.message : String(error), + }); + const registered = await this.registerClient(registrationEndpoint, [exactRedirect]); + return { ...registered, redirectUris: [exactRedirect] }; + } + } + /** * Non-blocking start for MCP OAuth login. * Cancels any in-flight attempt for the same server, returns { ok: true, loginId } * immediately, and drives the browser login and loopback callback in the background. */ - async start(serverId: string, serverUrl: string): Promise<{ ok: boolean; loginId: string }> { + async start( + serverId: string, + serverUrl: string, + record?: McpServerRecord, + ): Promise<{ ok: boolean; loginId: string }> { const existing = [...this.pendingLogins.values()].find((s) => s.serverId === serverId); if (existing) { this.cancel(existing.loginId); @@ -285,6 +372,7 @@ export class McpOAuthManager { loginId, serverId, serverUrl, + record, controller, cleanup: () => { for (const fn of cleanups) { @@ -338,6 +426,28 @@ export class McpOAuthManager { await new Promise((resolve, reject) => { let settled = false; + const codeVerifier = randomBytes(32).toString("base64url"); + const codeChallenge = createHash("sha256").update(codeVerifier).digest("base64url"); + const expectedState = randomBytes(16).toString("hex"); + let clientId = ""; + let clientSecret: string | undefined; + let redirectUri = ""; + let redirectUrisToStore: string[] | undefined; + + const finish = (error?: Error, token?: StoredMcpOAuthToken) => { + if (settled) return; + settled = true; + if (error) { + rejectToken(error); + reject(error); + return; + } + if (token) { + resolveToken(token); + resolve(); + } + }; + const server = this.createServer(async (req, res) => { try { const reqUrl = new URL(req.url ?? "/", "http://127.0.0.1"); @@ -347,37 +457,51 @@ export class McpOAuthManager { return; } + if (settled) { + res.writeHead(409, { "content-type": "text/html; charset=utf-8" }); + res.end(this.renderHtml(false, "Authorization already completed")); + return; + } + + if (!expectedState || !redirectUri || !clientId) { + res.writeHead(503, { "content-type": "text/html; charset=utf-8" }); + res.end(this.renderHtml(false, "Authorization is not ready")); + return; + } + const state = reqUrl.searchParams.get("state"); const code = reqUrl.searchParams.get("code"); const error = reqUrl.searchParams.get("error"); const errorDescription = reqUrl.searchParams.get("error_description"); + // CSRF: unmatched state is a stray request — do not abort the login. + if (state !== expectedState) { + res.writeHead(400, { "content-type": "text/html; charset=utf-8" }); + res.end(this.renderHtml(false, "Invalid OAuth callback state")); + return; + } + + settled = true; + if (error) { const displayErr = errorDescription ? `${error}: ${errorDescription}` : error; res.writeHead(400, { "content-type": "text/html; charset=utf-8" }); res.end(this.renderHtml(false, `Authorization error: ${displayErr}`)); const err = new Error(`OAuth authorization error: ${displayErr}`); - if (!settled) { - settled = true; - rejectToken(err); - reject(err); - } + rejectToken(err); + reject(err); return; } - if (state !== expectedState || !code) { + if (!code) { res.writeHead(400, { "content-type": "text/html; charset=utf-8" }); - res.end(this.renderHtml(false, "Invalid OAuth callback state or missing code")); - const err = new Error("Invalid OAuth callback state or missing code"); - if (!settled) { - settled = true; - rejectToken(err); - reject(err); - } + res.end(this.renderHtml(false, "Invalid OAuth callback: missing code")); + const err = new Error("Invalid OAuth callback: missing code"); + rejectToken(err); + reject(err); return; } - // Token exchange with RFC 8707 resource and redirect: manual const tokenParams = new URLSearchParams({ grant_type: "authorization_code", client_id: clientId, @@ -402,16 +526,15 @@ export class McpOAuthManager { if (!tokenRes.ok) { const errText = await tokenRes.text().catch(() => ""); + this.deps.log?.("warn", "mcp oauth token exchange failed", { + status: tokenRes.status, + body: errText.slice(0, 300), + }); res.writeHead(500, { "content-type": "text/html; charset=utf-8" }); res.end(this.renderHtml(false, `Token exchange failed (${tokenRes.status})`)); - const err = new Error( - `OAuth token exchange failed (${tokenRes.status}): ${errText.slice(0, 300)}`, - ); - if (!settled) { - settled = true; - rejectToken(err); - reject(err); - } + const err = new Error(`OAuth token exchange failed (HTTP ${tokenRes.status})`); + rejectToken(err); + reject(err); return; } @@ -422,11 +545,8 @@ export class McpOAuthManager { res.writeHead(500, { "content-type": "text/html; charset=utf-8" }); res.end(this.renderHtml(false, "Token response missing access_token")); const err = new Error("Token response missing access_token"); - if (!settled) { - settled = true; - rejectToken(err); - reject(err); - } + rejectToken(err); + reject(err); return; } @@ -443,6 +563,7 @@ export class McpOAuthManager { expiresAt: expiresIn ? Date.now() + expiresIn * 1000 : undefined, scope: typeof tokenJson.scope === "string" ? tokenJson.scope : undefined, resource: metadata.resource, + redirectUris: redirectUrisToStore, }; await this.deps.call("secrets.set", { @@ -458,15 +579,12 @@ export class McpOAuthManager { ), ); - if (!settled) { - settled = true; - resolveToken(storedToken); - resolve(); - } + resolveToken(storedToken); + resolve(); } catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); if (!settled) { settled = true; - const error = err instanceof Error ? err : new Error(String(err)); rejectToken(error); reject(error); } @@ -479,59 +597,44 @@ export class McpOAuthManager { }); const abortListener = () => { - if (!settled) { - settled = true; - const err = new Error("OAuth login cancelled"); - rejectToken(err); - reject(err); - } + finish(new Error("OAuth login cancelled")); }; session.controller.signal.addEventListener("abort", abortListener, { once: true }); cleanups.push(() => session.controller.signal.removeEventListener("abort", abortListener), ); - let expectedState = ""; - let codeVerifier = ""; - let clientId = ""; - let clientSecret: string | undefined; - let redirectUri = ""; - const timeoutTimer = setTimeout(() => { - if (!settled) { - settled = true; - const err = new Error("OAuth authorization timed out"); - rejectToken(err); - reject(err); - } + finish(new Error("OAuth authorization timed out")); }, DEFAULT_AUTH_TIMEOUT_MS); cleanups.push(() => clearTimeout(timeoutTimer)); - server.listen(0, "127.0.0.1", async () => { + void (async () => { try { - const address = server.address() as AddressInfo; - redirectUri = `http://127.0.0.1:${address.port}/callback`; - - // Check if existing token has stored clientId for this registration endpoint const existingRaw = await this.readStoredToken(session.serverId); - if ( - existingRaw?.clientId && - existingRaw?.registrationEndpoint === metadata.registrationEndpoint - ) { - clientId = existingRaw.clientId; - clientSecret = existingRaw.clientSecret; + const preferredPort = preferredLoopbackPort(existingRaw?.redirectUris); + const address = await this.listenOnLoopback(server, preferredPort); + server.on("error", (err) => { + finish(err instanceof Error ? err : new Error(String(err))); + }); + redirectUri = loopbackRedirectUri(address.port); + + if (canReuseDcrClient(existingRaw, metadata.registrationEndpoint, redirectUri)) { + clientId = existingRaw!.clientId; + clientSecret = existingRaw!.clientSecret; + redirectUrisToStore = existingRaw!.redirectUris; } else if (metadata.registrationEndpoint) { - const reg = await this.registerClient(metadata.registrationEndpoint, redirectUri); - clientId = reg.clientId; - clientSecret = reg.clientSecret; + const registered = await this.registerLoopbackClient( + metadata.registrationEndpoint, + redirectUri, + ); + clientId = registered.clientId; + clientSecret = registered.clientSecret; + redirectUrisToStore = registered.redirectUris; } else { clientId = "pi-desktop"; } - codeVerifier = randomBytes(32).toString("base64url"); - const codeChallenge = createHash("sha256").update(codeVerifier).digest("base64url"); - expectedState = randomBytes(16).toString("hex"); - const authUrl = new URL(metadata.authorizationEndpoint); authUrl.searchParams.set("response_type", "code"); authUrl.searchParams.set("client_id", clientId); @@ -539,9 +642,10 @@ export class McpOAuthManager { authUrl.searchParams.set("state", expectedState); authUrl.searchParams.set("code_challenge", codeChallenge); authUrl.searchParams.set("code_challenge_method", "S256"); - // RFC 8707 & MCP authorization specification requires resource parameter authUrl.searchParams.set("resource", metadata.resource); + // MVP: no per-server scope picker. Prefer a literal "default" if + // advertised (Notion-class), otherwise the first supported scope. if (metadata.scopesSupported?.includes("default")) { authUrl.searchParams.set("scope", "default"); } else if (metadata.scopesSupported?.[0]) { @@ -566,29 +670,16 @@ export class McpOAuthManager { opened, }); } catch (err) { - if (!settled) { - settled = true; - const error = err instanceof Error ? err : new Error(String(err)); - rejectToken(error); - reject(error); - } - } - }); - - server.on("error", (err) => { - if (!settled) { - settled = true; - rejectToken(err); - reject(err); + finish(err instanceof Error ? err : new Error(String(err))); } - }); + })(); }); // Hook for post-auth runtime updates (re-testing server and refreshing status) let status: McpServerStatus | undefined; if (this.deps.onAuthorized) { try { - status = await this.deps.onAuthorized(session.serverId); + status = await this.deps.onAuthorized(session.serverId, session.record); } catch { // Status will fallback to default below } @@ -646,16 +737,18 @@ export class McpOAuthManager { const token = await this.readStoredToken(serverId); if (!token) return null; - // If token is expiring in < 60s and we have a refresh token, refresh it if (token.expiresAt && Date.now() > token.expiresAt - 60_000 && token.refreshToken) { try { const refreshed = await this.refreshToken(serverId, token); return refreshed.accessToken; } catch (err) { - this.deps.log?.("warn", "failed to refresh mcp oauth token, falling back to existing", { + this.deps.log?.("warn", "failed to refresh mcp oauth token", { serverId, error: (err as Error).message, }); + const still = await this.readStoredToken(serverId); + if (!still) return null; + return still.accessToken; } } @@ -752,10 +845,14 @@ export class McpOAuthManager { serverId: string, token: StoredMcpOAuthToken, ): Promise { + if (!token.refreshToken) { + throw new Error("Token refresh failed: missing refresh_token"); + } + assertTlsProtectedUrl(token.tokenEndpoint, "token_endpoint"); const params = new URLSearchParams({ grant_type: "refresh_token", client_id: token.clientId, - refresh_token: token.refreshToken!, + refresh_token: token.refreshToken, }); if (token.clientSecret) { params.set("client_secret", token.clientSecret); @@ -776,7 +873,16 @@ export class McpOAuthManager { if (!res.ok) { const text = await res.text().catch(() => ""); - throw new Error(`Token refresh failed (${res.status}): ${text.slice(0, 300)}`); + this.deps.log?.("warn", "mcp oauth token refresh failed", { + serverId, + status: res.status, + body: text.slice(0, 300), + }); + if (res.status === 400 || res.status === 401) { + await this.clearStoredToken(serverId); + } + throw new Error(`Token refresh failed (HTTP ${res.status})`); + throw new Error(`Token refresh failed (HTTP ${res.status})`); } const json = (await res.json()) as Record; @@ -804,6 +910,41 @@ export class McpOAuthManager { return updated; } + private async clearStoredToken(serverId: string): Promise { + try { + await this.deps.call("secrets.delete", { + secretRef: secretRefForMcpOAuth(serverId), + }); + } catch { + // Best effort + } + } + + private listenOnLoopback(server: Server, preferredPort?: number): Promise { + return new Promise((resolve, reject) => { + const tryListen = (port: number, allowFallback: boolean) => { + const onError = (err: Error) => { + server.off("error", onError); + if (allowFallback && (err as { code?: string }).code === "EADDRINUSE") { + tryListen(0, false); + return; + } + reject(err); + }; + server.once("error", onError); + server.listen(port, "127.0.0.1", () => { + server.off("error", onError); + const address = server.address(); + if (!address || typeof address === "string") { + reject(new Error("OAuth loopback server has no address")); + return; + } + resolve(address); + }); + }; + tryListen(preferredPort ?? 0, Boolean(preferredPort && preferredPort > 0)); + }); + } private serialize(key: string, fn: () => Promise): Promise { const previous = this.refreshChains.get(key) ?? Promise.resolve(); diff --git a/apps/desktop/electron/main/services/plugin-services.ts b/apps/desktop/electron/main/services/plugin-services.ts index 075835eb6..b1822aa80 100644 --- a/apps/desktop/electron/main/services/plugin-services.ts +++ b/apps/desktop/electron/main/services/plugin-services.ts @@ -184,7 +184,6 @@ export function createPluginServices({ globalShortcut.unregister(accelerator); }, // Late-bound: the runtime is constructed just below, and a trigger can - // Late-bound: the runtime is constructed just below, and a trigger can // only arrive once the app is running and a plugin holds a shortcut. onTrigger: (entry) => { void plugins.triggerPluginShortcut(entry); @@ -460,9 +459,17 @@ export function createPluginServices({ emit: (event) => sendToRenderer(IPC.event.mcpOauth, event), openExternal: (url) => safeOpenExternal(url), log: (level, message, data) => logger.app("plugin", level, message, { data }), - onAuthorized: async (serverId): Promise => { + onAuthorized: async (serverId, record): Promise => { + const existed = userMcp.listRecords().some((item) => item.id === serverId); + if (record && !existed) { + userMcp.setRecords([...userMcp.listRecords(), record]); + } userMcp.invalidate(serverId); const status: McpServerStatus = await userMcp.test(serverId); + if (!existed) { + userMcp.invalidate(serverId); + userMcp.setRecords(userMcp.listRecords().filter((item) => item.id !== serverId)); + } sendToRenderer(IPC.event.pluginChanged, { reason: "mcp", pluginId: serverId }); return status; }, diff --git a/apps/desktop/src/components/settings/AgentMcpPage.tsx b/apps/desktop/src/components/settings/AgentMcpPage.tsx index 51c607c72..725e5f584 100644 --- a/apps/desktop/src/components/settings/AgentMcpPage.tsx +++ b/apps/desktop/src/components/settings/AgentMcpPage.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { GLOBAL_SCOPE, @@ -116,6 +116,14 @@ export function AgentMcpPage() { const [saving, setSaving] = useState(false); const [testingId, setTestingId] = useState(null); const [authorizingId, setAuthorizingId] = useState(null); + const pendingOAuthRef = useRef<{ unsubscribe: () => void } | null>(null); + + useEffect(() => { + return () => { + pendingOAuthRef.current?.unsubscribe(); + pendingOAuthRef.current = null; + }; + }, []); const [view, setView] = useState<"servers" | "market">("servers"); const { armed, setArmed } = useArmedDelete(); @@ -250,16 +258,23 @@ export function AgentMcpPage() { }; const authorizeServer = async (server: McpServerRecord, level: AgentCapabilityLevel) => { - if (authorizingId) return; + if (authorizingId || pendingOAuthRef.current) return; setAuthorizingId(server.id); let activeLoginId: string | null = null; let unsubscribed = false; let unsubscribe = () => {}; - const cleanup = () => { + const finish = () => { if (unsubscribed) return; unsubscribed = true; unsubscribe(); + if (pendingOAuthRef.current?.unsubscribe === unsubscribe) { + pendingOAuthRef.current = null; + } + }; + + const cleanup = () => { + finish(); setAuthorizingId(null); }; @@ -283,6 +298,7 @@ export function AgentMcpPage() { cleanup(); } }); + pendingOAuthRef.current = { unsubscribe }; try { showToast(t("extensions.mcp.authorizing"), { variant: "info" }); diff --git a/apps/desktop/test/agent-capability-settings.test.mjs b/apps/desktop/test/agent-capability-settings.test.mjs index 5adcd1b8a..cf419b0ce 100644 --- a/apps/desktop/test/agent-capability-settings.test.mjs +++ b/apps/desktop/test/agent-capability-settings.test.mjs @@ -446,3 +446,12 @@ test("the move action offers a level only when there is a destination", () => { assert.match(source, /showToast\(t\("settings\.selectProjectFirst"\)/); } }); + +test("MCP OAuth subscribe is owned by a ref cleaned up on unmount", () => { + const mcpIpc = readMainModuleSync("ipc/mcp-ipc.ts"); + assert.match(mcp, /pendingOAuthRef/); + assert.match(mcp, /useEffect\(\(\) => \{/); + assert.match(mcp, /pendingOAuthRef\.current\?\.unsubscribe\(\)/); + assert.match(mcp, /api\.onMcpOAuth\(/); + assert.match(mcpIpc, /oauth\.start\(server\.id, server\.url, server\)/); +}); diff --git a/apps/desktop/test/mcp-oauth.test.mjs b/apps/desktop/test/mcp-oauth.test.mjs index 2eae30cad..c85407551 100644 --- a/apps/desktop/test/mcp-oauth.test.mjs +++ b/apps/desktop/test/mcp-oauth.test.mjs @@ -2,7 +2,15 @@ import assert from "node:assert/strict"; import test from "node:test"; import { EventEmitter } from "node:events"; -import { McpOAuthManager, escapeHtml, parseExpiresIn } from "../electron/main/mcp-oauth.ts"; +import { + McpOAuthManager, + escapeHtml, + parseExpiresIn, + assertTlsProtectedUrl, + canReuseDcrClient, + isLoopbackHostname, + LOOPBACK_REDIRECT_PORTLESS, +} from "../electron/main/mcp-oauth.ts"; import { UserMcpRuntime } from "../electron/main/user-mcp.ts"; function fakeHost() { @@ -28,6 +36,15 @@ function fakeHost() { return { call, secrets, calls }; } +async function waitUntil(predicate, timeoutMs = 500) { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + if (await predicate()) return; + await new Promise((resolve) => setTimeout(resolve, 10)); + } + throw new Error("timed out waiting for condition"); +} + function createMockFetch(options = {}) { const registeredClients = []; const tokenRequests = []; @@ -120,6 +137,12 @@ function createMockFetch(options = {}) { if (grantType === "refresh_token") { refreshCount++; + if (options.refreshStatus) { + return new Response(JSON.stringify({ error: "unavailable" }), { + status: options.refreshStatus, + headers: { "Content-Type": "application/json" }, + }); + } const refreshToken = params.get("refresh_token"); if (refreshToken === "mock-refresh-token-456") { return new Response(JSON.stringify({ @@ -145,20 +168,35 @@ function createMockFetch(options = {}) { return { mockFetch, registeredClients, tokenRequests, getRefreshCount: () => refreshCount }; } -function createMockServerFactory() { +function createMockServerFactory(options = {}) { let activeRequestListener = null; let closed = false; + let assignedPort = options.port ?? 54321; + const listenPorts = []; + let failPreferred = options.failPreferred === true; const mockCreateServer = (requestListener) => { activeRequestListener = requestListener; closed = false; const emitter = new EventEmitter(); - emitter.listen = (_port, _host, callback) => { + emitter.listen = (port, _host, callback) => { + listenPorts.push(port); + if (port && port !== 0 && failPreferred) { + failPreferred = false; + queueMicrotask(() => { + emitter.emit( + "error", + Object.assign(new Error("listen EADDRINUSE"), { code: "EADDRINUSE" }), + ); + }); + return emitter; + } + assignedPort = !port || port === 0 ? (options.port ?? 54321) : port; if (callback) queueMicrotask(callback); return emitter; }; emitter.address = () => ({ - port: 54321, + port: assignedPort, family: "IPv4", address: "127.0.0.1", }); @@ -177,7 +215,7 @@ function createMockServerFactory() { } const req = new EventEmitter(); req.url = pathWithQuery; - req.headers = { host: "127.0.0.1:54321" }; + req.headers = { host: `127.0.0.1:${assignedPort}` }; let statusCode = 200; const headers = {}; @@ -197,7 +235,13 @@ function createMockServerFactory() { return { statusCode, headers, body }; }; - return { mockCreateServer, simulateCallback, isClosed: () => closed }; + return { + mockCreateServer, + simulateCallback, + isClosed: () => closed, + listenPorts, + getPort: () => assignedPort, + }; } test("McpOAuthManager: discovers metadata through RFC 9728 and RFC 8414", async (t) => { @@ -268,7 +312,10 @@ test("McpOAuthManager: executes full authorization flow with DCR, PKCE, 127.0.0. // Verify redirect_uri is on 127.0.0.1, not localhost (RFC 8252) assert.equal(parsedUrl.searchParams.get("redirect_uri"), "http://127.0.0.1:54321/callback"); assert.equal(registeredClients.length, 1); - assert.equal(registeredClients[0].redirect_uris[0], "http://127.0.0.1:54321/callback"); + assert.deepEqual(registeredClients[0].redirect_uris, [ + LOOPBACK_REDIRECT_PORTLESS, + "http://127.0.0.1:54321/callback", + ]); const state = parsedUrl.searchParams.get("state"); assert.ok(state); @@ -315,22 +362,24 @@ test("McpOAuthManager: escapes HTML on error callback to prevent reflected XSS", t.after(() => manager.disposeAll()); await manager.start("xss-test", "https://notion.test/mcp"); - for (let i = 0; i < 50; i++) { - if (openedUrl) break; - await new Promise((r) => setTimeout(r, 10)); - } + await waitUntil(() => openedUrl); - // Simulate error with XSS payload const xssPayload = ''; - const callbackRes = await simulateCallback(`/callback?error=access_denied&error_description=${encodeURIComponent(xssPayload)}`); - - assert.equal(callbackRes.statusCode, 400); - // Must NOT include raw script tag - assert.equal(callbackRes.body.includes("'), '<script>alert("xss")</script>'); - assert.equal(escapeHtml("Tom & Jerry 'cat'"), 'Tom & Jerry 'cat''); + assert.equal(escapeHtml("Tom & Jerry 'cat'"), "Tom & Jerry 'cat'"); +}); + +test("TLS helpers allow HTTPS and loopback HTTP only", () => { + assert.equal(isLoopbackHostname("127.0.0.1"), true); + assert.equal(isLoopbackHostname("localhost"), true); + assert.equal(isLoopbackHostname("evil.test"), false); + assert.equal(assertTlsProtectedUrl("https://auth.example/x", "authorization_endpoint").protocol, "https:"); + assert.equal(assertTlsProtectedUrl("http://127.0.0.1:9/x", "token_endpoint").hostname, "127.0.0.1"); + assert.throws( + () => assertTlsProtectedUrl("http://evil.test/token", "token_endpoint"), + /must use HTTPS/, + ); + assert.equal( + canReuseDcrClient( + { + clientId: "abc", + registrationEndpoint: "https://auth.example/register", + tokenEndpoint: "https://auth.example/token", + accessToken: "t", + redirectUris: [LOOPBACK_REDIRECT_PORTLESS], + }, + "https://auth.example/register", + "http://127.0.0.1:9/callback", + ), + true, + ); + assert.equal( + canReuseDcrClient( + { + clientId: "abc", + registrationEndpoint: "https://auth.example/register", + tokenEndpoint: "https://auth.example/token", + accessToken: "t", + redirectUris: ["http://127.0.0.1:11111/callback"], + }, + "https://auth.example/register", + "http://127.0.0.1:54321/callback", + ), + false, + ); +}); + +test("McpOAuthManager: rejects non-loopback HTTP authorization servers", async (t) => { + const host = fakeHost(); + const mockFetch = async (input) => { + const url = new URL(typeof input === "string" ? input : input.url); + if (url.pathname === "/.well-known/oauth-protected-resource") { + return new Response(JSON.stringify({ + resource: "http://insecure.test/mcp", + authorization_servers: ["http://insecure.test"], + }), { status: 200, headers: { "Content-Type": "application/json" } }); + } + return new Response(null, { status: 404 }); + }; + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + openExternal: async () => {}, + }); + t.after(() => manager.disposeAll()); + await assert.rejects( + () => manager.discoverMetadata("http://insecure.test/mcp"), + /authorization_server must use HTTPS/, + ); +}); + +test("McpOAuthManager: unmatched state does not abort login; replay is rejected", async (t) => { + const host = fakeHost(); + const { mockFetch, tokenRequests } = createMockFetch(); + const { mockCreateServer, simulateCallback } = createMockServerFactory(); + const events = []; + let openedUrl = null; + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + createServer: mockCreateServer, + emit: (event) => events.push(event), + openExternal: async (url) => { + openedUrl = url; + }, + }); + t.after(() => manager.disposeAll()); + + await manager.start("csrf-test", "https://notion.test/mcp"); + await waitUntil(() => openedUrl); + const state = new URL(openedUrl).searchParams.get("state"); + + const mismatch = await simulateCallback(`/callback?code=valid-code&state=deadbeef`); + assert.equal(mismatch.statusCode, 400); + assert.equal(events.some((event) => event.kind === "error" || event.kind === "done"), false); + + const [first, second] = await Promise.all([ + simulateCallback(`/callback?code=valid-code&state=${encodeURIComponent(state)}`), + simulateCallback(`/callback?code=valid-code&state=${encodeURIComponent(state)}`), + ]); + assert.deepEqual([first.statusCode, second.statusCode].sort((a, b) => a - b), [200, 409]); + assert.equal(tokenRequests.filter((req) => req.grant_type === "authorization_code").length, 1); + await waitUntil(() => events.some((event) => event.kind === "done")); +}); + +test("McpOAuthManager: re-registers when stored redirect is a different exact port", async (t) => { + const host = fakeHost(); + const { mockFetch, registeredClients } = createMockFetch(); + const { mockCreateServer } = createMockServerFactory({ failPreferred: true }); + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + createServer: mockCreateServer, + openExternal: async () => {}, + }); + t.after(() => manager.disposeAll()); + + await host.call("secrets.set", { + secretRef: "secret:mcp:port-lock:oauth", + value: JSON.stringify({ + clientId: "old-client", + registrationEndpoint: "https://notion.test/register", + tokenEndpoint: "https://notion.test/token", + accessToken: "mock-token", + redirectUris: ["http://127.0.0.1:11111/callback"], + }), + }); + + await manager.start("port-lock", "https://notion.test/mcp"); + await waitUntil(() => registeredClients.length === 1); + assert.equal(registeredClients[0].client_id, "registered-client-1"); + assert.ok(registeredClients[0].redirect_uris.includes("http://127.0.0.1:54321/callback")); +}); + +test("McpOAuthManager: invalid_grant refresh deletes the stored token", async (t) => { + const host = fakeHost(); + const { mockFetch } = createMockFetch(); + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + openExternal: async () => {}, + }); + t.after(() => manager.disposeAll()); + + await host.call("secrets.set", { + secretRef: "secret:mcp:dead-refresh:oauth", + value: JSON.stringify({ + clientId: "test-client-id", + tokenEndpoint: "https://notion.test/token", + accessToken: "old-token", + refreshToken: "revoked-refresh", + expiresAt: Date.now() + 5_000, + }), + }); + + const token = await manager.getValidAccessToken("dead-refresh"); + assert.equal(token, null); + assert.equal(await manager.hasOAuth("dead-refresh"), false); +}); + +test("McpOAuthManager: 5xx refresh keeps the existing access token", async (t) => { + const host = fakeHost(); + const { mockFetch } = createMockFetch({ refreshStatus: 503 }); + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + openExternal: async () => {}, + }); + t.after(() => manager.disposeAll()); + + await host.call("secrets.set", { + secretRef: "secret:mcp:refresh-5xx:oauth", + value: JSON.stringify({ + clientId: "test-client-id", + tokenEndpoint: "https://notion.test/token", + accessToken: "old-token", + refreshToken: "mock-refresh-token-456", + expiresAt: Date.now() + 5_000, + }), + }); + + const token = await manager.getValidAccessToken("refresh-5xx"); + assert.equal(token, "old-token"); + assert.equal(await manager.hasOAuth("refresh-5xx"), true); +}); + +test("McpOAuthManager: onAuthorized receives the server record", async (t) => { + const host = fakeHost(); + const { mockFetch } = createMockFetch(); + const { mockCreateServer, simulateCallback } = createMockServerFactory(); + let authorized = null; + let openedUrl = null; + const manager = new McpOAuthManager({ + call: host.call, + fetchImpl: mockFetch, + createServer: mockCreateServer, + openExternal: async (url) => { + openedUrl = url; + }, + onAuthorized: async (serverId, record) => { + authorized = { serverId, record }; + return { + serverId, + state: "ready", + toolCount: 3, + updatedAt: Date.now(), + }; + }, + }); + t.after(() => manager.disposeAll()); + + const record = { + id: "proj-mcp", + label: "Project MCP", + transport: "http", + url: "https://notion.test/mcp", + }; + await manager.start(record.id, record.url, record); + await waitUntil(() => openedUrl); + const state = new URL(openedUrl).searchParams.get("state"); + const callbackRes = await simulateCallback( + `/callback?code=valid-code&state=${encodeURIComponent(state)}`, + ); + assert.equal(callbackRes.statusCode, 200); + await waitUntil(() => authorized); + assert.equal(authorized.serverId, record.id); + assert.equal(authorized.record, record); }); diff --git a/docs/adr/0283-remote-mcp-oauth.md b/docs/adr/0283-remote-mcp-oauth.md index 4b9d1efcc..08c228063 100644 --- a/docs/adr/0283-remote-mcp-oauth.md +++ b/docs/adr/0283-remote-mcp-oauth.md @@ -41,3 +41,12 @@ Previous MCP implementations in PI-Desktop supported only static HTTP headers. U - Remote HTTP MCP servers requiring OAuth 2.1 PKCE can be authorized securely from the Settings UI. - Long-running browser interactions do not block IPC channels or leave orphan HTTP listeners upon window reload or quit. - Token material is kept out of the renderer process and ordinary logs. + +## Amendment (2026-09-18) — landing hardening + +- Authorization-server endpoints (`authorization_endpoint`, `token_endpoint`, `registration_endpoint`, discovered `authorization_servers`, and `resource_metadata`) must be HTTPS. Loopback `http://127.0.0.1` / `localhost` / `::1` remains allowed so local mock and LAN-loopback AS still work. The MCP resource URL itself may still be `http://` (ADR 0142). +- Dynamic client registration prefers RFC 8252 portless `http://127.0.0.1/callback` plus the current exact URI. Stored clients are reused only when that portless URI is on file, or the exact current redirect matches. Otherwise a new client is registered. Login also tries to rebind the previously used loopback port so strict AS that require an exact URI keep working. +- Loopback `/callback` validates `state` before looking at `error` or `code`. A mismatched `state` is a stray request and does not abort the login. A matching callback is consumed once (replay returns 409). +- `invalid_grant` on refresh deletes the stored secret; transient 5xx keeps the existing access token. +- `mcp/oauth/start` passes the listed `McpServerRecord` through to `onAuthorized` so a project-level server that is not in the current workspace runtime can still handshake after login. +- Scope selection remains an MVP heuristic (`default` if advertised, else `scopes_supported[0]`). There is no per-server scope picker, device-code flow, or DPoP. diff --git a/docs/spec/03-runtime/01-ipc-protocol.md b/docs/spec/03-runtime/01-ipc-protocol.md index 0ec80746b..b2bc09da5 100644 --- a/docs/spec/03-runtime/01-ipc-protocol.md +++ b/docs/spec/03-runtime/01-ipc-protocol.md @@ -1553,7 +1553,7 @@ Desktop-only MCP market channels (not host RPC) live on Electron IPC: cursor state for browse and server-side search. One failed source does not discard successful sources; the response and caches are bounded. -### MCP OAuth (ADR 0281) +### MCP OAuth (ADR 0283) Browser-based OAuth 2.1 authentication for HTTP MCP servers is handled in the Electron main process via non-blocking IPC invocations and an event stream: @@ -1580,7 +1580,7 @@ type McpOAuthLoginEvent = { - `McpServerStatus` includes: - `hasOauth: boolean` — whether the server has an encrypted OAuth secret stored in host-core (`secret:mcp::oauth`). - `authRequired: boolean` — flags that a connection attempt or `tools/call` returned HTTP 401 Unauthorized and user re-authentication is required. -- OAuth tokens (`accessToken`, `refreshToken`, `expiresAt`, `resource`, `clientId`) are persisted exclusively in host-core encrypted secrets under `secret:mcp::oauth` and never exposed to the renderer. +- OAuth tokens (`accessToken`, `refreshToken`, `expiresAt`, `resource`, `clientId`, `redirectUris`) are persisted exclusively in host-core encrypted secrets under `secret:mcp::oauth` and never exposed to the renderer. Authorization-server endpoints must be HTTPS (loopback HTTP is the only exception). Token-endpoint error bodies stay in main-process logs and are not copied into renderer events. ## 12c. Subagent API (D202) diff --git a/docs/spec/06-delivery/04-e2e-test-plan.md b/docs/spec/06-delivery/04-e2e-test-plan.md index e2fbafacd..69d29bcd6 100644 --- a/docs/spec/06-delivery/04-e2e-test-plan.md +++ b/docs/spec/06-delivery/04-e2e-test-plan.md @@ -6229,7 +6229,7 @@ identify the platform validation still needed. 6. Settings UI updates status to `Ready` with discovered tools, shows localized success toast, and displays `OAuth` badge. 7. When access token expires, `UserMcpRuntime` transparently uses refresh token to obtain a fresh access token without user prompt. 8. Moving the server via `mcp.transfer` preserves and re-keys the OAuth token secret under the destination ID. -- **Specs linked**: `03-runtime/01-ipc-protocol.md`, ADR 0281, ADR 0142 +- **Specs linked**: `03-runtime/01-ipc-protocol.md`, ADR 0283, ADR 0142 - **Acceptance**: E (tools & permissions), Security - **Milestone**: M5 - **Status**: Unit-covered (`apps/desktop/test/mcp-oauth.test.mjs`, `apps/desktop/test/user-mcp.test.mjs`); full UI journey Draft diff --git a/docs/zh-CN/spec/03-runtime/01-ipc-protocol.md b/docs/zh-CN/spec/03-runtime/01-ipc-protocol.md index d6e176b0f..d067d97ea 100644 --- a/docs/zh-CN/spec/03-runtime/01-ipc-protocol.md +++ b/docs/zh-CN/spec/03-runtime/01-ipc-protocol.md @@ -1229,7 +1229,7 @@ ASCII slug：frontmatter `name` 能 slugify 时用它，否则 `SKILL.md` 用技 - `pi-desktop/mcp/market/search` — `{ query?, sources[], more? }` → `{ entries, failedSources, exhausted }`。Main 校验源 URL，固定每个解析出的公网地址，只跟随有界的 HTTPS 重定向，并为 browse 与服务端搜索保留 cursor 状态。单个源失败不会丢弃成功源；响应和缓存均有界。 -### MCP OAuth（ADR 0281） +### MCP OAuth（ADR 0283） HTTP MCP 服务的基于浏览器的 OAuth 2.1 认证在 Electron 主进程中通过非阻塞 IPC 与事件流处理： @@ -1256,7 +1256,7 @@ type McpOAuthLoginEvent = { - `McpServerStatus` 包含： - `hasOauth: boolean` — 服务是否在 host-core 加密凭据库存储有 OAuth 凭据（`secret:mcp::oauth`）。 - `authRequired: boolean` — 连接握手或 `tools/call` 是否收到 HTTP 401 Unauthorized，提示用户需要认证/重新授权。 -- OAuth 令牌（`accessToken`, `refreshToken`, `expiresAt`, `resource`, `clientId`）仅持久化在 host-core 的加密 secret 中（`secret:mcp::oauth`），绝不向渲染层暴露。 +- OAuth 令牌（`accessToken`, `refreshToken`, `expiresAt`, `resource`, `clientId`, `redirectUris`）仅持久化在 host-core 的加密 secret 中（`secret:mcp::oauth`），绝不向渲染层暴露。授权服务器端点必须是 HTTPS（仅回环 HTTP 例外）。token 端点错误响应体只记入主进程日志，不进入渲染层事件。 ## 12c. 子代理 API (D202) diff --git a/docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md b/docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md index 464371718..c7b3500ef 100644 --- a/docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md +++ b/docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md @@ -4466,7 +4466,7 @@ IPC 请求无法关闭。 6. 设置界面状态更新为已连接及工具数量，弹出成功提示，并显示 OAuth 徽标。 7. 访问令牌过期时，`UserMcpRuntime` 透明使用 refresh token 换取新令牌，无需用户重新交互。 8. 通过 `mcp.transfer` 迁移服务器时，自动将 OAuth 令牌迁移至新 ID 下。 -- **链接规格**：`03-runtime/01-ipc-protocol.md`、ADR 0281、ADR 0142 +- **链接规格**：`03-runtime/01-ipc-protocol.md`、ADR 0283、ADR 0142 - **验收**：E（工具和权限）、安全性 - **里程碑**：M5 - **状态**：单元覆盖（`apps/desktop/test/mcp-oauth.test.mjs`、`apps/desktop/test/user-mcp.test.mjs`）；完整 UI 之旅草案