From cf075790ab78eb3041de6735893b287be81d05b9 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 13 Aug 2026 11:25:56 +0100 Subject: [PATCH] fix: k8s config and silent source ingests --- application/api/user/base.py | 2 +- application/storage/s3.py | 4 +++- application/utils.py | 13 ++++++++----- application/worker.py | 8 ++++++-- deployment/k8s/docsgpt-secrets.yaml | 9 ++++++--- tests/api/user/test_base.py | 23 +++++++++++++++++++++++ tests/storage/test_s3_storage.py | 13 +++++++++++++ tests/test_worker_utils.py | 5 +++-- tests/test_zip_extraction_security.py | 25 +++++++++++++------------ 9 files changed, 76 insertions(+), 26 deletions(-) diff --git a/application/api/user/base.py b/application/api/user/base.py index c73aa692..82e3c313 100644 --- a/application/api/user/base.py +++ b/application/api/user/base.py @@ -269,7 +269,7 @@ def _validate_agent_image_upload(file, filename: str) -> None: with Image.open(stream) as image: image_format = (image.format or "").upper() - if image_format != image_policy[0]: + if image_format not in image_policy[0]: raise ValueError("Image content does not match its extension") if image.width * image.height > settings.AGENT_IMAGE_MAX_PIXELS: raise ValueError("Image exceeds the pixel limit") diff --git a/application/storage/s3.py b/application/storage/s3.py index d6bf7d96..74c11975 100644 --- a/application/storage/s3.py +++ b/application/storage/s3.py @@ -128,7 +128,9 @@ class S3Storage(BaseStorage): metadata = self.s3.head_object(Bucket=self.bucket_name, Key=path) except ClientError as exc: error_code = str(exc.response.get("Error", {}).get("Code", "")) - if error_code in {"404", "NoSuchKey", "NotFound"}: + # HEAD on a missing key returns 403 instead of 404 when credentials + # lack s3:ListBucket, so access-denied codes also mean "absent". + if error_code in {"404", "NoSuchKey", "NotFound", "403", "AccessDenied", "Forbidden"}: raise FileNotFoundError(f"File not found: {path}") from exc raise return int(metadata["ContentLength"]) diff --git a/application/utils.py b/application/utils.py index bd52bf41..16dad7b8 100644 --- a/application/utils.py +++ b/application/utils.py @@ -238,12 +238,15 @@ def validate_function_name(function_name): return True +# Extension -> (accepted Pillow formats, MIME type). JPEG entries also accept +# MPO because Pillow reports multi-picture JPEGs (e.g. iPhone portrait photos, +# Samsung motion photos) as MPO. AGENT_IMAGE_FORMATS = { - ".gif": ("GIF", "image/gif"), - ".jpeg": ("JPEG", "image/jpeg"), - ".jpg": ("JPEG", "image/jpeg"), - ".png": ("PNG", "image/png"), - ".webp": ("WEBP", "image/webp"), + ".gif": (("GIF",), "image/gif"), + ".jpeg": (("JPEG", "MPO"), "image/jpeg"), + ".jpg": (("JPEG", "MPO"), "image/jpeg"), + ".png": (("PNG",), "image/png"), + ".webp": (("WEBP",), "image/webp"), } diff --git a/application/worker.py b/application/worker.py index f22368f2..c152c25b 100755 --- a/application/worker.py +++ b/application/worker.py @@ -375,6 +375,10 @@ def extract_zip_recursive( extract_to (str): Destination path for extracted files. current_depth (int): Current depth of recursion. max_depth (int): Maximum allowed depth of recursion to prevent infinite loops. + + Raises: + ZipExtractionError: If the archive fails safety validation, so ingestion + fails loudly instead of indexing an empty directory. """ if current_depth > max_depth: logging.warning(f"Reached maximum recursion depth of {max_depth}") @@ -407,7 +411,7 @@ def extract_zip_recursive( os.remove(zip_path) except OSError: pass - return + raise except Exception as e: logging.error( "Error extracting zip file %s: %s", @@ -415,7 +419,7 @@ def extract_zip_recursive( safe_zip_error_message(e), exc_info=True, ) - return + raise def download_file(url, params, dest_path): diff --git a/deployment/k8s/docsgpt-secrets.yaml b/deployment/k8s/docsgpt-secrets.yaml index 8fc04bdc..4d125e6c 100644 --- a/deployment/k8s/docsgpt-secrets.yaml +++ b/deployment/k8s/docsgpt-secrets.yaml @@ -12,8 +12,10 @@ type: Opaque # manually add a MONGO_URI key below (base64-encoded) and apply the # opt-in manifests under deployment/k8s/optional-mongo/. Example: # MONGO_URI: -# - Before deploying, add JWT_SECRET_KEY below using the base64 encoding of a -# strong random value. Every API and worker replica must receive the same key. +# - JWT_SECRET_KEY below is a known dev-grade default so the stock manifests +# boot out of the box. Replace it with the base64 encoding of a strong random +# value before exposing the API. Every API and worker replica must receive +# the same key. # Example generation: openssl rand -hex 32 | base64 data: LLM_PROVIDER: ZG9jc2dwdA== @@ -26,4 +28,5 @@ data: POSTGRES_URI: cG9zdGdyZXNxbDovL2RvY3NncHQ6ZG9jc2dwdEBwb3N0Z3Jlczo1NDMyL2RvY3NncHQ= postgres-user: ZG9jc2dwdA== postgres-password: ZG9jc2dwdA== - # JWT_SECRET_KEY: + # docsgpt-dev-jwt-secret-change-me — CHANGE before exposing the API + JWT_SECRET_KEY: ZG9jc2dwdC1kZXYtand0LXNlY3JldC1jaGFuZ2UtbWU= diff --git a/tests/api/user/test_base.py b/tests/api/user/test_base.py index abdabdd4..75145f6f 100644 --- a/tests/api/user/test_base.py +++ b/tests/api/user/test_base.py @@ -165,6 +165,29 @@ class TestHandleImageUpload: assert "user123" in url mock_storage.save_file.assert_called_once() + def test_accepts_multi_picture_jpeg(self, flask_app): + from application.api.user.base import handle_image_upload + + # Pillow reports multi-picture JPEGs (e.g. iPhone portrait photos) as + # MPO; they must still be accepted under a .jpg extension. + buffer = io.BytesIO() + frame = Image.new("RGB", (1, 1), color="white") + frame.save(buffer, format="MPO", save_all=True, append_images=[frame]) + + with flask_app.test_request_context(): + mock_file = FileStorage( + stream=io.BytesIO(buffer.getvalue()), filename="photo.jpg" + ) + mock_storage = Mock() + + url, error = handle_image_upload( + Mock(files={"image": mock_file}), "", "user123", mock_storage + ) + + assert error is None + assert url is not None + mock_storage.save_file.assert_called_once() + def test_uploads_image_with_non_ascii_basename(self, flask_app): from application.api.user.base import handle_image_upload diff --git a/tests/storage/test_s3_storage.py b/tests/storage/test_s3_storage.py index e13ca805..5cdbaa53 100644 --- a/tests/storage/test_s3_storage.py +++ b/tests/storage/test_s3_storage.py @@ -270,6 +270,19 @@ class TestS3StorageFileSize: with pytest.raises(FileNotFoundError, match="File not found"): s3_storage.get_file_size("avatars/missing.png") + @pytest.mark.unit + def test_get_file_size_maps_denied_head_to_file_not_found( + self, s3_storage, mock_boto3_client + ): + # Without s3:ListBucket, HEAD on a missing key returns 403, not 404. + mock_boto3_client.head_object.side_effect = ClientError( + {"Error": {"Code": "403", "Message": "Forbidden"}}, + "head_object", + ) + + with pytest.raises(FileNotFoundError, match="File not found"): + s3_storage.get_file_size("avatars/missing.png") + class TestS3StoragePresignedUrl: """Test response metadata on short-lived direct avatar delivery URLs.""" diff --git a/tests/test_worker_utils.py b/tests/test_worker_utils.py index 3b8f033d..bd6e4021 100644 --- a/tests/test_worker_utils.py +++ b/tests/test_worker_utils.py @@ -204,7 +204,7 @@ class TestExtractZipRecursive: assert found, "expected nested zip to be extracted" def test_nested_archives_share_one_expansion_budget(self, tmp_path): - from application.worker import extract_zip_recursive + from application.worker import extract_zip_recursive, ZipExtractionError inner = tmp_path / "inner.zip" with zipfile.ZipFile(inner, "w", zipfile.ZIP_STORED) as zf: @@ -222,7 +222,8 @@ class TestExtractZipRecursive: with patch("application.worker.MAX_UNCOMPRESSED_SIZE", limit), patch( "application.worker.MAX_COMPRESSION_RATIO", 10_000, ): - extract_zip_recursive(str(outer), str(extract_to)) + with pytest.raises(ZipExtractionError): + extract_zip_recursive(str(outer), str(extract_to)) assert not (extract_to / "inside.txt").exists() diff --git a/tests/test_zip_extraction_security.py b/tests/test_zip_extraction_security.py index 65214aa7..1b0b6dca 100644 --- a/tests/test_zip_extraction_security.py +++ b/tests/test_zip_extraction_security.py @@ -287,7 +287,7 @@ class TestExtractZipRecursive: assert not os.path.exists(os.path.join(extract_to, "inner.zip")) def test_respects_max_depth(self): - """Extraction should stop at max recursion depth.""" + """Nesting beyond max depth should fail the extraction loudly.""" with tempfile.TemporaryDirectory() as temp_dir: extract_to = os.path.join(temp_dir, "extract") os.makedirs(extract_to) @@ -311,10 +311,8 @@ class TestExtractZipRecursive: f.write(current_content) # Extract with max_depth=2 - extract_zip_recursive(zip_path, extract_to, max_depth=2) - - # The deepest nested zips should remain unextracted - # (we can't easily verify the exact behavior, but the function should not crash) + with pytest.raises(ZipExtractionError): + extract_zip_recursive(zip_path, extract_to, max_depth=2) def test_rejects_path_traversal(self): """Zip with path traversal should be rejected and removed.""" @@ -327,7 +325,8 @@ class TestExtractZipRecursive: with zipfile.ZipFile(zip_path, "w") as zf: zf.writestr("../../../tmp/malicious.txt", "malicious") - extract_zip_recursive(zip_path, extract_to) + with pytest.raises(ZipExtractionError): + extract_zip_recursive(zip_path, extract_to) # Zip should be removed assert not os.path.exists(zip_path) @@ -348,12 +347,13 @@ class TestExtractZipRecursive: with zipfile.ZipFile(zip_path, "w") as zf: zf.writestr(link, "../../outside") - extract_zip_recursive(zip_path, extract_to) + with pytest.raises(ZipExtractionError): + extract_zip_recursive(zip_path, extract_to) assert not os.path.lexists(os.path.join(extract_to, "escape-link")) - def test_handles_corrupted_zip_gracefully(self): - """Corrupted zip should be handled gracefully without crashing.""" + def test_corrupted_zip_fails_loudly(self): + """Corrupted zip should raise so ingestion fails instead of indexing nothing.""" with tempfile.TemporaryDirectory() as temp_dir: zip_path = os.path.join(temp_dir, "corrupted.zip") extract_to = os.path.join(temp_dir, "extract") @@ -363,10 +363,11 @@ class TestExtractZipRecursive: with open(zip_path, "wb") as f: f.write(b"This is not a valid zip file") - # Should not raise, just log error - extract_zip_recursive(zip_path, extract_to) + with pytest.raises(ZipExtractionError): + extract_zip_recursive(zip_path, extract_to) - # Function should complete without exception + # The rejected zip should be removed + assert not os.path.exists(zip_path) class TestZipBombProtection: