Files

T

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

9.9 KiB

Raw Blame History

Remark42 Development Guidelines

Build/Test/Lint Commands

Backend:
- Run server: make rundev
- Build: make backend
- Race test: make race_test
Backend Testing:
- Run all tests: cd backend/app && go test -timeout=300s -count 1 ./...
- Run single test: cd backend/app && go test -run TestName ./path/to/package
- IMPORTANT: Run example tests: cd backend/_example/memory_store && go test -race ./... && go build -race ./...
Frontend:
- Development: cd frontend/apps/remark42 && pnpm dev
- Tests: cd frontend/apps/remark42 && pnpm test
End-to-end: make e2e drives the widget in a real browser; see e2e/README.md. Build-tagged, so go test ./... never runs it.
Lint:
- Backend: cd backend && golangci-lint run
- IMPORTANT: Example lint: cd backend/_example/memory_store && golangci-lint run --config ../../.golangci.yml
- Frontend: cd frontend/apps/remark42 && pnpm lint
- Before committing: Always run tests and linter on both main backend AND examples
Go module changes:
- Any change to backend/go.mod or backend/go.sum requires go mod tidy in backend/_example/memory_store in the same commit. That covers dependency bumps, adding or removing a dependency, and changing the go directive, not only version updates.
- Only go mod tidy there, not go mod vendor: the example's vendor directory is gitignored (.gitignore:26), so its output is never committed, while a stale local copy silently becomes what the example resolves against.
- The example module replaces github.com/umputun/remark42/backend with ../../, so it carries the backend's dependencies as indirect entries. Leaving them stale fails the test examples CI step with go: updates to go.mod needed; to update it: go mod tidy.
- This applies to Dependabot pull requests too: the bot updates backend/ only, so its Go module PRs need the example tidied before they can go green.

Backend Test Determinism

Backend tests must never depend on how fast the machine is. CI runs them under -race with coverage on a shared runner, so any test that assumes an operation finishes within some duration eventually fails on a rerun-and-it-passes basis.

Wait on a condition, never on a duration. Use require.Eventually / require.EventuallyWithT to poll for the state the assertion needs, and require.Never when the point is that something did not happen. A bare time.Sleep before an assertion is a defect; sleeping until a deadline you computed, as waitPastMillisecond does, is not.
Polling closures must not touch *testing.T. testify runs them on a separate goroutine, where t.FailNow is undefined behaviour. Assert on the *assert.CollectT that EventuallyWithT hands the closure, so the real error also lands in the failure message.
Mind the rate limiter when polling over HTTP. Route groups are capped independently and most of the caps are hard-coded in rest.go, out of reach of a test: /auth/ at 2 req/s and the admin, protected and image routes at 10 req/s. Only the open-route group is settable, via openRouteLimiter (100 in startupT). Poll with the existing constants rather than a new number, httpPoll for anything issuing an HTTP request and pollInterval only for in-process or filesystem checks, or the poll manufactures the 429s it then has to interpret.
When a test needs time to have passed, pin the clock input rather than waiting for it: os.Chtimes for file ages, an explicit store.Comment.Timestamp for anything that formats a timestamp.
Prefer a testing/synctest bubble where the code under test has no real I/O. Inside one the clock is fake, so time.Sleep is instant and deterministic. app/notify, app/store/service, app/store/image, app/store/engine, app/providers, app/migrator and _example/memory_store/accessor already use it, and most surviving time.Sleep calls live in them.
Helpers fail loudly. A wait that gives up must call t.Fatal/require naming what it was waiting for, never return silently and leave the next assertion to fail with something unrelated. Because these packages run goleak.VerifyTestMain, a failing helper also exits the test goroutine, so anything that started a server in a goroutine must defer cancel() or defer srv.Shutdown() right after launching it; otherwise a failed readiness wait is reported as a goroutine leak rather than the failure that caused it.
Take ports and paths from outside the test. Ports come from the kernel with net.Listen("tcp", ":0"), files from t.TempDir(). go test ./... runs package binaries concurrently, so a number out of a fixed range or a fixed name under /tmp lets two of them collide.
Close idle connections before shutting a test server down. Clients built as http.Client{Timeout: x} share http.DefaultTransport, and Shutdown waits on their keep-alive connections until its own deadline expires.
Keep the test timeout budgets aligned. Makefile, ci-backend.yml, release.yml and the command above all use -timeout=300s; the wait helpers allow 30s per condition, so a shorter per-package budget turns a slow runner into a timeout panic instead of a readable failure.

