Harden MCP loopback request validation (#66665)

eleqtrizit · drobison00 · web-flow · commit 62430d9f3aa4 · 2026-04-14T14:32:05.000-06:00
* 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:<ui-port> talking to MCP on http://127.0.0.1:<mcp-port> — 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 <drobison@nvidia.com>
diff --git 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
@@ -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
@@ -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:<ui-port>`
+    // page to `http://127.0.0.1:<mcp-port>` 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", () => {
