fix(disk-hygiene): restore Windows readonly allowlist; ask in engine-gate - #2786
Conversation
…gate Move MSYS path mapping ahead of is_absolute(), strip .exe basenames, downgrade engine-gate readonly decisions to ask, exclude LocalAppData trust roots, and add a Windows GuardTests CI lane. Closes #2774 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 2m 27s —— View job Security review —
|
|
Last security-reviewed head: |
…etry Quote or prefer MSYS heads when which() returns a Program Files path, and use a .cmd Python sink on NT so HOOK_TELEMETRY_SINK can exec under CreateProcess. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 2m 53s —— View job Code review —
|
Use a .cmd→.py sink and wait for non-empty content so open('w') cannot
race the JSON parse on the fire-and-forget HOOK_TELEMETRY_SINK path.
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @cursor[bot]'s task in 4m 45s —— View job Code review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| "ProgramFiles": str(base / "empty-pf"), | ||
| "ProgramFiles(x86)": str(base / "empty-pf86"), | ||
| "LocalAppData": str(base / "empty-local"), | ||
| "LocalAppData": str(base / "Programs" / "Git"), | ||
| "SystemRoot": "", | ||
| "SYSTEMROOT": "", | ||
| }, | ||
| clear=False, | ||
| ), | ||
| ): | ||
| # Even with a user-writable LocalAppData\\Programs\\Git tree |
There was a problem hiding this comment.
Minor: this fixture doesn't actually verify the LocalAppData exclusion it claims to. It sets LocalAppData = base/"Programs"/"Git" and creates that exact directory, but the (now-removed) candidate tuple was ("LocalAppData", ("Programs", "Git")), i.e. the code would have joined "Programs"/"Git" onto the env value — so the path it would have probed is base/Programs/Git/Programs/Git, not the base/Programs/Git planted here. Even if that candidate were ever accidentally reintroduced, this particular fixture wouldn't catch it (the directory the reintroduced code would check still wouldn't exist).
test_nt_git_roots_exclude_user_writable_localappdata right below (lines 4569-4591) does align the planted directory correctly (LocalAppData = base/"Local", planted at base/"Local"/"Programs"/"Git") and is the one that actually guards this regression — so this isn't a coverage gap, just a misleading comment/assertion pairing here that could confuse a future reader into thinking this test proves something it doesn't.
2 participants
Summary
is_absolute(), strip.exe/.EXEbasenames.allowtoaskso consumer sessions keep the permission prompt (disk-hygiene: read-only Bash allowlist is inert on Windows (0 commands accepted); engine-gate hard-allow leak on Linux #2774 C).%LOCALAPPDATA%\\Programs\\Gitfrom NT trust roots; reword trust comments (disk-hygiene: read-only Bash allowlist is inert on Windows (0 commands accepted); engine-gate hard-allow leak on Linux #2774 D).disk-hygiene-guard-windowsCI lane (windows-2025, GuardTests only) (disk-hygiene: read-only Bash allowlist is inert on Windows (0 commands accepted); engine-gate hard-allow leak on Linux #2774 B).Test plan
python -m unittest test_hygiene.GuardTests(126 tests OK on Linux)Closes #2774
Related