From 30360ea60adde6ce059f33f1549e40f509cb7adc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 5 Aug 2026 10:04:34 +0900 Subject: [PATCH 1/2] fix(redact,responses): close the variant gaps left by the first two fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the merged stack found both fixes were correct in shape but too narrow, and named the exact variants that still fail. Redaction: the colon rule tokenized the value on quotes, spaces, and semicolons, so every delimiter-bearing form still leaked — `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 is masked now. `Bearer` stays the readable exception: the scheme word survives and only its token is consumed. Snapshot repair: fail-closed on a contradictory item_id was applied to output_text.done but not to its siblings. A foreign content_part.done was ignored, the item stayed open, and the terminal fabricated the entire closure sequence anyway — the same failure family, one event over. The same identity contract now covers content_part.added/done and output_text.delta. An OMITTED item_id remains legitimate and is still correlated by output_index, which is asserted so the guard cannot regress into failing closed on healthy streams. --- src/lib/redact.ts | 22 +++++++--- src/server/responses-snapshot-repair.ts | 20 +++++++++ tests/redact.test.ts | 24 +++++++++++ tests/responses-snapshot-repair.test.ts | 55 +++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 6 deletions(-) diff --git a/src/lib/redact.ts b/src/lib/redact.ts index 49ad81b4a..54769f6d3 100644 --- a/src/lib/redact.ts +++ b/src/lib/redact.ts @@ -13,12 +13,22 @@ 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: an auth scheme is diagnostically + // useful and the dedicated rule above already masks its token, so the scheme + // word is preserved and only what follows is consumed here. Other schemes + // (Basic, Digest, …) are masked whole, since their payload is the credential. + [/\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*(?:Bearer\s+)?)(?!\s*$)[^\r\n]+/gi, `$1${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" } })); From 753781d25b1ec1fa18d1038e154edfa1338ff5dc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 5 Aug 2026 10:13:02 +0900 Subject: [PATCH 2/2] fix(redact): keep the Bearer scheme readable and stop at end-of-line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up within this PR. Masking the whole colon-labelled value also ate the Bearer form, which broke a Vertex diagnostic: the error text quotes a header in prose (`… Authorization: Bearer at /path/file.json`) and the trailing path marker disappeared with the token. Bearer is now excluded from the colon rule by a lookahead placed BEFORE the whitespace is consumed — a greedy whitespace class inside the capture moved the lookahead's evaluation point past the scheme word, which is why the first two attempts silently kept matching. The dedicated Bearer rule handles that case and now preserves the space it matched, so `Authorization:` keeps its separator. --- src/lib/redact.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/lib/redact.ts b/src/lib/redact.ts index 54769f6d3..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], @@ -24,11 +24,19 @@ const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [ // `;`. A credential header's value IS the rest of the line, so that is what // gets masked. // - // `Bearer` is the one readable exception: an auth scheme is diagnostically - // useful and the dedicated rule above already masks its token, so the scheme - // word is preserved and only what follows is consumed here. Other schemes - // (Basic, Digest, …) are masked whole, since their payload is the credential. - [/\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*(?:Bearer\s+)?)(?!\s*$)[^\r\n]+/gi, `$1${REDACTED_SECRET}`], + // `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`],