Skip to content

docs(auth, avatar): fix misleading and stale docstrings around the security fix #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
umputun merged 1 commit into from
May 21, 2026
Merged

docs(auth, avatar): fix misleading and stale docstrings around the security fix #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 10 additions & 23 deletions auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,30 +239,17 @@ func (s *Service) Handlers() (authHandler, avatarHandler http.Handler) {
return withSecurityHeaders(http.HandlerFunc(ah)), withSecurityHeaders(http.HandlerFunc(s.avatarProxy.Handler))
}

// withSecurityHeaders wraps an auth response handler to apply strict CSP and nosniff
// on every response. The go-pkgz/auth package's own response surface is JSON-only
// for auth routes and images for the avatar route — no built-in HTML rendering
// anywhere — so this CSP is unconditionally safe and gives the auth origin
// defense-in-depth against any future trust-boundary regression that might emit a
// renderable body.
// withSecurityHeaders wraps a handler to set Content-Security-Policy
// "default-src 'none'; sandbox; frame-ancestors 'none'" and X-Content-Type-Options
// "nosniff" on every response. Safe to apply unconditionally because go-pkgz/auth
// only emits JSON (auth routes) and images (avatar route).
//
// - Content-Security-Policy: default-src 'none'; sandbox — 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.
//
// The avatar Handler additionally sets Content-Disposition: inline; filename="avatar"
// inside itself, so direct callers (tests, custom mounts) still get the full header
// set without going through this wrapper.
//
// CONSUMER NOTE: custom providers added via Service.AddCustomHandler / AddProvider
// are also wrapped. If a custom provider renders HTML (login forms, JS-based flows,
// 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'.
// CONSUMER NOTE: custom providers registered through AddCustomHandler / AddProvider
// are wrapped too. A provider that renders HTML (login form, JS flow, custom server
// login page, etc.) will be blocked by this CSP — default-src 'none' plus sandbox
// stop scripts, styles, forms, and images even when served from 'self'. Such
// providers must override the CSP on their own response (call w.Header().Set before
// writing — Set replaces the wrapper's value) and relax only the directives needed.
func withSecurityHeaders(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Security-Policy", "default-src 'none'; sandbox; frame-ancestors 'none'")
Expand Down
69 changes: 27 additions & 42 deletions avatar/avatar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,12 @@ import (
// http.sniffLen is 512 bytes which is how much we need to read to detect content type
const sniffLen = 512

// maxAvatarFetchSize bounds the bytes read from a remote avatar URL. 10 MiB is
// generous for any reasonable avatar (Telegram caps photo at 5 MiB; Gravatar is
// much smaller); the cap protects Proxy.Put against an upstream sending an
// unbounded body that would exhaust process memory inside resize.
// maxAvatarFetchSize caps the byte length of any avatar accepted by load,
// PutContent, or resize — i.e. the upper bound on bytes that ever reach Store.Put.
const maxAvatarFetchSize = 10 << 20

// maxAvatarPixels caps the declared pixel count of an avatar before any raster
// decode is allowed. Without this, a tiny compressed "decompression bomb" image
// declaring e.g. 65535x65535 px would force image.Decode to allocate gigabytes
// of pixel memory and OOM the auth service on a single login attempt. 16 MP
// covers any realistic avatar (~4096x4096) while keeping peak allocation bounded.
// maxAvatarPixels caps cfg.Width*cfg.Height (from image.DecodeConfig) before any
// full image.Decode runs. Defends against decompression-bomb inputs.
const maxAvatarPixels = 16 * 1024 * 1024

// Proxy provides http handler for avatars from avatar.Store
Expand All @@ -51,7 +46,11 @@ type Proxy struct {
ResizeLimit int
}

// Put stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url
// Put fetches u.Picture, validates and optionally resizes the body, stores it via
// Store, and returns the proxied avatar URL. If u.Picture is empty, the fetch fails,
// or the upstream bytes are not a recognized image format within the configured
// dimension and size limits, Put silently falls back to a generated identicon and
// returns its proxied URL — the caller is not told upstream was rejected.
func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err error) {

genIdenticon := func(userID string) (avatarURL string, err error) {
Expand Down Expand Up @@ -168,7 +167,9 @@ func (p *Proxy) load(url string, client *http.Client) ([]byte, error) {
return body, nil
}

// Handler returns token routes for given provider
// Handler serves stored avatar content by avatar id. GET only; rejects invalid ids
// (403) and stored bytes that fail the safeImgContentType sniff (415). Layered
// defense headers are set on every response via setAvatarDefenseHeaders.
func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
setAvatarDefenseHeaders(w)

Expand All @@ -195,11 +196,9 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
}
}()

// io.ReadFull keeps reading until the buffer is full or EOF, so a Store
// implementation that returns a buffered reader with a small first-Read size
// won't cause DetectContentType to misclassify a real image. Short bodies
// (avatars under sniffLen) return ErrUnexpectedEOF — that's expected, we sniff
// what we got.
// ReadFull (not Read) so a Store backend that hands back a short first Read
// doesn't truncate the sniff window. Short bodies are signaled by
// ErrUnexpectedEOF and treated like EOF.
buf := make([]byte, sniffLen)
n, err := io.ReadFull(avReader, buf)
if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF {
Expand All @@ -208,11 +207,9 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
return
}

// validate the bytes really are an image before declaring a content type. Even
// though Put() now refuses to store non-image content, this catches stores poisoned
// before the fix and any future regression — never trust the bytes at the store
// boundary alone, validate again at serve time. An empty body (e.g. NoOp store)
// is treated as a benign no-content case: nothing to render, no XSS surface.
// sniff buf[:n] against the safeImgContentType allowlist. This is not a full
// image decode; it catches stores poisoned before Put validated. An empty body
// (e.g. NoOp store) is treated as benign — nothing to render.
var contentType string
if n > 0 {
var ctErr error
Expand Down Expand Up @@ -248,20 +245,12 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
}
}

