feat(users): avatar file upload (POST/GET/DELETE /users/me/avatar)

[bdunncompany]

What

Replaces the URL-only avatar field on Settings → Profile with a real file upload. Three new endpoints back it; files are stored on the existing api_data volume at /data/avatars/<userId>.<ext>, mirroring the fileStorage.ts pattern used by remote transfers. No DB migration — reuses the existing users.avatar_url column (now holds an internal /api/v1/users/<id>/avatar URL instead of an external one).

API

• POST /users/me/avatar — multipart upload (single file field)
  • MIME allowlist: image/png, image/jpeg, image/webp (no SVG)
  • Magic-byte verification — does not trust the Content-Type header
  • 5 MB cap via bodyLimit middleware (413 on overflow)
  • Atomic write (tmp + rename), unlinks a prior file at a different extension
  • Sets users.avatar_url = /api/v1/users/<id>/avatar; audit user.avatar.upload
• GET /users/:id/avatar — auth-required, streams bytes, Cache-Control: private, max-age=300, weak ETag + 304 support
• DELETE /users/me/avatar — unlinks file, clears avatar_url; audit user.avatar.delete

Behavior change (please confirm the call)

avatarUrl is removed from the PATCH /users/me schema. Avatars now flow exclusively through the upload endpoints; the strict schema makes any client still sending avatarUrl get a 400 (rather than a silent drop). This is the deliberate intent — switching from "paste a URL" to "upload a file." Consequences for the existing SOC2 audit tests, all updated in this PR:

• The audit-attribution test that used avatarUrl as its example non-email field now uses name (same intent: partner-scope self-change → user.profile.update).
• The rejects avatarUrl with javascript: scheme test is removed — the avatarUrl URL-scheme refinement no longer exists, and "unknown field rejected" is already covered by the strict-schema test.

If you'd rather keep the URL field alongside upload, say so and I'll rework to additive.

Web

ProfilePage.tsx: 96px preview, file picker, drag-drop onto the preview, and a Remove button (shown only when an avatar is set). Helper: "PNG, JPG, or WebP. Max 5 MB." updateUser fires on success/delete so the topbar avatar refreshes immediately. The URL text field is removed.

docker-compose.yml: adds AVATAR_STORAGE_PATH=/data/avatars to the api env (matches the existing TRANSFER_STORAGE_PATH / PATCH_REPORT_STORAGE_PATH pattern).

Tests

• API tsc clean; full API vitest 5803 passed / 0 failed (9 new avatar cases: accept PNG/JPEG/WebP, reject SVG, reject Content-Type/byte mismatch, reject empty, reject missing field, DELETE, POST→GET roundtrip; storage path is a per-run tmpdir).
• Web astro check 0 errors; web vitest 721 passed / 0 failed (4 new ProfilePage cases).
• No new dependencies (hono/body-limit, hono/streaming are existing hono submodules). No DB migration.

[ToddHebebrand]

Cross-tenant avatar read — must-fix. GET /:id/avatar (apps/api/src/routes/users.ts:479) serves the avatar for an arbitrary :id with no permission check and no tenant scoping. The sibling GET /:id (:750) gates on requirePermission(USERS_READ) and resolves through getScopedUser(...); this route does neither, so any authenticated user in any partner/org can enumerate and fetch any other user's avatar across tenants. The // tenant-scoped via auth middleware comment is inaccurate — the * partner-scope middleware only gates partner-scope full-org access, not per-id reads. Fix: resolve :id through getScopedUser(userId, scopeContext) before serving, or restrict the read route to /me.

Secondary (must-look): the GET handler sends headers including Content-Length, then on a stream error the bytes are already partially written — the client gets a truncated body under a 200 plus a full Content-Length (:514-523). Open the read inside the same try and prefer pipeline().

The upload path itself is solid — magic-byte sniffing (rejects SVG), Content-Type cross-check, bodyLimit + explicit size guard, atomic tmp+rename, filename derived from the authenticated user id (no traversal). Nicely done there.

Nice-to-have (non-blocking): nothing calls deleteAvatar on user deletion, so files leak on api_data indefinitely — wire it into the user-delete path. The web handlers (ProfilePage.tsx:1092,1129) hand-roll fetchWithAuth + try/catch instead of runAction (CLAUDE.md web-mutation rule) — adopt it or add a justified runActionAllowlist.ts entry. DELETE returns 200 even when no file existed.

Bouncing on the cross-tenant read; everything else is in good shape once that route is scoped.

[bdunncompany]

Thanks for catching the cross-tenant read — fixed in b9f08fda.

Must-fix (cross-tenant avatar read): GET /:id/avatar now mirrors GET /:id — a caller may always read their own avatar (the top bar needs it without USERS_READ), but any other id must resolve through getScopedUser(userId, scopeContext) first. Out-of-scope returns the same 404 as a missing avatar, so it never reveals which user ids exist in other tenants. The inaccurate // tenant-scoped via auth middleware comment is gone.

