Skip to content

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

Open
CatJuly wants to merge 1 commit into
HKUDS:mainfrom
CatJuly:fix/attachment-cross-project
Open

fix(office-ui): serve attachments saved under any project#37
CatJuly wants to merge 1 commit into
HKUDS:mainfrom
CatJuly:fix/attachment-cross-project

Conversation

@CatJuly

@CatJuly CatJuly commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Images uploaded in the office-UI chat render as broken thumbnails (404 on /api/attachments/...) whenever the message was sent while a project other than the server's startup project was active.

Root cause

Uploads are processed by the per-project engine resolved via _engine_for_request, and AttachmentStore writes files to {opc_home}/projects/{project_id}/attachments/{id}/{filename}. The HTTP download handler built by _make_attachment_handler(engine) in opc/plugins/office_ui/server.py, however, only resolves paths against the root engine's active attachment store. Any attachment saved under a different project's directory is reported as 404 Not found, so <img> tags in the chat show broken images.

Reproduce: start opc ui, switch to (or create) a non-default project, paste an image into the chat, send. The stored file lands in projects/<that-project>/attachments/... while the handler looks in projects/default/attachments/....

Fix

_make_attachment_handler now resolves the attachment across all project attachment directories: the active store is tried first (fast path), then projects/*/attachments/{attachment_id}/{filename}. Attachment ids are 16-hex uuid4 prefixes and unique across projects, so the scan cannot serve a wrong file. Both URL path components are rejected up front if they contain path separators or .., and the existing _is_under_path traversal guard is kept per candidate.

Testing

  • New opc/plugins/office_ui/tests/test_attachment_http_handler.py:
    • serves an attachment from the active project (passes before and after);
    • serves an attachment saved under another project (fails with 404 before the fix, passes after);
    • returns 404 for missing attachments;
    • rejects .. / separator components in attachment_id and filename.
  • uv run pytest opc/plugins/office_ui/tests/ tests/test_attachment_multimodal_routing.py: failure list identical to the origin/main baseline (6 pre-existing test_event_adapter.py failures on both sides), so no regressions.
  • Manually verified against a live server: a previously-broken historical attachment stored under a non-default project now returns 200 image/png; traversal attempts never reach the filesystem.

Scope

Backend only (server.py + new test). No frontend or frontend_dist changes.

Images uploaded in chat render as broken thumbnails whenever the active
project is not the one the server started with. Uploads are saved by the
per-project engine under projects/{pid}/attachments/{id}/, but the
/api/attachments HTTP handler only looked in the root engine's active
attachment store, so every request for a file stored under another
project returned 404.

Resolve attachments across all project attachment directories: try the
active store first, then projects/*/attachments/{id}/{filename}.
Attachment ids are 16-hex uuid4 prefixes, unique across projects, so the
scan cannot serve the wrong file. Both path components are validated
(no separators, no '..') before touching the filesystem, keeping the
existing traversal guard intact.

Verified with a new test (opc/plugins/office_ui/tests/
test_attachment_http_handler.py) that reproduces the cross-project 404
before the fix and passes after; office_ui suite shows no regressions
vs the origin/main baseline (same 6 pre-existing event_adapter
failures).

@LZH-YS1998 LZH-YS1998 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the root-engine capture bug. The 404 diagnosis is correct, but scanning every project directory is not safe to merge because it removes the project boundary from attachment reads.

The current HTTP route carries no project identity or authorization context. With this patch, a request containing only an attachment id and filename can return a matching file from any project. The 16-hex uuid prefix is hard to guess but is not authorization or a uniqueness guarantee. The first-match behavior also permits wrong-file/cache collisions, and synchronous projects/* stat scans add O(project count) blocking work to the aiohttp event loop for every image request.

Please revise this PR so that:

  1. Attachment URLs or opaque references carry a validated project scope, for example /api/projects/{project_id}/attachments/{attachment_id}/{filename}, or use an equivalently scoped signed reference.
  2. The handler resolves only the exact project attachment root in O(1); do not iterate all project directories.
  3. Existing/historical attachment compatibility is handled without granting cross-project reads.
  4. Tests assert active-project success, cross-project rejection, traversal rejection, and deterministic behavior for duplicate ids/filenames.

I integrated this head with the current main in an isolated checkout and the targeted suites passed (31 tests), but test_serves_attachment_saved_under_other_project currently codifies the cross-project access that needs to be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants