Dmitry VerkhoturovandGitHub
0e20861419
* 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.
104 lines
4.3 KiB
Go
104 lines
4.3 KiB
Go
package rest
|
|
|
|
import (
|
|
"net/http/httptest"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// TestEtagMatches covers the strict If-None-Match parser that replaced a substring
|
|
// search prone to false positives (etag "abc" being matched inside "fooabc").
|
|
func TestEtagMatches(t *testing.T) {
|
|
tbl := []struct {
|
|
name string
|
|
header string
|
|
etag string
|
|
want bool
|
|
}{
|
|
{"exact match", `"v2:abc"`, `"v2:abc"`, true},
|
|
{"comma-separated, second matches", `"x", "v2:abc"`, `"v2:abc"`, true},
|
|
{"weak validator prefix", `W/"v2:abc"`, `"v2:abc"`, true},
|
|
{"wildcard matches anything", `*`, `"v2:abc"`, true},
|
|
{"leading/trailing whitespace", ` "v2:abc" `, `"v2:abc"`, true},
|
|
{"substring not enough", `"v2:abcdef"`, `"v2:abc"`, false},
|
|
{"prefix-only mismatch", `"v2:ab"`, `"v2:abc"`, false},
|
|
{"pre-fix etag no longer matches v2", `"abc"`, `"v2:abc"`, false},
|
|
{"empty header", ``, `"v2:abc"`, false},
|
|
{"different etag", `"v2:xyz"`, `"v2:abc"`, false},
|
|
}
|
|
for _, tt := range tbl {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
assert.Equal(t, tt.want, EtagMatches(tt.header, tt.etag))
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestSetImageDefenseHeaders(t *testing.T) {
|
|
w := httptest.NewRecorder()
|
|
SetImageDefenseHeaders(w)
|
|
assert.Equal(t, StrictImageCSP, w.Header().Get("Content-Security-Policy"))
|
|
assert.Equal(t, "nosniff", w.Header().Get("X-Content-Type-Options"))
|
|
assert.Equal(t, `inline; filename="image"`, w.Header().Get("Content-Disposition"))
|
|
}
|
|
|
|
// TestSafeImgContentType exercises the strict allowlist. The previous behavior
|
|
// (HasPrefix "image/" with an explicit image/svg+xml carve-out) is gone — the
|
|
// allowlist is the source of truth, and the explicit svg branch was dead code
|
|
// because http.DetectContentType never returns image/svg+xml (real SVG bodies
|
|
// sniff as text/xml or text/plain depending on whether they carry an XML decl,
|
|
// so they are rejected implicitly by not matching the allowlist).
|
|
func TestSafeImgContentType(t *testing.T) {
|
|
// minimal magic-byte bodies — verified via http.DetectContentType to produce
|
|
// the expected image/* result without needing testdata files for every format
|
|
pngMagic := []byte("\x89PNG\r\n\x1a\n")
|
|
jpegMagic := []byte("\xff\xd8\xff\xe0\x00\x10JFIF\x00")
|
|
gifBytes := []byte("GIF89a")
|
|
webpBytes := []byte("RIFF\x00\x00\x00\x00WEBPVP8 ")
|
|
bmpBytes := []byte("BM\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")
|
|
icoBytes := []byte("\x00\x00\x01\x00\x01\x00")
|
|
|
|
// SVG with XML decl sniffs as text/xml — rejected because it's not in the allowlist
|
|
svgWithXMLDecl := []byte(`<?xml version="1.0"?><svg xmlns="http://www.w3.org/2000/svg" onload="alert(1)"></svg>`)
|
|
// SVG without XML decl sniffs as text/plain — also rejected
|
|
svgPlain := []byte(`<svg xmlns="http://www.w3.org/2000/svg" width="10"></svg>`)
|
|
|
|
tbl := []struct {
|
|
name string
|
|
body []byte
|
|
wantCT string
|
|
wantErr bool
|
|
}{
|
|
{name: "nil rejected", body: nil, wantErr: true},
|
|
{name: "empty rejected", body: []byte{}, wantErr: true},
|
|
{name: "png magic accepted", body: pngMagic, wantCT: "image/png"},
|
|
{name: "jpeg magic accepted", body: jpegMagic, wantCT: "image/jpeg"},
|
|
{name: "gif accepted", body: gifBytes, wantCT: "image/gif"},
|
|
{name: "webp accepted", body: webpBytes, wantCT: "image/webp"},
|
|
{name: "bmp accepted", body: bmpBytes, wantCT: "image/bmp"},
|
|
{name: "ico accepted", body: icoBytes, wantCT: "image/x-icon"},
|
|
{name: "html doc rejected", body: []byte(`<!DOCTYPE html><html></html>`), wantErr: true},
|
|
{name: "html fragment rejected", body: []byte(`<body><img></body>`), wantErr: true},
|
|
{name: "plain text rejected", body: []byte("hello world"), wantErr: true},
|
|
{name: "octet-stream rejected", body: []byte{0x00, 0x01, 0x02, 0x03, 0x04}, wantErr: true},
|
|
{name: "svg with xml decl rejected (sniffs as text/xml)", body: svgWithXMLDecl, wantErr: true},
|
|
{name: "svg without xml decl rejected (sniffs as text/plain)", body: svgPlain, wantErr: true},
|
|
}
|
|
for _, tt := range tbl {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
got, err := SafeImgContentType(tt.body)
|
|
if tt.wantErr {
|
|
require.Error(t, err)
|
|
assert.Empty(t, got)
|
|
assert.Contains(t, err.Error(), "non-image content type")
|
|
return
|
|
}
|
|
require.NoError(t, err)
|
|
assert.Equal(t, tt.wantCT, got)
|
|
// returned type must never carry a charset suffix (the strip code path)
|
|
assert.NotContains(t, got, ";")
|
|
})
|
|
}
|
|
}
|