* Serve the build-independent web assets from the backend
`privacy.html`, `markdown-help.html` and the `400x400.jpeg` it embeds carry
no template variable, link no script or stylesheet, and are imported by
nothing in the widget. They now live in `backend/app/webassets/assets`,
embedded there, and are served under `/web` alongside the frontend build.
`/web` reads the frontend build first and falls back to them, which is what
lets an operator replace one by dropping a file into `--web-root`. That is
what `privacy.html` needs: it describes remark42.com, while the
authorization guide tells operators to hand its URL to Google and Facebook
as their own application's privacy policy.
Only a missing file falls through. An unreadable file in the web root keeps
reporting as unreadable rather than being silently replaced by the embedded
copy, and a name the filesystem rejects reports as missing rather than as a
server error, both matching what `http.Dir` did.
The dev server serves the same directory, so the Markdown help link in the
comment form resolves on the dev port as well as in production.
The two pages are served as they are written. `markdown-help.html` was
minified before, and its formatted inline stylesheet is most of its 8.5 kB;
that is 2.4 kB more over the wire, behind the hour-long cache header the
file server already sets.
Drops `copy-webpack-plugin`, which had no other pattern, and the stylelint
entries that only ever matched these files.
* Make pnpm dev:app start again
The dev server has been failing to start on two counts, so the flow the
contributing guide documents does not run at all.
`webpack-cli` 4 drives `webpack-dev-server` 5 through the argument order
of an older major, handing it the compiler where it expects the options
object. It rejects that against its schema and exits, complaining about an
unknown `_assetEmittingPreviousFiles` property, which is a field of the
compiler. `webpack-cli` 7 is the release that declares
`webpack-dev-server` 5 as a peer.
Past that, `http-proxy-middleware` resolves to 4.1.1, which no longer
accepts the two-argument call `webpack-dev-server` makes, so the `/api`
and `/auth` proxies throw on startup. It is pulled in by the security
override for CVE-2025-32996, the only override in the file with no upper
bound: `>=2.0.10` matches every later major. Bounding it to the 2.x line
keeps the fix and the API `webpack-dev-server` calls.
With both in place `pnpm dev:app` serves the widget and the pages under
`/web` on port 9000.
rest.CORS refuses "*" together with credentials since go-pkgz/rest#52, so the
bump and the option have to land together: the option does not exist in v1.22.0
and the panic fires at construction, inside routes(), which makes it a startup
failure rather than a request-time one.
The wildcard stays. The comment widget is embedded on arbitrary third-party
sites, so the set of origins is not knowable, which is why the escape hatch was
asked for upstream instead of accepting the panic. What it costs is unchanged
and now written next to the call: any site a signed-in user visits can read
authenticated responses, so state-changing requests have to keep being protected
by something other than the origin, X-XSRF-Token today.
The example module is tidied in the same commit, as it reaches go-pkgz/rest
through the replace directive and its indirect graph would otherwise keep the
old pin and fail the readonly module check in CI.
The bump also carries testify to v1.12.0, which drops go-spew and go-difflib
from the module graph.
exportCtrl mapped every export failure to 500 Internal Server Error, so
requesting a backup for a non-existent site (e.g. wrong -s/--site) came
back as a misleading 500 instead of a client error — inconsistent with
the rest of the admin/public API, which returns 400 + ErrSiteNotFound
for site-lookup failures.
Add an engine.ErrSiteNotFound sentinel (wrapped at the bolt db-lookup so
the existing "site %q not found" message is unchanged) and map it to 400
+ rest.ErrSiteNotFound in exportCtrl; genuine internal failures (gzip
close/write) still return 500.
The test decided the expected HTTP status with strings.Contains(tc.params,
"=bad"), but comment IDs are random UUIDs. When one started with "bad"
(e.g. offset_id=bad49e60-...), the param string contained "=bad" and the
case was wrongly expected to return 400 while the handler correctly
returned 200, failing the run about 0.2% of the time.
Identify bad-request cases by their error response body instead, which
is deterministic per case and independent of the generated IDs.
The limit() boundary used >=, so a subtree that fit the page exactly was
treated as overflow and dropped — under-filling the final page (e.g.
limit=5 over subtrees 3,2 returned only the first subtree, 3 comments,
instead of both). Change it to > so an exact-fit subtree is included;
the first node is still always returned in full and larger subtrees
still overflow to the next page.
Adds table tests for MakeTree's limit/offset pagination and countReplies,
and updates the /find consistent-count expectations for the corrected
boundary.
notify/email.go accumulated multi-recipient errors with
multierror.Append(fmt.Errorf(...)) instead of
multierror.Append(result, ...), so the accumulator was overwritten each
iteration and only the last failing recipient's error survived; earlier
failures were silently dropped. The telegram notifier did it correctly.
Replace hashicorp/go-multierror with the stdlib errors.Join everywhere
it was used (notify/email.go, notify/telegram.go, rest/api/rest_private.go,
store/service/service.go, store/image/image.go and store/engine/bolt.go),
which fixes the bug and drops the direct dependency. It stays indirect
because go-pkgz/lcw/v2 still imports it. A regression test in
email_test.go now sends two failing recipients and asserts both errors
are reported.
Follow-up to the review notes on #2116:
- warn at startup when --trusted-proxy contains a catch-all (0.0.0.0/0 or ::/0),
which trusts every peer and re-opens the bypass - mirrors the unset-case warning
- realIPMiddleware tests: cover the unparseable-peer and trusted-peer-without-header
branches, and make the observed values per-call so subtests don't share closure locals
- trim the flag description and shorten the startup warning to the terse [WARN] style
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
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.
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.
* 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).
Address all golangci-lint v2.10.1 (CI's version) findings:
* Add http.MaxBytesReader hard cap to ParseMultipartForm sites in
rest_private.savePictureCtrl (32MB) and api/migrator (256MB) — fixes
G120 by bounding total request body before form parsing.
* Suppress G70x in CLI subcommands cmd/{backup,cleanup,import,remap}.go:
all four issue HTTP requests against operator-supplied RemarkURL/CLI
flags, never user input. Each suppression carries a one-line reason.
* Suppress G122 in image fs_store cleanup walk: staging directory tree
is server-only, no untrusted symlinks land there.
CI's golangci-lint v2.10.1 (newer rule set than my local 2.11.4) flags
four G70x cases the previous run missed. All are false positives:
backup.go and cleanup.go drive HTTP requests against the operator's own
RemarkURL from CLI flags (not user input); migrator.go removes a temp
file whose name was returned by os.CreateTemp (server-controlled). Add
targeted //nolint:gosec comments naming the reason at each site.
Commit aca0cff3 silenced the path-traversal, SSRF and XSS taint rules
project-wide as "false positives" while fixing image-proxy SSRF. With
the path-traversal and TitleExtractor SSRF gaps now closed, restore the
rules so future regressions get flagged. The four genuine false positives
that remain (image proxy http.NewRequest, QR png Write, two RSS XML
Writes) get individual //nolint:gosec comments naming the reason.
Address PR #2045 review (umputun):
* The //nolint:gosec on telegramQrCtrl's w.Write(png) was byte-identical
to the same line in #2044 (gosec-rule restoration). Drop it here so
the two PRs do not conflict; #2044 owns it.
* `seg == ".."` in safePictureSegment was already covered by the
strings.Contains(seg, "..") check two lines down — trim and add an
inline comment so the cover-by-superset is explicit.
Address PR #2045 review feedback (Copilot #2045-1). The previous
safePictureSegment allowed CR/LF/TAB through, so a request such as
GET /api/v1/picture/dev%0Auser/abc.png would inject literal newlines
into the access log line ("GET - /api/v1/picture/dev\nuser/abc.png ...")
— a log-forgery primitive against any operator parsing those logs.
Reject any unicode.IsControl rune in either segment (NUL was already
caught via strings.ContainsAny). New TestRest_LoadPictureRejectsControlCharsInSegment
covers LF, CR, TAB, NUL across both segments.
The unauthenticated GET /api/v1/picture/{user}/{id} handler concatenated the
two URL params verbatim into a filesystem path via path.Join, so a request
like /api/v1/picture/../remark.db resolved to <base>/../remark.db, escaping
the image directory. With Partitions=0 (a documented option) this is a
direct arbitrary-file read; with the default Partitions=100 the constructed
path lands in a CRC-derived subdirectory but the server still leaks the
internal filesystem path back to the unauthenticated caller via the JSON
error body — confirmed against demo.remark42.com (master-80c12a3) which
returned `stat /var/folders/.../staging/.../remark.db` for `..` requests.
Validate both URL segments via safePictureSegment (no traversal markers,
no path separators, no NULs) at the handler entry, and replace the raw
storage error with a generic "image not found" response. The original
error is logged for operators.
Reproduction test asserts that ../remark.db, foo/..%2Fremark.db and
%2E%2E/remark.db all return 400 with no internal path leaked.
matchSiteID guarded most authenticated and admin routes with
`if siteID != "" && user.SiteID != siteID`. Dropping the ?site= query
parameter made the check no-op and any authenticated user passed the
middleware. Downstream handlers fell back to reading site from the JSON
body or just used the empty string, so on email/telegram subscribe
endpoints (which read site from body) a user authenticated to siteA
could perform actions targeting siteB without the cross-site guard
ever firing.
Require ?site= to be present and to match user.SiteID. Body-only site
flows are still supported provided the URL also carries the matching
?site= — both must agree, which removes the bypass and keeps the
declared site visible to the middleware.
Reproduction TestRest_matchSiteID enumerates four cases (matching,
mismatched, missing, empty). Existing test calls that relied on the
implicit pass had to add ?site=remark42 to the URL: the addComment
helper now derives the param from c.Locator.SiteID, picture upload
URL gets the param explicitly, and the email/telegram subscribe table
adds it to every endpoint. The negative cases that previously asserted
StatusBadRequest from the handler now correctly assert StatusForbidden
from the middleware.
Apply go fix ./... analysers (Go 1.26) across backend and examples:
- interface{} → any (type alias, no behaviour change)
- for i := 0; i < N; i++ → for range N / for i := range N
- slices.Contains / slices.ContainsFunc replacing manual loops
- strings.SplitSeq replacing strings.Split in range (avoids allocation)
- strings.CutPrefix replacing HasPrefix+TrimPrefix
- min() replacing manual if/else
- fmt.Appendf replacing []byte(fmt.Sprintf(...))
- strings.Builder replacing string += concatenation
- wg.Go(func(){}) replacing wg.Add(1)/go/wg.Done() pattern
- removed redundant ii := i loop variable copies (unnecessary since Go 1.22)
omitempty on struct-typed JSON fields: go fix removed omitempty from
struct-typed fields (time.Time, PostInfo, UserDetailEntry) because
encoding/json's omitempty never applied to struct types — it was always
a no-op. Kept as bare tags (no omitzero replacement) to preserve the
existing serialisation behaviour.
Replace strings.Split(RemoteAddr, ":") with net.SplitHostPort for correct
IPv6 address extraction in vote deduplication and comment IP tracking.
Harden image proxy: add SSRF-safe transport blocking private/reserved IPs
at connection time with DNS rebinding protection, sanitize error messages
to prevent information leakage, add response size limit via io.LimitReader.
Fix shadowed error variables in BlockedUsers, SetTitle, and Delete methods.
Exclude gosec taint analysis false positives at linter config level.
Add two missing security headers to the existing securityHeadersMiddleware:
- X-Content-Type-Options: nosniff — prevents browsers from MIME-sniffing
responses away from the declared Content-Type, stopping e.g. a
user-uploaded image from being reinterpreted as executable HTML/JS
- Referrer-Policy: strict-origin-when-cross-origin — limits URL information
leaked in the Referer header on cross-origin requests to just the origin
(no path), and sends nothing at all on HTTPS-to-HTTP downgrades
Replace WriteHeader() + RenderJSON() pattern with EncodeJSON() which
properly sets Content-Type header before writing status code. The
previous pattern caused Content-Type to default to text/plain instead
of application/json, breaking frontend JSON parsing.
Fixes#1979
Replace go-chi/render with go-pkgz/rest for JSON responses and custom
helpers for HTML/plain text responses.
Key changes:
- Replace render.JSON/render.Status with rest.RenderJSON and explicit
w.WriteHeader() calls
- Replace render.DecodeJSON with json.NewDecoder().Decode()
- Add SendErrorJSON helper that sets Content-Type header before
WriteHeader (required since rest.RenderJSON can't set headers after
WriteHeader is called)
- Add HTMLResponse and PlainTextResponse helpers
Fix export double-execution in migrator.go:
The original code called Export twice - once to io.Discard to check for
errors, then again to actually write. This was wasteful and had a race
condition risk. Now file mode buffers to memory first for atomic
success/failure, while stream mode writes directly with proper error
handling.
`format=tree` pagination provides top-level comments with all replies
and returns the last top-level comment as `last_comment` to be used
as `offset` for the next page. If comments and replies overflow
the limit, the one stepping out of the limit will not be returned.
If the first comment and its replies after the given offset overflow
the limit, it will be returned with all the replies.
`format=plain` pagination works by providing all comments and returning
the last comment as `last_comment` to be used as `offset`
for the next page.
Change the default img-src value to "*" and sets it to "'self'" when
image proxy is enabled. The previous state was inversion of this logic
which was wrong.
`Content-Security-Policy` now restricts resource loading and execution
to enhance security:
- `default-src 'none'`: Disallow all resource loading by default.
- `base-uri 'none'`: Prevents the use of `<base>` tag to change the
base URL for relative URLs.
- `form-action 'none'`: Disallows form submissions.
- `connect-src 'self'`: Restricts the origins that can be connected to
(via XHR, WebSockets, etc.) to the same origin.
- `frame-src 'self'`: Restricts the origins that can be embedded using
`<frame>` and `<iframe>` to the same origin (for `/web/` demo
endpoint).
- `frame-ancestors %s;`: Specifies the origins that are allowed to
embed this content in a frame. If no specific origins are allowed, it
defaults to `*` (any origin). This enhances security by controlling
which sites can embed your content.
- `img-src 'self'`: Allows images to be loaded only from the same
origin. If `imageProxyEnabled` is true, allows images from any origin
(`*`).
- `script-src 'self' 'unsafe-inline'`: Allows scripts to be loaded and
executed only from the same origin and allows inline scripts.
- `style-src 'self' 'unsafe-inline'`: Allows styles to be loaded and
applied only from the same origin and allows inline styles.
- `font-src data:`: Allows fonts to be loaded from data URIs.
- `object-src 'none'`: Disallows the use of `<object>`, `<embed>`, and
`<applet>` tags.
`Permissions-Policy` now restricts the use of certain browser features
which we don't use to enhance user privacy and security:
- `accelerometer=()`: Disables the use of the accelerometer sensor.
- `autoplay=()`: Disables automatic playback of media.
- `camera=()`: Disables the use of the camera.
- `cross-origin-isolated=()`: Disallows the page from being treated as
cross-origin isolated.
- `display-capture=()`: Disables the ability to capture the display.
- `encrypted-media=()`: Disables the use of Encrypted Media Extensions
.
- `fullscreen=()`: Disables the ability to use fullscreen mode.
- `geolocation=()`: Disables the use of geolocation.
- `gyroscope=()`: Disables the use of the gyroscope sensor.
- `keyboard-map=()`: Disables the use of the keyboard map.
- `magnetometer=()`: Disables the use of the magnetometer sensor.
- `microphone=()`: Disables the use of the microphone.
- `midi=()`: Disables the use of the MIDI API.
- `payment=()`: Disables the Payment Request API.
- `picture-in-picture=()`: Disables the use of Picture-in-Picture mode
.
- `publickey-credentials-get=()`: Disables the use of the Web
Authentication API.
- `screen-wake-lock=()`: Disables the ability to prevent the screen
from dimming.
- `sync-xhr=()`: Disables synchronous XMLHttpRequest.
- `usb=()`: Disables the use of the USB API.
- `xr-spatial-tracking=()`: Disables the use of spatial tracking in
WebXR.
- `clipboard-read=()`: Disables the ability to read from the clipboard
.
- `clipboard-write=()`: Disables the ability to write to the clipboard
.
- `gamepad=()`: Disables the use of the Gamepad API.
- `hid=()`: Disables the use of the Human Interface Device API.
- `idle-detection=()`: Disables the ability to detect idle state.
- `interest-cohort=()`: Disables the use of interest cohort tracking.
- `serial=()`: Disables the use of the Serial API.
- `unload=()`: Disables the ability to use the `beforeunload` and
`unload` events.
- `window-management=()`: Disables the ability to use window
management APIs.
