Greptile Summary

This PR introduces a dedicated MCP loopback HTTP server (src/gateway/mcp-http.ts) that scopes tool access for claude-cli sessions and adds structured logging for auditability. The server design (stateless JSON-RPC over plain HTTP on 127.0.0.1, TTL-based tool cache with config-snapshot invalidation, 1 MB body cap, union schema flattening for Anthropic compatibility) is solid and well-tested.

Two issues need attention before merging:

• useResume guard removed for model arg (src/agents/cli-runner/helpers.ts line 359): The PR silently drops the !params.useResume guard from the modelArg branch while every other resume-sensitive argument (systemPromptArg, sessionArg) retains it. This is a cross-cutting behavioral change for all CLI backends on resume: if any backend's CLI rejects --model when resuming, all resume sessions will break.
• OPENCLAW_MCP_ACCOUNT_ID hardcoded to "" (src/agents/cli-runner.ts line 319): The account ID env-var is always set to an empty string, so the x-openclaw-account-id header the CLI forwards to the MCP server is always empty. The server treats an empty string as undefined, meaning account-scoped group tool policies are never applied for CLI MCP calls, even when the session belongs to a named account.

Confidence Score: 2/5

• Not safe to merge as-is — the unexplained removal of the useResume guard for model args is a potential regression for all resume sessions across all CLI backends.
• The MCP server and test code are well-written, but the unexplained useResume guard removal in helpers.ts is a behavioral change that affects every CLI backend on resume, not just claude-cli. This is not documented in the PR description and directly contradicts the "No breaking changes" claim. Combined with the hardcoded empty account ID that silently disables account-scoped tool policies, there are two functional correctness issues that should be resolved before merging.
• Pay close attention to src/agents/cli-runner/helpers.ts (model arg guard removal) and src/agents/cli-runner.ts (hardcoded empty account ID on line 319).

_{Last reviewed commit: 5d7ad6a}

/review

🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

1. 🟡 MCP loopback server ignores session/account scoping headers, resolving tools under main session

Description

In startMcpLoopbackServer, the MCP HTTP handler does not use client-provided scoping headers (e.g. x-session-key, x-openclaw-account-id). Instead it always resolves tools using the main session key and forces accountId to undefined.

This creates an access-control / policy-scoping bypass because resolveFilteredTools() uses sessionKey, messageProvider, and accountId to apply:

• resolveEffectiveToolPolicy({ sessionKey }) (agent/global/profile/provider policy selection)
• resolveGroupToolPolicy({ sessionKey, messageProvider, accountId }) (group/account-scoped tool policy)
• resolveSubagentToolPolicy() gated by isSubagentSessionKey(sessionKey)

By hard-coding sessionKey to resolveMainSessionKey(cfg) and dropping accountId, any caller that can present the loopback bearer token can:

• Obtain the main session’s tool set even if it intended/should be restricted to a subagent or different session
• Bypass group/account-specific tool restrictions (since accountId is never passed)

Notably, ensureMcpConfigFile() writes an MCP config that includes x-session-key and x-openclaw-account-id headers, and mcp-http.test.ts expects these headers to affect policy resolution, but the server implementation ignores them—meaning tool policy scoping is effectively not enforced for MCP.

Vulnerable code:

const cfg = loadConfig();
const reqSessionKey = resolveMainSessionKey(cfg);
const messageProvider =
  normalizeMessageChannel(getHeader(req, "x-openclaw-message-channel")) ?? undefined;
const accountId = undefined; // not overrideable via headers
const { tools, toolSchema } = getTools({
  cfg,
  sessionKey: reqSessionKey,
  messageProvider,
  accountId,
});

Recommendation

Use the request scoping headers when resolving tools, and validate them.

Minimum fix (respect headers):

const cfg = loadConfig();

const headerSessionKey = getHeader(req, "x-session-key")?.trim();
const sessionKey = headerSessionKey && headerSessionKey !== "main"
  ? headerSessionKey
  : resolveMainSessionKey(cfg);

const messageProvider =
  normalizeMessageChannel(getHeader(req, "x-openclaw-message-channel")) ?? undefined;

const accountId = getHeader(req, "x-openclaw-account-id")?.trim() || undefined;

const { tools, toolSchema } = getTools({ cfg, sessionKey, messageProvider, accountId });

Hardening recommendations (to prevent confused-deputy / privilege escalation):

• Bind authorization to scope: include sessionKey (and optionally accountId) in the token derivation (e.g., HMAC) or store per-session tokens, and reject mismatches.
• Validate x-session-key format (and optionally confirm x-openclaw-agent-id matches the agent parsed from sessionKey).
• Update/extend tests to ensure the handler actually passes sessionKey/accountId through to policy resolution and tool creation.

2. 🟡 MCP loopback token can be leaked to attacker-controlled local process due to predictable port and fail-open startup

Description

The gateway starts an MCP loopback HTTP server on a predictable port (gatewayPort + 1) but does not fail closed when binding that port fails.

