From 2e70de9d32ae1a0c6bd9a906d411df160ffd87ee Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 18 Jul 2026 02:25:34 +0100 Subject: [PATCH] fix(mcp): own sandbox CSP fail-closed validation in the decoder (#110267) * fix(mcp): reject sandbox CSP metadata that normalizes to no policy A present ?csp= value that decodes to valid JSON but is not a usable CSP (e.g. null or a wrong-shaped object) normalized to undefined and was indistinguishable from an absent parameter, so the gateway served proxy HTML under the default policy. encodeCsp omits the query param entirely for such values, so a present-but-empty policy is never legitimate; throw so the sandbox endpoint fails closed with 400. Found by review on the revert of 73685b4e7c946; the gap predates that commit. * fix(mcp): treat empty csp query value as malformed, not absent ?csp= with an empty value passed the falsy absent-guard and served proxy HTML under the default policy. Only a truly absent parameter (null) may skip validation; an empty string now falls through to JSON.parse and fails closed with 400. * refactor(gateway): drop redundant sandbox CSP handler guard decodeMcpAppSandboxCsp now throws for every present-but-unusable value (1ca26c508db14 added the same fail-closed behavior at the handler seam), so the handler-level 'present but falsy' check is unreachable. Keep the invariant in the decoder, which owns policy decode semantics. * fix(test): reset diagnostic listener-presence mirror between non-isolated files resetOpenClawGlobalDiagnosticState clears the listener sets and deletes the diagnostic-events state key, but the listener-presence counts live under a separate globalThis record and survived, so hasInternalDiagnosticEventListeners() stayed true for every later file in the worker once any file leaked a registration (e.g. the import-time listener in src/logging/diagnostic-run-activity.ts whose stop handle is lost to the module-registry reset). Zero the counts to match the cleared sets. Root cause of the model-call-diagnostics flake in CI run 29624203224; #110288 added the victim-side reset, this fixes the class. --- src/agents/mcp-app-sandbox.ts | 12 ++++++++++-- src/gateway/mcp-app-sandbox-http.test.ts | 3 +++ src/gateway/mcp-app-sandbox-http.ts | 6 +----- test/non-isolated-runner.ts | 14 ++++++++++++++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/agents/mcp-app-sandbox.ts b/src/agents/mcp-app-sandbox.ts index 983793928e0f..4d8ac577598a 100644 --- a/src/agents/mcp-app-sandbox.ts +++ b/src/agents/mcp-app-sandbox.ts @@ -91,15 +91,23 @@ export function resolveMcpAppSandboxPort(gatewayPort: number, configuredPort?: n return sandboxPort; } +// Malformed input must throw: the gateway sandbox endpoint relies on it to fail +// closed with 400 instead of serving proxy HTML under a default policy. That +// includes valid JSON that is not a usable CSP — encodeCsp omits the query +// param entirely in that case, so a present-but-empty value is never legitimate. export function decodeMcpAppSandboxCsp(value: string | null): McpAppCsp | undefined { - if (!value) { + if (value === null) { return undefined; } if (value.length > MCP_APP_SANDBOX_CSP_MAX_ENCODED_BYTES) { throw new Error("MCP App CSP metadata is too large"); } const decoded = JSON.parse(Buffer.from(value, "base64url").toString("utf8")) as unknown; - return normalizeMcpAppCsp(decoded); + const normalized = normalizeMcpAppCsp(decoded); + if (!normalized) { + throw new Error("MCP App CSP metadata is not a valid policy"); + } + return normalized; } /** Trusted outer document. The untrusted app HTML is written only into its inner iframe. */ diff --git a/src/gateway/mcp-app-sandbox-http.test.ts b/src/gateway/mcp-app-sandbox-http.test.ts index 2698caadf44f..78eba7a0c022 100644 --- a/src/gateway/mcp-app-sandbox-http.test.ts +++ b/src/gateway/mcp-app-sandbox-http.test.ts @@ -49,6 +49,9 @@ describe("MCP App sandbox HTTP origin", () => { expect(request("/", "GET").res.statusCode).toBe(404); expect(request(buildMcpAppSandboxPath(), "POST").res.statusCode).toBe(404); expect(request(`${buildMcpAppSandboxPath()}?csp=not-json`).res.statusCode).toBe(400); + const jsonButNotCsp = Buffer.from("null", "utf8").toString("base64url"); + expect(request(`${buildMcpAppSandboxPath()}?csp=${jsonButNotCsp}`).res.statusCode).toBe(400); + expect(request(`${buildMcpAppSandboxPath()}?csp=`).res.statusCode).toBe(400); expect(request("http://[", "GET").res.statusCode).toBe(400); }); }); diff --git a/src/gateway/mcp-app-sandbox-http.ts b/src/gateway/mcp-app-sandbox-http.ts index 95ea9249dc50..015c34be0464 100644 --- a/src/gateway/mcp-app-sandbox-http.ts +++ b/src/gateway/mcp-app-sandbox-http.ts @@ -30,13 +30,9 @@ function handleMcpAppSandboxHttpRequest(req: IncomingMessage, res: ServerRespons return; } - const encodedCsp = url.searchParams.get("csp"); let csp; try { - csp = decodeMcpAppSandboxCsp(encodedCsp); - if (encodedCsp !== null && !csp) { - throw new Error("invalid MCP App sandbox policy"); - } + csp = decodeMcpAppSandboxCsp(url.searchParams.get("csp")); } catch { res.statusCode = 400; res.setHeader("Content-Type", "text/plain; charset=utf-8"); diff --git a/test/non-isolated-runner.ts b/test/non-isolated-runner.ts index f26970723924..1fe5bae43f30 100644 --- a/test/non-isolated-runner.ts +++ b/test/non-isolated-runner.ts @@ -28,6 +28,9 @@ const SHARED_TEST_SETUP = Symbol.for("openclaw.sharedTestSetup"); const EMBEDDED_RUN_STATE = Symbol.for("openclaw.embeddedRunState"); const REPLY_RUN_REGISTRY = Symbol.for("openclaw.replyRunRegistry"); const DIAGNOSTIC_EVENTS_STATE = Symbol.for("openclaw.diagnosticEvents.state.v1"); +const DIAGNOSTIC_EVENT_LISTENER_PRESENCE = Symbol.for( + "openclaw.diagnosticEventListenerPresence.v1", +); const nativeTimerGlobals = { setTimeout: globalThis.setTimeout, clearTimeout: globalThis.clearTimeout, @@ -230,6 +233,17 @@ function resetOpenClawGlobalDiagnosticState(): void { state?.toolExecutionListeners?.clear(); state?.asyncQueue?.splice(0); Reflect.deleteProperty(globalStore, DIAGNOSTIC_EVENTS_STATE); + // The listener-presence mirror is a separate globalThis record; clearing the + // sets above without zeroing it leaves hasInternalDiagnosticEventListeners() + // true for the next file (e.g. an import-time registration like + // diagnostic-run-activity.ts whose stop handle died with the module registry). + const presence = globalStore[DIAGNOSTIC_EVENT_LISTENER_PRESENCE] as + | { internalCount?: number; trustedCount?: number } + | undefined; + if (presence) { + presence.internalCount = 0; + presence.trustedCount = 0; + } } const SERIALIZED_RESOLVE_MOCKS = Symbol.for("openclaw.serializedResolveMocks");