remark42

mirrors/remark42

Fork 0

Commit Graph

Author SHA1 Message Date

Author	SHA1	Message	Date
Dmitry VerkhoturovandGitHub	4793c1cd2c	Fill the instance URL into the embedded frontend at serve time, and stop pinning compressor output in tests (#2198 ) * 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.	2026-08-22 13:15:27 -05:00
Dmitry VerkhoturovandGitHub	23be25d84a	Fix seven widget defects, including the cookies the separate-domain setup needs (#2197 ) * Drop the frontend workspace root and re-resolve the lockfile `frontend/` carried a `package.json`, a `pnpm-workspace.yaml` and the lockfile for a workspace of exactly one package. Two manifests meant two places to declare a version, and the app pin was the one that did not win: `preact` and `@babel/core` were each written twice, and a bump to the app manifest alone would have been a silent no-op, since `pnpm.overrides` decides and it lived at the root. Everything pnpm reads now lives in `frontend/apps/remark42`: dependencies, `packageManager`, `engines` and the overrides. `frontend/` keeps `.nvmrc`, `.husky` and `CLAUDE.md`, none of which pnpm reads. The directory nesting stays: every path in the repository points at `frontend/apps/remark42`, including the published contributing docs, so moving the package up would have rewritten 14 files to no benefit. Moving the manifest kept the old resolutions verbatim, which left optional peer subtrees the tree no longer reaches: `ts-node` under jest, `@swc/core` under webpack, `vitest` under `@testing-library/jest-dom`, `tslib` under `webpack-dev-server`. None is referenced by any config or source file here. Re-resolving drops 137 packages and moves 59 to versions already permitted by the ranges in the manifest, 1446 to 1308, with no direct dependency changing version: the five that look changed differ only in their peer suffix. Every file `pnpm build` produces is identical in size before and after. The frontend-deps stage of the Dockerfile sets `CI=true` so the `prepare` script skips husky, which has no git repository to install hooks into there. * Stop markdown-only changes triggering heavy workflows, and check the documented versions `ci-backend.yml`, `ci-build.yml` and `ci-frontend.yml` all end their path filters with `!.md`. The e2e workflow did not, so a change to any markdown file under `frontend/` or `backend/` matched its `frontend/` and `backend/*` entries and started a docker build and the whole browser suite. The release filter had the same hole and two of its own: it names `README.md` and `LICENSE` on purpose, since `.goreleaser.yml` packages both, so it now excludes markdown under `backend/` and `frontend/` only. `CLAUDE.md` and the installation page were listed as well, and neither is packaged. `ci-site.yml` goes on matching markdown, which is right, since the site is built from it. It excludes `CLAUDE.md`, so a future `site/CLAUDE.md` cannot start a site build, and `site/README.md`, which documents how to build the site rather than being part of it. The installation page tells a reader that a source build needs Go 1.25, Node 24+ and PNPM 10. Nothing kept those in step with `backend/go.mod`, `engines.node`, `packageManager` and `.nvmrc`, and the drift is silent: a wrong version in the docs builds and tests exactly as well as a right one. `.nvmrc` is the pin with form here, having sat at 16 through the whole node 20 migration because nothing red ever pointed at it. The check compares each stated version against its source and holds `.nvmrc` to `engines.node`, and it fails when the page states no version at all, so removing the claims cannot turn it into a check that passes by comparing nothing. Its own workflow rather than a step in an existing one, since the inputs span the backend module, the frontend manifest and the site. Fix the cookie fallback page, asset path, message senders, auth teardown and cookies Two defects with the same origin: `5825a55b`, the January 2021 frontend rewrite, first released in v1.7.0. It removed the build entry for comments.html while leaving both the template and the link to it in place, so the page the auth panel offers when third-party cookies are blocked has been a 404 ever since, for exactly the reader who has no other way in. The template needed no changes; it is built again, and an e2e case now opens it on a thread carrying a comment and waits for that comment, so the page being served, its inline script running and it asking for the thread named in its own query string are all covered. Against an image built without the plugin entry that case fails on the 404, which is the regression it exists for. It also fixed the public path to the domain root, so an instance mounted under a prefix, which manuals/separate-domain documents, asked for /web/google.svg when its own icons live under that prefix. Fifteen provider icons in remark.mjs and one in last-comments.mjs. The path is now derived from the url the bundle was loaded from, which is correct for both arrangements, and the file loader no longer overrides it. The host page also accepted postMessage from any window: every frame on a page can reach window.parent, and the handler resizes the widget, scrolls the page and opens the profile overlay. It now ignores anything that did not come from a frame this module created. A fourth, in the same family: the OAuth flow never tore its polling down. `subscribed` was declared, checked and cleared but never set, so the guard against a second subscription was dead code and every provider click attached another listener pair. The five minute deadline then rejected without unsubscribing, leaving those listeners and a retry that reschedules itself for as long as getUser returns null. Cross-domain is where getUser never stops returning null, so a reader on the arrangement manuals/separate-domain documents was left polling /auth/user once a minute for the life of the page, against a route capped at 2 req/s. It also rejected with no argument, and the caller stores that as the error state, so the interface had undefined to render. The deadline now tears the subscription down and rejects with an error. The message check had a second half. Hardening the parent left the widget document trusting any sender, and it acts on signout and theme, so anything holding a reference to the frame could sign a reader out. `auth.hooks` already checked `event.source !== window.parent`; that check is now a shared `isFromParent` and the three listeners that lacked it use it too. The origin cannot stand in for it, since the host page is whatever site embeds the widget and `ALLOWED_HOSTS` is enforced server side through `frame-ancestors`. And createInstance stacked its listeners. It reuses the marked iframe instead of building one, but installed three listeners plus a title observer on every call, while destroy could only reach the newest closure, so a second call without a destroy stranded a set for good. The listeners of the current instance are now detached before the next set goes on. Reuse and the ignored config are unchanged: that contract is open in the backlog note and not settled here. The auth cookies the embedded case needs were not being delivered, in both halves of the client's own writer. The name was decorated: setAuthCookie prefixed with __Host- whenever the page was https, so a real deployment wrote __Host-JWT and __Host-XSRF-TOKEN while the backend looks for JWT and the fetcher reads XSRF-TOKEN, and nothing anywhere reads a prefixed name. Nothing caught it because the prefix is applied from the page protocol and every test and the dev server run on http; there is now a second suite pinned to an https page, which is the only condition that shows it. And the attributes could not be delivered: both were SameSite=Strict, judged against the top-level site and not the request's own origin, so a Strict cookie is never sent from a third-party frame, which is the entire configuration this code exists for. They now follow the embedding, Strict while the widget shares its page origin and None with Secure and Partitioned once it does not, since that is the only third-party form browsers still accept. Over http in a third-party frame no combination works, and the strict form is written instead of one the browser would reject outright. That leaves the client half of #1877 working, whose reporter wanted AUTH_SEND_JWT_HEADER for exactly this arrangement, and whose first half merged as #1929. The server's own cookies still carry no Partitioned; that is upstream work in go-pkgz/auth. Two plan changes. A review pass corrected its central Path B premise, which said the first document render is anonymous permanently, in every configuration: it is anonymous in the configuration remark42 ships, go-pkgz/auth exposing XSRFIgnoreMethods and remark42 leaving it unset. The door is not shut, it is closed by a setting, and opening it is scoped security work and not a flag flip, because GET /deleteme deletes every comment a user has written and is a GET so the emailed link works. And the separate-domain arrangement is promoted from a constraint bullet to a named requirement with acceptance criteria, since a test that signs in and posts without reloading passes while persistence is entirely broken. Review found a seventh, and it was reachable only because of the first: comments.ejs built its title with innerHTML from the url query parameter, so restoring the build entry made a reflected XSS live on the instance origin, where the page is a top-level document, frame-ancestors does not apply and the /web CSP allows unsafe-inline. The anchor is now built through the DOM with textContent, and only http and https reach href, since escaping alone leaves a javascript: url working. Two e2e subtests pin both halves, and mutation testing separates them: restoring innerHTML fails four assertions, while keeping the escaping and dropping only the scheme guard fails the href one alone. Review also found the poll teardown test did not exercise the poll. handleWindowVisibilityChange is reachable only from the two listeners and from the retry it schedules itself, and the test dispatched neither, so no request was ever made and the assertion compared zero to zero; it passed with the teardown reverted. It now dispatches focus, asserts requests are being made and keep coming, and only then that they stop. And the teardown could not cancel an in-flight getUser: a null resolving after the deadline ran the code past the await and scheduled a fresh retry with nothing left to clear it. A closure-local flag checked after the await stops that, chosen over a second guard at the top of the handler because only one of the two is detectable by mutation and this is the one that prevents the stray timer rather than neutering it. The inline handler in the iframe template accepted messages from any window while acting on them through location.replace and document.title. It now takes only the parent, the same check the host page side makes.	2026-08-22 12:34:24 -05:00
UmputunandGitHub	556e0a70d5	chore(release): build binary artifacts with GoReleaser (#2070 ) 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.	2026-05-22 13:24:32 -05:00

Dmitry VerkhoturovandGitHub

4793c1cd2c

Fill the instance URL into the embedded frontend at serve time, and stop pinning compressor output in tests (#2198 )

* 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.

2026-08-22 13:15:27 -05:00

Dmitry VerkhoturovandGitHub

23be25d84a

Fix seven widget defects, including the cookies the separate-domain setup needs (#2197 )

* Drop the frontend workspace root and re-resolve the lockfile

`frontend/` carried a `package.json`, a `pnpm-workspace.yaml` and the lockfile
for a workspace of exactly one package. Two manifests meant two places to
declare a version, and the app pin was the one that did not win: `preact` and
`@babel/core` were each written twice, and a bump to the app manifest alone
would have been a silent no-op, since `pnpm.overrides` decides and it lived at
the root.

Everything pnpm reads now lives in `frontend/apps/remark42`: dependencies,
`packageManager`, `engines` and the overrides. `frontend/` keeps `.nvmrc`,
`.husky` and `CLAUDE.md`, none of which pnpm reads. The directory nesting
stays: every path in the repository points at `frontend/apps/remark42`,
including the published contributing docs, so moving the package up would have
rewritten 14 files to no benefit.

Moving the manifest kept the old resolutions verbatim, which left optional peer
subtrees the tree no longer reaches: `ts-node` under jest, `@swc/core` under
webpack, `vitest` under `@testing-library/jest-dom`, `tslib` under
`webpack-dev-server`. None is referenced by any config or source file here.
Re-resolving drops 137 packages and moves 59 to versions already permitted by
the ranges in the manifest, 1446 to 1308, with no direct dependency changing
version: the five that look changed differ only in their peer suffix. Every
file `pnpm build` produces is identical in size before and after.

The frontend-deps stage of the Dockerfile sets `CI=true` so the `prepare`
script skips husky, which has no git repository to install hooks into there.

* Stop markdown-only changes triggering heavy workflows, and check the documented versions

`ci-backend.yml`, `ci-build.yml` and `ci-frontend.yml` all end their path
filters with `!**.md`. The e2e workflow did not, so a change to any markdown
file under `frontend/` or `backend/` matched its `frontend/**` and `backend/**`
entries and started a docker build and the whole browser suite. The release
filter had the same hole and two of its own: it names `README.md` and `LICENSE`
on purpose, since `.goreleaser.yml` packages both, so it now excludes markdown
under `backend/` and `frontend/` only. `CLAUDE.md` and the installation page
were listed as well, and neither is packaged.

`ci-site.yml` goes on matching markdown, which is right, since the site is
built from it. It excludes `CLAUDE.md`, so a future `site/CLAUDE.md` cannot
start a site build, and `site/README.md`, which documents how to build the site
rather than being part of it.

The installation page tells a reader that a source build needs Go 1.25, Node
24+ and PNPM 10. Nothing kept those in step with `backend/go.mod`,
`engines.node`, `packageManager` and `.nvmrc`, and the drift is silent: a wrong
version in the docs builds and tests exactly as well as a right one. `.nvmrc`
is the pin with form here, having sat at 16 through the whole node 20 migration
because nothing red ever pointed at it. The check compares each stated version
against its source and holds `.nvmrc` to `engines.node`, and it fails when the
page states no version at all, so removing the claims cannot turn it into a
check that passes by comparing nothing.

Its own workflow rather than a step in an existing one, since the inputs span
the backend module, the frontend manifest and the site.

* Fix the cookie fallback page, asset path, message senders, auth teardown and cookies

Two defects with the same origin: 5825a55b, the January 2021 frontend
rewrite, first released in v1.7.0.

It removed the build entry for comments.html while leaving both the
template and the link to it in place, so the page the auth panel offers
when third-party cookies are blocked has been a 404 ever since, for
exactly the reader who has no other way in. The template needed no
changes; it is built again, and an e2e case now opens it on a thread
carrying a comment and waits for that comment, so the page being served,
its inline script running and it asking for the thread named in its own
query string are all covered. Against an image built without the plugin
entry that case fails on the 404, which is the regression it exists for.

It also fixed the public path to the domain root, so an instance mounted
under a prefix, which manuals/separate-domain documents, asked for
/web/google.svg when its own icons live under that prefix. Fifteen
provider icons in remark.mjs and one in last-comments.mjs. The path is
now derived from the url the bundle was loaded from, which is correct for
both arrangements, and the file loader no longer overrides it.

The host page also accepted postMessage from any window: every frame on a
page can reach window.parent, and the handler resizes the widget, scrolls
the page and opens the profile overlay. It now ignores anything that did
not come from a frame this module created.

A fourth, in the same family: the OAuth flow never tore its polling down.
`subscribed` was declared, checked and cleared but never set, so the guard
against a second subscription was dead code and every provider click
attached another listener pair. The five minute deadline then rejected
without unsubscribing, leaving those listeners and a retry that
reschedules itself for as long as getUser returns null. Cross-domain is
where getUser never stops returning null, so a reader on the arrangement
manuals/separate-domain documents was left polling /auth/user once a
minute for the life of the page, against a route capped at 2 req/s. It
also rejected with no argument, and the caller stores that as the error
state, so the interface had undefined to render. The deadline now tears
the subscription down and rejects with an error.

The message check had a second half. Hardening the parent left the widget
document trusting any sender, and it acts on signout and theme, so
anything holding a reference to the frame could sign a reader out.
`auth.hooks` already checked `event.source !== window.parent`; that check
is now a shared `isFromParent` and the three listeners that lacked it use
it too. The origin cannot stand in for it, since the host page is
whatever site embeds the widget and `ALLOWED_HOSTS` is enforced server
side through `frame-ancestors`.

And createInstance stacked its listeners. It reuses the marked iframe
instead of building one, but installed three listeners plus a title
observer on every call, while destroy could only reach the newest
closure, so a second call without a destroy stranded a set for good. The
listeners of the current instance are now detached before the next set
goes on. Reuse and the ignored config are unchanged: that contract is
open in the backlog note and not settled here.

The auth cookies the embedded case needs were not being delivered, in
both halves of the client's own writer. The name was decorated:
setAuthCookie prefixed with __Host- whenever the page was https, so a
real deployment wrote __Host-JWT and __Host-XSRF-TOKEN while the backend
looks for JWT and the fetcher reads XSRF-TOKEN, and nothing anywhere
reads a prefixed name. Nothing caught it because the prefix is applied
from the page protocol and every test and the dev server run on http;
there is now a second suite pinned to an https page, which is the only
condition that shows it. And the attributes could not be delivered: both
were SameSite=Strict, judged against the top-level site and not the
request's own origin, so a Strict cookie is never sent from a
third-party frame, which is the entire configuration this code exists
for. They now follow the embedding, Strict while the widget shares its
page origin and None with Secure and Partitioned once it does not, since
that is the only third-party form browsers still accept. Over http in a
third-party frame no combination works, and the strict form is written
instead of one the browser would reject outright.

That leaves the client half of #1877 working, whose reporter wanted
AUTH_SEND_JWT_HEADER for exactly this arrangement, and whose first half
merged as #1929. The server's own cookies still carry no Partitioned;
that is upstream work in go-pkgz/auth.

Two plan changes. A review pass corrected its central Path B premise,
which said the first document render is anonymous permanently, in every
configuration: it is anonymous in the configuration remark42 ships,
go-pkgz/auth exposing XSRFIgnoreMethods and remark42 leaving it unset.
The door is not shut, it is closed by a setting, and opening it is
scoped security work and not a flag flip, because GET /deleteme
deletes every comment a user has written and is a GET so the emailed
link works. And the separate-domain arrangement is promoted from a
constraint bullet to a named requirement with acceptance criteria, since
a test that signs in and posts without reloading passes while
persistence is entirely broken.

Review found a seventh, and it was reachable only because of the first:
comments.ejs built its title with innerHTML from the url query
parameter, so restoring the build entry made a reflected XSS live on the
instance origin, where the page is a top-level document, frame-ancestors
does not apply and the /web CSP allows unsafe-inline. The anchor is now
built through the DOM with textContent, and only http and https reach
href, since escaping alone leaves a javascript: url working. Two e2e
subtests pin both halves, and mutation testing separates them: restoring
innerHTML fails four assertions, while keeping the escaping and dropping
only the scheme guard fails the href one alone.

Review also found the poll teardown test did not exercise the poll.
handleWindowVisibilityChange is reachable only from the two listeners
and from the retry it schedules itself, and the test dispatched neither,
so no request was ever made and the assertion compared zero to zero; it
passed with the teardown reverted. It now dispatches focus, asserts
requests are being made and keep coming, and only then that they stop.
And the teardown could not cancel an in-flight getUser: a null resolving
after the deadline ran the code past the await and scheduled a fresh
retry with nothing left to clear it. A closure-local flag checked after
the await stops that, chosen over a second guard at the top of the
handler because only one of the two is detectable by mutation and this
is the one that prevents the stray timer rather than neutering it.

The inline handler in the iframe template accepted messages from any
window while acting on them through location.replace and document.title.
It now takes only the parent, the same check the host page side makes.

2026-08-22 12:34:24 -05:00

UmputunandGitHub

556e0a70d5

chore(release): build binary artifacts with GoReleaser (#2070 )

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.

2026-05-22 13:24:32 -05:00