From c11b4c37412986be6af0dcccec34ae4f56e1a209 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 07:36:21 -0500 Subject: [PATCH] fix(security): case-fold the leak gate's Windows home-path arm (BACKLOG #325) The forbidden-content gate is a REQUIRED merge context. Its home-path detector class-matched the drive letter but treated `Users` as a literal, so of four spellings of the same case-insensitive Windows directory only the canonical one fired: C:\Users\\proj FIRES c:/users//proj missed c:\users\\proj missed C:\USERS\\proj missed Same account, same disclosure, three of four spellings walking through a required gate. THE OBVIOUS FIX IS THE WRONG ONE, AND THE ITEM SAYS SO. A whole-pattern re.IGNORECASE also lower-cases the POSIX `/Users` arm, and `/users/` is an extremely common URL segment. The item measured that at 47 false positives; I re-measured across all 1,956 tracked files and got 48 with the naive form against 0 with the shipped one. (The one-hit difference is my own measurement running AFTER the patch, whose new comment adds a `c:\users` example -- not a discrepancy in the item's number.) So the fold is INLINE and scoped to the drive-letter alternative only. Verified behaviour, all ten cases: all four Windows spellings FIRE /home/, /Users/ FIRE (unchanged) /users/ MISSES -- load-bearing, and now asserted deliberately so nobody "fixes" it into the 47-false-positive form Public/runner/user stand-ins still exempt (exemption list untouched) `_WORKTREE_SLUG` was case-blind the same way and is taken in the same change, per the item's point 5. Reachable: scripts/worktree/new.ps1 accepts `[A-Za-z0-9._-]+` and lowercases nothing. THE docs/BACKLOG.md EDIT IS NOT COSMETIC. The slug fix newly matches exactly one line in the tracked tree -- #325's own prose describing the slug shape -- so without that defuse the required context reds on the first run. Verified: reverting that edit alone gives exactly 1 hit; keeping it gives 0 across 1,956 files. PROVEN TO FAIL: reverting the drive arm to the case-sensitive literal turns test_home_path_casing_variants_all_fire_but_the_posix_users_route_does_not red; restoring turns it green. Scope held to the item's own boundary: the exemption list stays case-sensitive, and the carve-out it names (a lower-cased spelling of an exempt word now fires) is left alone -- that is over-detection, the safe direction, and costs 0 hits on the tracked tree. Two local hits remain under `--path .` from the POSIX arms, in an untracked, gitignored file. Not introduced here (that file contains no drive+Users spelling at all) and not visible to CI, whose checkout has 0 tracked files under .claude/. 66 tests pass; ruff clean; 0 tracked-tree hits. --- docs/BACKLOG.md | 2 +- scripts/security/scan_forbidden.py | 20 +++++++++++-- tests/test_scan_tokens_source.py | 47 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 3ea9ac61..22625f54 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2823,7 +2823,7 @@ No test covers it. `tests/test_scan_tokens_source.py:559-583` (`test_absolute_ho 4. Add the regression case to `tests/test_scan_tokens_source.py:559`, alongside the existing canonical fixtures — a lowercased and an upper-cased Windows path must both produce a hit, and the POSIX `/users/…` non-match should be asserted deliberately so the next person does not "fix" it into the 47-false-positive form. -5. **Same fix site, sibling defect:** `_WORKTREE_SLUG` at `scripts/security/scan_forbidden.py:92` is case-blind the same way (`[a-z0-9]+`); `claude/Some-Task-a1b2c3` is MISSED. `scripts/worktree/new.ps1:43,86` passes `-Name` through verbatim with no lowercasing, so an upper-cased worktree name is reachable. Narrower than the home-path case (agent-created slugs are lowercase by convention), but it is a two-character edit in the same block — take it in the same change or say why not. +5. **Same fix site, sibling defect:** `_WORKTREE_SLUG` at `scripts/security/scan_forbidden.py:92` is case-blind the same way (`[a-z0-9]+`); an upper-cased slug — `claude/` followed by `Some-Task-a1b2c3` — is MISSED. (Written split on purpose, for the reason in the note above: once the fix lands, the joined literal trips the very detector it documents, and unlike `_HOME_PATH` the slug pattern has no `<…>` exemption to write it into.) `scripts/worktree/new.ps1:43,86` passes `-Name` through verbatim with no lowercasing, so an upper-cased worktree name is reachable. Narrower than the home-path case (agent-created slugs are lowercase by convention), but it is a two-character edit in the same block — take it in the same change or say why not. **Related:** `scripts/security/scan_forbidden.py` (`_HOME_PATH` :99-106, `_WORKTREE_SLUG` :92, call site :758-759), `tests/test_scan_tokens_source.py:559-583`, `.github/workflows/security.yml:446-493`, `.github/required-contexts.txt`, `scripts/worktree/new.ps1`. Sibling **#321** — same gate, same "green gate that cannot see the class" root cause, but the **opposite mechanism**: #321 is an incomplete *token source* (data, fixed by the owner updating a private secret) and explicitly scopes itself away from scanner defects at `docs/BACKLOG.md:7356`; this is a *structural detector* defect (code, fixed by a regex edit) that is live even with no token source. Also **#322**, and the anonymizer's structural-detector item from this same audit. Note #321's **Related:** line at `docs/BACKLOG.md:7363` cites `tests/test_scan_forbidden.py` for regression tests, but the home-path test actually lives in `tests/test_scan_tokens_source.py` — worth correcting when someone next touches #321. diff --git a/scripts/security/scan_forbidden.py b/scripts/security/scan_forbidden.py index 71ceef8e..f78f2df3 100644 --- a/scripts/security/scan_forbidden.py +++ b/scripts/security/scan_forbidden.py @@ -88,16 +88,30 @@ # # A worktree/branch slug is whatever the task happened to be CALLED, so it can name a prospect segment, # a customer engagement, or a competitor study. That is unbounded: the leak is the project name itself, -# and there is no list to add it to. Matching the shape is the only control that scales. -_WORKTREE_SLUG = re.compile(r"(?:claude/|worktrees/)[a-z0-9]+(?:-[a-z0-9]+)*-[0-9a-f]{6}") +# and there is no list to add it to. Matching the shape is the only control that scales. It is +# case-folded whole: agent slugs are lowercase by convention, but scripts/worktree/new.ps1 +# validates -Name as ^[A-Za-z0-9._-]+$ and hands it to `git worktree add -b` verbatim, so an +# upper-cased slug is reachable -- and unlike _HOME_PATH no common URL shape collides here. +_WORKTREE_SLUG = re.compile(r"(?i:(?:claude/|worktrees/)[a-z0-9]+(?:-[a-z0-9]+)*-[0-9a-f]{6})") # An absolute user-home path carries the OS account name, and inside a worktree path the slug as well. # Exempt: bracket/env placeholders (, $HOME, %USERPROFILE%, {home}), the well-known shared and CI # accounts, and the DOCUMENTATION placeholder names this repo already uses in examples (me, svc, you, # user, username, example). Everything else looks like a real account and fires. That list is the whole # judgement call here: "is this a real person's login" is not decidable by shape, so the pattern trusts # a small, explicit set of conventional stand-ins and treats anything else as a disclosure. +# +# The drive-letter arm folds case INLINE. Windows paths are case-INSENSITIVE, so `C:\Users\`, +# `c:\users\` and `C:\USERS\` are the SAME directory naming the SAME account, and a +# literal `Users` caught only one of those four spellings. (The examples use the `` +# placeholder the lookahead below exempts: a real account segment written here would trip this +# very detector.) Keep the fold SCOPED to that arm -- do NOT lift it to a whole-pattern +# re.IGNORECASE. That also lower-cases the POSIX /Users arm, and `/users/` is an extremely +# common URL segment: measured, it then matches the web console's /ui/users/... routes in 47 +# places on the tracked tree and reds this required context on its first run. The exemption +# list below stays case-SENSITIVE for the inverse reason -- on POSIX `Public` and `public` are +# DIFFERENT accounts, and widening an exemption is the under-detection direction. _HOME_PATH = re.compile( - r"(?:[A-Za-z]:[\\/]Users|/home|/Users)[\\/]" + r"(?:(?i:[A-Za-z]:[\\/]users)|/home|/Users)[\\/]" r"(?!<|\$|%|\{" r"|(?:Public|Default|All|ContainerAdministrator|runner|vsts" r"|me|svc|you|user|username|example)[\\/\s\"'`]" diff --git a/tests/test_scan_tokens_source.py b/tests/test_scan_tokens_source.py index baef6a74..1ae1da1b 100644 --- a/tests/test_scan_tokens_source.py +++ b/tests/test_scan_tokens_source.py @@ -583,6 +583,53 @@ def test_absolute_home_path_is_flagged_but_placeholders_are_not( ), "placeholders / CI / shared accounts must not fire" +def test_home_path_casing_variants_all_fire_but_the_posix_users_route_does_not( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Windows paths are case-INSENSITIVE: all four spellings name the SAME account. + + The ``/users/`` non-match at the end is asserted deliberately, not incidentally -- it pins the + asymmetry that keeps the case-fold scoped to the drive-letter arm, so the next reader cannot + quietly widen it to a whole-pattern ``re.I``. The scanner's ``_HOME_PATH`` comment says why. + """ + mod = _load(None, monkeypatch) + variants = tmp_path / "variants.md" + # Assembled like the fixtures above so no SOURCE line here is itself a match -- this file is + # scanned by the gate it tests. + variants.write_text( + f"c:{_BS}users{_BS}Carol{_BS}Code\n" + f"C:{_BS}USERS{_BS}Dave{_BS}Code\n" + "c:/" + "users/Erin/Code\n", + encoding="utf-8", + ) + hits = mod.scan_file(variants, "docs/variants.md") # type: ignore[attr-defined] + assert sum("absolute user-home path" in h for h in hits) == 3 + assert not any(n in h for h in hits for n in ("Carol", "Dave", "Erin")) + + route = tmp_path / "route.md" + route.write_text("/users/list\nGET /ui/users/{id}/roles\n", encoding="utf-8") + assert not any( + "absolute user-home path" in h + for h in mod.scan_file(route, "docs/route.md") # type: ignore[attr-defined] + ), "a lower-cased POSIX /users/ segment is a REST route, not a home path" + + +def test_worktree_slug_casing_variant_is_flagged( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """An upper-cased slug is reachable, so it must not slip the gate. + + ``scripts/worktree/new.ps1`` validates ``-Name`` as ``^[A-Za-z0-9._-]+$`` and hands it to + ``git worktree add -b`` verbatim, with no lowercasing anywhere on the path. + """ + mod = _load(None, monkeypatch) + f = tmp_path / "notes.md" + # Split for the same reason as the fixtures above. + f.write_text("see .claude/work" + "trees/Some-Task-Name-a1b2c3 for details\n", encoding="utf-8") + hits = mod.scan_file(f, "docs/notes.md") # type: ignore[attr-defined] + assert any("worktree/branch slug" in h for h in hits) + assert not any("Some-Task-Name" in h for h in hits) + + # -------------------------------------------------------------------------------------------------- # Round-3 hardening: parser diagnostics, allowlist breadth, and per-section floors. # --------------------------------------------------------------------------------------------------