fix(approval): tighten 'delete in root path' regex to system roots only - #18533
Closed
williams145 wants to merge 1 commit into
Closed
williams145 wants to merge 1 commit into
williams145 wants to merge 1 commit into
Conversation
The pattern at tools/approval.py:215 — `\brm\s+(-[^\s]*\s+)*/` — fired
on any `rm` whose target started with `/`, treating every absolute path
as if it were aimed at the system root. Routine cleanups like
`rm /home/user/foo`, `rm /tmp/x`, and `rm -rf /mnt/c/Users/u/foo` were
all flagged "delete in root path" and required human approval, creating
significant friction in the agent's own working-directory operations.
Tighten the regex to fire only when the target is one of:
- bare `/`, `/*`, or `/.*`
- a top-level system directory (bin, boot, dev, etc, lib, lib64, opt,
proc, root, run, sbin, srv, sys, usr, var) followed by a boundary
(`/`, whitespace, or end-of-string)
The boundary requirement also prevents false matches on look-alike
paths such as `/etc.bak/...`, `/var-something/...`, `/opting/...`.
Genuinely-dangerous root deletes are still caught. Recursive deletes
of user-space paths continue to be detected by the separate "recursive
delete" pattern (line 216), so commands like `rm -rf /home/user/.cache`
remain flagged — they just don't get the misleading "root path" label.
Add TestRmAbsolutePathNotFalseFlagged in tests/tools/test_approval.py
covering the real-world false positive from the issue plus the system-
root cases that must remain flagged.
Fixes NousResearch#18083
Contributor
Author
|
Thanks @alt-glitch — confirmed, closing in favor of #18089 by @briandevans. His version is more thorough (adds the legacy-key alias table and the divergence comment). Will redirect attention to the other two open PRs (#12584, #12592) which address different issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
tools/approval.py:215definesDANGEROUS_PATTERNS[0]as:Because
/only requires a single forward-slash to match, every absolute-pathrmis flagged "delete in root path" — including routine cleanups in the user's home,/tmp,/mnt, etc.Real example from the issue report:
That command deletes one file in the user's home directory — nowhere near the system root.
Fix
Replace the over-broad pattern with one that only fires on actual system-root targets:
( r'\brm\s+(-[^\s]*\s+)*/(\s|\*|$|\.\*|' r'(bin|boot|dev|etc|lib|lib64|opt|proc|root|run|sbin|srv|sys|usr|var)(/|\s|$))', "delete in root path", ),This matches:
/, root globs/*,/.*/bin,/boot,/dev,/etc,/lib,/lib64,/opt,/proc,/root,/run,/sbin,/srv,/sys,/usr,/var) followed by/, whitespace, or end of stringThe boundary requirement prevents false matches on look-alike paths (
/etc.bak/...,/var-something/...,/opting/...).Verification
rm -rf /rm -rf /*rm -rf /.*rm -rf /etc/passwdrm -rf /var/logrm /usr/bin/somethingrm /home/user/foorm -f /home/scott/hermes-home/x.htmlrm /tmp/xrm -rf /mnt/c/Users/u/foorm -rf /home/user/.cacherm /etc.bak/file(look-alike)Recursive deletes on user paths (e.g.
rm -rf /home/user) continue to be flagged correctly via the separate "recursive delete" pattern — they just lose the misleading "root path" label.Tests
New
TestRmAbsolutePathNotFalseFlaggedclass intests/tools/test_approval.pycovers:/tmp,/mnt/c/...user-space paths/etc.baklook-alike path/,/*,/.*,/etc/passwd,/var/log,/usr/bin/...,/etc,/lib64/...still flaggedExisting
TestDetectDangerousRmandTestRmRecursiveFlagVariantscontinue to pass — recursive-flag commands fall through to the "recursive delete" pattern, which still satisfies their"delete" in desc.lower()assertions. Full suite: 145 passed.Fixes #18083
Note: same author has two other open PRs ready for review — #12584 (fixes #6133) and #12592 (fixes #5861). Both apply cleanly to current main.