From ee552620ebc4e8875685d15507797fa06d237cf9 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 12 Aug 2026 17:05:57 +0100 Subject: [PATCH] fix: mini nits --- application/api/user/attachments/routes.py | 3 +-- application/api/user/sources/upload.py | 5 +++-- application/security/zip_archive.py | 10 ++++------ application/upload_limits.py | 22 +++++++--------------- tests/api/user/attachments/test_routes.py | 1 + tests/api/user/sources/test_upload.py | 6 +++++- 6 files changed, 21 insertions(+), 26 deletions(-) diff --git a/application/api/user/attachments/routes.py b/application/api/user/attachments/routes.py index b7d51a9c..f3e7a8b5 100644 --- a/application/api/user/attachments/routes.py +++ b/application/api/user/attachments/routes.py @@ -61,7 +61,7 @@ _AGENT_IMAGE_REDIRECT_CACHE_SECONDS = _AGENT_IMAGE_PRESIGNED_TTL_SECONDS - 60 def _stream_file(file_obj, size_bytes: int): - """Yield exactly the size-checked file range and always close the handle.""" + """Yield up to the size-checked file range and always close the handle.""" remaining = size_bytes try: while remaining > 0: @@ -742,7 +742,6 @@ class ServeImage(Resource): content_type=content_type, direct_passthrough=True, ) - response.content_length = size_bytes response.headers.set("Cache-Control", "public, max-age=86400, immutable") response.headers.set("X-Content-Type-Options", "nosniff") diff --git a/application/api/user/sources/upload.py b/application/api/user/sources/upload.py index 80901880..4fcf9cc5 100644 --- a/application/api/user/sources/upload.py +++ b/application/api/user/sources/upload.py @@ -344,8 +344,8 @@ class UploadFile(Resource): 413, ) except (UploadTooLargeError, ZipExtractionError) as err: - zip_error_detail = safe_zip_error_message(err) if isinstance(err, ZipExtractionError): + zip_error_detail = safe_zip_error_message(err) current_app.logger.warning( "Rejected unsafe ZIP upload %s: %s", active_upload_name, @@ -362,7 +362,8 @@ class UploadFile(Resource): { "success": False, "message": ( - f"{active_upload_name}: {zip_error_detail}" + f"{active_upload_name}: " + "ZIP archive failed safety validation" if isinstance(err, ZipExtractionError) else upload_limit_message() ), diff --git a/application/security/zip_archive.py b/application/security/zip_archive.py index 04ceb8a3..4d3b5527 100644 --- a/application/security/zip_archive.py +++ b/application/security/zip_archive.py @@ -7,6 +7,7 @@ import shutil import stat import tempfile import zipfile +from contextlib import suppress from dataclasses import dataclass from pathlib import PurePosixPath @@ -335,16 +336,13 @@ def _commit_staging(staging_root: str, destination_root: str) -> None: os.replace(source, target) moved_files.append(target) except Exception: + # Rollback is best effort and must not hide the original commit failure. for target in reversed(moved_files): - try: + with suppress(OSError): os.remove(target) - except OSError: - pass for target in reversed(created_directories): - try: + with suppress(OSError): os.rmdir(target) - except OSError: - pass raise diff --git a/application/upload_limits.py b/application/upload_limits.py index dc8ff379..98736c29 100644 --- a/application/upload_limits.py +++ b/application/upload_limits.py @@ -4,6 +4,7 @@ from __future__ import annotations import io import os +from contextlib import suppress from typing import BinaryIO from application.core.settings import settings @@ -80,10 +81,8 @@ def copy_upload_to_path( """ limit = int(max_bytes or settings.UPLOAD_MAX_FILE_BYTES) stream = getattr(upload, "stream", upload) - try: + with suppress(AttributeError, OSError): stream.seek(0) - except (AttributeError, OSError): - pass total = 0 with open(destination, "wb") as target: @@ -102,10 +101,8 @@ def read_upload_limited(upload: BinaryIO, max_bytes: int | None = None) -> bytes """Read at most one configured file into memory, rejecting overflow.""" limit = int(max_bytes or settings.UPLOAD_MAX_FILE_BYTES) stream = getattr(upload, "stream", upload) - try: + with suppress(AttributeError, OSError): stream.seek(0) - except (AttributeError, OSError): - pass data = bytearray() while True: chunk = stream.read(min(_COPY_CHUNK_BYTES, limit - len(data) + 1)) @@ -125,10 +122,8 @@ def read_text_upload_limited( """Decode a bounded upload without first duplicating it into ``bytes``.""" limit = int(max_bytes or settings.UPLOAD_MAX_FILE_BYTES) stream = getattr(upload, "stream", upload) - try: + with suppress(AttributeError, OSError): stream.seek(0) - except (AttributeError, OSError): - pass raw_reader = _LimitedRawReader(stream, limit) buffered_reader = io.BufferedReader(raw_reader, buffer_size=_COPY_CHUNK_BYTES) @@ -136,11 +131,8 @@ def read_text_upload_limited( try: return text_reader.read() finally: - try: + # Cleanup is best effort and must not mask the read result or exception. + with suppress(ValueError, OSError): text_reader.detach() - except (ValueError, OSError): - pass - try: + with suppress(ValueError, OSError): buffered_reader.detach() - except (ValueError, OSError): - pass diff --git a/tests/api/user/attachments/test_routes.py b/tests/api/user/attachments/test_routes.py index 2c2b33e2..d28989af 100644 --- a/tests/api/user/attachments/test_routes.py +++ b/tests/api/user/attachments/test_routes.py @@ -1588,6 +1588,7 @@ class TestServeImage: assert response.headers.get("Content-Type") == "image/png" assert response.headers.get("X-Content-Type-Options") == "nosniff" assert response.is_streamed + assert response.content_length is None assert b"".join(response.response) == b"\x89PNG\r\n" mock_storage.get_file.assert_called_once_with(self.image_path) diff --git a/tests/api/user/sources/test_upload.py b/tests/api/user/sources/test_upload.py index cabb92be..566cf4c0 100644 --- a/tests/api/user/sources/test_upload.py +++ b/tests/api/user/sources/test_upload.py @@ -300,9 +300,13 @@ class TestUploadFile: response = UploadFile().post() assert response.status_code == 413 - assert "documents.zip" in response.json["message"] + assert response.json["message"] == ( + "documents.zip: ZIP archive failed safety validation" + ) + assert "bad" not in response.json["message"] assert "\n" not in response.json["message"] logged_values = " ".join(str(value) for value in warning.call_args.args) + assert "bad\\x0aFORGED" in logged_values assert "\n" not in logged_values def test_expands_nested_zip_before_storage(self, app):