fix(redact): close the Bearer smuggling hole in the colon rule - #1040
Merged
Conversation
Re-review of #1038. Exempting the bare scheme word from the colon rule meant a credential only had to be prefixed with `Bearer` to pass through untouched, because the dedicated Bearer rule matches a single opaque `[A-Za-z0-9._~+/=-]{8,}` token and nothing else. Anything it could not parse survived: - `x-api-key: Bearer "smuggledcredential123456"` (quoted) - `Authorization: Bearer custom:credential123456` (punctuation) - `x-api-key: Bearer short` (under the length floor) All three were reachable through formatErrorBody on the sidecar bridge. The exemption now matches only the SANITIZED result — `Bearer [REDACTED]` — so a value the Bearer rule could not sanitize is masked whole by the colon rule. The readable case is unchanged: `Authorization: Bearer <token>` still renders as `Bearer [REDACTED]` with trailing diagnostics intact.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesBearer credential redaction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bf8f34093
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `x-api-key: Bearer "quoted…"`, `Authorization: Bearer custom:cred…`, and | ||
| // a short token all slipped through untouched. Anything the Bearer rule | ||
| // could not sanitize is therefore masked whole here. | ||
| [/\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[^\S\r\n]+\[REDACTED\]|\[REDACTED\])(?![^\s.,;)\]]))(?![^\S\r\n]*(?:\r?\n|$))([^\S\r\n]*)[^\r\n]+/gi, `$1$2${REDACTED_SECRET}`], |
There was a problem hiding this comment.
When an upstream error quotes the echoed header, e.g. error: "Authorization: Bearer abcdefgh12345678" at /path/file.json, the Bearer rule first produces Bearer [REDACTED], but this new boundary check does not treat the closing " as a delimiter. The colon rule then redacts from Authorization: through the end of the line, yielding error: "Authorization: [REDACTED] and dropping the closing quote plus the trailing path diagnostic that this exception is intended to preserve; include quote delimiters in the boundary set or add coverage for quoted prose.
Useful? React with 👍 / 👎.
Third re-review round on the same rule. Each previous attempt failed at the same seam — a pattern reasoning about what an earlier pattern had already done: 1. Exempting the bare word `Bearer` let anything the Bearer rule could not parse escape both rules (quoted, punctuation-bearing, or short values). 2. Exempting the sanitized marker `Bearer [REDACTED]` trusted a PUBLIC string that an upstream can emit too, so a suffix appended after it rode along. 3. Splitting into two ordered patterns had the second one eat the first one's output. So the header case is now a single pass with a replacement callback, and the boundary is stated in code rather than assembled from lookaheads: the value after the colon is a credential and is masked whole; `Bearer` keeps its scheme word and exactly one token is consumed, so trailing prose such as `… at /path/file.json` stays readable; nothing in the value grants trust. Also fixes the standalone Bearer rule crossing line boundaries — `\\s+` included newlines, so a header quoted with a trailing break masked the first word of the next line.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/redact.ts`:
- Around line 25-34: The Bearer handling in maskColonLabelledCredential
preserves chained credential text after the token, allowing values such as
x-api-key to leak. Update the matcher or masking flow around
COLON_LABELLED_CREDENTIAL and maskColonLabelledCredential so the Bearer branch
consumes only the scheme and token, then add a regression test covering an
Authorization value followed by another colon-labelled credential.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d8b9a701-3c76-402d-a8e5-09a7558c31b9
📒 Files selected for processing (2)
src/lib/redact.tstests/redact.test.ts
Fourth re-review round. Two more bypasses, both in what the Bearer carve-out chose to keep: - The preserved remainder was assumed to be prose, but the INPUT controls it. `Authorization: Bearer <tok> x-api-key: <tok2>` handed the second credential back untouched, and `Authorization: Bearer Bearer <tok>` masked the literal word and returned the real token. The remainder is now rescanned, and a repeated scheme word consumes the following token instead. - Only ASCII `:` was recognized, so `x-api-key：<secret>` and the small and vertical colon forms were never seen as headers at all. Confusables are normalized before matching. The carve-out is also scoped to `authorization` / `proxy-authorization`, where a scheme is meaningful. On `x-api-key` the word bought nothing and only gave an attacker a way to keep part of the line. The rescan is iterative with a bounded fixpoint, not recursive: a per-match recursion overflowed the stack on a line of 3000 repeated headers, which is now a regression test.
Fifth review round on this rule, and the last one that should be needed — because the thing that kept breaking was the design, not the regex. Every round preserved some readable part of a credential line, and every round the reviewer found a credential hidden inside exactly that part: a quoted value, a value with punctuation, a suffix after the public [REDACTED] marker, a second label after a Bearer token, a repeated scheme word, then a third token two levels deep. Preserving attacker-controlled text next to a credential IS the bug. So the value after a credential label now runs to end-of-line, unconditionally. The only thing kept is the literal word `Bearer` on authorization-style headers — emitted by the code, never copied from the input — so a diagnostic still says which auth scheme failed. The Vertex test that relied on a trailing path marker is updated to reflect that, with a separate case proving path redaction still works where no credential precedes it. Colon confusables and invisible format characters are folded for MATCHING only, with offsets mapped back to the original string. Folding the string itself rewrote innocent text (`ratio∶1` became `ratio:1`). Eight more separator forms are covered, along with zero-width and word-joiner characters placed before the colon.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/redact.ts`:
- Around line 47-50: Update the redaction regexes and line-end handling in the
relevant symbols of redact.ts to recognize U+2028 and U+2029 consistently: use
[^\S\r\n\u2028\u2029] for horizontal whitespace and [\r\n\u2028\u2029] for line
terminators. Add regression cases covering both characters, including
Authorization values, while preserving existing CR/LF behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5711ed02-bbef-48af-890d-5aca87a76d15
📒 Files selected for processing (3)
src/lib/redact.tstests/google-vertex-http.test.tstests/redact.test.ts
Comment on lines
+47
to
+50
| const COLON_LABELLED_CREDENTIAL = new RegExp( | ||
| `\\b(?:${CREDENTIAL_HEADER_LABEL})[^\\S\\r\\n]*:`, | ||
| "gi", | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
python3 - <<'PY'
import re
for char in ("\u2028", "\u2029"):
assert re.fullmatch(r"[^\S\r\n]", char), repr(char)
print("U+2028 and U+2029 match [^\\S\\r\\n].")
PY
rg -n -C 2 '\[\^\\S\\r\\n\]|\[\\r\\n\]' src/lib/redact.tsRepository: lidge-jun/opencodex
Length of output: 1558
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- src/lib/redact.ts ---'
cat -n src/lib/redact.ts | sed -n '1,135p'
printf '%s\n' '--- redact-related files ---'
rg -n -C 3 'redactSecretString|maskCredentialHeaders|U\\+2028|U\\+2029|Authorization: Bearer|REDACTED' . \
-g '!node_modules' -g '!dist' -g '!build' | sed -n '1,260p'
printf '%s\n' '--- ECMAScript behavior probe ---'
node - <<'JS'
const input = "Authorization: Bearer\u2028requestidentifier123456";
const gap = /^[^\S\r\n]*/.exec(" Bearer\u2028requestidentifier123456")?.[0] ?? "";
const bearer = /^[^\S\r\n]*Bearer[^\S\r\n]/i.test(" Bearer\u2028requestidentifier123456");
const lineEnd = input.slice("Authorization:".length).search(/[\r\n]/);
console.log(JSON.stringify({gap, bearer, lineEnd, length: input.length}));
JSRepository: lidge-jun/opencodex
Length of output: 27095
Recognize all ECMAScript line terminators.
In src/lib/redact.ts, lines 48, 89, 92, 94, and 105 use [^\S\r\n]. This class matches U+2028 and U+2029, but line 82 recognizes only CR and LF.
For Authorization: Bear\u2028requestidentifier123456, the matcher treats U+2028 as whitespace, while lineEnd returns the end of the string. The function therefore redacts the following diagnostic text as part of the credential value.
Use [^\S\r\n\u2028\u2029] for horizontal whitespace and [\r\n\u2028\u2029] for line ends. Add regression cases for both characters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/redact.ts` around lines 47 - 50, Update the redaction regexes and
line-end handling in the relevant symbols of redact.ts to recognize U+2028 and
U+2029 consistently: use [^\S\r\n\u2028\u2029] for horizontal whitespace and
[\r\n\u2028\u2029] for line terminators. Add regression cases covering both
characters, including Authorization values, while preserving existing CR/LF
behavior.
Sixth round. Two findings, both about the label rather than the value.
Homoglyph and invisible-character spoofing: `x-api-kеy` with a Cyrillic e,
`x-аpi-key` with a Cyrillic a, and five different default-ignorable code
points inserted mid-word all hid the label from the pattern. Growing the
character list again would just invite a seventh round, so the matching view
now folds systematically: `\p{Default_Ignorable_Code_Point}`, `\p{Cf}`,
and combining marks are dropped, NFKD collapses width and font variants, and a
homoglyph table covers the cross-script look-alikes NFKD deliberately leaves
alone. The offset map stays one-to-one, so output bytes are unchanged.
Over-redaction: `\b` matches after `-` and `_`, so `not-authorization:`
and `internal_token:` were redacted as the credential headers they merely end
with. The left boundary now excludes identifier characters.
Perf on the folded path: 2 MB value 98 ms, 20k repeated headers 28 ms, 5k
random UTF-16 strings with no throw.
Seventh round, two findings, both real. The fold iterated UTF-16 code UNITS, so every supplementary character arrived as two halves and neither half matched a Unicode property or normalized. That is a plain bug: `𝕩-api-key` (mathematical letter, NFKD-normalizes to x) and a U+E0100 variation selector inside a label both walked past the fold. It now iterates code points and keeps the offset map aligned per source code point. The homoglyph table missed Cyrillic ԁ and Greek ε / τ, which NFKD deliberately leaves alone. Extended to cover the credential-label alphabet across Cyrillic, Greek, Latin-extended, and Armenian. Perf after the per-code-point work: 2 MB value 149 ms, 20k repeated headers 42 ms, 5k random UTF-16 strings with no throw. Ordinary supplementary text (emoji) is byte-identical on output.
Structural gap, and the last one of that class: a serialized headers object
puts a closing quote between the field name and the colon, so the label
pattern never matched it, and the pre-existing JSON rules listed only a few
field names without sharing the credential-label grammar. Ordinary JSON
serialization — no homoglyphs, no attacker-chosen alphabet — walked a
credential straight through:
request headers: {"x-api-key":"<secret>"}
headers={"authorization":"Basic <payload>"}
headers={"cookie":"session=<secret>"}
The label now accepts optional surrounding quotes, so both spellings share one
grammar. A quoted value is masked to its CLOSING QUOTE rather than end-of-line:
running to the line end inside an object would swallow the closing brace and
the sibling fields, which are not the credential and which a reader needs.
Escaped quotes inside the value are handled, and multiple objects on one line
each get their own mask.
This predates the branch — it is a gap in the base patterns, not a regression
introduced here.
…mings Two findings from the round-8 review, the first a regression I introduced. REGRESSION: masking a quoted value to its closing quote ended the mask at the first quote regardless of what followed, so `x-api-key: "decoy"<secret>` handed the credential back as a suffix. That is the smuggling shape earlier rounds closed, walking back in through a different door, and it made the branch WEAKER than dev for a case dev already handled. A closing quote now terminates the value only when a structural terminator follows (comma, closing brace or bracket, semicolon, end of line); anything else falls back to masking the whole line. FRAMINGS: an upstream error body is not always a header dump. Form-encoded `authorization=<value>`, an XML element `<x-api-key>…</x-api-key>`, and a multipart part named after a credential all carried the same names past a colon-only matcher. Each now masks with the terminator its own grammar defines, so `&`-separated siblings, the closing tag, and the multipart boundary all survive. The usage-debug expectation is updated rather than worked around: a quoted credential field is now masked as a whole value, scheme word included, because inside a serialized object the scheme is part of what the upstream echoed back. The field name still survives, which is what keeps the sample readable.
…mings Round 9. The regression was mine twice over, and both times for the same reason: I let a property of attacker-controlled TEXT decide when to stop redacting. First 'stop at the closing quote', then 'stop at a closing quote followed by punctuation' — and `x-api-key: "decoy",<secret>` walked through both, masking less than the rule did before quoted-key support existed. Early termination is now decided by the LABEL. A quoted label (`"x-api-key":`) is the input proving it is a serialized field, so its value is one quoted token and the siblings after it are structure worth keeping. An unquoted label is a header line, and there the value is the rest of the line — the baseline behavior, restored unconditionally. The framings are bounded properly too: - XML tag names match exactly, so `<authorizationStatus>` and `<token-count>` keep their values. - A credential can be identified by the tag name, by a `name`/`key`/`id` attribute, or carried in an attribute value; all three are covered, and the attribute rules run first because the element rules consume the opening tag. - Multipart is part-based rather than line-based: the mask runs from the part header through the next boundary, covering a multi-line body, a missing blank line, and an unquoted `name=`. - Quoted form values (`authorization="…"&model=…`) are covered. Monotonicity is now asserted directly: an unquoted header label masks to end of line for every decoy terminator.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/redact.ts`:
- Around line 101-130: Refactor OTHER_FRAMED_CREDENTIALS and maskOtherFramings
to use a uniform captured-head contract: update the query/form pattern to
capture the text through the separator, then make all entries use the same
replacement callback that preserves the captured head and replaces only the
credential value with REDACTED_SECRET. Remove the "="/xml/multipart
discriminator and the match.indexOf("=") slicing logic.
In `@tests/redact.test.ts`:
- Around line 253-256: Add a multipart negative-case assertion alongside the
existing tests in “non-credential fields in those framings are untouched,” using
an ordinary non-credential multipart body and verifying redactSecretString
returns it unchanged. Keep the existing form-encoded and XML assertions intact
so all three redact.ts framing patterns have symmetric coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c1dd54c-af17-4976-a101-fe80e2924fcf
📒 Files selected for processing (3)
src/lib/redact.tstests/redact.test.tstests/usage-debug.test.ts
Comment on lines
+101
to
+130
| const OTHER_FRAMED_CREDENTIALS: Array<[RegExp, string]> = [ | ||
| // URL query / form-encoded: `authorization=<value>` up to `&` or `;`. | ||
| [ | ||
| new RegExp(`(?<![A-Za-z0-9_-])(?:${CREDENTIAL_HEADER_LABEL})=([^&;\\s"']+)`, "gi"), | ||
| "=", | ||
| ], | ||
| // XML/HTML element: `<x-api-key>value</x-api-key>`. | ||
| [ | ||
| new RegExp(`(<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)([^<]+)`, "gi"), | ||
| "xml", | ||
| ], | ||
| // Multipart part: `name="authorization"` followed by the blank line and body. | ||
| [ | ||
| new RegExp( | ||
| `(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)([^\\r\\n]+)`, | ||
| "gi", | ||
| ), | ||
| "multipart", | ||
| ], | ||
| ]; | ||
|
|
||
| function maskOtherFramings(value: string): string { | ||
| let out = value; | ||
| for (const [pattern, kind] of OTHER_FRAMED_CREDENTIALS) { | ||
| out = kind === "=" | ||
| ? out.replace(pattern, match => `${match.slice(0, match.indexOf("=") + 1)}${REDACTED_SECRET}`) | ||
| : out.replace(pattern, (_m, head: string) => `${head}${REDACTED_SECRET}`); | ||
| } | ||
| return out; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the three-way string discriminator with a uniform head capture.
The tuple tag at lines 105, 110, and 118 has three values, but maskOtherFramings only branches on "=" versus everything else. "xml" and "multipart" take the identical code path, so two of the three tags carry no behavior. The tag actually encodes one fact: whether the pattern captures its own head.
Add a head capture group to the query/form pattern. Then all three entries use one replacement callback, and the match.indexOf("=") slice arithmetic at line 126 disappears.
♻️ Proposed refactor to a uniform head-capture contract
-const OTHER_FRAMED_CREDENTIALS: Array<[RegExp, string]> = [
+const OTHER_FRAMED_CREDENTIALS: RegExp[] = [
// URL query / form-encoded: `authorization=<value>` up to `&` or `;`.
- [
- new RegExp(`(?<![A-Za-z0-9_-])(?:${CREDENTIAL_HEADER_LABEL})=([^&;\\s"']+)`, "gi"),
- "=",
- ],
+ new RegExp(`(?<![A-Za-z0-9_-])((?:${CREDENTIAL_HEADER_LABEL})=)[^&;\\s"']+`, "gi"),
// XML/HTML element: `<x-api-key>value</x-api-key>`.
- [
- new RegExp(`(<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)([^<]+)`, "gi"),
- "xml",
- ],
+ new RegExp(`(<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)[^<]+`, "gi"),
// Multipart part: `name="authorization"` followed by the blank line and body.
- [
- new RegExp(
- `(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)([^\\r\\n]+)`,
- "gi",
- ),
- "multipart",
- ],
+ new RegExp(
+ `(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)[^\\r\\n]+`,
+ "gi",
+ ),
];
function maskOtherFramings(value: string): string {
let out = value;
- for (const [pattern, kind] of OTHER_FRAMED_CREDENTIALS) {
- out = kind === "="
- ? out.replace(pattern, match => `${match.slice(0, match.indexOf("=") + 1)}${REDACTED_SECRET}`)
- : out.replace(pattern, (_m, head: string) => `${head}${REDACTED_SECRET}`);
+ for (const pattern of OTHER_FRAMED_CREDENTIALS) {
+ out = out.replace(pattern, (_m, head: string) => `${head}${REDACTED_SECRET}`);
}
return out;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const OTHER_FRAMED_CREDENTIALS: Array<[RegExp, string]> = [ | |
| // URL query / form-encoded: `authorization=<value>` up to `&` or `;`. | |
| [ | |
| new RegExp(`(?<![A-Za-z0-9_-])(?:${CREDENTIAL_HEADER_LABEL})=([^&;\\s"']+)`, "gi"), | |
| "=", | |
| ], | |
| // XML/HTML element: `<x-api-key>value</x-api-key>`. | |
| [ | |
| new RegExp(`(<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)([^<]+)`, "gi"), | |
| "xml", | |
| ], | |
| // Multipart part: `name="authorization"` followed by the blank line and body. | |
| [ | |
| new RegExp( | |
| `(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)([^\\r\\n]+)`, | |
| "gi", | |
| ), | |
| "multipart", | |
| ], | |
| ]; | |
| function maskOtherFramings(value: string): string { | |
| let out = value; | |
| for (const [pattern, kind] of OTHER_FRAMED_CREDENTIALS) { | |
| out = kind === "=" | |
| ? out.replace(pattern, match => `${match.slice(0, match.indexOf("=") + 1)}${REDACTED_SECRET}`) | |
| : out.replace(pattern, (_m, head: string) => `${head}${REDACTED_SECRET}`); | |
| } | |
| return out; | |
| } | |
| const OTHER_FRAMED_CREDENTIALS: RegExp[] = [ | |
| // URL query / form-encoded: `authorization=<value>` up to `&` or `;`. | |
| new RegExp(`(?<![A-Za-z0-9_-])((?:${CREDENTIAL_HEADER_LABEL})=)[^&;\\s"']+`, "gi"), | |
| // XML/HTML element: `<x-api-key>value</x-api-key>`. | |
| new RegExp(`(<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)[^<]+`, "gi"), | |
| // Multipart part: `name="authorization"` followed by the blank line and body. | |
| new RegExp( | |
| `(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)[^\\r\\n]+`, | |
| "gi", | |
| ), | |
| ]; | |
| function maskOtherFramings(value: string): string { | |
| let out = value; | |
| for (const pattern of OTHER_FRAMED_CREDENTIALS) { | |
| out = out.replace(pattern, (_m, head: string) => `${head}${REDACTED_SECRET}`); | |
| } | |
| return out; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 103-103: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((?<![A-Za-z0-9_-])(?:${CREDENTIAL_HEADER_LABEL})=([^&;\\s"']+), "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 108-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<[^\\S\\r\\n]*(?:${CREDENTIAL_HEADER_LABEL})[^>]*>)([^<]+), "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 113-116: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(name=["'](?:${CREDENTIAL_HEADER_LABEL})["'][^\\r\\n]*\\r?\\n\\r?\\n)([^\\r\\n]+),
"gi",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/redact.ts` around lines 101 - 130, Refactor OTHER_FRAMED_CREDENTIALS
and maskOtherFramings to use a uniform captured-head contract: update the
query/form pattern to capture the text through the separator, then make all
entries use the same replacement callback that preserves the captured head and
replaces only the credential value with REDACTED_SECRET. Remove the
"="/xml/multipart discriminator and the match.indexOf("=") slicing logic.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/redact.test.ts (1)
302-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe multipart framing still has no negative case.
Lines 303 and 304 guard the form-encoded and XML framings against false positives. The multipart pattern at line 147 of
src/lib/redact.tshas no such guard, and it is the widest of the framings: its body group((?:(?!--)[^\r\n]*\r?\n?)+)consumes every following line until a line starts with--.A regression there would silently redact ordinary multipart bodies, and nothing in this suite would fail.
💚 Proposed additional assertion
test("non-credential fields in those framings are untouched", () => { expect(redactSecretString("model=gpt-5.5&status=429")).toBe("model=gpt-5.5&status=429"); expect(redactSecretString("<model>gpt-5.5</model>")).toBe("<model>gpt-5.5</model>"); + // The multipart pattern consumes every line up to the boundary, so it needs + // the same negative guard as the other two framings. + const part = 'Content-Disposition: form-data; name="model"\r\n\r\ngpt-5.5\r\n--boundary'; + expect(redactSecretString(part)).toBe(part); });Based on the path instruction that a behavior change in
src/should come with a focused regression test near the existing tests for that subsystem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/redact.test.ts` around lines 302 - 305, Add a focused negative assertion in the multipart-related tests for redactSecretString, using a non-credential multipart body that must remain unchanged. Place it alongside the existing form-encoded and XML false-positive cases so regressions in the multipart pattern’s body matching are detected.Source: Path instructions
src/lib/redact.ts (1)
221-224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe U+2028/U+2029 asymmetry is still present.
Line 222 recognizes only
\rand\nas line ends. Lines 255 and 261 use[^\S\r\n], which matches U+2028 and U+2029 as horizontal whitespace. Line 147 has the same split in[^\r\n].For
Authorization: Bearer\u2028requestidentifier123456 diagnostic, the separator probes treat U+2028 as a space, while line 222 finds no line end and returnsvalue.length. The mask then runs to the end of the string and consumes the following diagnostic text.Use
[^\S\r\n\u2028\u2029]for horizontal whitespace and[\r\n\u2028\u2029]for line ends. The test at lines 322-327 oftests/redact.test.tscovers\nonly; add the two paragraph/line separators next to it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/redact.ts` around lines 221 - 224, Update the line-boundary and whitespace handling in the redaction logic: change the line-end checks in the visible `lineEnd` calculation and the related `[^\r\n]` expression to recognize U+2028/U+2029, and exclude those separators from horizontal whitespace alongside `\r`/`\n`. Extend the existing redaction test covering newline separators with cases for both U+2028 and U+2029.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/redact.ts`:
- Around line 221-224: Update the line-boundary and whitespace handling in the
redaction logic: change the line-end checks in the visible `lineEnd` calculation
and the related `[^\r\n]` expression to recognize U+2028/U+2029, and exclude
those separators from horizontal whitespace alongside `\r`/`\n`. Extend the
existing redaction test covering newline separators with cases for both U+2028
and U+2029.
In `@tests/redact.test.ts`:
- Around line 302-305: Add a focused negative assertion in the multipart-related
tests for redactSecretString, using a non-credential multipart body that must
remain unchanged. Place it alongside the existing form-encoded and XML
false-positive cases so regressions in the multipart pattern’s body matching are
detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 739f56e5-1006-485f-92e8-50b360b98181
📒 Files selected for processing (2)
src/lib/redact.tstests/redact.test.ts
Round 10, and the fourth time the same mistake produced a leak — so this
removes the idea rather than the instance.
Every early-termination rule was a way to read attacker-controlled text and let
it decide where a secret ends: stop at the first closing quote; stop at a quote
followed by punctuation; stop only when the LABEL was quoted. The third still
leaked on an unmatched opening quote (`"x-api-key: "decoy",<secret>`) and
on a correctly quoted key whose value quote was a decoy
(`{"x-api-key":"decoy"<secret>}`).
A credential value now runs to end-of-line, unconditionally, in every framing:
- Form values run to `&` or `;`, with no quoted-value shortcut.
- Multipart consumes the remainder of the body rather than stopping at the
first `--`, because the boundary token is attacker-controlled too — a body
line reading `--not-the-boundary` used to end the mask.
- A qualifying XML tag masks EVERY quoted attribute and its entire element
content: masking one attribute left `type="Basic" value="<secret>"`
leaking, and masking direct text only left a nested `<value>` untouched.
`name`/`key`/`id` must be the whole attribute name, so
`data-name="authorization"` no longer eats an innocent status.
The cost is real and accepted: serialized siblings after a credential are lost,
so a debug body sample keeps its first field name and little else. That is the
right trade — an uglier diagnostic against a redactor that cannot be talked out
of redacting.
XML was the last framing still using a stopping point, and it failed the same way everything else did. A closing tag is attacker-controlled text: same-name nesting ended the mask at the INNER `</authorization>` and exposed the outer element's remaining content, and a self-closing tag had no closing tag to find at all. Namespace-qualified names were not recognized either. A qualifying tag — credential name, optionally namespace-qualified, or a whole `name`/`key`/`id` attribute naming one — now keeps only its tag name and masks to end of line, like every other framing. `data-name` still does not qualify, and `<authorizationStatus>` / `<token-count>` still keep their values. The differential matrix the reviewer ran over 972 cases reports zero regression versus both baselines with early termination gone; this closes the last structural leak it found.
Second stopping point removed from the XML rule. After the closing tag came end-of-line, and an opening tag may legally span lines, so `<authorization\n value="…">` left the credential sitting on the next line. A qualifying tag now masks through end of input, like the multipart rule. Also allows whitespace around an attribute `=` (`name = "authorization"`, `key\t=\t"x-api-key"`), which XML permits and an upstream echo may well reproduce.
A JSON `\u0069`, a percent-encoded `%69`, and an XML `i` all spell the credential field name to whatever parses the body, and spell something else to a literal matcher. `{"author\u0069zation":"<secret>"}`, `author%69zation=<secret>`, and `<header name="authorization">` were therefore invisible — structural aliases, not confusable-table coverage. The fold now decodes all three, one folded character per escape with the whole escape mapped back to its start, so the offset map still writes the mask at the right place in the original bytes. The form and XML rules run over that folded view too, rather than the raw text, which is what let the percent and character reference forms through. Cost: the fold runs once per framing pass, so a 2 MB value goes from ~150 ms to ~340 ms. Still linear, still stack-safe, and this path only runs on error bodies.
Decoding introduced a regression, which is exactly the failure mode this rule keeps hitting: a change that masks MORE in one shape and LESS in another. `𝕩x-api-key: <secret>` decoded to a mathematical letter that folds to `x`, which moved the following label's left boundary and suppressed a match both baselines made. So decoding is now one-way by construction: the header and framing passes run over BOTH matching views — decoded and plain — and mask whatever either finds. Decoding can add coverage; it cannot take any away. The offset map also allocated one entry per source code point rather than per EMITTED UTF-16 unit, so an escaped supplementary character desynchronized every later offset and the mask landed mid-token (`😀authorization=o[REDACTED]model=…`). Entries are per emitted unit now. HTML named entities are decoded as well: `:` is the separator itself, and the Greek names decode to characters the homoglyph fold already handles, so `authorιzation` resolves to the label. Perf on this path is fine — the reviewer confirmed error bodies are capped at 64 KiB, where a full pass is ~35-39 ms.
…table Two more alias classes, both structural. Multi-unit escapes were decoded a unit at a time. A JSON surrogate PAIR is one code point, so decoding the halves separately left two lone surrogates that normalize to nothing; percent encoding is UTF-8, so `%D0%B5` is one Cyrillic character, not two Latin-1 ones. Both now decode as single characters. HTML named references are handled by giving up on naming them. A hand-picked list is a coverage promise nobody can keep — review found `ⅈ`, `ⅇ`, and `ⅆ` decoding to compatibility letters NFKD already maps, and the WHATWG table holds ~2200 entries that neither Bun nor Node exposes. An unresolved name now folds to a placeholder that the label grammar accepts wherever a letter may appear, so every named entity is covered without pretending to know what any of them mean. Only the separator names (`:` and friends) resolve exactly, since a separator is structure rather than part of the name.
lidge-jun
added a commit
that referenced
this pull request
Aug 5, 2026
…isory All thirteen campaign PRs are merged and every merge commit is an ancestor of dev. The two review follow-ups (#1038, #1040) are recorded with the reason they exist, and the one thing deliberately left unfinished — UTS #39 confusable coverage — points at the draft security advisory rather than a public issue, since it describes a redaction weakness and not a shipped fix.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Hardens
redactSecretString's credential-header rule after an adversarial review found that an upstream error body could carry a credential back to a client throughformatErrorBodyon the sidecar bridges.The review ran 15 rounds. Each round found a way to hide a credential inside whatever the previous round had chosen to preserve, and the fix that finally held was to stop preserving anything.
The pattern behind every finding
A credential value used to end where the text said it ended: at a closing quote, at a delimiter, at a closing tag, at a multipart boundary. All of those are attacker-controlled, and the attacker writes the text. Concretely, these all leaked at some point:
x-api-key: "quoted…"Authorization: Basic <payload>Cookie: a=1; b=2;x-api-key: Bearer <anything unparseable>Bearercarve-outx-api-key: Bearer [REDACTED].<secret>Authorization: Bearer <tok> x-api-key: <tok2>x-api-key: "decoy",<secret>{"x-api-key":"<secret>"}<authorization>…,authorization=…, multipart partsx-api-kеy:(Cyrillic е),𝕩-api-key,x-api-ke<U+E0100>yauthor\u0069zation,author%69zation,authorization,ⅈWhat it does now
Nothing stops the mask except a boundary the input cannot move. Colon-labelled values run to end of line; form values to
&/;; multipart and XML to end of input. The only text preserved is the literal wordBeareron authorization-style headers, emitted from a code literal and never copied from input.The label is canonicalized before matching, over a folded view with an offset map back to the original string so untouched bytes stay byte-identical:
\uXXXXincluding surrogate pairs, percent encoding as UTF-8, XML character references, and HTML named references (an unresolved name folds to a placeholder the label accepts wherever a letter may appear, since the WHATWG table has ~2200 entries and no runtime exposes it).Decoding is one-way by construction. The passes run over both the decoded and the plain view and mask whatever either finds — an earlier decode-only version masked less in some shapes, which is the one thing this change must never do.
Accepted costs
{"x-api-key":"s","model":"gpt-5.5"}masks from the value onward.tests/usage-debug.test.tsasserts the first field name survives, which is what makes a debug line readable.Each was a deliberate trade against a stopping point that turned out to be a bypass.
Verification
dev.model: gpt-5.5,ratio∶1,not-authorization:,internal_token:,<authorizationStatus>,<token-count>,<field data-name="authorization">,model=gpt-5.5&status=429.Known residual
Confusable coverage is a hand-maintained table rather than a UTS #39 skeleton. Tracked as a draft security advisory rather than a public issue, with reachability analysis: it needs the upstream to echo a header name the operator's own request spelled with an out-of-table homoglyph, and it cannot expose a credential the operator did not supply.