Skip to content

fix(classify): Remove-Item Users overmatch - deep per-user paths are not a system root#70

Merged
askalf merged 1 commit into
masterfrom
fix/remove-item-users-overmatch
Jul 16, 2026
Merged

fix(classify): Remove-Item Users overmatch - deep per-user paths are not a system root#70
askalf merged 1 commit into
masterfrom
fix/remove-item-users-overmatch

Conversation

@askalf

@askalf askalf commented Jul 16, 2026

Copy link
Copy Markdown
Owner

fix(classify): Remove-Item Users overmatch — deep per-user paths are not a system root

The Windows recursive-force-delete black rule matched Users\b, which
made EVERY path under any user profile a "drive/system root" — a deep
project dir (...\Desktop\proj_worktree) black-blocked the same as C:.
That contradicts the rule's own comment (drive-root / system targets
only) and the documented Remove-Item node_modules stays clean stance.

The Users branch now blacks only C:\Users itself or a WHOLE profile
(C:\Users<name> - one segment, then end-of-target, separator-smuggle
still caught). Windows/Program branches unchanged.

Companion RED rule: any Remove-Item -Recurse -Force of a drive-anchored
absolute path is now red (approve) rather than invisible - relative
deletes like node_modules stay allow, preserving the no-FP guarantee.

Tests: +6 black cases (Users root, whole profile quoted/unquoted,
separator smuggle, System32) and a new red-not-black test for deep
per-user and absolute non-system paths. 200/200 pass.

…not a system root

The Windows recursive-force-delete black rule matched `Users\b`, which
made EVERY path under any user profile a "drive/system root" — a deep
project dir (...\Desktop\proj\_worktree) black-blocked the same as C:\.
That contradicts the rule's own comment (drive-root / system targets
only) and the documented `Remove-Item node_modules stays clean` stance.

The Users branch now blacks only C:\Users itself or a WHOLE profile
(C:\Users\<name> - one segment, then end-of-target, separator-smuggle
still caught). Windows/Program branches unchanged.

Companion RED rule: any Remove-Item -Recurse -Force of a drive-anchored
absolute path is now red (approve) rather than invisible - relative
deletes like node_modules stay allow, preserving the no-FP guarantee.

Tests: +6 black cases (Users root, whole profile quoted/unquoted,
separator smuggle, System32) and a new red-not-black test for deep
per-user and absolute non-system paths. 200/200 pass.
@askalf
askalf enabled auto-merge (squash) July 16, 2026 21:42

@sprayberry-reviewer sprayberry-reviewer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Verdict: no blocking findings — approving. Two non-blocking observations inline.


What I checked

  • src/classify.mjs — new BLACK regex for the Users branch, new RED_SHELL rule for all drive-anchored Remove-Item, WindowsWindows\b tightening.
  • test/attacks.test.mjs — 6 new block assertions + 2 new approve assertions (deep per-user and absolute non-system paths).
  • Ran all regex permutations locally: C:\Users (BLACK), C:\Users\bob (BLACK), "C:\Users\bob" (BLACK), C:\Users\bob; echo done (BLACK), C:\Windows\System32\config (BLACK), node_modules (allow), .\\node_modules (allow), C:\code\repo\node_modules (RED/approve), deep path C:\Users\bob\Desktop\proj\_worktree (RED/approve). All matched stated expectations.

Non-blocking observations

1. ..-traversal through Users goes BLACK → RED (no test covers it)

The old regex matched Users\b, which caught C:\Users\bob\..\Windows\System32 because the \b fires between s and \. The new Users branch:

src/classify.mjs:167
Users(?:\\[^\\"';|&]+)?\\?["']?\s*(?:$|[;|&])

The [^\\"';|&]+ character class excludes \, so it captures exactly one non-backslash segment (bob), then requires end-of-command ($ / ; / | / &). For C:\Users\bob\..\Windows\System32 the next char after \bob\ is ., not an end-of-command character, so the Users branch fails. The Windows\b branch also fails (text immediately after C:\ is Users, not Windows). The command falls through to the new RED_SHELL rule, which catches it (approve, not block).

Remove-Item -Recurse -Force C:\Users\bob\..\Windows\System32 is a realistic payload — PowerShell resolves .. at the filesystem level. It was previously AUTO-BLOCKED; it is now APPROVE (human review required). The RED rule is a sound safety net here, but there is no test for this specific case, and the PR description says "separator-smuggle still caught" — it is, but only at RED level, not BLACK.

Suggested addition (non-blocking, for documentation parity):

// In 'more destructive blocked' or a new test:
// Traversal through a profile to a system dir is approve, not block
approve({ tool: 'powershell', input: { command: 'Remove-Item -Recurse -Force C:\\Users\\bob\\..\\Windows\\System32' } });

This won't change behaviour (it already approves), but it makes the trade-off explicit and regression-proof.


2. Quiet WindowsWindows\b tightening

src/classify.mjs:167  (old) …Windows|Program|…
src/classify.mjs:167  (new) …Windows\b|Program|…

Adding \b narrows the Windows branch: C:\WindowsApps\… (MS Store apps) was previously BLACK; it is now only RED. C:\Windows.old is fine — . is a non-word character so \b still fires. The change is arguably beneficial (fewer FPs against MS Store dirs) and the paths still hit RED, but it was not called out in the PR description or tested. Worth a brief comment or a // WindowsApps intentionally excluded — not a system root in the rule.


What's good

The Users-branch regex is carefully constructed: the single-segment constraint ([^\\"';|&]+ stopping at backslash), optional trailing \, optional quote, and (?:$|[;|&]) end-anchor together correctly implement the "whole profile only" contract. The semicolon-smuggle test (C:\Users\bob; echo done → BLACK) verifies the critical exploit path. The RED_SHELL companion uses [^;\n] delimiters (not [^|]) so it doesn't need to cross pipeline stages to catch absolute-path removals. CI 200/200, coverage of all explicitly claimed cases is solid.

@askalf
askalf merged commit 4960982 into master Jul 16, 2026
7 checks passed
@askalf
askalf deleted the fix/remove-item-users-overmatch branch July 16, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants