From 62430d9f3aa4caae13b3b7b988cbd497eb936cf2 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Tue, 14 Apr 2026 13:32:05 -0700 Subject: [PATCH] Harden MCP loopback request validation (#66665) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): harden loopback request guards * fix(commit): block staged user log * Revert pre-commit USER.md guard from this PR Out of scope for the MCP loopback hardening — keep this PR focused on the loopback request gate and the bearer-comparison fix. The pre-commit worklog guard can land separately if maintainers want it. * changelog: note MCP loopback constant-time + Origin guard (#66665) * fix(mcp): allow loopback flows that browsers flag as cross-site The previous Sec-Fetch-Site early-return rejected legit local browser callers like a UI hosted on http://localhost: talking to MCP on http://127.0.0.1: — browsers report that host mismatch as cross-site even though both ends are loopback. checkBrowserOrigin already authorizes those via its local-loopback matcher (loopback peer + loopback Origin host), so route every Origin-bearing request through that helper and let it decide. Native MCP clients (no Origin header) continue to short-circuit through to the bearer check unchanged. Adds a regression test asserting that origin: http://localhost:43123, sec-fetch-site: cross-site from a loopback peer is accepted with a valid bearer. --------- Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + src/gateway/mcp-http.request.ts | 34 ++++++++++++- src/gateway/mcp-http.test.ts | 85 +++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28816334d5a..a74904a2376 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Docs: https://docs.openclaw.ai - Slack/native commands: fix option menus for slash commands such as `/verbose` when Slack renders native buttons by giving each button a unique action ID while still routing them through the shared `openclaw_cmdarg*` listener. Thanks @Wangmerlyn. - Feishu/webhook: harden the webhook transport and card-action replay guards to fail closed on missing `encryptKey` and blank callback tokens — refuse to start the webhook transport without an `encryptKey`, reject unsigned requests when no key is present instead of accepting them, and drop blank card-action tokens before the dedupe claim and dispatcher. Defense-in-depth over the already-closed monitor-account layer. (#66707) Thanks @eleqtrizit. - Agents/workspace files: route `agents.files.get`, `agents.files.set`, and workspace listing through the shared `fs-safe` helpers (`openFileWithinRoot`/`readFileWithinRoot`/`writeFileWithinRoot`), reject symlink aliases for allowlisted agent files, and have `fs-safe` resolve opened-file real paths from the file descriptor before falling back to path-based `realpath` so a symlink swap between `open` and `realpath` can no longer redirect the validated path off the intended inode. (#66636) Thanks @eleqtrizit. +- Gateway/MCP loopback: switch the `/mcp` bearer comparison from plain `!==` to constant-time `safeEqualSecret` (matching the convention every other auth surface in the codebase uses), and reject non-loopback browser-origin requests via `checkBrowserOrigin` before the auth gate runs. Loopback origins (`127.0.0.1:*`, `localhost:*`, same-origin) still go through, including the `localhost`↔`127.0.0.1` host mismatch that browsers flag as `Sec-Fetch-Site: cross-site`. (#66665) Thanks @eleqtrizit. ## 2026.4.14 diff --git a/src/gateway/mcp-http.request.ts b/src/gateway/mcp-http.request.ts index 5347314b2ae..687f97f4591 100644 --- a/src/gateway/mcp-http.request.ts +++ b/src/gateway/mcp-http.request.ts @@ -1,12 +1,15 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import { resolveMainSessionKey } from "../config/sessions.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { safeEqualSecret } from "../security/secret-equal.js"; import { normalizeOptionalLowercaseString, normalizeOptionalString, } from "../shared/string-coerce.js"; import { normalizeMessageChannel } from "../utils/message-channel.js"; import { getHeader } from "./http-utils.js"; +import { isLoopbackAddress } from "./net.js"; +import { checkBrowserOrigin } from "./origin-check.js"; const MAX_MCP_BODY_BYTES = 1_048_576; @@ -22,6 +25,29 @@ function resolveScopedSessionKey(cfg: OpenClawConfig, rawSessionKey: string | un return !trimmed || trimmed === "main" ? resolveMainSessionKey(cfg) : trimmed; } +function rejectsBrowserLoopbackRequest(req: IncomingMessage): boolean { + const origin = getHeader(req, "origin"); + if (!origin) { + // No Origin header → not a browser request. Native MCP clients + // (curl, codex CLI, scripted MCP clients) never set Origin; let + // them through to the bearer check. + return false; + } + + // Defer to checkBrowserOrigin. It already treats loopback peers + // talking to a loopback Origin as `local-loopback`, which covers + // the legitimate `localhost`↔`127.0.0.1` mismatch that browsers + // flag as `Sec-Fetch-Site: cross-site` even though both ends are + // local. A blanket cross-site early-return here would block that + // flow even with a valid bearer; the helper's isLocalClient + + // isLoopbackHost gating is the authoritative check. + return !checkBrowserOrigin({ + requestHost: getHeader(req, "host"), + origin, + isLocalClient: isLoopbackAddress(req.socket?.remoteAddress), + }).ok; +} + export function validateMcpLoopbackRequest(params: { req: IncomingMessage; res: ServerResponse; @@ -54,8 +80,14 @@ export function validateMcpLoopbackRequest(params: { return false; } + if (rejectsBrowserLoopbackRequest(params.req)) { + params.res.writeHead(403, { "Content-Type": "application/json" }); + params.res.end(JSON.stringify({ error: "forbidden" })); + return false; + } + const authHeader = getHeader(params.req, "authorization") ?? ""; - if (authHeader !== `Bearer ${params.token}`) { + if (!safeEqualSecret(authHeader, `Bearer ${params.token}`)) { params.res.writeHead(401, { "Content-Type": "application/json" }); params.res.end(JSON.stringify({ error: "unauthorized" })); return false; diff --git a/src/gateway/mcp-http.test.ts b/src/gateway/mcp-http.test.ts index 84c4131de31..6f8b4e2a47e 100644 --- a/src/gateway/mcp-http.test.ts +++ b/src/gateway/mcp-http.test.ts @@ -198,6 +198,91 @@ describe("mcp loopback server", () => { }); expect(response.status).toBe(415); }); + + it("rejects cross-origin browser requests before auth", async () => { + server = await startMcpLoopbackServer(0); + const response = await sendRaw({ + port: server.port, + headers: { + "content-type": "application/json", + origin: "https://evil.example", + "sec-fetch-site": "cross-site", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + + expect(response.status).toBe(403); + }); + + it("rejects non-loopback origins even without fetch metadata", async () => { + server = await startMcpLoopbackServer(0); + const response = await sendRaw({ + port: server.port, + headers: { + "content-type": "application/json", + origin: "https://evil.example", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + + expect(response.status).toBe(403); + }); + + it("allows loopback browser origins for local clients", async () => { + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + const response = await sendRaw({ + port: server.port, + token: runtime?.token, + headers: { + "content-type": "application/json", + origin: "http://127.0.0.1:43123", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + + expect(response.status).toBe(200); + }); + + it("allows same-origin browser requests from loopback clients", async () => { + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + const response = await sendRaw({ + port: server.port, + token: runtime?.token, + headers: { + "content-type": "application/json", + origin: `http://127.0.0.1:${server.port}`, + "sec-fetch-site": "same-origin", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + + expect(response.status).toBe(200); + }); + + it("allows cross-site fetch metadata when both ends are loopback (localhost ↔ 127.0.0.1)", async () => { + // Browsers report a request from a `http://localhost:` + // page to `http://127.0.0.1:` as Sec-Fetch-Site: + // cross-site even though both ends are loopback. The gate must + // not blanket-reject on the cross-site signal — checkBrowserOrigin + // already authorizes loopback origins from loopback peers via + // its `local-loopback` matcher. + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + const response = await sendRaw({ + port: server.port, + token: runtime?.token, + headers: { + "content-type": "application/json", + origin: "http://localhost:43123", + "sec-fetch-site": "cross-site", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + + expect(response.status).toBe(200); + }); }); describe("createMcpLoopbackServerConfig", () => {