Must-look (truncated body under 200): since avatars are capped at 5 MB, the handler now reads the whole file into a Buffer (new readAvatarBuffer) before sending any headers, then returns it with an exact Content-Length. An I/O error is a clean 500 before the response starts, instead of a 200 with a short body. Dropped the hono/streaming path here.

Tests: 3 new cases — a cross-tenant read 404s even though the file exists on disk; an in-scope read serves the bytes; a self read serves without consulting getScopedUser. Full API suite 5806 passed; eslint/Trivy/Gitleaks clean.

Nice-to-haves:

• deleteAvatar on user deletion — deliberately not wired into DELETE /:id, because that route removes a tenant membership (partnerUsers/organizationUsers), not the global users row. Calling deleteAvatar(userId) there would wipe a multi-tenant user's avatar while they're still active in another tenant. The right hook is the global hard-delete path — happy to do it as a focused follow-up.
• runAction adoption in ProfilePage.tsx and the DELETE-when-no-file 200 — noted; can fold into the same follow-up or this PR, your call.

[bdunncompany]

Also folded in the display half (a6beda89): avatar_url is /api/v1/users/<id>/avatar, but a plain <img src> can't send the Bearer header the GET requires — so without this the upload worked but the served avatar rendered as a broken image. Added a refcounted useAvatarBlobUrl hook (lib/avatarBlobCache) that fetches via fetchWithAuth → createObjectURL → revokes on last-unmount, wired into ProfilePage + Header + PortalHeader (external URLs pass through after sanitizing). avatarBlobCache.test.ts covers cache hit / refcount / revoke / external passthrough. Full web suite 730 passed. Now the feature is complete end-to-end — sorry for the two pushes; better you review it whole.

[bdunncompany]

One more, and this one's load-bearing (024da3cd): the blob fetch I added still 404'd because buildApiUrl doubled the prefix — /api/v1/users/:id/avatar matched the bare /api/ branch first and became /api/v1/v1/users/:id/avatar. Added an explicit /api/v1/ branch. So the full chain is now: upload → blob fetch → correct URL. The blob-cache unit test mocks fetchWithAuth, so it couldn't catch this; added a fetchWithAuth regression that asserts an /api/v1/ path isn't doubled. That's the avatar feature genuinely working end-to-end now — last push on this, promise.

[ToddHebebrand]

Re-review — both round-21 must-fixes resolved. Clearing the block.

Cross-tenant read (must-fix): GET /:id/avatar now gates on userId !== auth.user.id → getScopedUser(userId, getScopeContext(auth)) (apps/api/src/routes/users.ts:563-585), the same resolution path as the sibling GET /:id. Nice touch returning the same 404 as a missing avatar so the route never reveals which user ids exist in other tenants (no enumeration oracle), and the own-avatar short-circuit keeps the top bar working without USERS_READ. The new GET /users/:id/avatar — cross-tenant authorization test block covers cross-partner → 404, in-scope → serves, own → serves without a scope lookup.

Truncated-stream (must-fix): now reads the whole size-capped buffer via readAvatarBuffer before sending any headers (users.ts GET handler) — an I/O error is a clean 500 instead of a 200 with a truncated body under a full Content-Length. Plus a ^[a-zA-Z0-9_-]+$ traversal guard and ETag/304 caching.

Both gates verified at file:line. Upload path was already solid (magic-byte sniff, size cap, atomic tmp+rename).

Status: review-clean, no must-fix remaining. Holding the merge only for a manual upload/display UI pass (top-bar + Profile authed-blob fetch). Non-blocking nice-to-have from the original review still open: no deleteAvatar on user-delete (file leak on account removal) — fine as a fast-follow.

[ToddHebebrand]

Pushed a fix directly to the branch (ed9a4284) after the avatar upload 400'd in a local browser test — flagging since it touches shared code.

Root cause: not in your avatar code. fetchWithAuth (apps/web/src/stores/auth.ts:330) called headers.set('Content-Type', 'application/json') unconditionally, so the headers: {} bypass in ProfilePage.tsx couldn't take effect. The FormData body went out as application/json with no multipart boundary, and the server's parseBody found no file → 400 {"error":"file field is required"}. The API side and the upload UI were both correct; the shared helper masked them.

Fix: skip the forced JSON content-type when options.body instanceof FormData so the browser sets multipart/form-data with its boundary. Also dropped the now-unneeded headers: {} and corrected the stale comment. Added a regression test in auth.test.ts asserting Content-Type is left unset for FormData.

Verified end-to-end locally (browser): valid PNG → 200, avatar renders + authed-blob GET 200; text-renamed-.png → server 415 "Unsupported image format" with the prior avatar preserved; Remove → DELETE → back to initials. auth.test.ts + ProfilePage.test.tsx 21/21, tsc clean.

Re-reviewing for merge once CI re-runs.