diff --git a/server/drivers/claude.test.ts b/server/drivers/claude.test.ts index 1ed739ce5..48a1b347f 100644 --- a/server/drivers/claude.test.ts +++ b/server/drivers/claude.test.ts @@ -6,7 +6,7 @@ // These used to be POSIX-only: the fake CLI is a shebang script Windows // cannot exec, and the broker is a unix socket. Both now go through // resolveCliSpawn / permissionSocketPath, so they run everywhere. -import { chmodSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; import { connect } from "node:net"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; @@ -155,11 +155,13 @@ describe("ClaudeDriver turns (fake CLI)", () => { await recorder.until((e) => e.type === "turn.completed"); const seen = JSON.parse(readFileSync(dump, "utf8")); - const mcpConfig = JSON.parse(seen.argv[seen.argv.indexOf("--mcp-config") + 1]); - expect(mcpConfig.mcpServers.agents).toMatchObject({ + expect(seen.mcpConfig.mcpServers.agents).toMatchObject({ args: ["/fake/agents-proxy.js"], env: { OMB_BOT_ID: "b1", OMB_COMMS_TOKEN: "tok" }, }); + // the config goes in a private file, never on argv, where `ps` would + // show the comms token to every other user on the machine + expect(JSON.stringify(seen.argv)).not.toContain("tok"); const allowed = seen.argv[seen.argv.indexOf("--allowedTools") + 1]; expect(allowed).toContain("mcp__agents"); }); @@ -177,9 +179,8 @@ describe("ClaudeDriver turns (fake CLI)", () => { await recorder.until((e) => e.type === "turn.completed"); const seen = JSON.parse(readFileSync(dump, "utf8")); - const mcpConfig = JSON.parse(seen.argv[seen.argv.indexOf("--mcp-config") + 1]); - expect(mcpConfig.mcpServers.dweb.args[0]).toMatch(/[\\/]drivers[\\/]dweb-proxy\.(?:ts|js)$/); - expect(mcpConfig.mcpServers.dweb.env.DWEB_URL).toBe("http://127.0.0.1:49737"); + expect(seen.mcpConfig.mcpServers.dweb.args[0]).toMatch(/[\\/]drivers[\\/]dweb-proxy\.(?:ts|js)$/); + expect(seen.mcpConfig.mcpServers.dweb.env.DWEB_URL).toBe("http://127.0.0.1:49737"); expect(seen.argv[seen.argv.indexOf("--allowedTools") + 1]).toContain("mcp__dweb"); }); @@ -200,15 +201,39 @@ describe("ClaudeDriver turns (fake CLI)", () => { await recorder.until((e) => e.type === "turn.completed"); const seen = JSON.parse(readFileSync(dump, "utf8")); - const mcpConfig = JSON.parse(seen.argv[seen.argv.indexOf("--mcp-config") + 1]); - expect(mcpConfig.mcpServers.composio).toMatchObject({ + expect(seen.mcpConfig.mcpServers.composio).toMatchObject({ type: "http", url: "https://connect.composio.dev/mcp", headers: { "x-consumer-api-key": "ck_test" }, }); + // the user's Composio key must not be readable via `ps` + expect(JSON.stringify(seen.argv)).not.toContain("ck_test"); expect(seen.argv[seen.argv.indexOf("--allowedTools") + 1]).toContain("mcp__composio"); }); + // the config file holds live credentials, so it must not outlive the turn — + // including when the CLI dies mid-turn, which is the path that leaks if + // cleanup is hung off the happy-path result instead of settle() + it.each([ + ["a completed turn", "happy"], + ["a crashed turn", "exit-early"], + ])("deletes the mcp config file after %s", async (_label, mode) => { + await create(mode); + const dump = join(scratch, "dump.json"); + process.env.FAKE_CLAUDE_DUMP = dump; + + await instance.adapter.sendTurn({ threadId: "t-cleanup", text: "hi", integrations: { composio: { key: "ck_x" } } }); + await recorder.until((e) => e.type === "turn.completed"); + + const configPath = (() => { + const seen = JSON.parse(readFileSync(dump, "utf8")); + return seen.argv[seen.argv.indexOf("--mcp-config") + 1] as string; + })(); + expect(configPath).toMatch(/omb-mcp-/); + expect(existsSync(configPath)).toBe(false); + expect(existsSync(dirname(configPath))).toBe(false); + }); + it("resumes with --resume when a cursor exists and reports that session id", async () => { await create(); const dump = join(scratch, "dump.json"); diff --git a/server/drivers/claude.ts b/server/drivers/claude.ts index a149cf887..515b82eb6 100644 --- a/server/drivers/claude.ts +++ b/server/drivers/claude.ts @@ -8,9 +8,9 @@ // - Composio Connect (connected apps → tools) over streamable HTTP // - the bot's cloud computer (box.ascii.dev) via server/computer-proxy.ts // — screenshot/exec/open_url, the CUA-on-the-box bridge -import { existsSync, unlinkSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; import { createServer as createNetServer } from "node:net"; -import { homedir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; @@ -378,8 +378,17 @@ export const ClaudeDriver: ProviderDriver = { mcpServers.ogb = { command: process.execPath, args: [PERM_PROXY_PATH, socketPath], env: { ...NODE_ENV_FLAG } }; allowed.push("mcp__ogb"); } + // The MCP config carries credentials — a Composio consumer key in a + // header, the box token in the computer proxy's env, the comms token in + // the agents proxy's env. On argv every one of those is world-readable + // through `ps` for the life of the turn, to any local process. The CLI + // accepts a FILE for this flag, so the secrets go in a 0600 file that + // is removed when the turn settles. + let mcpConfigPath: string | null = null; if (Object.keys(mcpServers).length) { - args.push("--mcp-config", JSON.stringify({ mcpServers })); + mcpConfigPath = join(mkdtempSync(join(tmpdir(), "omb-mcp-")), "mcp.json"); + writeFileSync(mcpConfigPath, JSON.stringify({ mcpServers }), { mode: 0o600 }); + args.push("--mcp-config", mcpConfigPath); args.push("--allowedTools", allowed.join(",")); } @@ -396,6 +405,12 @@ export const ClaudeDriver: ProviderDriver = { if (settled) return; settled = true; broker?.close(); + // the config file holds live credentials — it must not outlive the turn + if (mcpConfigPath) { + try { + rmSync(dirname(mcpConfigPath), { recursive: true, force: true }); + } catch {} + } active.delete(threadId); emit({ ...base(threadId, turnId), type: "turn.completed", ok, stopReason, cost }); }; diff --git a/server/drivers/native.test.ts b/server/drivers/native.test.ts new file mode 100644 index 000000000..5f83dd2c2 --- /dev/null +++ b/server/drivers/native.test.ts @@ -0,0 +1,57 @@ +// The native tee is the file people paste into bug reports, so the wiring +// that keeps credentials out of it is tested at the writer — redact.test.ts +// covers the masking function, this covers that appendNative actually calls it. +// (server/testing/setup.ts points HOME at a throwaway dir, so NATIVE_DIR is +// already isolated from the real fleet.) +import { readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; +import { beforeAll, describe, expect, it } from "vitest"; + +import { ensureDirs, NATIVE_DIR } from "../config.ts"; +import { appendNative } from "./native.ts"; + +beforeAll(() => ensureDirs()); + +describe("appendNative", () => { + it("masks the tokens an ACP session/new hands the agent", () => { + appendNative("t-native", { + dir: "out", + source: "acp", + msg: { + method: "session/new", + params: { + mcpServers: [ + { + name: "computer", + env: [ + { name: "OGB_BOX_ID", value: "box-7" }, + { name: "OGB_BOX_TOKEN", value: "box_live_dontlogme" }, + ], + }, + ], + }, + }, + }); + + const log = readFileSync(join(NATIVE_DIR, "t-native.ndjson"), "utf8"); + expect(log).not.toContain("box_live_dontlogme"); + // the shape a debugger needs is still there: which server, which var + expect(log).toContain("session/new"); + expect(log).toContain("OGB_BOX_TOKEN"); + expect(log).toContain("box-7"); + }); + + it("writes the log private to the user", () => { + appendNative("t-mode", { dir: "in", source: "acp", msg: { hello: "world" } }); + const mode = statSync(join(NATIVE_DIR, "t-mode.ndjson")).mode & 0o777; + // Windows does not implement POSIX modes; everywhere else, owner-only + if (process.platform !== "win32") expect(mode).toBe(0o600); + }); + + it("never throws, whatever it is handed", () => { + expect(() => appendNative("t-bad", { dir: "in", source: "acp", msg: undefined })).not.toThrow(); + const cyclic: Record = {}; + cyclic.self = cyclic; + expect(() => appendNative("t-cyclic", { dir: "in", source: "acp", msg: cyclic })).not.toThrow(); + }); +}); diff --git a/server/drivers/native.ts b/server/drivers/native.ts index 620bf27b8..76730c86b 100644 --- a/server/drivers/native.ts +++ b/server/drivers/native.ts @@ -6,12 +6,19 @@ import { appendFileSync } from "node:fs"; import { join } from "node:path"; import { NATIVE_DIR } from "../config.ts"; +import { redactSecrets } from "../redact.ts"; export function appendNative(threadId: string, entry: { dir: "in" | "out"; source: string; msg: unknown }) { try { + // The session-setup messages carry the credentials the agent is handed — + // the box and comms tokens ride inside session/new's mcpServers env, and + // an MCP header can carry a Composio key. These files are ordinary + // 0644 files people paste into bug reports, so values are masked while + // the shape stays intact. appendFileSync( join(NATIVE_DIR, `${threadId}.ndjson`), - JSON.stringify({ at: new Date().toISOString(), ...entry }) + "\n", + JSON.stringify({ at: new Date().toISOString(), ...entry, msg: redactSecrets(entry.msg) }) + "\n", + { mode: 0o600 }, ); } catch { /* never let logging break a run */ diff --git a/server/redact.test.ts b/server/redact.test.ts new file mode 100644 index 000000000..3e59e351e --- /dev/null +++ b/server/redact.test.ts @@ -0,0 +1,103 @@ +// The native log must keep the shape of a session-setup message and lose the +// credential values. These tests use the exact shapes the drivers actually +// write — the ACP `env: [{name,value}]` wire form and the claude mcpServers +// object form — so a change to either shape breaks the test, not the secret. +import { describe, expect, it } from "vitest"; + +import { redactSecrets } from "./redact.ts"; + +const flat = (value: unknown) => JSON.stringify(value); + +describe("redactSecrets", () => { + it("masks the tokens in an ACP session/new, keeping the shape", () => { + const sessionNew = { + jsonrpc: "2.0", + id: 3, + method: "session/new", + params: { + cwd: "/Users/someone", + mcpServers: [ + { + name: "agents", + command: "/usr/bin/node", + args: ["/app/agents-proxy.js"], + env: [ + { name: "OMB_BOT_ID", value: "bot-123" }, + { name: "OMB_COMMS_TOKEN", value: "s3cret-comms-token-value" }, + ], + }, + { + name: "computer", + command: "/usr/bin/node", + args: ["/app/computer-proxy.js"], + env: [ + { name: "OGB_BOX_ID", value: "box-9" }, + { name: "OGB_BOX_TOKEN", value: "box_live_abcdefghijklmnop" }, + ], + }, + ], + }, + }; + + const out = flat(redactSecrets(sessionNew)); + + expect(out).not.toContain("s3cret-comms-token-value"); + expect(out).not.toContain("box_live_abcdefghijklmnop"); + // shape survives: still the same method, servers, names and non-secret env + expect(out).toContain("session/new"); + expect(out).toContain("OMB_COMMS_TOKEN"); + expect(out).toContain("OGB_BOX_TOKEN"); + expect(out).toContain("bot-123"); + expect(out).toContain("box-9"); + expect(out).toContain("/app/agents-proxy.js"); + // and it says how long the value was, which is what you debug with + expect(out).toContain("«redacted 24 chars»"); + }); + + it("masks a Composio key in an MCP header and an env object", () => { + const config = { + mcpServers: { + composio: { + type: "http", + url: "https://connect.composio.dev/mcp", + headers: { "x-consumer-api-key": "ck_live_supersecret" }, + }, + computer: { env: { ELECTRON_RUN_AS_NODE: "1", OGB_BOX_TOKEN: "box_live_zzz" } }, + }, + }; + + const out = flat(redactSecrets(config)); + expect(out).not.toContain("ck_live_supersecret"); + expect(out).not.toContain("box_live_zzz"); + expect(out).toContain("connect.composio.dev"); + expect(out).toContain("ELECTRON_RUN_AS_NODE"); + expect(out).toContain('"1"'); // a non-secret value is untouched + }); + + it("leaves ordinary protocol traffic alone", () => { + const update = { + method: "session/update", + params: { update: { sessionUpdate: "agent_message_chunk", content: { text: "the key to this bug" } } }, + }; + expect(redactSecrets(update)).toEqual(update); + }); + + it("does not mangle words that merely contain 'key'", () => { + const msg = { keyboard: "cmd+k", monkey: "business", keys: "SECRET-LIST", hotkey: "ctrl" }; + const out = redactSecrets(msg) as Record; + expect(out.keyboard).toBe("cmd+k"); + expect(out.monkey).toBe("business"); + expect(out.hotkey).toBe("ctrl"); + // `keys` standing alone IS treated as a credential holder + expect(out.keys).toContain("redacted"); + }); + + it("survives cycles-adjacent depth and non-objects", () => { + expect(redactSecrets("plain")).toBe("plain"); + expect(redactSecrets(null)).toBe(null); + expect(redactSecrets(42)).toBe(42); + let deep: Record = { token: "deep-secret" }; + for (let i = 0; i < 20; i++) deep = { nested: deep }; + expect(() => redactSecrets(deep)).not.toThrow(); + }); +}); diff --git a/server/redact.ts b/server/redact.ts new file mode 100644 index 000000000..286dff85b --- /dev/null +++ b/server/redact.ts @@ -0,0 +1,61 @@ +// Keeping secrets out of the native protocol log. +// +// The native tee writes every provider message verbatim, which is what makes +// protocol drift diagnosable — but the messages that set a session up carry +// the credentials the agent is handed: the box token and the comms token +// travel inside `session/new`'s mcpServers env, and a Composio consumer key +// travels in an MCP header. Those logs sit in ~/.openmausbot/native as +// ordinary files, are read by anyone debugging, and get pasted into issues. +// +// So the log keeps the SHAPE and loses the VALUES: a redacted entry still +// tells you a token was passed, under which name, and how long it was — +// enough to debug "the proxy got no token" without the token being there. + +/** Key names whose value is a credential. Matched case-insensitively as a + * substring, so KEY catches ANTHROPIC_API_KEY and x-consumer-api-key. */ +const SECRET_KEY_PARTS = ["token", "secret", "password", "passwd", "apikey", "api_key", "authorization", "auth_token"]; + +/** `key` alone is too broad — it matches `keyboard`, `keys`, `hotkey`. Only + * treat it as a credential when it stands alone or is a suffix, which is how + * every real one is spelled (API_KEY, consumer-key, xai_key). */ +function isSecretName(name: string): boolean { + const lower = name.toLowerCase(); + if (SECRET_KEY_PARTS.some((part) => lower.includes(part))) return true; + return /(^|[_.-])keys?$/.test(lower); +} + +const mask = (value: string) => `«redacted ${value.length} chars»`; + +/** Deep copy with credential VALUES replaced. Handles the two shapes that + * actually carry them: a plain object of env vars ({KEY: "v"}) and the ACP + * wire shape (env: [{name, value}]). Anything unrecognised is copied as-is. */ +export function redactSecrets(input: unknown, depth = 0): unknown { + if (depth > 12 || input === null || typeof input !== "object") return input; + + if (Array.isArray(input)) { + return input.map((item) => { + // ACP env entries: {name: "OMB_COMMS_TOKEN", value: "…"} + if ( + item !== null && + typeof item === "object" && + !Array.isArray(item) && + typeof (item as { name?: unknown }).name === "string" && + typeof (item as { value?: unknown }).value === "string" + ) { + const entry = item as { name: string; value: string }; + return isSecretName(entry.name) ? { ...entry, value: mask(entry.value) } : entry; + } + return redactSecrets(item, depth + 1); + }); + } + + const out: Record = {}; + for (const [key, value] of Object.entries(input as Record)) { + if (typeof value === "string" && isSecretName(key)) { + out[key] = mask(value); + continue; + } + out[key] = redactSecrets(value, depth + 1); + } + return out; +} diff --git a/server/testing/fake-claude-cli.ts b/server/testing/fake-claude-cli.ts index fa94498cb..5edd5db21 100755 --- a/server/testing/fake-claude-cli.ts +++ b/server/testing/fake-claude-cli.ts @@ -7,13 +7,17 @@ // FAKE_CLAUDE_MODE happy (default) | exit-early | hang | malformed // | stream (partial-message text deltas before the // whole-message frame, plus subagent noise to drop) -// FAKE_CLAUDE_DUMP path to write {argv, env, prompt} as JSON, so the -// test can assert on argv shape and env hygiene +// FAKE_CLAUDE_DUMP path to write {argv, env, prompt, mcpConfig} as JSON, +// so the test can assert on argv shape and env hygiene. +// mcpConfig is read back from the --mcp-config file the +// way the real CLI reads it — the driver writes it to a +// private temp file and deletes it when the turn settles, +// so a test cannot open it after the fact. // FAKE_CLAUDE_AUTH in (default) | out | unsupported | malformed | // inherited-api-key — what `auth status` reports // // Keep this file dependency-free — it runs as a bare `node` subprocess. -import { writeFileSync } from "node:fs"; +import { readFileSync, writeFileSync } from "node:fs"; const mode = process.env.FAKE_CLAUDE_MODE ?? "happy"; @@ -59,7 +63,16 @@ process.stdin.on("end", () => { } if (process.env.FAKE_CLAUDE_DUMP) { - writeFileSync(process.env.FAKE_CLAUDE_DUMP, JSON.stringify({ argv, env: process.env, prompt }, null, 2)); + const configPath = argAfter("--mcp-config"); + let mcpConfig: unknown = null; + if (configPath) { + try { + mcpConfig = JSON.parse(readFileSync(configPath, "utf8")); + } catch { + /* leave null — the test will see it */ + } + } + writeFileSync(process.env.FAKE_CLAUDE_DUMP, JSON.stringify({ argv, env: process.env, prompt, mcpConfig }, null, 2)); } const sessionId = argAfter("--resume") ?? argAfter("--session-id") ?? "fake-session";