diff --git a/docs/plans/2026-08-19-frontend-simplification.md b/docs/plans/2026-08-19-frontend-simplification.md index 58dfff0e..abdbbf3d 100644 --- a/docs/plans/2026-08-19-frontend-simplification.md +++ b/docs/plans/2026-08-19-frontend-simplification.md @@ -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