fix(attachment): resolve string X-Hero-User in /api/attachment/{id} #29

Merged

sameh-farouk merged 2 commits from fix/attachment-http-auth-resolve-external-id into development

2026-04-27 18:02:08 +00:00

sameh-farouk commented

2026-04-27 16:48:39 +00:00

Member

Summary

Closes #28.

Closes the canvas image upload regression in proxy mode. GET /api/attachment/{id} returned 401 for every authenticated user because the HTTP byte-streaming handler parsed X-Hero-User as u64 directly — that worked for the legacy dev-mode picker convention (numeric caller_id) but silently failed in proxy mode where the proxy injects external_id/email strings.

Symptom

User in proxy mode uploads an image to a canvas. The upload RPC succeeds, the editor inserts an <img> tag pointing at /api/attachment/{id}, the browser fetches that URL, and gets 401 Unauthorized → broken-image placeholder.

DevTools console:

GET http://127.0.0.1:9997/hero_collab/ui/api/attachment/2 401 (Unauthorized)

Affects every proxy-mode user, including the image's own uploader. Pre-existing bug — surfaced during dogfooding in #10's testing thread.

Root cause

crates/hero_collab_server/src/handlers/attachment_http.rs:79 did:

let caller_id = headers
    .get("x-hero-user")
    .and_then(|v| v.to_str().ok())
    .and_then(|s| s.parse::<u64>().ok());

In proxy mode, X-Hero-User: viewer_test@example.com. parse::<u64>() returns None → caller_id=None → check_attachment_access hits the proxy-mode fail-closed branch → 401. The JSON-RPC dispatcher (main.rs::handle_rpc) does the right thing (try external_id, then email/alias, with a caller_id_cache hot path), but the HTTP attachment route never got the same treatment.

Fix

Mirror the dispatcher's two-step resolution:

