fix(path): stop Windows paths leaking when an email or secret follows on the same line - #127
fix(path): stop Windows paths leaking when an email or secret follows on the same line#127addyCooks wants to merge 3 commits into
Conversation
will-lamerton
left a comment
will-lamerton
left a comment
There was a problem hiding this comment.
Nice work, and the two-layer approach is the right instinct: fix the detector, then harden resolveCollisions so any over-broad detector degrades to over-redaction rather than a silent leak. The resolver half I'd merge as-is. One blocker on the regex.
Blocking: paths ending in a space-bearing segment now truncate and leak the remainder
Excluding whitespace from the final segment cuts any path that ends in a segment containing a space. Verified end-to-end on this branch:
"Path is C:\Program Files" -> "Path is «Path_1» Files" (Path_1 = C:\Program)
"User dir C:\Users\John Doe" -> "User dir «Path_1» Doe" (Path_1 = C:\Users\John)
Both matched in full on main, so this is a regression — and it's the same signature as #123: a line that looks scrubbed sitting next to cleartext PII, here a surname. The collision hardening can't rescue it, since no other detector claims Doe.
Note the quoted test case passes because app.exe has no space, not because of the quotes — the trailing segment is what decides it. Suggested direction: an explicit alternation, a quoted branch that permits spaces freely and an unquoted branch that doesn't. Either way this needs a test for the unquoted trailing-space case.
Non-blocking
- The regex fix only covers the final segment. Interior segments still allow whitespace, so a later backslash re-opens the greedy match:
Log C:\a\b.txt mail alice@corp.com dir share\x.txtstill swallows the email at the detector layer. No cleartext leak (the new narrowing catches it, which is a good demonstration that the hardening works) but "the match stops at whitespace" overstates it in both the description and the test name. subtractdrops a part silently when it overlaps another accepted finding, which reintroduces the leak class narrowly. I couldn't construct a natural repro, so just a comment noting the trade-off would do.confidencecarries over unchanged via{...loser}— a leftover fragment is weaker evidence than the original match.- Consider a test asserting
resolveCollisionsoutput is pairwise non-overlapping. The function now adds findings rather than only filtering, andscrub's right-to-left replacement depends on that invariant. - The changeset is ~180 words for a patch entry; the first sentence plus the hardening clause would carry it, with the rest staying here in the PR body.
Test quality is high throughout — a winner inside the loser keeps the loser on both sides and the same-category 10 Downing St case target real edge conditions, and asserting the sessionMap value in the scrub test covers the rehydrate side properly.
will-lamerton
left a comment
will-lamerton
left a comment
There was a problem hiding this comment.
Re-reviewed on df557a6. The resolver half is fully addressed and then some. The regex blocker is half fixed.
Still blocking: the same leak, now on lowercase segments
The two cases I gave now pass. But the new boundary is capitalisation rather than the final segment, so the identical leak survives whenever a space-bearing segment starts lowercase:
"Home C:\Users\john smith\AppData\creds.json" -> "Home «Path_1» smith\AppData\creds.json"
"C:\dev\acme corp\client list.csv" -> "«Path_1» corp\client list.csv"
"Build failed in C:\repos\my project\src\config.ini" -> "«Path_1» project\src\config.ini"
"Backup to D:\backups\jan 2026\payroll.xlsx" -> truncates at "D:\backups"
All four match in full on main, so these are regressions, and the signature is the one from #123: a placeholder next to a cleartext surname, employer or filename. my project, client list.csv, quarterly report.xlsx are ordinary shapes.
The underlying issue is direction. The resolver hardening in this same PR makes over-matching safe, so the detector should over-match when ambiguous; the current rule under-matches (a lowercase token ends the path), which is the unsafe side. I checked the safety claim by pairing main's greedy regex with this PR's resolver: #123 closes with no leaks, only over-redaction of trailing prose. So the heuristic is buying precision, not safety, and paying for it with a leak.
One direction that works, if useful: also continue over a space when the next token still contains a backslash (more path follows) or carries a file extension. I prototyped it against every case in your test file plus the four above; all hold, cfg.ini owner alice@corp.com still stops at the file, and Copy C:\a\b.txt to D:\c\d.txt stays two findings.
Non-blocking from last round: all addressed
- Interior segments / overstated test name: fixed, with a direct test for the later-backslash case.
subtractdropping a part: fixed properly rather than documented. I fuzzedresolveCollisionswith 200k random inputs (no overlaps, no span/value mismatches, no throws, max 0.19ms), and a second fuzz over distinct-category inputs confirms no covered character is ever lost. That leak class is closed, not narrowed.- Non-overlap test: added, and the invariant holds under fuzz.
- Changeset: trimmed.
confidence: my point was wrong,Findinghas no such field. The comment onsubtractis the right resolution.
Two new items
docs/features/detectors.md contradicts the code it ships with. It lists a third exception, "a narrowed part that still overlaps another already-accepted finding is not narrowed a second time", but the work queue in this same commit removed that behaviour (the commit message says as much). Path 0-24 against Secret 5-9 and Email 15-19 yields Path 0-4, Secret 5-9, Path 10-14, Email 15-19, Path 20-24: re-narrowed, not dropped. That sentence should go.
The PR description is stale. It still describes the abandoned approach ("the final segment of WIN_PATH_REGEX now excludes whitespace. Interior segments still allow spaces"), claims 7 new tests, and reports 243 passed / 2 failed. Current suite is 278 passed, 0 failed.
No ReDoS risk, for what it's worth: the unquoted branch cannot fail after the drive letter, so there is nothing to backtrack into. Timings stay flat to 2KB of adversarial input.
|
Hey @will-lamerton, Pushed as Took your rule: a space continues the path when the next token contains a backslash, looks like a path component, or carries a file extension with the last two blocked straight after an extension so You were right that direction was the problem, so one consequence is deliberate: Dropped the stale third-exception sentence, and checked your worked example against the code On the failing Semgrep check: it fails on |
|
Thanks @will-lamerton, The lowercase blocker is fixed (8d87298, pushed before this round landed). All four of your cases match in full. The rule is the direction you suggested: continue over a space when the next token contains a backslash, looks like a path component, or carries a file extension. Each lookahead reaches only to the end of the current token, so Since over-matching is now deliberate, I verified the safety claim end-to-end rather than assuming it: for every shape where the detector over-matches ( Docs sentence is gone. Line 64 says a narrowed part is narrowed again, two exceptions not three. Confirmed against the code: Path 0-24 vs Secret 5-9 and Email 15-19 gives your five parts, non-overlapping, all 24 chars retained. PR description rewritten it still described the abandoned final-segment approach. One note on numbers: I used 74 passing across the three suites this PR touches rather than repeating your 278, because this checkout has 5 pre-existing Windows-only failures (NTFS Also re-ran ReDoS across five adversarial shapes: flat and sub-millisecond to 40 KB, matching your read. Merged |
WIN_PATH_REGEX allowed spaces in its final segment, so a `C:\...` path matched greedily to the end of the line. When an email or secret sat on that line, resolveCollisions dropped the over-wide Path finding in favour of the higher-priority finding inside it and the path was emitted in cleartext next to a placeholder that made the line look scrubbed. With no competing entity, the same over-match swallowed the trailing prose into a single «Path_1». Exclude whitespace from the final segment. Interior segments still allow it, so "C:\Program Files\App\app.exe" is matched in full. As hardening, resolveCollisions now narrows a losing finding to the part the winner does not cover instead of discarding it, so an over-broad detector degrades to over-redaction rather than a silent leak. Rival findings of the same category are still resolved in favour of the winner alone, and a loser whose value does not map 1:1 onto its span cannot be re-sliced here, so it is dropped as before. Closes Nano-Collective#123
Excluding whitespace from the final segment closed the leak but truncated any path whose last segment contains a space: "C:\Program Files" matched only "C:\Program", and "C:\Users\John Doe" left the surname in cleartext next to a placeholder — the same signature as the bug it was fixing. Match the two forms separately instead. Inside double quotes the path is already delimited, so spaces are allowed freely. Unquoted, a space only continues the path when the token after it still looks like a path component (capitalised, a digit, or an opening paren) and the preceding segment did not already end in a file extension. That keeps "C:\Users\John Doe" and "C:\Program Files (x86)\Common Files" whole while "C:\app\cfg.ini owner alice@corp.com" still ends at the file. The rule applies to every segment rather than only the last, so a later backslash no longer re-opens the match across intervening prose: "C:\a\b.txt mail alice@corp.com dir share\x.txt" now yields one finding. resolveCollisions becomes a work queue. Narrowing can place a part to the right of findings still waiting, so a candidate could overlap more than one accepted finding while only the first was ever compared, leaving overlapping spans in the output that scrub's right-to-left replacement relies on not seeing. Unsettled findings are requeued until they overlap nothing, which also removes the case where a narrowed part that collided with another accepted finding was silently dropped. Refs Nano-Collective#123
The capitalisation rule fixed the two reported cases but moved the boundary rather than removing it, so the same truncation leak survived wherever a space-bearing segment starts lowercase: C:\Users\john smith\AppData\creds.json -> C:\Users\john C:\dev\acme corp\client list.csv -> C:\dev\acme C:\repos\my project\src\config.ini -> C:\repos\my All three match in full on main, so these were regressions with the signature from Nano-Collective#123: a placeholder beside a cleartext surname, employer or filename. Direction was the underlying problem. The resolver hardening in this branch makes over-matching recoverable — an over-broad Path is narrowed against the email or secret inside it — while under-matching drops the tail into cleartext with no way back. So a space now continues the path whenever the next token contains a backslash, looks like a path component (capitalised, a digit, an opening paren), or carries a file extension. Where none of those hold the path ends, which is what still stops "cfg.ini owner alice@corp.com" at the file and keeps "C:\a\b.txt to D:\c\d.txt" as two findings. Each lookahead reaches only to the end of the current token, so a backslash later on the line cannot reach back and re-open the match. The three alternatives start with disjoint characters, so every step consumes exactly one character with nothing to backtrack into. Also drop the stale third exception from the collision section of docs/features/detectors.md: the work queue narrows a colliding part again rather than dropping it, so the sentence described behaviour this branch had already removed. Refs Nano-Collective#123
835171d to
bfbaf7b
Compare
2 participants
Closes #123
Description
Fixes the cleartext Windows-path leak in #123, at both contributing sites the issue identified.
src/detectors/path.ts— Windows paths are matched in two forms. A quoted path is delimited by its quotes, so spaces are taken freely ("C:\Program Files\App\app.exe"). An unquoted path continues over a space when the next token still contains a backslash (more path demonstrably follows), looks like a path component (capitalised, a digit, an opening paren), or carries a file extension (it reads as the filename the path ends at). Each lookahead reaches only to the end of the current token, so a backslash later on the line cannot reach back and re-open the match.The direction is deliberate: when a path is ambiguous the detector matches more, not less. The two errors are not symmetric. Over-matching is recoverable — collision resolution narrows the
Pathagainst whatever email or secret sits inside it, so the worst case is over-redaction. Under-matching is the bug in #123: the tail the match dropped goes to the model in cleartext, sitting next to a placeholder that makes the line look scrubbed.src/core/collision-resolver.ts— a losing finding is narrowed to the part the winner does not cover instead of being discarded, and narrowed parts are re-queued so a part that still overlaps something is narrowed again until nothing overlaps. Two cases still drop the loser: same-category overlaps (rival readings of one entity, so the winner's span is authoritative), and findings whosevaluedoes not map 1:1 onto theirspan(the covered text cannot be re-sliced without the source string).docs/features/detectors.md— documents both the space rule and the narrowing.Not platform-gated: nothing in the detector checks
process.platform, so scrubbing a colleague's Windows log or stack trace on macOS/Linux hit this too.Type of Change
Testing Notes
Automated — 29 new tests across
tests/detectors/path.test.ts,tests/core/collision-resolver.test.tsandtests/core/scrub.test.ts, covering:C:\Users\john smith\AppData\creds.json,C:\dev\acme corp\client list.csv,C:\repos\my project\src\config.ini,D:\backups\jan 2026\payroll.xlsx,C:\Program Files,C:\Users\John DoeC:\app\cfg.ini ownerends at the file,C:\logs\out.txt Failed to startdoes not absorb the sentence,Copy C:\a\b.txt to D:\c\d.txtstays two findings, a later backslash does not re-open the match across prosescrub.test.ts, asserting thesessionMapvalue so the rehydrate side is covered tooManual — the three repro commands from the issue, through the built
dist/CLI:inspectreports the corrected span (Path10-24), and«Path_1»maps toC:\app\cfg.inirather than the trailing prose.Suite — 74 passing across the three suites this PR touches. The full local run also shows the pre-existing Windows-only failures documented on earlier PRs (a
chmodSyncno-op on NTFS instorage, an ESMimport()of an absoluteC:\path inrule-packs, and stdin-piping CLI tests that hang because this checkout path contains a space); they are identical with the branch stashed and none are touched here. CI is the clean signal.ReDoS — the unquoted branch cannot fail after the drive letter, so there is nothing to backtrack into. Measured across five adversarial shapes (repeated spaces before capitals, repeated dotted tokens, backslash bait, an unclosed quote, an extension followed by prose): flat and sub-millisecond out to 40 KB.
Checklist
pnpm run check)package.json(maintainers will handle this).pnpm changeset)