Bring the plan up to master, and mark two scope calls as the maintainer's

An end-to-end pass against current master found the document stale in places
where it is used as a factual base for a decision, and found two places where
this branch had overstepped.

The two overreaches first. The acceptance criteria narrowed R1 from any
configured provider to three flows, which is a product-scope decision and not
the factual correction it was presented as, especially while the Path B
constraints still say fixing #1139 must not get harder. And the criterion is
met for anonymous and email by measurement but for Telegram by inference, which
sits badly with a standing requirement being a test a proposal passes. Both are
now written as open decisions with the alternatives spelled out.

Two costs were wrong in Path B's favour and against it. The R1 line claimed a
server-rendered fragment has no client-side writer, contradicting Path B's own
task list, which retains the existing auth through htmx:configRequest; fetcher.ts
already turns X-JWT into the partitioned pair, so the upstream Partitioned work
is mandatory only for a server-set default-on solution. And the XSRF caution
rested on GET /deleteme as though it were unprotected: it sits under radmin
behind Auth, AdminOnly and matchSiteID, and the handler needs a signed token
carrying delete_me, so exempting GET removes one gate of three. The caution
stands but now asks for an audit instead of leaning on that example.

Task 2's backlog was sending someone to write tests that exist: the cross-origin
page, comments.html and its injection case, and the config surface are all
covered now. What is left is hash deep links, max_shown_comments, Telegram, the
storage-denied trigger, and a path-prefixed deployment that #2219 makes concrete.

