Feat/workspace upload

[antoniocarlos97ss]

Summary

Adds a file upload button to the workspace file-panel toolbar that POSTs to a new POST /api/workspace/upload endpoint. The handler:

• Writes uploaded files into the session's workspace directory (resolved via resolve_trusted_workspace)
• Deduplicates filenames with -1/-2 suffixes
• Auto-extracts archives (.zip/.tar.*) into the target subdirectory with zip-bomb and zip-slip protections
• Extraction errors are surfaced to the frontend (not silently swallowed)
• Failed archive extractions clean up the archive file

Changes

• api/upload.py: New handle_workspace_upload handler (109 lines)
• api/routes.py: Route registration
• static/workspace.js: Upload button + drag-drop onto file tree
• CHANGELOG.md: Entry under [Unreleased]
• tests/test_workspace_upload.py: 12 tests covering happy path, dedup, path traversal, oversized rejection, archive extraction containment, zip-slip blocking, corrupt archive handling, and zip-bomb cap

Review fixes (addressing @nesquena-hermes feedback)

1. ✅ Archive extraction target: now extracts into target_dir (not workspace root)
2. ✅ Error handling: replaced overbroad except (ValueError, Exception) with specific exceptions (zipfile.BadZipFile, tarfile.TarError, ValueError) + fallback Exception; errors are surfaced to frontend
3. ✅ Zip-bomb guard: extract_archive already enforced _MAX_EXTRACTED_BYTES; extraction failures now clean up the archive file
4. ✅ Path traversal: documented safe_resolve_ws as authoritative guard
5. ✅ Tests: 12 comprehensive tests added
6. ✅ CHANGELOG entry added

[nesquena-hermes]

Review — workspace upload feature, security shape needs tightening

Reading the diff against api/upload.py:323-429 (new handle_workspace_upload), api/routes.py:5051-5052 (route registration), and static/workspace.js:636-707 (file picker + drag-drop). The feature shape is right — workspace gets a file uploader, drag-drop onto the file tree works, archives are extracted, image MIME is flagged. Three concerns before this should land.

1. Multi-file uploads return 403 on a single bad name and abort the whole batch

At api/upload.py:368-371:

for field_name, (filename, file_bytes) in files.items():
    if not filename:
        continue
    safe_name = _sanitize_upload_name(filename)
    dest = safe_resolve_ws(target_dir, safe_name)
    if not dest.resolve().is_relative_to(workspace.resolve()):
        return j(handler, {'error': f'Path traversal blocked: {safe_name}'}, status=403)

If a user drags 10 files onto the tree and any one has a name that _sanitize_upload_name somehow can't pin inside the workspace, the loop returns mid-iteration with no record of which earlier files already wrote to disk. The frontend at static/workspace.js:670 then shows a single 403 toast and loadDir(S.currentDir) repaints, but the user has no idea half their upload succeeded. Either:

• Accumulate per-file errors into results and always return a 200 with {files: [...], errors: [...]}, or
• Buffer all dest paths up front, validate them all, then write — atomic-ish.

The agent-side safe_resolve_ws at api/workspace.py:653-681 already raises ValueError on traversal, so the explicit .is_relative_to re-check at upload.py:369 is belt-and-suspenders. The real failure mode here is the partial-write, not the resolve.

2. Archive extraction doesn't validate that _session_attachment_dir semantics match resolve_trusted_workspace

At api/upload.py:386-401:

is_archive = safe_name.lower().endswith(('.zip', '.tar.gz', '.tgz', '.tar.bz2', '.tar.xz'))
if is_archive:
    try:
        extract_archive(file_bytes, safe_name, workspace)
        dest.unlink(missing_ok=True)

extract_archive is defined at api/upload.py:141-200 and calls safe_resolve_ws(workspace, stem) to compute an extraction subdirectory. But this PR passes workspace (the trusted root from resolve_trusted_workspace), not the per-target subdirectory the user dragged onto. So if a user is browsing ~/repo/src/components and drops vendor.zip, the archive extracts to ~/repo/vendor/ instead of ~/repo/src/components/vendor/. That's a confusing UX regression vs. the regular file-write path, which respects target_dir.

