From 4d5dae20e2bc8ed18023ee4ae9f805c033f981ce Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 22 Aug 2026 17:59:37 +0100 Subject: [PATCH] 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 e3d1d0e2, the commit before the alias, this fails with 32 red subtests. * Cover the widget behavior the e2e suite never drove Removing npm from the widget takes the jest tests with it, and everything here was protected by jest or by nothing at all. Signing out was untested at every level, and the panel repainting is the half that always works: the assertion after the reload is the one that catches a session the server never ended. The edit form has to hand back the source that was posted and not the rendered comment, which #2040 shipped the other way round, taking every entity and tag the author had written with it. A draft has to survive a reload and be gone once the comment is posted. A refused comment has to stay in the form with something said about it, so that case drives the backend's own restricted-words code and then retries with the route removed. Uploads had no browser coverage in either direction. The posted case asserts naturalWidth instead of visibility, since a broken src still renders as an empty box, and the failing case holds the intercepted request long enough for the in-flight state to be observed: without that, "the text is unchanged afterwards" would hold for an upload that never started. Moderation and reader-side hiding are asserted from a page other than the one that made the change. Hiding seeds a second author, so it proves one person is hidden and the thread not emptied, and both the blocked author and the moderated one carry the run id in their names: a block and a verification are properties of the user and outlive the run in the stack's database, so a fixed name works exactly once. Locales were the largest hole. Each catalog is a chunk fetched at runtime, and loadLocale falls back to english on any failure instead of throwing, exactly as an unrecognized name does, so every case compares the rendered string against the file on disk. One case per catalog covers that they are all served and render; another fetches one through the widget document and asserts it parsed, which is the half a chunk that serves but fails to parse would slip through. The delete page and the last-comments stylesheet had no coverage of any kind. Two things the suite itself needed. The widget's own aria-label is translated, so commentFormSel only ever finds an english widget and a localized case cannot use widget(). And the auth probe is capped at two requests a second for the whole suite, hard-coded in rest.go: the added cases pushed past it and the suite began manufacturing its own 429s, which render as a signed-out widget and fail whichever test happens to be signing in, so the pacing gap is wider and the locale cases stub the probe they never needed. * Harden the e2e harness against silent failures and stale stacks Three things the suite could not tell you about itself. A browser failure nothing asserts on now fails the test that caused it. Uncaught exceptions are the ones worth the machinery: a widget throwing while it renders leaves most of these cases green, since they assert on elements the browser lays out either way. Rate-limit responses are recorded as well as logged for the same reason. A test driving an error path declares what it expects by substring, so a case that means to break something says which thing. The stack the suite adopts is now checked against the sources under test. Every checkout builds the image tag the compose file names, so a stack from another worktree, or from this one before an edit, answers on these ports and passes every readiness probe while serving code nobody is looking at. stamp.sh digests what goes into the image, compose passes it as the revision label, and a mismatch is refused with what to do about it. Checked after our own build too, or a stamp that never reaches the image would be a guard that silently passes everything. assertSignedIn no longer waits out the whole timeout on a refused status read. /auth/ is capped at two requests a second for the entire suite, a bare literal at rest.go:242, and a case signing in on two pages spends that twice; when the read that repaints the panel is the one the limiter turns down, the widget shows signed out over a session that exists and no later request will ask again. The short first wait now ends in a focus handoff, which the widget answers by re-probing, and only then does the real wait run. A sign-in that genuinely failed still fails, since the second read finds no state either. That is what TestComment_AdminPinsAndVerifies and TestComment_BlockedAuthorCannotPost were failing on in CI while passing locally. signInAnon takes the page for that reason, and its callers pass it. * Cover iframe geometry, the embed contract and five deployment modes Seventeen cases for the parts of the widget that broke repeatedly and that nothing here could see, plus the areas jest was the only check on. Geometry is the biggest of them. The widget measures its own document and posts the number for the parent to apply, and every way that has gone wrong is invisible to assertions about elements, which read the same whether the frame is right, twice too tall or a strip. So: the first height the parent is given describes rendered content and not the preloader, the frame matches the document it holds and does not stand 24px taller, no_footer leaves the last comment inside the frame, and the frame follows the sign-in dropdown and the growing textarea and comes back down again. Each was verified by reintroducing the defect it covers and watching it fail: a 63px report before the real one, six pixels of body padding, and a frame sized under the content. The embed surface is the other half the widget cannot see. An element placeholder gives way to exactly one iframe carrying the embed's own marker, a second createInstance reuses it, destroy takes it away with its handles, and a theme change after load reaches both the element and the document. A page that posts a message of its own, which is all embed.ts's own title observer does, no longer empties an open login form. Five configurations that cannot share an instance get one each, since each changes the widget for every reader: an admin's unlimited edit window, a session carried in a header and not a cookie, an instance with no auth provider to offer, anonymous voting, and the notify module, without which email_notifications is false and the subscribe control never renders at all. The subscription round trip is the one place a token from a real message is exchanged for state the server keeps. Two things surfaced there and are left alone, both said so in place. The panel confirming an unsubscribe cannot be observed, because the click changes the step and the dropdown closes on an element no longer in the rerendered view, which the component notes as its own awkwardness; the case asserts the request and the answer, which is what decides whether the reader still gets mail. And the widget takes the subscribed state from user.email_subscription, absent from what it hydrates the user with on the next load, so after a reload it offers to subscribe somebody who already is. simple_view needs no instance, being a query parameter, so both branches run against the main one. A transient failure of the status probe has a case too: the session belongs to the server, and one refused answer must not end it. The suite runs about four and a half minutes now, so the workflow's own budget goes to 20m to match the Makefile, well inside the job timeout, and the workflow stamps the stack it starts the way the Makefile does. The locale case that loads the widget document directly is renamed for what it protects, the origin and CSP its chunks are fetched under. Telegram gets no test: the base URL is formatted inline inside go-pkgz/auth, so nothing here can point it elsewhere, and the fix belongs upstream in v1 and v2 both. Reported as #2208. Three things CI found that a laptop cannot. The anonymous sign-in form validates its input against pattern="[\p{L}\d\s_]+", and E2E_RUN_ID is "-" on a runner, so every username built from it carried a hyphen the browser refused to submit: no request was made and the case waited out its timeout on a panel that was never going to change. Names are built by anonName now, which drops what the pattern does not allow and adds the pid, so a second run against a surviving stack does not meet its own blocked and verified users. signInAnon waits for the request the submit makes, so the next such refusal fails as itself. The admin instance takes its admin from an email address, not a name. remark42 hashes an anonymous id from the name and the client address together, to tell apart two people picking the same name, so the id written into ADMIN_SHARED_ID belonged to nobody on a runner and the instance had no admin at all: the countdown stayed, and the backend refused the edit. An email id is sha1 of the address, which is the same everywhere. The subscription case clears its own precondition and confirms through the page's session. The dev user is shared and a subscription outlives the run, so the panel opened on the subscribed step; and the panel moves to that step while its token textarea is still on screen, leaving no moment at which the control to submit it exists. Three settings of remark_config get cases of their own, none having had any: __colors__, which is the one setting that travels through window.name and not the query string, so nothing else in the suite would notice the path going; the url override, which is how a canonical address keeps one conversation across pages that differ; and the subscription controls an integrator turns off, with the both-shown case as the control. Writing the url case turned up a backend defect, reported as #2204 and not fixed here: a thread url containing "&" cannot be commented on at all. Sanitize runs the locator through SanitizeAsURL, which round-trips it through bluemonday, so the url is stored html-escaped; the bucket is created under the escaped key, the read-back uses the real one and answers 500, and every later find, count and feed asks for the real url and is told the thread is empty. Any page addressed with two query parameters is affected. The case uses a single-parameter url for that reason. Five more from the same audit, none needing a service. Collapsing a thread shrinks the frame, which every other geometry case would miss: they all assert growth, and a widget that only grew would satisfy them while leaving a hole under each collapsed thread. Voting gains the direction nothing covered, downvoting and its survival of a reload, and the rule the other vote cases work around, that your own comment offers no buttons and the backend refuses the vote anyway. A vote with the X-XSRF-TOKEN header stripped has to be refused. That check is why a document navigation, an iframe src among them, is always anonymous and why the widget hydrates its user over XHR, so anything designed around that wants it pinned. An unrecognized locale has to render English, which is loadLocale's only observable guarantee: it falls back the same way for a name it does not know and for a chunk it cannot fetch. And a comment's timestamp has to be the reader's own, which is the one part of rendering that cannot move to the server, asserted from a context in Kiritimati against the same Intl the widget uses. A host page on an origin the widget is not served from, which is the separate-domain setup the manuals describe and the configuration readers actually hit problems with. Every other host page here is served by remark42 itself, so the cross-site path was never taken: an nginx on its own name and port serves e2e/hostsite, and the case asserts the frame is revealed, which means its document loaded and reported itself inited across the origin boundary, and that the thread it renders is the one the page's address names. Signing in is left out on purpose, an embedded cookie needing SameSite=None, which browsers take only as Secure, and this stack speaks http; that is #1139 and not something a case here can settle. The other half is ALLOWED_HOSTS. The no-provider instance names only itself, so a page elsewhere embedding it is refused by the browser, the document never runs, and the reveal comes from the widget's own fallback five seconds later. Both directions are worth holding: without the fallback a mistyped host leaves a permanently invisible widget with nothing to say why, and without the refusal the setting does nothing. The host page's title reaching the stored comment gets a case, the path running the other way from everything else here: the page posts its title into the widget, the widget sends it with the comment, and it is what a feed and the admin listing show. Set after the widget is up, so it covers the observer embed.ts installs and not the value read at boot. max_shown_comments has no case. The setting reaches the widget, appears in the iframe's query string and changes nothing: four comments render with it set to two, which is #812, still open, where the reproduction is now recorded. A case for it would be red on master. The README gains what the suite cannot reach. Every service here speaks http, so anything the browser gates on the page protocol is invisible: a Secure cookie, anything keyed on window.location.protocol, and the SameSite=None with Secure and Partitioned form that is the only one an embedded frame can still use. That is not hypothetical, setAuthCookie having decorated its cookies with __Host- on any https page and survived precisely because nothing here runs on one. The cross-origin case names the assertion to add if the stack ever gets TLS, which is the reload: the widget holds its token in memory for the life of a page, so signing in and posting without reloading passes while persistence is broken. And the trap waiting for whoever acts on that: 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, so a run meaning to prove the third-party case keeps an ordinary third-party cookie exactly as it would with no flags at all. IgnoreDefaultArgs is the lever, and a blocking run has to assert a control before anything it reports can be believed. None of it reaches the widget's own storage fallback either: IS_STORAGE_AVAILABLE stays true with partitioning enforced, chromium partitioning localStorage instead of denying it, so comments.html needs webkit and not a flag. The downvote case now actually corrects. It claimed the score ends where the second vote leaves it and never cast one, so #728, a reader taking a vote back, could break with it green. It also turns out the opposite vote takes the first one back instead of flipping it, so the score returns to zero and never reaches +1, which is what the case asserts, before and after a reload. Renamed for what it covers. --- .github/workflows/e2e-tests.yml | 11 +- Makefile | 4 +- compose-e2e-test.yml | 169 ++++++++++++++++++- e2e/README.md | 49 +++++- e2e/auth_test.go | 172 +++++++++++++++++-- e2e/comment_test.go | 284 +++++++++++++++++++++++++++++++- e2e/config_test.go | 220 +++++++++++++++++++++++++ e2e/crossorigin_test.go | 99 +++++++++++ e2e/deployment_test.go | 116 +++++++++++++ e2e/e2e_test.go | 182 ++++++++++++++++++-- e2e/embed_test.go | 121 ++++++++++++++ e2e/geometry_test.go | 283 +++++++++++++++++++++++++++++++ e2e/harness_test.go | 178 ++++++++++++++++++++ e2e/hostsite/post.html | 37 +++++ e2e/hostsite/restricted.html | 34 ++++ e2e/iframe_test.go | 39 +++-- e2e/stamp.sh | 36 ++++ e2e/subscribe_test.go | 88 ++++++++++ e2e/thread_test.go | 87 +++++++++- e2e/vote_test.go | 148 ++++++++++++++++- e2e/webfiles_test.go | 266 ++++++++++++++++++++++++++++++ e2e/widgets_test.go | 155 ++++++++++++++++- 22 files changed, 2699 insertions(+), 79 deletions(-) create mode 100644 e2e/config_test.go create mode 100644 e2e/crossorigin_test.go create mode 100644 e2e/deployment_test.go create mode 100644 e2e/embed_test.go create mode 100644 e2e/geometry_test.go create mode 100644 e2e/harness_test.go create mode 100644 e2e/hostsite/post.html create mode 100644 e2e/hostsite/restricted.html create mode 100755 e2e/stamp.sh create mode 100644 e2e/subscribe_test.go create mode 100644 e2e/webfiles_test.go diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index d8af5b5b..d977a08e 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -88,8 +88,12 @@ jobs: key: playwright-${{ hashFiles('e2e/go.sum') }} restore-keys: playwright- + # E2E_STAMP is what the suite compares the running stack against, so a stack started here + # has to carry the same value `make e2e-up` and the suite itself would give it - name: Build & start the stack - run: COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait + run: | + COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 E2E_STAMP=$(./e2e/stamp.sh) \ + docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait # no retry: a failure here is evidence about a suite too young to have a flake rate, # and a rerun is how an intermittent regression becomes invisible. revisit when there @@ -99,7 +103,10 @@ jobs: # log names the run it came from env: E2E_RUN_ID: ${{ github.run_id }}-${{ github.run_attempt }} - run: cd e2e && go test -tags=e2e -count 1 -timeout 8m -v ./... + # 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 ./... - name: Server logs on failure if: failure() diff --git a/Makefile b/Makefile index 366b9073..f1b4f677 100644 --- a/Makefile +++ b/Makefile @@ -39,8 +39,10 @@ rundev: docker compose -f compose-private.yml build docker compose -f compose-private.yml up +# stamped the same way the suite stamps a stack it starts itself, so one brought up here is +# accepted instead of rejected as belonging to another checkout e2e-up: - docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait + E2E_STAMP=$$(./e2e/stamp.sh) docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait e2e-down: docker compose -f compose-e2e-test.yml down -v diff --git a/compose-e2e-test.yml b/compose-e2e-test.yml index 85ab0067..bf6493fa 100644 --- a/compose-e2e-test.yml +++ b/compose-e2e-test.yml @@ -21,7 +21,7 @@ services: # dev oauth2 provider, registered from the REMARK_URL hostname - "127.0.0.1:8084:8084" - # the hostname has to be a name rather than 127.0.0.1: the dev oauth2 server binds the + # the hostname has to be a name, not 127.0.0.1: the dev oauth2 server binds the # host it reads out of REMARK_URL, and a loopback bind inside the container cannot be # published. the suite maps the name back to 127.0.0.1 in the browser, see e2e/ environment: @@ -35,13 +35,27 @@ services: - ADMIN_SHARED_ID=dev_user # set admin flag for default user on local oauth2 - AUTH_ANON=true - AUTH_EMAIL_ENABLE=true - # the auth sender reads AUTH_EMAIL_FROM; NOTIFY_EMAIL_FROM feeds the notify module, - # which this stack does not enable + # the auth sender reads AUTH_EMAIL_FROM, the notify module reads NOTIFY_EMAIL_FROM. both + # go to mailpit, which the suite reads back over HTTP - AUTH_EMAIL_FROM=remark42@example.com + # without this email_notifications is false and the subscribe control never renders, so + # the whole subscribe, confirm and unsubscribe flow is unreachable from a browser + - NOTIFY_USERS=email + - NOTIFY_EMAIL_FROM=notify@example.com - SMTP_HOST=mailpit - SMTP_PORT=1025 # default is 0.5/sec, which the suite exceeds whenever a test posts twice in a row - UPDATE_LIMIT=100 + # digest of the sources this stack was brought up from, from e2e/stamp.sh, which the suite + # reads back and compares against the checkout it is running in. every checkout builds the + # image tag above, so a stack from another worktree answers on these ports and passes every + # readiness probe while serving code nobody is looking at. remark42 ignores it. + # + # set as the environment and not as a build argument on purpose: an argument reaching + # the backend stage is part of the go build's cache key, so every frontend edit would + # rebuild the backend too. the cost is that `docker compose up` without --build would stamp + # a stale image, which neither `make e2e-up` nor the suite does + - E2E_SOURCE_STAMP=${E2E_STAMP:-unstamped} volumes: - remark42-e2e-var:/srv/var depends_on: @@ -88,6 +102,149 @@ services: timeout: 3s retries: 30 + # ADMIN_EDIT gives admins an unlimited edit window, which #1986 reported the frontend ignoring. + # a separate instance because the setting is global and removes the countdown the ordinary + # deadline cases assert on + remark42-adminedit: + image: ghcr.io/umputun/remark42:dev + container_name: "remark42-e2e-adminedit" + pull_policy: never + depends_on: + remark42: + condition: service_started + mailpit: + condition: service_healthy + + ports: + - "127.0.0.1:8082:8080" + + environment: + - REMARK_URL=http://remark42-adminedit:8082 + - SECRET=12345 + - ADMIN_EDIT=true + # email auth, because its user id is the only one that can be written down ahead of time. + # the dev provider's port is fixed at 8084 and cannot be published twice, and the anonymous + # id is hashed from the name AND the client address (server.go:1220), which differs between + # a laptop and a runner, so an id pinned here would make an admin of nobody on CI. + # the email id is sha1 of the address alone (go-pkgz/auth provider/verify.go:92), so this is + # email_$(printf adminedit@example.com | shasum) and the test signs in as that address + - AUTH_EMAIL_ENABLE=true + - AUTH_EMAIL_FROM=remark42@example.com + - SMTP_HOST=mailpit + - SMTP_PORT=1025 + - ADMIN_SHARED_ID=email_0212c0d9ebf846e898888d617f82d4147747bdc3 + # short, so "after the deadline" is a few seconds instead of the default five minutes + - EDIT_TIME=15s + - UPDATE_LIMIT=100 + volumes: + - remark42-e2e-adminedit-var:/srv/var + healthcheck: + test: ["CMD", "curl", "--fail", "http://localhost:8080/ping"] + interval: 2s + timeout: 3s + retries: 30 + + # AUTH_SEND_JWT_HEADER moves the token out of the cookie and into a header the frontend has to + # keep for itself, which #1877 reported losing on every reload + remark42-jwtheader: + image: ghcr.io/umputun/remark42:dev + container_name: "remark42-e2e-jwtheader" + pull_policy: never + depends_on: + remark42: + condition: service_started + + ports: + - "127.0.0.1:8083:8080" + + environment: + - REMARK_URL=http://remark42-jwtheader:8083 + - SECRET=12345 + - AUTH_ANON=true + - AUTH_SEND_JWT_HEADER=true + - UPDATE_LIMIT=100 + volumes: + - remark42-e2e-jwtheader-var:/srv/var + healthcheck: + test: ["CMD", "curl", "--fail", "http://localhost:8080/ping"] + interval: 2s + timeout: 3s + retries: 30 + + # no auth provider at all, which #1456 reported rendering as a widget with nothing to say. it + # cannot be a setting on another instance: the absence is the configuration + remark42-noauth: + image: ghcr.io/umputun/remark42:dev + container_name: "remark42-e2e-noauth" + pull_policy: never + depends_on: + remark42: + condition: service_started + + ports: + - "127.0.0.1:8085:8080" + + environment: + - REMARK_URL=http://remark42-noauth:8085 + - SECRET=12345 + # its own address and nothing else, so this instance also serves as the negative case for + # embedding: the browser refuses to frame it anywhere but here, and the widget document + # never runs to report itself inited + - ALLOWED_HOSTS=http://remark42-noauth:8085 + - UPDATE_LIMIT=100 + volumes: + - remark42-e2e-noauth-var:/srv/var + healthcheck: + test: ["CMD", "curl", "--fail", "http://localhost:8080/ping"] + interval: 2s + timeout: 3s + retries: 30 + + # anonymous voting, which the default configuration refuses: rest_private.go turns down a vote + # from an anonymous user unless ANON_VOTE is set, and it only works alongside VOTES_IP, which + # is what scopes it + remark42-anonvote: + image: ghcr.io/umputun/remark42:dev + container_name: "remark42-e2e-anonvote" + pull_policy: never + depends_on: + remark42: + condition: service_started + + ports: + - "127.0.0.1:8086:8080" + + environment: + - REMARK_URL=http://remark42-anonvote:8086 + - SECRET=12345 + - AUTH_ANON=true + - ANON_VOTE=true + - VOTES_IP=true + - UPDATE_LIMIT=100 + volumes: + - remark42-e2e-anonvote-var:/srv/var + healthcheck: + test: ["CMD", "curl", "--fail", "http://localhost:8080/ping"] + interval: 2s + timeout: 3s + retries: 30 + + # a page on an origin the widget is not served from, which is the separate-domain setup the + # manuals describe and the one readers actually hit problems with. every other host page in the + # suite is served by remark42 itself, so nothing else exercises a cross-origin embed + host-site: + image: nginx:1.29-alpine + container_name: "remark42-e2e-hostsite" + ports: + - "127.0.0.1:8090:80" + volumes: + - ./e2e/hostsite:/usr/share/nginx/html:ro + healthcheck: + test: ["CMD", "wget", "-q", "-O", "/dev/null", "http://localhost/post.html"] + interval: 2s + timeout: 3s + retries: 30 + # catches the email-auth verification message; the suite reads it back over the HTTP API mailpit: image: axllent/mailpit:v1.30.7 @@ -102,8 +259,12 @@ services: timeout: 3s retries: 30 -# named rather than bind mounts, so `docker compose down -v` really does discard the +# named volumes, not bind mounts, so `docker compose down -v` really does discard the # databases and a run can start from an empty site volumes: remark42-e2e-var: remark42-e2e-shortedit-var: + remark42-e2e-adminedit-var: + remark42-e2e-jwtheader-var: + remark42-e2e-noauth-var: + remark42-e2e-anonvote-var: diff --git a/e2e/README.md b/e2e/README.md index c373ddaa..d1bddc03 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -49,21 +49,57 @@ CI does not retry a failing test. The suite is young enough that a failure is ev Beyond that: `docker compose -f compose-e2e-test.yml logs` for the server side, and mailpit's web UI on for anything email. +A failing test also logs whatever the browser wrote to its console, which is where a failed request or a widget-side error shows up. Those are context only. What does fail a test on its own is an uncaught exception in the page and a rate-limit response, both of which otherwise corrupt a run silently: a widget that throws while rendering still satisfies most assertions here, and a refused `/auth/status` renders as a signed-out reader. + +## The stack the suite runs against + +The suite refuses a running stack that was not brought up from the sources under test. Every checkout builds the image tag the compose file names, so a stack from another worktree, or from this one before an edit, answers on these ports and passes every readiness probe while serving code nobody is looking at. + +`e2e/stamp.sh` digests the content of `backend`, `frontend`, `Dockerfile` and `docker-init.sh`; compose puts it in the container's environment and the suite reads it back. It digests content and not `HEAD`, so a commit touching only the suite does not invalidate a stack. `make e2e-up` stamps the same way, so a stack started by hand is accepted. On a mismatch the failure says to run `make e2e-down`. + Before pushing, `cd e2e && go vet -tags=e2e ./...` and `golangci-lint run --build-tags=e2e --config ../backend/.golangci.yml`. CI runs both, and neither is covered by a plain `go vet ./...` because of the build tag. ## The stack -`compose-e2e-test.yml` at the repository root runs three services, each bound to the loopback interface since it holds a known secret and an admin shared id: +`compose-e2e-test.yml` at the repository root runs six services, each bound to the loopback interface since it holds a known secret and an admin shared id: - **remark42** on `:8080`, with the dev oauth2 provider on `:8084`, anonymous and email sign-in - **remark42-shortedit** on `:8081`, with `EDIT_TIME=15s` and anonymous sign-in only, since the dev oauth2 provider's port is fixed at 8084 and cannot be published twice. It exists so the expired-edit path is observable without holding a test open for the default five minutes -- **mailpit** on `:8025`, which catches the email-auth verification message for the suite to read back +- **remark42-adminedit** on `:8082`, with `ADMIN_EDIT=true` and the same short window, for the unlimited window an admin is supposed to get +- **remark42-jwtheader** on `:8083`, with `AUTH_SEND_JWT_HEADER=true`, where the token arrives in a header instead of a cookie and the frontend has to keep it itself +- **remark42-noauth** on `:8085`, with no auth provider at all, which the widget has to say something about, and with `ALLOWED_HOSTS` set to its own address so it doubles as the instance that refuses to be framed elsewhere +- **remark42-anonvote** on `:8086`, with `ANON_VOTE` and the `VOTES_IP` it depends on, since the default configuration turns an anonymous vote down +- **host-site** on `:8090`, an nginx serving `e2e/hostsite/`, which is a page on an origin the widget is not served from. Every other host page here is served by remark42 itself, so without it the separate-domain setup the manuals describe is never exercised. `post.html` embeds the main instance; `restricted.html` embeds the one whose `ALLOWED_HOSTS` names only itself, which is the refusal case +- **mailpit** on `:8025`, which catches the email-auth verification message and the subscription token for the suite to read back -Three settings exist for the tests rather than for realism, and each is there for a reason: +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 four 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. + +Three 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` rather than by a setting. See `pauseForAuthLimit`. +- 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`. + +## What this suite cannot reach + +Every service here speaks http, and nothing in it holds a certificate. Any behaviour the browser gates on the page protocol is therefore invisible: a cookie the widget writes with `Secure`, anything keyed on `window.location.protocol`, and the whole third-party cookie form of `SameSite=None; Secure; Partitioned`, which is the only one browsers still accept from an embedded frame. + +That is not hypothetical. `setAuthCookie` prefixed its cookies with `__Host-` on any https page, so a real deployment stored `__Host-JWT` while the backend looked for `JWT`; it survived because the prefix comes from the page protocol and every test and the dev server run on http. Fixed in #2197, under a second suite pinned to an https page, because this one cannot show it. + +There is a second trap waiting for whoever gives the stack TLS and then tries to prove the third-party case. 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. Measured on a cross-site https embed: + +| | ordinary third-party cookie | `Partitioned` cookie | +|---|---|---| +| Playwright defaults | kept | kept | +| partitioning left enabled | dropped | stored, with its partition key | + +So a blocking run has to assert a control before anything it reports can be believed: set an ordinary `SameSite=None` cookie from inside the widget frame and require the browser to drop it. If it survives, the run is not blocking anything. + +None of that reaches the widget's own storage fallback, which the auth panel offers as `comments.html` when `IS_THIRD_PARTY && !IS_STORAGE_AVAILABLE`. `IS_STORAGE_AVAILABLE` stays true even with partitioning properly enforced, because Chromium partitions `localStorage` instead of denying it, so the probe behind that constant never throws and the condition cannot fire. That case needs WebKit, not a Chromium flag. + +The practical consequence is for the cross-origin case in `crossorigin_test.go`, which asserts rendering and deliberately not signing in. Give the stack TLS and signing in there becomes testable, and the assertion that matters is **the reload**: the widget holds its token in memory for the life of a page, so a case that signs in and posts without reloading passes while persistence is entirely broken. ## Isolation @@ -73,9 +109,9 @@ Thread URLs deliberately keep the underscores a test name carries, since collaps ## Browsers -The `iframe_test.go` group runs in Chromium, Firefox and WebKit. It is about rendering rather than logic: the widget holds the frame hidden until its document reports itself inited, and the opaque canvas that guards against is a WebKit behaviour, so Chromium alone would not exercise it. +The `iframe_test.go` group runs in Chromium, Firefox and WebKit. It is about rendering and not logic: the widget holds the frame hidden until its document reports itself inited, and the opaque canvas that guards against is a WebKit behaviour, so Chromium alone would not exercise it. -Those tests address the server as `127.0.0.1` rather than by name, since `--host-resolver-rules` is a Chromium flag and they need no dev oauth2, which is the only reason the hostname exists. +Those tests address the server as `127.0.0.1` and not by name, since `--host-resolver-rules` is a Chromium flag and they need no dev oauth2, which is the only reason the hostname exists. Everything else runs in Chromium alone, for the same reason inverted: those tests sign in, sign-in needs the dev oauth2 provider, and reaching it by name from the host is Chromium-only. Running them in the other engines would mean putting the suite back inside the compose network. @@ -84,5 +120,6 @@ Everything else runs in Chromium alone, for the same reason inverted: those test The production bundle strips `data-testid`, so tests use what ships: the stable class hooks the widget keeps outside CSS modules (`.auth-button`, `.auth-submit`, `.comment-actions`, `.sort-picker`, `.preloader`), `title` attributes on icon-only controls, and visible text. Three shapes are worth knowing: - `.auth` only exists while signed out, so waiting on it hangs after sign-in. `widget()` waits on the comment form, which is present either way. +- The production build hashes every css-module class name to a short opaque id, so a component's own class is not something a test can hold. `role` is: the footer is `[role="contentinfo"]` and the edit countdown is `[role="timer"]`. - Comments render through an IntersectionObserver, so one below the fold is an empty `article` with no text in it. That makes any absence assertion written as a text filter pass whether the comment is gone or merely off screen; count articles instead, which is what `articleCount` is for. - Collapsing a thread hides the comment text, so a locator filtered by that text stops matching the element under test. `TestThread_CollapsePersistsAcrossReload` anchors on the comment's id instead. diff --git a/e2e/auth_test.go b/e2e/auth_test.go index c739273d..24a2326a 100644 --- a/e2e/auth_test.go +++ b/e2e/auth_test.go @@ -4,6 +4,7 @@ package e2e import ( "fmt" + "net/http" "regexp" "testing" @@ -30,11 +31,12 @@ func signInDev(t *testing.T, page playwright.Page, frame playwright.FrameLocator // frame to make it re-read auth state pauseForAuthLimit() require.NoError(t, page.Locator("#remark42 iframe").Press("Tab")) - assertSignedIn(t, frame) + assertSignedIn(t, page, frame) } -// signInAnon signs in through the anonymous provider, an in-frame form with no popup -func signInAnon(t *testing.T, frame playwright.FrameLocator, username string) { +// signInAnon signs in through the anonymous provider, an in-frame form with no popup. it takes +// the page because assertSignedIn may have to nudge it, see there +func signInAnon(t *testing.T, page playwright.Page, frame playwright.FrameLocator, username string) { t.Helper() pauseForAuthLimit() @@ -48,9 +50,17 @@ func signInAnon(t *testing.T, frame playwright.FrameLocator, username string) { require.NoError(t, tab.Click()) } require.NoError(t, frame.Locator(".auth-input-username").Fill(username)) - require.NoError(t, frame.Locator(".auth-submit").Click()) - assertSignedIn(t, frame) + // wait for the request the submit is supposed to make, so a form the browser refuses to + // submit fails here naming that, and not fifteen seconds later on a panel that was never + // going to change. the input is validated against a pattern, see anonName + _, err := page.ExpectResponse("**/auth/anonymous/login**", func() error { + return frame.Locator(".auth-submit").Click() + }, playwright.PageExpectResponseOptions{Timeout: playwright.Float(float64(waitTimeout.Milliseconds()))}) + require.NoError(t, err, "the anonymous form was never submitted, most likely because %q is not "+ + "a username it accepts", username) + + assertSignedIn(t, page, frame) } func TestAuth_DevProviderSignsIn(t *testing.T) { @@ -58,15 +68,15 @@ func TestAuth_DevProviderSignsIn(t *testing.T) { frame := openThread(t, page) signInDev(t, page, frame) - assertSignedIn(t, frame) + assertSignedIn(t, page, frame) } func TestAuth_AnonymousSignsIn(t *testing.T) { page := newPage(t) frame := openThread(t, page) - signInAnon(t, frame, "anontester") - assertSignedIn(t, frame) + signInAnon(t, page, frame, "anontester") + assertSignedIn(t, page, frame) name, err := frame.Locator(`[title="Open My Profile"]`).InnerText() require.NoError(t, err) @@ -75,21 +85,32 @@ func TestAuth_AnonymousSignsIn(t *testing.T) { // TestAuth_EmailSignsIn drives the full email flow: request a code, read it back out of the // mail catcher, and submit it. The token is what the widget sends, so a broken template or a -// broken token round-trip fails here rather than silently in production. +// broken token round-trip fails here and not silently in production. func TestAuth_EmailSignsIn(t *testing.T) { page := newPage(t) frame := openThread(t, page) // the mailbox is per-run: mailpit keeps everything, and with the stack left up between // runs a fixed address would let this test read a previous run's token - address := fmt.Sprintf("email-tester-%s@example.com", runID) + signInEmail(t, page, frame, "emailtester", fmt.Sprintf("email-tester-%s@example.com", runID)) +} + +// signInEmail completes the email flow: ask for a code, read it out of the mail catcher, submit +// it. The address decides the user id, sha1 of it, so a case needing a known id picks the address +func signInEmail(t *testing.T, page playwright.Page, frame playwright.FrameLocator, username, address string) { + t.Helper() pauseForAuthLimit() require.NoError(t, frame.Locator(".auth-button").Click()) waitVisible(t, frame.Locator(".auth-dropdown")) - require.NoError(t, frame.Locator(`label[for="form-provider-email"]`).Click()) - require.NoError(t, frame.Locator(".auth-input-username").Fill("emailtester")) + // only rendered when more than one form provider is enabled; with email alone the form is + // shown directly + tab := frame.Locator(`label[for="form-provider-email"]`) + if n, err := tab.Count(); err == nil && n > 0 { + require.NoError(t, tab.Click()) + } + require.NoError(t, frame.Locator(".auth-input-username").Fill(username)) require.NoError(t, frame.Locator(".auth-input-email").Fill(address)) require.NoError(t, frame.Locator(".auth-submit").Click()) @@ -99,16 +120,35 @@ func TestAuth_EmailSignsIn(t *testing.T) { require.NoError(t, frame.Locator(".auth-token-textarea").Fill(token)) require.NoError(t, frame.Locator(".auth-submit").Click()) - assertSignedIn(t, frame) + assertSignedIn(t, page, frame) } // assertSignedIn checks the panel has swapped Sign In for the signed-in user's own controls. -// Sign Out is an icon button, so its title is the only text it carries -func assertSignedIn(t *testing.T, frame playwright.FrameLocator) { +// 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 +func assertSignedIn(t *testing.T, page playwright.Page, frame playwright.FrameLocator) { t.Helper() - waitVisible(t, frame.Locator(`[title="Sign Out"]`)) + + signOut := frame.Locator(`[title="Sign Out"]`) + if err := signOut.WaitFor(playwright.LocatorWaitForOptions{ + State: playwright.WaitForSelectorStateVisible, + Timeout: playwright.Float(float64(authRepaintWait.Milliseconds())), + }); err != nil { + pauseForAuthLimit() + require.NoError(t, page.Locator("#remark42 iframe").Press("Tab")) + } + + waitVisible(t, signOut) waitVisible(t, frame.Locator(`[title="Open My Profile"]`)) - waitHidden(t, frame.Locator(".auth-button")) + waitHidden(t, frame.Locator(".auth-button"), "the panel still offers sign-in after a sign-in") } // verificationToken pulls the JWT out of the confirmation mail @@ -118,3 +158,101 @@ func verificationToken(t *testing.T, body string) string { require.NotEmpty(t, m, "no token in message body:\n%s", body) return m } + +// TestAuth_SignOutEndsTheSession covers the one half of authentication nothing tested at any +// level: that signing out actually ends the session instead of only repainting the panel. The +// 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) { + page := newPage(t) + frame := openThread(t, page) + + signInDev(t, page, frame) + + pauseForAuthLimit() + require.NoError(t, frame.Locator(`[title="Sign Out"]`).Click()) + waitVisible(t, frame.Locator(".auth-button")) + waitHidden(t, frame.Locator(`[title="Sign Out"]`)) + + frame = reload(t, page) + waitVisible(t, frame.Locator(".auth-button")) + waitHidden(t, frame.Locator(`[title="Sign Out"]`), + "the panel signed out but the session survived the reload, so the cookie was never cleared") +} + +// TestAuth_HostPageMessageDoesNotCloseTheDropdown covers #2139. Every message reaching the widget +// closed the sign-in dropdown, because the handler returned early only for a clickOutside payload +// and fell through to closing in every other case. embed.ts watches the host page's title element +// and posts on every mutation, so a page that updates its own title discarded whatever the reader +// had typed into the login form. Browser extensions posting into the page did the same, which is +// what #1761 reports. +// +// The theme change at the end is the synchronization, not a second assertion: postMessage is +// 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) { + page := newPage(t) + stubSignedOut(t, page) + embedConfig(t, page, map[string]any{"theme": "light"}) + frame := widget(t, page) + + require.NoError(t, frame.Locator(".auth-button").Click()) + waitVisible(t, frame.Locator(".auth-dropdown")) + + require.NoError(t, frame.Locator(`label[for="form-provider-email"]`).Click()) + const typed = "half-written name" + require.NoError(t, frame.Locator(".auth-input-username").Fill(typed)) + + // an ordinary thing for a host page to do, and all it took + _, err := page.Evaluate(`() => { document.title = 'the host page renamed itself'; }`) + require.NoError(t, err) + + _, err = page.Evaluate(`() => window.REMARK42.changeTheme('dark')`) + require.NoError(t, err) + eventually(t, waitTimeout, "the widget never acted on the message sent after the title change", func() bool { + return widgetColorScheme(t, page) == "dark" + }) + + waitVisible(t, frame.Locator(".auth-dropdown")) + value, err := frame.Locator(".auth-input-username").InputValue() + require.NoError(t, err) + assert.Equal(t, typed, value, "the host page's own message emptied the login form") + + // and the message that is supposed to close it still does, or the fix above would be a + // dropdown that never closes + require.NoError(t, page.Mouse().Click(5, 5)) + waitHidden(t, frame.Locator(".auth-dropdown"), "a genuine click outside the widget no longer closes the dropdown") +} + +// TestAuth_SessionSurvivesATransientStatusFailure covers the probe #1763 introduced. The widget +// asks /auth/status on every load, and a single failed answer must not be taken for a signed-out +// reader in any lasting way: the session lives in a cookie the server issued, and one bad +// 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) { + page := newPage(t) + frame := openThread(t, page) + signInDev(t, page, frame) + + // 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{ + Status: playwright.Int(http.StatusInternalServerError), + ContentType: playwright.String("application/json"), + Body: playwright.String(`{"error":"failed"}`), + }) + })) + + pauseForAuthLimit() + _, err := page.Reload() + require.NoError(t, err) + widget(t, page) + + require.NoError(t, page.Unroute("**/auth/status**")) + + // the session has to be there again once the endpoint is, which is the whole claim: a + // transient failure cost the reader nothing + frame = reload(t, page) + assertSignedIn(t, page, frame) +} diff --git a/e2e/comment_test.go b/e2e/comment_test.go index 725464cf..1cfe1edc 100644 --- a/e2e/comment_test.go +++ b/e2e/comment_test.go @@ -3,8 +3,12 @@ package e2e import ( + "encoding/base64" "fmt" + "net/http" neturl "net/url" + "os" + "path/filepath" "strings" "testing" "time" @@ -72,7 +76,7 @@ func TestComment_EditWithinTheDeadline(t *testing.T) { submitForm(t, replyForm(t, frame), edited) waitVisible(t, comment(frame, edited)) - // an edit replaces the comment rather than adding one, and counting is the only sound + // an edit replaces the comment instead of adding one, and counting is the only sound // way to say the old text is gone: a text filter cannot tell absent from off screen assert.Equal(t, 1, articleCount(t, frame), "editing should not add a comment") txt, err := frame.Locator("article").First().InnerText() @@ -98,7 +102,7 @@ func TestComment_EditExpiresAfterTheDeadline(t *testing.T) { page := newPage(t) url := threadURLOn(t, shortEditURL) frame := openURL(t, page, url) - signInAnon(t, frame, "expirytester") + signInAnon(t, page, frame, "expirytester") text := "expires " + runID postComment(t, frame, text) @@ -136,7 +140,7 @@ func TestComment_DeleteRemovesTheText(t *testing.T) { // deliberately not the dev user: ADMIN_SHARED_ID makes that one an admin, and the widget // sends admins to the admin endpoint, so signing in there would leave the path every // ordinary reader takes untested - signInAnon(t, frame, "deletetester") + signInAnon(t, page, frame, "deletetester") text := "doomed " + runID survivor := "survivor " + runID @@ -154,10 +158,282 @@ func TestComment_DeleteRemovesTheText(t *testing.T) { // scrolled out of view would also satisfy waitVisible(t, comment(frame, "This comment was deleted")) - // and it stays gone rather than reappearing from cache on the next load. the survivor is + // and it stays gone instead of reappearing from cache on the next load. the survivor is // what makes this assertion mean anything: without it a thread that had not rendered yet // would satisfy "the deleted text is absent" just as well frame = reload(t, page) waitVisible(t, comment(frame, survivor)) assert.Equal(t, 1, articleCount(t, frame), "the deleted comment should be gone from the thread") } + +// TestComment_EditKeepsTheOriginalSource covers what the widget puts back in the textarea when a +// comment is edited. The thread shows rendered html, so the form has to hold the source it was +// 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) { + page := newPage(t) + frame := openThread(t, page) + signInDev(t, page, frame) + + // entities, markup and a character outside latin1, each of which a render-and-read-back + // round trip mangles differently + source := "5 < 10 & **bold** tag ю " + runID + postCommentMatching(t, frame, source, runID) + + require.NoError(t, actions(frame, runID).Locator(`button:has-text("Edit")`).Click()) + form := replyForm(t, frame) + + got, err := form.Locator("textarea").InputValue() + require.NoError(t, err) + assert.Equal(t, source, got, "the edit form has to hold the source that was posted, not the rendered comment") + + submitForm(t, form, source+" edited") + waitVisible(t, comment(frame, "edited")) + + frame = reload(t, page) + require.NoError(t, actions(frame, runID).Locator(`button:has-text("Edit")`).Click()) + got, err = replyForm(t, frame).Locator("textarea").InputValue() + require.NoError(t, err) + assert.Equal(t, source+" edited", got, "the stored source has to survive the round trip through the backend") +} + +// 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) { + page := newPage(t) + frame := openThread(t, page) + signInDev(t, page, frame) + + draft := "half written " + runID + require.NoError(t, frame.Locator(commentFormSel).First().Locator("textarea").Fill(draft)) + + frame = reload(t, page) + textarea := frame.Locator(commentFormSel).First().Locator("textarea") + eventually(t, waitTimeout, "the draft was not restored after the reload", func() bool { + v, err := textarea.InputValue() + return err == nil && v == draft + }) + + postCommentMatching(t, frame, draft, draft) + + frame = reload(t, page) + got, err := frame.Locator(commentFormSel).First().Locator("textarea").InputValue() + require.NoError(t, err) + assert.Empty(t, got, "a posted draft has to be cleared, or the reader is handed their own comment back") +} + +// TestComment_PostFailureKeepsTheText covers the path a reader hits when the server refuses the +// 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) { + 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{ + Status: playwright.Int(http.StatusBadRequest), + ContentType: playwright.String("application/json"), + Body: playwright.String(`{"code":19,"details":"comment contains restricted words","error":"rejected"}`), + })) + })) + + text := "rejected " + runID + form := frame.Locator(commentFormSel).First() + submitForm(t, form, text) + + waitVisible(t, form.Locator(`p[role="alert"]`)) + + got, err := form.Locator("textarea").InputValue() + require.NoError(t, err) + assert.Equal(t, text, got, "a refused comment has to stay in the form, it is the only copy the reader has") + + require.NoError(t, page.Unroute("**/api/v1/comment?**")) + require.NoError(t, form.Locator(`button[type="submit"]`).Click()) + waitVisible(t, comment(frame, text)) +} + +// TestComment_AdminPinsAndVerifies covers two moderator actions that change what every reader +// 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) { + text := "moderated " + runID + + // verification is a property of the user and outlives the run in the stack's database, so a + // fixed name is only verifiable once: the next run would toggle an already verified author + // off and wait for a badge that is being taken away + author := newPage(t) + authorFrame := openThread(t, author) + signInAnon(t, author, authorFrame, anonName("moderated")) + postComment(t, authorFrame, text) + + admin := newPage(t) + adminFrame := openURL(t, admin, threadURL(t)) + signInDev(t, admin, adminFrame) + + admin.OnDialog(func(d playwright.Dialog) { _ = d.Accept() }) + require.NoError(t, actions(adminFrame, text).Locator(`button:has-text("Pin")`).Click()) + // pinning re-renders the thread, and a click that lands during that render is lost, so wait + // for the pinned region to exist before touching the same comment again + waitVisible(t, adminFrame.Locator(`[role="region"][aria-label="Pinned comments"]`)) + + // the verification toggle sits in the comment header beside the author, not in the action bar + require.NoError(t, comment(adminFrame, text).Locator(`[title="Toggle verification"]`).First().Click()) + waitVisible(t, comment(adminFrame, text).Locator(`[title="Verified user"]`).First()) + + reader := newPage(t) + readerFrame := openURL(t, reader, threadURL(t)) + + pinned := readerFrame.Locator(`[role="region"][aria-label="Pinned comments"]`) + waitVisible(t, pinned) + waitVisible(t, pinned.Locator("article", playwright.LocatorLocatorOptions{HasText: text})) + waitVisible(t, comment(readerFrame, text).Locator(`[title="Verified user"]`).First()) + + // unpinning has to reach every reader too, so the region goes away instead of merely + // emptying on the moderator's own page + require.NoError(t, actions(adminFrame, text).Locator(`button:has-text("Unpin")`).Click()) + + readerFrame = reload(t, reader) + waitHidden(t, readerFrame.Locator(`[role="region"][aria-label="Pinned comments"]`), + "the comment was unpinned but readers still see the pinned region") +} + +// TestComment_ImageUploadRendersAndRecovers covers the upload path end to end, which nothing +// exercised in a browser: the file input, the temporary markdown the form writes while the request +// is in flight, the final picture URL, and the image actually loading in the posted comment. +// 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) { + page := newPage(t) + frame := openThread(t, page) + signInDev(t, page, frame) + + // a 1x1 png, written out inline so the case does not depend on a fixture file + png, err := base64.StdEncoding.DecodeString( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==") + require.NoError(t, err) + + path := filepath.Join(t.TempDir(), "pixel.png") + require.NoError(t, os.WriteFile(path, png, 0o600)) + + form := frame.Locator(commentFormSel).First() + textarea := form.Locator("textarea") + + t.Run("a failed upload leaves the text as it was", func(t *testing.T) { + require.NoError(t, page.Route("**/api/v1/picture**", func(route playwright.Route) { + // held briefly so the in-flight state is observable: without it the placeholder + // 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{ + Status: playwright.Int(http.StatusInternalServerError), + ContentType: playwright.String("application/json"), + Body: playwright.String(`{"code":0,"details":"upload failed","error":"nope"}`), + })) + })) + defer func() { require.NoError(t, page.Unroute("**/api/v1/picture**")) }() + + written := "before the upload " + runID + require.NoError(t, textarea.Fill(written)) + require.NoError(t, form.Locator(`input[type="file"]`).SetInputFiles(path)) + + eventually(t, waitTimeout, "the form never showed the upload in progress", func() bool { + v, verr := textarea.InputValue() + return verr == nil && v != written + }) + + waitVisible(t, form.Locator(`p[role="alert"]`)) + eventually(t, waitTimeout, "the upload placeholder was left in the text after the failure", func() bool { + v, verr := textarea.InputValue() + return verr == nil && v == written + }) + }) + + t.Run("an uploaded image is posted and renders", func(t *testing.T) { + require.NoError(t, textarea.Fill("with an image "+runID+" ")) + require.NoError(t, form.Locator(`input[type="file"]`).SetInputFiles(path)) + + eventually(t, waitTimeout, "the upload never produced a picture url", func() bool { + v, verr := textarea.InputValue() + return verr == nil && strings.Contains(v, "/api/v1/picture/") + }) + + require.NoError(t, form.Locator(`button[type="submit"]`).Click()) + posted := comment(frame, "with an image "+runID) + waitVisible(t, posted) + + img := posted.Locator(`img[src*="/api/v1/picture/"]`).First() + waitVisible(t, img) + + // visible is not loaded: a broken src renders as an empty box, and naturalWidth is the + // only thing that says the bytes came back + eventually(t, waitTimeout, "the posted image never loaded", func() bool { + w, jerr := img.Evaluate("el => el.naturalWidth", nil) + n, ok := w.(int) + return jerr == nil && ok && n > 0 + }) + }) +} + +// TestComment_BlockedAuthorCannotPost covers the refusal a blocked author meets. The backend +// 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) { + text := "before the block " + runID + + // the block is permanent and the stack's database outlives the run, so a fixed name would + // only be postable once: every later run would find the author already blocked + author := newPage(t) + authorFrame := openThread(t, author) + signInAnon(t, author, authorFrame, anonName("blocked")) + postComment(t, authorFrame, text) + + admin := newPage(t) + adminFrame := openURL(t, admin, threadURL(t)) + signInDev(t, admin, adminFrame) + + admin.OnDialog(func(d playwright.Dialog) { _ = d.Accept() }) + _, err := actions(adminFrame, text).Locator("select").SelectOption(playwright.SelectOptionValues{ + Values: &[]string{"permanently"}, + }) + require.NoError(t, err) + + // the author's own page still believes it can post, which is the point: the refusal has to + // come back from the server and be shown + form := authorFrame.Locator(commentFormSel).First() + submitForm(t, form, "after the block "+runID) + + // not scoped to the form: the widget re-renders the whole panel once the server reports the + // author as blocked, so where the message lands is not the point, only that it is said + waitVisible(t, authorFrame.Locator("text=blocked").First()) +} + +// TestComment_ReadOnlyThreadTakesTheFormAway covers the admin switch that closes a thread. A +// 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) { + page := newPage(t) + url := threadURL(t) + frame := openURL(t, page, url) + signInDev(t, page, frame) + + // the admin panel swaps its own button instead of showing the read-only notice, which is + // what an ordinary reader gets + require.NoError(t, frame.Locator(`button:has-text("Disable comments")`).Click()) + waitVisible(t, frame.Locator(`button:has-text("Enable comments")`)) + + // 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) + + readerFrame := reader.FrameLocator("#remark42 iframe") + waitVisible(t, readerFrame.Locator(`text=Read-only`)) + waitHidden(t, readerFrame.Locator(commentFormSel).First(), + "the thread is read-only but a reader is still shown a comment form") + + require.NoError(t, frame.Locator(`button:has-text("Enable comments")`).Click()) + waitVisible(t, frame.Locator(commentFormSel).First()) +} diff --git a/e2e/config_test.go b/e2e/config_test.go new file mode 100644 index 00000000..7dbb4eeb --- /dev/null +++ b/e2e/config_test.go @@ -0,0 +1,220 @@ +//go:build e2e + +package e2e + +import ( + "encoding/json" + "fmt" + "net/http" + neturl "net/url" + "testing" + + "github.com/mxschmitt/playwright-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// remark_config is the widget's public surface: whatever an integrator sets there has to keep +// working, and discussion #1714 asks for it to be written down. None of it was covered, so a +// setting could stop reaching the widget without anything here going red. + +// TestConfig_ColorsReachTheWidget covers __colors__, the only setting that does not travel in the +// iframe's query string: the parent puts it in window.name and an inline script in the widget +// 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) { + page := newPage(t) + stubSignedOut(t, page) + embedConfig(t, page, map[string]any{"__colors__": map[string]any{"--color15": "rgb(1, 2, 3)"}}) + widget(t, page) + + value, err := page.FrameLocator("#remark42 iframe").Locator(":root").Evaluate( + `(el) => getComputedStyle(el).getPropertyValue('--color15').trim()`, nil, + playwright.LocatorEvaluateOptions{Timeout: playwright.Float(float64(waitTimeout.Milliseconds()))}) + require.NoError(t, err) + assert.Equal(t, "rgb(1, 2, 3)", value, "the colors an integrator sets never reached the widget document") +} + +// TestConfig_URLOverrideDecidesTheThread covers remark_config.url, which is how a canonical +// address keeps one conversation across pages that differ: a print view, a path with tracking +// parameters, a page that moved. Two host pages at different addresses name the same thread here, +// and the second has to see what the first posted. +// +// The override carries a query string on purpose, since createInstance strips the fragment and +// nothing else, so the query is part of the thread's identity and dropping it would split the +// 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) { + shared := fmt.Sprintf("%s/web/?e2e=config-url-%s", baseURL, runID) + text := "shared thread " + runID + + page := newPage(t) + embedConfigOn(t, page, "/web/privacy.html", map[string]any{"url": shared}) + frame := widget(t, page) + signInAnon(t, page, frame, anonName("urloverride")) + postComment(t, frame, text) + + // a different host page, same override + other := newPage(t) + stubSignedOut(t, other) + embedConfigOn(t, other, "/web/markdown-help.html", map[string]any{"url": shared}) + otherFrame := widget(t, other) + // markdown-help.html is long enough to leave the widget below the fold, and comments render + // through an IntersectionObserver, so one that has not been reached is an empty article no + // text filter matches + require.NoError(t, other.Locator("#remark42 iframe").ScrollIntoViewIfNeeded()) + waitVisible(t, comment(otherFrame, text)) + + // and the query string reached the widget, which is what makes this a test of the override + // and not of two pages agreeing on a default + src, err := other.Locator("#remark42 iframe").GetAttribute("src") + require.NoError(t, err) + assert.Contains(t, src, "e2e%3Dconfig-url", "the query string was dropped from the thread url") +} + +// TestConfig_SubscriptionControlsCanBeHidden covers show_rss_subscription and +// show_email_subscription, which an integrator turns off to keep the form to itself, read in +// 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) { + for _, tc := range []struct { + name string + config map[string]any + rss, byMail bool + }{ + {"both shown", map[string]any{}, true, true}, + {"rss hidden", map[string]any{"show_rss_subscription": false}, false, true}, + {"email hidden", map[string]any{"show_email_subscription": false}, true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + page := newPage(t) + embedConfig(t, page, tc.config) + frame := widget(t, page) + // the email control is offered to a registered reader only, which anonymous is not + signInDev(t, page, frame) + + rss := frame.Locator(`[title="Subscribe by RSS"]`) + byMail := frame.Locator(`[title="Subscribe by Email"]`) + + if tc.rss { + waitVisible(t, rss) + } else { + waitHidden(t, rss, "show_rss_subscription=false left the RSS control in place") + } + if tc.byMail { + waitVisible(t, byMail) + } else { + waitHidden(t, byMail, "show_email_subscription=false left the email control in place") + } + }) + } +} + +// TestConfig_UnknownLocaleFallsBackToEnglish covers loadLocale's only observable guarantee. It +// returns the English defaults both for a name it does not recognize and for a chunk it fails to +// fetch, and the two are indistinguishable from outside, so a widget rendering English for +// 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) { + page := newPage(t) + stubSignedOut(t, page) + embedConfig(t, page, map[string]any{"locale": "xx"}) + + frame := widget(t, page) + placeholder, err := frame.Locator(commentFormSel).First().Locator("textarea").GetAttribute("placeholder") + require.NoError(t, err) + assert.Equal(t, "Your comment here", placeholder, + "an unrecognized locale did not fall back to the english defaults") +} + +// TestConfig_TimesRenderInTheReadersTimezone covers the one part of a comment's rendering that +// cannot move to the server. The locale is configured and the server knows it, but the timezone +// belongs to the reader's machine, so a design that renders timestamps anywhere else has to keep +// 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) { + const zone = "Pacific/Kiritimati" + + page := newPageInContext(t, browser, playwright.BrowserNewContextOptions{ + TimezoneId: playwright.String(zone), + }) + frame := openThread(t, page) + signInAnon(t, page, frame, anonName("timezone")) + + text := "timezone " + runID + posted := postCommentMatching(t, frame, text, text) + + shown, err := posted.Locator(`a[href*="#remark42__comment-"]`).First().InnerText() + require.NoError(t, err) + + // the server's own timestamp for that comment, so the comparison is against what was stored + // and not against the clock this process reads + status, body := pageFetch(t, page, "GET", + fmt.Sprintf("%s/api/v1/find?site=remark&url=%s&format=tree", baseURL, neturl.QueryEscape(threadURL(t))), nil) + require.Equal(t, http.StatusOK, status, "could not read the thread back: %s", body) + + var thread struct { + Comments []struct { + Comment struct { + Time string `json:"time"` + } `json:"comment"` + } `json:"comments"` + } + require.NoError(t, json.Unmarshal([]byte(body), &thread)) + require.NotEmpty(t, thread.Comments, "the thread came back empty") + + // formatted in the page, by the same Intl the widget uses, so this asserts the timezone + // reached the rendering and not that two libraries agree on a format + want, err := page.Evaluate(`([iso, zone]) => { + const d = new Date(iso); + const day = new Intl.DateTimeFormat('en', {timeZone: zone}).format(d); + const time = new Intl.DateTimeFormat('en', {timeZone: zone, hour: 'numeric', minute: 'numeric'}).format(d); + return {day, time}; + }`, []any{thread.Comments[0].Comment.Time, zone}) + require.NoError(t, err) + + parts, ok := want.(map[string]any) + require.True(t, ok, "unexpected shape from the page: %#v", want) + day, _ := parts["day"].(string) + require.NotEmpty(t, day) + + assert.Contains(t, shown, day, + "the comment is dated %q, which is not the reader's own day in %s", shown, zone) +} + +// TestConfig_PageTitleReachesTheStoredComment covers the title path, which runs the other way +// from everything else here: the host page's title is posted into the widget, the widget sends it +// 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) { + title := "A title only this test uses " + runID + + page := newPage(t) + embedConfig(t, page, map[string]any{}) + frame := widget(t, page) + + // set after the widget is up, so this covers the observer embed.ts installs on the title + // element and not merely the value read once at boot + _, err := page.Evaluate(`(t) => { document.title = t; }`, title) + require.NoError(t, err) + + signInAnon(t, page, frame, anonName("pagetitle")) + text := "titled " + runID + postComment(t, frame, text) + + status, body := pageFetch(t, page, "GET", + fmt.Sprintf("%s/api/v1/find?site=remark&url=%s&format=tree", baseURL, neturl.QueryEscape(threadURL(t))), nil) + require.Equal(t, http.StatusOK, status, "could not read the thread back: %s", body) + + var thread struct { + Comments []struct { + Comment struct { + Title string `json:"title"` + } `json:"comment"` + } `json:"comments"` + } + require.NoError(t, json.Unmarshal([]byte(body), &thread)) + require.NotEmpty(t, thread.Comments) + assert.Equal(t, title, thread.Comments[0].Comment.Title, + "the host page's title never reached the stored comment") +} diff --git a/e2e/crossorigin_test.go b/e2e/crossorigin_test.go new file mode 100644 index 00000000..ec161aef --- /dev/null +++ b/e2e/crossorigin_test.go @@ -0,0 +1,99 @@ +//go:build e2e + +package e2e + +import ( + "fmt" + "testing" + "time" + + "github.com/mxschmitt/playwright-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCrossOrigin_WidgetRendersOnAnotherOrigin covers the separate-domain setup the manuals +// describe, which is the configuration readers actually hit problems with and which nothing else +// here reaches: every other host page in this suite is served by remark42 itself, so the widget +// and its embedder always share an origin and the cross-site path is never taken. +// +// The host page is served by its own nginx on a different name and port, and the widget it embeds +// addresses remark42 by remark_config.host. What has to hold is that the frame is revealed at all, +// which means its document loaded and reported itself inited through postMessage across origins, +// and that the thread it renders is the one the page's own address names. +// +// Signing in is deliberately not asserted. An embedded cookie needs SameSite=None, which browsers +// only accept as Secure, and this stack speaks http, so the form cannot be delivered here at all; +// see "What this suite cannot reach" in the README. Give the stack TLS and the case to add is +// signing in and then reloading, since the widget holds its token in memory for the life of a +// page and a sign-in that never reloads passes while persistence is broken +func TestCrossOrigin_WidgetRendersOnAnotherOrigin(t *testing.T) { + thread := fmt.Sprintf("%s/post.html?e2e=%s-%s", hostSiteURL, "crossorigin", runID) + text := "cross origin " + runID + + // seeded from a page on remark42's own origin, since posting needs a session and this case is + // about rendering, not about carrying a cookie across origins + seeder := newPage(t) + seederFrame := openThread(t, seeder) + signInAnon(t, seeder, seederFrame, anonName("crossorigin")) + status, body := pageFetch(t, seeder, "POST", baseURL+"/api/v1/comment?site=remark", map[string]any{ + "text": text, + "locator": map[string]string{"site": "remark", "url": thread}, + }) + require.Equal(t, 201, status, "could not seed the cross-origin thread: %s", body) + + page := newPage(t) + stubSignedOut(t, page) + pauseForAuthLimit() + _, err := page.Goto(thread) + require.NoError(t, err) + + frame := widget(t, page) + + // revealed, so the document loaded and its inited message crossed the origin boundary. a + // frame that never reported would still be here, hidden, until the fallback timer + waitVisible(t, page.Locator("#remark42 iframe")) + waitVisible(t, comment(frame, text)) +} + +// TestCrossOrigin_DisallowedHostNeverReportsInited is the other half of ALLOWED_HOSTS. An +// operator sets it so their comments cannot be framed by anyone else, and what the reader on such +// a page gets is decided entirely by the widget's own fallback: the browser refuses to load the +// document, nothing ever posts inited, and the frame is revealed five seconds later by the timer. +// +// Worth pinning in both directions. Without the fallback an integrator who mistyped the host +// 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) { + 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) + + require.NoError(t, page.Locator("#remark42 iframe").WaitFor(playwright.LocatorWaitForOptions{ + State: playwright.WaitForSelectorStateAttached, + Timeout: playwright.Float(float64(waitTimeout.Milliseconds())), + })) + + // hidden for as long as the fallback runs, since the document the browser refused cannot + // report anything + require.Equal(t, "hidden", iframeVisibility(t, page)) + + eventually(t, revealTimeout*2, "the frame was never revealed, so a mistyped host would stay invisible", func() bool { + _, ok := revealDelay(t, page) + return ok + }) + + delay, ok := revealDelay(t, page) + require.True(t, ok) + assert.Greater(t, delay, revealTimeout-500*time.Millisecond, + "the frame was revealed too early to have been the fallback, so something reported inited "+ + "from a document the browser should not have loaded") + + // and the widget really is not there, which is what makes the reveal above the fallback's + forms, err := page.FrameLocator("#remark42 iframe").Locator(commentFormSel).Count() + require.NoError(t, err) + assert.Zero(t, forms, "the widget rendered on a host the instance does not allow") +} diff --git a/e2e/deployment_test.go b/e2e/deployment_test.go new file mode 100644 index 00000000..851fd72b --- /dev/null +++ b/e2e/deployment_test.go @@ -0,0 +1,116 @@ +//go:build e2e + +package e2e + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Three configurations that cannot be settings on an instance shared with anything else: each +// changes the widget for every reader of that instance, and each broke in a way nothing else in +// this suite can reach. Their services are in compose-e2e-test.yml. + +// adminEditAddress is the address whose email-derived id compose names in ADMIN_SHARED_ID, so +// signing in with it on that instance produces an admin. +// +// Email and not anonymous: remark42 hashes an anonymous id from the name and the client address +// together (server.go:1220, to tell apart two people picking the same name), so an id written +// down here would belong to nobody on any machine but the one it was read from. The email id is +// sha1 of the address alone +const adminEditAddress = "adminedit@example.com" + +// TestComment_AdminEditHasNoDeadline covers #1986: the backend honored ADMIN_EDIT while the +// frontend went on counting an admin's edit window down and taking the button away at the end, +// so the setting did nothing where it is visible. The instance's ordinary window is short, so the +// 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) { + page := newPage(t) + url := threadURLOn(t, adminEditURL) + frame := openURL(t, page, url) + signInEmail(t, page, frame, "admineditor", adminEditAddress) + + text := "admin edit " + runID + postedAt := time.Now() + postComment(t, frame, text) + + // the countdown is what #2001 left running. an admin has no deadline, so it should never + // have been rendered at all + // asserted on a loaded thread, not on the comment the widget has just added to the + // page. The optimistic render puts a countdown on an admin's own new comment and drops it on + // the next load, so asserting here would fail on that instead of on the deadline logic this + // case is about + frame = reload(t, page) + count, err := actions(frame, text).Locator(`[role="timer"]`).Count() + require.NoError(t, err) + assert.Zero(t, count, "an admin's comment is counting down an edit window that does not apply to it") + + // past the window every other user on this instance is held to. editWindow is EDIT_TIME on + // both short-window instances in compose-e2e-test.yml + waitPastEditWindow(postedAt) + + require.NoError(t, actions(frame, text).Locator(`button:has-text("Edit")`).Click()) + edited := "admin edited after the deadline " + runID + submitForm(t, replyForm(t, frame), edited) + waitVisible(t, comment(frame, edited)) + + // stored and not only rendered: the backend has its own view of the deadline, and the + // widget showing the new text says nothing about which of the two answered + frame = reload(t, page) + waitVisible(t, comment(frame, edited)) +} + +// TestAuth_HeaderJWTSurvivesReload covers #1877. With AUTH_SEND_JWT_HEADER the token arrives in a +// response header instead of a cookie, so the frontend holds it itself and a reload starts with +// 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) { + page := newPage(t) + frame := openURL(t, page, threadURLOn(t, jwtHeaderURL)) + signInAnon(t, page, frame, anonName("headerjwt")) + + frame = reload(t, page) + assertSignedIn(t, page, frame) + + pauseForAuthLimit() + require.NoError(t, frame.Locator(`[title="Sign Out"]`).Click()) + waitVisible(t, frame.Locator(".auth-button")) + + frame = reload(t, page) + waitVisible(t, frame.Locator(".auth-button")) + waitHidden(t, frame.Locator(`[title="Sign Out"]`), + "the header-borne session came back after signing out") +} + +// TestAuth_NoProvidersSaysSo covers #1456, where an instance with no auth provider rendered a +// 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) { + page := newPage(t) + frame := openURL(t, page, threadURLOn(t, noAuthURL)) + + require.NoError(t, frame.Locator(".auth-button").Click()) + waitVisible(t, frame.Locator("text=No providers available")) + + // and no empty form is offered alongside it + inputs, err := frame.Locator(".auth-input-username").Count() + require.NoError(t, err) + assert.Zero(t, inputs, "a sign-in form is offered on an instance with nothing to sign in with") +} + +// waitPastEditWindow waits out the instance's ordinary edit window, measured from the moment the +// comment was posted. There is nothing to poll for here: the admin's comment shows no countdown, +// which is the assertion above, so the deadline passing is not observable in the page. Sleeping +// until a deadline computed from a known setting is not the same as sleeping for a guess +func waitPastEditWindow(postedAt time.Time) { + // a margin over the window itself, since the backend compares against its own clock and the + // post round trip sits between the two + deadline := postedAt.Add(editWindow + time.Second) + if wait := time.Until(deadline); wait > 0 { + time.Sleep(wait) + } +} diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index cf0b6286..db685081 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -6,11 +6,19 @@ // up itself when nothing is listening. Files: // // - e2e_test.go: TestMain, shared helpers, constants +// - harness_test.go: the stale-stack guard and the browser failures no assertion covers // - auth_test.go: dev, anonymous and email sign-in // - comment_test.go: post, reply, edit, delete // - vote_test.go: voting and its failure path // - thread_test.go: sorting and collapse persistence // - iframe_test.go: the iframe's color scheme and reveal handshake +// - geometry_test.go: the height the widget reports and the parent applies +// - embed_test.go: the surface the host page holds, placeholder to destroy +// - config_test.go: the remark_config surface an integrator sets +// - crossorigin_test.go: a host page on an origin the widget is not served from +// - deployment_test.go: the instances whose configuration is the thing under test +// - subscribe_test.go: the email subscription round trip +// - webfiles_test.go: the published /web surface // - widgets_test.go: last-comments, counter and the profile iframe package e2e @@ -28,22 +36,37 @@ import ( "sync/atomic" "testing" "time" + "unicode" "github.com/mxschmitt/playwright-go" "github.com/stretchr/testify/require" ) const ( - // what the browser asks for. these have to be names rather than 127.0.0.1: the dev oauth2 + // what the browser asks for. these have to be names, not 127.0.0.1: the dev oauth2 // server binds whatever host it reads out of REMARK_URL, and a loopback bind inside the // container cannot be published, so the browser resolves the names back with // --host-resolver-rules and reaches the published ports baseURL = "http://remark42:8080" shortEditURL = "http://remark42-shortedit:8081" + // the deployment modes that cannot be a setting on an instance shared with anything else, + // each covering a regression the default configuration cannot reach. see compose-e2e-test.yml + adminEditURL = "http://remark42-adminedit:8082" + jwtHeaderURL = "http://remark42-jwtheader:8083" + noAuthURL = "http://remark42-noauth:8085" + anonVoteURL = "http://remark42-anonvote:8086" + + // a page on an origin the widget is not served from, see compose-e2e-test.yml + hostSiteURL = "http://host-site:8090" // what this process asks for, since it is not the browser and has no resolver rules probeURL = "http://127.0.0.1:8080" shortEditProbeURL = "http://127.0.0.1:8081" + adminEditProbeURL = "http://127.0.0.1:8082" + jwtHeaderProbeURL = "http://127.0.0.1:8083" + noAuthProbeURL = "http://127.0.0.1:8085" + anonVoteProbeURL = "http://127.0.0.1:8086" + hostSiteProbeURL = "http://127.0.0.1:8090" mailpitURL = "http://127.0.0.1:8025" composeFile = "../compose-e2e-test.yml" @@ -57,6 +80,11 @@ const ( // generous because CI runners are slower and less predictable than a laptop waitTimeout = 15 * time.Second + // how long assertSignedIn waits before nudging the widget into re-reading auth state. short + // enough that a refused status read costs a fraction of a test instead of a whole timeout, + // long enough that the ordinary path never takes the nudge + authRepaintWait = 3 * time.Second + // what a single locator call inside a poll body gets. playwright's own default is 30s, // twice the budget of the loops here, so without this a missing element would block one // attempt for longer than the whole loop and then report the wrong thing @@ -73,7 +101,7 @@ var ( extraBrowsersMu sync.Mutex // distinguishes this run's threads from those a previous run left in the database. - // E2E_RUN_ID pins it, so the urls a CI run works on name that run rather than the moment + // E2E_RUN_ID pins it, so the urls a CI run works on name that run and not the moment // the process started runID = firstNonEmpty(os.Getenv("E2E_RUN_ID"), fmt.Sprintf("%d", time.Now().UnixNano())) @@ -88,12 +116,16 @@ 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 rather than a setting, so the suite has to pace +// 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 func pauseForAuthLimit() { - const spacing = 700 * time.Millisecond + // 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() @@ -129,7 +161,10 @@ func TestMain(m *testing.M) { Headless: playwright.Bool(headless), SlowMo: playwright.Float(slowMo), Args: []string{ - "--host-resolver-rules=MAP remark42 127.0.0.1, MAP remark42-shortedit 127.0.0.1", + "--host-resolver-rules=MAP remark42 127.0.0.1, MAP remark42-shortedit 127.0.0.1, " + + "MAP remark42-adminedit 127.0.0.1, MAP remark42-jwtheader 127.0.0.1, " + + "MAP remark42-noauth 127.0.0.1, MAP remark42-anonvote 127.0.0.1, " + + "MAP host-site 127.0.0.1", }, }) if err != nil { @@ -150,11 +185,22 @@ func TestMain(m *testing.M) { // ensureStack waits for a running stack and starts one with compose when there is none func ensureStack() error { + // the digest of the sources the image is built from, exported so compose stamps the build + // with it. read back off the running stack below, since answering on the right ports says + // nothing about which checkout the code came from + stamp, err := sourceStamp() + if err != nil { + return err + } + if err := os.Setenv(stampEnv, stamp); err != nil { + return fmt.Errorf("exporting %s: %w", stampEnv, err) + } + // every service, not just the first: compose-dev-backend.yml and `make rundev` also // publish 8080, and adopting one of those would run the suite against a developer's own // database and fail later as unexplained locator timeouts if stackReady(2 * time.Second) { - return nil + return assertStackMatches(stamp, true) } log.Printf("[INFO] no complete stack on 127.0.0.1, bringing one up from %s", composeFile) @@ -175,7 +221,9 @@ func ensureStack() error { if !stackReady(waitTimeout) { return fmt.Errorf("compose reported the stack healthy but it does not answer") } - return nil + // checked after our own build too, so a stamp that never reaches the image is a failure + // here, and not a guard that silently passes everything for the rest of its life + return assertStackMatches(stamp, false) } func firstNonEmpty(values ...string) string { @@ -192,6 +240,11 @@ func stackReady(timeout time.Duration) bool { for _, url := range []string{ probeURL + "/ping", shortEditProbeURL + "/ping", + adminEditProbeURL + "/ping", + jwtHeaderProbeURL + "/ping", + noAuthProbeURL + "/ping", + anonVoteProbeURL + "/ping", + hostSiteProbeURL + "/post.html", mailpitURL + "/api/v1/messages", } { if err := serverReady(url, timeout); err != nil { @@ -246,11 +299,18 @@ func newPage(t *testing.T) playwright.Page { func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { t.Helper() - ctx, err := b.NewContext() + return newPageInContext(t, b, playwright.BrowserNewContextOptions{}) +} + +// newPageInContext is newPageOn with the context configured, for the cases where what the browser +// says about itself is the thing under test +func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.BrowserNewContextOptions) playwright.Page { + t.Helper() + ctx, err := b.NewContext(opts) require.NoError(t, err) // the reveal timers start when the iframe element is created, so the tests that bound - // them have to measure from there rather than from anything this process can time + // 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)})) tracing := ctx.Tracing().Start(playwright.TracingStartOptions{ @@ -268,7 +328,7 @@ func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { _ = ctx.Close() return } - // say so rather than swallowing it: this runs only on a test that already failed, + // say so instead of swallowing it: this runs only on a test that already failed, // and a silently missing trace is what the reader goes looking for // the pid keeps two processes sharing a run id, and so a trace directory, from // overwriting each other: the counter restarts with every process @@ -288,12 +348,17 @@ func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { page, err := ctx.NewPage() require.NoError(t, err) + // before the first navigation, so nothing the page reports on its way up is missed + watchPage(t, page) debug := os.Getenv("E2E_DEBUG") != "" page.OnResponse(func(r playwright.Response) { switch { case r.Status() == http.StatusTooManyRequests: // the rate limiter answers the widget, not the test, so without this the failure - // arrives as an unexplained locator timeout + // arrives as an unexplained locator timeout. recorded as well as logged: a lost + // /auth/status renders as signed out, and every sign-in assertion then fails + // somewhere that does not name the cause + recordPageIssue(page, "rate limited", r.URL()) log.Printf("[WARN] rate limited: %s", r.URL()) case debug && r.Status() >= 400: body, _ := r.Text() @@ -382,6 +447,31 @@ func threadURLOn(t *testing.T, base string) string { return fmt.Sprintf("%s/web/?e2e=%s-%s", base, name, runID) } +// anonName builds a username no earlier run has used. Blocking a user and verifying one are +// properties of the user, and the stack's database outlives the run, so a name that comes back +// arrives already blocked or already verified: the case then toggles the state off and waits for +// something that is being taken away. +// +// The run id alone is not enough, since E2E_RUN_ID pins it and a second run against a surviving +// stack repeats it, which is exactly what `make e2e-up` invites. The pid distinguishes those +// while leaving CI, where every job is one process, with the run id it wants in the name. +// +// Everything outside [\p{L}\d\s_] is dropped, because the form validates its input against that +// pattern (auth.tsx:332) and the browser refuses to submit anything else: no request is made, and +// the test waits out its timeout on a panel that was never going to change. E2E_RUN_ID is +// "-" on CI, whose hyphen is exactly such a character +func anonName(prefix string) string { + var b strings.Builder + b.WriteString(prefix) + for _, r := range runID { + if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '_' { + b.WriteRune(r) + } + } + fmt.Fprintf(&b, "_%d", os.Getpid()) + return b.String() +} + // openThread loads the demo page on this test's own thread and waits for the widget func openThread(t *testing.T, page playwright.Page) playwright.FrameLocator { t.Helper() @@ -398,6 +488,63 @@ func openURL(t *testing.T, page playwright.Page, url string) playwright.FrameLoc return widget(t, page) } +// embedConfig loads a page carrying a remark_config of the test's choosing and runs the embed +// script against it. +// +// The demo page cannot be used for this: it writes its own remark_config, so anything a test +// needs to vary has to go into a page that carries no widget of its own. privacy.html is served +// by the instance and has none, which also keeps the widget on its own origin, where its CSP +// allows the chunks it loads. host, site_id and url are filled in when the caller leaves them out +// placeholder, when given, is markup left inside the root before the widget runs, which the +// documentation promises is cleared once the iframe reports itself inited +func embedConfig(t *testing.T, page playwright.Page, config map[string]any, placeholder ...string) { + t.Helper() + embedConfigOn(t, page, "/web/privacy.html", config, placeholder...) +} + +// embedConfigOn is embedConfig on a chosen host page, for the cases where the page's own address +// is part of what is under test +func embedConfigOn(t *testing.T, page playwright.Page, hostPage string, config map[string]any, placeholder ...string) { + t.Helper() + + for key, value := range map[string]any{"host": baseURL, "site_id": "remark", "url": threadURL(t)} { + if _, ok := config[key]; !ok { + config[key] = value + } + } + + pauseForAuthLimit() + _, err := page.Goto(baseURL + hostPage) + require.NoError(t, err) + + _, err = page.Evaluate(`([config, placeholder]) => { + window.remark_config = config; + const node = document.createElement('div'); + node.id = 'remark42'; + node.innerHTML = placeholder; + document.body.appendChild(node); + }`, []any{config, strings.Join(placeholder, "")}) + require.NoError(t, err) + + _, err = page.AddScriptTag(playwright.PageAddScriptTagOptions{URL: playwright.String(baseURL + "/web/embed.mjs")}) + 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 +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{ + Status: playwright.Int(http.StatusOK), + ContentType: playwright.String("application/json"), + Body: playwright.String(`{"status":"not logged in"}`), + })) + })) +} + // reload re-navigates and returns the widget once the thread itself has loaded. // // waiting for the comment form is not enough here: root.tsx renders it as soon as the user @@ -437,7 +584,7 @@ func postCommentMatching(t *testing.T, frame playwright.FrameLocator, source, ma } // submitForm fills a comment form and submits it. the submit label is Send, Reply or Save -// depending on the form's mode, so match on the type rather than the text +// depending on the form's mode, so match on the type and not the text func submitForm(t *testing.T, form playwright.Locator, text string) { t.Helper() require.NoError(t, form.Locator("textarea").Fill(text)) @@ -487,7 +634,7 @@ func actions(frame playwright.FrameLocator, text string) playwright.Locator { } // widget returns the widget's iframe once its own content has rendered. it waits on the -// comment form rather than the auth panel, because .auth is only present while signed out +// comment form and not the auth panel, because .auth is only present while signed out func widget(t *testing.T, page playwright.Page) playwright.FrameLocator { t.Helper() frame := page.FrameLocator("#remark42 iframe") @@ -503,12 +650,15 @@ func waitVisible(t *testing.T, loc playwright.Locator) { })) } -func waitHidden(t *testing.T, loc playwright.Locator) { +// waitHidden waits for the element to go away. msgAndArgs says what its staying would mean, +// since the failure otherwise names a selector and not the behavior that broke +func waitHidden(t *testing.T, loc playwright.Locator, msgAndArgs ...any) { t.Helper() - require.NoError(t, loc.WaitFor(playwright.LocatorWaitForOptions{ + err := loc.WaitFor(playwright.LocatorWaitForOptions{ State: playwright.WaitForSelectorStateHidden, Timeout: playwright.Float(float64(waitTimeout.Milliseconds())), - })) + }) + require.NoError(t, err, msgAndArgs...) } // pollText reads an element's text with a timeout short enough to be used inside eventually diff --git a/e2e/embed_test.go b/e2e/embed_test.go new file mode 100644 index 00000000..e40e8b93 --- /dev/null +++ b/e2e/embed_test.go @@ -0,0 +1,121 @@ +//go:build e2e + +package e2e + +import ( + "testing" + + "github.com/mxschmitt/playwright-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEmbed_PlaceholderGivesWayToOneOwnedIframe covers #2192 together with the placeholder +// support #2002 added. createInstance took root.firstElementChild as its iframe, so anything the +// integrator left in the root was adopted instead: a