* Assert what the image endpoints promise rather than the compressor's output
Three tests pinned the exact bytes or the exact length of an encoded
image, so they fail on any toolchain whose deflate or png encoder emits
something different. CI pins go 1.25 and passes; go 1.27 fails all three,
while the images themselves are perfectly valid.
TestRest_QR now decodes both the golden file and the response and
compares the pixels, which is the same assertion about the qr code and
none about the encoder. The two resize cases assert the decoded image
fits the box resize was given and touches one of its sides, which is what
fitting to a box means and what the function actually promises.
Resolves#2200.
* Fill the instance URL into the embedded frontend at serve time
The widget falls back to a compiled-in URL whenever a page omits
`remark_config.host`. The bundler cannot know that URL, so it emits
`{% REMARK_URL %}` and each distribution substitutes it: the docker image
rewrites the files under its web root at container start, and the release
binary, which serves the build embedded in itself, had nothing doing it.
`prepare-release-assets.sh` filled the marker with `http://127.0.0.1:8080`
before the embed instead, so every copy of the binary shipped pointing at
the visitor's own loopback address, and on an https site the request is
blocked as mixed content besides.
It has been that way since v1.11.0, the first release to embed the
frontend, and the earlier binaries embedded none, so the tarball has never
served a correctly addressed widget.
The placeholder now survives into the embedded copy and the file server
fills it with the configured `REMARK_URL` as it serves, which is what the
docker image already does to its own copy. The image no longer bakes the
loopback address into its embedded copy either, so the fallback it keeps
for a missing web root is correct rather than misleading.
Substituted in html, js and mjs, the same set `docker-init.sh` rewrites,
and the served size is the substituted one so a response is neither
truncated nor left hanging.
Nothing exercised the marker the frontend build emits wherever the instance
url belongs. Every page in the suite sets `remark_config.host` from its own
origin, so the compiled-in fallback is never read, and a distribution that
stopped substituting would keep the suite green.
Two tests. The first reads the served bundles and pages back and asserts the
marker is gone from each and that what replaced it is this instance. The
second covers what the substitution is for: the widget document carries no
host of its own, since `iframe.html` builds its config from a query string the
parent never puts one in, so everything it requests is addressed with the
compiled-in url. It asserts the widget renders and that the config request
went to this instance.
The demo pages cannot show the second. Their loader builds the bundle's own
script url from `remark_config.host`, so a page without one never gets as far
as loading the widget.
Verified by disabling both substitution paths, the serve-time one and the
docker image's, and rebuilding: both tests fail. Editing the files on disk is
not enough, since the file server substitutes as it serves.
The served body now depends on remarkURL, but cacheControl builds its
etag from version and path only. An operator who notices the widget is
addressed to the wrong host, corrects REMARK_URL and restarts the same
binary gets 304 on revalidation, so the client keeps a bundle pointing
at the old host. Cache-Control is no-cache, so it revalidates every time
and never ages out of that state either.
That is the exact situation this substitution exists to fix, so the
validator has to carry the url.
* 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).
Go 1.25's testing/synctest package (GA) provides a fake clock bubble
for deterministic goroutine and timer testing. Convert tests that
waited on real-time durations to use synctest, removing most wall-clock
time.Sleep workarounds.
Converted (11 tests, 9 files):
- notify/notify_test.go — all tests, replaced 17 time.Sleep(110ms) with synctest.Wait()
- store/service/service_test.go — VoteSameIPWithDuration, UserReplies, submitImages,
ResubmitStagingImages, deleteImagesOnCommentDelete
- store/image/{image,bolt_store}_test.go — Cleanup, Submit, SubmitDelay
- store/engine/bolt_test.go — FlagListBlocked
- providers/telegram_test.go — DispatchTelegramUpdates
- migrator/backup_test.go — TestBackup_Do
- _example/memory_store/accessor/data_test.go — FlagListBlocked
Simplifications along the way:
- notify/notify_mock.go: dropped the 10ms time.After delay and
ctx.Done select in MockDest — the artificial I/O simulation is
pointless and blocked synctest.Wait from draining the queue
- Removed three dead-code time.Sleep(1s) calls in EditCommentDurationFailed,
EditCommentAdmin, and Info tests: prepopulated comments from 2017
already exceed any EditDuration/ReadOnlyAge under real clock, making
the sleeps meaningless
- UserReplies: replaced the Eventually+Sleep+mutex polling with a
direct time.Sleep under fake clock
Skipped (incompatible with synctest):
- fs_store_test.go: relies on OS file mtime (real wall clock)
- rss_test.go: needs real wall-clock second boundary for pubDate
- admin/rest_private/rest_public tests: httptest network I/O
- cmd/server_test.go: real HTTP server startup polling
Notes on quirks encountered:
- synctest.Wait() does NOT advance fake time, contrary to what one
might expect. It only returns once all other bubble goroutines are
durably blocked. To advance the fake clock, the test goroutine must
itself call time.Sleep
- BoltDB keys the "last" bucket by comment.Timestamp nanosecond string.
Rapid b.Create calls under frozen fake time produce identical keys
and overwrite each other. TestService_UserReplies adds
time.Sleep(time.Nanosecond) between Creates to advance the clock
- Bolt image Cleanup uses strict age > ttl. Under fake time the
age-ttl delta is exactly zero at the boundary, so subtract 1ms from
the passed ttl to stay strictly under
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: umputun <535880+umputun@users.noreply.github.com>
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.
Previously, proxied and local images were checked for presence in the
storage before previewing or posting the comment. That logic resulted in
an inability to post with an image when a proxy for images is enabled,
as proxied images are not downloaded to disk before the first time
someone loads them, which could only happen after the user either
previews or posts the message.
After this change, preview and post only checks the local images'
presence and ignore the proxied ones.
Also:
- make commitTTL equal to EditDuration,
so that image is committed to permanent
storage after comment can no longer be edited
- move cleanupTTL to Cleanup function,
as it's not used elsewhere in the code
- add variables to some tests sleeps, so that
instead of being magic numbers they would
rely on timers of structures they suppose
to wait for
That function returns an error in a never
expected condition, and that error would be
logged message on the caller side:
none of the callers handles it.
That change hides that error from the caller
so that function would have a signature that
better fit what it does and how it behaves.
This allows having separate values of TTL for Commit and Cleanup
and moving them apart in time, also clarifying their connection
to EditTime which was previously outside of the package level.
@paskal
* sort imports, add missing copyright
* regenerate engine mock
* make all image.Store interface functions public
* go mod tidy
* make image.Store.Load return []byte instead of io.ReadCloser
* separate memory_store example RPC server to multiple files by handlers groups
* fix potential race on close
* move validate inside
* demote commit and cleanup in image.Store to non-exposed functions.
* replace immediate image commit with delayed via Submit
* minor: remove error logging, rename tests
* minor: err wrapping, comments wording
* clarify FileSystem.Save code
* attempt to fix#584 by making submitted image commits on a half of TTL
Co-authored-by: Dmitry Verkhoturov <paskal.07@gmail.com>
* increase timeout for TestServerAuthHooks http client
* replace assert.Equal checks for slice length with require.Equal
* unify channel name across tests
* fix panic in Test_Main
* increase TestRest_CreateWithPictures timeout for HDD slowness
* increase TestService_VoteSameIPWithDuration timeout for HDD slowness
* increase go test timeout for HDD run
* increase TestRest_CreateWithPictures timeout for HDD slowness
* resize uploaded images #309
* fix temp files created in current dire after image test
* update mod
* missing image test files
* fix failed goroutine test on image-resize submit
* switch to mockery
* missing in mock change
* resize by both ResizeWidth and ResizeHeight
* add IMAGE_RESIZE params to readme
* add latest features to the description
* lint: missing file close in image test
Dmitry VerkhoturovandGitHub
vdimirandUmputun
Anton PanferovandUmputun