// resize validates that the input is a real image and, if needed, re-encodes it to
// fit within "limit" px on the larger side preserving aspect ratio. Returns nil for
// non-image content or for declared dimensions exceeding maxAvatarPixels so attacker
// payloads (HTML/SVG/decompression bombs) never reach the store. With limit <= 0 or
// when the image already fits, the original bytes are returned verbatim so animated
// GIFs and other multi-frame formats round-trip without being flattened to one frame.
//
// Validation uses image.DecodeConfig (cheap — declares dimensions, allocates nothing)
// before any full image.Decode, so a 100 KB compressed image declaring 65535x65535 px
// is rejected without ever materializing the raster.
//
// Callers (load, PutContent, identicon generation) must ensure body is bounded by
// maxAvatarFetchSize before calling; resize trusts the size invariant rather than
// re-buffering. An empty body or a body over the cap is refused defensively.
// resize checks body via image.DecodeConfig (format + dimensions only, no raster
// allocation), then either returns the original bytes verbatim (limit <= 0, or
// dimensions already within limit) or fully decodes and re-encodes to PNG fitting
// within limit px on the larger side. Returns nil for non-image input or for
// dimensions exceeding maxAvatarPixels. Caller must ensure body is within
// maxAvatarFetchSize; a body over the cap is also refused defensively.
func (p *Proxy) resize(body []byte, limit int) io.Reader {
if len(body) == 0 || int64(len(body)) > maxAvatarFetchSize {
p.Logf("[WARN] avatar resize(): refusing body of size %d (cap %d)", len(body), maxAvatarFetchSize)
Expand Down Expand Up @@ -315,13 +304,9 @@ func (p *Proxy) resize(body []byte, limit int) io.Reader {
return &out
}

// safeImgContentType returns the sniffed content type if the bytes look like a safe
// raster image format. The set is an explicit allowlist (PNG, JPEG, GIF, WebP, BMP,
// ICO) — no HasPrefix("image/") catch-all — so future scriptable image/* MIME types
// added by http.DetectContentType cannot silently pass. Returns an error otherwise.
// image/svg+xml is excluded because SVG can execute scripts when navigated to top
// level; image/* coverage of icon files uses both spellings http.DetectContentType
// is known to return.
// safeImgContentType returns the sniffed Content-Type for img if it is one of the
// allow-listed raster image formats (PNG, JPEG, GIF, WebP, BMP, ICO), else an error.
// SVG is excluded.
func safeImgContentType(img []byte) (string, error) {
ct := http.DetectContentType(img)
base := ct
Expand Down
33 changes: 10 additions & 23 deletions v2/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,30 +240,17 @@ func (s *Service) Handlers() (authHandler, avatarHandler http.Handler) {
return withSecurityHeaders(http.HandlerFunc(ah)), withSecurityHeaders(http.HandlerFunc(s.avatarProxy.Handler))
}

// withSecurityHeaders wraps an auth response handler to apply strict CSP and nosniff
// on every response. The go-pkgz/auth package's own response surface is JSON-only
// for auth routes and images for the avatar route — no built-in HTML rendering
// anywhere — so this CSP is unconditionally safe and gives the auth origin
// defense-in-depth against any future trust-boundary regression that might emit a
// renderable body.
// withSecurityHeaders wraps a handler to set Content-Security-Policy
// "default-src 'none'; sandbox; frame-ancestors 'none'" and X-Content-Type-Options
// "nosniff" on every response. Safe to apply unconditionally because go-pkgz/auth
// only emits JSON (auth routes) and images (avatar route).
//
// - Content-Security-Policy: default-src 'none'; sandbox — 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.
//
// The avatar Handler additionally sets Content-Disposition: inline; filename="avatar"
// inside itself, so direct callers (tests, custom mounts) still get the full header
// set without going through this wrapper.
//
// CONSUMER NOTE: custom providers added via Service.AddCustomHandler / AddProvider
// are also wrapped. If a custom provider renders HTML (login forms, JS-based flows,
// 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'.
// CONSUMER NOTE: custom providers registered through AddCustomHandler / AddProvider
// are wrapped too. A provider that renders HTML (login form, JS flow, custom server
// login page, etc.) will be blocked by this CSP — default-src 'none' plus sandbox
// stop scripts, styles, forms, and images even when served from 'self'. Such
// providers must override the CSP on their own response (call w.Header().Set before
// writing — Set replaces the wrapper's value) and relax only the directives needed.
func withSecurityHeaders(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Security-Policy", "default-src 'none'; sandbox; frame-ancestors 'none'")
Expand Down
69 changes: 27 additions & 42 deletions v2/avatar/avatar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,12 @@ import (
// http.sniffLen is 512 bytes which is how much we need to read to detect content type
const sniffLen = 512

// maxAvatarFetchSize bounds the bytes read from a remote avatar URL. 10 MiB is
// generous for any reasonable avatar (Telegram caps photo at 5 MiB; Gravatar is
// much smaller); the cap protects Proxy.Put against an upstream sending an
// unbounded body that would exhaust process memory inside resize.
// maxAvatarFetchSize caps the byte length of any avatar accepted by load,
// PutContent, or resize — i.e. the upper bound on bytes that ever reach Store.Put.
const maxAvatarFetchSize = 10 << 20

// maxAvatarPixels caps the declared pixel count of an avatar before any raster
// decode is allowed. Without this, a tiny compressed "decompression bomb" image
// declaring e.g. 65535x65535 px would force image.Decode to allocate gigabytes
// of pixel memory and OOM the auth service on a single login attempt. 16 MP
// covers any realistic avatar (~4096x4096) while keeping peak allocation bounded.
// maxAvatarPixels caps cfg.Width*cfg.Height (from image.DecodeConfig) before any
// full image.Decode runs. Defends against decompression-bomb inputs.
const maxAvatarPixels = 16 * 1024 * 1024

// Proxy provides http handler for avatars from avatar.Store
Expand All @@ -51,7 +46,11 @@ type Proxy struct {
ResizeLimit int
}

// Put stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url
// Put fetches u.Picture, validates and optionally resizes the body, stores it via
// Store, and returns the proxied avatar URL. If u.Picture is empty, the fetch fails,
// or the upstream bytes are not a recognized image format within the configured
// dimension and size limits, Put silently falls back to a generated identicon and
// returns its proxied URL — the caller is not told upstream was rejected.
func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err error) {

genIdenticon := func(userID string) (avatarURL string, err error) {
Expand Down Expand Up @@ -168,7 +167,9 @@ func (p *Proxy) load(url string, client *http.Client) ([]byte, error) {
return body, nil
}

// Handler returns token routes for given provider
// Handler serves stored avatar content by avatar id. GET only; rejects invalid ids
// (403) and stored bytes that fail the safeImgContentType sniff (415). Layered
// defense headers are set on every response via setAvatarDefenseHeaders.
func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
setAvatarDefenseHeaders(w)

Expand All @@ -195,11 +196,9 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
}
}()

// io.ReadFull keeps reading until the buffer is full or EOF, so a Store
// implementation that returns a buffered reader with a small first-Read size
// won't cause DetectContentType to misclassify a real image. Short bodies
// (avatars under sniffLen) return ErrUnexpectedEOF — that's expected, we sniff
// what we got.
// ReadFull (not Read) so a Store backend that hands back a short first Read
// doesn't truncate the sniff window. Short bodies are signaled by
// ErrUnexpectedEOF and treated like EOF.
buf := make([]byte, sniffLen)
n, err := io.ReadFull(avReader, buf)
if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF {
Expand All @@ -208,11 +207,9 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
return
}

// validate the bytes really are an image before declaring a content type. Even
// though Put() now refuses to store non-image content, this catches stores poisoned
// before the fix and any future regression — never trust the bytes at the store
// boundary alone, validate again at serve time. An empty body (e.g. NoOp store)
// is treated as a benign no-content case: nothing to render, no XSS surface.
// sniff buf[:n] against the safeImgContentType allowlist. This is not a full
// image decode; it catches stores poisoned before Put validated. An empty body
// (e.g. NoOp store) is treated as benign — nothing to render.
var contentType string
if n > 0 {
var ctErr error
Expand Down Expand Up @@ -248,20 +245,12 @@ func (p *Proxy) Handler(w http.ResponseWriter, r *http.Request) {
}
}

// resize validates that the input is a real image and, if needed, re-encodes it to
// fit within "limit" px on the larger side preserving aspect ratio. Returns nil for
// non-image content or for declared dimensions exceeding maxAvatarPixels so attacker
// payloads (HTML/SVG/decompression bombs) never reach the store. With limit <= 0 or
// when the image already fits, the original bytes are returned verbatim so animated
// GIFs and other multi-frame formats round-trip without being flattened to one frame.
//
// Validation uses image.DecodeConfig (cheap — declares dimensions, allocates nothing)
// before any full image.Decode, so a 100 KB compressed image declaring 65535x65535 px
// is rejected without ever materializing the raster.
//
// Callers (load, PutContent, identicon generation) must ensure body is bounded by
// maxAvatarFetchSize before calling; resize trusts the size invariant rather than
// re-buffering. An empty body or a body over the cap is refused defensively.
// resize checks body via image.DecodeConfig (format + dimensions only, no raster
// allocation), then either returns the original bytes verbatim (limit <= 0, or
// dimensions already within limit) or fully decodes and re-encodes to PNG fitting
// within limit px on the larger side. Returns nil for non-image input or for
// dimensions exceeding maxAvatarPixels. Caller must ensure body is within
// maxAvatarFetchSize; a body over the cap is also refused defensively.
func (p *Proxy) resize(body []byte, limit int) io.Reader {
if len(body) == 0 || int64(len(body)) > maxAvatarFetchSize {
p.Logf("[WARN] avatar resize(): refusing body of size %d (cap %d)", len(body), maxAvatarFetchSize)
Expand Down Expand Up @@ -315,13 +304,9 @@ func (p *Proxy) resize(body []byte, limit int) io.Reader {
return &out
}

// safeImgContentType returns the sniffed content type if the bytes look like a safe
// raster image format. The set is an explicit allowlist (PNG, JPEG, GIF, WebP, BMP,
// ICO) — no HasPrefix("image/") catch-all — so future scriptable image/* MIME types
// added by http.DetectContentType cannot silently pass. Returns an error otherwise.
// image/svg+xml is excluded because SVG can execute scripts when navigated to top
// level; image/* coverage of icon files uses both spellings http.DetectContentType
// is known to return.
// safeImgContentType returns the sniffed Content-Type for img if it is one of the
// allow-listed raster image formats (PNG, JPEG, GIF, WebP, BMP, ICO), else an error.
// SVG is excluded.
func safeImgContentType(img []byte) (string, error) {
ct := http.DetectContentType(img)
base := ct
Expand Down
Loading