diff --git a/app/tasks/extract_metadata_with_gpt.py b/app/tasks/extract_metadata_with_gpt.py index f4c7163b..e673e3bf 100644 --- a/app/tasks/extract_metadata_with_gpt.py +++ b/app/tasks/extract_metadata_with_gpt.py @@ -129,16 +129,16 @@ def extract_metadata_with_gpt(self, filename: str, cleaned_text: str, file_id: i import re filename = metadata.get("filename", "") if filename: - # Check if filename contains only safe characters (alphanumeric, dash, underscore, period, space) - # This matches the sanitize_filename behavior but validates before use - if not re.match(r'^[\w\-\. ]+$', filename): + # Check if filename contains only safe characters AND explicitly check for ".." + # Defense in depth: While the regex [\w\-\. ]+ already excludes / and \, + # we explicitly reject ".." to guard against: + # 1. Potential locale-specific \w behavior + # 2. Files literally named ".." which are valid but problematic + # 3. Future code changes that might relax the regex + if not re.match(r'^[\w\-\. ]+$', filename) or ".." in filename: logger.warning(f"[{task_id}] Invalid filename format from GPT: '{filename}', using fallback") # Reset to empty to trigger fallback to original filename metadata["filename"] = "" - # Additional check: ensure no path traversal patterns - elif ".." in filename or "/" in filename or "\\" in filename: - logger.warning(f"[{task_id}] Path traversal attempt in GPT filename: '{filename}', using fallback") - metadata["filename"] = "" logger.info(f"[{task_id}] Extracted metadata: {metadata}") log_task_progress( diff --git a/tests/test_path_traversal_security.py b/tests/test_path_traversal_security.py index 6c61951c..878b547d 100644 --- a/tests/test_path_traversal_security.py +++ b/tests/test_path_traversal_security.py @@ -220,7 +220,8 @@ class TestExtractMetadataFilenameValidation: """Test that invalid filename formats are rejected.""" import re - # Valid pattern from sanitize_filename: alphanumeric, dash, underscore, period, space + # Valid pattern from extract_metadata_with_gpt.py + # TODO: Consider extracting this to a shared constant to avoid duplication valid_pattern = r'^[\w\-\. ]+$' # Test valid filenames @@ -376,10 +377,10 @@ class TestFileUploadSecurity: # os.path.basename should extract just the filename basename = os.path.basename(malicious) - # Verify no path traversal remains - assert ".." not in basename or malicious.endswith("..") - assert "/" not in basename - assert "\\" not in basename + # Verify no path traversal remains in basename + assert ".." not in basename, f"Path traversal not removed: {malicious} -> {basename}" + assert "/" not in basename, f"Path separator not removed: {malicious} -> {basename}" + assert "\\" not in basename, f"Path separator not removed: {malicious} -> {basename}" def test_sanitize_after_basename(self): """Test that sanitization happens after basename extraction."""