diff --git a/src/lib/redact.ts b/src/lib/redact.ts index 49ad81b4a..185acc3f8 100644 --- a/src/lib/redact.ts +++ b/src/lib/redact.ts @@ -3,7 +3,7 @@ export const REDACTED_SECRET = "[REDACTED]"; const SENSITIVE_KEY_PATTERN = /^(?:authorization|proxy-authorization|cookie|set-cookie|set-cookie2|api[-_]?key|x-api-key|x-goog-api-key|x-amz-security-token|access[-_]?token|refresh[-_]?token|id[-_]?token|token|secret|client[-_]?secret|password|profile[-_]?arn)$/i; const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [ - [/\bBearer\s+[A-Za-z0-9._~+/=-]{8,}\b/gi, `Bearer ${REDACTED_SECRET}`], + [/\b(Bearer)(\s+)[A-Za-z0-9._~+/=-]{8,}\b/gi, `$1$2${REDACTED_SECRET}`], [/\b(sk-[A-Za-z0-9][A-Za-z0-9._-]{6,})\b/g, REDACTED_SECRET], // GitHub tokens (classic + fine-grained + OAuth/refresh): ghp_/gho_/ghu_/ghs_/ghr_/github_pat_. [/\b(gh[pousr]_[A-Za-z0-9_]{8,}|github_pat_[A-Za-z0-9_]{20,})\b/g, REDACTED_SECRET], @@ -13,12 +13,30 @@ const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [ [/\btid=[A-Za-z0-9-]+(?:;[A-Za-z0-9_.-]+=[^;\s"']*)+(?::[A-Za-z0-9+/=_-]+)?/g, REDACTED_SECRET], [/\b((?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)=)([^&\s"',;]+)/gi, `$1${REDACTED_SECRET}`], // Colon-labelled credentials. Upstream error bodies quote the offending header - // or field back at us ("x-api-key: abc…"), and the `=` rule above never fires - // for that shape, so the credential survived into client-visible error text. - // Header-style names are included because that is exactly what a provider - // echoes when it rejects a request. A `Bearer ` value is left to the - // dedicated rule above so its scheme prefix stays readable in diagnostics. - [/\b((?:x-api-key|x-goog-api-key|x-amz-security-token|api[_-]?key|apiKey|access[_-]?token|accessToken|refresh[_-]?token|refreshToken|id[_-]?token|client[_-]?secret|clientSecret|authorization|proxy-authorization|cookie|password|secret|token)\s*:\s*)(?!\s)(?!Bearer\b)([^\s"',;]+)/gi, `$1${REDACTED_SECRET}`], + // or field back at us ("x-api-key: abc…"), and the `=` rules never fire for + // that shape, so the credential survived into client-visible error text. + // + // The value class deliberately runs to end-of-line rather than stopping at a + // quote, space, or semicolon. A first attempt tokenized on those characters + // and leaked every delimiter-bearing variant: `x-api-key: "quoted…"` kept the + // whole quoted secret, `Authorization: Basic dXNlcjpwYXNz` kept the payload + // after the scheme, and `Cookie: a=1; b=2` kept everything after the first + // `;`. A credential header's value IS the rest of the line, so that is what + // gets masked. + // + // `Bearer` is the one readable exception, and it is handled by the dedicated + // Bearer rule ABOVE rather than here: an auth scheme is diagnostically useful, + // and its token is a single opaque word, so consuming the rest of the line + // there would swallow trailing diagnostics that follow a quoted header in + // prose (`… Authorization: Bearer at /path/file.json`). Every other + // scheme (Basic, Digest, …) carries its credential as the payload, so those + // are masked whole by this rule. + // + // The rules run in order, so by the time this one fires the Bearer rule has + // already replaced `Bearer ` with `Bearer [REDACTED]`. Skipping a value + // that is already redacted keeps this rule from eating that result — and from + // eating the trailing diagnostics after it. + [/\b((?:x-api-key|x-goog-api-key|x-amz-security-token|api[_-]?key|apiKey|access[_-]?token|accessToken|refresh[_-]?token|refreshToken|id[_-]?token|client[_-]?secret|clientSecret|authorization|proxy-authorization|cookie|set-cookie|password|secret|token)\s*:)(?![^\S\r\n]*(?:Bearer\b|\[REDACTED\]|\r?\n|$))([^\S\r\n]*)[^\r\n]+/gi, `$1$2${REDACTED_SECRET}`], [/((?:"(?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$3`], // Raw JSON "token" field values (Copilot token exchange bodies echo the credential here). [/(("token"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$4`], diff --git a/src/server/responses-snapshot-repair.ts b/src/server/responses-snapshot-repair.ts index 6818e6492..9ae137888 100644 --- a/src/server/responses-snapshot-repair.ts +++ b/src/server/responses-snapshot-repair.ts @@ -368,6 +368,18 @@ export function createResponsesSnapshotBlockRewrite( && isPlainObject(event.part)) { if (outputIndex !== undefined) { const open = openItems.get(outputIndex); + // A PRESENT-but-mismatched item_id is contradictory lifecycle evidence, + // exactly like output_item.done and output_text.done: the stream is + // telling us our identity model for this index is wrong. Merely + // ignoring it left the item open and let the terminal fabricate a full + // closure sequence (content_part.added → output_text.done → + // content_part.done → output_item.done) on top of a stream we do not + // understand. Go fail-closed instead. An OMITTED item_id stays + // legitimate and is still correlated by output_index. + if (open && itemId !== undefined && itemId !== open.itemId) { + taintAndRelease(); + return [changed ? jsonBlock(nextEvent) : block]; + } // Correlate by item_id when present: a mismatched event must not // mutate (or suppress injections for) the tracked item (#893 review). if (open && (itemId === undefined || itemId === open.itemId)) { @@ -398,6 +410,14 @@ export function createResponsesSnapshotBlockRewrite( } if (type === "response.output_text.delta" && typeof event.delta === "string" && outputIndex !== undefined) { const open = openItems.get(outputIndex); + // Same identity contract as the *.done terminals: a present-but-foreign + // item_id on a tracked index means our model of this index is wrong, and + // reconstructing from the text we DID accept would ship a message the + // upstream never assembled that way. + if (open && itemId !== undefined && itemId !== open.itemId) { + taintAndRelease(); + return [changed ? jsonBlock(nextEvent) : block]; + } if (open && (itemId === undefined || itemId === open.itemId)) { const deltaBytes = Buffer.byteLength(event.delta, "utf8"); open.text += event.delta; diff --git a/tests/redact.test.ts b/tests/redact.test.ts index a7556cc0b..1a299be8c 100644 --- a/tests/redact.test.ts +++ b/tests/redact.test.ts @@ -60,6 +60,30 @@ describe("redactSecretString", () => { expect(redactSecretString("model: gpt-5.5\nstatus: 429\nrequest: ocx-abc123")) .toBe("model: gpt-5.5\nstatus: 429\nrequest: ocx-abc123"); }); + + test("masks the WHOLE colon-labelled value, including delimiter-bearing forms", () => { + // Re-review of the first fix: tokenizing the value on quotes, spaces, and + // semicolons leaked every variant that contains one. A credential header's + // value is the rest of the line, so that is what must be masked. + expect(redactSecretString('x-api-key: "quotedcredential123456"')) + .toBe(`x-api-key: ${REDACTED_SECRET}`); + expect(redactSecretString("Authorization: Basic dXNlcjpwYXNz")) + .toBe(`Authorization: ${REDACTED_SECRET}`); + expect(redactSecretString("Cookie: session=secret-one; csrf=secret-two")) + .toBe(`Cookie: ${REDACTED_SECRET}`); + }); + + test("keeps the Bearer scheme readable while masking its token", () => { + // An auth scheme is diagnostically useful; the credential after it is not. + expect(redactSecretString("Authorization: Bearer abcdefgh12345678")) + .toBe(`Authorization: Bearer ${REDACTED_SECRET}`); + }); + + test("masks each credential line independently without eating the next", () => { + // End-of-line, not end-of-string: a multi-line error body must not collapse. + expect(redactSecretString("x-api-key: one-secret\nmodel: gpt-5.5\ncookie: two=secret")) + .toBe(`x-api-key: ${REDACTED_SECRET}\nmodel: gpt-5.5\ncookie: ${REDACTED_SECRET}`); + }); }); describe("redactSecrets", () => { diff --git a/tests/responses-snapshot-repair.test.ts b/tests/responses-snapshot-repair.test.ts index 08093c4b6..8bc019ac7 100644 --- a/tests/responses-snapshot-repair.test.ts +++ b/tests/responses-snapshot-repair.test.ts @@ -227,6 +227,61 @@ describe("createResponsesSnapshotBlockRewrite", () => { expect(Object.hasOwn(terminal.response as Record, "output")).toBe(false); }); + test("a mismatched item_id on content_part.done also goes fail-closed", () => { + // Re-review: fixing output_text.done alone left the sibling terminal open. + // A foreign content_part.done was ignored, the item stayed open, and the + // completed terminal fabricated the whole closure sequence + // (content_part.added → output_text.done → content_part.done → + // output_item.done) on a stream whose identity model was already wrong. + const rewrite = createResponsesSnapshotBlockRewrite(); + rewrite(dataBlock(ISSUE_FIXTURE.itemAdded)); + rewrite(dataBlock({ + type: "response.content_part.done", + item_id: "msg_OTHER", + output_index: 0, + part: { type: "output_text", text: "foreign" }, + })); + rewrite(dataBlock(ISSUE_FIXTURE.delta)); + const out = rewrite(dataBlock(ISSUE_FIXTURE.completed)); + expect(typesOf(out)).not.toContain("response.content_part.added"); + expect(typesOf(out)).not.toContain("response.content_part.done"); + expect(typesOf(out)).not.toContain("response.output_text.done"); + expect(typesOf(out)).not.toContain("response.output_item.done"); + const terminal = eventsOf(out).find(event => event.type === "response.completed")!; + expect(Object.hasOwn(terminal.response as Record, "output")).toBe(false); + }); + + test("a mismatched item_id on a text delta goes fail-closed instead of being dropped", () => { + // Reconstructing from only the deltas we accepted would ship a message the + // upstream never assembled that way. + const rewrite = createResponsesSnapshotBlockRewrite(); + rewrite(dataBlock(ISSUE_FIXTURE.itemAdded)); + rewrite(dataBlock({ + type: "response.output_text.delta", + item_id: "msg_OTHER", + output_index: 0, + delta: "foreign", + })); + const out = rewrite(dataBlock(ISSUE_FIXTURE.completed)); + expect(typesOf(out)).not.toContain("response.output_item.done"); + const terminal = eventsOf(out).find(event => event.type === "response.completed")!; + expect(Object.hasOwn(terminal.response as Record, "output")).toBe(false); + }); + + test("an omitted item_id stays legitimate on every correlated event", () => { + // The taint must fire on a PRESENT-but-wrong id only. A gateway that omits + // item_id is common and correlates by output_index alone; breaking that + // would fail closed on healthy streams. + const rewrite = createResponsesSnapshotBlockRewrite(); + rewrite(dataBlock(ISSUE_FIXTURE.itemAdded)); + rewrite(dataBlock({ type: "response.content_part.added", output_index: 0, part: { type: "output_text", text: "" } })); + rewrite(dataBlock({ type: "response.output_text.delta", output_index: 0, delta: "hello" })); + const out = rewrite(dataBlock(ISSUE_FIXTURE.completed)); + expect(typesOf(out)).toContain("response.output_item.done"); + const injected = eventsOf(out).filter(event => event.type === "response.output_text.done"); + expect(injected.some(event => event.text === "hello")).toBe(true); + }); + test("a completed terminal with absent output and zero items gets the canonical empty list", () => { const rewrite = createResponsesSnapshotBlockRewrite(); const out = rewrite(dataBlock({ type: "response.completed", response: { id: "r" } }));