-
Notifications
You must be signed in to change notification settings - Fork 650
fix(redact,responses): close the credential and lifecycle-identity variant gaps #1038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 <token>` 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 <tok> 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 <tok>` 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}`], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/lib/redact.ts ---'
sed -n '1,180p' src/lib/redact.ts
printf '%s\n' '--- relevant redact tests ---'
sed -n '1,155p' tests/redact.test.ts
printf '%s\n' '--- focused behavioral probe of the redaction rules ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("src/lib/redact.ts").read_text()
line = next(
line.strip()
for line in text.splitlines()
if "x-api-key|x-goog-api-key" in line and "set-cookie" in line
)
pattern_text = line.split("[/", 1)[1].split("/gi", 1)[0]
pattern = re.compile(pattern_text, re.IGNORECASE)
replacement = r"\1\2[REDACTED]"
for value in [
"Set-Cookie: session=secret",
"Set-Cookie2: session=secret",
"set-cookie2: session=secret\nstatus: 429",
]:
print(repr(value), "=>", repr(pattern.sub(replacement, value)))
PYRepository: lidge-jun/opencodex Length of output: 13590 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/lib/redact.ts ---'
sed -n '1,180p' src/lib/redact.ts
printf '%s\n' '--- relevant redact tests ---'
sed -n '1,155p' tests/redact.test.ts
printf '%s\n' '--- focused behavioral probe ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("src/lib/redact.ts").read_text()
line = next(line.strip() for line in text.splitlines()
if "x-api-key|x-goog-api-key" in line and "set-cookie" in line)
pattern_text = line.split("[/", 1)[1].split("/gi", 1)[0]
pattern = re.compile(pattern_text, re.IGNORECASE)
for value in [
"Set-Cookie: session=secret",
"Set-Cookie2: session=secret",
]:
print(repr(value), "=>", repr(pattern.sub(r"\1\2[REDACTED]", value)))
PYRepository: lidge-jun/opencodex Length of output: 13566 🏁 Script executed: #!/bin/bash
set -eu
node - <<'JS'
const REDACTED_SECRET = "[REDACTED]";
const patterns = [
[/\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],
[/\b(gh[pousr]_[A-Za-z0-9_]{8,}|github_pat_[A-Za-z0-9_]{20,})\b/g, REDACTED_SECRET],
[/\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}`],
[/\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}`],
[/(("token"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$4`],
];
function redact(value) {
return patterns.reduce((result, [pattern, replacement]) =>
result.replace(pattern, replacement), value);
}
for (const input of [
"Set-Cookie: session=secret",
"Set-Cookie2: session=secret",
"set-cookie2: session=secret\nstatus: 429",
]) {
const output = redact(input);
console.log(JSON.stringify({ input, output, leaked: output.includes("session=secret") }));
}
JSRepository: lidge-jun/opencodex Length of output: 457 🏁 Script executed: #!/bin/bash
set -eu
node - <<'JS'
const REDACTED_SECRET = "[REDACTED]";
const pattern = /\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;
for (const input of [
"Set-Cookie: session=secret",
"Set-Cookie2: session=secret",
"set-cookie2: session=secret\nstatus: 429",
]) {
const output = input.replace(pattern, `$1$2${REDACTED_SECRET}`);
console.log(JSON.stringify({ input, output, leaked: output.includes("session=secret") }));
}
JSRepository: lidge-jun/opencodex Length of output: 457 Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor Reachability: External Reachability pathAdd
🤖 Prompt for AI AgentsSource: Path instructions |
||
| [/((?:"(?: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`], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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]; | ||
|
Comment on lines
+379
to
+381
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a sparse/malformed provider sends Useful? React with 👍 / 👎. |
||
| } | ||
| // 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]; | ||
|
Comment on lines
+417
to
+419
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This foreign- Useful? React with 👍 / 👎. |
||
| } | ||
| if (open && (itemId === undefined || itemId === open.itemId)) { | ||
| const deltaBytes = Buffer.byteLength(event.delta, "utf8"); | ||
| open.text += event.delta; | ||
|
|
||
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: lidge-jun/opencodex
Length of output: 15775
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 15623
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 948
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 692
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Reachability path
Complete Bearer and
Set-Cookie2redaction.src/lib/redact.ts:6, remove{8,}and the trailing\b. The current rule leaks short tokens, quoted values, and padding such as=.src/lib/redact.ts:39, skip onlyBearer [REDACTED], and addset-cookie2to the key alternatives. Add regression tests for short, quoted, padded Bearer values andSet-Cookie2lines.🤖 Prompt for AI Agents
Source: Path instructions