disk-hygiene: read-only Bash allowlist is inert on Windows (0 commands accepted); engine-gate hard-allow leak on Linux

[kyle-sexton]

PR #2706 (merged as de26c27b, disk-hygiene 0.20.7) re-landed the read-only Bash allowlist and hardened its trust anchor. Both hardenings genuinely hold. But independent post-merge verification found the allowlist is completely inert on Windows — 0 commands accepted, by any spelling — so #2591's user-facing purpose is undelivered on the platform this plugin is built around, and the original #2618 complaint (the belt denying read-only inspection for the rest of every session once armed) still stands there.

A separate defect leaks a hard allow through the plugin-level engine gate on Linux.

Found by a fresh-context verifier that executed the guard rather than reading it. The central claim is independently reconfirmed below.

A. The allowlist accepts nothing on Windows — two compounding defects

A1 — the MSYS reinterpretation branch is unreachable dead code.

_trusted_system_readonly_head gates on if not candidate.is_absolute(): return False before the os.name == "nt" and head.startswith("/") MSYS-mapping branch. On Windows, pathlib requires both a drive and a root for is_absolute(), so a POSIX-style head never reaches the mapping.

Reconfirmed directly on the hook runtime's own interpreter (uv CPython 3.14.7):

os.name: nt
'/usr/bin/ls'                          is_absolute=False
'/usr/bin/find'                        is_absolute=False
'C:/Program Files/Git/usr/bin/ls.EXE'  is_absolute=True

A2 — extensions are never stripped. _readonly_supporting_basename does not remove .exe/.EXE, so real Windows binaries (ls.EXE, find.exe) never match _READONLY_SUPPORTING_BASH_HEADS. Native paths such as C:/Program Files/Git/usr/bin/ls.EXE additionally split on the space in "Program Files".

Measured result: brute-forcing every file across all five resolved trusted roots yielded 0 accepted commands. None of these work: ls -la, /usr/bin/ls -la, /usr/bin/find . -name x, /usr/bin/[ -d . ], C:/Program Files/Git/usr/bin/ls.EXE /tmp.

Scope: applies when the hook's interpreter is Windows-native Python (os.name == "nt") — the common case, and what run-python-hook.sh resolves on a normal Windows box. An MSYS/Cygwin Python would take the posix path instead.

B. The PR's own test encodes the broken expectation, and CI structurally cannot catch it

test_readonly_supporting_bare_name_is_denied_as_shadowable (test_hygiene.py:4372) asserts the absolute resolved system ls is allowed. It returns False on Windows, so the test fails there.

CI runs ubuntu-24.04 for every lane except hook-utils-windows, so the NT trust machinery — _nt_known_git_installation_roots, the MSYS mapping, the %SystemRoot% logic — is never exercised on NT. That structural gap is why A shipped green. A Windows GuardTests lane is the durable fix; without it the next NT-specific change has the same blind spot.

C. Engine-gate hard-allow leak (live on Linux, masked on Windows by A)

is_exact_readonly_supporting_command sits in _decide after the engine-gate early return, so an allowlisted command that also names the engine path passes _engine_gate_relevant. Confirmed end-to-end: in --mode engine-gate with trust satisfied, /usr/bin/ls -la <engine> and /usr/bin/find <engine> -name x both emit permissionDecision: "allow". Before this change they were denied.

The plugin-level gate runs in every consumer session, including sessions that never invoke clean, and a hard allow bypasses the user's own permission prompt. That contradicts the plugin's own stated doctrine that a plugin-level hook must never tax unrelated work — and ask would preserve the prompt while keeping the ergonomic win.

D. Trust roots remain environment-derived

_nt_known_git_installation_roots reads ProgramFiles, ProgramFiles(x86), and LocalAppData. Pointing ProgramFiles at an attacker-controlled directory makes <dir>/Git/usr/bin/find allowed.

Environment-variable control is a far stronger precondition than the PATH bug that #2706 fixed, so this is a weakness rather than a working repo-planted bypass. But the code comment's claim of "independently trusted install locations" is not accurate, and %LOCALAPPDATA%\Programs\Git is a user-writable path that contributes trusted roots whenever it exists.

What #2706 got right — not in question

• Trust anchor is genuinely anchored, not substring-matched. The attack path …/anyrepo/git/usr/bin/find was built on disk and run against both revisions: old → trusted, new → denied. Sibling-prefix Gitx/usr/bin also denied. _TRUSTED_READONLY_BIN_SUBSTRINGS_NT no longer exists.
• [ is no longer trusted on name alone — merged code goes further than asked and denies bare [ … ] entirely; only the absolute form can qualify.
• Fail-closed proven by execution, not inference. Guard run as a subprocess with real stdin: malformed JSON → deny; missing tool_input → deny; non-string command → deny; empty stdin → deny; rm -rf / → deny. With all anchor env vars unset, roots == () and every shape denied; no exception escaped.
• All nine find side-effect primaries denied (-delete -exec -execdir -ok -okdir -fprint -fprint0 -fprintf -fls, plus casefolded -DELETE), while read-only -printf/-newer are allowed. Operators, redirections, expansions, and relative paths all denied.
• Scope was clean — exactly four files; SKILL.md and safety-model.md belonged to sibling fix(disk-hygiene): re-land tidiness-first reporting and correct the belt's documented posture #2714 and were untouched.

Suggested fix, cheapest first

1. Move the MSYS reinterpretation ahead of the is_absolute() gate (A1), and strip executable extensions in _readonly_supporting_basename (A2). Together these are what make the feature work at all on Windows.
2. Correct test_readonly_supporting_bare_name_is_denied_as_shadowable so it encodes the intended behavior on both platforms.
3. Downgrade the engine-gate path from allow to ask (C), preserving the user's permission prompt in sessions that never invoked clean.
4. Add a Windows GuardTests CI lane (B). Without it, items 1-3 can regress silently exactly as this did.
5. Reword or harden the trust-root derivation comment (D) so it does not claim independence from untrusted input, and consider excluding the user-writable %LOCALAPPDATA% root.

Verification honesty notes

• Everything above ran on Windows only. Linux behavior for the NT-specific paths is inferred from code, not executed — except finding C, which was exercised.
• pytest is not installed on the verifying host; unittest was used. Worktree revision: 293 tests, 3 failures, 4 skipped — all failures in HygieneTests, untouched by this diff, so pre-existing or environmental. Merged revision GuardTests: 122 tests, 2 failures — one is defect B above; the other (test_deny_emits_blocked_telemetry_when_sink_wired) is believed environmental but was not isolated.
• The Path.is_absolute() result underpinning A1 was independently reconfirmed on the hook runtime's own interpreter before filing.

Refs #2618, #2591, #2706.