From fbcae5f71e3af1b820ddd42e3361edbc8e11a4c9 Mon Sep 17 00:00:00 2001 From: nesquena-hermes <[email protected]> Date: Tue, 2 Jun 2026 02:20:05 +0000 Subject: [PATCH] fix: harden workspace upload surface (#3104 follow-up hotfix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex regression-gate findings on the shipped #3104 upload code, each verified with a repro and fixed: 1. Negative Content-Length bypassed the size cap → unbounded rfile.read(-1). The per-handler 'content_length > MAX_UPLOAD_BYTES' check is False for a negative value, so the guard is now centralized in parse_multipart() (validates [0, MAX_UPLOAD_BYTES]) — protects all four upload handlers. 2. .tar/.tbz2/.txz uploads silently skipped extraction (is_archive suffix set was narrower than extract_archive's) → now matches. 3. Rejected archives (zip-slip/zip-bomb/corrupt/too-many-members) showed a misleading 'Uploaded' success toast → workspace.js now surfaces extract_error. 4. An in-workspace symlink subpath let mkdir/writes escape the workspace root → target_dir is now required to be is_relative_to(workspace) before mkdir. Regression tests added (negative+oversize CL, .tar extraction, symlink target). --- CHANGELOG.md | 5 ++ api/upload.py | 28 +++++++++-- static/workspace.js | 8 +++ tests/test_workspace_upload.py | 90 ++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 3 deletions(-) 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..a57118f7 100644 --- a/api/upload.py +++ b/api/upload.py @@ -44,7 +44,19 @@ def parse_multipart(rfile, content_type, content_length) -> tuple: 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') + 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 +413,13 @@ 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. + if target_dir.resolve() != workspace.resolve() and 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 +453,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..7836b605 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,81 @@ 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: + 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")}, + ) + # Either a 403 escape rejection, or at minimum nothing written outside. + assert status == 403 or not (escape / "pwned.txt").exists(), ( + f"upload escaped workspace via symlink: status={status} result={result}" + ) + assert not (escape / "pwned.txt").exists()