Fix: pass target_dir instead of workspace to extract_archive. The existing safe_resolve_ws(workspace, stem) inside extract_archive resolves relative to its arg, so if we pass it target_dir it'll naturally extract into the subfolder the user was browsing. And safe_resolve_ws will still block traversal because target_dir itself was resolved through safe_resolve_ws(workspace, subpath) at line 357.

# Suggested fix at line 388
extract_archive(file_bytes, safe_name, target_dir)

Also: the bare except (ValueError, Exception) at line 399 silently swallows extraction errors and reports extracted: False with no diagnostic. Users dragging a corrupt zip will see "uploaded successfully" but no extracted contents and no error toast. At minimum log the exception; ideally surface it on the result entry.

3. The retry-name loop blocks legitimate concurrent uploads

At api/upload.py:373-385:

if dest.exists():
    stem = dest.stem
    suffix = dest.suffix
    for idx in range(1, 1000):
        candidate = safe_resolve_ws(target_dir, f'{stem}-{idx}{suffix}')
        ...
        if not candidate.exists():
            dest = candidate
            break
    else:
        return j(handler, {'error': 'Too many uploads with the same filename'}, status=400)

The exists() → write_bytes() is a TOCTOU race — two parallel uploads of report.pdf can both see report.pdf doesn't exist after report-3.pdf was the previous max, both pick report-4.pdf, both write, one wins. For workspace uploads this is unlikely but the safer pattern is os.open(path, O_CREAT | O_EXCL) in the loop body to atomically claim a name.

Smaller things

• mime = mimetypes.guess_type(safe_name)[0] or 'application/octet-stream' is also computed inside the is_archive branch (line 391) but the resulting mime is then included in a payload that says extracted: True — which is contradictory. If the file was extracted (and the archive deleted), the MIME of the deleted archive isn't useful info. Drop mime from the extracted-archive result, or at least mark it null.
• target_dir.mkdir(parents=True, exist_ok=True) at line 359 will create directory trees outside what the user requested if subpath contains intermediate directories that don't exist. Worth confirming this is intentional — if someone POSTs path=a/b/c to an empty workspace, you'll create three nested dirs. Probably fine for a file upload feature but worth documenting.
• No CHANGELOG entry. The PR body is empty.
• No tests. Compare to PR fix: forward Gateway tool activity to WebUI #3098 / feat: add advanced model options #3097 which both ship comprehensive regression tests. This file adds 109 lines of upload-handling code and a new POST route with security implications — needs at least:
  • Path-traversal-via-filename test (file named ../foo.txt)
  • Path-traversal-via-subpath test (subpath = ../../etc)
  • Archive-with-zip-slip test
  • Dedup-name collision test
  • Drag-drop multi-file test for the frontend (mock FormData + drop event)

Verdict

The user-facing feature is genuinely nice — drag-drop into the workspace panel has been a missing affordance. But this needs the three concrete fixes above (archive target-dir, partial-write, race) plus tests before it's safe to merge. Happy to re-review once those land.

[nesquena-hermes]

Thanks for this, @antoniocarlos97ss — a workspace-panel upload button is a genuinely useful affordance. Before we can move it toward review, though, it needs several changes. Marking changes-requested + hold until these are addressed.

What this PR does (for context)

It adds an "Upload file" button to the workspace file-panel toolbar that POSTs to a new POST /api/workspace/upload endpoint. The handler writes the uploaded file(s) into the session's workspace directory (resolved via resolve_trusted_workspace), de-duplicates filenames with -1/-2 suffixes, and auto-extracts archives (.zip/.tar.*).

Required changes before review

1. Don't swallow extraction errors with except (ValueError, Exception). That clause is equivalent to bare except Exception (the ValueError is redundant), and the repo style bans over-broad excepts. Catch the specific archive errors (zipfile.BadZipFile, tarfile.TarError, ValueError) and surface a real error to the user instead of silently keeping the archive with no feedback.

2. Archive extraction target looks wrong. extract_archive(file_bytes, safe_name, workspace) extracts to the workspace root, ignoring the subpath the user uploaded into. A zip dropped into subdir/ should extract under subdir/, not the workspace root. Please extract into target_dir (and confirm extract_archive still enforces the workspace-root containment check for each member).

3. Cap extracted size (zip-bomb guard). The sibling handle_upload_extract path enforces _MAX_EXTRACTED_BYTES (10× MAX_UPLOAD_BYTES). This new path has no equivalent cap, so a small archive can write unbounded bytes into the workspace. Please enforce the same total-extracted-bytes limit and reject (clean up partial output) when exceeded.

4. Add tests. Uploads and path-traversal/archive handling are security-sensitive; the repo requires tests for non-trivial logic. Please cover: (a) happy-path upload writes the file into the workspace, (b) filename de-dup produces -1/-2, (c) a ../-style traversal name is blocked with 403, (d) oversized body → 413, (e) archive extraction stays within the workspace (no member escapes the root), (f) the zip-bomb cap from fix(frontend): use URL origin for fetch/EventSource to support revers… #3 trips. There's an existing harness pattern in tests/test_*upload* you can mirror.

5. Add a PR description + CHANGELOG entry. The PR currently has no description. Please add a short summary and a user-visible CHANGELOG.md [Unreleased] bullet (per AGENTS.md).

6. Path-traversal guard nit. safe_resolve_ws already resolves within the workspace, so the extra dest.resolve().is_relative_to(...) checks are belt-and-suspenders — that's fine to keep, but please confirm safe_resolve_ws is the single authoritative guard and the manual checks aren't masking a case where safe_resolve_ws would otherwise raise.

Once these are in (especially the tests and the extraction-target/zip-bomb fixes), re-request review and we'll take another look. Appreciate the contribution!

[antoniocarlos97ss]

@nesquena-hermes thanks for the thorough review! All six concerns have been addressed:

1. Extraction errors: Replaced except (ValueError, Exception) with specific catches (zipfile.BadZipFile, tarfile.TarError, ValueError) + fallback Exception. Errors are now surfaced to the frontend via extract_error field instead of being silently swallowed. The archive file is removed on failure (no partial content left behind).

2. Archive target: Extraction now uses target_dir instead of workspace root — archives drop where the user intends, matching the regular file-write path.

3. Zip-bomb cap: The underlying extract_archive() already enforced _MAX_EXTRACTED_BYTES per-chunk for both zip and tar paths (the check exists at lines 184-203 and 217-238). The new handler path now properly cleans up the archive and surfaces the error on cap trip (test included).

4. Tests: Added test_workspace_upload.py with 12 tests covering: happy path (single file, subdirectory, image MIME), filename dedup, path traversal (dotdot filename, subpath traversal), oversized rejection (413), archive extraction containment, zip-slip blocking, corrupt zip error surfacing, and zip-bomb cap enforcement.

5. PR description + CHANGELOG: Done.

6. Path traversal guard: Added a comment documenting that safe_resolve_ws is the authoritative guard and the manual .is_relative_to is belt-and-suspenders.

Ready for re-review!

[nesquena-hermes]

Shipped in v0.51.206 (Release FZ) via release PR #3367 (merge f0c3668d). Thanks @antoniocarlos97ss! The workspace Upload button + drag-and-drop are now live, with archive auto-extraction guarded by zip-slip, zip-bomb (size + member-count caps), and path-traversal protections.

Cherry-picked onto current master (the branch was ~423 commits behind) and applied a few maintainer fixes during review: corrected the dedup filename reported in the response, made the extraction size-cap env-tunable (HERMES_WEBUI_MAX_EXTRACTED_MB) + actually testable, added an archive member-count cap, and added the missing uploading/uploaded i18n keys across all locales. Your six earlier review fixes all held up. Closing as merged — credit preserved.