diff --git a/api/upload.py b/api/upload.py index c799c8f7d..4b0d6e4e9 100644 --- a/api/upload.py +++ b/api/upload.py @@ -13,6 +13,28 @@ from api.helpers import j, bad from api.models import get_session from api.workspace import safe_resolve_ws, resolve_trusted_workspace + +def _max_extracted_bytes() -> int: + """Total-extracted-bytes cap for archive uploads (zip/tar-bomb guard). + + Independently tunable from the upload size cap via + HERMES_WEBUI_MAX_EXTRACTED_MB; defaults to 10x the upload cap. Read at call + time (not import) so the value reflects the running process's environment + and is exercisable by tests against the out-of-process test server. + """ + raw = os.getenv("HERMES_WEBUI_MAX_EXTRACTED_MB", "").strip() + if raw: + try: + mb = float(raw) + if mb > 0: + return int(mb * 1024 * 1024) + except ValueError: + pass + return 10 * MAX_UPLOAD_BYTES + + +# Back-compat module constant (some call sites / tests reference it). The +# authoritative value is _max_extracted_bytes(), read at extraction time. _MAX_EXTRACTED_BYTES = 10 * MAX_UPLOAD_BYTES @@ -146,6 +168,7 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): """ import zipfile, tarfile, io, os, shutil + cap = _max_extracted_bytes() name = Path(filename).name stem = Path(filename).stem # strip .zip / .tar.gz etc. @@ -181,10 +204,10 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): if not member_path.is_relative_to(dest_dir.resolve()): raise ValueError(f'Zip-slip blocked: {member.filename}') # Zip-bomb protection: track actual extracted bytes (not declared file_size) - if total_extracted > _MAX_EXTRACTED_BYTES: + if total_extracted > cap: raise ValueError( f'Extraction too large ({total_extracted // (1024*1024)} MB > ' - f'{_MAX_EXTRACTED_BYTES // (1024*1024)} MB limit). ' + f'{cap // (1024*1024)} MB limit). ' f'Possible zip bomb.' ) member_path.parent.mkdir(parents=True, exist_ok=True) @@ -195,10 +218,10 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): if not chunk: break total_extracted += len(chunk) - if total_extracted > _MAX_EXTRACTED_BYTES: + if total_extracted > cap: raise ValueError( f'Extraction too large (> ' - f'{_MAX_EXTRACTED_BYTES // (1024*1024)} MB limit). ' + f'{cap // (1024*1024)} MB limit). ' f'Possible zip bomb.' ) dst.write(chunk) @@ -214,10 +237,10 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): if not member_path.is_relative_to(dest_dir.resolve()): raise ValueError(f'Tar-slip blocked: {member.name}') # Tar-bomb protection: track actual extracted bytes (not declared size) - if total_extracted > _MAX_EXTRACTED_BYTES: + if total_extracted > cap: raise ValueError( f'Extraction too large ({total_extracted // (1024*1024)} MB > ' - f'{_MAX_EXTRACTED_BYTES // (1024*1024)} MB limit). ' + f'{cap // (1024*1024)} MB limit). ' f'Possible zip bomb.' ) member_path.parent.mkdir(parents=True, exist_ok=True) @@ -230,10 +253,10 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): if not chunk: break total_extracted += len(chunk) - if total_extracted > _MAX_EXTRACTED_BYTES: + if total_extracted > cap: raise ValueError( f'Extraction too large (> ' - f'{_MAX_EXTRACTED_BYTES // (1024*1024)} MB limit). ' + f'{cap // (1024*1024)} MB limit). ' f'Possible zip bomb.' ) dst.write(chunk) @@ -362,7 +385,7 @@ def handle_workspace_upload(handler): target_dir.mkdir(parents=True, exist_ok=True) results = [] - for field_name, (filename, file_bytes) in files.items(): + for _field_name, (filename, file_bytes) in files.items(): if not filename: continue @@ -440,7 +463,7 @@ def handle_workspace_upload(handler): continue results.append({ - 'filename': safe_name, + 'filename': dest.name, 'path': str(dest), 'size': dest.stat().st_size, 'mime': mime, diff --git a/tests/conftest.py b/tests/conftest.py index 43c91a618..dcccfa28e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -600,6 +600,11 @@ def test_server(): env["HERMES_WEBUI_TEST_NETWORK_BLOCK"] = "1" env.update({ "HERMES_WEBUI_WORKSPACE_GIT_DESTRUCTIVE": "1", + # Small archive-extraction cap so the zip-bomb guard is exercisable + # against the out-of-process test server (the real 10x-upload default is + # ~200MB — impractical to exceed in a test). 5MB is far above any other + # test's archive payload, so only the bomb test trips it. + "HERMES_WEBUI_MAX_EXTRACTED_MB": "5", "HERMES_WEBUI_PORT": str(TEST_PORT), "HERMES_WEBUI_HOST": "127.0.0.1", "HERMES_WEBUI_STATE_DIR": str(TEST_STATE_DIR), diff --git a/tests/test_workspace_upload.py b/tests/test_workspace_upload.py index 567fb0757..ee686d5a5 100644 --- a/tests/test_workspace_upload.py +++ b/tests/test_workspace_upload.py @@ -12,7 +12,6 @@ Covers the POST /api/workspace/upload handler: import io import json -import os import sys import uuid import urllib.request @@ -368,20 +367,26 @@ class TestWorkspaceUploadArchive: # Corrupt archive file should be removed assert not (ws / "corrupt.zip").exists() - def test_zip_bomb_cap_trips(self, cleanup_test_sessions, monkeypatch): - """When extraction exceeds the cap, it should be rejected and cleaned up.""" - from api import upload as upload_mod + def test_zip_bomb_cap_trips(self, cleanup_test_sessions): + """When extraction exceeds the cap, it should be rejected and cleaned up. + The test server runs with HERMES_WEBUI_MAX_EXTRACTED_MB=5 (set in + conftest), so a highly-compressible archive that extracts to >5MB trips + the byte-tracking zip-bomb guard. (Monkeypatching the cap in the pytest + process does nothing — extraction runs in the out-of-process server.) + """ sid, ws = make_session_tracked(cleanup_test_sessions) - # Set a very small extraction cap (100 bytes) - monkeypatch.setattr(upload_mod, "_MAX_EXTRACTED_BYTES", 100) + from api.config import MAX_UPLOAD_BYTES - # Create a zip with enough content to exceed 100 bytes when extracted + # ~6.4MB of zeros across two members — compresses to a tiny zip but + # exceeds the 5MB extraction cap during the chunked write. zip_data = _make_zip({ - "small.txt": b"x" * 80, # 80 bytes — under cap - "medium.txt": b"y" * 80, # +80 = 160 bytes — exceeds cap during extraction + "a.bin": b"\0" * (4 * 1024 * 1024), # 4MB — under cap + "b.bin": b"\0" * (4 * 1024 * 1024), # +4MB = 8MB — exceeds 5MB cap mid-extraction }) + # Sanity: the compressed archive itself must stay under the upload cap. + assert len(zip_data) < MAX_UPLOAD_BYTES, f"test zip too big to upload: {len(zip_data)}" result, status = post_multipart( "/api/workspace/upload", @@ -395,6 +400,5 @@ class TestWorkspaceUploadArchive: # Archive should be removed on failure assert not (ws / "bomb.zip").exists() - - # Cleanup: restore the original cap - monkeypatch.undo() + # No partial extraction directory left behind + assert not (ws / "bomb").exists()