diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 571faa15f3..c68b05f2a9 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -61,7 +61,7 @@ jobs: tests: name: Tests needs: vet - # generous against the docker build plus one 8m go test: a job cancelled on timeout skips + # generous against the docker build plus the full browser suite: a job cancelled on timeout skips # its own failure steps, so the run would end with neither logs nor traces timeout-minutes: 45 runs-on: ubuntu-latest @@ -106,10 +106,9 @@ jobs: # log names the run it came from env: E2E_RUN_ID: ${{ github.run_id }}-${{ github.run_attempt }} - # 20m, matching the Makefile. the suite runs about four minutes on a laptop and a runner - # is slower, so a tighter budget turns a loaded runner into a timeout panic instead of a - # readable failure. the job's own timeout above is what bounds a wedged run - run: cd e2e && go test -tags=e2e -count 1 -timeout 20m -v ./... + # 20m and four parallel tests, matching the Makefile. the timeout leaves a loaded runner + # room to produce a readable test failure; the job's own timeout bounds a wedged run + run: cd e2e && go test -tags=e2e -count 1 -parallel 4 -timeout 20m -v ./... - name: Server logs on failure if: failure() diff --git a/Makefile b/Makefile index 8d3d68c0f5..2f2857dc0e 100644 --- a/Makefile +++ b/Makefile @@ -51,9 +51,9 @@ e2e-down: # the suite brings the stack up itself when it finds none, so e2e-up is only worth running # to keep the containers between invocations e2e: - cd e2e && go test -tags=e2e -count 1 -timeout 20m ./... + cd e2e && go test -tags=e2e -count 1 -parallel 4 -timeout 20m ./... e2e-ui: - cd e2e && E2E_HEADLESS=false E2E_KEEP=1 go test -tags=e2e -count 1 -v -timeout 20m ./... + cd e2e && E2E_HEADLESS=false E2E_KEEP=1 go test -tags=e2e -count 1 -parallel 4 -v -timeout 20m ./... .PHONY: bin docker dockerx release race_test backend frontend rundev e2e e2e-up e2e-down e2e-ui diff --git a/compose-e2e-test.yml b/compose-e2e-test.yml index 1358e6f8c5..cf97f02b3a 100644 --- a/compose-e2e-test.yml +++ b/compose-e2e-test.yml @@ -1,5 +1,10 @@ # compose for the e2e suite; the tests themselves run on the host, see e2e/ # +# TRUSTED_PROXY stays unset on every instance. The loopback-only harness gives each browser context +# a distinct X-Real-IP, and the backend honours a forwarding header from any client only while no +# trusted proxy is configured. Setting one collapses every context onto the docker gateway address +# and puts the whole parallel run in one rate-limit and vote-dedup bucket. +# # every port is bound to the loopback interface on purpose. this stack runs with a known # secret, dev oauth2 and an admin shared id, and `go test` can start it unattended, so it # must not be reachable from the network. the suite only ever talks to 127.0.0.1. diff --git a/e2e/README.md b/e2e/README.md index f13a9cbbb7..009a912dea 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -24,6 +24,8 @@ make e2e make e2e-down ``` +The default run admits four top-level tests at once. Each browser context has isolated cookies, storage, a unique thread URL and a stable test-only client IP, so the backend's per-IP controls remain local to one reader. The last-comments case reads a site-wide feed, so it stays serial. + Run from `e2e/`; the compose path is relative to it. A single test: ``` @@ -78,17 +80,20 @@ Both TLS services read a self-signed certificate from `e2e/tls/`, which `e2e/tls The main instance enables the notify module (`NOTIFY_USERS=email`). Without it `email_notifications` is false in the config, the widget never renders the subscribe control, and the whole subscribe, confirm and unsubscribe flow is unreachable from a browser. -The remark42 instances beyond the first offer anonymous sign-in only, for the reason `remark42-shortedit` does: the dev oauth2 provider binds a port fixed at 8084 and cannot be published twice. `remark42-adminedit` gets its admin from `ADMIN_SHARED_ID`, since the anonymous provider derives the user id from the name and the id for a chosen name can be written into the compose file ahead of time. +The remark42 instances beyond the first offer anonymous sign-in only, for the reason `remark42-shortedit` does: the dev oauth2 provider binds a port fixed at 8084 and cannot be published twice. `remark42-adminedit` enables email authentication and sets `ADMIN_SHARED_ID` to the SHA-1-derived ID of `adminedit@example.com`, which is the address its browser case uses. -Three settings exist for the tests and not for realism, and each is there for a reason: +Four settings exist for the tests and not for realism, and each is there for a reason: - `REMARK_URL` uses a **hostname**, not `127.0.0.1`. The dev oauth2 server binds whatever host it reads out of `REMARK_URL` (`localBindAddr` in go-pkgz/auth), and a loopback bind inside a container cannot be published. The browser maps the names back with `--host-resolver-rules`. - `UPDATE_LIMIT=100`, because the default of 0.5 updates a second rejects any test that posts twice in a row. -- The suite paces its own calls to `/auth/`, which is limited to two requests a second by a bare literal at `backend/app/rest/api/rest.go:242` and not by a setting. See `pauseForAuthLimit`. +- Each browser context has its own forwarded client IP. Fresh contexts use the limiter's initial allowance; later auth actions wait one token-refill interval through `pauseForAuthLimit`. The limit is a bare literal at `backend/app/rest/api/rest.go:242`, not a setting. +- `TRUSTED_PROXY` stays unset. The stack is loopback-only, and the harness verifies that its per-context `X-Real-IP` reaches the backend and separates auth limiter buckets. + +`VOTES_IP` follows the same reader model: one browser context is one voter IP. A vote-deduplication case that needs repeated actions from one reader must perform them in the same context. ## What this suite cannot reach -The stack now carries TLS on two services, so behaviour the browser gates on the page protocol is reachable: `Secure` cookies, `SameSite=None`, `Partitioned`, and anything keyed on `window.location.protocol`. `https_test.go` is where those cases live. What is still out of reach is a browser engine other than Chromium for them, since the resolver rules the hostnames need are a Chromium flag. +The stack carries TLS on two services, so behaviour the browser gates on the page protocol is reachable: `Secure` cookies, `SameSite=None`, `Partitioned`, and anything keyed on `window.location.protocol`. `https_test.go` is where those cases live. What is still out of reach is a browser engine other than Chromium for them, since the resolver rules the hostnames need are a Chromium flag. The trap that remains is which cookie policy a run is under. Playwright's own default `--disable-features` argument carries `ThirdPartyStoragePartitioning`, and it beats both `--test-third-party-cookie-phaseout` and `--block-third-party-cookies` passed through `Args`. A run configured that way keeps an ordinary third-party cookie exactly as it would with no flags at all, so it proves nothing while looking like it proved something. The lever is `IgnoreDefaultArgs` on the launch options: drop that default entry and re-supply `--disable-features` without that one feature, which is what `TestHTTPS_SessionSurvivesThirdPartyCookieBlocking` does. Measured on a cross-site https embed: diff --git a/e2e/assets_test.go b/e2e/assets_test.go index e429d5242d..4bd824fd61 100644 --- a/e2e/assets_test.go +++ b/e2e/assets_test.go @@ -22,9 +22,10 @@ const remarkURLPlaceholder = "{% REMARK_URL %}" // The demo pages set remark_config.host from location.origin themselves, so the compiled-in // fallback is never read and the marker could survive into a release without a test noticing. func TestAssets_InstanceURLIsFilledIn(t *testing.T) { + t.Parallel() + page := newPage(t) - pauseForAuthLimit() _, err := page.Goto(baseURL + "/web/") require.NoError(t, err) @@ -52,6 +53,8 @@ func TestAssets_InstanceURLIsFilledIn(t *testing.T) { // The demo pages cannot show this. 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. func TestAssets_WidgetRunsOnTheCompiledInURL(t *testing.T) { + t.Parallel() + page := newPage(t) var configURL string @@ -61,7 +64,6 @@ func TestAssets_WidgetRunsOnTheCompiledInURL(t *testing.T) { } }) - pauseForAuthLimit() _, err := page.Goto(fmt.Sprintf("%s/web/iframe.html?site_id=remark&url=%s", baseURL, neturl.QueryEscape(threadURL(t)))) require.NoError(t, err) @@ -93,9 +95,10 @@ func TestAssets_WidgetRunsOnTheCompiledInURL(t *testing.T) { // intact, so asserting the element or the attribute proves nothing; only a decoded image has a // non-zero natural size. That catches a 404, a malformed URL and a CSP refusal alike. func TestAssets_WidgetImagesActuallyLoad(t *testing.T) { + t.Parallel() + page := newPage(t) - pauseForAuthLimit() _, err := page.Goto(threadURL(t)) require.NoError(t, err) diff --git a/e2e/auth_test.go b/e2e/auth_test.go index 24a2326aa4..40e97dddb3 100644 --- a/e2e/auth_test.go +++ b/e2e/auth_test.go @@ -24,6 +24,9 @@ func signInDev(t *testing.T, page playwright.Page, frame playwright.FrameLocator }) require.NoError(t, err) + // Opening the provider and completing its callback are separate /auth/ requests. The initial + // status probe may still occupy the other token in this reader's bucket. + pauseForAuthLimit() require.NoError(t, popup.Locator("text=Authorize").Click()) // the popup closes itself through the ?selfClose stub. while an oauth sign-in is pending @@ -64,6 +67,8 @@ func signInAnon(t *testing.T, page playwright.Page, frame playwright.FrameLocato } func TestAuth_DevProviderSignsIn(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) @@ -72,6 +77,8 @@ func TestAuth_DevProviderSignsIn(t *testing.T) { } func TestAuth_AnonymousSignsIn(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) @@ -87,6 +94,8 @@ func TestAuth_AnonymousSignsIn(t *testing.T) { // mail catcher, and submit it. The token is what the widget sends, so a broken template or a // broken token round-trip fails here and not silently in production. func TestAuth_EmailSignsIn(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) @@ -127,13 +136,12 @@ func signInEmail(t *testing.T, page playwright.Page, frame playwright.FrameLocat // Sign Out is an icon button, so its title is the only text it carries. // // The first wait is deliberately short. Everything under /auth/ is capped at two requests a -// second for the whole suite, a bare literal at backend/app/rest/api/rest.go:242, and a case -// that signs in on two pages spends that budget twice over. When the read that repaints the -// panel is the request the limiter refuses, the widget shows signed out over a session that -// exists, and waiting longer cannot help because nothing will ask again. So on the short wait -// expiring, hand focus back to the frame: the widget re-probes on visibilitychange and window -// focus while a sign-in is pending, and by then the cookie is long since set. A sign-in that -// genuinely failed still fails here, since the second read finds no state either +// second per reader, a bare literal at backend/app/rest/api/rest.go:242. The initial status and +// login can spend the bucket before the read that repaints the panel, leaving the widget signed +// out over a session that exists. Waiting longer cannot help because nothing will ask again, so +// on the short wait expiring, hand focus back to the frame: the widget re-probes on +// visibilitychange and window focus while a sign-in is pending. A sign-in that genuinely failed +// still fails here, since the second read finds no state either func assertSignedIn(t *testing.T, page playwright.Page, frame playwright.FrameLocator) { t.Helper() @@ -164,6 +172,8 @@ func verificationToken(t *testing.T, body string) string { // assertion after the reload is the point, since a cleared store with a live cookie looks // identical until the page comes back func TestAuth_SignOutEndsTheSession(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) @@ -191,6 +201,8 @@ func TestAuth_SignOutEndsTheSession(t *testing.T) { // delivered in order, so a widget that has visibly acted on the later message has already had the // title message. Without it this would assert on a dropdown that simply has not closed yet. func TestAuth_HostPageMessageDoesNotCloseTheDropdown(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{"theme": "light"}) @@ -230,6 +242,8 @@ func TestAuth_HostPageMessageDoesNotCloseTheDropdown(t *testing.T) { // response says nothing about it. A reader on a flaky connection otherwise appears to be logged // out and cannot get back without signing in again func TestAuth_SessionSurvivesATransientStatusFailure(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -237,11 +251,13 @@ func TestAuth_SessionSurvivesATransientStatusFailure(t *testing.T) { // one failure, then out of the way. Unroute and not a counter, so the restored state is the // real endpoint and not a stub standing in for it require.NoError(t, page.Route("**/auth/status**", func(route playwright.Route) { - _ = route.Fulfill(playwright.RouteFulfillOptions{ + if err := route.Fulfill(playwright.RouteFulfillOptions{ Status: playwright.Int(http.StatusInternalServerError), ContentType: playwright.String("application/json"), Body: playwright.String(`{"error":"failed"}`), - }) + }); err != nil { + t.Errorf("fulfill transient auth failure: %v", err) + } })) pauseForAuthLimit() diff --git a/e2e/comment_test.go b/e2e/comment_test.go index 1cfe1edc15..b3bdf8599e 100644 --- a/e2e/comment_test.go +++ b/e2e/comment_test.go @@ -19,6 +19,8 @@ import ( ) func TestComment_PostRendersMarkdownAndSurvivesReload(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -43,6 +45,8 @@ func TestComment_PostRendersMarkdownAndSurvivesReload(t *testing.T) { } func TestComment_ReplyNestsUnderItsParent(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -61,6 +65,8 @@ func TestComment_ReplyNestsUnderItsParent(t *testing.T) { } func TestComment_EditWithinTheDeadline(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -99,6 +105,8 @@ func TestComment_EditWithinTheDeadline(t *testing.T) { const editWindow = 15 * time.Second func TestComment_EditExpiresAfterTheDeadline(t *testing.T) { + t.Parallel() + page := newPage(t) url := threadURLOn(t, shortEditURL) frame := openURL(t, page, url) @@ -135,6 +143,8 @@ func TestComment_EditExpiresAfterTheDeadline(t *testing.T) { } func TestComment_DeleteRemovesTheText(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) // deliberately not the dev user: ADMIN_SHARED_ID makes that one an admin, and the widget @@ -171,6 +181,8 @@ func TestComment_DeleteRemovesTheText(t *testing.T) { // posted with: #2040 shipped a version that handed back the rendered text, and everything the // author had written in entities or markup was lost on the next save func TestComment_EditKeepsTheOriginalSource(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -200,6 +212,8 @@ func TestComment_EditKeepsTheOriginalSource(t *testing.T) { // TestComment_DraftSurvivesReloadAndClearsAfterPost covers the local draft. A reader who reloads // mid-sentence keeps what they typed, and a reader who posts does not get it handed back func TestComment_DraftSurvivesReloadAndClearsAfterPost(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -226,16 +240,20 @@ func TestComment_DraftSurvivesReloadAndClearsAfterPost(t *testing.T) { // comment. The text is the only copy they have, so it has to stay in the form, and the failure has // to say something instead of swallowing itself func TestComment_PostFailureKeepsTheText(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) require.NoError(t, page.Route("**/api/v1/comment?**", func(route playwright.Route) { - require.NoError(t, route.Fulfill(playwright.RouteFulfillOptions{ + if err := route.Fulfill(playwright.RouteFulfillOptions{ Status: playwright.Int(http.StatusBadRequest), ContentType: playwright.String("application/json"), Body: playwright.String(`{"code":19,"details":"comment contains restricted words","error":"rejected"}`), - })) + }); err != nil { + t.Errorf("fulfill refused comment response: %v", err) + } })) text := "rejected " + runID @@ -257,6 +275,8 @@ func TestComment_PostFailureKeepsTheText(t *testing.T) { // sees. Both are server-side, so the assertions come after a reload on a second reader's page // and not from the moderator's own optimistic render func TestComment_AdminPinsAndVerifies(t *testing.T) { + t.Parallel() + text := "moderated " + runID // verification is a property of the user and outlives the run in the stack's database, so a @@ -304,6 +324,8 @@ func TestComment_AdminPinsAndVerifies(t *testing.T) { // The second half is the part a reader notices most, since a failed upload that leaves the // placeholder behind corrupts what they were writing func TestComment_ImageUploadRendersAndRecovers(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInDev(t, page, frame) @@ -325,11 +347,13 @@ func TestComment_ImageUploadRendersAndRecovers(t *testing.T) { // comes and goes inside one frame, and "the text is unchanged" would hold just as // well for an upload that never started time.Sleep(300 * time.Millisecond) - require.NoError(t, route.Fulfill(playwright.RouteFulfillOptions{ + if err := route.Fulfill(playwright.RouteFulfillOptions{ Status: playwright.Int(http.StatusInternalServerError), ContentType: playwright.String("application/json"), Body: playwright.String(`{"code":0,"details":"upload failed","error":"nope"}`), - })) + }); err != nil { + t.Errorf("fulfill failed upload response: %v", err) + } })) defer func() { require.NoError(t, page.Unroute("**/api/v1/picture**")) }() @@ -379,6 +403,8 @@ func TestComment_ImageUploadRendersAndRecovers(t *testing.T) { // answers with its own code, and the widget has to turn that into something the reader can read // instead of swallowing it, which is the half no unit test can speak for func TestComment_BlockedAuthorCannotPost(t *testing.T) { + t.Parallel() + text := "before the block " + runID // the block is permanent and the stack's database outlives the run, so a fixed name would @@ -412,6 +438,8 @@ func TestComment_BlockedAuthorCannotPost(t *testing.T) { // reader arriving afterwards has to find no way to post, and the state has to come from the // server and not from the admin's own page func TestComment_ReadOnlyThreadTakesTheFormAway(t *testing.T) { + t.Parallel() + page := newPage(t) url := threadURL(t) frame := openURL(t, page, url) @@ -425,7 +453,6 @@ func TestComment_ReadOnlyThreadTakesTheFormAway(t *testing.T) { // not openURL: it waits for a comment form, and a read-only thread is exactly the case with // no form to wait for reader := newPage(t) - pauseForAuthLimit() _, err := reader.Goto(url, playwright.PageGotoOptions{WaitUntil: playwright.WaitUntilStateDomcontentloaded}) require.NoError(t, err) diff --git a/e2e/config_test.go b/e2e/config_test.go index 7dbb4eeb8e..438dff2b2a 100644 --- a/e2e/config_test.go +++ b/e2e/config_test.go @@ -23,6 +23,8 @@ import ( // document, in templates/iframe.ejs, reads it back before the bundle runs. Nothing else opens // window.name, so the whole path could be removed unnoticed func TestConfig_ColorsReachTheWidget(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{"__colors__": map[string]any{"--color15": "rgb(1, 2, 3)"}}) @@ -45,6 +47,8 @@ func TestConfig_ColorsReachTheWidget(t *testing.T) { // conversation in two. One parameter and not two: a url containing "&" cannot be commented on // at all, see #2204 func TestConfig_URLOverrideDecidesTheThread(t *testing.T) { + t.Parallel() + shared := fmt.Sprintf("%s/web/?e2e=config-url-%s", baseURL, runID) text := "shared thread " + runID @@ -77,6 +81,8 @@ func TestConfig_URLOverrideDecidesTheThread(t *testing.T) { // common/settings.ts. The both-shown case is the control: without it these would hold on a // widget that offers neither func TestConfig_SubscriptionControlsCanBeHidden(t *testing.T) { + t.Parallel() + for _, tc := range []struct { name string config map[string]any @@ -116,6 +122,8 @@ func TestConfig_SubscriptionControlsCanBeHidden(t *testing.T) { // locale=xx is all a caller can be promised. Without this, a build that stopped resolving // catalogs altogether would still look correct to anyone reading English func TestConfig_UnknownLocaleFallsBackToEnglish(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{"locale": "xx"}) @@ -133,6 +141,8 @@ func TestConfig_UnknownLocaleFallsBackToEnglish(t *testing.T) { // this working. Kiritimati is UTC+14, far enough that a wrong timezone usually lands on the wrong // day as well as the wrong hour func TestConfig_TimesRenderInTheReadersTimezone(t *testing.T) { + t.Parallel() + const zone = "Pacific/Kiritimati" page := newPageInContext(t, browser, playwright.BrowserNewContextOptions{ @@ -187,6 +197,8 @@ func TestConfig_TimesRenderInTheReadersTimezone(t *testing.T) { // with the comment, and the backend stores it against the thread. It is what a feed and the admin // listing show, and nothing else here would notice it going func TestConfig_PageTitleReachesTheStoredComment(t *testing.T) { + t.Parallel() + title := "A title only this test uses " + runID page := newPage(t) diff --git a/e2e/crossorigin_test.go b/e2e/crossorigin_test.go index 98a1f818f4..4f1507b4e8 100644 --- a/e2e/crossorigin_test.go +++ b/e2e/crossorigin_test.go @@ -28,6 +28,8 @@ import ( // matters is the reload: the widget holds its token in memory for the life of a page, so a // sign-in that never reloads passes while persistence is broken func TestCrossOrigin_WidgetRendersOnAnotherOrigin(t *testing.T) { + t.Parallel() + thread := fmt.Sprintf("%s/post.html?e2e=%s-%s", hostSiteURL, "crossorigin", runID) text := "cross origin " + runID @@ -44,7 +46,6 @@ func TestCrossOrigin_WidgetRendersOnAnotherOrigin(t *testing.T) { page := newPage(t) stubSignedOut(t, page) - pauseForAuthLimit() _, err := page.Goto(thread) require.NoError(t, err) @@ -65,10 +66,11 @@ func TestCrossOrigin_WidgetRendersOnAnotherOrigin(t *testing.T) { // would be left with a permanently invisible widget and nothing in the page to say why, and // without the refusal ALLOWED_HOSTS would be doing nothing at all func TestCrossOrigin_DisallowedHostNeverReportsInited(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) - pauseForAuthLimit() _, err := page.Goto(fmt.Sprintf("%s/restricted.html?e2e=%s-%s", hostSiteURL, "crossorigin-blocked", runID)) require.NoError(t, err) diff --git a/e2e/deployment_test.go b/e2e/deployment_test.go index 851fd72bfb..b3ec833ca8 100644 --- a/e2e/deployment_test.go +++ b/e2e/deployment_test.go @@ -29,6 +29,8 @@ const adminEditAddress = "adminedit@example.com" // wait is the same one TestComment_EditExpiresAfterTheDeadline pays, and both halves are asserted // against it: no countdown at any point, and an edit that still lands afterwards func TestComment_AdminEditHasNoDeadline(t *testing.T) { + t.Parallel() + page := newPage(t) url := threadURLOn(t, adminEditURL) frame := openURL(t, page, url) @@ -69,6 +71,8 @@ func TestComment_AdminEditHasNoDeadline(t *testing.T) { // nothing in hand. Signing out matters as much as signing in: a token kept somewhere the sign-out // does not clear leaves a session that outlives the button func TestAuth_HeaderJWTSurvivesReload(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openURL(t, page, threadURLOn(t, jwtHeaderURL)) signInAnon(t, page, frame, anonName("headerjwt")) @@ -90,6 +94,8 @@ func TestAuth_HeaderJWTSurvivesReload(t *testing.T) { // sign-in panel offering nothing and no explanation, so the operator's own misconfiguration read // as the widget being broken func TestAuth_NoProvidersSaysSo(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openURL(t, page, threadURLOn(t, noAuthURL)) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 505a047a4e..bdff825e71 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -19,6 +19,8 @@ // - https_test.go: the widget over TLS, where the browser's protocol gates apply // - deployment_test.go: the instances whose configuration is the thing under test // - subscribe_test.go: the email subscription round trip +// - hostframe_test.go: sender checks on both sides of the iframe boundary +// - profile_test.go: the reader's own-comment overlay // - webfiles_test.go: the published /web surface // - widgets_test.go: last-comments, counter and the profile iframe package e2e @@ -30,9 +32,12 @@ import ( "fmt" "log" "net/http" + neturl "net/url" "os" "os/exec" "path/filepath" + "regexp" + "slices" "strings" "sync" "sync/atomic" @@ -113,11 +118,20 @@ var ( // the process started runID = firstNonEmpty(os.Getenv("E2E_RUN_ID"), fmt.Sprintf("%d", time.Now().UnixNano())) - authGate sync.Mutex - lastAuthCall time.Time - contextSeq atomic.Int64 + // Intercept only traffic going to a Remark42 instance. Routing every resource disables the + // browser cache and puts unrelated host-page traffic through Playwright's driver pipe, which + // distorts the iframe timing cases this suite measures. + readerIPRoutePattern = func() *regexp.Regexp { + // the dots in an address are escaped, so they cannot stand for any character + quoted := make([]string, 0, len(readerIPAuthorities)) + for _, authority := range readerIPAuthorities { + quoted = append(quoted, regexp.QuoteMeta(authority)) + } + return regexp.MustCompile(`^https?://(?:` + strings.Join(quoted, "|") + `)(?:/|$)`) + }() + // the default client has no timeout, so a port that accepts and then stalls would block // a probe well past its own deadline and leave TestMain looking hung probeClient = &http.Client{ @@ -128,24 +142,11 @@ var ( } ) -// everything under /auth/ is rate limited to 2 requests a second, and that figure is a bare -// literal at backend/app/rest/api/rest.go:242 and not a setting, so the suite has to pace -// itself: the widget calls /auth/status on every load, and again on visibilitychange or -// window focus while an oauth popup sign-in is pending. without this the limiter starts -// answering 429 and the widget renders as signed out +// Everything under /auth/ is a token bucket refilling twice a second per instance and client IP. +// Each browser context has its own forwarded IP, so one refill interval is enough between auth +// actions by the same reader. Fresh contexts need no delay because their bucket starts full. func pauseForAuthLimit() { - // 1200ms, up from the 700 this started at: the widget fires an unpaced /auth/status on - // every load and again on visibilitychange, so the paced side has to stay well under the - // 2/s cap to leave room for them. at 700 the suite manufactured its own 429s, and a lost - // probe renders as signed out, which fails whichever test happens to be signing in - const spacing = 1200 * time.Millisecond - - authGate.Lock() - defer authGate.Unlock() - if wait := spacing - time.Since(lastAuthCall); wait > 0 { - time.Sleep(wait) - } - lastAuthCall = time.Now() + time.Sleep(600 * time.Millisecond) } func TestMain(m *testing.M) { @@ -327,6 +328,8 @@ func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { // says about itself is the thing under test func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.BrowserNewContextOptions) playwright.Page { t.Helper() + seq := contextSeq.Add(1) + // the https services carry a self-signed certificate, and a context that refuses it cannot // reach them at all. harmless for the http ones opts.IgnoreHttpsErrors = playwright.Bool(true) @@ -334,6 +337,28 @@ func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.Browse ctx, err := b.NewContext(opts) require.NoError(t, err) + // Inject the reader address after the browser has made its CORS decision. Extra HTTP headers + // would turn cross-origin script loads into preflighted requests, while routing at the transport + // boundary leaves their browser-visible shape unchanged. + // RealIP deliberately rejects special-use ranges. These globally routable-form values stay + // inside the loopback-only stack and give more than enough distinct readers for one process. + readerIP := forwardedReaderIP(0, seq) + require.NoError(t, ctx.Route(readerIPRoutePattern, func(route playwright.Route) { + continueRequest := func(options ...playwright.RouteContinueOptions) { + if routeErr := route.Continue(options...); routeErr != nil && + !strings.Contains(strings.ToLower(routeErr.Error()), "target closed") { + t.Errorf("continue routed request: %v", routeErr) + } + } + if !requestNeedsReaderIP(route.Request().URL()) { + continueRequest() + return + } + headers := route.Request().Headers() + headers["X-Real-IP"] = readerIP + continueRequest(playwright.RouteContinueOptions{Headers: headers}) + })) + // the reveal timers start when the iframe element is created, so the tests that bound // them have to measure from there and not from anything this process can time require.NoError(t, ctx.AddInitScript(playwright.Script{Content: playwright.String(iframeMarkScript)})) @@ -345,7 +370,6 @@ func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.Browse // a test that opens two contexts would otherwise have them write the same file, and // cleanup runs last-in-first-out, so the surviving trace would be of the page that was // only setting the scenario up - seq := contextSeq.Add(1) t.Cleanup(func() { if tracing { if !t.Failed() { @@ -393,6 +417,52 @@ func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.Browse return page } +// forwardedReaderIP returns a globally routable-form address that stays inside the loopback-only +// stack. The second octet separates browser traffic from harness probes, while seq separates the +// limiter, vote-deduplication and anonymous-reader state of each browser context. +func forwardedReaderIP(second byte, seq int64) string { + const addressSpace = int64(256 * 254) + // Different test processes may share a kept stack. Spacing each process's first slot widely + // apart keeps their short context sequences from reusing one limiter and voter identity. + slot := (int64(os.Getpid())*7919 + seq - 1) % addressSpace + return fmt.Sprintf("8.%d.%d.%d", second, slot/254, slot%254+1) +} + +// readerIPAuthorities lists every host and port through which a browser can reach a remark42 +// instance, by name inside the compose network and over the loopback the demo pages are built on. +// Host pages and Mailpit are absent on purpose: they must keep their ordinary transport identity. +var readerIPAuthorities = []string{ + "remark42:8080", + "remark42:8084", // the dev oauth2 provider, whose port the provider fixes + "remark42-shortedit:8081", + "remark42-adminedit:8082", + "remark42-jwtheader:8083", + "remark42-noauth:8085", + "remark42-anonvote:8086", + "remark42-https:8443", + "127.0.0.1:8080", + "127.0.0.1:8081", + "127.0.0.1:8082", + "127.0.0.1:8083", + "127.0.0.1:8085", + "127.0.0.1:8086", + "127.0.0.1:8443", +} + +// requestNeedsReaderIP answers whether a request is going to a remark42 instance. The port is part +// of the answer: an address is matched whole, so a host reached on a port the stack does not serve +// is not one of ours. +func requestNeedsReaderIP(rawURL string) bool { + u, err := neturl.Parse(rawURL) + if err != nil { + return false + } + if u.Scheme != "http" && u.Scheme != "https" { + return false + } + return slices.Contains(readerIPAuthorities, u.Host) +} + // installOpts asks for the browser system libraries on CI only: install-deps shells out to // apt with sudo, which is right for a runner and wrong for someone's laptop func installOpts(browsers ...string) *playwright.RunOptions { @@ -505,7 +575,6 @@ func openThread(t *testing.T, page playwright.Page) playwright.FrameLocator { func openURL(t *testing.T, page playwright.Page, url string) playwright.FrameLocator { t.Helper() - pauseForAuthLimit() _, err := page.Goto(url, playwright.PageGotoOptions{ WaitUntil: playwright.WaitUntilStateDomcontentloaded, }) @@ -538,7 +607,6 @@ func embedConfigOn(t *testing.T, page playwright.Page, hostPage string, config m } } - pauseForAuthLimit() _, err := page.Goto(baseURL + hostPage) require.NoError(t, err) @@ -555,18 +623,18 @@ func embedConfigOn(t *testing.T, page playwright.Page, hostPage string, config m require.NoError(t, err) } -// stubSignedOut answers the widget's auth probe from the browser, for a page that never signs -// in. /auth/ is capped at two requests a second for the whole suite and the widget probes on -// every load, so a case that only needs a signed-out widget should not spend that budget: the -// tests that do sign in are the ones that cannot fake it +// stubSignedOut keeps a case whose subject is outside authentication independent of the auth +// deployment. Cases that exercise identity use the endpoint itself. func stubSignedOut(t *testing.T, page playwright.Page) { t.Helper() require.NoError(t, page.Route("**/auth/status**", func(route playwright.Route) { - require.NoError(t, route.Fulfill(playwright.RouteFulfillOptions{ + if err := route.Fulfill(playwright.RouteFulfillOptions{ Status: playwright.Int(http.StatusOK), ContentType: playwright.String("application/json"), Body: playwright.String(`{"status":"not logged in"}`), - })) + }); err != nil { + t.Errorf("fulfill signed-out auth status: %v", err) + } })) } diff --git a/e2e/embed_test.go b/e2e/embed_test.go index e40e8b9334..b6b0840171 100644 --- a/e2e/embed_test.go +++ b/e2e/embed_test.go @@ -20,6 +20,8 @@ import ( // A text node and an element, because only the element was ever adopted and a test carrying text // alone passes against the defect. func TestEmbed_PlaceholderGivesWayToOneOwnedIframe(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{}, @@ -59,6 +61,8 @@ func TestEmbed_PlaceholderGivesWayToOneOwnedIframe(t *testing.T) { // single-page app holds, documented in configuration/frontend/spa.md and exercised by nothing. // Neither half is observable from inside the widget, which is where the rest of this suite looks func TestEmbed_DestroyRemovesTheWidgetAndCreateInstanceBringsItBack(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{}) @@ -85,6 +89,8 @@ func TestEmbed_DestroyRemovesTheWidgetAndCreateInstanceBringsItBack(t *testing.T // path the demo page's own toggle takes, and the one an integrator calls when a reader switches // themes on the host page func TestEmbed_ThemeChangesReachTheWidgetAfterLoad(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{"theme": "light"}) diff --git a/e2e/geometry_test.go b/e2e/geometry_test.go index 720023ea2c..39eb0a9ed3 100644 --- a/e2e/geometry_test.go +++ b/e2e/geometry_test.go @@ -38,6 +38,8 @@ const ( // preloader is the whole difference between the working and the broken version, and not one step // in a sequence that legitimately grows as comments arrive. func TestGeometry_FirstReportedHeightIsRenderedContent(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{}) @@ -60,6 +62,8 @@ func TestGeometry_FirstReportedHeightIsRenderedContent(t *testing.T) { // embed sat inset with 24px of empty space underneath. Both halves are asserted, since the // arithmetic and the padding failed independently func TestGeometry_ReportedHeightMatchesTheDocument(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{}) @@ -87,6 +91,8 @@ func TestGeometry_ReportedHeightMatchesTheDocument(t *testing.T) { // Both modes run, because the footer-shown case is the control: it is what says the parameter // reached the widget at all instead of being quietly ignored. func TestGeometry_NoFooterKeepsTheContentInsideTheFrame(t *testing.T) { + t.Parallel() + thread := threadURL(t) poster := newPage(t) @@ -149,6 +155,8 @@ func TestGeometry_NoFooterKeepsTheContentInsideTheFrame(t *testing.T) { // the document directly. Both have to come back down again, or the widget leaves a hole in the // page for as long as the reader stays on it func TestGeometry_HeightFollowsTheAuthPanelAndTheTextarea(t *testing.T) { + t.Parallel() + page := newPage(t) stubSignedOut(t, page) embedConfig(t, page, map[string]any{}) @@ -251,6 +259,8 @@ func waitHeightNear(t *testing.T, page playwright.Page, want float64, msg string // all of them while leaving a hole in the page under every collapsed thread for as long as the // reader stays on it func TestGeometry_CollapsingAThreadShrinksTheFrame(t *testing.T) { + t.Parallel() + page := newPage(t) frame := openThread(t, page) signInAnon(t, page, frame, anonName("collapsegeometry")) diff --git a/e2e/harness_test.go b/e2e/harness_test.go index 36554f95d1..676c5f40e3 100644 --- a/e2e/harness_test.go +++ b/e2e/harness_test.go @@ -5,14 +5,17 @@ package e2e import ( "context" "fmt" + "net/http" "os/exec" "slices" "strings" "sync" + "sync/atomic" "testing" "github.com/mxschmitt/playwright-go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // stampEnv carries the source digest into compose, which sets it on the remark42 service. @@ -23,6 +26,141 @@ const stampEnv = "E2E_STAMP" // stampVar is what compose names it inside the container const stampVar = "E2E_SOURCE_STAMP" +var limiterProbeSeq atomic.Int64 + +func TestHarness_ReaderIPRequestScope(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + rawURL string + want bool + }{ + {name: "named instance", rawURL: baseURL + "/auth/status", want: true}, + {name: "dev oauth provider", rawURL: "http://remark42:8084/login/oauth/authorize", want: true}, + {name: "named deployment mode", rawURL: shortEditURL + "/auth/status", want: true}, + {name: "named tls instance", rawURL: "https://remark42-https:8443/auth/status", want: true}, + {name: "loopback instance", rawURL: probeURL + "/auth/status", want: true}, + {name: "loopback tls instance", rawURL: httpsProbeURL + "/auth/status", want: true}, + {name: "host page", rawURL: hostSiteURL + "/post.html", want: false}, + {name: "tls host page", rawURL: httpsHostSiteURL + "/post-https.html", want: false}, + {name: "mailpit", rawURL: mailpitURL + "/api/v1/messages", want: false}, + {name: "unrelated similar host", rawURL: "https://remark42.example.com/file", want: false}, + {name: "remark url in host-page query", rawURL: hostSiteURL + "/?next=" + baseURL, want: false}, + {name: "non-http scheme", rawURL: "ftp://remark42:8080/file", want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, requestNeedsReaderIP(tc.rawURL), tc.rawURL) + assert.Equal(t, tc.want, readerIPRoutePattern.MatchString(tc.rawURL), "route pattern: "+tc.rawURL) + }) + } +} + +func TestHarness_ForwardedIPSeparatesAuthLimiter(t *testing.T) { + t.Parallel() + + type result struct { + status int + err error + } + status := func(ip string) result { + req, err := http.NewRequest(http.MethodGet, probeURL+"/auth/status?site=remark", http.NoBody) + if err != nil { + return result{err: err} + } + req.Header.Set("X-Real-IP", ip) + + resp, err := probeClient.Do(req) + if err != nil { + return result{err: err} + } + if err = resp.Body.Close(); err != nil { + return result{err: err} + } + return result{status: resp.StatusCode} + } + + seq := limiterProbeSeq.Add(1) - 1 + clientIP := forwardedReaderIP(255, seq*2+1) + freshIP := forwardedReaderIP(255, seq*2+2) + + first := status(clientIP) + require.NoError(t, first.err) + require.Equal(t, http.StatusOK, first.status) + + const burst = 8 + results := make(chan result, burst) + for range burst { + go func() { results <- status(clientIP) }() + } + refused := false + for range burst { + got := <-results + require.NoError(t, got.err) + require.Contains(t, []int{http.StatusOK, http.StatusTooManyRequests}, got.status) + refused = refused || got.status == http.StatusTooManyRequests + } + require.True(t, refused, "one forwarded IP should share one auth-limiter bucket") + + // A new forwarded IP receives its own allowance, proving that the stack configuration and + // RealIP classification preserve the isolation every parallel browser context relies on. + fresh := status(freshIP) + require.NoError(t, fresh.err) + require.Equal(t, http.StatusOK, fresh.status, + "a fresh forwarded IP did not get its own bucket; check TRUSTED_PROXY and RealIP classification") +} + +func TestHarness_BrowserContextsHaveIndependentAuthLimiters(t *testing.T) { + t.Parallel() + + pages := []playwright.Page{newPage(t), newPage(t)} + for _, page := range pages { + resp, err := page.Goto(probeURL + "/web/privacy.html") + require.NoError(t, err) + require.NotNil(t, resp) + require.Equal(t, http.StatusOK, resp.Status()) + } + + type burstResult struct { + statuses []int + err error + } + burst := func(page playwright.Page) burstResult { + value, err := page.Evaluate(`url => Promise.all([fetch(url), fetch(url)]).then(rs => rs.map(r => r.status))`, + probeURL+"/auth/status?site=remark") + if err != nil { + return burstResult{err: err} + } + raw, ok := value.([]any) + if !ok { + return burstResult{err: fmt.Errorf("auth burst returned %T", value)} + } + statuses := make([]int, 0, len(raw)) + for _, item := range raw { + switch n := item.(type) { + case int: + statuses = append(statuses, n) + case float64: + statuses = append(statuses, int(n)) + default: + return burstResult{err: fmt.Errorf("auth burst status is %T", item)} + } + } + return burstResult{statuses: statuses} + } + + results := make(chan burstResult, len(pages)) + for _, page := range pages { + go func() { results <- burst(page) }() + } + for range pages { + got := <-results + require.NoError(t, got.err) + require.Equal(t, []int{http.StatusOK, http.StatusOK}, got.statuses, + "two browser contexts shared one auth bucket, so the Playwright reader-IP route is not reaching the backend") + } +} + // sourceStamp digests the sources that end up in the image. Shelling out keeps one definition of // what the digest covers, since the Makefile needs the same value and cannot call into this package func sourceStamp() (string, error) { diff --git a/e2e/hostframe_test.go b/e2e/hostframe_test.go new file mode 100644 index 0000000000..a13a2fc8fc --- /dev/null +++ b/e2e/hostframe_test.go @@ -0,0 +1,139 @@ +//go:build e2e + +package e2e + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Every frame on a page can reach window.parent, and the host page acts on what arrives: it resizes +// the widget, scrolls the document and opens the profile overlay. The embed accepts messages only +// from the frame it created, and the widget accepts messages only from its parent. +// +// These cases exercise both guards in a browser. jsdom dispatches events itself, so the sender +// identity a real browser sets is whatever the test assigns, and the layout effects the parent +// applies are not observable there. + +// TestHostFrame_ForeignFrameCannotDriveTheHostPage puts a second frame on the host page and posts +// the three messages the embed script acts on. None may take effect. +// +// The height is read from the iframe element the parent sizes, the scroll from the document, and the +// profile from the overlay the parent appends to the body, so each assertion is on the effect rather +// than on a listener's internals +func TestHostFrame_ForeignFrameCannotDriveTheHostPage(t *testing.T) { + t.Parallel() + + page := newPage(t) + frame := openThread(t, page) + signInAnon(t, page, frame, anonName("hostframeguard")) + waitHeightSettled(t, page) + + before := frameHeight(t, page) + require.Greater(t, before, preloaderCeiling, "the widget never rendered, so there is nothing to protect") + + // tall enough that the document can actually scroll, or the scrollTo assertion would pass on a + // page that simply has nowhere to go + _, err := page.Evaluate(`() => { + const filler = document.createElement('div'); + filler.style.height = '4000px'; + document.body.appendChild(filler); + window.scrollTo(0, 0); + return new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve))); + }`) + require.NoError(t, err) + + scrollBefore := evalNumber(t, page, `() => window.scrollY`) + require.Less(t, scrollBefore, float64(100), "the host page did not settle at the top before the foreign message") + + // posted from inside a frame the embed script did not create, so the event's source really is + // that frame. Calling parent.postMessage from this script instead would run in the host page's + // own realm and set source to the top window, which is a different sender and a different test. + // + // The last message is the barrier. postMessage delivery is ordered, so a probe posted after the + // three and echoed back means all four were dispatched and the three + // were ignored. A fixed wait would turn a slow runner into a pass + _, err = page.Evaluate("() => new Promise((resolve) => {\n" + + " window.addEventListener('message', function once(e) {\n" + + " if (e.data && e.data.e2eProbe) { window.removeEventListener('message', once); resolve(); }\n" + + " });\n" + + " const foreign = document.createElement('iframe');\n" + + " foreign.srcdoc = `