master
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  Dmitry VerkhoturovandGitHub
|
2640aaee9e |
Reach what http cannot: the widget over TLS, embedded cross-origin (#2214)
Every service in the suite spoke http, and the browser gates a whole class of behaviour on the page protocol: Secure cookies, SameSite=None, Partitioned, and any code reading location.protocol. None of it was executed, which is how setAuthCookie came to decorate its cookies with __Host- on https pages and survive for years. A TLS pair joins the stack: remark42 with SSL_TYPE=static on 8443, and an nginx serving a host page on its own name on 8444, both on a self-signed certificate that e2e/tls/generate.sh makes and .gitignore keeps out. Every context accepts it, and so does the readiness client, since those are the only servers either talks to. The instance also runs with AUTH_SEND_JWT_HEADER, which is what makes the widget write cookies of its own: without it the client-side writer never runs on any https page here and every assertion about the attributes it chooses is vacuous. Three cases. Signing in across origins and then reloading, which is the one the http cross-origin case cannot make: the widget holds its token in memory for the life of a page, so signing in and posting says nothing about persistence and only the reload asks whether the cookie was delivered, stored under a name the backend reads and sent back from a third-party frame. The cookies themselves, read out of the browser store while the widget is embedded elsewhere, since a cookie the browser refused is absent from that list entirely and one it kept but will not send is worse than useless: every copy of both names has to be Secure and SameSite=None, at least one has to be partitioned, and none may carry a __Host- prefix nothing on either side reads. And the same reload under a browser that blocks third-party cookies, which the widget's own partitioned pair is the only reason to survive. That last one needs a browser playwright does not offer: its default arguments disable ThirdPartyStoragePartitioning outright, so a run configured wrongly keeps every third-party cookie and the case would pass while asserting nothing. IgnoreDefaultArgs drops that list and re-supplies it without the one feature, and a control cookie set from inside the frame has to be refused before anything else is read, so a playwright release that changes the list fails as itself instead of going quietly vacuous. All three pass against master. What TLS still cannot reach, the OAuth popup above all, is written down in the README. |
||
|
Dmitry VerkhoturovandGitHub
|
4d5dae20e2 |
Broaden the e2e suite from 21 cases to 63, and harden its harness (#2196)
* Pin the published /web surface in the e2e suite
#2178 renamed the widget bundles from .js to .mjs and the URLs earlier
releases served under those names stopped resolving. Three were noticed
from the demo site; the rest, including every locale chunk, were found
only by requesting the whole surface of both images over HTTP. #2192
restored them with a server-side alias, and nothing in the suite would
have caught the break or would notice it returning.
Two cases with deliberately different criteria. The documented names are
written out, because the documentation decides that list and not the
build: an operator pastes privacy.html into an OAuth application, the
nginx manual proxies index.html by name, and the integration guides start
from the embed script. Everything else is taken from the build itself, so
whatever the bundler emitted has to serve identical bytes under its
legacy .js name and parse as a classic script, which is the premise
serving one under the other rests on. A third case requests a name that
does not exist, without which a fallback serving one page for everything
would keep the whole table green.
All of them check the content type as well as the bytes: nosniff is set
on every response, so a bundle served as text/plain is as broken as one
that 404s while comparing equal.
On
|
||
|
Dmitry VerkhoturovandGitHub
|
49bf83b09c |
Address the review follow-ups from #2188, #2189 and #2190 (#2193)
* Read the collapsed-threads key through getJsonItem `getFromLocalStorage` parsed the stored string directly, so anything malformed under `__remarkCollapsed` threw out of `restoreCollapsedThreads`. That call sits in `remark.tsx` ahead of the `render`, so the throw took the whole widget with it: the reader was left on the preloader, over a view preference. `getJsonItem` in `common/local-storage.ts` already wraps a parse of a localStorage key and returns null on failure, and null is a shape the check below already reads as empty. The rest of that function is total against whatever the browser holds, and the bare parse was the one way in. * Stop retrying a failed e2e test in CI The suite went in with one gotestsum rerun. It has no failures on record to justify that: 31 CI runs since it landed, all green, and no rerun report has ever been produced. A retry is what turns an intermittent regression into a green build, and while the suite is this young its own failures are the evidence worth keeping. `E2E_RUN_ID` stays. It stamps the threads a run works on with the CI run id, so a thread url in a trace or a log names the run it came from. It carries no data across: the stack is disposable, and a local run under the same id gets those urls on an empty database. * Stop two chooseUnusedPort comments claiming collisions cannot happen All four copies listen on :0, read the assigned port, close the listener and bind later, so nothing holds the number across that gap and another binary can take it. The copies in app/cmd and app/rest/api call a collision very unlikely, which is accurate; the ones in app and the example module said binaries never land on the same number, which is not, and a comment ruling out a port collision is what would send the next person chasing one somewhere else. All four now read the same. Closing the window rather than describing it means the server binding :0 itself and reporting the address it got, which is a larger change. |
||
|
Dmitry VerkhoturovandGitHub
|
b6975af63c |
Fix collapsed threads not restoring, and the clock skew correction (#2188)
* Fix collapsed threads not restoring, and the clock skew correction
Collapse state was kept as a flat list of `siteID_url_commentID` strings
and read back by splitting on `_`. Any underscore in the url, the site id
or the comment id made the pieces impossible to tell apart, so a page
whose url contains one lost its collapsed threads on every reload, and one
page's entries could be read or deleted as another's: `/post` matched
everything stored for `/post_2`, and a site id of `blog` matched `blog_ru`.
No separator fixes that, since every candidate can occur inside the values,
so the ids are now nested under the site and the url instead. Anything
stored in the old shape reads as empty: collapsed threads are a view
preference, and re-expanding them once is not worth a migration.
The e2e suite had been stripping underscores out of its own thread urls to
work around this, which left its collapse test unable to fail on the bug it
covers. That workaround is gone, and the test now fails without this fix.
`serverClientTimeDiff` was written in seconds and added to an epoch in
milliseconds, so the correction it exists to apply was a thousandth of the
real skew. It is now milliseconds, and named for the unit.
A response with no usable `date` used to fall back to a zero timestamp,
which already made the "skew" about twenty days and would have made it
fifty-five years once the units were right. Nothing is stored now unless
the reading is plausible, since `Date.parse` is lenient enough to turn junk
into a date and let an absurd value through the branch that parses.
The score tooltip reports controversy again when there is any. It has been
dead since the vote component was rewritten in
|
||
|
Dmitry VerkhoturovandGitHub
|
a0879b2336 |
Measure the iframe reveal budgets from inside the page (#2189)
The three reveal tests timed their budgets from before `page.Goto`, so a slow navigation was spent against a window that belongs to the iframe. In `TestIframe_StaysHiddenUntilTheDocumentReportsInited` that made the test vacuous rather than flaky: on a navigation between 2.5 and 5 seconds the loop bounding the visibility assertion had no budget left, ran zero times, and the test passed having asserted nothing. Reproduced by delaying the demo document by three seconds, where the assertion ran 0 times before and runs 23 after. The timeout test had the mirror of it, with navigation counting toward the lower bound that exists to catch a shortened fallback. An init script now records, in the page, when the widget's iframe element enters the document and when its visibility first flips. `create-iframe.ts` arms its fallback a moment earlier, on the detached element, so these read a shade short and every bound is conservative in the same direction. Both bounds were also wider than the thing they guard. The hidden window now runs almost to the fallback rather than half of it, and the lower bound sits just under it rather than at three quarters, which a fallback shortened to four seconds used to clear. CI reruns a failing test once rather than failing the build on the first flake. A browser suite has a floor no amount of care removes, and one flake failing the build is what stops people trusting the suite. Once rather than twice, because a rerun stops at the first pass and each further attempt only widens the window where a real intermittent regression is absorbed. What needed a rerun is written to a report and uploaded with the traces, which are kept whether or not the job went green: a run that recovered on the rerun is exactly the one whose evidence used to be discarded. |
||
|
Dmitry VerkhoturovandGitHub
|
ff77f41a3a |
Move the e2e suite to Go and playwright-go (#2180)
* Move the e2e suite to Go and playwright-go The seven playwright tests in `frontend/e2e` become twenty in `e2e/`, a separate Go module driving the same browsers through playwright-go. The npm project, its lockfile entries, its prettier config and `Dockerfile.e2e` go with it, leaving `frontend/` a single-member workspace. The suite covers posting with markdown, replying and the nesting that implies, editing inside the deadline and the backend refusing one outside it, deleting, voting with the optimistic score observed mid-flight and rolled back on failure, changing the sort, collapse persistence across a reload, dev, anonymous and email sign-in end to end, the profile iframe, and the two scripts that render into the host page rather than the widget's own frame. The rendering tests run in chromium, firefox and webkit. The rest sign in, sign-in needs the dev oauth2 provider, and reaching that by name from the host is chromium-only, so they run there alone. `compose-e2e-test.yml` runs remark42, a second instance with a short edit window so that path does not need a five-minute test, and mailpit, which catches the email verification message the suite reads back. Everything binds to the loopback interface: the stack holds a known secret and an admin shared id, and `go test` can start it unattended. The tests run on the host rather than in a container. Three settings there exist for the tests rather than for realism. `REMARK_URL` uses a hostname because the dev oauth2 server binds whatever host it reads out of it, and a loopback bind inside a container cannot be published. `UPDATE_LIMIT` is raised because the default of 0.5/sec rejects any test posting twice in a row. The suite also paces its own `/auth/` calls, which are capped at 2/sec by a bare literal in `rest.go` rather than by a setting. Each test gets its own comment thread from a query string on the demo page, so nothing has to reset the database between runs. CI gains a vet and lint job for the module, since the build tag keeps it out of a plain `go test ./...`, and uploads a browser trace for any test that fails. `e2e/README.md` carries the rest: how to run it, what the stack is for, and the widget behaviour the assertions have to work around. * Update golangci-lint to 2.13.1 in the backend workflow The pin sat three minors behind what the linter installs locally, so CI checked the backend with an older set of rules than anyone running it by hand. 2.10.1 also fetches its config schema over the network on every `config verify`, which is a failure mode with no bearing on the code. Both targets are clean on 2.13.1, `backend/app` and the memory_store example. |