mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 09:20:43 +00:00
Harden MCP loopback request validation (#66665)
* 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>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user