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).
This commit is contained in:
Ayaan Zaidi
2026-07-14 10:29:07 +05:30
parent 834561ad67
commit d50feceadb
11 changed files with 486 additions and 35 deletions

View File

@@ -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`,

View File

@@ -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<string>())
: undefined;

View File

@@ -36,14 +36,15 @@ export function sanitizeServerName(raw: string, usedNames: Set<string>): 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<string>): Map<string, string> {
const usedNames = new Set<string>();
const assignments = new Map<string, string>();
for (const serverName of [...serverNames].toSorted((a, b) => a.localeCompare(b))) {
for (const serverName of serverNames) {
assignments.set(serverName, sanitizeServerName(serverName, usedNames));
}
return assignments;

View File

@@ -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<string, (toolName: string) => 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();
});

View File

@@ -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<McpToolCatalog> | null = null;
let catalogInFlight: Promise<McpToolCatalog> | undefined;
const serverOwner = new Map<string, SessionMcpRuntime>();
@@ -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<McpToolCatalog> => {
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<void> => {
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) {

View File

@@ -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(),

View File

@@ -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;

View File

@@ -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([

View File

@@ -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<string,
return { staticServers, requesterScopedServerNames };
}
/**
* Debug-proxy capture and log redaction match registered exact values, not
* header names alone. Resolver output is credential material (auth headers,
* signed-URL query tokens), so register it before it can reach any transport.
*/
function registerResolvedConnectionSecrets(connection: McpServerConnectionResolved): void {
for (const value of Object.values(connection.headers ?? {})) {
registerSecretValueForRedaction(value);
// Scheme-prefixed values ("Bearer <token>"): 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).

View File

@@ -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([]);
});
});

View File

@@ -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;