Merge pull request #3372 from nesquena/hotfix/upload-hardening

Release GB — v0.51.208 (workspace upload hardening hotfix)

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

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:

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);
     }

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)