Fix path traversal false positives for filenames with ellipsis
Replace naive string-based ".." detection with component-based analysis to eliminate false positives while maintaining security. Problem: - Filenames like "Battery... Rekon 35.m4a" were incorrectly flagged - String check `if ".." in path` matched ellipsis (...) as traversal Solution: - Parse path into components using Path().parts - Check each component for exact ".." match - Allows ellipsis in filenames while blocking actual traversal Security maintained: - ✅ Blocks: ../etc/passwd, dir/../../secret, /../../../etc/hosts - ✅ Allows: file...mp3, Wait... what.m4a, Battery...Rekon.m4a Tests: - Added comprehensive test suite with 8 test cases - Verified ellipsis filenames pass validation - Verified path traversal attacks still blocked - All tests passing (8/8) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Alihan
@@ -178,10 +178,13 @@ def validate_path_safe(file_path: str, allowed_dirs: Optional[List[str]] = None)
|
||||
except Exception as e:
|
||||
raise ValidationError(f"Invalid path format: {sanitize_error_message(str(e))}")
|
||||
|
||||
# Check for path traversal attempts
|
||||
# Check for path traversal attempts in path components
|
||||
# This allows filenames with ellipsis (e.g., "Wait...mp3", "file...audio.m4a")
|
||||
# while blocking actual path traversal (e.g., "../../../etc/passwd")
|
||||
path_str = str(path)
|
||||
if ".." in path_str:
|
||||
logger.warning(f"Path traversal attempt detected: {path_str}")
|
||||
path_parts = path.parts
|
||||
if any(part == ".." for part in path_parts):
|
||||
logger.warning(f"Path traversal attempt detected in components: {path_str}")
|
||||
raise PathTraversalError("Path traversal (..) is not allowed")
|
||||
|
||||
# Check for null bytes
|
||||
|
||||
208
tests/test_path_traversal_fix.py
Normal file
208
tests/test_path_traversal_fix.py
Normal file
@@ -0,0 +1,208 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
Test path traversal detection with ellipsis support.
|
||||
|
||||
Tests the fix for false positives where filenames containing ellipsis (...)
|
||||
were incorrectly flagged as path traversal attempts.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
import sys
|
||||
import os
|
||||
from pathlib import Path
|
||||
import tempfile
|
||||
|
||||
# Add src to path
|
||||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', 'src'))
|
||||
|
||||
from utils.input_validation import (
|
||||
validate_path_safe,
|
||||
validate_audio_file,
|
||||
PathTraversalError,
|
||||
ValidationError,
|
||||
InvalidFileTypeError
|
||||
)
|
||||
|
||||
|
||||
class TestPathTraversalWithEllipsis:
|
||||
"""Test that ellipsis in filenames is allowed while blocking real attacks."""
|
||||
|
||||
def test_filename_with_ellipsis_allowed(self, tmp_path):
|
||||
"""Filenames with ellipsis (...) should be allowed."""
|
||||
test_cases = [
|
||||
"Wait... what.mp3",
|
||||
"This Weird FPV Drone only takes one kind of Battery... Rekon 35 V2.m4a",
|
||||
"file...mp3",
|
||||
"test....audio.wav",
|
||||
"a..b..c.mp3",
|
||||
"dots.........everywhere.m4a"
|
||||
]
|
||||
|
||||
for filename in test_cases:
|
||||
# Create test file
|
||||
test_file = tmp_path / filename
|
||||
test_file.write_text("fake audio data")
|
||||
|
||||
# Should NOT raise PathTraversalError
|
||||
try:
|
||||
result = validate_path_safe(str(test_file), [str(tmp_path)])
|
||||
assert result.exists(), f"File should exist: {filename}"
|
||||
print(f"✓ PASS: {filename}")
|
||||
except PathTraversalError as e:
|
||||
pytest.fail(f"False positive for filename: {filename}. Error: {e}")
|
||||
|
||||
def test_actual_path_traversal_blocked(self, tmp_path):
|
||||
"""Actual path traversal attempts should be blocked."""
|
||||
attack_cases = [
|
||||
"../../../etc/passwd",
|
||||
"..\\..\\..\\windows\\system32",
|
||||
"legitimate/../../../etc/passwd",
|
||||
"dir/../../secret",
|
||||
"../",
|
||||
"..",
|
||||
"subdir/../../../etc/hosts"
|
||||
]
|
||||
|
||||
for attack_path in attack_cases:
|
||||
with pytest.raises(PathTraversalError):
|
||||
validate_path_safe(attack_path, [str(tmp_path)])
|
||||
print(f"✗ FAIL: Should have blocked: {attack_path}")
|
||||
print(f"✓ PASS: Blocked attack: {attack_path}")
|
||||
|
||||
def test_ellipsis_in_full_path_allowed(self, tmp_path):
|
||||
"""Full paths with ellipsis in filename should be allowed."""
|
||||
# Create nested directory
|
||||
subdir = tmp_path / "uploads"
|
||||
subdir.mkdir()
|
||||
|
||||
filename = "Wait... what.mp3"
|
||||
test_file = subdir / filename
|
||||
test_file.write_text("fake audio data")
|
||||
|
||||
# Full path should be allowed when directory is in allowed_dirs
|
||||
result = validate_path_safe(str(test_file), [str(tmp_path)])
|
||||
assert result.exists()
|
||||
print(f"✓ PASS: Full path with ellipsis: {test_file}")
|
||||
|
||||
def test_mixed_dots_edge_cases(self, tmp_path):
|
||||
"""Test edge cases with various dot patterns."""
|
||||
edge_cases = [
|
||||
("single.dot.mp3", True), # Normal filename
|
||||
("..two.dots.mp3", True), # Starts with two dots (filename)
|
||||
("three...dots.mp3", True), # Three consecutive dots
|
||||
("many.....dots.mp3", True), # Many consecutive dots
|
||||
(".", False), # Current directory (should fail)
|
||||
("..", False), # Parent directory (should fail)
|
||||
]
|
||||
|
||||
for filename, should_pass in edge_cases:
|
||||
if should_pass:
|
||||
# Create test file
|
||||
test_file = tmp_path / filename
|
||||
test_file.write_text("fake audio data")
|
||||
|
||||
try:
|
||||
result = validate_path_safe(str(test_file), [str(tmp_path)])
|
||||
assert result.exists(), f"File should exist: {filename}"
|
||||
print(f"✓ PASS: Allowed: {filename}")
|
||||
except PathTraversalError:
|
||||
pytest.fail(f"Should have allowed: {filename}")
|
||||
else:
|
||||
with pytest.raises((PathTraversalError, ValidationError)):
|
||||
validate_path_safe(filename, [str(tmp_path)])
|
||||
print(f"✓ PASS: Blocked: {filename}")
|
||||
|
||||
|
||||
class TestAudioFileValidationWithEllipsis:
|
||||
"""Test full audio file validation with ellipsis support."""
|
||||
|
||||
def test_audio_file_with_ellipsis(self, tmp_path):
|
||||
"""Audio files with ellipsis should pass validation."""
|
||||
filename = "This Weird FPV Drone only takes one kind of Battery... Rekon 35 V2.m4a"
|
||||
test_file = tmp_path / filename
|
||||
test_file.write_bytes(b"fake audio data" * 100) # Non-empty file
|
||||
|
||||
# Should pass validation
|
||||
result = validate_audio_file(str(test_file), [str(tmp_path)])
|
||||
assert result.exists()
|
||||
print(f"✓ PASS: Audio validation with ellipsis: {filename}")
|
||||
|
||||
def test_audio_file_traversal_attack_blocked(self, tmp_path):
|
||||
"""Audio file validation should block path traversal."""
|
||||
attack_path = "../../../etc/passwd"
|
||||
|
||||
with pytest.raises(PathTraversalError):
|
||||
validate_audio_file(attack_path, [str(tmp_path)])
|
||||
print(f"✓ PASS: Audio validation blocked attack: {attack_path}")
|
||||
|
||||
|
||||
class TestComponentBasedDetection:
|
||||
"""Test that detection is based on path components, not string matching."""
|
||||
|
||||
def test_component_analysis(self, tmp_path):
|
||||
"""Verify that we're analyzing components, not doing string matching."""
|
||||
# These should PASS (ellipsis is in the filename component)
|
||||
safe_cases = [
|
||||
tmp_path / "file...mp3",
|
||||
tmp_path / "subdir" / "Wait...what.m4a",
|
||||
]
|
||||
|
||||
for test_path in safe_cases:
|
||||
test_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
test_path.write_text("data")
|
||||
|
||||
# Check that ".." is not in any component
|
||||
parts = Path(test_path).parts
|
||||
assert not any(part == ".." for part in parts), \
|
||||
f"Should not have '..' as a component: {test_path}"
|
||||
|
||||
# Validation should pass
|
||||
result = validate_path_safe(str(test_path), [str(tmp_path)])
|
||||
assert result.exists()
|
||||
print(f"✓ PASS: Component analysis correct: {test_path}")
|
||||
|
||||
def test_component_attack_detection(self):
|
||||
"""Verify that actual '..' components are detected."""
|
||||
# These should FAIL ('..' is a path component)
|
||||
attack_cases = [
|
||||
"../etc/passwd",
|
||||
"dir/../secret",
|
||||
"../../file.mp3",
|
||||
]
|
||||
|
||||
for attack_path in attack_cases:
|
||||
path = Path(attack_path)
|
||||
parts = path.parts
|
||||
|
||||
# Verify that ".." IS in components
|
||||
assert any(part == ".." for part in parts), \
|
||||
f"Should have '..' as a component: {attack_path}"
|
||||
print(f"✓ PASS: Attack has '..' component: {attack_path}")
|
||||
|
||||
|
||||
def run_tests():
|
||||
"""Run all tests with verbose output."""
|
||||
print("=" * 70)
|
||||
print("Running Path Traversal Detection Tests")
|
||||
print("=" * 70)
|
||||
|
||||
# Run pytest with verbose output
|
||||
exit_code = pytest.main([
|
||||
__file__,
|
||||
"-v",
|
||||
"--tb=short",
|
||||
"-p", "no:warnings"
|
||||
])
|
||||
|
||||
print("=" * 70)
|
||||
if exit_code == 0:
|
||||
print("✓ All tests passed!")
|
||||
else:
|
||||
print("✗ Some tests failed!")
|
||||
print("=" * 70)
|
||||
|
||||
return exit_code
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(run_tests())
|
||||
Reference in New Issue
Block a user