Try parse::<u64> (preserves legacy dev-mode-numeric path).
Cache hot-path: state.caller_id_cache (populated by every authenticated RPC, so a user who's already loaded the canvas page has a hit here).
DB lookup on external_id first, then email/alias fallback.
Populate the cache on hit so subsequent attachment fetches skip the DB.

DB-error handling is deliberately permissive (mapped to None) instead of the dispatcher's noisy Internal-error path. A transient lock contention here would otherwise look like permission-denied to the browser, which is a worse failure mode than transparent re-resolution. The dispatcher logs noisily on the same code path, so blame is recoverable from logs even if the HTTP path swallows the error code.

Test plan

GET /api/attachment/1 with X-Hero-User: viewer_test@example.com → 200 OK + image bytes (was 401).
GET /api/attachment/1 with X-Hero-User: 2 (numeric) → still 200 OK (legacy dev-mode preserved).
GET /api/attachment/1 with no header → 401 (proxy-mode fail-closed semantic preserved).
GET /api/attachment/1 with X-Hero-User: nonexistent-user@example.com → 401 (no DB row → caller_id=None).
No regression in JSON-RPC attachment.get — that path was already correct and is unchanged.
cargo build --release -p hero_collab_server clean.

Out of scope

Refactoring the identity resolution into a shared helper used by both the dispatcher and HTTP routes. Inlining in attachment_http.rs keeps this PR scoped to the actual bug; if a third HTTP route ever needs the same logic, that's the trigger to extract.

🤖 Generated with Claude Code

## Summary Closes #28. Closes the canvas image upload regression in proxy mode. `GET /api/attachment/{id}` returned 401 for every authenticated user because the HTTP byte-streaming handler parsed `X-Hero-User` as `u64` directly — that worked for the legacy dev-mode picker convention (numeric `caller_id`) but silently failed in proxy mode where the proxy injects `external_id`/email strings. ### Symptom User in proxy mode uploads an image to a canvas. The upload RPC succeeds, the editor inserts an `<img>` tag pointing at `/api/attachment/{id}`, the browser fetches that URL, and gets 401 Unauthorized → broken-image placeholder. DevTools console: ``` GET http://127.0.0.1:9997/hero_collab/ui/api/attachment/2 401 (Unauthorized) ``` Affects every proxy-mode user, including the image's own uploader. Pre-existing bug — surfaced during dogfooding in #10's testing thread. ### Root cause `crates/hero_collab_server/src/handlers/attachment_http.rs:79` did: ```rust let caller_id = headers .get("x-hero-user") .and_then(|v| v.to_str().ok()) .and_then(|s| s.parse::<u64>().ok()); ``` In proxy mode, `X-Hero-User: viewer_test@example.com`. `parse::<u64>()` returns `None` → `caller_id=None` → `check_attachment_access` hits the proxy-mode fail-closed branch → 401. The JSON-RPC dispatcher (`main.rs::handle_rpc`) does the right thing (try `external_id`, then `email`/`alias`, with a `caller_id_cache` hot path), but the HTTP attachment route never got the same treatment. ### Fix Mirror the dispatcher's two-step resolution: 1. Try `parse::<u64>` (preserves legacy dev-mode-numeric path). 2. Cache hot-path: `state.caller_id_cache` (populated by every authenticated RPC, so a user who's already loaded the canvas page has a hit here). 3. DB lookup on `external_id` first, then `email`/`alias` fallback. 4. Populate the cache on hit so subsequent attachment fetches skip the DB. DB-error handling is deliberately permissive (mapped to `None`) instead of the dispatcher's noisy `Internal`-error path. A transient lock contention here would otherwise look like permission-denied to the browser, which is a worse failure mode than transparent re-resolution. The dispatcher logs noisily on the same code path, so blame is recoverable from logs even if the HTTP path swallows the error code. ### Test plan - [x] `GET /api/attachment/1` with `X-Hero-User: viewer_test@example.com` → 200 OK + image bytes (was 401). - [x] `GET /api/attachment/1` with `X-Hero-User: 2` (numeric) → still 200 OK (legacy dev-mode preserved). - [x] `GET /api/attachment/1` with no header → 401 (proxy-mode fail-closed semantic preserved). - [x] `GET /api/attachment/1` with `X-Hero-User: nonexistent-user@example.com` → 401 (no DB row → caller_id=None). - [x] No regression in JSON-RPC `attachment.get` — that path was already correct and is unchanged. - [x] `cargo build --release -p hero_collab_server` clean. ### Out of scope - Refactoring the identity resolution into a shared helper used by both the dispatcher and HTTP routes. Inlining in `attachment_http.rs` keeps this PR scoped to the actual bug; if a third HTTP route ever needs the same logic, that's the trigger to extract. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

sameh-farouk added 1 commit

2026-04-27 16:48:39 +00:00

fix(attachment): resolve string X-Hero-User in /api/attachment/{id} 69c5159b94

The HTTP byte-streaming route for attachments parsed `X-Hero-User` as
`u64` directly. That worked in the legacy dev-mode picker convention
(numeric `caller_id`) but silently failed in proxy mode, where the
proxy injects `external_id`/email strings — `parse::<u64>()` returned
None, `caller_id=None` reached `check_attachment_access`, and the
fail-closed proxy-mode branch returned 401 for every image GET.

Symptom: every canvas image upload in proxy mode rendered as a broken
placeholder, with `GET /api/attachment/{id}` returning 401 in DevTools
even for the image's own uploader.

Fix: mirror the JSON-RPC dispatcher's two-step resolution. Try
`parse::<u64>` first (preserves the dev-mode-numeric path), then fall
back to `state.caller_id_cache` (populated by every authenticated RPC
call), then to a DB lookup on `external_id` and finally on
`email`/`alias`. Cache is populated on hit so subsequent attachment
fetches skip the DB. Cache- and lock-failures are non-fatal — they
just trigger re-resolution next time.

DB-error handling is deliberately permissive (mapped to None) rather
than the dispatcher's noisy Internal-error path: a transient lock
contention here would otherwise look like permission-denied to the
browser, which is a worse failure mode than transparent re-resolution.
The dispatcher logs noisily on the same path, so blame is recoverable
from logs even if attachment GETs swallow the error code.

Verified in proxy mode: `GET /api/attachment/1` with
`X-Hero-User: viewer_test@example.com` returns 200 + image bytes
(was 401). HTTP and JSON-RPC paths now agree on identity resolution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>