diff --git a/CHANGELOG.md b/CHANGELOG.md index be70f72b4..5f720f72e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ ### Fixed +- **Uploading an Office document now writes a Markdown sidecar so the agent can read its text.** When you upload a `.docx`, `.xlsx`, or `.pptx` to a workspace, a best-effort `.md` sidecar with the extracted text is written alongside it (reusing the existing Office-preview extractor — no new dependency). The write is path-contained (`O_NOFOLLOW` openat-walk within the workspace), never overwrites an existing `.md` (`O_CREAT|O_EXCL`), and is best-effort — a sidecar failure leaves the uploaded Office file fully intact. Thanks @rodboev. (#5734, #540) + - **Security: the embedded-terminal local-origin gate can no longer be bypassed via a spoofed `X-Forwarded-For` header, and no longer locks out direct LAN clients.** Previously, with `HERMES_WEBUI_TRUST_FORWARDED_FOR=1`, the gate honored the forwarded client IP without first verifying the request actually came through a trusted proxy — so a public client connecting directly to a passwordless WebUI could send `X-Forwarded-For: 127.0.0.1` and reach the embedded terminal (which runs a PTY shell). The forwarded chain is now consulted **only when the un-spoofable raw socket peer is a trusted proxy** (loopback, or an address in the new `HERMES_WEBUI_TRUSTED_PROXY_CIDRS` allowlist), it consumes the full `X-Forwarded-For` chain right-to-left (skipping trusted-proxy hops to find the real client), and it fails closed on malformed/empty chains. Direct loopback/LAN clients with no proxy header remain local, so onboarding, first-password/passkey setup, and passwordless terminal access keep working on the common direct-LAN deployment. Thanks @Hinotoi-agent for the original report. (#5764) - **Workspace image previews and file downloads now work when the WebUI is served under a subpath.** Behind a reverse proxy that mounts the app at a subpath (e.g. `/hermes/`), workspace image previews and downloads 404'd: those consumers built root-relative `/api/...` URLs that resolved to the server root instead of the mount, while text/code previews (which route through `api()`) kept working — a confusing partial failure. The workspace route helper now resolves against `document.baseURI`, so every consumer (image preview, media/pdf/html frames, download link, open-in-browser) gets a correctly-mounted URL. The normal root-mount deployment is unaffected (identical URLs). Thanks @tomtong2015. (#5830) diff --git a/api/upload.py b/api/upload.py index 5cd87ae23..ba5999adb 100644 --- a/api/upload.py +++ b/api/upload.py @@ -4,12 +4,11 @@ Hermes Web UI -- File upload: multipart parser and upload handler. import mimetypes import os import re as _re -import email.parser import tempfile from pathlib import Path from api.config import MAX_UPLOAD_BYTES, STATE_DIR -from api.helpers import j, bad +from api.helpers import j from api.models import get_session from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name from api.workspace import ( @@ -75,7 +74,6 @@ def parse_multipart(rfile, content_type, content_length) -> tuple: fields = {} files = {} delimiter = b'--' + boundary - end_marker = b'--' + boundary + b'--' parts = raw.split(delimiter) for part in parts[1:]: stripped = part.lstrip(b'\r\n') @@ -166,6 +164,45 @@ def _reject_invisible_session(handler, session) -> bool: return True +def _write_office_upload_sidecar(workspace: Path, dest: Path, file_bytes: bytes) -> dict | None: + """Write a Markdown preview sidecar for supported Office uploads.""" + if dest.suffix.lower() not in {'.docx', '.xlsx', '.pptx'}: + return None + + error_message = 'Office sidecar extraction failed' + sidecar = dest.with_name(f'{dest.name}.md') + sidecar_path = sidecar.resolve() + created_sidecar = False + try: + from api.office_documents import preview_office_document + + preview = preview_office_document(dest.name, file_bytes) + if not sidecar_path.is_relative_to(workspace.resolve()): + raise ValueError('Invalid sidecar destination') + sidecar_fd = open_anchored_create_fd(workspace, sidecar_path) + created_sidecar = True + with os.fdopen(sidecar_fd, 'w', encoding='utf-8', closefd=True) as sidecar_file: + sidecar_file.write(str(preview.get('content') or '')) + return { + 'filename': sidecar_path.name, + 'path': str(sidecar_path), + 'size': sidecar_path.stat().st_size, + 'preview_kind': preview.get('preview_kind'), + 'office_format': preview.get('office_format'), + } + except FileExistsError: + return {'error': error_message} + except Exception: + if created_sidecar: + try: + unlink_anchored(workspace, sidecar_path) + except FileNotFoundError: + pass + except Exception: + pass + return {'error': error_message} + + def handle_upload(handler): import traceback as _tb try: @@ -210,7 +247,7 @@ def extract_archive(file_bytes: bytes, filename: str, workspace: Path): Returns a dict with ``extracted`` (int), ``files`` (list[str]). Raises ValueError on zip-slip or unsupported format. """ - import zipfile, tarfile, io, os, shutil + import zipfile, tarfile, io, os cap = _max_extracted_bytes() name = Path(filename).name @@ -708,6 +745,7 @@ def handle_workspace_upload(handler): }) continue + sidecar = _write_office_upload_sidecar(workspace, dest, file_bytes) results.append({ 'filename': dest.name, 'path': str(dest), @@ -715,6 +753,8 @@ def handle_workspace_upload(handler): 'mime': mime, 'is_image': mime.startswith('image/'), 'extracted': False, + **({'sidecar': sidecar} if sidecar and 'error' not in sidecar else {}), + **({'sidecar_error': sidecar['error']} if sidecar and 'error' in sidecar else {}), }) if len(results) == 1: diff --git a/tests/test_workspace_upload.py b/tests/test_workspace_upload.py index 440bccbb6..b71790ff7 100644 --- a/tests/test_workspace_upload.py +++ b/tests/test_workspace_upload.py @@ -18,6 +18,9 @@ import urllib.request import urllib.error import pathlib import zipfile +from types import SimpleNamespace + +import pytest sys.path.insert(0, str(pathlib.Path(__file__).parent.parent.parent)) @@ -99,6 +102,84 @@ def _make_tar(members: dict[str, bytes], mode: str = "w") -> bytes: return buf.getvalue() +class _FakeUploadHandler: + def __init__(self, content_length: int = 0): + self.status = None + self.sent_headers = [] + self.rfile = io.BytesIO() + self.wfile = io.BytesIO() + self.headers = { + "Content-Type": "multipart/form-data; boundary=test", + "Content-Length": str(content_length), + } + + def send_response(self, status): + self.status = status + + def send_header(self, name, value): + self.sent_headers.append((name, value)) + + def end_headers(self): + pass + + def json_body(self): + return json.loads(self.wfile.getvalue().decode("utf-8")) + + +def _configure_direct_office_upload(monkeypatch, tmp_path): + import api.office_documents as office_documents + import api.upload as upload + + workspace = tmp_path / "workspace" + workspace.mkdir() + session = SimpleNamespace(workspace=workspace, profile="default") + + monkeypatch.setattr(upload, "get_session", lambda _sid: session) + monkeypatch.setattr(upload, "_reject_invisible_session", lambda *_args: False) + monkeypatch.setattr(upload, "resolve_trusted_workspace", lambda path: path) + return upload, office_documents, workspace + + +def _set_office_upload_payload(monkeypatch, upload, filename, file_bytes): + monkeypatch.setattr( + upload, + "parse_multipart", + lambda *_args, **_kwargs: ( + {"session_id": "session-1", "path": ""}, + {"file": (filename, file_bytes)}, + ), + ) + + +def _set_office_preview(monkeypatch, office_documents, *, content, preview_kind, office_format): + monkeypatch.setattr( + office_documents, + "preview_office_document", + lambda path, raw: { + "path": path, + "content": content, + "preview_kind": preview_kind, + "office_format": office_format, + "size": len(raw), + }, + ) + + +class _FailingSidecarWriter: + def __init__(self, inner): + self._inner = inner + + def write(self, _data): + raise OSError("sidecar write failed") + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + self._inner.close() + return False + + # ── Health check ────────────────────────────────────────────────────────── def test_health(): @@ -126,12 +207,235 @@ class TestWorkspaceUploadHappyPath: assert result["filename"] == "hello.txt" assert result["size"] == len(content) assert result["extracted"] is False + assert "sidecar" not in result + assert "sidecar_error" not in result # Verify file actually exists in the workspace uploaded = ws / "hello.txt" assert uploaded.exists(), f"File not found at {uploaded}" assert uploaded.read_bytes() == content + def test_upload_docx_writes_sidecar_and_metadata(self, monkeypatch, tmp_path): + """Office uploads should write a Markdown sidecar and surface it.""" + docx_bytes = b"fake-docx-bytes" + + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + _set_office_upload_payload(monkeypatch, upload, "report.docx", docx_bytes) + _set_office_preview( + monkeypatch, + office_documents, + content="# Preview\n\nOffice upload", + preview_kind="office-preview", + office_format="docx", + ) + + handler = _FakeUploadHandler(content_length=len(docx_bytes)) + upload.handle_workspace_upload(handler) + + assert handler.status == 200 + result = handler.json_body() + assert result["filename"] == "report.docx" + assert result["sidecar"]["filename"] == "report.docx.md" + assert result["sidecar"]["preview_kind"] == "office-preview" + assert result["sidecar"]["office_format"] == "docx" + + uploaded = workspace / "report.docx" + sidecar = workspace / "report.docx.md" + assert uploaded.exists() + assert uploaded.read_bytes() == docx_bytes + assert sidecar.exists() + assert sidecar.read_text(encoding="utf-8") == "# Preview\n\nOffice upload" + + def test_upload_docx_preview_failure_returns_generic_sidecar_error(self, monkeypatch, tmp_path): + """Preview failures should not leak raw errors or paths.""" + docx_bytes = b"fake-docx-bytes" + + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + _set_office_upload_payload(monkeypatch, upload, "report.docx", docx_bytes) + + def _raise_preview(_path, _raw): + raise ValueError(f"preview failed for {workspace}") + + monkeypatch.setattr(office_documents, "preview_office_document", _raise_preview) + + handler = _FakeUploadHandler(content_length=len(docx_bytes)) + upload.handle_workspace_upload(handler) + + assert handler.status == 200 + result = handler.json_body() + assert result["sidecar_error"] == "Office sidecar extraction failed" + assert str(workspace) not in result["sidecar_error"] + assert "preview failed" not in result["sidecar_error"] + + uploaded = workspace / "report.docx" + sidecar = workspace / "report.docx.md" + assert uploaded.exists() + assert uploaded.read_bytes() == docx_bytes + assert not sidecar.exists() + + @pytest.mark.parametrize( + "filename, file_bytes, content, office_format", + [ + ("report.xlsx", b"fake-xlsx-bytes", "# Preview\n\nWorkbook", "xlsx"), + ("slides.pptx", b"fake-pptx-bytes", "# Preview\n\nDeck", "pptx"), + ], + ) + def test_upload_office_xlsx_and_pptx_write_sidecars( + self, + monkeypatch, + tmp_path, + filename, + file_bytes, + content, + office_format, + ): + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + _set_office_upload_payload(monkeypatch, upload, filename, file_bytes) + _set_office_preview( + monkeypatch, + office_documents, + content=content, + preview_kind="office-preview", + office_format=office_format, + ) + + handler = _FakeUploadHandler(content_length=len(file_bytes)) + upload.handle_workspace_upload(handler) + + assert handler.status == 200 + result = handler.json_body() + assert result["filename"] == filename + assert result["sidecar"]["filename"] == f"{filename}.md" + assert result["sidecar"]["preview_kind"] == "office-preview" + assert result["sidecar"]["office_format"] == office_format + + uploaded = workspace / filename + sidecar = workspace / f"{filename}.md" + assert uploaded.exists() + assert uploaded.read_bytes() == file_bytes + assert sidecar.exists() + assert sidecar.read_text(encoding="utf-8") == content + + def test_upload_docx_sidecar_write_failure_cleans_up_and_keeps_upload_success(self, monkeypatch, tmp_path): + """Sidecar write failures must not fail the original upload.""" + docx_bytes = b"fake-docx-bytes" + + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + _set_office_upload_payload(monkeypatch, upload, "report.docx", docx_bytes) + _set_office_preview( + monkeypatch, + office_documents, + content="# Preview\n\nOffice upload", + preview_kind="office-preview", + office_format="docx", + ) + + real_fdopen = upload.os.fdopen + fdopen_calls = {"count": 0} + + def _failing_fdopen(fd, *args, **kwargs): + fdopen_calls["count"] += 1 + writer = real_fdopen(fd, *args, **kwargs) + if fdopen_calls["count"] == 2: + return _FailingSidecarWriter(writer) + return writer + + monkeypatch.setattr(upload.os, "fdopen", _failing_fdopen) + + handler = _FakeUploadHandler(content_length=len(docx_bytes)) + upload.handle_workspace_upload(handler) + + assert handler.status == 200 + result = handler.json_body() + assert result["filename"] == "report.docx" + assert result["sidecar_error"] == "Office sidecar extraction failed" + assert "sidecar write failed" not in result["sidecar_error"] + assert "sidecar" not in result + + uploaded = workspace / "report.docx" + sidecar = workspace / "report.docx.md" + assert uploaded.exists() + assert uploaded.read_bytes() == docx_bytes + assert not sidecar.exists() + + def test_upload_docx_sidecar_collision_preserves_existing_sidecar(self, monkeypatch, tmp_path): + """A pre-existing Markdown sidecar must survive a create collision.""" + docx_bytes = b"fake-docx-bytes" + + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + _set_office_upload_payload(monkeypatch, upload, "report.docx", docx_bytes) + _set_office_preview( + monkeypatch, + office_documents, + content="# Preview\n\nOffice upload", + preview_kind="office-preview", + office_format="docx", + ) + + sidecar = workspace / "report.docx.md" + sidecar.write_text("keep this", encoding="utf-8") + + handler = _FakeUploadHandler(content_length=len(docx_bytes)) + upload.handle_workspace_upload(handler) + + assert handler.status == 200 + result = handler.json_body() + assert result["filename"] == "report.docx" + assert "sidecar" not in result + assert result["sidecar_error"] == "Office sidecar extraction failed" + assert "report.docx.md" not in result["sidecar_error"] + + uploaded = workspace / "report.docx" + assert uploaded.exists() + assert uploaded.read_bytes() == docx_bytes + assert sidecar.read_text(encoding="utf-8") == "keep this" + + def test_upload_docx_duplicate_names_get_deduped_sidecars(self, monkeypatch, tmp_path): + """Repeated Office uploads should dedupe both the file and its sidecar.""" + upload, office_documents, workspace = _configure_direct_office_upload(monkeypatch, tmp_path) + office_uploads = iter([b"first office payload", b"second office payload"]) + preview_paths = [] + + def _next_upload(*_args, **_kwargs): + payload = next(office_uploads) + return ( + {"session_id": "session-1", "path": ""}, + {"file": ("report.docx", payload)}, + ) + + def _preview(path, raw): + preview_paths.append(path) + return { + "path": path, + "content": f"# {path}\n\n{raw.decode('utf-8')}", + "preview_kind": "office-preview", + "office_format": "docx", + "size": len(raw), + } + + monkeypatch.setattr(upload, "parse_multipart", _next_upload) + monkeypatch.setattr(office_documents, "preview_office_document", _preview) + + handler1 = _FakeUploadHandler(content_length=len(b"first office payload")) + upload.handle_workspace_upload(handler1) + handler2 = _FakeUploadHandler(content_length=len(b"second office payload")) + upload.handle_workspace_upload(handler2) + + assert handler1.status == 200 + assert handler2.status == 200 + result1 = handler1.json_body() + result2 = handler2.json_body() + assert result1["filename"] == "report.docx" + assert result1["sidecar"]["filename"] == "report.docx.md" + assert result2["filename"] == "report-1.docx" + assert result2["sidecar"]["filename"] == "report-1.docx.md" + assert preview_paths == ["report.docx", "report-1.docx"] + + assert (workspace / "report.docx").read_bytes() == b"first office payload" + assert (workspace / "report.docx.md").read_text(encoding="utf-8") == "# report.docx\n\nfirst office payload" + assert (workspace / "report-1.docx").read_bytes() == b"second office payload" + assert (workspace / "report-1.docx.md").read_text(encoding="utf-8") == "# report-1.docx\n\nsecond office payload" + def test_upload_into_subdirectory(self, cleanup_test_sessions): """Upload a file into a subdirectory within the workspace.""" sid, ws = make_session_tracked(cleanup_test_sessions) @@ -283,7 +587,7 @@ class TestWorkspaceUploadOversized: {"file": ("big.bin", big)}, ) assert status == 413, f"Expected 413, got {status}: {result}" - except (urllib.error.URLError, ConnectionResetError, BrokenPipeError): + except (urllib.error.URLError, ConnectionResetError, ConnectionAbortedError, BrokenPipeError): # Server may close connection after reading Content-Length > limit pass @@ -317,6 +621,8 @@ class TestWorkspaceUploadArchive: # Archive file itself should be removed after extraction assert not (ws / "projects" / "vendor.zip").exists() + assert "sidecar" not in result + assert "sidecar_error" not in result def test_zip_extracts_to_workspace_root_when_no_subpath(self, cleanup_test_sessions): """Zip uploaded without subpath extracts to workspace root."""