mirror of
https://github.com/nesquena/hermes-webui.git
synced 2026-07-08 16:52:02 +00:00
fix(#3104): correct dedup filename reporting + make zip-bomb cap testable
Two issues caught by the PR's own tests against the out-of-process test server: 1. Dedup reporting bug: after a filename collision the file was correctly written to e.g. report-1.pdf, but the JSON response reported the ORIGINAL name (safe_name) — now reports dest.name. (Real user-facing bug.) 2. Zip-bomb cap was untestable: the test monkeypatched _MAX_EXTRACTED_BYTES in the pytest process, which has no effect on the separate server process where extraction runs. Made the cap env-configurable (HERMES_WEBUI_MAX_EXTRACTED_MB, read at call time via _max_extracted_bytes(); defaults to 10x upload cap), set it to 5MB in the conftest server env, and rewrote the test to upload a compressible archive that genuinely extracts past the cap. Also asserts no partial extraction dir is left behind. Plus lint: unused field_name loop var -> _field_name, unused os import in test.
This commit is contained in:
+33
-10
@@ -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,
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user