User-uploaded avatars: PUT upload with server-side square crop (Fix #601) #608

Merged

crueber merged 2 commits from fix/issue-601 into main 2026-09-15 22:21:03 +00:00

Conversation 1 Commits 2 Files changed 12 +1003 -46

crueber commented 2026-09-15 22:16:12 +00:00

Owner

Copy link

Implements Forgejo #601: user-uploaded avatar (square, server-side center-crop, circular display, Regenerate/Remove preserved).

What: PUT on /api/v1/users/{principal}/avatar (self-or-admin, the org-twin verb — was free on the user path) installs a custom avatar: 2 MiB cap (413), PNG/JPEG/GIF magic-sniff (415 otherwise), server-side center-crop to square with PNG re-encode. GET serves either kind via the pointer Content-Type; the circular display and Regenerate/Remove controls are unchanged; upload control added on the owner page next to them (isSelf gate); SDK repos.users.avatar.upload; ?v=/ETag cache-bust bumped on every upload.

Decisions (all noted in code + docs/features/01_identity_permissions.md + PR):

• IMAGE: pure-Go stdlib only (image/jpeg/png/gif) — decode, center-crop, png.Encode. NO new module (law 1).
• WebP: 415-rejected with an explicit message for user uploads while the org twin keeps its list. Rationale: stdlib cannot decode WebP; accept-and-store-without-crop was rejected as inconsistent; cropping via stdlib is impossible. PNG/JPEG/GIF processed.
• Regenerate: POST keeps meaning 'install fresh generated avatar' and REPLACES an upload (stays visible beside the upload control — the documented opt-back-in after Remove). Hiding it would strand opt-back-in behind delete-then-discover.
• Route: PUT on the existing avatar path (org-twin convention; no verb collision — POST owns regenerate).
• Single raster type (PNG re-encode, lossless + transparency); animated GIFs collapse to first frame (documented). Bucket key keeps its avatar.svg spelling (renaming orphans existing avatars); pointer is authoritative. Uploads 404 unknown principals (no synthesis). Last-writer-wins concurrency (org-twin handling, no lock across store calls). No schema change (law 5).

Verification: go test ./internal/identity/... -race green; identity cover 95.5% (≥95% gate); gofmt/vet clean; go build ./... green; web full-minus-smoke 1546 pass; vite build + esbuild SDK bundle green (bundle contains the control); web/dist/.keep restored. 390px: input constrained w-full in the sidebar column, Tailwind-only, no one-off CSS (pinned in web test).

Implements Forgejo #601: user-uploaded avatar (square, server-side center-crop, circular display, Regenerate/Remove preserved). **What:** PUT on /api/v1/users/{principal}/avatar (self-or-admin, the org-twin verb — was free on the user path) installs a custom avatar: 2 MiB cap (413), PNG/JPEG/GIF magic-sniff (415 otherwise), server-side center-crop to square with PNG re-encode. GET serves either kind via the pointer Content-Type; the circular display and Regenerate/Remove controls are unchanged; upload control added on the owner page next to them (isSelf gate); SDK repos.users.avatar.upload; ?v=/ETag cache-bust bumped on every upload. **Decisions (all noted in code + docs/features/01_identity_permissions.md + PR):** - IMAGE: pure-Go stdlib only (image/jpeg/png/gif) — decode, center-crop, png.Encode. NO new module (law 1). - WebP: 415-rejected with an explicit message for user uploads while the org twin keeps its list. Rationale: stdlib cannot decode WebP; accept-and-store-without-crop was rejected as inconsistent; cropping via stdlib is impossible. PNG/JPEG/GIF processed. - Regenerate: POST keeps meaning 'install fresh generated avatar' and REPLACES an upload (stays visible beside the upload control — the documented opt-back-in after Remove). Hiding it would strand opt-back-in behind delete-then-discover. - Route: PUT on the existing avatar path (org-twin convention; no verb collision — POST owns regenerate). - Single raster type (PNG re-encode, lossless + transparency); animated GIFs collapse to first frame (documented). Bucket key keeps its avatar.svg spelling (renaming orphans existing avatars); pointer is authoritative. Uploads 404 unknown principals (no synthesis). Last-writer-wins concurrency (org-twin handling, no lock across store calls). No schema change (law 5). **Verification:** go test ./internal/identity/... -race green; identity cover 95.5% (≥95% gate); gofmt/vet clean; go build ./... green; web full-minus-smoke 1546 pass; vite build + esbuild SDK bundle green (bundle contains the control); web/dist/.keep restored. 390px: input constrained w-full in the sidebar column, Tailwind-only, no one-off CSS (pinned in web test).

crueber added 1 commit 2026-09-15 22:16:12 +00:00

User-uploaded avatars: PUT upload with server-side square crop (Fix #601) ... c46e5a418b

Implements docs/features/01_identity_permissions.md §8 + docs/go/12_web_ui.md
(same commit, law 12): PUT /api/v1/users/{principal}/avatar installs a
custom avatar — 2 MiB cap, PNG/JPEG/GIF magic-sniff, server-side
center-crop to square PNG via pure stdlib (no new deps, law 1).

Decisions (recorded in code + features doc): WebP 415-rejected with an
explicit message (stdlib cannot decode WebP; org twin keeps its list —
deliberate divergence); POST regenerate replaces an upload (stays visible,
the opt-back-in); PUT on the existing path (org-twin verb, was free);
single raster type (PNG re-encode); last-writer-wins concurrency (org-twin
handling); uploads require an existing profile (404, no synthesis).

Tests: Go upload round-trips (square + non-square → cropped square,
centering proof), cap, SVG/WebP 415, ETag/?v= bump, regenerate-replaces,
delete-opts-out both kinds, idempotency (identity 95.5% cover); web unit
for SDK upload + control pins. gofmt/vet clean, -race green, web
full-minus-smoke green (1546), vite + esbuild green.

crueber referenced this pull request from a commit 2026-09-15 22:20:23 +00:00

Bound user-avatar decode dimensions (decompression-bomb guard, review #608 on Fix #601)

crueber added 1 commit 2026-09-15 22:20:23 +00:00

Bound user-avatar decode dimensions (decompression-bomb guard, review #608 on Fix #601) ... 290f368c42

cropSquarePNG gated decoded dims from the header (DecodeConfig) before
any pixel buffer: 4096px side / 16M px total (400). The 2 MiB input cap
alone does not bound pixels (solid 8000x8000 PNG is ~424 KiB on the
wire, 244 MiB decoded, doubled by the crop copy). 4096 admits phone
photos (4032x3024 = 12M). Docs route table + decision note updated
in the same change (law 12).

crueber commented 2026-09-15 22:20:39 +00:00

Author

Owner

Copy link

Independent review — PR #608 (Fix #601), branch fix/issue-601

Verdict: APPROVE with one hardening fix applied (290f368, pushed to this branch by the reviewer — see §9). No large rework needed; all #601 acceptance criteria are met.

Reviewed diff origin/main..c46e5a4 plus verification runs in the worktree. Coverage re-measured after the fix: 95.5% statements, -race green.

1. Bucket key — PASS

Same users/<username>/avatar.svg key for both kinds (renaming would orphan existing avatars — correct call, law 5). GET serves prof.AvatarContentType from the pointer (http.go:433), never sniffed from the suffix; PutBytes sets matching object ContentType per kind. Repos.jsx:385 gates display on avatar_content_type (no byte probing); ETag is user-avatar-<AvatarUpdatedAt> (version-based, no suffix); UserAvatarURL builds a suffix-free path with ?v=. grep over internal/identity + Repos.jsx + SDK found no .svg-suffix branching. Bucket-compat reasoning holds: same key, content-type-driven.

2. Image pipeline — PASS with one fix (see §9)

• Stdlib-only confirmed: go.mod/go.sum untouched by the branch; only image(+/jpeg/png/gif) stdlib imports.
• Center-crop math is the largest centered square: side=min(w,h), ox=(w-side)/2 — integer-division remainder ≤1px off-center on odd diffs, unavoidable and correct. Centering is pixel-pinned by TestCropSquarePNGCenters both axes.
• PNG re-encode canonical (uploadedUserAvatarContentType=image/png); animated-GIF first-frame collapse documented in code + docs.
• Corrupt body → ErrInvalid → 400 (pinned by "png magic but corrupt body rejected" case); 2 MiB cap enforced twice: MaxBytesReader(cap+1) at the handler (413) + len check pre-decode in the service (ErrTooLarge → 413).
• Defect found & fixed: no decoded-pixel bound (decompression bomb). The input-byte cap does not bound pixels — measured: a solid-color 8000×8000 PNG is 424 KiB on the wire but 64M px → 244 MiB RGBA, doubled by the crop copy; any signed-in user could OOM the server with a <2 MiB upload. The org twin is immune (stores raw, never decodes), so this was new attack surface. Fix in 290f368: DecodeConfig header gate (4096px side / 16M px total → 400) before any pixel allocation; 4096 admits phone photos (4032×3024=12M). Tests + docs updated in the same commit (law 12).

3. WebP/SVG 415 + org twin — PASS

Distinct messages pinned by tests: generic allowlist 415 for SVG/text, WebP 415 explicitly naming WebP (divergence from the org twin documented in code + docs with the stdlib rationale). orgs.go has zero diff — twin untouched.

4. AvatarUpdatedAt / opt-out lifecycle — PASS

Upload bumps AvatarUpdatedAt via pointer CAS (ETag + ?v= change pinned by TestUserAvatarUploadBumpsCacheBust); regenerate flows through putUserAvatar which also stamps now, so regenerate-replaces-upload busts caches too. Flag lifecycle verified in code + tests: install clears AvatarDisabled (upload AND regenerate), DELETE sets it for both kinds; EnsureAvatarAsync early-returns on AvatarContentType != "" (upload survives logins) and on AvatarDisabled (delete→login→no-regen). Re-upload-after-delete opts back in (TestDeleteOptsOutUpload).

5. Regenerate/DELETE/unknown-principal — PASS

Regenerate-replaces-upload stays visible beside the upload control — coherent (hiding it would strand the documented opt-back-in). DELETE is kind-agnostic pointer-clear + bytes best-effort delete, idempotent 200 (cur already clear+disabled short-circuits). Unknown principal → ErrNotFound → 404 on PUT (no synthesis); bad spelling → 400. Both pinned.

6. Concurrency / routes / authZ — PASS

PutUserAvatarBytes = PutBytes then pointer CAS, no lock held across store calls — mirrors the org-twin comment block. Last-writer-wins documented. Route table GET/PUT/POST/DELETE collision-free (POST owns regenerate; old PUT→405 test correctly moved to PATCH→405; exposed_test gains the PUT route row, both lanes delegate pre-method). AuthZ self-or-admin in the handler; explicit tests: foreign PUT→403, anon PUT→401 (plus ghost-via-admin→404 proving the check order).

7. UI — PASS

Upload <label>+<input type=file> under Show when={isSelf()} beside Regenerate; preview is the existing rounded-full h-24 w-24 circle (zero new display code); accept="image/png,image/jpeg,image/gif" matches the server allowlist; refreshAvatar() invalidates user: + me so navbar + asides refetch the ?v=-busted URL; 413/415 mapped to allowlist notes. Tailwind-only (w-full text-xs, shared .muted/.btn), input constrained to the sidebar column (390px pin in user-avatar-upload-601.test.js). SDK users.avatar.upload(principal, data, {contentType}) is shape-identical to the orgs.avatar.upload twin (modulo each module's local call convention).

8. Coverage / pre-fix red / docs / comment — PASS

• go test ./internal/identity/... -race -cover: 95.5% (≥95% gate holds, before and after 290f368).
• Pre-fix red: the entire feature (service symbols, handler verb, test file) is absent on origin/main (grep count 0, test file not present) — the new suite cannot compile pre-fix.
• Law 12: 01_identity_permissions.md (route table + §8 decision, incl. the bomb-guard amendment) and 12_web_ui.md updated in-branch.
• avatar.go:53 prohibition comment rewritten to the two-kind design (generated SVG vs uploaded raster, SVG-upload rejection retained with the same-origin rationale).

9. Reviewer fix commit (pushed to fix/issue-601)

• 290f368 — Bound user-avatar decode dimensions (4096px/16M px, 400): DecodeConfig gate in cropSquarePNG, TestCropSquarePNGRejectsDimensions (side/pixel rejects at crop+service+HTTP layers, no-clobber pin, photo-sized pass-through), docs route-table + decision-note amendment. gofmt/vet clean, identity -race + cover (95.5%) green, affected node --test suites (12 tests) green.

## Independent review — PR #608 (Fix #601), branch `fix/issue-601` **Verdict: APPROVE with one hardening fix applied** (`290f368`, pushed to this branch by the reviewer — see §9). No large rework needed; all #601 acceptance criteria are met. Reviewed diff `origin/main..c46e5a4` plus verification runs in the worktree. Coverage re-measured after the fix: **95.5% statements, `-race` green**. ### 1. Bucket key — PASS Same `users/<username>/avatar.svg` key for both kinds (renaming would orphan existing avatars — correct call, law 5). GET serves `prof.AvatarContentType` from the pointer (`http.go:433`), never sniffed from the suffix; `PutBytes` sets matching object `ContentType` per kind. `Repos.jsx:385` gates display on `avatar_content_type` (no byte probing); ETag is `user-avatar-<AvatarUpdatedAt>` (version-based, no suffix); `UserAvatarURL` builds a suffix-free path with `?v=`. `grep` over `internal/identity` + `Repos.jsx` + SDK found no `.svg`-suffix branching. Bucket-compat reasoning holds: same key, content-type-driven. ### 2. Image pipeline — PASS with one fix (see §9) - Stdlib-only confirmed: `go.mod`/`go.sum` untouched by the branch; only `image(+/jpeg/png/gif)` stdlib imports. - Center-crop math is the largest centered square: `side=min(w,h)`, `ox=(w-side)/2` — integer-division remainder ≤1px off-center on odd diffs, unavoidable and correct. Centering is pixel-pinned by `TestCropSquarePNGCenters` both axes. - PNG re-encode canonical (`uploadedUserAvatarContentType=image/png`); animated-GIF first-frame collapse documented in code + docs. - Corrupt body → `ErrInvalid` → 400 (pinned by "png magic but corrupt body rejected" case); 2 MiB cap enforced twice: `MaxBytesReader(cap+1)` at the handler (413) + `len` check pre-decode in the service (`ErrTooLarge` → 413). - **Defect found & fixed: no decoded-pixel bound (decompression bomb).** The input-byte cap does not bound pixels — measured: a solid-color 8000×8000 PNG is **424 KiB on the wire but 64M px → 244 MiB RGBA**, doubled by the crop copy; any signed-in user could OOM the server with a <2 MiB upload. The org twin is immune (stores raw, never decodes), so this was new attack surface. Fix in `290f368`: `DecodeConfig` header gate (4096px side / 16M px total → 400) before any pixel allocation; 4096 admits phone photos (4032×3024=12M). Tests + docs updated in the same commit (law 12). ### 3. WebP/SVG 415 + org twin — PASS Distinct messages pinned by tests: generic allowlist 415 for SVG/text, WebP 415 explicitly naming WebP (divergence from the org twin documented in code + docs with the stdlib rationale). `orgs.go` has zero diff — twin untouched. ### 4. AvatarUpdatedAt / opt-out lifecycle — PASS Upload bumps `AvatarUpdatedAt` via pointer CAS (ETag + `?v=` change pinned by `TestUserAvatarUploadBumpsCacheBust`); regenerate flows through `putUserAvatar` which also stamps `now`, so regenerate-replaces-upload busts caches too. Flag lifecycle verified in code + tests: install clears `AvatarDisabled` (upload AND regenerate), DELETE sets it for both kinds; `EnsureAvatarAsync` early-returns on `AvatarContentType != ""` (upload survives logins) and on `AvatarDisabled` (delete→login→no-regen). Re-upload-after-delete opts back in (`TestDeleteOptsOutUpload`). ### 5. Regenerate/DELETE/unknown-principal — PASS Regenerate-replaces-upload stays visible beside the upload control — coherent (hiding it would strand the documented opt-back-in). DELETE is kind-agnostic pointer-clear + bytes best-effort delete, idempotent 200 (`cur` already clear+disabled short-circuits). Unknown principal → `ErrNotFound` → 404 on PUT (no synthesis); bad spelling → 400. Both pinned. ### 6. Concurrency / routes / authZ — PASS `PutUserAvatarBytes` = `PutBytes` then pointer CAS, no lock held across store calls — mirrors the org-twin comment block. Last-writer-wins documented. Route table `GET/PUT/POST/DELETE` collision-free (POST owns regenerate; old `PUT→405` test correctly moved to `PATCH→405`; `exposed_test` gains the PUT route row, both lanes delegate pre-method). AuthZ `self-or-admin` in the handler; explicit tests: foreign PUT→403, anon PUT→401 (plus ghost-via-admin→404 proving the check order). ### 7. UI — PASS Upload `<label>`+`<input type=file>` under `Show when={isSelf()}` beside Regenerate; preview is the existing `rounded-full h-24 w-24` circle (zero new display code); `accept="image/png,image/jpeg,image/gif"` matches the server allowlist; `refreshAvatar()` invalidates `user:` + `me` so navbar + asides refetch the `?v=`-busted URL; 413/415 mapped to allowlist notes. Tailwind-only (`w-full text-xs`, shared `.muted`/`.btn`), input constrained to the sidebar column (390px pin in `user-avatar-upload-601.test.js`). SDK `users.avatar.upload(principal, data, {contentType})` is shape-identical to the `orgs.avatar.upload` twin (modulo each module's local call convention). ### 8. Coverage / pre-fix red / docs / comment — PASS - `go test ./internal/identity/... -race -cover`: **95.5%** (≥95% gate holds, before and after `290f368`). - Pre-fix red: the entire feature (service symbols, handler verb, test file) is absent on `origin/main` (`grep` count 0, test file not present) — the new suite cannot compile pre-fix. - Law 12: `01_identity_permissions.md` (route table + §8 decision, incl. the bomb-guard amendment) and `12_web_ui.md` updated in-branch. - `avatar.go:53` prohibition comment rewritten to the two-kind design (generated SVG vs uploaded raster, SVG-upload rejection retained with the same-origin rationale). ### 9. Reviewer fix commit (pushed to `fix/issue-601`) - `290f368` — Bound user-avatar decode dimensions (4096px/16M px, 400): `DecodeConfig` gate in `cropSquarePNG`, `TestCropSquarePNGRejectsDimensions` (side/pixel rejects at crop+service+HTTP layers, no-clobber pin, photo-sized pass-through), docs route-table + decision-note amendment. `gofmt`/`vet` clean, identity `-race` + cover (95.5%) green, affected `node --test` suites (12 tests) green.

crueber merged commit 755ab5db5c into main 2026-09-15 22:21:03 +00:00

crueber referenced this pull request from a commit 2026-09-15 22:21:04 +00:00

Merge pull request 'User-uploaded avatars: PUT upload with server-side square crop (Fix #601)' (#608) from fix/issue-601 into main

crueber referenced this pull request 2026-09-15 22:21:10 +00:00

Allow user-uploaded square avatar (server-side center-crop, circular display, Regenerate/Remove preserved) #601

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

actions

  

bug

Something is not working

  

cli

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

fork

  

forum

  

git storage

  

help wanted

Need some help

  

insights

  

invalid

Something is wrong

  

issues

  

moderation

  

oidc

  

ownership transfer

  

packages

  

pr/merge protection rules

  

projects

  

pull requests

  

question

More information is needed

  

releases

  

sponsorships

  

tags

  

webhooks

  

wiki

  

wontfix

This won't be fixed

No labels

actions

bug

cli

duplicate

enhancement

fork

forum

git storage

help wanted

insights

invalid

issues

moderation

oidc

ownership transfer

packages

pr/merge protection rules

projects

pull requests

question

releases

sponsorships

tags

webhooks

wiki

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

crueber/walhub!608

No description provided.