From d50feceadbf37723afd2dbdab3bc9186496010ea Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Tue, 14 Jul 2026 10:29:07 +0530 Subject: [PATCH] fix(mcp): address maintainer review of requester-scoped connections Fail-closed app views for requester-scoped servers, tools.effective fingerprint parity, declaration-order safe names, fail-closed duplicate resolver registration, resolved-credential redaction registration, per-session LRU cap on idle requester runtimes, combined-catalog re-merge on part refresh, and SDK contract docs (timings, schema ownership). --- docs/plugins/sdk-overview.md | 26 ++- src/agents/agent-bundle-mcp-materialize.ts | 7 +- src/agents/agent-bundle-mcp-names.ts | 9 +- src/agents/agent-bundle-mcp-runtime.test.ts | 203 +++++++++++++++++- src/agents/agent-bundle-mcp-runtime.ts | 94 +++++++- ...agent-bundle-mcp-tools.materialize.test.ts | 24 +++ src/agents/agent-bundle-mcp-types.ts | 6 + src/agents/mcp-connection-resolver.test.ts | 23 ++ src/agents/mcp-connection-resolver.ts | 45 +++- ...ry-registrars-network.mcp-resolver.test.ts | 76 +++++++ src/plugins/registry-registrars-network.ts | 8 +- 11 files changed, 486 insertions(+), 35 deletions(-) create mode 100644 src/plugins/registry-registrars-network.mcp-resolver.test.ts diff --git a/docs/plugins/sdk-overview.md b/docs/plugins/sdk-overview.md index 7870e7a80cec..bda7f42a1ddd 100644 --- a/docs/plugins/sdk-overview.md +++ b/docs/plugins/sdk-overview.md @@ -237,14 +237,32 @@ Contract notes: - Resolver context carries trusted host identity only (`requesterSenderId`, optional `agentAccountId` / `messageChannel`). Future trusted fields (for example cron/subagent user context) can be added additively. -- Tool names and schemas stay requester-independent (prompt-cache stable). +- One plugin owns one server name: a duplicate + `registerMcpServerConnectionResolver` for the same `serverName` from another + plugin is rejected with an error diagnostic (first registration wins), so + connection ownership never depends on plugin load order. +- Tool names are derived from the full declared server set so partial resolution + never changes safe server names between requesters or turns. Core does not + verify that different requester endpoints serve identical tool schemas; a + resolver must point every requester at the same logical service, or tool + schemas (and prompt-cache stability) diverge per requester. - Runs without a trusted `requesterSenderId` (cron, subagent, heartbeat, public gateway) never materialize requester-scoped servers. There is no shared fallback connection. +- `resolve` is bounded at 10 seconds per server; a timeout or throw omits that + server for the run without failing static MCP. +- Resolved connections are revalidated at most every 5 minutes per requester: + rotation rebuilds the transport with fresh credentials, and a `null` result + revokes it (the cached runtime is disposed even mid-session). A revoked or + rotated credential can therefore stay in use for up to 5 minutes. - Resolved `headers` are never logged or persisted; core keeps only an ephemeral - in-memory keyed digest (process-local HMAC) to detect credential rotation. -- Tool names are derived from the full declared server set so partial resolution - never changes safe server names between requesters or turns. + in-memory keyed digest (process-local HMAC) to detect credential rotation, and + registers resolved header/URL credential values with the log/debug-capture + redaction registry. +- Requester-scoped servers do not mint MCP App views: a view outlives the + requester-authenticated run and the gateway view boundary has no requester + identity, so app previews stay fail-closed for these servers. Tool results + are unaffected. - Static servers without a resolver keep the existing session-scoped lifecycle. Memory prompt supplement builders receive optional `agentId`, diff --git a/src/agents/agent-bundle-mcp-materialize.ts b/src/agents/agent-bundle-mcp-materialize.ts index cb044fe803da..828a313e2a00 100644 --- a/src/agents/agent-bundle-mcp-materialize.ts +++ b/src/agents/agent-bundle-mcp-materialize.ts @@ -422,7 +422,12 @@ export async function materializeBundleMcpToolsForRun(params: { toolName: tool.toolName, result, }); - if (params.runtime.mcpAppsEnabled && tool.uiResourceUri) { + // Requester-scoped servers never mint app views: a view outlives the + // requester-authenticated run and the gateway view boundary (peek + + // transcript reconstruction) has no requester identity to recover it. + const requesterScopedServer = + params.runtime.isRequesterScopedServer?.(tool.serverName) === true; + if (params.runtime.mcpAppsEnabled && tool.uiResourceUri && !requesterScopedServer) { const allowedAppToolNames = allowedAppToolsByServer ? (allowedAppToolsByServer.get(tool.serverName) ?? new Set()) : undefined; diff --git a/src/agents/agent-bundle-mcp-names.ts b/src/agents/agent-bundle-mcp-names.ts index 280891ea7ec3..5bca186814c2 100644 --- a/src/agents/agent-bundle-mcp-names.ts +++ b/src/agents/agent-bundle-mcp-names.ts @@ -36,14 +36,15 @@ export function sanitizeServerName(raw: string, usedNames: Set): string } /** - * Assign safe server names from the full declared set (sorted), independent of - * which servers resolve for a requester. All runtimes in a session must share - * this map so tool names stay stable across partial resolution. + * Assign safe server names from the full declared set in declaration order, + * independent of which servers resolve for a requester. Declaration order + * preserves legacy collision-suffix ownership for existing static configs; + * sorting here would silently swap safe names between colliding servers. */ export function assignSafeServerNames(serverNames: Iterable): Map { const usedNames = new Set(); const assignments = new Map(); - for (const serverName of [...serverNames].toSorted((a, b) => a.localeCompare(b))) { + for (const serverName of serverNames) { assignments.set(serverName, sanitizeServerName(serverName, usedNames)); } return assignments; diff --git a/src/agents/agent-bundle-mcp-runtime.test.ts b/src/agents/agent-bundle-mcp-runtime.test.ts index 091d80b1fcee..548855755a18 100644 --- a/src/agents/agent-bundle-mcp-runtime.test.ts +++ b/src/agents/agent-bundle-mcp-runtime.test.ts @@ -2360,6 +2360,48 @@ describe("requester-scoped MCP connection resolution", () => { await manager.disposeAll(); }); + it("keeps the tools.effective config summary in fingerprint parity with the peeked runtime", async () => { + const { testing: resolverTesting } = await import("./mcp-connection-resolver.js"); + const { resolveSessionMcpConfigSummary } = await import("./agent-bundle-mcp-tools.js"); + const manager = testing.createSessionMcpRuntimeManager(); + const cfg = { + mcp: { + servers: { + shared: { command: "true" }, + "user-mail": { transport: "streamable-http", url: "https://static.example.test" }, + }, + }, + }; + + // Static-only config: the summary must match the bare runtime byte-for-byte. + const staticRuntime = await manager.getOrCreate({ + sessionId: "session-parity-static", + workspaceDir: "/workspace", + cfg: cfg as never, + }); + expect( + resolveSessionMcpConfigSummary({ workspaceDir: "/workspace", cfg: cfg as never }).fingerprint, + ).toBe(staticRuntime.configFingerprint); + + // With a resolver registered, tools.effective peeks the bare static-partition + // runtime; summary parity keeps it from reporting stale-config forever. + resolverTesting.setMcpServerConnectionResolversForTest([ + { serverName: "user-mail", resolve: async () => null }, + ]); + await manager.getOrCreate({ + sessionId: "session-parity-scoped", + workspaceDir: "/workspace", + cfg: cfg as never, + requesterSenderId: "sender-a", + }); + const peeked = manager.peekSession({ sessionId: "session-parity-scoped" }); + expect(peeked?.configFingerprint).toBe( + resolveSessionMcpConfigSummary({ workspaceDir: "/workspace", cfg: cfg as never }).fingerprint, + ); + + await manager.disposeAll(); + }); + it("skips connection resolve on requester cache hits", async () => { let resolveCalls = 0; const { testing: resolverTesting } = await import("./mcp-connection-resolver.js"); @@ -2839,6 +2881,145 @@ describe("requester-scoped MCP connection resolution", () => { await manager.disposeAll(); }); + it("evicts LRU idle requester runtimes past the per-session cap", async () => { + const { testing: resolverTesting } = await import("./mcp-connection-resolver.js"); + resolverTesting.setMcpServerConnectionResolversForTest([ + { + serverName: "user-mail", + resolve: async (ctx) => ({ url: `https://mcp.example.test/${ctx.requesterSenderId}` }), + }, + ]); + + const disposedSenders: string[] = []; + let syntheticLastUsedAt = 100_000; + const createRuntime: RuntimeFactory = (params) => { + const sender = params.requesterScope?.requesterSenderId; + const base = makeRuntime([{ toolName: "probe", description: "probe" }]); + // Distinct ascending lastUsedAt per runtime so LRU ordering is deterministic. + const lastUsedAt = (syntheticLastUsedAt += 1_000); + return { + ...base, + sessionId: params.sessionId, + workspaceDir: params.workspaceDir, + configFingerprint: params.configFingerprint ?? "fingerprint", + requesterScope: params.requesterScope, + get lastUsedAt() { + return lastUsedAt; + }, + markUsed: () => {}, + dispose: async () => { + if (sender) { + disposedSenders.push(sender); + } + }, + }; + }; + const manager = testing.createSessionMcpRuntimeManager({ + createRuntime, + // Pin the sweep clock near the synthetic lastUsedAt values so the idle + // TTL sweep never fires; this test exercises only the cap eviction. + now: () => 150_000, + maxIdleRequesterRuntimesPerSession: 2, + }); + const cfg = { + mcp: { servers: { "user-mail": { transport: "streamable-http" } } }, + }; + + for (const sender of ["sender-a", "sender-b", "sender-c"]) { + await manager.getOrCreate({ + sessionId: "session-cap", + workspaceDir: "/workspace", + cfg: cfg as never, + requesterSenderId: sender, + messageChannel: "telegram", + }); + } + + // sender-a is the least recently used zero-lease scoped runtime. + expect(disposedSenders).toEqual(["sender-a"]); + // Bare static reconcile key + two newest requester keys survive. + expect(manager.listRuntimeKeys()).toHaveLength(3); + + await manager.disposeAll(); + }); + + it("re-merges the combined catalog after a part refreshes on tools/list_changed", async () => { + const { testing: resolverTesting } = await import("./mcp-connection-resolver.js"); + resolverTesting.setMcpServerConnectionResolversForTest([ + { + serverName: "user-mail", + resolve: async () => ({ url: "https://mcp.example.test/user" }), + }, + ]); + + const makeCatalog = (serverName: string, toolName: string) => ({ + version: 1, + generatedAt: 0, + servers: { + [serverName]: { serverName, launchSummary: serverName, toolCount: 1 }, + }, + tools: [ + { + serverName, + safeServerName: serverName, + toolName, + description: toolName, + inputSchema: { type: "object", properties: {} }, + fallbackDescription: toolName, + }, + ], + }); + const swapCatalogByServer = new Map void>(); + const createRuntime: RuntimeFactory = (params) => { + const serverName = params.includeServerNames?.has("user-mail") ? "user-mail" : "shared"; + let current = makeCatalog(serverName, serverName === "user-mail" ? "send" : "shared_tool"); + swapCatalogByServer.set(serverName, (toolName) => { + current = makeCatalog(serverName, toolName); + }); + return { + ...makeRuntime([{ toolName: "unused", description: "unused" }]), + sessionId: params.sessionId, + workspaceDir: params.workspaceDir, + configFingerprint: params.configFingerprint ?? "fingerprint", + requesterScope: params.requesterScope, + peekCatalog: () => current, + getCatalog: async () => current, + }; + }; + const manager = testing.createSessionMcpRuntimeManager({ createRuntime }); + const runtime = await manager.getOrCreate({ + sessionId: "session-combined-refresh", + workspaceDir: "/workspace", + cfg: { + mcp: { + servers: { + shared: { command: "true" }, + "user-mail": { transport: "streamable-http" }, + }, + }, + } as never, + requesterSenderId: "sender-a", + messageChannel: "telegram", + }); + + const before = await runtime.getCatalog(); + expect(before.tools.map((tool) => tool.toolName).toSorted()).toEqual(["send", "shared_tool"]); + + // A part replacing its catalog (tools/list_changed refresh) must invalidate + // the merged facade cache instead of serving the stale combined catalog. + swapCatalogByServer.get("user-mail")?.("send_v2"); + const after = await runtime.getCatalog(); + expect(after.tools.map((tool) => tool.toolName).toSorted()).toEqual(["send_v2", "shared_tool"]); + expect( + runtime + .peekCatalog() + ?.tools.map((tool) => tool.toolName) + .toSorted(), + ).toEqual(["send_v2", "shared_tool"]); + + await manager.disposeAll(); + }); + it("disposes cached scoped runtime when revalidation resolves empty", async () => { let allow = true; const { testing: resolverTesting } = await import("./mcp-connection-resolver.js"); @@ -3196,9 +3377,10 @@ describe("requester-scoped MCP connection resolution", () => { it("uses full-set safe names independent of which servers resolve", async () => { const { assignSafeServerNames } = await import("./agent-bundle-mcp-names.js"); const fullSet = assignSafeServerNames(["mail.prod", "mail-prod", "shared"]); - // Sorted order: mail-prod, mail.prod, shared → first claim wins unsuffixed base. - expect(fullSet.get("mail-prod")).toBe("mail-prod"); - expect(fullSet.get("mail.prod")).toBe("mail-prod-2"); + // Declaration order: mail.prod declared first claims the unsuffixed base, + // matching legacy collision ownership for existing configs. + expect(fullSet.get("mail.prod")).toBe("mail-prod"); + expect(fullSet.get("mail-prod")).toBe("mail-prod-2"); expect(fullSet.get("shared")).toBe("shared"); let resolveBoth = true; @@ -3271,25 +3453,26 @@ describe("requester-scoped MCP connection resolution", () => { messageChannel: "telegram", }); - // Every create for this session received the same full-set assignments. + // Every create for this session received the same full-set assignments; + // declaration order gives "mail.prod" (declared first) the unsuffixed base. expect(passedMaps.length).toBeGreaterThan(1); for (const map of passedMaps) { - expect(map?.get("mail.prod")).toBe("mail-prod-2"); - expect(map?.get("mail-prod")).toBe("mail-prod"); + expect(map?.get("mail.prod")).toBe("mail-prod"); + expect(map?.get("mail-prod")).toBe("mail-prod-2"); } const catalogA = await runtimeA.getCatalog(); const catalogB = await runtimeB.getCatalog(); - expect(catalogA.servers["mail.prod"]?.safeServerName).toBe("mail-prod-2"); + expect(catalogA.servers["mail.prod"]?.safeServerName).toBe("mail-prod"); // B may only have static part if scoped omitted; shared names still match full-set map. if (catalogA.servers["mail-prod"]) { - expect(catalogA.servers["mail-prod"]?.safeServerName).toBe("mail-prod"); + expect(catalogA.servers["mail-prod"]?.safeServerName).toBe("mail-prod-2"); } - expect(catalogB.servers["mail.prod"]?.safeServerName).toBe("mail-prod-2"); + expect(catalogB.servers["mail.prod"]?.safeServerName).toBe("mail-prod"); // Merge preserves precomputed names (no further re-suffix). const merged = testing.mergeMcpToolCatalogs([catalogA, catalogB]); - expect(merged.servers["mail.prod"]?.safeServerName).toBe("mail-prod-2"); + expect(merged.servers["mail.prod"]?.safeServerName).toBe("mail-prod"); await manager.disposeAll(); }); diff --git a/src/agents/agent-bundle-mcp-runtime.ts b/src/agents/agent-bundle-mcp-runtime.ts index 219d8e06005a..9c96ec932772 100644 --- a/src/agents/agent-bundle-mcp-runtime.ts +++ b/src/agents/agent-bundle-mcp-runtime.ts @@ -76,6 +76,9 @@ const MCP_APPS_CLIENT_EXTENSION = "io.modelcontextprotocol/ui"; const MCP_APP_RESOURCE_MIME_TYPE = "text/html;profile=mcp-app"; const DEFAULT_SESSION_MCP_RUNTIME_IDLE_TTL_MS = 10 * 60 * 1000; const SESSION_MCP_RUNTIME_SWEEP_INTERVAL_MS = 60 * 1000; +// Bounds live per-sender MCP transports in one session between idle sweeps; +// far above concurrent-run parallelism, so active requesters never evict. +const SESSION_MCP_MAX_IDLE_REQUESTER_RUNTIMES = 64; const BUNDLE_MCP_FAILURE_THRESHOLD = 3; const BUNDLE_MCP_FAILURE_COOLDOWN_MS = 60_000; const BUNDLE_MCP_CATALOG_LIST_TIMEOUT_MS = 1_500; @@ -454,10 +457,26 @@ export function resolveSessionMcpConfigSummary(params: { logDiagnostics: false, manifestRegistry: params.manifestRegistry, }); - return { - fingerprint, - serverNames: Object.keys(loaded.mcpServers).toSorted((a, b) => a.localeCompare(b)), - }; + const serverNames = Object.keys(loaded.mcpServers).toSorted((a, b) => a.localeCompare(b)); + if (serverNames.length === 0) { + return { fingerprint, serverNames }; + } + // Mirror getOrCreate: the bare-keyed runtime folds full-set safe names into + // its fingerprint and excludes requester-scoped servers from its partition. + // Compare apples-to-apples or tools.effective reports stale-config forever. + const safeServerNamesByServer = assignSafeServerNames(Object.keys(loaded.mcpServers)); + const { requesterScopedServerNames } = partitionMcpServersByConnectionScope(loaded.mcpServers); + const { fingerprint: bareRuntimeFingerprint } = loadSessionMcpConfig({ + workspaceDir: params.workspaceDir, + cfg: params.cfg, + logDiagnostics: false, + manifestRegistry: params.manifestRegistry, + ...(requesterScopedServerNames.length > 0 + ? { excludeServerNames: new Set(requesterScopedServerNames) } + : {}), + safeServerNamesByServer, + }); + return { fingerprint: bareRuntimeFingerprint, serverNames }; } function createDisposedError(sessionId: string): Error { @@ -946,6 +965,8 @@ export function createSessionMcpRuntime(params: { agentDir: params.agentDir, configFingerprint, ...(params.requesterScope ? { requesterScope: params.requesterScope } : {}), + // A runtime partition hosts either only static or only requester-scoped servers. + isRequesterScopedServer: () => params.requesterScope !== undefined, mcpAppsEnabled, createdAt, get lastUsedAt() { @@ -1143,6 +1164,7 @@ function createCombinedSessionMcpRuntime(params: { const parts = params.parts; let lastUsedAt = Math.max(...parts.map((part) => part.lastUsedAt)); let cachedCatalog: McpToolCatalog | null = null; + let mergedSourceCatalogs: ReadonlyArray | null = null; let catalogInFlight: Promise | undefined; const serverOwner = new Map(); @@ -1152,8 +1174,16 @@ function createCombinedSessionMcpRuntime(params: { } }; + // Parts invalidate their own catalogs on tools/list_changed by replacing or + // clearing the cached object. Identity-compare against what was merged so the + // facade re-merges instead of serving a stale combined catalog. + const cachedCatalogIsCurrent = (): boolean => + cachedCatalog !== null && + mergedSourceCatalogs !== null && + parts.every((part, index) => part.peekCatalog() === mergedSourceCatalogs?.[index]); + const loadCatalog = async (): Promise => { - if (cachedCatalog && serverOwner.size > 0) { + if (cachedCatalog && cachedCatalogIsCurrent()) { return cachedCatalog; } if (catalogInFlight) { @@ -1161,9 +1191,11 @@ function createCombinedSessionMcpRuntime(params: { } const inFlight = (async () => { const catalogs = await Promise.all(parts.map((part) => part.getCatalog())); + serverOwner.clear(); for (let index = 0; index < parts.length; index += 1) { rememberServerOwners(catalogs[index]!, parts[index]!); } + mergedSourceCatalogs = catalogs; cachedCatalog = mergeMcpToolCatalogs(catalogs); return cachedCatalog; })(); @@ -1198,6 +1230,10 @@ function createCombinedSessionMcpRuntime(params: { workspaceDir: params.workspaceDir, agentDir: params.agentDir, configFingerprint: parts.map((part) => part.configFingerprint).join(":"), + isRequesterScopedServer(serverName) { + // Owner map is populated by the catalog load that exposed the tool. + return serverOwner.get(serverName)?.requesterScope !== undefined; + }, mcpAppsEnabled: parts.some((part) => part.mcpAppsEnabled === true), createdAt: Math.min(...parts.map((part) => part.createdAt)), get lastUsedAt() { @@ -1221,7 +1257,7 @@ function createCombinedSessionMcpRuntime(params: { }, getCatalog: loadCatalog, peekCatalog() { - if (cachedCatalog) { + if (cachedCatalog && cachedCatalogIsCurrent()) { return cachedCatalog; } const peeked = parts.map((part) => part.peekCatalog()); @@ -1294,6 +1330,7 @@ function createSessionMcpRuntimeManager( now?: () => number; enableIdleSweepTimer?: boolean; idleSweepIntervalMs?: number; + maxIdleRequesterRuntimesPerSession?: number; } = {}, ): SessionMcpRuntimeManager { // Keys are bare sessionId for static runtimes, or requester composite JSON keys. @@ -1400,6 +1437,50 @@ function createSessionMcpRuntimeManager( return expired.length; }; + const maxIdleRequesterRuntimes = + opts.maxIdleRequesterRuntimesPerSession ?? SESSION_MCP_MAX_IDLE_REQUESTER_RUNTIMES; + + /** + * A busy shared channel can otherwise accumulate one live scoped runtime per + * sender until the idle TTL fires. Evict LRU zero-lease requester runtimes + * beyond the cap; leased runtimes and the bare static runtime never evict. + */ + const enforceRequesterRuntimeCap = async ( + sessionId: string, + keepRuntimeKey: string, + ): Promise => { + const requesterKeys = runtimeKeysForSessionId(sessionId).filter( + (runtimeKey) => runtimeKey !== sessionId, + ); + const overflow = requesterKeys.length - maxIdleRequesterRuntimes; + if (overflow <= 0) { + return; + } + const evictable = requesterKeys + .filter((runtimeKey) => runtimeKey !== keepRuntimeKey) + .map((runtimeKey) => ({ runtimeKey, runtime: runtimesBySessionId.get(runtimeKey) })) + .filter( + (entry): entry is { runtimeKey: string; runtime: SessionMcpRuntime } => + entry.runtime !== undefined && (entry.runtime.activeLeases ?? 0) === 0, + ) + .toSorted((a, b) => a.runtime.lastUsedAt - b.runtime.lastUsedAt) + .slice(0, overflow); + for (const { runtimeKey, runtime } of evictable) { + // Serialize with in-flight work on that key so eviction cannot clobber a + // concurrent reuse or install for the same requester. + await runExclusiveOnRuntimeKey(runtimeKey, async () => { + const current = runtimesBySessionId.get(runtimeKey); + if (current !== runtime || (current.activeLeases ?? 0) > 0) { + return; + } + runtimesBySessionId.delete(runtimeKey); + idleTtlMsBySessionId.delete(runtimeKey); + connectionMetaByRuntimeKey.delete(runtimeKey); + await current.dispose(); + }); + } + }; + const queueIdleSweep = () => { if (idleSweepInFlight) { return; @@ -1871,6 +1952,7 @@ function createSessionMcpRuntimeManager( if (scopedRuntime) { parts.push(scopedRuntime); } + await enforceRequesterRuntimeCap(params.sessionId, runtimeKey); } if (parts.length === 0) { diff --git a/src/agents/agent-bundle-mcp-tools.materialize.test.ts b/src/agents/agent-bundle-mcp-tools.materialize.test.ts index 0d466f386f14..3c4c1f9d5a6d 100644 --- a/src/agents/agent-bundle-mcp-tools.materialize.test.ts +++ b/src/agents/agent-bundle-mcp-tools.materialize.test.ts @@ -198,6 +198,30 @@ describe("createBundleMcpToolRuntime", () => { ).toEqual(new Set(["show"])); }); + it("never mints app views for tools from requester-scoped servers", async () => { + const tool: McpCatalogTool = { + serverName: "user-mail", + safeServerName: "user-mail", + toolName: "show", + inputSchema: { type: "object" }, + fallbackDescription: "show", + uiResourceUri: "ui://user-mail/app", + }; + const sessionRuntime = makeToolRuntime({ tools: [tool], serverName: "user-mail" }); + sessionRuntime.mcpAppsEnabled = true; + // View recovery (peek + transcript reconstruction) has no requester + // identity, so scoped servers stay fail-closed at view creation. + sessionRuntime.isRequesterScopedServer = (serverName) => serverName === "user-mail"; + const materialized = await materializeBundleMcpToolsForRun({ runtime: sessionRuntime }); + + const result = await expectDefined( + materialized.tools[0], + "materialized.tools[0] test invariant", + ).execute("call-1", {}, undefined, undefined); + expect(result.details ?? {}).not.toHaveProperty("mcpAppPreview"); + expect(mcpAppMocks.fetchMcpAppView).not.toHaveBeenCalled(); + }); + it("materializes bundle MCP tools and executes them", async () => { const runtime = await materializeBundleMcpToolsForRun({ runtime: makeToolRuntime(), diff --git a/src/agents/agent-bundle-mcp-types.ts b/src/agents/agent-bundle-mcp-types.ts index c58b410dae5a..4fc473b0ed61 100644 --- a/src/agents/agent-bundle-mcp-types.ts +++ b/src/agents/agent-bundle-mcp-types.ts @@ -92,6 +92,12 @@ export type SessionMcpRuntime = { configFingerprint: string; /** Present when this runtime is keyed by requester-scoped connection identity. */ requesterScope?: SessionMcpRequesterScope; + /** + * True when the named server's connection is requester-scoped. App views for + * such servers stay fail-closed: views outlive the requester-authenticated + * run and the gateway view boundary carries no requester identity. + */ + isRequesterScopedServer?: (serverName: string) => boolean; mcpAppsEnabled?: boolean; createdAt: number; lastUsedAt: number; diff --git a/src/agents/mcp-connection-resolver.test.ts b/src/agents/mcp-connection-resolver.test.ts index 96c5cfa4c5ee..dedd23c466fa 100644 --- a/src/agents/mcp-connection-resolver.test.ts +++ b/src/agents/mcp-connection-resolver.test.ts @@ -62,6 +62,29 @@ describe("mcp connection resolver helpers", () => { ).resolves.toEqual(new Map([["user-mail", { url: "https://example.test/ok" }]])); }); + it("registers resolved header and signed-URL credentials for redaction", async () => { + const { isSecretValueRegisteredForRedaction, resetSecretRedactionRegistryForTest } = + await import("../logging/secret-redaction-registry.js"); + resetSecretRedactionRegistryForTest(); + testing.setMcpServerConnectionResolversForTest([ + { + serverName: "user-mail", + resolve: async () => ({ + url: "https://mcp.example.test/mail?sig=placeholder-signature", + headers: { Authorization: "Bearer test-auth-token" }, + }), + }, + ]); + await resolveRequesterScopedMcpConnections({ + serverNames: ["user-mail"], + requesterSenderId: "sender", + }); + expect(isSecretValueRegisteredForRedaction("Bearer test-auth-token")).toBe(true); + expect(isSecretValueRegisteredForRedaction("test-auth-token")).toBe(true); + expect(isSecretValueRegisteredForRedaction("placeholder-signature")).toBe(true); + resetSecretRedactionRegistryForTest(); + }); + it("contains per-server resolve throws without rejecting the map", async () => { const logWarn = vi.spyOn(await import("../logger.js"), "logWarn").mockImplementation(() => {}); testing.setMcpServerConnectionResolversForTest([ diff --git a/src/agents/mcp-connection-resolver.ts b/src/agents/mcp-connection-resolver.ts index f9ccdc9abdb9..fa64d5dcbe87 100644 --- a/src/agents/mcp-connection-resolver.ts +++ b/src/agents/mcp-connection-resolver.ts @@ -6,6 +6,7 @@ import crypto from "node:crypto"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; import { resolveOpenClawMcpTransportAlias } from "../config/mcp-config-normalize.js"; import { logWarn } from "../logger.js"; +import { registerSecretValueForRedaction } from "../logging/secret-redaction-registry.js"; import { getActivePluginRegistry } from "../plugins/runtime.js"; import type { McpServerConnectionResolved, @@ -142,7 +143,8 @@ function listMcpServerConnectionResolversByServerName(): Map< if (!serverName || typeof entry.resolver.resolve !== "function") { continue; } - // Last registration wins when multiple plugins claim the same server. + // The registry registrar rejects duplicate serverName claims across + // plugins, so entries here are unique per server. byName.set(serverName, { pluginId: entry.pluginId, serverName, @@ -172,6 +174,34 @@ export function partitionMcpServersByConnectionScope(mcpServers: Record"): the bare token can appear + // alone in captured payloads, so register it separately. + const bareToken = value.trim().split(/\s+/).at(-1); + if (bareToken && bareToken !== value) { + registerSecretValueForRedaction(bareToken); + } + } + try { + const url = new URL(connection.url); + for (const queryValue of url.searchParams.values()) { + registerSecretValueForRedaction(queryValue); + } + if (url.password) { + registerSecretValueForRedaction(url.password); + } + } catch { + // Invalid URLs never reach a transport; nothing to redact. + } +} + /** * Resolve requester-scoped server connections. Fail closed without requesterSenderId: * returns an empty map (no shared-connection fallback). Per-server resolve errors and @@ -223,13 +253,12 @@ export async function resolveRequesterScopedMcpConnections(params: { .toSorted(([a], [b]) => a.localeCompare(b)), ) : undefined; - return { - serverName, - connection: { - url: result.url.trim(), - ...(headers && Object.keys(headers).length > 0 ? { headers } : {}), - } satisfies McpServerConnectionResolved, - }; + const connection = { + url: result.url.trim(), + ...(headers && Object.keys(headers).length > 0 ? { headers } : {}), + } satisfies McpServerConnectionResolved; + registerResolvedConnectionSecrets(connection); + return { serverName, connection }; } catch (error) { // External plugin boundary: never fail the whole MCP run for one resolver. // Fixed classification only — no dynamic error text (plugin-controlled / secret-bearing). diff --git a/src/plugins/registry-registrars-network.mcp-resolver.test.ts b/src/plugins/registry-registrars-network.mcp-resolver.test.ts new file mode 100644 index 000000000000..874bf7a548ea --- /dev/null +++ b/src/plugins/registry-registrars-network.mcp-resolver.test.ts @@ -0,0 +1,76 @@ +/** Verifies MCP connection resolver registration ownership is fail-closed. */ +import { describe, expect, it } from "vitest"; +import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { createPluginRegistry } from "./registry.js"; +import type { PluginRuntime } from "./runtime/types.js"; +import { createPluginRecord } from "./status.test-fixtures.js"; + +function createRegistryHarness() { + const pluginRegistry = createPluginRegistry({ + logger: { + info() {}, + warn() {}, + error() {}, + debug() {}, + }, + runtime: {} as PluginRuntime, + activateGlobalSideEffects: false, + }); + const config = {} as OpenClawConfig; + const apiFor = (id: string) => { + const record = createPluginRecord({ id, source: `/plugins/${id}/index.ts` }); + pluginRegistry.registry.plugins.push(record); + return pluginRegistry.createApi(record, { config }); + }; + return { pluginRegistry, apiFor }; +} + +describe("registerMcpServerConnectionResolver ownership", () => { + it("rejects a duplicate serverName from another plugin with an error diagnostic", () => { + const { pluginRegistry, apiFor } = createRegistryHarness(); + const firstResolve = async () => null; + apiFor("plugin-a").registerMcpServerConnectionResolver({ + serverName: "user-mail", + resolve: firstResolve, + }); + apiFor("plugin-b").registerMcpServerConnectionResolver({ + serverName: "user-mail", + resolve: async () => ({ url: "https://mcp.example.test/hijack" }), + }); + + expect(pluginRegistry.registry.mcpServerConnectionResolvers).toHaveLength(1); + expect(pluginRegistry.registry.mcpServerConnectionResolvers[0]).toMatchObject({ + pluginId: "plugin-a", + resolver: { serverName: "user-mail", resolve: firstResolve }, + }); + expect(pluginRegistry.registry.diagnostics).toContainEqual( + expect.objectContaining({ + level: "error", + pluginId: "plugin-b", + message: expect.stringContaining('already registered by plugin "plugin-a"'), + }), + ); + }); + + it("lets the owning plugin replace its own resolver", () => { + const { pluginRegistry, apiFor } = createRegistryHarness(); + const api = apiFor("plugin-a"); + const replacement = async () => null; + api.registerMcpServerConnectionResolver({ + serverName: "user-mail", + resolve: async () => null, + }); + api.registerMcpServerConnectionResolver({ + serverName: "user-mail", + resolve: replacement, + }); + + expect(pluginRegistry.registry.mcpServerConnectionResolvers).toHaveLength(1); + expect(pluginRegistry.registry.mcpServerConnectionResolvers[0]?.resolver.resolve).toBe( + replacement, + ); + expect( + pluginRegistry.registry.diagnostics.filter((diagnostic) => diagnostic.level === "error"), + ).toEqual([]); + }); +}); diff --git a/src/plugins/registry-registrars-network.ts b/src/plugins/registry-registrars-network.ts index 6c2ee3e9dfb4..206658dd7604 100644 --- a/src/plugins/registry-registrars-network.ts +++ b/src/plugins/registry-registrars-network.ts @@ -258,13 +258,17 @@ export function createNetworkRegistrars(state: PluginRegistryState) { }; if (existingIndex >= 0) { const existing = registry.mcpServerConnectionResolvers[existingIndex]; + // Resolver ownership is an authorization boundary: connection identity + // must not depend on plugin load order. First registration wins; a + // duplicate from another plugin is rejected, not silently replaced. if (existing && existing.pluginId !== record.id) { pushDiagnostic({ - level: "warn", + level: "error", pluginId: record.id, source: record.source, - message: `MCP server connection resolver for "${serverName}" replaces registration from plugin "${existing.pluginId}"`, + message: `MCP server connection resolver for "${serverName}" rejected: already registered by plugin "${existing.pluginId}"`, }); + return; } registry.mcpServerConnectionResolvers[existingIndex] = registration; return;