Rate limiting and (with --votes-ip) vote de-duplication key on the client IP,
recovered from forwarding headers (X-Real-IP / X-Forwarded-For / CF-Connecting-IP)
when behind a reverse proxy. Those headers were accepted from any client, so a
caller could set them to change its apparent IP.
Add --trusted-proxy / TRUSTED_PROXY (comma-separated CIDR/IP): forwarding headers
are honored only when the direct peer is a trusted proxy; other peers keep their
real socket address. Unset preserves the previous trust-all behavior (with a
startup warning) so existing deployments keep working on upgrade.
Docs: a 'Trusted proxies and client IP' section with per-topology guidance, plus a
note in the nginx manual.
Bumps go-pkgz/auth to v2.1.5, which adds avatar.ErrNotFound. deleteMeRequestCtrl's
avatar removal was best-effort (log and continue on any error) because before the
sentinel there was no portable way to tell an already-removed avatar from a genuine
failure. It now tolerates only errors.Is(err, avatar.ErrNotFound) - keeping the
repeated-request idempotency - and surfaces any other store failure as 500.
The delete_me token built by deleteMeCtrl omitted the user's Picture, so the
avatar-removal branch in deleteMeRequestCtrl never ran for real requests and
avatars survived account deletion. Carry Picture in the token so the stored
avatar is removed when the request is processed.
Make the removal best-effort: the avatar stores report an already-missing
avatar as an error with no distinguishable sentinel, and the user's data is
already deleted at that point, so a missing avatar (e.g. a repeated request)
no longer fails the whole deletion with a 400.
Only remove a well-formed avatar id ("<hash>.image") so a malformed picture
can't make a filesystem-backed store target an unexpected path.
go-pkgz/rest v1.22.0 ships an enforcing Timeout middleware (net/http.TimeoutHandler
style): it runs the handler with a deadline and returns 504 at the deadline even if
the handler ignores the context - unlike the local cooperative timeout, which only
cancelled the context and never actually stopped a stuck handler.
Replace the local timeout with rest.Timeout on every route with a bounded response.
The streaming and long-polling routes are deliberately left without it, since the
enforcing timeout buffers the whole response in memory and aborts at the deadline:
- GET /api/v1/userdata and GET /api/v1/admin/export stream gzipped exports
- GET /api/v1/admin/wait long-polls for up to 15m
- POST /api/v1/admin/import[/form] and /remap ingest large uploads
Delete the local timeout middleware and its test; the enforcing behaviour is covered
by go-pkgz/rest. TestRouteTimeout locks the enforcing-vs-exempt contract in this build.
v1.22.0 includes the preflight Vary fix (https://github.com/go-pkgz/rest/pull/44):
rest.CORS now adds Vary: Access-Control-Request-Method and
Access-Control-Request-Headers on preflight itself, making the local wrapper
that added them redundant. corsMiddleware now returns rest.CORS directly;
TestCorsMiddleware still asserts those preflight Vary headers, now supplied
upstream.
Also tidies the _example/memory_store module for the new version.
deleteUser now succeeds for a user who has no comments (e.g. one who only logged
in) instead of failing on the missing per-user bucket. In hard mode the per-user
bucket is deleted, tolerating bbolt's ErrBucketNotFound so a bucket left behind by
an earlier partial removal is still removed; the comment-deletion failure path now
wraps the actual error.
Because the engine cannot distinguish a valid login-only user from a never-existed
one, deletion is idempotent: /admin/deleteme returns 200 for an unknown (but validly
signed) token rather than 400. The deleteme test is updated to this contract, engine
tests cover hard and soft deletion of login-only and unknown users, and the API docs
note the idempotent behaviour.
Migrate the REST router off go-chi/chi onto go-pkgz/routegroup (backed by the
stdlib http.ServeMux), removing the last use of go-chi from the backend:
- rest.go routes() builds the tree with routegroup (Mount/Group/Route/With) and
net/http method+path patterns instead of chi's Get/Post/Route/Mount helpers
- chi.URLParam(...) -> r.PathValue(...) in the admin, public and private handlers
- rest_public_test.go loadPictureCtrl test uses routegroup + http.ServeMux
- rest_test.go: add TestRest_FileServerStaticAssets (bare /web -> /web/ redirect,
cache headers, 404, directory-listing block) and update the path-traversal test
for ServeMux normalising a literal ".." (encoded traversal is still rejected
by the handler)
- drop go-chi/chi from go.mod, go.sum and vendor; update the CLAUDE.md reference
Reword "May be" to "Maybe" in the auth.no-providers message and run
translation:generate to register the key in every locale dictionary, then
replace the English placeholders with proper translations for each locale.
Add a test asserting the error is hidden when providers are configured.
Swap the go-chi/cors middleware for rest.CORS (already a dependency),
removing the go-chi/cors module entirely. Behaviour-preserving:
- with AllowedOrigins "*" and credentials enabled, both reflect the request
Origin into Access-Control-Allow-Origin (a literal "*" is invalid with
credentials)
- preflight responses also vary on Access-Control-Request-Method/-Headers, not
just Origin, matching go-chi/cors so caches don't reuse a preflight response
across different requests
Extract the config into corsMiddleware() in middleware.go and add
TestCorsMiddleware covering origin reflection, credentials, preflight
methods/headers/max-age/Vary, and the no-Origin case. go-chi/cors dropped
from go.mod; the chi router stays until the router migration. go test -race,
vet, golangci-lint, govulncheck clean.
The cleanup command test builds a self-contained mock HTTP server with
only static routes (and {id} patterns read via r.URL.Path, not URLParam),
so chi.NewRouter is unnecessary — http.NewServeMux (Go 1.22 routing) covers
it. Independent of the main router; drops the chi import from app/cmd.
go test -race and golangci-lint clean.
Pure relocation, no behaviour change: gather all request-scoped middlewares
and their tests into dedicated files instead of scattering them across
rest.go and ssl.go.
funcs -> app/rest/api/middleware.go:
timeout (from ssl.go); rejectAnonUser, matchSiteID, cacheControl,
apiCSPMiddleware, securityHeadersMiddleware, subscribersOnly,
validEmailAuth, rateLimiter (from rest.go)
tests -> app/rest/api/middleware_test.go:
TestTimeout (from ssl_test.go); TestRest_rejectAnonUser,
TestRest_cacheControl, TestRest_apiCSP, TestRest_securityHeaders,
TestRest_subscribersOnly, Test_validEmailAuth, TestRest_matchSiteID
(from rest_test.go)
go test -race, vet, golangci-lint and govulncheck clean; example builds.
middleware.Timeout was the last use of go-chi/chi/v5/middleware (RealIP, the
other user, landed in #2099). Swap it for the local timeout helper (context
deadline + 504 on deadline, matching chi exactly; covered by TestTimeout),
which removes the go-chi/chi/v5/middleware package from the vendor tree.
The go-chi/chi module stays in go.mod — the router (chi.NewRouter etc.) still
uses it, so go.mod only shrinks after the router migration. Build, vet, race
tests and golangci-lint clean.
Behaviour-preserving swap: rest.RealIP sets r.RemoteAddr from
X-Real-IP / X-Forwarded-For like chi's middleware.RealIP, removing chi from
the RealIP path without changing the trust model (GHSA-56x6-q882-mf27 stays
present, to be fixed separately). chi/middleware stays imported for Timeout;
whichever of this PR and the Timeout PR (#2097) merges last drops the import.
Drop-in to a tested rest middleware; covered by existing api router tests.
go-pkgz/rest.NoCache is borrowed from chi's middleware.NoCache and behaves
identically: same no-cache response headers and the same stripping of
conditional request headers (If-None-Match etc.), which the image-proxy
etag logic relies on. Drop-in swap, chi router left in place.
chi/middleware stays imported for RealIP and Timeout. Build, vet, race
tests and golangci-lint clean.
Drop-in swap of the global concurrency limiter (go-pkgz/rest.Throttle has
the same signature and semantics as chi's middleware.Throttle), with the
chi router left in place. First of the per-middleware swaps that chip away
at go-chi/chi/v5/middleware before the router itself is migrated.
chi/middleware is still imported for RealIP/Timeout/NoCache; build, vet,
race tests and golangci-lint clean.
First step of the go-chi -> go-pkgz/routegroup migration. The HTTP->HTTPS
redirect and ACME http-01 challenge routers are small, self-contained
http.Handlers separate from the main API router, so they move cleanly:
- chi.NewRouter() -> routegroup.New(http.NewServeMux())
- middleware.Throttle -> rest.Throttle (same concurrency-limit semantics)
- middleware.Timeout -> local timeout helper (context deadline, mirrors chi)
- drop middleware.RealIP: these routers do redirect/challenge only, with no
per-IP logic, so the spoofable header trust is simply removed here
- return http.Handler instead of chi.Router (callers already take http.Handler)
chi stays a dependency (still used by the main API router); this only removes
its use from ssl.go. go test -race, vet, golangci-lint and govulncheck clean.
Email notification templates rendered the comment HTML via text/template,
so the store-level UGC sanitizer's permitted <a> and <img> tags reached
the email body verbatim. An authenticated user could plant phishing links
and remote tracking pixels in notification emails sent from the legitimate
remark42 address.
Switch notify to html/template (auto-escaping every non-HTML field) and
add a stricter email-only bluemonday policy that drops <a> and <img> while
keeping basic text formatting; the sanitized comment HTML is passed as
template.HTML. Add regression tests asserting links and images are stripped
while anchor text and formatting survive.
Eleventy passes a falsy outputPath to transforms for templates rendered
without a written file (e.g. permalink: false); calling .endsWith on it
would throw. Skip minification in that case instead. Pre-existing latent
issue surfaced by Copilot review on #2091.
Captures what isn't obvious from the diff alone: the ten places a
node/pnpm version is pinned and must move together (including .nvmrc,
which CI never reads and is how the node-16 drift in this PR's first
push went unnoticed), the pnpm-10 layout pins, the msw 1->2 migration,
the deliberately held-back majors, and the abandoned html-minifier
replacement. Written so the next dependency bump doesn't repeat the
same gaps.
- frontend/.nvmrc was still pinned to 16, left behind by the node 16->20
bump everywhere else (Dockerfile, CI matrices). A contributor running
'nvm use' in frontend/ would land on node 16, which cannot even run
pnpm 10 (requires node >=18) -- CI never reads .nvmrc, so this was
invisible to every check.
- pnpm/action-setup 'version: 10' floated the patch release in CI,
inconsistent with the exact 10.10.0 pin now used in Dockerfile,
Dockerfile.e2e and packageManager. Pinned all ten occurrences across
ci-frontend.yml, ci-frontend-api.yml and release.yml to 10.10.0.
- Pin pnpm to the exact version (10.10.0) when installing it in the
production Dockerfile, matching packageManager and Dockerfile.e2e,
instead of a floating major that can drift the lockfile behaviour.
- Fix mockEndpoint's array header handling in the api test utility:
append each value instead of joining with a comma, which is how
multi-value headers (e.g. set-cookie) are actually represented.
- Update apps/remark42's engines to node >=18 / pnpm >=10, matching
the pnpm 10 requirement instead of the stale node 16 / pnpm 8 range.
- frontend/Dockerfile.e2e: bump base image to mcr.microsoft.com/playwright:
v1.61.1-noble to match the Playwright 1.61.1 npm bump (browser revision
mismatch was failing all e2e specs), and corepack pnpm@8 -> pnpm@10.10.0 to
match the pnpm bump and the v9 lockfile.
- release.yml validate: pnpm 10 forwards 'test -- --runInBand' literally as
'jest -- --runInBand' (treated as a path pattern, 0 tests). Drop the extra
separator: 'pnpm test --runInBand'.
yarn audit: 0 vulnerabilities (was 52 findings). Site builds via eleventy +
tailwind on node 20.
Direct bumps: markdown-it 14.2, cross-env 10, date-fns 4.4, prettier 3.9,
@tailwindcss/typography 0.5.20, @11ty/eleventy-plugin-syntaxhighlight 5.0.2.
Replaced abandoned html-minifier (unpatched ReDoS, no fix released) with the
maintained html-minifier-terser fork; the .eleventy.js htmlmin transform is now
async. Transitive vulns patched via yarn resolutions. js-yaml resolves to 3.15.0
(3.x backport) which keeps gray-matter working.
Held: tailwindcss 3.4 (tailwind 4 is a config rewrite) and @11ty/eleventy 2
(eleventy 3 is an ESM migration) - both invasive.
Build output verified against a clean master build: every HTML page differs only
by the build-time ?v= cache-bust query; style.css differs only by an equivalent
refactor of @tailwindcss/typography's prose kbd-shadow variables (same rendered
result). Functionally identical.
pnpm 8.15.9 -> 10.10.0 (packageManager + lockfile regenerated to v9). Frontend
CI (ci-frontend.yml, ci-frontend-api.yml, release.yml) and the production
Dockerfile bumped from node 16 + pnpm 8 to node 20 + pnpm 10 (pnpm 10 requires
node 18+). pnpm audit: no known vulnerabilities (was 63 alerts).
packages/api: bumped to latest including the major test stack - vitest 4, jsdom
29, @vitest/coverage-v8 4, @typescript-eslint 8.62, typescript 5.9, prettier
3.9, @types/node 26, and msw 1 -> 2. Migrated tests/test-utils.ts to the msw 2
http/HttpResponse API (capturing a compatible request shape) and made test base
URLs absolute so node 20's native fetch is intercepted; added the jsdom base
URL. type-check:api, lint:api and coverage:api (45 tests) all pass.
apps/remark42: safe in-major bumps (webpack 5.108, postcss, mini-css-extract,
html-webpack-plugin, ts-loader, webpack-dev-server 5.2.5, core-js, clsx 2,
lodash-es 4.18, dotenv 17, @types/*). Transitive vulns patched via
pnpm.overrides. type-check, lint, build, jest coverage (299 tests) and
translations all pass.
pnpm 10's stricter layout required a few pins to keep the app's preact-compat
setup compiling: preact 10.6.2 (override), react-intl 6.0.5 and
@testing-library/preact 3.2.2 (newer types break the build), tsconfig paths for
preact, @types/minimatch 5.1.2 (6.x is an empty stub) and cheerio 1.0.0-rc.12
(1.2 is ESM and breaks jest 28). Held: react/react-dom (preact compat alias),
babel 7, eslint 8, stylelint 14, jest 28, typescript 4.7 (app),
redux/react-redux - majors that change the bundle or need a config migration.
Build output verified against a clean master build: apps/remark42 output is
functionally identical (the only diffs are webpack module-id numbering and
css-module class tokens from the webpack/css-loader bump; all HTML, CSS values
and translations byte-identical).
Update all Go modules in backend/ and backend/_example/memory_store/ to
their latest versions (chroma 2.27, go-redis 9.21, bbolt 1.5, slack 0.27,
golang.org/x/* and others); re-tidy and re-vendor, keep the example module
in sync.
Hold github.com/go-chi/chi/v5 at v5.2.5: v5.3.0 deprecates
middleware.RealIP (IP-spoofing advisories). Switching off RealIP changes
how the client IP is derived for rate limiting and votes, which is a
security decision better made on its own rather than inside a dependency
bump.
go test -race, go vet, golangci-lint and govulncheck all clean on both
modules.
* site: fetch latest version client-side instead of embedding at build time
The header version badge was filled in by site/src/data/github.js calling
the GitHub releases API at Eleventy build time and baking data[0].tag_name
into every page. This had three failure modes:
1. Layered cache: Buildx caches the yarn build layer; on a release-triggered
workflow nothing under ./site changes, so the cached HTML (with the
previous tag baked in) gets shipped. v1.16.0 went out and remark42.com
kept showing v1.15.0 until a separate site/ commit landed and naturally
invalidated the COPY layer.
2. Tag mismatch: the deploy pulls ghcr.io/umputun/remark42-site:master,
but release events build :v1.16.0 and :latest only. A cache-skip
workflow tweak wouldn't even reach the served image.
3. API propagation race: the workflow fires ~2s after release publish,
so even with cache disabled the API might still return the previous
tag from a stale read replica.
All three vanish if the version is fetched in the browser. GitHub serves
the /releases/latest response with Cache-Control: public, max-age=60 so
per-visitor cost is bounded; failures fall through silently and the
badge stays empty rather than wrong.
Changes:
- header.njk: replace {{ github.latestVersion }} with a
<span data-remark42-version></span> placeholder.
- inline.js: add a fetch of /releases/latest that fills any
[data-remark42-version] element on the page. fallback is no-op on any
network/parse failure.
- delete site/src/data/github.js (Eleventy data file is no longer used).
- drop node-fetch from devDependencies (was used only by github.js).
* site: address PR review on version badge fetch
- gate DOM update on DOMContentLoaded — inline.js is loaded sync in <head>,
so a cache-hit fetch can resolve before the placeholder span is parsed.
- hide placeholder span by default (`hidden`) so a failed/blocked fetch
doesn't leave a 0.5rem stray gap before the github icon.
- log fetch failures (rate limit, offline, blocked) instead of silently
swallowing — matches the prior behaviour of build-time github.js.
* site: cache latest version in sessionStorage with 1h TTL
avoids hitting the GitHub API on every page load — repeated navigations
within a tab read from sessionStorage instead. TTL caps stale display at
1h for very long-lived tabs. cleared on tab close, so each new session
fetches once and reuses the result throughout.
* site: address PR review on header & version fetch
Copilot review on the cache commit raised three points:
1. inline.js had a hard-coded `https://api.github.com/repos/umputun/remark42`
while the templates use `site.githubUrl`. Rename inline.js → inline.njk
so nunjucks evaluates it, add `githubApiUrl` to site.json, and template
the fetch URL from it. One place to update if the repo ever moves.
2. header.njk aria-label said "Remark42's GitHub Repository" but the link
target is `/releases`. Change to "{{ site.name }} releases on GitHub"
so screen readers describe the actual destination.
3. console.warn on fetch failure (kept after umputun's prior review noted
the trade-off): addressed in the PR description, no code change.
* site: actually template fetch URL via site.githubApiUrl
Copilot's second pass caught that 7697dcf3 added site.githubApiUrl,
renamed inline.js → inline.njk, and pointed head.njk at the .njk file
— but the fetch() call itself was never changed to use the template
variable. Build output looked correct because the literal hard-coded
URL happened to match what {{ site.githubApiUrl }} would expand to.
* site: normalise Nunjucks spacing in header.njk
{{ site.githubUrl}}/releases → {{ site.githubUrl }}/releases. Cosmetic
only; matches the spacing used everywhere else in the templates.
* fix(frontend): no_footer scrollbar regression introduced in v1.16.0
Two unrelated changes in v1.16.0 combined to surface a scrollbar in
no_footer=true mode:
1. c26f45e5 removed the deprecated `scrolling="no"` iframe attribute
on the grounds that "overflow is already hidden via CSS". That CSS
(`overflow: hidden` in createIframe styles) is on the iframe ELEMENT
in the parent page; it has no effect on the iframe DOCUMENT's own
scrollbars. The spec-correct replacement is `overflow: hidden` on
the iframe document's body — added here to global.css.
2. The negative `margin-bottom: -24px` on `.thread:last-child` was a
trick to tighten the gap to the footer (combined with the footer's
`margin-top: 48px` it collapsed to a 24px net gap). With no_footer
the negative margin had no positive-margin sibling to collapse
against and instead propagated up through .root, leaving body
~24px shorter than the visual content. The iframe height calc
(`body.offsetHeight + 12`) then sized the iframe below the visible
bottom of the last thread → scrollbar.
Replace the negative-margin trick with a straight `margin-top: 24px`
on `.copyright`. Same 24px visual gap when the footer is shown, no
propagation when it isn't. The mix={styles.thread} on Thread becomes
a dead reference and is dropped.
Closes#2073
* fix(frontend): drop dead Thread.mix prop after root.tsx removed its only caller
Both Copilot and umputun flagged this in PR review: after the parent
commit on this branch dropped `mix={styles.thread}` from root.tsx, the
`mix?: string` prop and the corresponding entry in the clsx() call in
thread.tsx are dead code — no caller passes it (the recursive Thread
render in thread.tsx:82 never did either). Remove the prop, the
destructure, and the clsx entry.
* fix: address parameter docs and --help text inconsistencies
Audit findings from comparing site/src/docs/configuration/parameters/
against the backend flag tags.
Docs (parameters/index.md):
- image.bolt.file default was `/var/pictures.db` (absolute, looks like
a system path); actual default is `./var/pictures.db` (relative,
under the working dir).
- notify.webhook.template default was shown as
`{"text": {{.Text | escapeJSONString}}}` — both the function name
doesn't exist and the unescaped pipe inside the table cell broke the
Description column count for that row. Real default is the literal
`{"text": "{{.Text}}"}`.
- "Custom OAuth2 integration currently supports only one custom
provider at a time" was a free-standing paragraph wedged between two
table rows. kramdown terminated the table on that paragraph and
restarted a new headerless table for the rest of the rows. Moved it
to its own subsection after the table so the table stays contiguous.
Backend --help text (server.go):
- allowed-hosts description ended with a stray double apostrophe in
`CSP 'frame-ancestors''` (typo).
- Deprecated auth.email.{port,passwd,user,tls} flag descriptions were
shuffled — port said "SMTP password", passwd said "SMTP port", user
said "enable TLS", tls said "SMTP TCP connection timeout". Fixed
each to match the flag it's actually describing. Docs already had
the correct descriptions for these deprecated flags.
* fix: webhook template flag default override masking safe fallback
Copilot flagged the audit's "real default" claim and was right. server.go:286
had default:"{\"text\": \"{{.Text}}\"}" — the literal, JSON-unsafe template
that produces invalid JSON if a comment contains a quote or newline. The
notify package (webhook.go:50) has a safer fallback:
if params.Template == "" {
params.Template = webhookDefaultTemplate
}
where webhookDefaultTemplate is {"text": {{.Text | escapeJSONString}}}. But
go-flags applies its default tag at parse time, so the field is never empty
when the user omits --notify.webhook.template, and the safer fallback never
runs.
Drop the unsafe default tag so the webhook package's escapeJSONString-based
default takes effect. Also:
- fix the --help description (was "webhook authentication template", but
it's a payload template, not an auth one; same for headers).
- update parameters/index.md to document the actual safe default
({{.Text | escapeJSONString}}); escape the cell's | as \| so kramdown
doesn't treat it as a column separator.
- typo: "bellow" -> "below" in the headers env-delim comment.
opening backtick had no closer in the Default column, so kramdown saw a
broken cell and stopped rendering the parameters table — every row from
smtp.login_auth onward (~40 rows) rendered as raw pipe-delimited text
instead of HTML table cells. closes#2074
replace the Docker artifact build with GoReleaser config and a tag release workflow. Keep local artifact builds snapshot-only and clean generated frontend embed files after release runs.
* 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.
readAndValidateImage caps the byte size of incoming images but the resize()
helper that follows still called image.Decode unconditionally, allocating
pixel memory proportional to the *declared* image dimensions. A ~100 KB
compressed PNG or GIF that declares 65535x65535 px forces image.Decode to
allocate ~17 GB of raster, OOMing the service on a single comment upload
(or on the proxy's CacheExternal path when caching a malicious upstream).
Hardening:
- maxImagePixels = 16 MP constant. Covers any realistic image (~4096x4096)
while bounding peak allocation.
- resize() now runs image.DecodeConfig first (cheap, no pixel allocation)
to read declared width/height before any full decode.
- Multiplication of width × height uses int64 to defeat 32-bit overflow
(GOARCH=386, 32-bit arm): on those targets, int(cfg.Width)*int(cfg.Height)
could wrap below maxImagePixels and bypass the cap. GIF's 16-bit logical
screen and JPEG's 16-bit SOF dimensions both reach this if int-multiplied.
- Bytes exceeding the cap, or non-image input that fails DecodeConfig,
return nil. prepareImage propagates the rejection as a clear error
instead of storing the malformed/oversized data verbatim.
- The no-resize-needed path returns the validated original bytes verbatim
so animated GIFs round-trip without being flattened to a single frame.
The DecodeConfig precheck applies even when MaxWidth/MaxHeight are 0
(resize disabled) — the dimension cap is unconditional defense-in-depth.
Two adjacent fixes surfaced by the new resize contract:
1. readAndValidateImage previously did `data[:512]` without a bounds check,
panicking on any body shorter than 512 bytes. Now bounded with min().
2. image/webp was listed as an allowed format but no WebP decoder was
registered, so DecodeConfig would refuse legitimate WebP uploads. Added
`_ "golang.org/x/image/webp"` (already in go.mod via x/image/draw) so
the registered decoders match the allowlist.
Tests:
- TestService_resizeRejectsDecompressionBomb builds a 14-byte GIF87a header
declaring 65535x65535 and asserts resize() refuses it both at the unit
level and through SaveWithID end-to-end (no store write).
- TestService_SaveWithIDShortPayload regression-tests the short-body panic.
- TestService_SaveWithIDWebP regression-tests WebP round-trip through
prepareImage with the new DecodeConfig requirement.
- TestService_resize subtests updated to assert non-image bytes are now
refused (previously the helper fell back to returning the raw bytes
verbatim, letting malformed content reach the store).
Mention the "Report a vulnerability" button (GitHub private vulnerability
reporting) alongside the existing email contact, now that private reporting
is enabled on the repository.
Dmitry VerkhoturovandUmputun
Fredrik AppelrosandUmputun
Eugene OrlovandUmputun
dependabot[bot]andUmputun
Paul MineevandUmputun