fix(avatar): prevent stored XSS via content-type spoofing#290
Merged
Conversation
…nt-type spoofing
The avatar proxy stored whatever bytes upstream returned at u.Picture and served
them back with whatever http.DetectContentType decided — including text/html for
HTML/script payloads. A user whose login flow allows attacker-controlled u.Picture
(custom OAuth provider, self-signup with picture URL) could plant HTML in the avatar
store and have it served from the auth-service origin as text/html, executing JS in
that origin.
Layered defense:
* Proxy.resize() validates input via image.DecodeConfig (cheap, no pixel
allocation) before any image.Decode. Refuses non-image content with nil so
attacker payloads (HTML / SVG / text) never reach Store.Put. Previously it
silently fell back to teeBuf on decode failure, poisoning the store.
* Per-pixel cap maxAvatarPixels (16 MP) rejects compressed decompression-bomb
images that would force image.Decode to allocate gigabytes of raster memory
on a single login attempt. Multiplication uses int64 to avoid wraparound on
32-bit builds (GOARCH=386, 32-bit arm) where the int32 product of 16-bit GIF
or JPEG dimensions would otherwise bypass the cap.
* The no-resize path (limit<=0 or image already fits) returns the original
bytes verbatim so animated GIFs and other multi-frame formats round-trip
without being flattened to the first frame.
* Proxy.Put() falls back to a generated identicon when resize() returns nil.
* Proxy.PutContent() returns an explicit error for non-image input.
* Proxy.Handler() validates served bytes via the new safeImgContentType helper
(strict allowlist: image/{png,jpeg,gif,webp,bmp,x-icon}; image/svg+xml is
explicitly rejected because SVG can execute scripts when navigated to top
level). This catches stores poisoned before this fix and any future
regression at serve time.
* Every Handler response (success, 304, or error) carries layered per-endpoint
headers via setAvatarDefenseHeaders():
Content-Security-Policy: default-src 'none'; sandbox; frame-ancestors 'none'
X-Content-Type-Options: nosniff
Content-Disposition: inline; filename="avatar"
Applied to both v1 (avatar/) and v2 (v2/avatar/) so v1 consumers can pick up the
fix via a v1.x.y patch release and v2 consumers via v2.x.y from the same commit.
Existing tests that codified the broken pass-through behavior have been updated.
New tests:
TestAvatar_PutRejectsNonImage — store is not poisoned by attacker u.Picture
TestAvatar_HandlerRejectsPoisonedStore — handler refuses pre-poisoned bytes
TestAvatar_resizeRejectsDecompressionBomb — DecodeConfig + int64 dim cap path
Provider tests that exercised PutContent with placeholder "png-bytes" now use a
real 1x1 PNG (tinyPNG) since PutContent validates input.
…() response
go-pkgz/auth's response surface is JSON-only for auth routes and images for the
avatar route — no consumer-customisable HTML anywhere. Setting a strict CSP on
every response is therefore unconditionally safe and gives the auth origin
defense-in-depth against any future trust-boundary regression that might emit
a renderable body.
* Content-Security-Policy: default-src 'none'; sandbox; frame-ancestors 'none'
blocks inline scripts and event handlers even if a body is ever served as
HTML by mistake; the sandbox directive additionally isolates any rendered
document from this origin.
* X-Content-Type-Options: nosniff prevents browsers from MIME-overriding the
declared Content-Type to a more dangerous one.
withSecurityHeaders wraps both handlers returned from Service.Handlers() (the
auth route dispatcher and the avatar handler). The avatar Handler keeps its
internal call to setAvatarDefenseHeaders so direct callers — tests, custom
mounts, anything that doesn't go through Handlers() — still get the full
header set.
Applied to both v1 (auth.go) and v2 (v2/auth.go).
Before this change a remote avatar was buffered twice: once inside load() to
enforce maxAvatarFetchSize before returning an io.ReadCloser, and again inside
resize() when it called io.ReadAll on that reader to feed image.DecodeConfig.
For a 10 MiB upstream avatar each in-flight login held ~20 MiB peak instead of
~10 MiB. Bounded, but wasteful under concurrent login pressure.
Collapse the duplication by moving the size cap to the trust boundary and
flowing bytes straight through:
* load() now returns ([]byte, error). No Reader wrapping, no Close needed
at the caller, no second ReadAll downstream.
* resize() takes ([]byte, int) instead of (io.Reader, int). It works on the
same bytes load() already validated against the cap, with a defensive
len(body) == 0 || > cap check for callers that bypass load (identicon,
PutContent).
* PutContent() now runs io.ReadAll(io.LimitReader(content, cap+1)) at the
public API boundary so the cap is visible to callers reading the code.
The total work is the same — resize used to do this internally — but
the size enforcement is now explicit at the trust boundary.
* Put()'s deferred body.Close() block is gone since load returns bytes.
* The identicon path drops bytes.NewBuffer(b) wrapping and passes b directly.
No public API change, no behavior change other than halved peak memory per
concurrent login. All existing tests pass on both v1 and v2 unchanged in
intent — only the test inputs were simplified to pass []byte directly to
resize instead of wrapping in strings.NewReader / bytes.NewReader.
Coverage Report for CI Build 26197092980Coverage increased (+0.1%) to 85.425%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR hardens the avatar proxy pipeline to prevent stored XSS via content-type spoofing and adds defense-in-depth security headers (CSP + nosniff) to all handlers returned by Service.Handlers() across both v1 and v2 modules.
Changes:
- Rejects non-image avatar bytes during fetch/store and re-validates stored bytes at serve time to avoid serving attacker-controlled HTML/SVG as
text/html. - Adds pixel-count preflight (
DecodeConfig+maxAvatarPixels) to mitigate decompression bombs and 32-bit overflow edge cases, and preserves original bytes when no resize is needed. - Applies mandatory security headers (CSP +
X-Content-Type-Options: nosniff) via handler wrapping; avatar responses also getContent-Disposition.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| avatar/avatar.go | Core fix: validate avatar bytes before storing/serving; add defense headers; refactor buffering; add pixel cap. |
| avatar/avatar_test.go | Updates tests to reflect new rejection behavior and adds regression/security tests. |
| auth.go | Wraps returned handlers with withSecurityHeaders to enforce CSP + nosniff. |
| auth_test.go | Adds coverage ensuring security headers are present on auth routes. |
| provider/telegram_test.go | Updates tests to use real PNG bytes now that PutContent validates content. |
| v2/avatar/avatar.go | Same as v1 avatar hardening for v2 module. |
| v2/avatar/avatar_test.go | Same as v1 tests for v2 module. |
| v2/auth.go | Same as v1 handler wrapping for v2 module. |
| v2/auth_test.go | Same as v1 security header test for v2 module. |
| v2/provider/telegram_test.go | Same as v1 Telegram test update for v2 module. |
Comments suppressed due to low confidence (2)
avatar/avatar.go:328
safeImgContentTypecurrently accepts any MIME type with animage/prefix (exceptimage/svg+xml), but both the function comment and the PR description describe an allowlist of specific safe raster types. As written, this is broader than documented and could inadvertently permit future scriptable/unsafeimage/*types. Consider enforcing an explicit allowlist (e.g. png/jpeg/gif/webp/bmp/x-icon) or updating the comment/PR docs to match the actual behavior.
// safeImgContentType returns the sniffed content type if the bytes look like a safe
// image format (rasterised: PNG, JPEG, GIF, WebP, BMP, ICO). Returns an error for any
// non-image content or for image/svg+xml, which can execute scripts when navigated to
// top-level and must never be served from the avatar origin.
func safeImgContentType(img []byte) (string, error) {
ct := http.DetectContentType(img)
base := ct
if idx := strings.Index(base, ";"); idx >= 0 {
base = strings.TrimSpace(base[:idx])
}
if !strings.HasPrefix(base, "image/") || base == "image/svg+xml" {
return "", fmt.Errorf("non-image content type %q", ct)
}
return base, nil
v2/avatar/avatar.go:328
safeImgContentTypecurrently accepts any MIME type with animage/prefix (exceptimage/svg+xml), but both the function comment and the PR description describe an allowlist of specific safe raster types. As written, this is broader than documented and could inadvertently permit future scriptable/unsafeimage/*types. Consider enforcing an explicit allowlist (e.g. png/jpeg/gif/webp/bmp/x-icon) or updating the comment/PR docs to match the actual behavior.
// safeImgContentType returns the sniffed content type if the bytes look like a safe
// image format (rasterised: PNG, JPEG, GIF, WebP, BMP, ICO). Returns an error for any
// non-image content or for image/svg+xml, which can execute scripts when navigated to
// top-level and must never be served from the avatar origin.
func safeImgContentType(img []byte) (string, error) {
ct := http.DetectContentType(img)
base := ct
if idx := strings.Index(base, ";"); idx >= 0 {
base = strings.TrimSpace(base[:idx])
}
if !strings.HasPrefix(base, "image/") || base == "image/svg+xml" {
return "", fmt.Errorf("non-image content type %q", ct)
}
return base, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umputun
requested changes
May 20, 2026
Member
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
couple things to fix before merge:
-
Discord avatars look broken by this change.
provider/providers.go:289builds.webpURLs, butavatar/store.goonly registers GIF/JPEG/PNG decoders andavatar/avatar.go:269now requiresimage.DecodeConfigbefore storing. That means Discord WebP avatars will fall back to identicons even thoughsafeImgContentTypesays WebP is allowed. Same in v2. This should be fixed in the avatar validation path, for example by registeringgolang.org/x/image/webpin both modules and adding a regression test. -
If-None-Matchcompares against the unquoted raw store id (avatar/avatar.go:224-230, v2 same), while the response sends a quoted ETag. Standard clients sendIf-None-Match: "<etag>", sometimes weak or comma-separated. This should parse validators or at least compare against the quoted value. Add tests for quoted and weak/multiple forms. -
safeImgContentTypesays it is an allowlist of raster formats, but it accepts anyimage/*except SVG (avatar/avatar.go:315-328, v2 same). For this security fix I’d make it an explicit switch over PNG/JPEG/GIF/WebP/BMP/ICO. -
Since global CSP is intentional, pls document the
AddCustomHandlerimpact.Service.Handlers()now appliesdefault-src 'none'; sandboxto custom provider handlers too, so HTML/form/script based custom flows can break.
minor: var tinyPNG, _ = ... in Telegram tests should use a mustDecode helper. Also consider using io.ReadFull/ReadAtLeast for the sniff buffer instead of a single Read, so a short read from a store doesn't reject a valid avatar.
…t allowlist Addresses review on #290: * Register golang.org/x/image/webp via blank import. Without this, Go's stdlib image.DecodeConfig only knows PNG/JPEG/GIF — Discord (which uses .webp avatars) would silently fall back to identicons after the validation fix. Regression test TestAvatar_resizeAcceptsWebP exercises a real WebP avatar end-to-end. * safeImgContentType is now an explicit switch over the allowed raster MIME set (image/png, image/jpeg, image/gif, image/webp, image/bmp, image/x-icon, image/vnd.microsoft.icon). The previous HasPrefix("image/") catch-all matched the docstring but not the intent — any future scriptable image/* MIME added by http.DetectContentType would have silently passed. * If-None-Match parsing now follows RFC 7232: handles quoted ETag (the form browsers actually send), W/"..." weak validators, comma-separated lists, and the * wildcard. The previous comparison stripped quotes from the response ETag before comparing against the (almost always quoted) request header, so 304 short-circuits never fired in practice. Added etagMatches helper with its own table test plus an extended 304 path test in TestAvatar_Routes that sends both the strong and weak validator forms. * Handler now uses io.ReadFull for the sniff buffer so a Store that returns a buffered reader with a small first-Read won't cause DetectContentType to misclassify a valid image. ErrUnexpectedEOF is expected for short bodies and treated the same as EOF. * withSecurityHeaders docstring now spells out the impact on consumer-added custom handlers (AddCustomHandler / AddProvider): the strict CSP also wraps those, so any custom provider rendering HTML must either override the CSP by calling w.Header().Set before writing, or move scripts/styles to external files served from 'self'. Minor: tinyPNG / tinyWebP test fixtures now go through a mustDecodeBase64 helper so corrupt fixtures surface as a clear panic rather than silently discarded errors. Drive-by from `go fix ./...`: provider/verify_test.go loops modernised to the Go 1.22 `for range N` form. Both v1 and v2 mirrored.
umputun
approved these changes
May 21, 2026
Member
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
lgtm, thx. Requested fixes look addressed.
Comment on lines
+262
to
+265
| // event handlers on those pages. Such providers should either (a) override the CSP | ||
| // for their own response by calling w.Header().Set("Content-Security-Policy", ...) | ||
| // before writing — Set replaces the wrapper's value — or (b) move any required | ||
| // scripts/styles to external files served from 'self'. |
Comment on lines
+262
to
+266
| // the dev_provider's login page, etc.), the strict CSP will block inline scripts and | ||
| // event handlers on those pages. Such providers should either (a) override the CSP | ||
| // for their own response by calling w.Header().Set("Content-Security-Policy", ...) | ||
| // before writing — Set replaces the wrapper's value — or (b) move any required | ||
| // scripts/styles to external files served from 'self'. |
paskal
added a commit
that referenced
this pull request
May 21, 2026
…curity fix Sweep over the docstrings touched (or adjacent to) PR #290's security work, prompted by Copilot's post-merge review of withSecurityHeaders and an adversarial pass from Codex on the rest of the same surface. All changes are docstring/comment-only; no code, no behavior, no test churn. * withSecurityHeaders CONSUMER NOTE (auth.go, v2/auth.go) — the previous text told consumers HTML custom handlers could fix CSP blocking by "moving scripts/styles to external files served from 'self'", but the wrapper applies default-src 'none' and sandbox, so even self-hosted resources are blocked. New text spells out what the wrapper actually does and gives a concrete relaxed-CSP example. The example list also drops "dev_provider's login page" — that page is served by DevAuthServer on its own HTTP listener, not by handlers Service.Handlers wraps. Replaced with "custom server login pages". * Proxy.Put godoc — was "stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url", which omitted the identicon fallback that fires on empty u.Picture, fetch failure, or non-image upstream bytes. Doc now describes that the function silently substitutes an identicon in those cases and returns its proxied URL — the caller is not told the upstream was rejected. * Proxy.Handler godoc — was "returns token routes for given provider", a leftover from a much older shape of the code. Replaced with a description of what Handler actually does today: serves stored avatar bytes by id, sniffs against an allowlist, sets defense headers. * Handler's inline serve-time validation comment — said "validate the bytes really are an image", but Handler reads up to sniffLen bytes and runs them through http.DetectContentType + an allowlist. That is content-type sniffing, not proof of full decodability. Reworded to match. * Proxy.resize godoc — said "validates that the input is a real image", but the no-resize path returns the original bytes after only image.DecodeConfig and the dimension cap; no full decode runs. Split into format/dimension checks (DecodeConfig only) vs full decode (resize path only). * maxAvatarFetchSize constant — mentioned only the remote-URL fetch path; after #290 the cap also bounds PutContent's caller-supplied reader and is the implicit size invariant resize trusts. Doc updated to name both callers. All changes mirrored across v1 and v2.
paskal
added a commit
that referenced
this pull request
May 21, 2026
…curity fix Sweep over the docstrings touched (or adjacent to) PR #290's security work, prompted by Copilot's post-merge review of withSecurityHeaders and an adversarial pass from Codex on the rest of the same surface. All changes are docstring/comment-only; no code, no behavior, no test churn. * withSecurityHeaders CONSUMER NOTE (auth.go, v2/auth.go) — the previous text told consumers HTML custom handlers could fix CSP blocking by "moving scripts/styles to external files served from 'self'", but the wrapper applies default-src 'none' and sandbox, so even self-hosted resources are blocked. New text spells out what the wrapper actually does and gives a concrete relaxed-CSP example. The example list also drops "dev_provider's login page" — that page is served by DevAuthServer on its own HTTP listener, not by handlers Service.Handlers wraps. Replaced with "custom server login pages". * Proxy.Put godoc — was "stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url", which omitted the identicon fallback that fires on empty u.Picture, fetch failure, or non-image upstream bytes. Doc now describes that the function silently substitutes an identicon in those cases and returns its proxied URL — the caller is not told the upstream was rejected. * Proxy.Handler godoc — was "returns token routes for given provider", a leftover from a much older shape of the code. Replaced with a description of what Handler actually does today: serves stored avatar bytes by id, sniffs against an allowlist, sets defense headers. * Handler's inline serve-time validation comment — said "validate the bytes really are an image", but Handler reads up to sniffLen bytes and runs them through http.DetectContentType + an allowlist. That is content-type sniffing, not proof of full decodability. Reworded to match. * Proxy.resize godoc — said "validates that the input is a real image", but the no-resize path returns the original bytes after only image.DecodeConfig and the dimension cap; no full decode runs. Split into format/dimension checks (DecodeConfig only) vs full decode (resize path only). * maxAvatarFetchSize constant — mentioned only the remote-URL fetch path; after #290 the cap also bounds PutContent's caller-supplied reader and is the implicit size invariant resize trusts. Doc updated to name both callers. All changes mirrored across v1 and v2.
paskal
added a commit
that referenced
this pull request
May 21, 2026
…curity fix Sweep over the docstrings touched (or adjacent to) PR #290's security work, prompted by Copilot's post-merge review of withSecurityHeaders and an adversarial pass from Codex on the rest of the same surface. All changes are docstring/comment-only; no code, no behavior, no test churn. * withSecurityHeaders CONSUMER NOTE (auth.go, v2/auth.go) — the previous text told consumers HTML custom handlers could fix CSP blocking by "moving scripts/styles to external files served from 'self'", but the wrapper applies default-src 'none' and sandbox, so even self-hosted resources are blocked. New text spells out what the wrapper actually does and gives a concrete relaxed-CSP example. The example list also drops "dev_provider's login page" — that page is served by DevAuthServer on its own HTTP listener, not by handlers Service.Handlers wraps. Replaced with "custom server login pages". * Proxy.Put godoc — was "stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url", which omitted the identicon fallback that fires on empty u.Picture, fetch failure, or non-image upstream bytes. Doc now describes that the function silently substitutes an identicon in those cases and returns its proxied URL — the caller is not told the upstream was rejected. * Proxy.Handler godoc — was "returns token routes for given provider", a leftover from a much older shape of the code. Replaced with a description of what Handler actually does today: serves stored avatar bytes by id, sniffs against an allowlist, sets defense headers. * Handler's inline serve-time validation comment — said "validate the bytes really are an image", but Handler reads up to sniffLen bytes and runs them through http.DetectContentType + an allowlist. That is content-type sniffing, not proof of full decodability. Reworded to match. * Proxy.resize godoc — said "validates that the input is a real image", but the no-resize path returns the original bytes after only image.DecodeConfig and the dimension cap; no full decode runs. Split into format/dimension checks (DecodeConfig only) vs full decode (resize path only). * maxAvatarFetchSize constant — mentioned only the remote-URL fetch path; after #290 the cap also bounds PutContent's caller-supplied reader and is the implicit size invariant resize trusts. Doc updated to name both callers. All changes mirrored across v1 and v2.
umputun
pushed a commit
that referenced
this pull request
May 21, 2026
…curity fix Sweep over the docstrings touched (or adjacent to) PR #290's security work, prompted by Copilot's post-merge review of withSecurityHeaders and an adversarial pass from Codex on the rest of the same surface. All changes are docstring/comment-only; no code, no behavior, no test churn. * withSecurityHeaders CONSUMER NOTE (auth.go, v2/auth.go) — the previous text told consumers HTML custom handlers could fix CSP blocking by "moving scripts/styles to external files served from 'self'", but the wrapper applies default-src 'none' and sandbox, so even self-hosted resources are blocked. New text spells out what the wrapper actually does and gives a concrete relaxed-CSP example. The example list also drops "dev_provider's login page" — that page is served by DevAuthServer on its own HTTP listener, not by handlers Service.Handlers wraps. Replaced with "custom server login pages". * Proxy.Put godoc — was "stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url", which omitted the identicon fallback that fires on empty u.Picture, fetch failure, or non-image upstream bytes. Doc now describes that the function silently substitutes an identicon in those cases and returns its proxied URL — the caller is not told the upstream was rejected. * Proxy.Handler godoc — was "returns token routes for given provider", a leftover from a much older shape of the code. Replaced with a description of what Handler actually does today: serves stored avatar bytes by id, sniffs against an allowlist, sets defense headers. * Handler's inline serve-time validation comment — said "validate the bytes really are an image", but Handler reads up to sniffLen bytes and runs them through http.DetectContentType + an allowlist. That is content-type sniffing, not proof of full decodability. Reworded to match. * Proxy.resize godoc — said "validates that the input is a real image", but the no-resize path returns the original bytes after only image.DecodeConfig and the dimension cap; no full decode runs. Split into format/dimension checks (DecodeConfig only) vs full decode (resize path only). * maxAvatarFetchSize constant — mentioned only the remote-URL fetch path; after #290 the cap also bounds PutContent's caller-supplied reader and is the implicit size invariant resize trusts. Doc updated to name both callers. All changes mirrored across v1 and v2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a stored XSS in
avatar.Proxyand adds defense-in-depth headers acrossthe rest of
Service.Handlers(). Both v1 (avatar/,auth.go) and v2(
v2/avatar/,v2/auth.go) on the same commit, so a single coordinated release(
v1.x.yandv2.x.ytags) covers both module versions.The bug
avatar/avatar.go:Proxy.resize()fell back to the raw bytes (teeBuf) whenimage.Decodefailed.avatar/avatar.go:Proxy.Handler()then calledhttp.DetectContentTypeon whatever was stored and emitted the result asContent-Type— includingtext/htmlfor HTML bodies.Attack scenario
The realistic path requires the attacker to control
u.Picture:pictureclaim pointsat attacker-controlled
https://evil.tld/avatar.png.Proxy.Putfetches the URL,resizefails to decode theHTML body, falls through to
teeBuf, stores the HTML under<sha1(userID)>.image.https://auth-host/avatar/<sha1>.image. Browsergets
Content-Type: text/htmlwith noContent-Disposition, renders theattacker's HTML as a document in the auth-service origin, runs script.
This is not exploitable on remark42's standard setup —
u.Picturetherecomes from admin-configured providers (GitHub, Google, etc.) which the user
cannot redirect. It is exploitable on:
pictureclaim is attacker-controlled.Codex-flagged residual risks addressed in the same fix:
decodes to ~17 GB of pixel data. Before this PR
Proxy.resizealways calledimage.Decodeand would OOM the auth service on a single login. After, animage.DecodeConfigpre-check rejects oversized declared dimensions beforeany pixel allocation.
int, so onGOARCH=386 / 32-bit arm
cfg.Width * cfg.Heightfor 16-bit GIF or JPEGdimensions could wrap below the cap. Multiplication is now in
int64.bytes verbatim; previously the
TeeReadercould capture only the firstframe.
What changes
Commit 1 —
fix(avatar): reject non-image content to prevent stored XSS via content-type spoofingavatar/avatar.go+v2/avatar/avatar.go:Proxy.resize()validates input viaimage.DecodeConfig(cheap, no pixelallocation) before any
image.Decode. Refuses non-image content withnilso attacker payloads never reach
Store.Put.New
maxAvatarPixelsconstant (16 MP). Multiplication inint64.The no-resize path returns the original bytes verbatim.
Proxy.Put()falls back to a generated identicon whenresizereturnsnil.Proxy.PutContent()returns an explicit error for non-image input.Proxy.Handler()validates served bytes via the newsafeImgContentTypehelper (allowlist:
image/{png,jpeg,gif,webp,bmp,x-icon};image/svg+xmlexplicitly rejected because SVG can execute scripts when navigated to top
level). This catches stores poisoned before this fix.
Every Handler response (success / 304 / error) carries:
Tests updated:
TestAvatar_resizeandTestAvatar_Routespreviously codifiedthe broken pass-through behavior; those assertions are inverted now.
New tests:
TestAvatar_PutRejectsNonImage— store is not poisoned by attackeru.Picture.TestAvatar_HandlerRejectsPoisonedStore— handler refuses pre-poisoned bytes.TestAvatar_resizeRejectsDecompressionBomb— DecodeConfig + int64 dim cap.provider/telegram_test.goplaceholder"png-bytes"is now a real 1x1 PNG(
tinyPNG) sincePutContentvalidates input.Commit 2 —
feat(auth): apply mandatory CSP and nosniff on every Service.Handlers() responseauth.go+v2/auth.go:Service.Handlers()wraps both returned handlers (auth route dispatcher andavatar handler) with
withSecurityHeaders. Sets:go-pkgz/auth's response surface is JSON-only for auth routes and images for
avatar — no consumer-customisable HTML anywhere — so this CSP is unconditionally
safe.
Commit 3 —
refactor(avatar): collapse load/resize double-bufferingavatar/avatar.go+v2/avatar/avatar.go:Before this commit a remote avatar was buffered twice: once inside
load()toenforce
maxAvatarFetchSize, again insideresize()forimage.DecodeConfig.For a 10 MiB upstream avatar each in-flight login held ~20 MiB peak.
load()returns([]byte, error)instead of(io.ReadCloser, error).resize()takes([]byte, int)instead of(io.Reader, int).PutContent()now runsio.ReadAll(io.LimitReader(content, cap+1))at thepublic API boundary so the size cap is visible to callers reading the code.
No public API change, no behavior change other than halved peak memory per
concurrent login.
What end-users will see
Anyone who upgrades and uses
Service.Handlers():Content-Security-Policy: default-src 'none'; sandbox; frame-ancestors 'none'X-Content-Type-Options: nosniffContent-Disposition: inline; filename="avatar".of being stored verbatim. The user gets an identicon where previously they
would have gotten a broken avatar (or, in the attacker case, JavaScript).
Proxy.PutContentnow returns an error for non-image input instead ofsilently storing whatever bytes the caller passed.
What might break
Nothing for standard consumers. Specifically:
headers are set on the response; downstream proxies pass them through.
If a consumer was relying on the avatar handler returning
text/htmlforinvalid content, that was the bug — they should not be doing that.
Proxy.PutContentwith invalid bytes — they nowget an error instead of an empty/poisoned avatar. This is the intended
behavior.
one in an avatar slot was always a latent XSS risk.
Release plan
Both
v1andv2module versions live onmasterin this repo, so a singlePR fixes both. After merge, tag
v1.x.yandv2.x.yfrom the same commit.remark42 will bump to
v2.x.yin its own follow-up PR.Codex review
The branch was reviewed adversarially with Codex (GPT-5.5, high reasoning) in
four rounds. Rounds 1 and 2 each surfaced one high-severity finding (the
decompression bomb on no-resize path, and the 32-bit int overflow on the
pixel-count guard) — both addressed in this PR. Rounds 3 and 4 approved with
no material findings.
Tests
All v1 and v2 test suites pass, lint clean (no new
gosec/govet/misspellfindings). Cross-compile to
GOARCH=386succeeds, confirming theint64arithmetic is portable.