fix: use canonical parser for Windows path validation
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>
This commit is contained in:
parent
3700f78355
commit
a219f2e80e
3 changed files with 67 additions and 48 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 () {
|
||||
|
|
|
|||
64
tests/Unit/WindowsPathVolumeTest.php
Normal file
64
tests/Unit/WindowsPathVolumeTest.php
Normal file
|
|
@ -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');
|
||||
});
|
||||
Loading…
Reference in a new issue