If another local process binds the MCP port first (intentional attack or accidental conflict):

• startGatewayServer() logs a warning and continues running without the MCP server.
• The Claude CLI integration (via generated MCP config) will still attempt to connect to http://127.0.0.1:<gatewayPort+1>/mcp and will send the bearer token in the Authorization header.
• An attacker-controlled local process listening on that port can capture OPENCLAW_MCP_TOKEN (a credential used to access the real MCP endpoint) and later use it to call tools exposed by the gateway MCP server (cross-user local attack on multi-user systems).

Vulnerable behavior (fail-open on bind error):

const mcpPort = port + MCP_PORT_OFFSET;
let mcpServer: { close: () => Promise<void> } | undefined;
try {
  mcpServer = await startMcpLoopbackServer(mcpPort);
  ensureMcpConfigFile(path.join(os.homedir(), ".openclaw"), mcpPort);
  log.info(`MCP loopback server listening on http://127.0.0.1:${mcpPort}/mcp`);
} catch (err) {
  log.warn(`MCP loopback server failed to start: ${String(err)}`);
}

Security impact is amplified by the deterministic port selection (MCP_PORT_OFFSET = 1).

Recommendation

Fail closed and/or make the MCP port non-predictable.

Recommended mitigations:

1. Treat bind failures as fatal for MCP enablement and ensure clients are not pointed at the MCP endpoint unless it is confirmed listening.

   • If MCP is required for a given mode, abort startup on EADDRINUSE.
   • If MCP is optional, do not generate/advertise MCP config/env for clients when the server is not running.

2. Use an OS-assigned ephemeral port (or scan for a free port) rather than gatewayPort + 1, then write that chosen port into the MCP config file.

Example (pick a free port and fail closed on unexpected errors):

import net from "node:net";

async function pickFreePort(): Promise<number> {
  return await new Promise((resolve, reject) => {
    const s = net.createServer();
    s.listen(0, "127.0.0.1", () => {
      const addr = s.address();
      s.close(() => {
        if (typeof addr === "object" && addr?.port) resolve(addr.port);
        else reject(new Error("failed to resolve port"));
      });
    });
    s.on("error", reject);
  });
}

const mcpPort = await pickFreePort();
mcpServer = await startMcpLoopbackServer(mcpPort);
ensureMcpConfigFile(openclawDir, mcpPort);

3. Optionally: detect and explicitly handle EADDRINUSE with a clear error and guidance, instead of silently continuing.

3. 🔵 Symlink/race arbitrary file overwrite via predictable tmp file in ensureMcpConfigFile

Description

ensureMcpConfigFile() writes to a predictable path (mcp-<port>.json.tmp) using fs.writeFileSync() and then renames it into place.

If an attacker can write within openclawDir (e.g., the directory is group/other-writable due to permissive umask, or OPENCLAW_HOME points to a shared location), they can pre-create mcp-<port>.json.tmp as a symlink/hardlink to another file. writeFileSync(tmpPath, ...) will follow that link and overwrite the target with attacker-chosen JSON content.

This is a classic local file clobbering primitive and can become privilege escalation if the process runs with elevated privileges or writes into a directory writable by less-privileged users.

Vulnerable code:

const tmpPath = filePath + ".tmp";
fs.writeFileSync(tmpPath, content, { encoding: "utf-8", mode: 0o600 });
fs.renameSync(tmpPath, filePath);

Recommendation

Harden the write to be symlink/race resistant:

1. Ensure openclawDir is a real directory (not a symlink) with user-only permissions (POSIX 0700).
2. Write to a unique temp file created with O_EXCL (and ideally O_NOFOLLOW where supported), then fsync and rename.
3. Refuse to operate if any path component is insecure.

Example (POSIX-focused):

import fs from "node:fs";
import path from "node:path";
import crypto from "node:crypto";

function secureMkdir(dir: string) {
  fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
  try { fs.chmodSync(dir, 0o700); } catch {}
  const st = fs.lstatSync(dir);
  if (!st.isDirectory() || st.isSymbolicLink()) {
    throw new Error(`Unsafe openclawDir: ${dir}`);
  }
}

function atomicWriteNoFollow(target: string, content: string) {
  const dir = path.dirname(target);
  const tmp = path.join(dir, `${path.basename(target)}.${crypto.randomUUID()}.tmp`);
  ​// 'wx' => fail if exists (O_EXCL)
  fs.writeFileSync(tmp, content, { encoding: "utf-8", mode: 0o600, flag: "wx" });
  fs.renameSync(tmp, target);
}

Also consider using fs.openSync with platform-specific O_NOFOLLOW (when available) plus lstatSync/fstatSync verification.

Analyzed PR: #35676 at commit 94272d4

_{Last updated on: 2026-03-10T00:33:25Z}

/review

Closing: upstream not accepting merge at this time.

Landed maintainer-side via #60714.

• Land commit: 6a71bdc
• Merge commit: 3de09fb

Thanks @mylukin!