fix(office-ui): serve attachments saved under any project

opc/plugins/office_ui/server.py

@@ -217,23 +217,37 @@ async def _serve_spa_fallback(request: aiohttp.web.Request) -> aiohttp.web.Respo
 
 def _make_attachment_handler(engine: OPCEngine):
     """Factory that returns an HTTP handler for serving stored attachments."""
-    import mimetypes as _mt
 
-    async def _handle(request: aiohttp.web.Request) -> aiohttp.web.Response:
+    def _is_safe_component(part: str) -> bool:
+        return bool(part) and "/" not in part and "\\" not in part and ".." not in part
+
+    async def _handle(request: aiohttp.web.Request) -> aiohttp.web.StreamResponse:
         attachment_id = request.match_info["attachment_id"]
         filename = request.match_info["filename"]
-        att_store = getattr(engine, "attachment_store", None)
-        if not att_store:
-            return aiohttp.web.Response(status=503, text="Attachment store not available")
-        file_path = att_store.base_dir / attachment_id / filename
-        if not file_path.is_file():
-            return aiohttp.web.Response(status=404, text="Not found")
-        # Path-traversal guard
-        if not _is_under_path(file_path.resolve(), att_store.base_dir.resolve()):
+        if not (_is_safe_component(attachment_id) and _is_safe_component(filename)):
             return aiohttp.web.Response(status=403, text="Forbidden")
-        ct, _ = _mt.guess_type(filename)
-        headers = {"Cache-Control": "public, max-age=86400"}
-        return aiohttp.web.FileResponse(file_path, headers=headers)
+
+        # Attachments are written by the per-project engine that handled the
+        # upload (projects/{pid}/attachments/...), so the file may live under
+        # any project's dir — not only the root engine's active one.
+        candidates: list[Path] = []
+        att_store = getattr(engine, "attachment_store", None)
+        if att_store:
+            candidates.append(att_store.base_dir / attachment_id / filename)
+        projects_root = Path(engine.opc_home) / "projects"
+        if projects_root.is_dir():
+            for project_dir in sorted(projects_root.iterdir()):
+                candidates.append(project_dir / "attachments" / attachment_id / filename)
+
+        for file_path in candidates:
+            if not file_path.is_file():
+                continue
+            attachments_root = file_path.parent.parent
+            if not _is_under_path(file_path.resolve(), attachments_root.resolve()):
+                continue
+            headers = {"Cache-Control": "public, max-age=86400"}
+            return aiohttp.web.FileResponse(file_path, headers=headers)
+        return aiohttp.web.Response(status=404, text="Not found")
 
     return _handle
 

opc/plugins/office_ui/tests/test_attachment_http_handler.py

@@ -0,0 +1,90 @@
+"""Tests for the /api/attachments HTTP handler in server.py.
+
+Attachments are written by the per-project engine that handled the upload
+(`projects/{project_id}/attachments/{id}/{filename}`), while the HTTP handler
+is built around the root engine.  The handler must therefore locate files
+under any project's attachments dir, not just the root engine's active one.
+"""
+
+from __future__ import annotations
+
+import asyncio
+from pathlib import Path
+from types import SimpleNamespace
+
+import aiohttp.web
+
+from opc.core.attachment_store import AttachmentStore
+from opc.plugins.office_ui.server import _make_attachment_handler
+
+_PNG_BYTES = b"\x89PNG\r\n\x1a\n" + b"0" * 32
+
+
+def _make_engine(opc_home: Path, project_id: str) -> SimpleNamespace:
+    return SimpleNamespace(
+        opc_home=opc_home,
+        attachment_store=AttachmentStore(opc_home, project_id),
+    )
+
+
+def _write_attachment(opc_home: Path, project_id: str, attachment_id: str, filename: str) -> Path:
+    dest_dir = opc_home / "projects" / project_id / "attachments" / attachment_id
+    dest_dir.mkdir(parents=True)
+    dest = dest_dir / filename
+    dest.write_bytes(_PNG_BYTES)
+    return dest
+
+
+def _request(attachment_id: str, filename: str) -> SimpleNamespace:
+    return SimpleNamespace(match_info={"attachment_id": attachment_id, "filename": filename})
+
+
+def _handle(engine: SimpleNamespace, attachment_id: str, filename: str) -> aiohttp.web.StreamResponse:
+    handler = _make_attachment_handler(engine)
+    return asyncio.run(handler(_request(attachment_id, filename)))
+
+
+def test_serves_attachment_from_active_project(tmp_path: Path) -> None:
+    engine = _make_engine(tmp_path, "default")
+    _write_attachment(tmp_path, "default", "aid1234567890abc", "image.png")
+
+    response = _handle(engine, "aid1234567890abc", "image.png")
+
+    assert isinstance(response, aiohttp.web.FileResponse)
+    assert response.status == 200
+
+
+def test_serves_attachment_saved_under_other_project(tmp_path: Path) -> None:
+    # Upload happened while project "astron-agent" was active; the root
+    # engine's store still points at "default".
+    engine = _make_engine(tmp_path, "default")
+    _write_attachment(tmp_path, "astron-agent", "bid1234567890abc", "image.png")
+
+    response = _handle(engine, "bid1234567890abc", "image.png")
+
+    assert isinstance(response, aiohttp.web.FileResponse)
+    assert response.status == 200
+
+
+def test_missing_attachment_returns_404(tmp_path: Path) -> None:
+    engine = _make_engine(tmp_path, "default")
+
+    response = _handle(engine, "does-not-exist", "image.png")
+
+    assert response.status == 404
+
+
+def test_rejects_path_traversal_components(tmp_path: Path) -> None:
+    engine = _make_engine(tmp_path, "default")
+    secret = tmp_path / "projects" / "default" / "secret.txt"
+    secret.parent.mkdir(parents=True)
+    secret.write_text("top secret", encoding="utf-8")
+
+    for attachment_id, filename in [
+        ("..", "secret.txt"),
+        ("aid", "../secret.txt"),
+        ("aid", "..\\secret.txt"),
+    ]:
+        response = _handle(engine, attachment_id, filename)
+        assert response.status in (403, 404)
+        assert not isinstance(response, aiohttp.web.FileResponse)