Also refreshes the figures to 7de51ad2, and adds two items master made
necessary: the published locale list names 17 languages against 24 shipped, and
Path B has to keep the explicit height report #2213 added on panel close.
This commit is contained in:
Dmitry Verkhoturov
2026-08-24 01:09:05 +01:00
parent 6f7313fcd7
commit a0680b11ea
@@ -39,6 +39,23 @@ cookies. Nothing short of the reload proves it: the widget holds a token in memo
page, so a test that signs in and posts without reloading passes while the persistence is entirely
broken.
**Two things about this criterion are decisions, not findings, and they belong to the
maintainer.** They are marked because an earlier version of this section made both silently.
The first is that it excludes OAuth, where the requirement as originally written said any
configured provider. That is a narrowing of product scope, not a correction of fact. OAuth
off-domain is impossible for the flow as built, but the routes priced below would change that, and
the narrowing sits awkwardly beside `fixing #1139 must not get harder` in the Path B constraints,
which is the same subject. Either the requirement excludes OAuth and #1139 is a separate goal
carrying its own timeline, or the requirement keeps OAuth and is not met today. This document
assumes the first and is not entitled to.
The second is what counts as evidence. The criterion is met for anonymous and email by measurement,
and for Telegram by inference from the client writer keying off `X-JWT` and not off the provider. A
standing requirement is meant to be a test a proposal passes, so either inference is acceptable
evidence here and the section should say so, or Telegram leaves the criterion partly pending until
#2208 and `go-pkgz/auth` #316 make it measurable.
OAuth sits outside that criterion as the flow is built today. The provider callback is a top-level
navigation in a popup, so `Service.Set` writes its
cookie there as an ordinary first-party cookie for the auth host, carrying no `Partitioned` and so
@@ -92,20 +109,19 @@ it is partitioned. #2214's control cookie encodes exactly that: an unpartitioned
cookie written by the same script in the same frame has to be dropped, or the case declares itself
vacuous.
Documenting it is the remaining gap for the flows named above, and it is two edits, not one.
`site/content/docs/manuals/separate-domain/index.md` covers only `ALLOWED_HOSTS` and
`AUTH_SAME_SITE`, so it needs the flag added. `site/content/docs/configuration/parameters/index.md`
does document the flag, but its mitigation list still promises `SameSite=Strict` cookies and a
`__Host-` prefix on https, neither of which `authCookieOptions` does in a third-party context since
#2197, so that page needs correcting. Its warning that the flag increases XSS exposure compared with
server-set `HttpOnly` cookies is the trade-off an operator has to be handed in the same breath as
the recommendation.
The documentation gap this section named is closed by #2218. The separate-domain manual now
recommends `AUTH_SEND_JWT_HEADER=true` and carries the XSS trade-off in the same breath, and the
parameter page no longer promises `SameSite=Strict` cookies and a `__Host-` prefix that
`authCookieOptions` stopped emitting in a third-party context at #2197. That PR also measured
`AUTH_SAME_SITE=none` out of the recommended recipe: with the header flag on it adds an
unpartitioned `HttpOnly` JWT to third-party delivery and contributes nothing to persistence, which
the cookie jar shows directly.
The e2e suite covers the rendering half through `TestCrossOrigin_WidgetRendersOnAnotherOrigin`,
which
proves the document loads on another origin and reports itself through postMessage across the
boundary, and `ALLOWED_HOSTS` refusal through `TestCrossOrigin_DisallowedHostNeverReportsInited`.
The open #2214 adds the authentication half over TLS, the reload included, and runs it once more
#2214 added the authentication half over TLS, the reload included, and runs it once more
against a browser configured to block third-party cookies. That second case needs
`IgnoreDefaultArgs`, because Playwright's own `--disable-features` list switches partitioning off
and
@@ -263,11 +279,17 @@ exposes on `Opts` and threads into the JWT service. remark42 leaves it unset, so
list applies and the short circuit in `Service.Get` never fires for any method. Setting it would
make an authenticated document render possible.
Do not reach for it globally, though. `GET /deleteme` deletes every comment a user has written, and
is a GET deliberately, so that the link in the confirmation email works when clicked. Exempting GET
from XSRF wholesale removes that protection from a destructive endpoint. Anything built on this has
to scope the exemption to the document route alone, and that route has to be provably side-effect
free. Cost that work rather than assuming either that the door is shut or that it is open.
Do not reach for it globally, though, and note that the obvious example does not carry the argument.
`GET /deleteme` deletes every comment a user has written and is a GET deliberately, so that the
link in the confirmation email works when clicked, but XSRF is only one of three gates on it: the
route sits under `radmin`, which applies `Auth`, `AdminOnly` and `matchSiteID`, and
`deleteMeRequestCtrl` then requires a separately signed token carrying a `delete_me` attribute it
cannot forge. Exempting GET wholesale removes one defence there, not the only one.
The caution still stands, but it has to be earned by an audit instead of by that example: scope any
exemption to the document route, establish that route is side-effect free, and check every other
authenticated GET before deciding how much route-scoped work the library needs. Cost that audit,
without assuming either that the door is shut or that it is open.
There is also a query-parameter path, and it is worse than the constraint: `Service.Get` accepts the
token from a query parameter, `?jwt=` rather than the library default `?token=` because remark42
@@ -285,7 +307,7 @@ item in Path B for how much that is worth.
## Verified facts
Checked against the code at `a82dc8d3` and by independent reviewers.
Checked against the code at `7de51ad2` and by independent reviewers.
- 9 runtime dependencies against 61 devDependencies, and **68** override entries, all in the single
`frontend/apps/remark42/package.json` since #2197 removed the workspace root. Before this effort
@@ -305,7 +327,7 @@ Checked against the code at `a82dc8d3` and by independent reviewers.
- The e2e suite is Go and playwright-go since #2180, and passed 60 tests while this was being
written, up from 7. Treat the exact figure as stale on sight; it is the only coverage that
survives a rewrite
- The unit suite is 46 files, 25 `*.test.*` plus 21 `*.spec.*`, and **426 cases**, as jest
- The unit suite is 48 files, 27 `*.test.*` plus 21 `*.spec.*`, and **426 cases**, as jest
enumerates them. Counting only `*.test.*` understates it by twenty files, which is the trap
- `en.json` is 180 keys with no ICU plural or select forms
- 136 non-test source files under `app/` excluding typings, mocks and stubs, 8,498 lines; 152 files
@@ -338,7 +360,7 @@ Checked against the code at `a82dc8d3` and by independent reviewers.
The concrete "what is left" list, and what any no-npm proposal has to answer for.
- transpiles TS and JSX for 136 to 152 source files, typed by `tsconfig.json`, compiled by the
- transpiles TS and JSX for 158 source files, typed by `tsconfig.json`, compiled by the
preset list in `.babelrc.js`
- CSS modules for 29 `*.module.css` files, 2,219 lines, including 177 nested `&`, 4 `composes` and
10 `:global` occurrences across 6 files, all handled by the CSS-modules rule in
@@ -415,20 +437,33 @@ widget *code* through npm breaks OAuth, but it adds a publish step and a version
- [ ] Document `__colors__` from `window.name` (`templates/iframe.ejs`), which works today and is
undocumented
- [ ] Fix the Astro and Gatsby manuals, which declare `REMARK42: any` and `remark_config: any`
- [ ] Correct the published locale list. `site/content/docs/configuration/frontend/_index.md` names
17 languages under `Locales`, and `app/locales/` ships 24 catalogs, so seven are documented
nowhere and a reader cannot discover them
### Task 2: Extend the e2e suite
**Done.** #2180 moved the suite to Go and playwright-go and took it from 7 tests to 22; #2196 took
it to 48. Every item this task originally listed is covered: vote and its failure path
(`vote_test.go`), edit inside and outside the deadline, delete and reply (`comment_test.go`), sort
change and collapse persistence (`thread_test.go`), anonymous and email auth (`auth_test.go`), the
profile iframe and last-comments (`widgets_test.go`).
it to 63, and master now carries 71 top-level tests. Every item this task originally listed is
covered: vote and its failure path (`vote_test.go`), edit inside and outside the deadline, delete
and reply (`comment_test.go`), sort change and collapse persistence (`thread_test.go`), anonymous
and email auth (`auth_test.go`), the profile iframe and last-comments (`widgets_test.go`).
What remains uncovered is a different list, and it is the contract surface rather than the
behaviour: the `comments.html` fallback, `remark_config` fields (`url`,
`page_title`, hash deep links, `max_shown_comments`, the three `show_*_subscription` flags,
`__colors__`), the listener leak on a repeated `createInstance`, timezone-local date rendering, the
unknown-locale fallback, and the composer.
Most of what this task once listed as the remaining contract surface has since been covered too,
and the list is kept short here because an out-of-date backlog sends someone to write tests that
exist. Now covered: the cross-origin host page (`crossorigin_test.go` and `https_test.go`), the
`comments.html` fallback and its injection case
(`TestWidgets_CommentsPageOpensAThreadOnItsOwnOrigin`
and `TestWidgets_CommentsPageRefusesInjectedMarkup`), and the `remark_config` fields `url`,
`page_title`, `__colors__`, the subscription flags, timezone rendering and the unknown-locale
fallback, all as `TestConfig_*` cases in `config_test.go`. The composer is substantially covered by
`comment_test.go`, including drafts and a failed post. Repeated `createInstance` is covered at unit
level in `embed.test.ts`, though not end to end.
What is genuinely still uncovered: hash deep links, `max_shown_comments`, Telegram auth and its
subscription flag, and the storage-denied fallback trigger, which `e2e/README.md` explains needs
WebKit. Telegram is blocked on #2208 and `go-pkgz/auth` #316. #2219 adds one more: a deployment
under a path prefix, verifying chunks and assets for every entry including `deleteme`.
### Task 3: Stable class names and a documented override stylesheet
@@ -521,6 +556,10 @@ compiled, it is what verifies it.
(`URLKeyWithUser`, used by `findCommentsCtrl`), one entry per user with a separate `admin!!` key.
An HTML cache fragments the same way, so the shared-cache benefit only pays for logged-out readers
- [ ] Attach the existing auth via `htmx:configRequest` on fragment requests
- [ ] Preserve the explicit height report on panel close. `useDropdown` in `auth.hooks.ts` calls
`updateIframeHeight()` when the sign-in panel closes, added by #2213, because the panel is
absolutely positioned and neither ResizeObserver sees it disappear. A server-rendered replacement
loses the widget's height entirely if it drops that call
- [ ] Keep client-side: embed script, auth popups, composer, collapse and hidden-user state,
optimistic votes, and the **three subscription flows**. Email, Telegram and RSS
(`comment-form/__subscribe-by-email/`, `__subscribe-by-telegram/`, `__subscribe-by-rss/`) are each
@@ -552,9 +591,13 @@ compiled, it is what verifies it.
**Cost**: months to an opt-in parallel UI reads as a floor derived from the optimistic architecture,
and the optimistic architecture does not hold. Anonymous-first is forced rather than chosen, so the
fragment layer reproduces the entire authenticated tree rather than a delta; add the three
subscription flows and, if R1 is to be honoured, the upstream `Partitioned` work in `go-pkgz/auth`,
since a server-rendered fragment has no client-side writer and its `Set-Cookie` is stored in a
third-party frame only when it carries the attribute.
subscription flows. R1 costs Path B less than an earlier version of this line claimed: it said a
server-rendered fragment has no client-side writer and therefore needs the upstream `Partitioned`
work, which contradicts Path B's own design, since the task list below retains the existing auth
through `htmx:configRequest`. `request` in `fetcher.ts` already turns `X-JWT` into the partitioned
pair, so Path B can keep that writer or attach the same handling to a response hook. The upstream
work is mandatory only for a server-set, `HttpOnly`, default-on solution, which is a different
choice from the one Path B is making.
On one contributor the realistic figure is long enough that the plan's own warning applies to the
schedule and not only to the design. Deletes the entire npm toolchain. **Against it**: 2,400 lines
of the most stateful code get rewritten; a second HTML-fragment API surface becomes permanent