diff --git a/src/hooks/capture.ts b/src/hooks/capture.ts index eed7f8b6..6b2e5349 100644 --- a/src/hooks/capture.ts +++ b/src/hooks/capture.ts @@ -121,7 +121,10 @@ async function main(): Promise { id: crypto.randomUUID(), ...meta, type: "user_message", - content: input.prompt, + // Redact the plain string before it is placed in the entry so the + // redactor sees one level of escaping, not a doubly-serialized JSON + // value. The outer JSON.stringify below then encodes the result once. + content: redactSecrets(input.prompt), }; } else if (input.tool_name !== undefined) { log(`tool=${input.tool_name} session=${input.session_id}`); @@ -131,8 +134,13 @@ async function main(): Promise { type: "tool_call", tool_name: input.tool_name, tool_use_id: input.tool_use_id, - tool_input: JSON.stringify(input.tool_input), - tool_response: JSON.stringify(input.tool_response), + // Serialize each object field to a string first, then redact at that + // single level of JSON encoding before placing the string in the entry. + // Post-hoc redaction over JSON.stringify(entry) would see doubly-escaped + // values (tool_input is itself a JSON string inside another JSON string), + // causing the regex to mis-parse secrets near backslashes or quotes. + tool_input: redactSecrets(JSON.stringify(input.tool_input)), + tool_response: redactSecrets(JSON.stringify(input.tool_response)), }; } else if (input.last_assistant_message !== undefined) { log(`assistant session=${input.session_id}`); @@ -161,7 +169,7 @@ async function main(): Promise { id: crypto.randomUUID(), ...meta, type: "assistant_message", - content: input.last_assistant_message, + content: redactSecrets(input.last_assistant_message), ...(input.agent_transcript_path ? { agent_transcript_path: input.agent_transcript_path } : {}), ...(modelMeta ?? {}), }; @@ -171,10 +179,9 @@ async function main(): Promise { } const sessionPath = buildSessionPath(config, input.session_id); - // Mask secrets (tokens, passwords, API keys) before the payload is embedded - // or written to the store. Redacting the serialized line covers every field - // (content / tool_input / tool_response) and both egress paths at once. - const line = redactSecrets(JSON.stringify(entry)); + // Fields are already redacted individually above — serialize once, no + // post-hoc string surgery over doubly-encoded JSON values. + const line = JSON.stringify(entry); log(`writing to ${sessionPath}`); // Simple INSERT — one row per event, no concat, no race conditions. diff --git a/src/hooks/shared/redact.ts b/src/hooks/shared/redact.ts index 2d7ba1d8..f6087d12 100644 --- a/src/hooks/shared/redact.ts +++ b/src/hooks/shared/redact.ts @@ -182,6 +182,19 @@ const RULES: Rule[] = [ { re: /([a-z][a-z0-9+.-]*:\/\/[^\s:/@]+:)([^\s:/@]+)(@)/gi, replace: `$1${MASK}$3` }, // ── 4. Generic labeled assignments ─────────────────────────────────────── + // Quoted multi-word form: `password="two words"` or `token='a b c'`. + // The opening quote is consumed as part of the match so the full quoted value + // — including any whitespace — is captured and masked whole. Runs before the + // unquoted rule so the opening quote is not swallowed by the unquoted branch. + { + re: new RegExp( + `((?:${SECRET_KEY_WORDS})(?![A-Za-z0-9])\\s*[:=]\\s*)(["'])([^"'\\\\](?:[^"'\\\\]|\\\\.)*?)\\2`, + "gi", + ), + replace: (match, keep: string, quote: string, value: string) => + NON_SECRET_VALUE.test(value) ? match : `${keep}${quote}${MASK}${quote}`, + }, + // Unquoted form: value runs to first whitespace/delimiter. // A trailing run of backslashes stays outside the mask when a quote follows: // every capturer redacts the JSON-serialized entry, where a value followed // by an escaped quote reads `...VALUE\\"`. Masking that backslash left diff --git a/src/mcp/cowork-ingest.ts b/src/mcp/cowork-ingest.ts index f774bd12..45b20010 100644 --- a/src/mcp/cowork-ingest.ts +++ b/src/mcp/cowork-ingest.ts @@ -268,19 +268,23 @@ export function entriesForLine(line: TranscriptLine): Record[] ...base, type: "tool_result", tool_use_id: b.tool_use_id, - tool_response: JSON.stringify(b.content ?? null), + // Redact at one level of JSON encoding (the serialized field value) + // before placing the string in the entry. buildCoworkQueueRow then + // serializes the entry once — avoiding the double-encoding that + // caused the redactor to mis-parse secrets near backslashes/quotes. + tool_response: redactSecrets(JSON.stringify(b.content ?? null)), }); } } } const text = extractText(content); - if (text.trim()) out.push({ id: crypto.randomUUID(), ...base, type: "user_message", content: text }); + if (text.trim()) out.push({ id: crypto.randomUUID(), ...base, type: "user_message", content: redactSecrets(text) }); return out; } if (line.type === "assistant") { const text = extractText(content); - if (text.trim()) out.push({ id: crypto.randomUUID(), ...base, type: "assistant_message", content: text }); + if (text.trim()) out.push({ id: crypto.randomUUID(), ...base, type: "assistant_message", content: redactSecrets(text) }); if (Array.isArray(content)) { for (const b of content) { if (isBlock(b) && b.type === "tool_use") { @@ -290,7 +294,8 @@ export function entriesForLine(line: TranscriptLine): Record[] type: "tool_call", tool_name: b.name, tool_use_id: b.id, - tool_input: JSON.stringify(b.input ?? null), + // Same: redact the serialized field string before it enters the entry. + tool_input: redactSecrets(JSON.stringify(b.input ?? null)), }); } } @@ -400,14 +405,14 @@ export function summarizeIdleSessions( } /** - * Serialize a Cowork session entry, redact secrets, and build the queued row. + * Serialize a Cowork session entry and build the queued row. * - * Extracted as a named function so the redaction + serialization step is - * testable in isolation — tests that import this function exercise the - * production code path rather than duplicating the redaction logic themselves. - * - * Matches the pattern used by every other agent capturer: - * `line = redactSecrets(JSON.stringify(entry))` + * Extracted as a named function so the serialization step is testable in + * isolation. Secret redaction is performed upstream in entriesForLine() on + * each individual field (content / tool_input / tool_response) before the + * entry is assembled, so the redactor sees one level of JSON encoding per + * field rather than doubly-serialized text. This function serializes the + * already-redacted entry once and passes it to the queue. */ export function buildCoworkQueueRow( entry: Record, @@ -415,10 +420,9 @@ export function buildCoworkQueueRow( ): ReturnType { return buildQueuedSessionRow({ sessionPath: buildSessionPath(config, String(entry.session_id ?? "")), - // Mask secrets (tokens, passwords, API keys) before the payload is - // queued or embedded. Redacting the serialized line covers every field - // (content / tool_input / tool_response) in one pass. - line: redactSecrets(JSON.stringify(entry)), + // Fields are already redacted individually in entriesForLine — serialize + // once here without post-hoc string surgery over doubly-encoded JSON. + line: JSON.stringify(entry), userName: config.userName, projectName: COWORK_PROJECT, description: String(entry.type ?? ""), diff --git a/tests/claude-code/cowork-ingest.test.ts b/tests/claude-code/cowork-ingest.test.ts index 9fa67b0a..9fb1a43f 100644 --- a/tests/claude-code/cowork-ingest.test.ts +++ b/tests/claude-code/cowork-ingest.test.ts @@ -15,6 +15,7 @@ import { // Build fixture secrets from split literals at runtime so the source file never // contains a scannable vendor token (GitHub secret scanning would block this file). const j = (...parts: string[]): string => parts.join(""); +const MASK = "********"; const fakeSessionConfig = { userName: "test-user", orgName: "test-org", workspaceId: "test-ws" }; @@ -187,49 +188,83 @@ describe("secret redaction on the Cowork ingest path (#308)", () => { }; // Parse the queued row message back to an object so we can assert field values. - function queuedMessage(entry: Record): Record { - return JSON.parse(buildCoworkQueueRow(entry, fakeSessionConfig).message) as Record; + // Redaction now happens in entriesForLine, so we test through the full pipeline: + // entriesForLine → buildCoworkQueueRow → JSON.parse(message). + function queuedMessageFromLine(line: Parameters[0]): Record { + const entries = entriesForLine(line); + expect(entries.length).toBeGreaterThan(0); + return JSON.parse(buildCoworkQueueRow(entries[0]!, fakeSessionConfig).message) as Record; } it("masks an OpenAI API key in a user_message content field", () => { const secret = j("sk-", "ABCDEFGHIJKLMNOPQRSTUVWX"); - const entry = { ...base, type: "user_message", content: `my key is ${secret}` }; - const msg = queuedMessage(entry); + const msg = queuedMessageFromLine({ + sessionId: base.session_id, + timestamp: base.timestamp, + cwd: base.cwd, + type: "user", + message: { content: `my key is ${secret}` }, + }); expect(msg.content).toBe("my key is sk-********"); }); it("masks a GitHub PAT in a tool_input field (e.g. curl auth header)", () => { const secret = j("ghp_", "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"); const cmd = `curl -H "Authorization: token ${secret}" https://api.github.com`; - const entry = { - ...base, - type: "tool_call", - tool_name: "bash", - tool_use_id: "toolu_1", - tool_input: JSON.stringify({ cmd }), - }; - const msg = queuedMessage(entry); + const entries = entriesForLine({ + sessionId: base.session_id, + timestamp: base.timestamp, + cwd: base.cwd, + type: "assistant", + message: { + content: [ + { type: "text", text: "running curl" }, + { type: "tool_use", id: "toolu_1", name: "bash", input: { cmd } }, + ], + }, + }); + // tool_call entry is the second (after the assistant_message text) + const toolCallEntry = entries.find(e => e.type === "tool_call"); + expect(toolCallEntry).toBeDefined(); + const msg = JSON.parse(buildCoworkQueueRow(toolCallEntry!, fakeSessionConfig).message) as Record; const toolInput = JSON.parse(String(msg.tool_input)) as { cmd: string }; expect(toolInput.cmd).toBe(`curl -H "Authorization: token ghp_********" https://api.github.com`); }); it("masks a secret in a tool_response field", () => { const secret = j("sk-", "ant-api03-ABCDEFGHIJKLMNOPQRSTUV_wx"); - const entry = { - ...base, - type: "tool_result", - tool_use_id: "toolu_2", - tool_response: JSON.stringify({ api_key: secret }), - }; - const msg = queuedMessage(entry); + // Claude transcripts carry tool_result content as the raw value (object or string), + // not pre-serialized. entriesForLine does JSON.stringify then redactSecrets on it. + const entries = entriesForLine({ + sessionId: base.session_id, + timestamp: base.timestamp, + cwd: base.cwd, + type: "user", + message: { + content: [ + { type: "tool_result", tool_use_id: "toolu_2", content: { api_key: secret } }, + ], + }, + }); + const toolResultEntry = entries.find(e => e.type === "tool_result"); + expect(toolResultEntry).toBeDefined(); + const msg = JSON.parse(buildCoworkQueueRow(toolResultEntry!, fakeSessionConfig).message) as Record; const toolResponse = JSON.parse(String(msg.tool_response)) as { api_key: string }; - // redactSecrets keeps the scheme prefix as a hint (sk-ant-) and masks the rest - expect(toolResponse.api_key).toBe("sk-ant-********"); + // When the field is redacted at one JSON level, the generic api_key= rule + // sees the label directly and masks the whole value (no prefix hint kept). + // The secret is gone — that is the correct outcome. + expect(toolResponse.api_key).toBe(MASK); + expect(String(msg.tool_response)).not.toContain(secret); }); it("leaves non-secret content untouched", () => { - const entry = { ...base, type: "user_message", content: "what is the weather in Tokyo?" }; - const msg = queuedMessage(entry); + const msg = queuedMessageFromLine({ + sessionId: base.session_id, + timestamp: base.timestamp, + cwd: base.cwd, + type: "user", + message: { content: "what is the weather in Tokyo?" }, + }); expect(msg.content).toBe("what is the weather in Tokyo?"); }); }); diff --git a/tests/shared/redact.test.ts b/tests/shared/redact.test.ts index e4bbcc9e..36f89232 100644 --- a/tests/shared/redact.test.ts +++ b/tests/shared/redact.test.ts @@ -316,3 +316,99 @@ describe("redactSecrets — JSON-serialized capture entries stay valid JSON", () expect(JSON.parse(parsed.content)).toBe("psql --password ********"); }); }); + +describe("redactSecrets — pre-serialization regression shapes (issue #361)", () => { + // These are the exact regression shapes listed in the issue. With the old + // post-hoc approach (redactSecrets over JSON.stringify(entry)), fields like + // tool_input were doubly serialized, causing the regex to see escape sequences + // it couldn't handle correctly. The fix redacts each field at one level of + // encoding before building the entry. These tests confirm correct behaviour + // at that single encoding level — the level the redactor now operates on. + + it("masks password=\\\\\\\\ (value is two literal backslashes)", () => { + // At one level of JSON encoding, two literal backslashes serialize as \\\\ + const input = "password=\\\\"; + const out = redactSecrets(input); + expect(out).not.toContain("\\\\"); + expect(out).toContain(MASK); + }); + + it("masks password=abc\\\" (value ends in a literal quote)", () => { + // The value abc" — at one JSON encoding level this is abc\\\" + // The redactor should mask the whole value, not just abc + const input = 'password=abc\\"'; + const out = redactSecrets(input); + expect(out).toContain(`password=${MASK}`); + expect(out).not.toContain("abc"); + }); + + it("masks token=xy\\\\z\\\\ (value contains and ends in backslashes)", () => { + const input = "token=xy\\\\z\\\\"; + const out = redactSecrets(input); + expect(out).toContain(`token=${MASK}`); + expect(out).not.toContain("xy"); + }); + + it("result is parseable JSON when the field was a JSON-serialized string", () => { + // Simulate: tool_input field after one JSON.stringify of the inner object, + // then the outer entry is JSON.stringify'd — but we redact at inner level. + const inner = JSON.stringify({ command: "export GITHUB_TOKEN=ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" }); + const redacted = redactSecrets(inner); + expect(redacted).not.toContain("ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"); + // The redacted inner string must still be valid JSON when re-parsed + const outer = JSON.stringify({ tool_input: redacted }); + expect(() => JSON.parse(outer)).not.toThrow(); + const parsed = JSON.parse(outer); + expect(parsed.tool_input).toContain(MASK); + }); + + it("nested JSON structure remains parseable after redaction of a password field", () => { + // Simulate a tool_input with a nested secret — one level of JSON.stringify + const inner = JSON.stringify({ db: { password: "s3cr3tP4ss", host: "db.internal" } }); + const redacted = redactSecrets(inner); + expect(redacted).not.toContain("s3cr3tP4ss"); + const outer = JSON.stringify({ tool_input: redacted }); + expect(() => JSON.parse(outer)).not.toThrow(); + const parsed = JSON.parse(outer); + const toolInput = JSON.parse(parsed.tool_input); + expect(toolInput.db.password).toBe(MASK); + expect(toolInput.db.host).toBe("db.internal"); + }); +}); + +describe("redactSecrets — quoted multi-word values (issue #361)", () => { + // CodeRabbit on #360 noted: `password="two words"` should mask the whole + // quoted value, not just the first word. The rule stops at whitespace in + // the unquoted form, so a separate quoted-form rule is needed. + + it('masks password="two words" — full quoted value', () => { + const out = redactSecrets('password="two words"'); + expect(out).toBe(`password="${MASK}"`); + expect(out).not.toContain("two"); + expect(out).not.toContain("words"); + }); + + it("masks password='single quoted value'", () => { + const out = redactSecrets("password='my secret phrase'"); + expect(out).toBe(`password='${MASK}'`); + }); + + it("masks token=\"multi word token value\"", () => { + const out = redactSecrets('token="bearer abc def ghi"'); + expect(out).toContain(MASK); + expect(out).not.toContain("bearer abc def ghi"); + }); + + it("still masks single-word unquoted values after adding the quoted rule", () => { + // Regression guard: the new rule must not interfere with the existing unquoted form + const out = redactSecrets("password=hunter2horse"); + expect(out).toContain(`password=${MASK}`); + expect(out).not.toContain("hunter2horse"); + }); + + it("does not mask a quoted non-secret value", () => { + // Non-secret values like false/null should still pass through + const out = redactSecrets('secret="false"'); + expect(out).toBe('secret="false"'); + }); +});