From ebfa05d2c4f8dcf2bcec82f882896e9d2207b64e Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sat, 1 Aug 2026 07:37:03 +0300 Subject: [PATCH 1/2] fix(redaction): a session cookie survives redactSensitiveText `Cookie: session=` came out of redactSensitiveText byte-identical, on a PUBLIC repository whose SECURITY.md routes readers to docs/redaction.md. A session cookie IS bearer authentication under a different header name, so anything logging an HTTP request through this function emitted live sessions. Measured against main@b75e651 by execution, one synthetic value, three discriminating controls in the same run: 9 of 10 cookie shapes leaked; `Authorization: Bearer ` was redacted (the probe can fire); the same literal in free prose was left alone (the probe is not over-masking). WHY THE FAMILY READ AS HANDLED, which is the finding worth more than the fix. `Cookie: __Secure-next-auth.session-token=` was ALREADY redacted before this change -- not by any cookie handling, of which there was none, but because the generic *TOKEN* key rule matched the substring `token` inside the cookie NAME. This is not the usual hazard of a check that cannot fail; a positive control does not catch it, because the instrument really does fire. It is a check that passes for a reason unrelated to the capability under test, and the cookie name a reviewer reaches for first is exactly the one that accidentally works. Recorded as a named section in docs/redaction.md. KEYED ON THE DELIMITER'S ROLE, NOT ON NAMES. A Cookie/Set-Cookie value is a third delimiter role after the single-token scheme value and the Digest parameter list: the header value is itself a `;`-delimited list of name=value pairs. The rule captures that value whole -- to the closing quote if quoted, with the quote optional so a truncated log line is still covered, otherwise to end-of-line -- then masks every pair VALUE and keeps every pair NAME. There is no list of cookie names anywhere; `session`, `sid`, `PHPSESSID`, `JSESSIONID`, `connect.sid`, `laravel_session`, `__Host-*` and `__Secure-*` are covered because none of them is special. THE EXEMPTION TABLE LISTS ATTRIBUTES, AND THAT DIRECTION IS THE POINT. Cookie names are application-chosen and unbounded, so a table of them fails OPEN on the next framework. RFC 6265 fixes the ATTRIBUTE vocabulary, so exempting that closed set and masking everything else fails CLOSED. Value shapes are checked as well as names, and the first pair is never exempt -- in both header directions it is the cookie itself -- so `Set-Cookie: sid=1; path=` does not walk through. Attributes and neighbouring log fields stay readable: destroying context is its own defect, and this file has already had to fix that once. THE ReDoS WAS MEASURED BEFORE IT WAS WRITTEN. The obvious inner pattern, /([^\s;,=]+)=([^\s;,]*)/g, is quadratic: 3.60 / 14.35 / 57.22 / 228.89 ms at 1/2/4/8 KiB, ratios 3.98 / 3.99 / 4.00, and a first probe at 16-128 KiB had to be killed at 120s. 128 KiB is exactly agentic.ts's maxBuffer and mcp/index.ts has no bound at all. So the captured value is scanned by a hand-written single forward pass, linear by construction rather than by measurement. Shipped scaling, base vs this change at 16/32/64/128 KiB: cookie-dense one line 2.05/1.91/1.97; the newline control 2.05/1.91/1.97; the digest rows unchanged; and the pre-existing generic-key quadratic neither added to nor removed (1612.0ms vs 1613.4ms at 64 KiB, 4.0x on both). Evidence: 36 tests pass, 0 fail, rc=0 measured unpiped on three consecutive runs; typecheck rc=0; build rc=0. No turbo/nx in this repo, so every run executes. An A/B output-drift corpus of 251 NON-cookie shapes is byte-identical between base and this change, with a positive control proving that comparison can report drift. Residuals are named rather than implied. Encoded and folded header spellings -- percent-encoded, fullwidth Unicode, obs-fold -- all still leak, are a property of every ASCII-literal key pattern in this file rather than of this rule, and are filed as todos 4afd4361 with their measurements. Two deliberate trades of this design are recorded in docs/redaction.md with measurements in both directions: whitespace-separated pairs carrying no semicolon, and a credential that happens to match an attribute's value shape under that attribute's name. The corpus varies cookie name, header spelling and case, separator, quoting including unterminated, the credential's position among pairs, pair count, Set-Cookie attributes, an attribute name reused as a cookie name, position within the line, and multi-line input. It does NOT vary encoding of the header name or separator, nor line folding -- and the defect class demonstrably lives on both, which is why they were probed separately and filed. Refs: todos 6200c4e4, 4afd4361 Agent: aemilius --- docs/redaction.md | 127 +++++++++++++++++++++++++++- src/redaction.ts | 180 ++++++++++++++++++++++++++++++++++++++++ tests/redaction.test.ts | 140 +++++++++++++++++++++++++++++++ 3 files changed, 443 insertions(+), 4 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index a8e4246..bcf26a0 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -43,6 +43,12 @@ throughout — no real credential is used or rendered at any point. | `{"Authorization": "Basic "}` and nested serialized JSON | redacted | | quoted, single-quoted and `export`-prefixed spellings | redacted | | AWS SigV4 trailing `Signature=`, bare / in-header / in a query string | redacted | +| `Cookie:` / `Set-Cookie:` — **every** `;`-delimited pair value, whatever the cookie is named (`session`, `sid`, `PHPSESSID`, `JSESSIONID`, `connect.sid`, `laravel_session`, `__Host-*`, `__Secure-*`) | redacted | +| the same, in any header spelling: `set-cookie:`, `HTTP_COOKIE=`, `cookie_header:`, `cookie=`, `cookie = `, `{"cookie":"…"}`, `'…'`, and a line truncated before its closing quote | redacted | +| a credential-bearing pair that is **not first** in the header — `Cookie: theme=dark; sid=; lang=en` | redacted | +| an RFC 6265 attribute name reused as a cookie name — `Set-Cookie: sid=1; path=` | redacted | +| Set-Cookie attributes (`Path`, `Domain`, `Expires`, `Max-Age`, `SameSite`, `HttpOnly`, `Secure`, …) beside a masked cookie | preserved | +| ordinary log fields beside a cookie header (`cookie: sid= status=200 user=bob`) | preserved | | a line **truncated mid-value** by a byte limit, so the closing quote is missing | redacted | | the value masked **without deleting the fields beside it** (`authorization=denied user=bob` keeps `user=bob`) | preserved | | `sk-`, `gsk_`, `csk-`, `AKIA…` provider keys | redacted | @@ -65,6 +71,56 @@ Two details are load-bearing and easy to undo by accident: the first version of this rule; see the covered table above. Neither is cosmetic — the first left a credential in every truncated log line, and the second could either delete adjacent fields or leave `sig=...` beside a marker. +4. **The cookie rule's exemption table lists ATTRIBUTES, not cookie names, and + that direction is the whole point.** Cookie names are chosen by the + application and are unbounded, so a table of credential-bearing names fails + OPEN on the next framework. RFC 6265 §4.1.1 fixes the *attribute* vocabulary, + and RFC 6265bis adds `Partitioned` — a closed set. Exempting that closed set + and masking everything else means an unrecognised name is treated as a cookie, + so the guard fails CLOSED. The attribute's **value shape** is checked as well + as its name, and the **first pair is never exempt** (in both header directions + the opening pair is the cookie itself, never an attribute), so + `Set-Cookie: sid=1; path=` does not walk through the exemption. + Both properties are pinned by tests. + +### A probe that PASSES FOR THE WRONG REASON hides a missing mechanism + +This is a distinct failure from the one the rest of this file guards against, and +it is worth naming because the defence against it is different. + +The known hazard is a check that **cannot fail** — a grep whose pattern cannot +match, an absence claim from a truncated read. The defence is a positive control. +This is the other shape: a check that **passes, correctly, for a reason unrelated +to the capability being checked**. A positive control does not catch it, because +the instrument really does fire. + +Measured on `main` at `b75e651`, before this change, with the same synthetic +value in every row: + +| shape | result before the cookie rule existed | why | +|---|---|---| +| `Cookie: __Secure-next-auth.session-token=` | **redacted** | the generic `*TOKEN*` key rule matched the substring `token` in the cookie **name** | +| `Cookie: session=` | leaked | — | +| `Cookie: sid=` | leaked | — | +| `Cookie: PHPSESSID=` | leaked | — | +| `Set-Cookie: sid=; Path=/; HttpOnly` | leaked | — | + +Nine of ten cookie shapes leaked. **There was no cookie handling in this file at +all** — and yet a reviewer's most likely single spot-check came back clean, +because the cookie name people reach for first tends to contain the word `token` +or `auth`. An incidental match by an unrelated rule masked an entirely absent +mechanism, and would have gone on masking it. + +Two consequences, both applied here: + +1. **A fixture must not contain a substring any other rule keys on.** + `COOKIE_CREDENTIAL` in `tests/redaction.test.ts` deliberately carries no + `token`, `secret`, `auth`, `key`, `sk-` or `gsk_`. A fixture another rule + happens to catch cannot detect the rule under test. +2. **Vary the axis the capability lives on, and check that the *majority* of it + behaves the same way.** One passing shape out of a family is evidence about + that shape, never about the family. Here the family is the cookie *name*, + which the application chooses and which is therefore unbounded. **Correction carried forward from #13 — the sibling's quadratic is `URL_USERINFO_PATTERN`, not `redactNamedAssignments`.** Earlier versions of this @@ -153,6 +209,48 @@ On bare repeated characters every version above is linear, but the `~0.5ms at at 50k, `47de35d` ranges `-` 1.7ms through `a` 3.7ms, `62f8f14` ranges 1.8–2.8ms. The cost is character-dependent, so quote the character with the number. +#### The cookie rule: the ReDoS that was measured BEFORE it was written + +The natural way to mask every pair inside a captured cookie header is one global +regex, `/([^\s;,=]+)=([^\s;,]*)/g`. **It is quadratic, and this repo shipped a +ReDoS inside a credential-leak fix once already**, so it was measured before being +written rather than after. + +Adversarial input — one run of non-delimiter characters carrying **no `=` at +all**, so every start position must scan the run and fail. station01, loadavg +10.3, `bun`: + +| 1 KiB | 2 KiB | 4 KiB | 8 KiB | ratios | +|---|---|---|---|---| +| 3.60ms | 14.35ms | 57.22ms | 228.89ms | **3.98× / 3.99× / 4.00×** | + +A first probe at 16–128 KiB had to be killed at 120s. The mechanism: `[^\s;,=]+` +followed by a literal `=` backtracks across the whole run at every start position, +and every retry position holds a character that is *by construction* not `=`. +128 KiB is exactly the `maxBuffer` bound in `src/agentic.ts`, and +`src/mcp/index.ts` applies no bound at all — the same two call sites that made the +Digest quadratic a ReDoS rather than a slow function. + +So the header value is captured by regex and then scanned by a **hand-written +single forward pass** (`redactCookiePairs`). Every character is visited once and +nothing is re-scanned, so it is linear *by construction* rather than by +measurement. Shipped behaviour, base `b75e651` vs this change, median of 9 after a +warmup, 16/32/64/128 KiB: + +| shape | base ratios | this change | absolute @128 KiB | +|---|---|---|---| +| cookie-dense, one line | 1.99 / 1.95 / 2.01 | 2.05 / 1.91 / 1.97 | 22.8ms → 20.2ms | +| cookie-dense, newline-separated (control) | 2.02 / 2.01 / 1.98 | 2.05 / 1.91 / 1.97 | 22.5ms → 18.1ms | +| `cookie` literal repeated, **no separator** | 2.05 / 2.00 / 2.02 | 2.01 / 2.01 / 2.01 | 4.2ms → 8.4ms | +| digest-dense, one line | 2.03 / 1.88 / 2.03 | 1.97 / 2.00 / 1.99 | 7.0ms → 7.0ms | +| auth-dense, the pre-existing quadratic | 3.94 / 4.00 / 4.04 | 3.95 / 3.97 / 4.02 | 1612.0ms → 1613.4ms @64 KiB | + +The last row is the honest one: **the pre-existing generic-key quadratic is +neither added to nor removed by this change** — 1612.0ms against 1613.4ms is +parity, and it stays listed as an open residual below. The `cookie`-literal row +costs 2× the base constant because a rule that did not exist now runs; the +exponent is unchanged. + ## Not covered — known residuals **Known leaking shapes measured at `127ffc4` on 2026-07-31 (UTC), station02. @@ -167,21 +265,42 @@ not probed at all. The list therefore tells you **scope, never completeness**. If you find a shape that leaks and is not here, add a row — that is this file working, not this file failing. +**A corpus's coverage is bounded by its AXES, not its size — so name them.** The +cookie work added a 30-shape coverage corpus and a 251-shape A/B output-drift +corpus (base vs this change, 0 drift, with a positive control proving the +comparison *can* report drift). Between them they vary: cookie **name**; header +**spelling** and **case**; **separator** (`:` / `=` / spaced); **quoting** +(bare / double / single / unterminated); the credential's **position** among +several pairs; **pair count**; presence of **Set-Cookie attributes**; an +attribute name **reused** as a cookie name; header **position within the line**; +and **multi-line** inputs. + +**Axes they do NOT vary, and the defect class demonstrably lives on some of +them:** any **encoding** of the header name or separator (percent-encoding, +Unicode/fullwidth, HTML entities), and **line folding**. Both were probed +separately after the fact and both leak — see the rows below. No amount of +additional shapes along the axes above would have found either, because the +generators cannot express them. + | shape | class | why it is still open | |---|---|---| | bare `Bearer ` with no `authorization` key | honest gap | The only rule that closes it — `/Bearer\s+[A-Za-z0-9._~+/=-]+/gi`, which `hasnaxyz/iapp-sms` carries — over-redacts ordinary prose: `Bearer authentication is required` becomes `Bearer [REDACTED] is required`. Closing this gap would trade a marker-free gap for a real over-redaction regression. Deliberately deferred, not overlooked. | -| `Cookie: session=` | honest gap | **Live.** Nothing keys on `session`. An agent logging an HTTP request is exactly where this appears, and it is not covered by the structural row below — `session=` **is** a recognisable key, just not one this file recognises. | -| `Set-Cookie: sid=; HttpOnly` | honest gap | **Live.** Same cause; `sid` is likewise not keyed on. | +| ~~`Cookie: session=`~~ | — | **CLOSED.** Covered by the cookie rule; see the covered table above. | +| ~~`Set-Cookie: sid=; HttpOnly`~~ | — | **CLOSED.** Same rule. | +| a cookie pair separated from the header by **whitespace only**, `Cookie: a=1 sid=` | honest gap | **Live**, and deliberate. RFC 6265 delimits cookie pairs with `;`, so the rule masks a pair only when it opens the header or follows a `;` or `,`. Without that condition a cookie header sitting mid-line turns the rest of the line into markers — `cookie: sid= status=200 user=bob` would lose `status` and `user`, which is the adjacent-field destruction this file has already had to fix once. Browsers and servers emit `; `, so the shape is non-conformant. Measured: `Cookie: a=1 sid=` → `a=[REDACTED] sid=`. | +| a **non-conformant cookie value containing whitespace**, `Cookie: sid=abc def` | honest gap | **Live**, same cause. `cookie-octet` excludes SP, so a value with an interior space is not a cookie value; the rule masks up to the space. Measured: `Cookie: sid=abcSYNTH defSYNTH` → `sid=[REDACTED] defSYNTH`. | +| a credential that **happens to match an attribute's value shape**, under that attribute's name and not in first position — `Cookie: a=1; path=/` | honest gap | **Live**, and the known cost of the exemption. The attribute table checks the value's shape as well as the name, so `path=` where `` is not path-shaped **is** masked; a token that genuinely begins with `/` under the name `path` is not. Measured both directions: `Cookie: a=1; path=abcSYNTHdef` → `path=[REDACTED]`, but `Cookie: a=1; path=/abcSYNTHdef` survives. Also `domain=abcSYNTH.def`. | +| **obs-fold** — a header value continued on the next line with leading whitespace | honest gap | **Live**, and shared with every other rule in this file: each value class excludes `\r\n`, so a folded continuation is never part of the match. Measured: `Cookie: a=1;\n sid=` → the continuation survives. Obsolete since RFC 7230 §3.2.4 but still produced by some proxies. | | URL userinfo — `scheme://user:@host` | honest gap | **Live.** `tai` has no userinfo rule at all. `hasnaxyz/iapp-sms` redacts this via `URL_USERINFO_PATTERN`; this is a genuine divergence, not a shared gap. | | PEM private key armour — a `-----BEGIN … PRIVATE KEY-----` block | honest gap | **Live.** No rule keys on PEM armour, and the body is bare base64 across newlines with no assignment shape to anchor on. (Written with an ellipsis on purpose so this row does not itself trip a secret scanner. Do not "fix" it back.) | | `authorization.value=Basic ` — `.` as a key separator | honest gap | **Live.** The trailing key run is `[A-Za-z0-9_-]*`, which excludes `.`, so the match stops at `authorization` and never reaches the `=`. | -| `authorization%3DBasic%20` — percent-encoded | honest gap | **Live.** Nothing percent-decodes free text before matching, so no key is ever seen. | +| `authorization%3DBasic%20` — percent-encoded | honest gap | **Live.** Nothing percent-decodes free text before matching, so no key is ever seen. **Measured to apply to the cookie rule identically**: `cookie%3Dsid%3D` and `cookie%3A%20sid%3D` both survive. It is one gap in the decoding layer, not one per rule. | | a bare high-entropy value with no recognisable key or prefix | honest gap | Structural. No keyword and no prefix means nothing to key on; this cannot be closed by pattern matching. | | quadratic growth on repeated `*AUTHORIZATION*`-shaped tokens | availability, P2 | **Live and pre-existing**, 4.0×/doubling on every version measured. Comes from the generic `[A-Z0-9_]*…[A-Z0-9_]*` key rules. Fixing it means restructuring those rules, not widening a pattern. The absolute cost is **not** unchanged by this change — see the performance section: auth-dense is 0.66× (faster). A `~324ms at 50k, byte-identical` figure previously stood here and is retracted as unreproducible. | | **NEW quadratic in the `response=` rule, on repeated `Authorization: Digest` within one line** | availability, **P1** | **Live, and introduced by this change** at `4b10ea5`. 2.0ms → 267ms at 50k and 4.3ms → 1060ms at 100k, **3.97×/doubling**, widening with n — a complexity-class change, not a constant factor. The same bytes newline-separated stay linear, which locates the cause in the `[^\r\n]*?` scan to end-of-line. **Reachable** — see the ReDoS row below. Tracked as `a0b7904f`; deliberately NOT fixed in the docs change that recorded it, because a documentation PR must not quietly alter redaction behaviour. | | digest, **unterminated** single-header shape costs ~23× more | availability, P2 | **Live, same origin** (`4b10ea5`): 2.6ms → 60ms at 50k. On *this* shape growth stays linear (~1.9×/doubling), so it is a constant-factor regression. Recorded separately from the row above because the two shapes differ in complexity class, and an earlier version of this file generalised from this one and got the other wrong. | | the `response=` quadratic is reachable from real call sites | **ReDoS, P1** | **Live.** `src/agentic.ts:97-98,105-106` redacts shell stdout/stderr — and `redactSensitiveText(stdout).slice(0, 12000)` truncates **after** redaction, so the 12k slice does **not** bound the regex input; the real bound is `maxBuffer: 128 * 1024`. Measured at 128KiB: **7ms → 2339ms**. `src/mcp/index.ts:107` redacts caller-supplied MCP tool text with **no maxBuffer at all**: at 512KiB, **30ms → 29058ms**. Single-threaded runtime, so this blocks the event loop. Anyone who can influence command output or call the MCP tool can spend it. | -| Unicode or non-ASCII spellings of header names | unmeasured | Never probed. Absence of a finding here is absence of evidence, not evidence of absence. | +| Unicode or non-ASCII spellings of header names | honest gap | **Live, and now measured** rather than merely suspected: a fullwidth `ｃookie: sid=` survives, because every key pattern in this file is an ASCII literal and nothing normalises the input first. Previously listed here as *unmeasured*; one probe moved it. The same is expected — but **not** measured — for the `authorization` and `signature` keys. | | whether every runtime call site actually routes through this function | unmeasured | This file measures the function, not its callers. A correct redactor on a path nothing calls redacts nothing. | **Over-masking that is intentional, stated so it is not mistaken for a bug.** diff --git a/src/redaction.ts b/src/redaction.ts index 9b41aa9..c13bfac 100644 --- a/src/redaction.ts +++ b/src/redaction.ts @@ -20,6 +20,40 @@ const AUTHORIZATION_PARAMETER_PATTERN = /(?:^|[\s,])[A-Za-z_][A-Za-z0-9_.-]{0,32 // the note above the Digest entry in SECRET_PATTERNS. const DIGEST_RESPONSE_PATTERN = /(\bresponse\s*=\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2?|[^\s'",]+)/gi; +// RFC 6265 fixes the Set-Cookie attribute vocabulary (§4.1.1), and RFC 6265bis +// adds `Partitioned`. That the vocabulary is CLOSED is the whole reason an +// attribute table is safe here where a table of credential-bearing cookie names +// would not be: cookie names are chosen by the application and are unbounded, so +// a list of them fails OPEN on the next framework — `sid`, `PHPSESSID`, +// `connect.sid`, `laravel_session` and `__Host-*` are all one deployment apart. +// Listing the attributes instead inverts that: any name NOT in this table is +// treated as a cookie and masked, so the guard fails CLOSED. +// +// The VALUE shape is checked as well as the name, so an attribute name reused as +// a cookie name cannot smuggle a credential past the exemption: +// `Set-Cookie: sid=1; path=` does not look like a path and is +// masked. Each shape is anchored and either bounded or free of nested +// quantifiers, so none can be made to backtrack. +const COOKIE_ATTRIBUTES = new Map([ + // Only the first whitespace-free run reaches here — `Expires=Wed, 09 Jun 2027` + // arrives as the token `Expires=Wed`, and the rest of the date carries no `=` + // and is passed through untouched. + ["expires", /^[A-Za-z0-9:+-]{1,32}$/], + ["max-age", /^-?\d{1,20}$/], + ["domain", /^\.?[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)*$/], + ["path", /^\/[^\s;,]{0,255}$/], + ["samesite", /^(?:strict|lax|none)$/i], + ["priority", /^(?:low|medium|high)$/i], + ["version", /^\d{1,3}$/], + // Valueless flags on the wire, so they normally carry no `=` and never reach + // this table at all. Some serializers render them as `Secure=true`. + ["secure", /^(?:true|false)$/i], + ["httponly", /^(?:true|false)$/i], + ["partitioned", /^(?:true|false)$/i] +]); + +const LONGEST_COOKIE_ATTRIBUTE_NAME = 16; + const SECRET_PATTERNS: Array<[RegExp, SecretReplacement]> = [ [/\b(sk-[A-Za-z0-9_-]{12,})\b/g, "[REDACTED_OPENAI_KEY]"], [/\b(gsk_[A-Za-z0-9_-]{12,})\b/g, "[REDACTED_GROQ_KEY]"], @@ -162,6 +196,48 @@ const SECRET_PATTERNS: Array<[RegExp, SecretReplacement]> = [ // The value class stops at `&`, `,` and `;` so that redacting a signature in a // query string does not swallow the unrelated parameters after it. [/(signature[A-Za-z0-9_-]{0,32}['"]?\s*[:=]\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2?|[^\s'"&,;]+)/gi, "$1$2[REDACTED]$2"], + // A `Cookie:` / `Set-Cookie:` value is a THIRD delimiter role, after the + // single-token scheme value and the comma-separated Digest parameter list: the + // header value is itself a `;`-delimited list of `name=value` pairs, and the + // credential is one pair among several whose name the application chose. A + // session cookie IS bearer authentication under a different header name, so + // anything that logs an HTTP request through this function was emitting live + // sessions. + // + // THE RULE KEYS ON THE ROLE, NOT ON NAMES. Every pair value in the header is + // masked and every pair NAME is kept, with the RFC's own attribute vocabulary + // exempted (see COOKIE_ATTRIBUTES). Keying on `session`, `sid`, `PHPSESSID`, + // `JSESSIONID`, `connect.sid` and so on is the list-shaped guard this file has + // now grown four times, each correct for the spellings its author pictured. + // The measured proof that a list reads as coverage while covering nothing: + // before this change `__Secure-next-auth.session-token=` was ALREADY + // redacted — not by any cookie handling, but because the generic `*TOKEN*` key + // rule matched the substring `token` inside the cookie NAME. One incidental hit + // is what makes a whole family look handled. + // + // The prefix follows the Authorization rules exactly and for the same measured + // reasons: no leading `\b` (`_` is a word character, so `\b` can never match + // inside `HTTP_COOKIE`), no leading `[A-Za-z0-9_-]*` (a star before the literal + // is quadratic), and the trailing key run bounded at 32 (unbounded, it rescans + // the rest of the input from every position the literal matches). `Set-Cookie` + // needs no separate rule: the match simply starts at `cookie` and leaves `Set-` + // outside it. + // + // The value is captured WHOLE — to the closing quote if quoted, with the quote + // optional so a log line truncated at a byte limit is still covered, and + // otherwise to end-of-line — and then scanned ONCE, linearly, by + // redactCookiePairs. That split is not stylistic. The obvious inner pattern, + // `/([^\s;,=]+)=([^\s;,]*)/g`, is a ReDoS: `[^\s;,=]+` followed by a literal + // `=` backtracks across the whole run at every start position, and every retry + // position holds a character that by construction is NOT `=`. Measured on + // station01 (loadavg 10.3) against a run carrying no `=` at all: 3.60 / 14.35 / + // 57.22 / 228.89 ms at 1/2/4/8 KiB — 3.98x, 3.99x, 4.00x per doubling, and a + // first probe at 16-128 KiB had to be killed at 120s. 128 KiB is exactly the + // `maxBuffer` bound in agentic.ts, and mcp/index.ts has no bound at all. A hand + // written forward scan has no backtracking to exploit, so the fix is a + // restructure rather than a bound — the same conclusion the Digest rule above + // reached by a different route. + [/(cookie[A-Za-z0-9_-]{0,32}['"]?\s*[:=]\s*)(?:(["'])((?:(?!\2)[^\r\n])*)(\2?)|([^\r\n]*))/gi, redactCookieHeader], [/(\b[A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|AUTH)[A-Z0-9_]*\s*=\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], [/((?:api|access|secret|token|password|passwd|pwd)[_-]?key?\s*=\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], [/(\b[A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|AUTH)[A-Z0-9_]*['"]?\s*:\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], @@ -195,6 +271,110 @@ function redactDigestResponse(match: string, prefix: string, headerValue: string return `${ prefix }${ headerValue.replace(DIGEST_RESPONSE_PATTERN, "$1$2[REDACTED]$2") }`; } +function redactCookieHeader( + match: string, + prefix: string, + quote: string | undefined, + quotedValue: string | undefined, + closingQuote: string | undefined, + unquotedValue: string | undefined +): string { + if (quote !== undefined) { + return `${ prefix }${ quote }${ redactCookiePairs(quotedValue ?? "") }${ closingQuote ?? "" }`; + } + + return `${ prefix }${ redactCookiePairs(unquotedValue ?? "") }`; +} + +function isCookieSeparator(character: string): boolean { + return character === ";" + || character === "," + || character === " " + || character === "\t" + || character === "\r" + || character === "\n" + || character === "\f" + || character === "\v"; +} + +// One forward pass over ONE captured header value. Every character is visited +// once and nothing is re-scanned, so this is linear by construction rather than +// by measurement — which is the property the regex it replaces could not have. +// +// Separator runs are emitted verbatim, so the output differs from the input only +// where a cookie value was masked. That matters beyond tidiness: over-redaction +// that DELETES a neighbouring field is worse than a gap, because the value is +// then gone from the log entirely, and this file has already had to fix that +// once (`authorization=denied user=bob` swallowing `user=bob`). +// +// A pair is masked when it OPENS the cookie-string or when the separator run +// before it carries a `;` or `,`. RFC 6265 delimits cookie pairs with `;`, so +// `key=value` text separated from the header only by whitespace is ordinary log +// context — `cookie: sid=X status=200 user=bob` keeps `status` and `user`. The +// comma is honoured as well so that a comma-folded header does not have its later +// pairs swallowed into one value; a comma INSIDE a date attribute simply starts a +// run of tokens that carry no `=` and are passed through. +function redactCookiePairs(value: string): string { + const parts: string[] = []; + let index = 0; + let tokenCount = 0; + let startsCookiePair = true; + + while (index < value.length) { + const separatorStart = index; + while (index < value.length && isCookieSeparator(value.charAt(index))) { + index += 1; + } + + if (index > separatorStart) { + const separator = value.slice(separatorStart, index); + parts.push(separator); + if (tokenCount > 0) { + startsCookiePair = separator.includes(";") || separator.includes(","); + } + } + + if (index >= value.length) { + break; + } + + const tokenStart = index; + while (index < value.length && !isCookieSeparator(value.charAt(index))) { + index += 1; + } + + const token = value.slice(tokenStart, index); + parts.push(startsCookiePair ? maskCookiePair(token, tokenCount === 0) : token); + tokenCount += 1; + } + + return parts.join(""); +} + +// `isFirstPair` is not a micro-optimisation. In BOTH header directions the +// opening pair is the cookie itself and never an attribute — RFC 6265's +// `set-cookie-string` is `cookie-pair *( ";" SP cookie-av )` — so exempting an +// attribute NAME there would mean `Set-Cookie: path=` walks straight +// through. An empty value is left alone: `sid=` is a deletion cookie, and +// printing a marker where no credential existed teaches readers to discount the +// marker. +function maskCookiePair(token: string, isFirstPair: boolean): string { + const separator = token.indexOf("="); + if (separator < 0 || separator === token.length - 1) { + return token; + } + + const name = token.slice(0, separator); + if (!isFirstPair && name.length <= LONGEST_COOKIE_ATTRIBUTE_NAME) { + const attributeShape = COOKIE_ATTRIBUTES.get(name.toLowerCase()); + if (attributeShape?.test(token.slice(separator + 1))) { + return token; + } + } + + return `${ name }=[REDACTED]`; +} + function redactParameterizedAuthorization( match: string, prefix: string, diff --git a/tests/redaction.test.ts b/tests/redaction.test.ts index b38ef5a..13c98b4 100644 --- a/tests/redaction.test.ts +++ b/tests/redaction.test.ts @@ -316,3 +316,143 @@ test("does not over-redact text that carries no credential", () => { test("strips hidden reasoning blocks", () => { expect(stripHiddenReasoning("private{\"command\":\"ls\",\"summary\":\"list\"}")).toBe("{\"command\":\"ls\",\"summary\":\"list\"}"); }); + +// --------------------------------------------------------------------------- +// Cookie headers. A `Cookie:` / `Set-Cookie:` value is a THIRD delimiter role: +// the header value is itself a `;`-delimited list of `name=value` pairs, and the +// credential is one pair among several. Nothing in this file keyed on that +// shape, so every cookie below survived verbatim on a PUBLIC repository whose +// SECURITY.md points readers at docs/redaction.md. +// +// Synthetic, never-issued. Deliberately carries no substring any other rule in +// this file keys on — no `token`, `secret`, `auth`, `key`, `sk-`, `gsk_`. A +// fixture that another rule happens to catch cannot detect a cookie rule at all, +// and that is not hypothetical: `__Secure-next-auth.session-token=` was +// ALREADY redacted before this change, purely because the generic `*TOKEN*` key +// rule matched the substring `token` in the cookie NAME. One spelling covered by +// an unrelated rule is exactly what makes a family read as handled. +const COOKIE_CREDENTIAL = "syntheticcookievalue0000notreal1111"; + +test("removes cookie values from Cookie and Set-Cookie headers", () => { + const cases = [ + // AXIS: cookie name. Names are chosen by the application, so any list of + // "session-ish" names fails open on the next framework. None of these + // contains a substring another rule in this file keys on. + `Cookie: session=${COOKIE_CREDENTIAL}`, + `Cookie: sid=${COOKIE_CREDENTIAL}`, + `Cookie: PHPSESSID=${COOKIE_CREDENTIAL}`, + `Cookie: JSESSIONID=${COOKIE_CREDENTIAL}`, + `Cookie: connect.sid=${COOKIE_CREDENTIAL}`, + `Cookie: laravel_session=${COOKIE_CREDENTIAL}`, + `Cookie: _csrf=${COOKIE_CREDENTIAL}`, + `Cookie: __Host-sid=${COOKIE_CREDENTIAL}`, + `Cookie: __Secure-sid=${COOKIE_CREDENTIAL}`, + // AXIS: header spelling and case. `Set-Cookie` is the response direction and + // leaks the same value; `\b` would never have matched inside `HTTP_COOKIE`. + `Set-Cookie: sid=${COOKIE_CREDENTIAL}`, + `set-cookie: sid=${COOKIE_CREDENTIAL}`, + `SET-COOKIE: SID=${COOKIE_CREDENTIAL}`, + `HTTP_COOKIE=session=${COOKIE_CREDENTIAL}`, + `cookie_header: sid=${COOKIE_CREDENTIAL}`, + // AXIS: separator, and whitespace around it. + `cookie=sid=${COOKIE_CREDENTIAL}`, + `cookie = sid=${COOKIE_CREDENTIAL}`, + `Cookie:sid=${COOKIE_CREDENTIAL}`, + // AXIS: position of the credential-bearing pair among several. A rule that + // only reaches the first pair passes the single-pair cases above. + `Cookie: theme=dark; sid=${COOKIE_CREDENTIAL}; lang=en`, + `Cookie: a=1; b=2; c=3; sid=${COOKIE_CREDENTIAL}`, + `Cookie: sid=${COOKIE_CREDENTIAL}; theme=dark`, + // AXIS: quoting, including the truncated line that syslog/journald/CloudWatch + // produce — the closing quote is simply absent. + `{"headers":{"cookie":"session=${COOKIE_CREDENTIAL}"}}`, + `{"Set-Cookie": "sid=${COOKIE_CREDENTIAL}; Path=/"}`, + `cookie='sid=${COOKIE_CREDENTIAL}'`, + `{"lvl":"info","headers":{"cookie":"sid=${COOKIE_CREDENTIAL}`, + // AXIS: Set-Cookie attributes trailing the pair. + `Set-Cookie: sid=${COOKIE_CREDENTIAL}; Path=/; HttpOnly; Secure; SameSite=Lax`, + `Set-Cookie: sid=${COOKIE_CREDENTIAL}; Expires=Wed, 09 Jun 2027 10:18:14 GMT; Max-Age=3600`, + // AXIS: an attribute NAME reused as a cookie name. The exemption that keeps + // `Path=/` readable must not become a way to smuggle a credential past it. + `Set-Cookie: sid=1; path=${COOKIE_CREDENTIAL}`, + `Cookie: domain=${COOKIE_CREDENTIAL}`, + // AXIS: not at the start of the line, and more than one header per input. + `req GET /v1 cookie: sid=${COOKIE_CREDENTIAL} done`, + `Cookie: sid=${COOKIE_CREDENTIAL}\nAuthorization: Bearer ${BEARER_CREDENTIALS}` + ] as const; + + for (const input of cases) { + const redacted = redactSensitiveText(input); + // Assert the CREDENTIAL IS GONE. A `[REDACTED]`-present assertion would pass + // against `Cookie: session=` unchanged, because other rules on + // the same line can print a marker. + expect(redacted).not.toContain(COOKIE_CREDENTIAL); + expect(redactSensitiveText(redacted)).toBe(redacted); + } +}); + +test("positive control: the cookie absence assertion can fail", () => { + // Without this, `not.toContain(COOKIE_CREDENTIAL)` could be passing because the + // literal never survives anything, rather than because a cookie rule masked it. + const prose = `The build log mentioned ${COOKIE_CREDENTIAL} in passing.`; + expect(redactSensitiveText(prose)).toContain(COOKIE_CREDENTIAL); + + // …and the paired must-redact control, so the two point in opposite directions: + // the same literal under a shape this file already covers IS removed. + expect(redactSensitiveText(`Authorization: Bearer ${COOKIE_CREDENTIAL}`)).not.toContain(COOKIE_CREDENTIAL); +}); + +test("keeps Set-Cookie attributes and neighbouring log fields readable", () => { + // Over-redaction that DESTROYS context is its own defect — a masked cookie + // whose Path/Domain/Expires went with it loses the forensic value of the log + // line. RFC 6265's attribute vocabulary is CLOSED, which is what makes an + // attribute exemption safe where a cookie-name allowlist would not be: an + // unrecognised name is treated as a cookie and masked. + const attributed = `Set-Cookie: sid=${COOKIE_CREDENTIAL}; Path=/admin; Domain=example.test; Max-Age=3600; SameSite=Lax; Expires=Wed, 09 Jun 2027 10:18:14 GMT; HttpOnly; Secure`; + const redacted = redactSensitiveText(attributed); + expect(redacted).not.toContain(COOKIE_CREDENTIAL); + for (const survivor of ["Path=/admin", "Domain=example.test", "Max-Age=3600", "SameSite=Lax", "Expires=Wed, 09 Jun 2027 10:18:14 GMT", "HttpOnly", "Secure"]) { + expect(redacted).toContain(survivor); + } + + // A cookie header sitting mid-line must not turn the rest of the line into + // markers. RFC 6265 delimits cookie pairs with `;` — whitespace-separated + // `key=value` text after the header is ordinary log context, not a cookie. + const inline = `cookie: sid=${COOKIE_CREDENTIAL} status=200 user=bob duration=1.5s`; + const inlineRedacted = redactSensitiveText(inline); + expect(inlineRedacted).not.toContain(COOKIE_CREDENTIAL); + for (const survivor of ["status=200", "user=bob", "duration=1.5s"]) { + expect(inlineRedacted).toContain(survivor); + } + + // Prose that merely contains the word must come back byte-identical. + for (const safe of [ + "Cookie consent is handled by the gateway.", + "The cookie policy changed in June.", + "cookies: chocolate and vanilla" + ]) { + expect(redactSensitiveText(safe)).toBe(safe); + } +}); + +test("the cookie rule stays linear on a cookie-dense single line", () => { + // Same instrument as the Digest pair above: a GROWTH RATIO, not a millisecond + // figure, because the exponent does not move with machine load. The naive + // inner pattern for this rule — /([^\s;,=]+)=([^\s;,]*)/g — was measured at + // 3.98x/3.99x/4.00x per doubling on a long run carrying no `=` at all, which + // is why the implementation scans linearly instead of matching pairs. + // + // This is the adversarial shape for that pattern: header keys with no + // separator, so every start position that matches the literal must scan and + // fail. + expect(growthPerDoubling("Cookie: sid=a; b=c; ", "")).toBeLessThan(2.8); + expect(growthPerDoubling("cookiecookiecookie", "")).toBeLessThan(2.8); +}); + +test("newline-separated cookie control: identical bytes, bounded rescans", () => { + // The PAIR is the signal. If a future change makes the cookie rule rescan to + // end-of-line from every start position, the single-line shape above goes + // quadratic while this one stays flat. One timing alone cannot tell "the regex + // got slow" from "the box got busy". + expect(growthPerDoubling("Cookie: sid=a; b=c; ", "\n")).toBeLessThan(2.8); +}); From bb0a3f2f111a08bc2df662485254aed0a5225e17 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sat, 1 Aug 2026 07:54:20 +0300 Subject: [PATCH 2/2] fix(redaction): mask request cookie attribute names Agent: Augustus --- docs/redaction.md | 16 ++++++++-------- src/redaction.ts | 34 ++++++++++++++++++---------------- tests/redaction.test.ts | 2 ++ 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/docs/redaction.md b/docs/redaction.md index bcf26a0..0b398e1 100644 --- a/docs/redaction.md +++ b/docs/redaction.md @@ -46,7 +46,7 @@ throughout — no real credential is used or rendered at any point. | `Cookie:` / `Set-Cookie:` — **every** `;`-delimited pair value, whatever the cookie is named (`session`, `sid`, `PHPSESSID`, `JSESSIONID`, `connect.sid`, `laravel_session`, `__Host-*`, `__Secure-*`) | redacted | | the same, in any header spelling: `set-cookie:`, `HTTP_COOKIE=`, `cookie_header:`, `cookie=`, `cookie = `, `{"cookie":"…"}`, `'…'`, and a line truncated before its closing quote | redacted | | a credential-bearing pair that is **not first** in the header — `Cookie: theme=dark; sid=; lang=en` | redacted | -| an RFC 6265 attribute name reused as a cookie name — `Set-Cookie: sid=1; path=` | redacted | +| an RFC 6265 attribute name reused as a request cookie name — `Cookie: sid=1; path=/` / `domain=.example` | redacted | | Set-Cookie attributes (`Path`, `Domain`, `Expires`, `Max-Age`, `SameSite`, `HttpOnly`, `Secure`, …) beside a masked cookie | preserved | | ordinary log fields beside a cookie header (`cookie: sid= status=200 user=bob`) | preserved | | a line **truncated mid-value** by a byte limit, so the closing quote is missing | redacted | @@ -75,11 +75,11 @@ Two details are load-bearing and easy to undo by accident: that direction is the whole point.** Cookie names are chosen by the application and are unbounded, so a table of credential-bearing names fails OPEN on the next framework. RFC 6265 §4.1.1 fixes the *attribute* vocabulary, - and RFC 6265bis adds `Partitioned` — a closed set. Exempting that closed set - and masking everything else means an unrecognised name is treated as a cookie, - so the guard fails CLOSED. The attribute's **value shape** is checked as well - as its name, and the **first pair is never exempt** (in both header directions - the opening pair is the cookie itself, never an attribute), so + and RFC 6265bis adds `Partitioned` — a closed set. The exemption is applied + only in the `Set-Cookie` direction; request `Cookie` headers have no + attributes, so every pair is masked even when its name is `path`, `domain` or + another RFC attribute word. In `Set-Cookie`, the attribute's **value shape** is + checked as well as its name, and the **first pair is never exempt**, so `Set-Cookie: sid=1; path=` does not walk through the exemption. Both properties are pinned by tests. @@ -266,7 +266,7 @@ completeness**. If you find a shape that leaks and is not here, add a row — that is this file working, not this file failing. **A corpus's coverage is bounded by its AXES, not its size — so name them.** The -cookie work added a 30-shape coverage corpus and a 251-shape A/B output-drift +cookie work added a 32-shape coverage corpus and a 251-shape A/B output-drift corpus (base vs this change, 0 drift, with a positive control proving the comparison *can* report drift). Between them they vary: cookie **name**; header **spelling** and **case**; **separator** (`:` / `=` / spaced); **quoting** @@ -289,7 +289,7 @@ generators cannot express them. | ~~`Set-Cookie: sid=; HttpOnly`~~ | — | **CLOSED.** Same rule. | | a cookie pair separated from the header by **whitespace only**, `Cookie: a=1 sid=` | honest gap | **Live**, and deliberate. RFC 6265 delimits cookie pairs with `;`, so the rule masks a pair only when it opens the header or follows a `;` or `,`. Without that condition a cookie header sitting mid-line turns the rest of the line into markers — `cookie: sid= status=200 user=bob` would lose `status` and `user`, which is the adjacent-field destruction this file has already had to fix once. Browsers and servers emit `; `, so the shape is non-conformant. Measured: `Cookie: a=1 sid=` → `a=[REDACTED] sid=`. | | a **non-conformant cookie value containing whitespace**, `Cookie: sid=abc def` | honest gap | **Live**, same cause. `cookie-octet` excludes SP, so a value with an interior space is not a cookie value; the rule masks up to the space. Measured: `Cookie: sid=abcSYNTH defSYNTH` → `sid=[REDACTED] defSYNTH`. | -| a credential that **happens to match an attribute's value shape**, under that attribute's name and not in first position — `Cookie: a=1; path=/` | honest gap | **Live**, and the known cost of the exemption. The attribute table checks the value's shape as well as the name, so `path=` where `` is not path-shaped **is** masked; a token that genuinely begins with `/` under the name `path` is not. Measured both directions: `Cookie: a=1; path=abcSYNTHdef` → `path=[REDACTED]`, but `Cookie: a=1; path=/abcSYNTHdef` survives. Also `domain=abcSYNTH.def`. | +| ~~a request Cookie credential that happens to match an attribute's value shape, under that attribute's name and not in first position — `Cookie: a=1; path=/`~~ | — | **CLOSED.** Request `Cookie` headers no longer apply the `Set-Cookie` attribute exemption; `Cookie: a=1; path=/` and the domain-shaped equivalent are masked. `Set-Cookie` attributes remain preserved. | | **obs-fold** — a header value continued on the next line with leading whitespace | honest gap | **Live**, and shared with every other rule in this file: each value class excludes `\r\n`, so a folded continuation is never part of the match. Measured: `Cookie: a=1;\n sid=` → the continuation survives. Obsolete since RFC 7230 §3.2.4 but still produced by some proxies. | | URL userinfo — `scheme://user:@host` | honest gap | **Live.** `tai` has no userinfo rule at all. `hasnaxyz/iapp-sms` redacts this via `URL_USERINFO_PATTERN`; this is a genuine divergence, not a shared gap. | | PEM private key armour — a `-----BEGIN … PRIVATE KEY-----` block | honest gap | **Live.** No rule keys on PEM armour, and the body is bare base64 across newlines with no assignment shape to anchor on. (Written with an ellipsis on purpose so this row does not itself trip a secret scanner. Do not "fix" it back.) | diff --git a/src/redaction.ts b/src/redaction.ts index c13bfac..5fb0bad 100644 --- a/src/redaction.ts +++ b/src/redaction.ts @@ -220,8 +220,8 @@ const SECRET_PATTERNS: Array<[RegExp, SecretReplacement]> = [ // inside `HTTP_COOKIE`), no leading `[A-Za-z0-9_-]*` (a star before the literal // is quadratic), and the trailing key run bounded at 32 (unbounded, it rescans // the rest of the input from every position the literal matches). `Set-Cookie` - // needs no separate rule: the match simply starts at `cookie` and leaves `Set-` - // outside it. + // is captured as its own direction so RFC attributes are preserved only there; + // request `Cookie` headers mask every pair, including names such as `path`. // // The value is captured WHOLE — to the closing quote if quoted, with the quote // optional so a log line truncated at a byte limit is still covered, and @@ -237,7 +237,7 @@ const SECRET_PATTERNS: Array<[RegExp, SecretReplacement]> = [ // written forward scan has no backtracking to exploit, so the fix is a // restructure rather than a bound — the same conclusion the Digest rule above // reached by a different route. - [/(cookie[A-Za-z0-9_-]{0,32}['"]?\s*[:=]\s*)(?:(["'])((?:(?!\2)[^\r\n])*)(\2?)|([^\r\n]*))/gi, redactCookieHeader], + [/((?:set-)?cookie[A-Za-z0-9_-]{0,32}['"]?\s*[:=]\s*)(?:(["'])((?:(?!\2)[^\r\n])*)(\2?)|([^\r\n]*))/gi, redactCookieHeader], [/(\b[A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|AUTH)[A-Z0-9_]*\s*=\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], [/((?:api|access|secret|token|password|passwd|pwd)[_-]?key?\s*=\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], [/(\b[A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|AUTH)[A-Z0-9_]*['"]?\s*:\s*)(?:(["'])(?:(?!\2)[^\r\n])*\2|[^\s'"]+)/gi, "$1$2[REDACTED]$2"], @@ -279,11 +279,12 @@ function redactCookieHeader( closingQuote: string | undefined, unquotedValue: string | undefined ): string { + const preserveAttributes = prefix.toLowerCase().startsWith("set-cookie"); if (quote !== undefined) { - return `${ prefix }${ quote }${ redactCookiePairs(quotedValue ?? "") }${ closingQuote ?? "" }`; + return `${ prefix }${ quote }${ redactCookiePairs(quotedValue ?? "", preserveAttributes) }${ closingQuote ?? "" }`; } - return `${ prefix }${ redactCookiePairs(unquotedValue ?? "") }`; + return `${ prefix }${ redactCookiePairs(unquotedValue ?? "", preserveAttributes) }`; } function isCookieSeparator(character: string): boolean { @@ -314,7 +315,7 @@ function isCookieSeparator(character: string): boolean { // comma is honoured as well so that a comma-folded header does not have its later // pairs swallowed into one value; a comma INSIDE a date attribute simply starts a // run of tokens that carry no `=` and are passed through. -function redactCookiePairs(value: string): string { +function redactCookiePairs(value: string, preserveAttributes: boolean): string { const parts: string[] = []; let index = 0; let tokenCount = 0; @@ -344,28 +345,29 @@ function redactCookiePairs(value: string): string { } const token = value.slice(tokenStart, index); - parts.push(startsCookiePair ? maskCookiePair(token, tokenCount === 0) : token); + parts.push(startsCookiePair ? maskCookiePair(token, tokenCount === 0, preserveAttributes) : token); tokenCount += 1; } return parts.join(""); } -// `isFirstPair` is not a micro-optimisation. In BOTH header directions the -// opening pair is the cookie itself and never an attribute — RFC 6265's -// `set-cookie-string` is `cookie-pair *( ";" SP cookie-av )` — so exempting an -// attribute NAME there would mean `Set-Cookie: path=` walks straight -// through. An empty value is left alone: `sid=` is a deletion cookie, and -// printing a marker where no credential existed teaches readers to discount the -// marker. -function maskCookiePair(token: string, isFirstPair: boolean): string { +// `isFirstPair` is not a micro-optimisation. When Set-Cookie attributes are +// being preserved, the opening pair is still the cookie itself and never an +// attribute — RFC 6265's `set-cookie-string` is +// `cookie-pair *( ";" SP cookie-av )` — so exempting an attribute NAME there +// would mean `Set-Cookie: path=` walks straight through. Request +// Cookie headers pass `preserveAttributes=false` and mask every pair. An empty +// value is left alone: `sid=` is a deletion cookie, and printing a marker where +// no credential existed teaches readers to discount the marker. +function maskCookiePair(token: string, isFirstPair: boolean, preserveAttributes: boolean): string { const separator = token.indexOf("="); if (separator < 0 || separator === token.length - 1) { return token; } const name = token.slice(0, separator); - if (!isFirstPair && name.length <= LONGEST_COOKIE_ATTRIBUTE_NAME) { + if (preserveAttributes && !isFirstPair && name.length <= LONGEST_COOKIE_ATTRIBUTE_NAME) { const attributeShape = COOKIE_ATTRIBUTES.get(name.toLowerCase()); if (attributeShape?.test(token.slice(separator + 1))) { return token; diff --git a/tests/redaction.test.ts b/tests/redaction.test.ts index 13c98b4..0b9a94c 100644 --- a/tests/redaction.test.ts +++ b/tests/redaction.test.ts @@ -376,6 +376,8 @@ test("removes cookie values from Cookie and Set-Cookie headers", () => { // `Path=/` readable must not become a way to smuggle a credential past it. `Set-Cookie: sid=1; path=${COOKIE_CREDENTIAL}`, `Cookie: domain=${COOKIE_CREDENTIAL}`, + `Cookie: sid=1; path=/${COOKIE_CREDENTIAL}`, + `Cookie: sid=1; domain=${COOKIE_CREDENTIAL}.example`, // AXIS: not at the start of the line, and more than one header per input. `req GET /v1 cookie: sid=${COOKIE_CREDENTIAL} done`, `Cookie: sid=${COOKIE_CREDENTIAL}\nAuthorization: Bearer ${BEARER_CREDENTIALS}`