From 32efbc4c90b82a317ec1392fd5095421591634c3 Mon Sep 17 00:00:00 2001
From: Harlan Zhou <zhouhua258@outlook.com>
Date: Mon, 14 Sep 2026 12:22:33 +0000
Subject: [PATCH] fix: allow rm -rf on nested paths while still blocking
 top-level and wildcard wipes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#18's fallback 'Remove arbitrary absolute path' rule blocked EVERY
absolute-path rm -rf, including /root/myapp/build or /tmp/build —
this tool's normal daily use. Worse, it trained callers to bypass the
validator entirely via 'cd /root && rm -rf subdir'.

Rework the hardening rules:
- block rm -rf targeting any TOP-LEVEL absolute path (any argument,
  optional trailing slash): rm -rf /, /home, /etc/, multi-arg variants
- block wildcard wipes (rm -rf /home/*, /*) and critical system files
  (/etc/shadow, /etc/passwd, /etc/sudoers, /etc/fstab, /etc/group,
  /etc/gshadow, /boot/*)
- ALLOW nested paths (depth >= 2): rm -rf /root/otherdirectory,
  /tmp/build, /home/user/project, /etc/nginx

Also fix the keyword-layer 'rm -rf /root\b' pattern whose word boundary
between 't' and '/' made it match /root/child too.

All issue #17 payloads remain blocked (verified in tests).
---
 src/remoteshell_mcp/command_validator.py    | 28 +++++++++++++++------
 tests/test_command_validator_integration.py | 19 ++++++++++++++
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/src/remoteshell_mcp/command_validator.py b/src/remoteshell_mcp/command_validator.py
index 5784ac8..a210dd9 100644
--- a/src/remoteshell_mcp/command_validator.py
+++ b/src/remoteshell_mcp/command_validator.py
@@ -65,12 +65,24 @@ class CommandValidator:
         (r'\brm\s+.*-.*rf\s+\*', 'Remove all files in current directory'),
         (r'\brm\s+.*-.*rf\s+\.\.(?:\s|$|/)', 'Remove parent directory'),
 
-        # Hardened (issue #17): absolute-path `rm -rf` under common user/system
-        # trees that the 11-path list missed, and destructive ops on individual files.
-        (r'\brm\s+.*-.*rf\s+(?:/home|/tmp|/mnt|/data|/opt|/srv|/media)(?:\s|$|/)',
-         'Remove absolute path under /home /tmp /mnt /data /opt /srv /media'),
-        (r'\brm\s+.*-.*rf\s+/\S+(?:/|(?:\s|$))',
-         'Remove arbitrary absolute path (individual file/dir)'),
+        # Hardened (issue #17): block rm -rf aimed at any TOP-LEVEL absolute
+        # path (any argument, optional trailing slash). Nested paths (e.g.
+        # /root/myapp/build, /tmp/build) are intentionally ALLOWED: deleting
+        # specific user-created directories is this tool's normal use case,
+        # and blocking them just trains callers to bypass the validator with
+        # `cd /root && rm -rf subdir`.
+        #   (?:\S+\s+)*?  skips earlier arguments so every argument is checked
+        #   /[^/\s]+      exactly one path segment (no inner slash = top level)
+        #   (?:/\s*(?:$|\s)|\s|$)  segment ends the argument (trailing "/" ok)
+        (r'\brm\s+.*-.*rf\s+(?:\S+\s+)*?/[^/\s]+(?:/\s*(?:$|\s)|\s|$)',
+         'Remove top-level directory'),
+        (r'\brm\s+.*-.*rf\s+/\S*\*',
+         'Remove wildcard path'),
+        # Deleting critical system files / credentials (destroys the host's
+        # security state, not just user data). Regular nested paths like
+        # /etc/nginx or /var/log/app remain allowed.
+        (r'\brm\s+.*-.*rf\s+/(?:etc/(?:shadow|passwd|sudoers|group|gshadow|fstab)|boot/[^/\s]+)',
+         'Remove critical system file'),
         # File-exfil / overwrite primitives that return secrets to the caller.
         (r'\b(?:cat|cp|mv|tar|dd|scp|nc|rsync)\s+.*(?:/etc/shadow|/etc/passwd|/etc/sudoers|/root/\.|/home/.*?/\.ssh)',
          'Exfiltrate or disclose sensitive system/private files'),
@@ -117,7 +129,9 @@ def validate(cls, command: str) -> None:
             (r'\brm\s+-rf\s+/\s*$', 'rm -rf /'),
             (r'\brm\s+-rf\s+/\s+', 'rm -rf /'),
             (r'\brm\s+-rf\s+/\*', 'rm -rf /*'),
-            (r'\brm\s+-rf\s+/root\b', 'rm -rf /root'),
+            # Only the directory itself — must NOT match /root/child (word
+            # boundary between 't' and '/' made the old pattern do that).
+            (r'\brm\s+-rf\s+/root(?:\s|$)', 'rm -rf /root'),
             (r'\bdd\s+.*if=/dev/zero\b', 'dd if=/dev/zero'),
         ]
         
diff --git a/tests/test_command_validator_integration.py b/tests/test_command_validator_integration.py
index b5a04b8..d678e66 100644
--- a/tests/test_command_validator_integration.py
+++ b/tests/test_command_validator_integration.py
@@ -22,6 +22,15 @@ def test_command_validator_blocks_dangerous_commands(self):
             "echo x > /dev/sda",
             "echo x >/dev/nvme0n1",
             "echo x > /dev/mapper/vg-root",
+            "rm -rf /home",
+            "rm -rf /tmp",
+            "rm -rf /home/*",
+            "rm -rf /root",
+            "rm -rf /root/ /etc/",
+            "rm -rf /*",
+            "rm -rf /etc/passwd",
+            "rm -rf /etc/sudoers",
+            "rm -rf /boot/vmlinuz",
             "halt",
             "shutdown -h now",
             ":(){ :|:& };:",
@@ -47,6 +56,16 @@ def test_command_validator_allows_safe_commands(self):
             "cmd > /dev/null 2>&1",
             "cat /etc/hostname > /dev/full",
             "echo x 1>/dev/stdout 2>/dev/stderr",
+            "rm -rf /root/otherdirectory",
+            "rm -rf /tmp/build",
+            "rm -rf /home/user/project",
+            "rm -rf /var/log/app/",
+            "rm -rf /opt/mytool/cache",
+            "rm -rf /var/backups/",
+            "rm -rf /opt/mytool",
+            "rm -rf /etc/nginx",
+            "rm -rf /usr/share/doc",
+            "rm -rf ./build",
         ]
 
         for command in safe_commands: