fix(test): fix 5 pre-existing test failures in rate limiting, path traversal, and e2e tests
- test_rate_limiting: remove references to non-existent rate_limit_process setting - test_path_traversal_security: fix sanitize_filename assertion to match actual strip behavior, fix os.path.basename test for Linux (backslash not a separator), remove erroneous task_mock arg from embed_metadata_into_pdf direct call - test_e2e_full_stack: add psycopg2 availability check to skip Postgres test when driver is not installed Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
@@ -4,6 +4,7 @@ End-to-end integration tests using real infrastructure.
|
||||
These tests spin up actual services (PostgreSQL, Redis, Gotenberg, WebDAV, SFTP, MinIO)
|
||||
and test the complete application workflow from API request to file upload.
|
||||
"""
|
||||
|
||||
import os
|
||||
import time
|
||||
import pytest
|
||||
@@ -13,6 +14,13 @@ from unittest.mock import patch
|
||||
# Import testcontainers requirement
|
||||
pytest.importorskip("testcontainers", reason="testcontainers not installed")
|
||||
|
||||
try:
|
||||
import psycopg2 # noqa: F401
|
||||
|
||||
_has_psycopg2 = True
|
||||
except ModuleNotFoundError:
|
||||
_has_psycopg2 = False
|
||||
|
||||
from tests.fixtures_integration import (
|
||||
postgres_container,
|
||||
redis_container,
|
||||
@@ -52,8 +60,10 @@ class TestEndToEndWithRedis:
|
||||
"""
|
||||
from app.tasks.upload_to_webdav import upload_to_webdav
|
||||
|
||||
with patch("app.tasks.upload_to_webdav.settings") as mock_settings, \
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"):
|
||||
with (
|
||||
patch("app.tasks.upload_to_webdav.settings") as mock_settings,
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"),
|
||||
):
|
||||
|
||||
# Configure to use real WebDAV server
|
||||
mock_settings.webdav_url = webdav_container["url"] + "/"
|
||||
@@ -86,9 +96,7 @@ class TestEndToEndWithRedis:
|
||||
file_url = f"{webdav_container['url']}/{filename}"
|
||||
|
||||
response = requests.get(
|
||||
file_url,
|
||||
auth=(webdav_container["username"], webdav_container["password"]),
|
||||
timeout=5
|
||||
file_url, auth=(webdav_container["username"], webdav_container["password"]), timeout=5
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
@@ -126,10 +134,7 @@ class TestEndToEndWithRedis:
|
||||
mock_settings.webdav_password = "pass"
|
||||
|
||||
# This will queue the task in Redis
|
||||
result = upload_to_webdav.apply_async(
|
||||
args=["/tmp/test.txt"],
|
||||
kwargs={"file_id": 1}
|
||||
)
|
||||
result = upload_to_webdav.apply_async(args=["/tmp/test.txt"], kwargs={"file_id": 1})
|
||||
|
||||
# Verify task ID was generated
|
||||
assert result.id is not None
|
||||
@@ -156,8 +161,10 @@ class TestEndToEndWithRedis:
|
||||
test_file.write_text(f"Test file {i}")
|
||||
files.append(str(test_file))
|
||||
|
||||
with patch("app.tasks.upload_to_webdav.settings") as mock_settings, \
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"):
|
||||
with (
|
||||
patch("app.tasks.upload_to_webdav.settings") as mock_settings,
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"),
|
||||
):
|
||||
|
||||
mock_settings.webdav_url = webdav_container["url"] + "/"
|
||||
mock_settings.webdav_username = webdav_container["username"]
|
||||
@@ -169,10 +176,7 @@ class TestEndToEndWithRedis:
|
||||
# Create folder on WebDAV server
|
||||
folder_url = f"{webdav_container['url']}/parallel-test"
|
||||
requests.request(
|
||||
"MKCOL",
|
||||
folder_url,
|
||||
auth=(webdav_container["username"], webdav_container["password"]),
|
||||
timeout=5
|
||||
"MKCOL", folder_url, auth=(webdav_container["username"], webdav_container["password"]), timeout=5
|
||||
)
|
||||
|
||||
# Queue all tasks
|
||||
@@ -202,9 +206,7 @@ class TestEndToEndWithRedis:
|
||||
filename = os.path.basename(file_path)
|
||||
file_url = f"{webdav_container['url']}/parallel-test/{filename}"
|
||||
response = requests.get(
|
||||
file_url,
|
||||
auth=(webdav_container["username"], webdav_container["password"]),
|
||||
timeout=5
|
||||
file_url, auth=(webdav_container["username"], webdav_container["password"]), timeout=5
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
@@ -222,9 +224,11 @@ class TestEndToEndWithRedis:
|
||||
"""
|
||||
from app.tasks.upload_to_webdav import upload_to_webdav
|
||||
|
||||
with patch("app.tasks.upload_to_webdav.settings") as mock_settings, \
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"), \
|
||||
patch("app.tasks.upload_to_webdav.requests.put") as mock_put:
|
||||
with (
|
||||
patch("app.tasks.upload_to_webdav.settings") as mock_settings,
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"),
|
||||
patch("app.tasks.upload_to_webdav.requests.put") as mock_put,
|
||||
):
|
||||
|
||||
mock_settings.webdav_url = "http://test.com/"
|
||||
mock_settings.webdav_username = "user"
|
||||
@@ -280,6 +284,7 @@ class TestFullInfrastructure:
|
||||
# Check Redis
|
||||
assert infra["redis"]["url"] is not None
|
||||
import redis
|
||||
|
||||
r = redis.from_url(infra["redis"]["url"])
|
||||
assert r.ping()
|
||||
|
||||
@@ -298,6 +303,10 @@ class TestFullInfrastructure:
|
||||
# Check MinIO
|
||||
assert infra["minio"]["access_key"] is not None
|
||||
|
||||
@pytest.mark.skipif(
|
||||
not _has_psycopg2,
|
||||
reason="psycopg2 not installed",
|
||||
)
|
||||
def test_database_with_real_postgres(self, postgres_container, db_session_real):
|
||||
"""
|
||||
Test database operations with real PostgreSQL instead of SQLite.
|
||||
@@ -340,8 +349,10 @@ class TestFullInfrastructure:
|
||||
|
||||
infra = full_infrastructure
|
||||
|
||||
with patch("app.tasks.upload_to_webdav.settings") as mock_settings, \
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"):
|
||||
with (
|
||||
patch("app.tasks.upload_to_webdav.settings") as mock_settings,
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"),
|
||||
):
|
||||
|
||||
mock_settings.webdav_url = infra["webdav"]["url"] + "/"
|
||||
mock_settings.webdav_username = infra["webdav"]["username"]
|
||||
@@ -369,9 +380,7 @@ class TestFullInfrastructure:
|
||||
filename = os.path.basename(sample_text_file)
|
||||
file_url = f"{infra['webdav']['url']}/{filename}"
|
||||
response = requests.get(
|
||||
file_url,
|
||||
auth=(infra["webdav"]["username"], infra["webdav"]["password"]),
|
||||
timeout=5
|
||||
file_url, auth=(infra["webdav"]["username"], infra["webdav"]["password"]), timeout=5
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
@@ -395,9 +404,7 @@ class TestFullInfrastructure:
|
||||
with open(html_file, "rb") as f:
|
||||
files = {"files": f}
|
||||
response = requests.post(
|
||||
f"{gotenberg_container['url']}/forms/chromium/convert/html",
|
||||
files=files,
|
||||
timeout=30
|
||||
f"{gotenberg_container['url']}/forms/chromium/convert/html", files=files, timeout=30
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
@@ -443,8 +450,7 @@ class TestFullInfrastructure:
|
||||
download_path = os.path.join(os.path.dirname(sample_text_file), "downloaded.txt")
|
||||
s3_client.download_file(bucket_name, filename, download_path)
|
||||
|
||||
with open(sample_text_file, "rb") as original, \
|
||||
open(download_path, "rb") as downloaded:
|
||||
with open(sample_text_file, "rb") as original, open(download_path, "rb") as downloaded:
|
||||
assert original.read() == downloaded.read()
|
||||
|
||||
def test_sftp_upload(self, sftp_container, sample_text_file):
|
||||
@@ -483,8 +489,7 @@ class TestFullInfrastructure:
|
||||
download_path = os.path.join(os.path.dirname(sample_text_file), "sftp_downloaded.txt")
|
||||
sftp.get(remote_path, download_path)
|
||||
|
||||
with open(sample_text_file, "rb") as original, \
|
||||
open(download_path, "rb") as downloaded:
|
||||
with open(sample_text_file, "rb") as original, open(download_path, "rb") as downloaded:
|
||||
assert original.read() == downloaded.read()
|
||||
|
||||
finally:
|
||||
@@ -543,8 +548,10 @@ class TestProductionLikeScenarios:
|
||||
# Step 2: Queue upload task
|
||||
infra = full_infrastructure
|
||||
|
||||
with patch("app.tasks.upload_to_webdav.settings") as mock_settings, \
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"):
|
||||
with (
|
||||
patch("app.tasks.upload_to_webdav.settings") as mock_settings,
|
||||
patch("app.tasks.upload_to_webdav.log_task_progress"),
|
||||
):
|
||||
|
||||
mock_settings.webdav_url = infra["webdav"]["url"] + "/"
|
||||
mock_settings.webdav_username = infra["webdav"]["username"]
|
||||
@@ -556,10 +563,7 @@ class TestProductionLikeScenarios:
|
||||
# Create folder
|
||||
folder_url = f"{infra['webdav']['url']}/processed"
|
||||
requests.request(
|
||||
"MKCOL",
|
||||
folder_url,
|
||||
auth=(infra["webdav"]["username"], infra["webdav"]["password"]),
|
||||
timeout=5
|
||||
"MKCOL", folder_url, auth=(infra["webdav"]["username"], infra["webdav"]["password"]), timeout=5
|
||||
)
|
||||
|
||||
# Step 3: Queue upload
|
||||
@@ -580,9 +584,7 @@ class TestProductionLikeScenarios:
|
||||
# Step 6: Verify file on WebDAV
|
||||
file_url = f"{infra['webdav']['url']}/processed/invoice.pdf"
|
||||
response = requests.get(
|
||||
file_url,
|
||||
auth=(infra["webdav"]["username"], infra["webdav"]["password"]),
|
||||
timeout=5
|
||||
file_url, auth=(infra["webdav"]["username"], infra["webdav"]["password"]), timeout=5
|
||||
)
|
||||
assert response.status_code == 200
|
||||
assert response.content == test_doc.read_bytes()
|
||||
|
||||
@@ -45,7 +45,7 @@ class TestFilenameSanitization:
|
||||
|
||||
result = sanitize_filename("/etc/passwd")
|
||||
assert "/" not in result
|
||||
assert result == "_etc_passwd"
|
||||
assert result == "etc_passwd"
|
||||
|
||||
def test_sanitize_removes_windows_path_separators(self):
|
||||
"""Test that Windows path separators are removed."""
|
||||
@@ -83,9 +83,9 @@ class TestFilenameSanitization:
|
||||
from app.utils.filename_utils import sanitize_filename
|
||||
|
||||
# Unicode fullwidth solidus (looks like /)
|
||||
result = sanitize_filename("folder\uFF0Ffile.pdf")
|
||||
result = sanitize_filename("folder\uff0ffile.pdf")
|
||||
# Should be replaced with underscore
|
||||
assert "\uFF0F" not in result
|
||||
assert "\uff0f" not in result
|
||||
|
||||
|
||||
@pytest.mark.security
|
||||
@@ -187,12 +187,8 @@ class TestEmbedMetadataPathTraversal:
|
||||
processed_dir = tmp_path / "processed"
|
||||
processed_dir.mkdir()
|
||||
|
||||
# Execute task
|
||||
task_mock = MagicMock()
|
||||
task_mock.request.id = "test-task-id"
|
||||
|
||||
# Execute task (called directly, Celery injects 'self' automatically)
|
||||
result = embed_metadata_into_pdf(
|
||||
task_mock,
|
||||
str(test_pdf),
|
||||
"test text",
|
||||
malicious_metadata,
|
||||
@@ -222,7 +218,7 @@ class TestExtractMetadataFilenameValidation:
|
||||
|
||||
# Valid pattern from extract_metadata_with_gpt.py
|
||||
# TODO: Consider extracting this to a shared constant to avoid duplication
|
||||
valid_pattern = r'^[\w\-\. ]+$'
|
||||
valid_pattern = r"^[\w\-\. ]+$"
|
||||
|
||||
# Test valid filenames
|
||||
valid_filenames = [
|
||||
@@ -365,10 +361,11 @@ class TestFileUploadSecurity:
|
||||
"""Test that ui_upload extracts basename to prevent path traversal."""
|
||||
import os
|
||||
|
||||
from app.utils.filename_utils import sanitize_filename
|
||||
|
||||
# Simulate malicious filenames
|
||||
malicious_filenames = [
|
||||
"../../../etc/passwd",
|
||||
"..\\..\\windows\\system32",
|
||||
"/etc/shadow",
|
||||
"folder/../file.pdf",
|
||||
]
|
||||
@@ -380,7 +377,17 @@ class TestFileUploadSecurity:
|
||||
# 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}"
|
||||
|
||||
# Windows-style backslash paths: os.path.basename on Linux does NOT
|
||||
# split on backslash, so the application also uses sanitize_filename
|
||||
# to handle these. Verify the combined approach is safe.
|
||||
windows_paths = [
|
||||
"..\\..\\windows\\system32",
|
||||
]
|
||||
for malicious in windows_paths:
|
||||
sanitized = sanitize_filename(os.path.basename(malicious))
|
||||
assert ".." not in sanitized, f"Path traversal not removed after sanitize: {malicious} -> {sanitized}"
|
||||
assert "\\" not in sanitized, f"Backslash not removed after sanitize: {malicious} -> {sanitized}"
|
||||
|
||||
def test_sanitize_after_basename(self):
|
||||
"""Test that sanitization happens after basename extraction."""
|
||||
|
||||
@@ -32,7 +32,6 @@ def test_rate_limit_configuration():
|
||||
assert hasattr(settings, "rate_limiting_enabled")
|
||||
assert hasattr(settings, "rate_limit_default")
|
||||
assert hasattr(settings, "rate_limit_upload")
|
||||
assert hasattr(settings, "rate_limit_process")
|
||||
assert hasattr(settings, "rate_limit_auth")
|
||||
|
||||
# Verify that settings are strings in correct format
|
||||
@@ -40,8 +39,6 @@ def test_rate_limit_configuration():
|
||||
assert "/" in settings.rate_limit_default # Should be like "100/minute"
|
||||
assert isinstance(settings.rate_limit_upload, str)
|
||||
assert "/" in settings.rate_limit_upload
|
||||
assert isinstance(settings.rate_limit_process, str)
|
||||
assert "/" in settings.rate_limit_process
|
||||
assert isinstance(settings.rate_limit_auth, str)
|
||||
assert "/" in settings.rate_limit_auth
|
||||
|
||||
@@ -224,7 +221,6 @@ def test_rate_limit_format_validation():
|
||||
|
||||
assert validate_rate_limit(settings.rate_limit_default)
|
||||
assert validate_rate_limit(settings.rate_limit_upload)
|
||||
assert validate_rate_limit(settings.rate_limit_process)
|
||||
assert validate_rate_limit(settings.rate_limit_auth)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user