chooseUnusedPort and the server-start wait helpers are duplicated in app, app/cmd, app/rest/api and _example/memory_store/server. Nothing shares them today; keep the copies in step when changing one.

Release Procedure

Remark42 uses two tags for each release:

vX.Y.Z - product release tag used by GitHub releases, GoReleaser binary artifacts, and Docker image publishing.
backend/vX.Y.Z - nested Go module tag for github.com/umputun/remark42/backend.

Release flow:

Create the GitHub release for vX.Y.Z with title Version X.Y.Z. The GitHub release must exist before the vX.Y.Z tag reaches the remote; gh release create vX.Y.Z satisfies this because it creates and pushes the tag.
The vX.Y.Z tag triggers GoReleaser, which builds and uploads binary artifacts to the existing release.
Create and push the matching backend module tag pointing at the same commit:

git fetch origin --tags
git tag backend/vX.Y.Z vX.Y.Z
git push origin backend/vX.Y.Z

GoReleaser must ignore backend/* tags in .goreleaser.yml so release notes and current-tag detection use only product tags. Docker image publishing stays separate and is handled by the existing Docker workflow.

For local artifact runs, install GoReleaser, Go 1.25, Node 24+, PNPM 10, and Perl, then use make release. The target runs a snapshot/no-publish GoReleaser build, leaves local artifacts and metadata in dist/, and cleans generated frontend embed files after GoReleaser exits. Do not run raw goreleaser release for local artifacts unless you also run ./scripts/cleanup-release-assets.sh afterward.

Milestones and Issue Labels

Milestones — one vX.Y.Z milestone per release. Assign every merged PR, and every issue closed by a code change, to the milestone of the release it shipped in.

Decide which release a PR belongs to by whether its merge commit is contained in a release tag — not by comparing dates (a tag can be cut from an earlier commit, or moved). git fetch --tags, then git tag --contains <merge_sha> | grep '^v' | sort -V | head -1 is its release. If no release tag contains it yet, it belongs to the next (unreleased) version's milestone — create it if missing (gh api repos/umputun/remark42/milestones -f title="vX.Y.Z").
An issue gets a milestone only when it was closed by a code change (a linked closing PR/commit); take the milestone from that PR/commit (via the commit-in-tag rule). Issues closed as duplicate/invalid/wontfix/answered get no milestone.
Find unassigned: gh pr list --state merged --search "no:milestone", gh issue list --state closed --search "no:milestone". Assign with gh pr edit N --milestone "vX.Y.Z" / gh issue edit N --milestone "vX.Y.Z".

Issue labels — classify each issue with a type and an area (add priority when relevant):

Type: bug, enhancement, question, documentation, discussion
Area: backend, frontend, site, CI, design, localization
Priority: important, minor, some day
Contribution: help wanted, good-first-issue
Resolution (on close, when applicable): duplicate, invalid, wontfix, no-action-needed
PR auto-labels (applied by Dependabot/Actions, not manual PRs): dependencies, go, javascript, github_actions

Code Style

Backend: Formatting with golangci-lint, strict error handling
Frontend: TypeScript with ESLint, Stylelint and Prettier
Imports: Group stdlib, external packages, then internal packages
CSS: All components use CSS Modules (component.module.css). Class naming: BEM block = .root, elements = camelCase, modifiers = camelCase. Use clsx for conditional class composition. raw-content.css is the only global CSS file (syntax highlighting utility). Root wrapper keeps bare .dark/.light theme class — 8+ module CSS files depend on :global(.dark) ancestor. comment_highlighting uses :global() for imperative classList usage in root.tsx

Key Backend Packages

Web/API: github.com/go-pkgz/routegroup, github.com/go-pkgz/rest
Auth: github.com/go-pkgz/auth/v2
Logging: github.com/go-pkgz/lgr
Testing: github.com/stretchr/testify
Notifications: github.com/go-pkgz/notify

Repository Structure

Backend: Go server using BoltDB for storage
Frontend: Preact/Redux-based UI with iframe embedding
/web is served from two sources, in lookup order: the frontend build output (frontend/apps/remark42/public, embedded at backend/app/cmd/web or read from --web-root), then backend/app/webassets/assets, embedded in the binary. A plain page or image the bundler does not process belongs in webassets; anything needing templating or the widget's CSS/JS goes through webpack. A name present in both is served from the frontend build.

9.9 KiB Raw Blame History