From a219f2e80e42c14d5d59a3e6816fcb91b771e4a9 Mon Sep 17 00:00:00 2001
From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com>
Date: Thu, 16 Oct 2025 09:02:26 +0200
Subject: [PATCH] fix: use canonical parser for Windows path validation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem:
- validateVolumeStringForInjection used explode(':') to parse volume strings
- This incorrectly splits Windows paths like "C:\host\path:/container" at the drive letter colon
- Could lead to false positives/negatives in injection detection

Solution:
- Replace custom parsing in validateVolumeStringForInjection with call to parseDockerVolumeString
- parseDockerVolumeString already handles Windows paths, environment variables, and performs validation
- Eliminates code duplication and uses single source of truth for volume string parsing

Tests:
- All 77 existing security tests pass (211 assertions)
- Added 6 new Windows path tests (8 assertions)
- Fixed pre-existing test bug: preg_match returns int 1, not boolean true

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

Co-Authored-By: Claude <noreply@anthropic.com>
---
 bootstrap/helpers/parsers.php                | 49 +--------------
 tests/Unit/VolumeArrayFormatSecurityTest.php |  2 +-
 tests/Unit/WindowsPathVolumeTest.php         | 64 ++++++++++++++++++++
 3 files changed, 67 insertions(+), 48 deletions(-)
 create mode 100644 tests/Unit/WindowsPathVolumeTest.php

diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php
index 7cc084530..f3d71c83d 100644
--- a/bootstrap/helpers/parsers.php
+++ b/bootstrap/helpers/parsers.php
@@ -104,53 +104,8 @@ function validateDockerComposeForInjection(string $composeYaml): void
  */
 function validateVolumeStringForInjection(string $volumeString): void
 {
-    // Parse the volume string to extract source and target
-    $parts = explode(':', $volumeString);
-    if (count($parts) < 2) {
-        // Named volume without target - only validate the name
-        try {
-            validateShellSafePath($parts[0], 'volume name');
-        } catch (\Exception $e) {
-            throw new \Exception(
-                'Invalid Docker volume definition: '.$e->getMessage().
-                ' Please use safe names without shell metacharacters.',
-                0,
-                $e
-            );
-        }
-
-        return;
-    }
-
-    $source = $parts[0];
-    $target = $parts[1];
-
-    // Validate source (but allow simple environment variables)
-    $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source);
-    if (! $isSimpleEnvVar) {
-        try {
-            validateShellSafePath($source, 'volume source');
-        } catch (\Exception $e) {
-            throw new \Exception(
-                'Invalid Docker volume definition: '.$e->getMessage().
-                ' Please use safe path names without shell metacharacters.',
-                0,
-                $e
-            );
-        }
-    }
-
-    // Validate target
-    try {
-        validateShellSafePath($target, 'volume target');
-    } catch (\Exception $e) {
-        throw new \Exception(
-            'Invalid Docker volume definition: '.$e->getMessage().
-            ' Please use safe path names without shell metacharacters.',
-            0,
-            $e
-        );
-    }
+    // Canonical parsing also validates and throws on unsafe input
+    parseDockerVolumeString($volumeString);
 }
 
 function parseDockerVolumeString(string $volumeString): array
diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php
index f52c9bd11..b8e5a7f7a 100644
--- a/tests/Unit/VolumeArrayFormatSecurityTest.php
+++ b/tests/Unit/VolumeArrayFormatSecurityTest.php
@@ -168,7 +168,7 @@
     expect($source)->toBe('${DATA_PATH}');
     // Our validation allows simple env var references
     $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source);
-    expect($isSimpleEnvVar)->toBeTrue();
+    expect($isSimpleEnvVar)->toBe(1); // preg_match returns 1 on success, not true
 });
 
 test('array-format with malicious environment variable default', function () {
diff --git a/tests/Unit/WindowsPathVolumeTest.php b/tests/Unit/WindowsPathVolumeTest.php
new file mode 100644
index 000000000..1d3af6abd
--- /dev/null
+++ b/tests/Unit/WindowsPathVolumeTest.php
@@ -0,0 +1,64 @@
+<?php
+
+test('parseDockerVolumeString correctly handles Windows paths with drive letters', function () {
+    $windowsVolume = 'C:\\host\\path:/container';
+
+    $result = parseDockerVolumeString($windowsVolume);
+
+    expect((string) $result['source'])->toBe('C:\\host\\path');
+    expect((string) $result['target'])->toBe('/container');
+});
+
+test('validateVolumeStringForInjection correctly handles Windows paths via parseDockerVolumeString', function () {
+    $windowsVolume = 'C:\\Users\\Data:/app/data';
+
+    // Should not throw an exception
+    validateVolumeStringForInjection($windowsVolume);
+
+    // If we get here, the test passed
+    expect(true)->toBeTrue();
+});
+
+test('validateVolumeStringForInjection rejects malicious Windows-like paths', function () {
+    $maliciousVolume = 'C:\\host\\`whoami`:/container';
+
+    expect(fn () => validateVolumeStringForInjection($maliciousVolume))
+        ->toThrow(\Exception::class);
+});
+
+test('validateDockerComposeForInjection handles Windows paths in compose files', function () {
+    $dockerComposeYaml = <<<'YAML'
+services:
+  web:
+    image: nginx
+    volumes:
+      - C:\Users\Data:/app/data
+YAML;
+
+    // Should not throw an exception
+    validateDockerComposeForInjection($dockerComposeYaml);
+
+    expect(true)->toBeTrue();
+});
+
+test('validateDockerComposeForInjection rejects Windows paths with injection', function () {
+    $dockerComposeYaml = <<<'YAML'
+services:
+  web:
+    image: nginx
+    volumes:
+      - C:\Users\$(whoami):/app/data
+YAML;
+
+    expect(fn () => validateDockerComposeForInjection($dockerComposeYaml))
+        ->toThrow(\Exception::class);
+});
+
+test('Windows paths with complex paths and spaces are handled correctly', function () {
+    $windowsVolume = 'C:\\Program Files\\MyApp:/app';
+
+    $result = parseDockerVolumeString($windowsVolume);
+
+    expect((string) $result['source'])->toBe('C:\\Program Files\\MyApp');
+    expect((string) $result['target'])->toBe('/app');
+});