diff --git a/CHANGELOG.md b/CHANGELOG.md index 56444dc2..80ab2a65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,11 @@ ## [Unreleased] +## [v0.51.208] — 2026-06-02 — Release GB (workspace upload hardening hotfix) + +### Fixed +- Hardened the workspace file-upload surface (#3104 follow-up): (1) a negative `Content-Length` no longer bypasses the size cap and triggers an unbounded `rfile.read(-1)` — the length is now validated `[0, MAX_UPLOAD_BYTES]` centrally in `parse_multipart` for every upload handler; (2) `.tar`, `.tbz2`, and `.txz` archives now auto-extract (the upload handler's archive-suffix set was narrower than `extract_archive`'s, so those silently landed as raw files); (3) a rejected archive (zip-slip / zip-bomb / corrupt / too-many-members) now surfaces an error toast in the workspace panel instead of a misleading "Uploaded" success; (4) an in-workspace symlink subpath can no longer make the upload target `mkdir`/write outside the workspace root. Regression tests added. + ## [v0.51.207] — 2026-06-02 — Release GA (Edge TTS as an alternative speech engine) ### Added diff --git a/api/upload.py b/api/upload.py index 2b7fd62d..9399c197 100644 --- a/api/upload.py +++ b/api/upload.py @@ -40,11 +40,30 @@ _MAX_EXTRACTED_BYTES = 10 * MAX_UPLOAD_BYTES def parse_multipart(rfile, content_type, content_length) -> tuple: import re as _re, email.parser as _ep + # Imported locally (not just module-level) so the function stays + # self-contained — some tests exec() this function's source in an isolated + # namespace, and a bare module global would NameError there. + try: + from api.config import MAX_UPLOAD_BYTES as _MAX_UPLOAD_BYTES + except Exception: + _MAX_UPLOAD_BYTES = 20 * 1024 * 1024 m = _re.search(r'boundary=([^;\s]+)', content_type) if not m: raise ValueError('No boundary in Content-Type') boundary = m.group(1).strip('"').encode() - raw = rfile.read(content_length) + # Centralized length guard for ALL upload callers: a missing/garbage or + # NEGATIVE Content-Length must never reach rfile.read(<0), which reads the + # stream unbounded (read(-1) == read-to-EOF) and bypasses the per-handler + # size cap. Reject anything not in [0, MAX_UPLOAD_BYTES]. + try: + length = int(content_length) + except (TypeError, ValueError): + raise ValueError('Invalid Content-Length') from None + if length < 0: + raise ValueError('Invalid Content-Length (negative)') + if length > _MAX_UPLOAD_BYTES: + raise ValueError(f'Upload too large (max {_MAX_UPLOAD_BYTES} bytes)') + raw = rfile.read(length) fields = {} files = {} delimiter = b'--' + boundary @@ -401,6 +420,14 @@ def handle_workspace_upload(handler): # Resolve target subdirectory within workspace target_dir = safe_resolve_ws(workspace, subpath) if subpath else workspace + # safe_resolve_ws intentionally permits in-workspace symlinks pointing + # outside the root (read trust model). For an UPLOAD target that's not + # acceptable: a planted symlink subpath would let mkdir() + writes create + # files OUTSIDE the workspace. Require the resolved target to be inside + # the workspace before creating anything. (is_relative_to is True for the + # workspace==target equality case, so the normal subpath='' path passes.) + if not target_dir.resolve().is_relative_to(workspace.resolve()): + return j(handler, {'error': 'Upload target escapes workspace'}, status=403) target_dir.mkdir(parents=True, exist_ok=True) results = [] @@ -434,8 +461,11 @@ def handle_workspace_upload(handler): dest.write_bytes(file_bytes) mime = mimetypes.guess_type(safe_name)[0] or 'application/octet-stream' - # For archives, optionally extract into the target directory - is_archive = safe_name.lower().endswith(('.zip', '.tar.gz', '.tgz', '.tar.bz2', '.tar.xz')) + # For archives, optionally extract into the target directory. + # Suffix set MUST match extract_archive()'s supported formats, else + # accepted-but-unlisted archives (.tar/.tbz2/.txz) silently land as + # raw files instead of extracting. + is_archive = safe_name.lower().endswith(('.zip', '.tar', '.tar.gz', '.tgz', '.tar.bz2', '.tbz2', '.tar.xz', '.txz')) if is_archive: import zipfile, tarfile, traceback as _extract_tb try: diff --git a/static/workspace.js b/static/workspace.js index 874931ed..3e9906bf 100644 --- a/static/workspace.js +++ b/static/workspace.js @@ -801,6 +801,14 @@ async function uploadToWorkspace(file, dir) { }); if (data && data.error) { showToast(data.error, 5000, 'error'); + } else if (data && (data.extract_error || (Array.isArray(data.files) && data.files.some(function(f){return f && f.extract_error;})))) { + // Archive was rejected (zip-slip / zip-bomb / corrupt / too-many-members): + // the file uploaded but extraction failed. Surface it as an error instead + // of a misleading "Uploaded" success toast. + var msg = data.extract_error + || (data.files.find(function(f){return f && f.extract_error;}) || {}).extract_error + || 'Archive extraction failed'; + showToast(msg, 5000, 'error'); } else { showToast(t('uploaded') || ('Uploaded ' + (data.filename || file.name)), 2000); } diff --git a/tests/test_workspace_upload.py b/tests/test_workspace_upload.py index 6f8363d8..25cfb73a 100644 --- a/tests/test_workspace_upload.py +++ b/tests/test_workspace_upload.py @@ -87,6 +87,18 @@ def _make_zip(members: dict[str, bytes]) -> bytes: return buf.getvalue() +def _make_tar(members: dict[str, bytes], mode: str = "w") -> bytes: + """Create a tar archive in memory (mode 'w' = uncompressed .tar).""" + import tarfile + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode=mode) as tf: + for name, data in members.items(): + info = tarfile.TarInfo(name=name) + info.size = len(data) + tf.addfile(info, io.BytesIO(data)) + return buf.getvalue() + + # ── Health check ────────────────────────────────────────────────────────── def test_health(): @@ -427,3 +439,84 @@ class TestWorkspaceUploadArchive: assert "extract_error" in result assert not (ws / "many.zip").exists() assert not (ws / "many").exists() + + +# ── Hardening regression tests (v0.51.208 hotfix) ────────────────────────── + +def test_parse_multipart_rejects_negative_content_length(): + """Negative Content-Length must not reach rfile.read(<0) (unbounded read). + + The per-handler `content_length > MAX_UPLOAD_BYTES` gate is False for a + negative value, so the guard has to live in parse_multipart itself (the + shared chokepoint for every upload handler). + """ + from api.upload import parse_multipart + big = b"x" * (2 * 1024 * 1024) + rfile = io.BytesIO( + b"--b\r\nContent-Disposition: form-data; name=\"f\"\r\n\r\n" + big + b"\r\n--b--\r\n" + ) + try: + parse_multipart(rfile, "multipart/form-data; boundary=b", -1) + assert False, "negative Content-Length should have been rejected" + except ValueError as e: + assert "Content-Length" in str(e) + # The stream must not have been drained by an unbounded read. + assert rfile.tell() == 0 + + +def test_parse_multipart_rejects_oversize_content_length(): + from api.config import MAX_UPLOAD_BYTES + from api.upload import parse_multipart + rfile = io.BytesIO(b"ignored") + try: + parse_multipart(rfile, "multipart/form-data; boundary=b", MAX_UPLOAD_BYTES + 1) + assert False, "oversize Content-Length should have been rejected" + except ValueError as e: + assert "too large" in str(e).lower() + + +class TestWorkspaceUploadArchiveSuffixes: + def test_plain_tar_is_extracted(self, cleanup_test_sessions): + """A `.tar` (and .tbz2/.txz) upload must extract, matching extract_archive's + supported set — previously `is_archive` omitted them so they landed raw.""" + sid, ws = make_session_tracked(cleanup_test_sessions) + tar_data = _make_tar({"docs/readme.txt": b"hello from tar"}) + result, status = post_multipart( + "/api/workspace/upload", + {"session_id": sid, "path": ""}, + {"file": ("bundle.tar", tar_data)}, + ) + assert status == 200, f"Upload failed {status}: {result}" + assert result["extracted"] is True + assert result.get("extracted_count", 0) >= 1 + # The raw .tar should have been removed after successful extraction. + assert not (ws / "bundle.tar").exists() + + +class TestWorkspaceUploadSymlinkTarget: + def test_symlink_subpath_target_is_rejected(self, cleanup_test_sessions): + """An in-workspace symlink subdir pointing outside the workspace must not + let the upload target (mkdir + writes) escape the workspace root.""" + import os + sid, ws = make_session_tracked(cleanup_test_sessions) + escape = ws.parent / f"escape-{uuid.uuid4().hex[:8]}" + escape.mkdir(parents=True, exist_ok=True) + link = ws / "outlink" + try: + try: + os.symlink(str(escape), str(link)) + except (OSError, NotImplementedError): + import pytest + pytest.skip("symlinks not supported in this environment") + result, status = post_multipart( + "/api/workspace/upload", + {"session_id": sid, "path": "outlink"}, + {"file": ("pwned.txt", b"should not land outside")}, + ) + # The escaping target must be rejected outright, and nothing may land + # outside the workspace. + assert status == 403, f"expected 403, got status={status} result={result}" + assert not (escape / "pwned.txt").exists() + finally: + import shutil + shutil.rmtree(escape, ignore_errors=True)