Keep provider credentials out of the native log and out of argv #130
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown> = {}; | ||
| cyclic.self = cyclic; | ||
| expect(() => appendNative("t-cyclic", { dir: "in", source: "acp", msg: cyclic })).not.toThrow(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, string>; | ||
| 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<string, unknown> = { token: "deep-secret" }; | ||
| for (let i = 0; i < 20; i++) deep = { nested: deep }; | ||
| expect(() => redactSecrets(deep)).not.toThrow(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
|
Comment on lines
+32
to
+33
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Remove the depth-based redaction bypass. At Line 33, the function returns the original object after depth 12. A credential at a deeper acyclic path remains unmasked and Track active object references to terminate cycles instead of returning unprocessed nested data. Update the deep-nesting test to assert that 🤖 Prompt for AI Agents |
||
|
|
||
| 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<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(input as Record<string, unknown>)) { | ||
| if (typeof value === "string" && isSecretName(key)) { | ||
| out[key] = mask(value); | ||
| continue; | ||
| } | ||
| out[key] = redactSecrets(value, depth + 1); | ||
| } | ||
| return out; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: milind-soni/OpenMausBot
Length of output: 1628
🏁 Script executed:
Repository: milind-soni/OpenMausBot
Length of output: 3666
Enforce
0600on existing native log files.appendFileSyncdoes not change the mode of an existing file. ApplychmodSync(path, 0o600)before appending, and test a pre-existing0644log.🤖 Prompt for AI Agents