Files
remark42/backend/app/rest/proxy/image.go
T
Dmitry VerkhoturovandGitHub 0e20861419 fix(security): reject non-image content-types in image proxy and /picture/ to prevent stored XSS (#2067)
* fix(security): reject non-image content-types in image proxy and /picture/ to prevent stored XSS

The /api/v1/img proxy and /api/v1/picture/{user}/{id} endpoints emitted
http.DetectContentType on the served bytes as the response Content-Type. A
controlled upstream serving Content-Type: image/png with an HTML body passed
the upstream check (only the response header was inspected, not the body),
and the body bytes then sniffed back to text/html — so the proxy served the
attacker's HTML from the remark42 origin. Browsers honoured the declared
text/html and executed the response as a document with access to cookies and
CSRF tokens. Affected from v1.6.0 (April 2020) through v1.15.0; verified live
via published docker images.

Layered defense applied to both handlers:

- rest.SafeImgContentType (in backend/app/rest/) validates sniffed content
  against a strict allowlist: image/png, image/jpeg, image/gif, image/webp,
  image/bmp, image/x-icon. Anything else (HTML, XML, SVG, plain text,
  octet-stream, or any future image type the stdlib sniffer may learn) is
  rejected with no body echo. SVG is implicitly excluded — it sniffs as
  text/xml or text/plain, never image/svg+xml, and SVG can execute scripts
  when navigated to top-level. The previous octet-stream → image/* fallback
  is gone.
- Per-endpoint Content-Security-Policy override sets
  "default-src 'none'; sandbox; frame-ancestors 'none'" on every response
  (success, 304, or error). Sandbox neuters scripts even if Content-Type
  ever regresses. The same policy is also applied to all /api/v1/* via
  apiCSPMiddleware as defense-in-depth.
- Content-Disposition: inline; filename="image" frames the response as a
  file rather than a renderable document.
- /picture/ rejection paths set Cache-Control: no-store so 4xx responses
  are never cached.

The defense headers and the strict ETag matcher are extracted as
rest.SetImageDefenseHeaders and rest.EtagMatches in the shared rest package
(consumed by both proxy/image and api/rest_public — no package cycle).

The /api/v1/img path additionally bumps the ETag to a versioned `"v2:..."`
so revalidating clients (top-level navigation, Ctrl+R, intermediaries) get
a fresh 200 instead of a 304 against poisoned pre-fix cached HTML.

DELIBERATE TRADEOFF: Cache-Control on /api/v1/img success responses remains
max-age=2592000 (30 days), unchanged from before. An aggressive "force
revalidate on every reuse" policy was prototyped during review but reverted
because the perf cost (a server round-trip on every image view, even with
304 saving the body bytes) outweighed the corner-case mitigation. The
realistic exposure of cache carryover is narrow: cache carryover only
affects users who navigated top-level to an attacker URL pre-fix and still
have it in their local cache — the normal <img> embed path cached text/html
but never executed it. Local browser caches that hold pre-fix bytes
continue to serve them until their 30-day TTL expires or are evicted under
memory pressure. The ETag bump reaches all clients that DO revalidate
during the cached lifetime (Ctrl+R, intermediaries, post-expiry use); for
the rest, exposure self-limits via cache expiry. Operators running a
CDN/edge cache in front of remark42 should purge /api/v1/img after deploy.

The /api/v1/img handler short-circuits on a matching current-version
If-None-Match before any store Load or upstream fetch, returning a bodyless
304 with the defense headers set. Safe because the 304 carries no body and
the client's cached bytes came from a prior validated 200; an attacker
fabricating an etag value can only short-circuit fetches for URLs they
themselves crafted. This avoids upstream DoS amplification when clients
revalidate on hot comment pages.

The /api/v1/img route was moved from the "open routes" group (which uses
middleware.NoCache, stripping If-None-Match from incoming requests) to the
"open routes, cached" group alongside /picture/ and /qr/telegram so the
304 revalidation path is no longer broken upstream of the handler.

The /picture/{user}/{id} endpoint does not need the v2 etag prefix. Upload
validates input format via readAndValidateImage and the serve path
re-validates the stored bytes via rest.SafeImgContentType. Bytes within
the resize dimension limits are preserved verbatim, so the browser defense
relies on the response headers (validated Content-Type + nosniff + strict
CSP + Content-Disposition: inline), not on byte normalization.

Global CSP: font-src data: → font-src 'none'. Audit confirmed no @font-face,
no base64 fonts, no icon-font library in the bundle. Drops an unnecessary
attack surface; no behavioural change.

Tests: TestImage_ContentTypeHandling table-tests a real PNG and attack
shapes (HTML claimed as image/png, image/jpeg, image/gif, image/svg+xml,
image/webp; svg with onload; html fragment; polyglot PNG+HTML), proving
the defense holds across arbitrary upstream Content-Type variation.
Polyglot case is intentionally served as image/png — the browser cannot
execute the trailing HTML when the response type is image/png with nosniff.
TestImage_ContentTypeHandling_CacheHit exercises the cache-hit branch with
attacker bytes preloaded into the store. TestImage_PerRequestRevalidation
alternates upstream PNG/HTML across four proxy calls to prove no trust
accumulates between requests. TestImage_RoutesUsingCachedImage asserts
cache-poisoning is caught at serve time. TestImage_EtagVersioned asserts
the v2 prefix invalidates pre-fix etags AND that the revalidation 304
triggers no store Load. TestImage_RevalidationSkipsIO proves the
short-circuit works even with no upstream reachable. TestSafeImgContentType
covers the allowlist directly. TestRest_LoadPictureDefenseHeaders and
TestRest_LoadPictureRejectsNonImage exercise the /picture/ endpoint.
TestRest_apiCSP covers the strict CSP middleware on JSON API + RSS routes;
TestRest_securityHeaders confirms /web/ HTML pages keep the global CSP.

Verified end-to-end against the dev docker image: the original demo URL
(arbitrary HTML claimed as image/png) now returns 415 application/json with
CSP/nosniff/Content-Disposition set, no XSS in the browser.

* fix(security): set Cache-Control: no-store on image-proxy error paths, sync stale route comment

Addresses two review comments on #2067:

1. Cache-Control: max-age=2592000 and Etag were set before the
   load/download/validation block, so 404/400/415 error responses inherited
   the 30-day cache TTL and the versioned etag — a transient failure (or an
   intentionally triggered 415) would be pinned in browser/intermediary
   caches for that TTL, keeping users locked out even after the underlying
   cause was resolved. Now: etag is computed but not set as a header until
   after validation succeeds; error paths route through sendImageProxyError
   which sets Cache-Control: no-store and never sets Etag. The 304
   short-circuit still sets both because that path serves the same validated
   content the client already has cached.

2. The comment at rest.go:282 still described the prototyped
   no-cache/must-revalidate Cache-Control policy that was reverted before
   the PR landed. Updated to match the actual 30-day max-age behavior.

Tests: TestImage_ContentTypeHandling now asserts reject paths carry
Cache-Control: no-store and have no Etag header, and accept paths carry
the max-age=2592000 + v2: etag.
2026-05-20 22:37:25 -05:00

262 lines
9.8 KiB
Go

package proxy
import (
"bytes"
"context"
"encoding/base64"
"errors"
"fmt"
"io"
"net/http"
"strings"
"time"
"github.com/PuerkitoBio/goquery"
log "github.com/go-pkgz/lgr"
"github.com/go-pkgz/repeater/v2"
"github.com/umputun/remark42/backend/app/rest"
"github.com/umputun/remark42/backend/app/safehttp"
"github.com/umputun/remark42/backend/app/store/image"
)
// errInvalidUpstreamContentType is returned by downloadImage when the upstream's
// Content-Type header is not image/*. The handler checks via errors.Is to convert
// it into a 400 (input rejected) instead of the generic 404 (fetch failed).
var errInvalidUpstreamContentType = errors.New("invalid upstream content type")
// Image extracts image src from comment's html and provides proxy for them
// this is needed to keep remark42 running behind of HTTPS serve all images via https
type Image struct {
RemarkURL string
RoutePath string
HTTP2HTTPS bool
CacheExternal bool
Timeout time.Duration
ImageService *image.Service
// Transport, if non-nil, is used as-is for outbound image fetches and is the
// caller's responsibility to make SSRF-safe. When nil, safehttp.Transport()
// is installed, which blocks dialing any private/reserved IP and resolves
// hostnames to defeat DNS rebinding.
Transport http.RoundTripper
}
// Convert img src links to proxied links depends on enabled options
func (p Image) Convert(commentHTML string) string {
if p.CacheExternal {
imgs, err := p.extract(commentHTML, func(img string) bool { return !strings.HasPrefix(img, p.RemarkURL) })
if err != nil {
return commentHTML
}
commentHTML = p.replace(commentHTML, imgs)
}
if p.HTTP2HTTPS && !strings.HasPrefix(p.RemarkURL, "http://") {
imgs, err := p.extract(commentHTML, func(img string) bool { return strings.HasPrefix(img, "http://") })
if err != nil {
return commentHTML
}
commentHTML = p.replace(commentHTML, imgs)
}
return commentHTML
}
// extract gets all images matching predicate and return list of src
func (p Image) extract(commentHTML string, imgSrcPred func(string) bool) ([]string, error) {
doc, err := goquery.NewDocumentFromReader(strings.NewReader(commentHTML))
if err != nil {
return nil, fmt.Errorf("can't create document: %w", err)
}
result := []string{}
doc.Find("img").Each(func(_ int, s *goquery.Selection) {
if im, ok := s.Attr("src"); ok {
if imgSrcPred(im) {
result = append(result, im)
}
}
})
return result, nil
}
// replace img links in commentHTML with route to proxy, base64 encoded original link
func (p Image) replace(commentHTML string, imgs []string) string {
for _, img := range imgs {
encodedImgURL := base64.URLEncoding.EncodeToString([]byte(img))
resImgURL := p.RemarkURL + p.RoutePath + "?src=" + encodedImgURL
commentHTML = strings.ReplaceAll(commentHTML, img, resImgURL)
}
return commentHTML
}
// etagVersionPrefix is the security-version tag bumped whenever cached responses for the
// same src need to be invalidated. Pre-fix responses were served as text/html and cached
// by browsers/proxies under ETag `"<base64(src)>"`; the prefix invalidates those validators
// so revalidating clients get a fresh 200 instead of letting the cached HTML 304.
//
// LIMITATION: with the 30-day max-age below, browsers serve pre-fix bytes from their
// local cache without contacting the server until that TTL expires or the cache is
// evicted under memory pressure. The prefix only helps clients that revalidate during
// the cached lifetime (Ctrl+R, intermediaries, post-expiry use). Operators running a
// CDN/edge cache in front of remark42 should purge /api/v1/img after deploy. The
// realistic exposure is narrow: cache carryover only affects users who navigated
// top-level to an attacker URL pre-fix and still have that URL cached — the normal
// <img> embed path cached text/html but never executed it.
const etagVersionPrefix = "v2:"
// Handler returns http handler respond to proxied request
func (p Image) Handler(w http.ResponseWriter, r *http.Request) {
rest.SetImageDefenseHeaders(w)
srcParam := r.URL.Query().Get("src")
src, err := base64.URLEncoding.DecodeString(srcParam)
if err != nil {
sendImageProxyError(w, r, http.StatusBadRequest, err, "can't decode image url", rest.ErrDecode)
return
}
imgURL := string(src)
imgID, err := image.CachedImgID(imgURL)
if err != nil {
sendImageProxyError(w, r, http.StatusBadRequest, fmt.Errorf("invalid image url"), "can't parse image url", rest.ErrAssetNotFound)
return
}
// compute the current-version etag once. We don't set it as a response header yet
// because error paths below must NOT inherit it — otherwise transient failures
// (4xx) would get cached alongside the 30-day Cache-Control of the success path.
// The etag (and Cache-Control) are set only on the 304 short-circuit and the
// validated 200 path.
etag := `"` + etagVersionPrefix + srcParam + `"`
// short-circuit revalidation before any cache lookup or upstream fetch: a matching
// current-version If-None-Match means the client already has bytes from a prior
// successful (post-fix, validated) 200, so a bodyless 304 is safe and avoids
// upstream DoS amplification on hot comment pages without CacheExternal.
if match := r.Header.Get("If-None-Match"); match != "" && rest.EtagMatches(match, etag) {
w.Header().Set("Etag", etag)
w.Header().Set("Cache-Control", "max-age=2592000") // 30 days
w.WriteHeader(http.StatusNotModified)
return
}
// try to load from cache for case it was saved when CacheExternal was enabled
img, _ := p.ImageService.Load(imgID)
if img == nil {
img, err = p.downloadImage(r.Context(), imgURL)
if err != nil {
log.Printf("[WARN] failed to download image: %v", err)
if errors.Is(err, errInvalidUpstreamContentType) {
sendImageProxyError(w, r, http.StatusBadRequest, fmt.Errorf("invalid content type"), "invalid content type", rest.ErrImgNotFound)
return
}
sendImageProxyError(w, r, http.StatusNotFound, fmt.Errorf("failed to fetch"), "can't get image", rest.ErrAssetNotFound)
return
}
if p.CacheExternal {
p.cacheImage(bytes.NewReader(img), imgID)
}
}
// validate body bytes are actually an image — never trust upstream Content-Type or cache
contentType, err := rest.SafeImgContentType(img)
if err != nil {
log.Printf("[WARN] rejecting non-image content from %s: %v", imgURL, err)
sendImageProxyError(w, r, http.StatusUnsupportedMediaType, err, "invalid image content", rest.ErrImgNotFound)
return
}
// success path: long-lived client cache with etag for cheap revalidation. 30-day
// TTL keeps the proxy efficient for hot pages; when clients DO revalidate
// (Ctrl+R, intermediaries, post-expiry), the versioned etag ensures pre-fix
// poisoned validators don't match and a fresh validated 200 is returned. See
// etagVersionPrefix godoc for the limitation on browser-local caches.
w.Header().Set("Etag", etag)
w.Header().Set("Cache-Control", "max-age=2592000") // 30 days
w.Header().Set("Content-Type", contentType)
_, err = io.Copy(w, bytes.NewReader(img))
if err != nil {
log.Printf("[WARN] can't copy image stream, %s", err)
}
}
// sendImageProxyError writes a no-store error response so a transient failure (4xx)
// cannot inherit the success path's 30-day Cache-Control or the versioned ETag, which
// would otherwise pin the error in the browser/intermediary cache for that TTL.
// Defense headers from SetImageDefenseHeaders at the top of the handler survive.
func sendImageProxyError(w http.ResponseWriter, r *http.Request, status int, err error, details string, errCode int) {
w.Header().Set("Cache-Control", "no-store")
rest.SendErrorJSON(w, r, status, err, details, errCode)
}
// cache image from provided Reader using given ID
func (p Image) cacheImage(r io.Reader, imgID string) {
err := p.ImageService.SaveWithID(imgID, r)
if err != nil {
log.Printf("[WARN] unable to save image to the storage: %+v", err)
}
}
// download an image.
func (p Image) downloadImage(ctx context.Context, imgURL string) ([]byte, error) {
log.Printf("[DEBUG] downloading image %s", imgURL)
timeout := 60 * time.Second // default
if p.Timeout > 0 {
timeout = p.Timeout
}
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
transport := p.Transport
if transport == nil {
transport = safehttp.Transport()
}
client := http.Client{
Timeout: 30 * time.Second,
Transport: transport,
}
defer client.CloseIdleConnections()
var resp *http.Response
err := repeater.NewFixed(5, time.Second).Do(ctx, func() error {
var e error
// SSRF safety: client.Transport is safehttp.Transport() when p.Transport is nil
// (see Image.Transport contract above); when caller supplies a transport they
// own SSRF safety for that path.
req, e := http.NewRequest("GET", imgURL, http.NoBody) //nolint:gosec // see comment above
if e != nil {
return fmt.Errorf("failed to make request for %s: %w", imgURL, e)
}
resp, e = client.Do(req.WithContext(ctx)) //nolint:bodyclose,gosec // body closed in defer; transport contract above
return e
})
if err != nil {
return nil, fmt.Errorf("can't download image %s: %w", imgURL, err)
}
defer resp.Body.Close() //nolint gosec // we don't care about response body
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("got unsuccessful response status %d while fetching %s", resp.StatusCode, imgURL)
}
contentType := resp.Header.Get("Content-Type")
if !strings.HasPrefix(contentType, "image/") {
return nil, fmt.Errorf("%w: %s", errInvalidUpstreamContentType, contentType)
}
maxSize := 5 * 1024 * 1024 // 5MB default
if p.ImageService != nil && p.ImageService.MaxSize > 0 {
maxSize = p.ImageService.MaxSize
}
lr := io.LimitReader(resp.Body, int64(maxSize)+1)
imgData, err := io.ReadAll(lr)
if err != nil {
return nil, fmt.Errorf("unable to read image body: %w", err)
}
if len(imgData) > maxSize {
return nil, fmt.Errorf("image is too large")
}
return imgData, nil
}