diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 224a3fe5..571faa15 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -94,6 +94,7 @@ jobs: # has to carry the same value `make e2e-up` and the suite itself would give it - name: Build & start the stack run: | + ./e2e/tls/generate.sh 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 diff --git a/.gitignore b/.gitignore index a2379876..f07e734e 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,6 @@ http-client.env.json # traces from failed e2e runs /e2e/traces/ + +# self-signed certificate for the e2e https services, made by e2e/tls/generate.sh +/e2e/tls/*.pem diff --git a/Makefile b/Makefile index f1b4f677..8d3d68c0 100644 --- a/Makefile +++ b/Makefile @@ -42,6 +42,7 @@ rundev: # 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: + ./e2e/tls/generate.sh E2E_STAMP=$$(./e2e/stamp.sh) docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait e2e-down: diff --git a/compose-e2e-test.yml b/compose-e2e-test.yml index bf6493fa..1358e6f8 100644 --- a/compose-e2e-test.yml +++ b/compose-e2e-test.yml @@ -245,6 +245,71 @@ services: timeout: 3s retries: 30 + # the same widget over TLS, which is the only way to reach anything the browser gates on the + # page protocol: Secure cookies, SameSite=None, Partitioned, and any code reading + # location.protocol. The certificate is self-signed and the suite passes IgnoreHTTPSErrors + remark42-https: + image: ghcr.io/umputun/remark42:dev + container_name: "remark42-e2e-https" + pull_policy: never + depends_on: + remark42: + condition: service_started + mailpit: + condition: service_started + + ports: + - "127.0.0.1:8443:8443" + + environment: + - REMARK_URL=https://remark42-https:8443 + - SECRET=12345 + - AUTH_ANON=true + - SSL_TYPE=static + - SSL_PORT=8443 + - SSL_CERT=/srv/tls/cert.pem + - SSL_KEY=/srv/tls/key.pem + # the widget is embedded from another origin here, so its cookies have to survive a + # third-party frame; this is the setting that decides whether they do + - AUTH_SAME_SITE=none + # and the token arrives in a header, so the widget writes the cookies itself through + # setAuthCookie. Without this the client-side writer never runs on any https page in the + # stack, and every assertion about the attributes it chooses is vacuous + - AUTH_SEND_JWT_HEADER=true + # email as well as anonymous, so the flow most operators run off-domain is measured here + # and not inferred from the anonymous one. the writer keys off the X-JWT header rather than + # off the provider, which is exactly the assumption a case signing in by email checks + - AUTH_EMAIL_ENABLE=true + - AUTH_EMAIL_FROM=remark42@example.com + - SMTP_HOST=mailpit + - SMTP_PORT=1025 + - UPDATE_LIMIT=100 + volumes: + - remark42-e2e-https-var:/srv/var + - ./e2e/tls:/srv/tls:ro + healthcheck: + test: ["CMD", "curl", "--fail", "--insecure", "https://localhost:8443/ping"] + interval: 2s + timeout: 3s + retries: 30 + + # and a host page for it, on its own name, so the embed is genuinely cross-site over TLS + host-site-https: + image: nginx:1.29-alpine + container_name: "remark42-e2e-hostsite-https" + ports: + - "127.0.0.1:8444:443" + volumes: + - ./e2e/hostsite:/usr/share/nginx/html:ro + - ./e2e/tls:/etc/nginx/tls:ro + - ./e2e/tls/nginx-tls.conf:/etc/nginx/conf.d/default.conf:ro + healthcheck: + # 127.0.0.1 and not localhost: nginx listens on IPv4 only and busybox wget tries ::1 first + test: ["CMD", "wget", "-q", "-O", "/dev/null", "http://127.0.0.1/post-https.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 @@ -268,3 +333,4 @@ volumes: remark42-e2e-jwtheader-var: remark42-e2e-noauth-var: remark42-e2e-anonvote-var: + remark42-e2e-https-var: diff --git a/e2e/README.md b/e2e/README.md index d1bddc03..f13a9cbb 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -61,7 +61,7 @@ Before pushing, `cd e2e && go vet -tags=e2e ./...` and `golangci-lint run --buil ## The stack -`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: +`compose-e2e-test.yml` at the repository root runs ten 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 @@ -70,11 +70,15 @@ Before pushing, `cd e2e && go vet -tags=e2e ./...` and `golangci-lint run --buil - **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 +- **remark42-https** on `:8443`, the widget over TLS with `AUTH_SAME_SITE=none` and `AUTH_SEND_JWT_HEADER=true`. The header mode is what makes the widget write its own cookies through `setAuthCookie`, so the attributes it chooses are observable at all. Anonymous and email sign-in are both enabled, since the third-party cases run each flow: the writer keys off the `X-JWT` header rather than off the provider, and that is an assumption worth measuring rather than asserting +- **host-site-https** on `:8444`, an nginx serving the same `e2e/hostsite/` over TLS, so the embed is cross-site *and* secure. `post-https.html` is its page - **mailpit** on `:8025`, which catches the email-auth verification message and the subscription token for the suite to read back +Both TLS services read a self-signed certificate from `e2e/tls/`, which `e2e/tls/generate.sh` writes and `.gitignore` keeps out of the tree. `make e2e-up`, the workflow and `ensureStack` all run it before compose, so bringing the stack up by hand with a bare `docker compose up` is the one path that needs it run first. Every browser context and the readiness client accept that certificate, and they talk to nothing else. + 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. +The remark42 instances beyond the first offer anonymous sign-in only, for the reason `remark42-shortedit` does: the dev oauth2 provider binds a port fixed at 8084 and cannot be published twice. `remark42-adminedit` gets its admin from `ADMIN_SHARED_ID`, since the anonymous provider derives the user id from the name and the id for a chosen name can be written into the compose file ahead of time. Three settings exist for the tests and not for realism, and each is there for a reason: @@ -84,22 +88,22 @@ Three settings exist for the tests and not for realism, and each is there for a ## 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. +The stack now carries TLS on two services, so behaviour the browser gates on the page protocol is reachable: `Secure` cookies, `SameSite=None`, `Partitioned`, and anything keyed on `window.location.protocol`. `https_test.go` is where those cases live. What is still out of reach is a browser engine other than Chromium for them, since the resolver rules the hostnames need are a Chromium flag. -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: +The trap that remains is which cookie policy a run is under. Playwright's own default `--disable-features` argument carries `ThirdPartyStoragePartitioning`, and it beats both `--test-third-party-cookie-phaseout` and `--block-third-party-cookies` passed through `Args`. A run configured that way keeps an ordinary third-party cookie exactly as it would with no flags at all, so it proves nothing while looking like it proved something. The lever is `IgnoreDefaultArgs` on the launch options: drop that default entry and re-supply `--disable-features` without that one feature, which is what `TestHTTPS_SessionSurvivesThirdPartyCookieBlocking` does. Measured on a cross-site https embed: | | 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. +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. The argument list is Playwright's own and version-specific, so that control is what keeps it from rotting silently. + +That control has to read the cookie back through `document.cookie` in the same frame evaluate that writes it, never through `page.Context().Cookies()`. The two are separate channels with no ordering between them: in the Chromium that Playwright 1.62.1 ships, `CookieJar::SetCookie` queues the write and returns, and the renderer's own `document.cookie` getter is the barrier that forces it to settle, while Playwright's context read is a browser-session `Storage.getCookies` that never touches that frame's jar. A control read that way can find the name absent because the write has not landed, which is the one outcome it exists to rule out. It also writes a valid `Secure; SameSite=None; Partitioned` sentinel and requires that one to be present, so "the browser refused the control" is distinguishable from "nothing was written at all". 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. +Partitioned cookies do not make cross-domain OAuth work, whatever the plan once implied. The partition key is the top-level site at the moment the cookie is set, and `oauthSignin` opens a popup, which is its own top-level context: the callback cookie is keyed to the auth host while the frame is keyed to the embedder, and the two never match. The forms that do work embedded are the ones whose cookie is written inside the frame, which is anonymous, email and telegram. A case asserting OAuth works cross-domain would be asserting something untrue. ## Isolation diff --git a/e2e/crossorigin_test.go b/e2e/crossorigin_test.go index ec161aef..98a1f818 100644 --- a/e2e/crossorigin_test.go +++ b/e2e/crossorigin_test.go @@ -22,11 +22,11 @@ import ( // 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 +// Signing in is deliberately not asserted here. An embedded cookie needs SameSite=None, which +// browsers only accept as Secure, and this page is served over http, so the form cannot be +// delivered at all. That half is covered over TLS in https_test.go, where the assertion that +// matters is the reload: the widget holds its token in memory for the life of a page, so a +// sign-in that never reloads passes while persistence is broken func TestCrossOrigin_WidgetRendersOnAnotherOrigin(t *testing.T) { thread := fmt.Sprintf("%s/post.html?e2e=%s-%s", hostSiteURL, "crossorigin", runID) text := "cross origin " + runID diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index db685081..505a047a 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -16,6 +16,7 @@ // - 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 +// - https_test.go: the widget over TLS, where the browser's protocol gates apply // - deployment_test.go: the instances whose configuration is the thing under test // - subscribe_test.go: the email subscription round trip // - webfiles_test.go: the published /web surface @@ -24,6 +25,7 @@ package e2e import ( "context" + "crypto/tls" "encoding/json" "fmt" "log" @@ -59,6 +61,10 @@ const ( // a page on an origin the widget is not served from, see compose-e2e-test.yml hostSiteURL = "http://host-site:8090" + // the host page over TLS, which is the only way to reach what the browser gates on the page + // protocol. the certificate is self-signed, so every context passes IgnoreHTTPSErrors + httpsHostSiteURL = "https://host-site-https:8444" + // 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" @@ -67,6 +73,8 @@ const ( noAuthProbeURL = "http://127.0.0.1:8085" anonVoteProbeURL = "http://127.0.0.1:8086" hostSiteProbeURL = "http://127.0.0.1:8090" + httpsProbeURL = "https://127.0.0.1:8443" + httpsHostProbeURL = "https://127.0.0.1:8444" mailpitURL = "http://127.0.0.1:8025" composeFile = "../compose-e2e-test.yml" @@ -112,7 +120,12 @@ var ( // the default client has no timeout, so a port that accepts and then stalls would block // a probe well past its own deadline and leave TestMain looking hung - probeClient = &http.Client{Timeout: 5 * time.Second} + probeClient = &http.Client{ + Timeout: 5 * time.Second, + // the https services in the stack are self-signed, and this client only ever talks to + // them, so refusing the certificate would only stop the readiness probe + Transport: &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}, //nolint:gosec // test stack + } ) // everything under /auth/ is rate limited to 2 requests a second, and that figure is a bare @@ -164,7 +177,8 @@ func TestMain(m *testing.M) { "--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", + "MAP host-site 127.0.0.1, " + + "MAP remark42-https 127.0.0.1, MAP host-site-https 127.0.0.1", }, }) if err != nil { @@ -203,6 +217,11 @@ func ensureStack() error { return assertStackMatches(stamp, true) } + // the https services will not start without one, and compose cannot make it itself + if out, gerr := exec.Command("./tls/generate.sh").CombinedOutput(); gerr != nil { + return fmt.Errorf("generating the test certificate: %w\n%s", gerr, out) + } + log.Printf("[INFO] no complete stack on 127.0.0.1, bringing one up from %s", composeFile) ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute) defer cancel() @@ -245,6 +264,8 @@ func stackReady(timeout time.Duration) bool { noAuthProbeURL + "/ping", anonVoteProbeURL + "/ping", hostSiteProbeURL + "/post.html", + httpsProbeURL + "/ping", + httpsHostProbeURL + "/post-https.html", mailpitURL + "/api/v1/messages", } { if err := serverReady(url, timeout); err != nil { @@ -306,6 +327,10 @@ func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { // says about itself is the thing under test func newPageInContext(t *testing.T, b playwright.Browser, opts playwright.BrowserNewContextOptions) playwright.Page { t.Helper() + // the https services carry a self-signed certificate, and a context that refuses it cannot + // reach them at all. harmless for the http ones + opts.IgnoreHttpsErrors = playwright.Bool(true) + ctx, err := b.NewContext(opts) require.NoError(t, err) diff --git a/e2e/hostsite/post-https.html b/e2e/hostsite/post-https.html new file mode 100644 index 00000000..079757fe --- /dev/null +++ b/e2e/hostsite/post-https.html @@ -0,0 +1,34 @@ + + + + + A page on somebody else's site, over TLS + + +

A page on somebody else's site, over TLS

+

+ Served from a different origin than the comments it embeds, and over https, which is the only + way the widget's cookies can carry Secure, SameSite=None and Partitioned. +

+
+ + + diff --git a/e2e/https_test.go b/e2e/https_test.go new file mode 100644 index 00000000..ae205e54 --- /dev/null +++ b/e2e/https_test.go @@ -0,0 +1,324 @@ +//go:build e2e + +package e2e + +import ( + "fmt" + "os" + "slices" + "strings" + "testing" + + "github.com/mxschmitt/playwright-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Everything else in this suite speaks http, which hides an entire class of defect: the browser +// gates Secure cookies, SameSite=None, Partitioned and anything reading location.protocol on the +// page protocol, so code taking those paths is never executed. setAuthCookie decorating its +// cookies with __Host- on https pages survived for exactly that reason. +// +// These run against the TLS pair in compose-e2e-test.yml, which carries a self-signed certificate +// every context here accepts. That instance also runs with AUTH_SEND_JWT_HEADER, so the widget +// writes its own cookies through setAuthCookie: without it the client-side writer never runs on +// any https page in the stack and every assertion about the attributes it chooses is vacuous. + +// An unpartitioned third-party cookie is refused only when storage partitioning is enforced, and +// playwright's own default arguments disable it. Dropping that default and re-supplying it without +// the one feature is the only lever; see "What this suite cannot reach" in the README. +// +// The list is playwright's own and therefore version-specific. It is not trusted on its own: the +// case using it asserts a control first, so a list that stops matching fails as itself. +const ( + playwrightDisabledFeatures = "--disable-features=AvoidUnnecessaryBeforeUnloadCheckSync," + + "BoundaryEventDispatchTracksNodeRemoval,DestroyProfileOnBrowserClose,DialMediaRouteProvider," + + "GlobalMediaControls,HttpsUpgrades,LensOverlay,MediaRouter,PaintHolding," + + "ThirdPartyStoragePartitioning,BlockOriginHeaderModificationOnRedirect,Translate,AutoDeElevate," + + "OptimizationHints,msForceBrowserSignIn,msEdgeUpdateLaunchServicesPreferredVersion" + + // the one entry that has to go, named once + partitioningFeature = "ThirdPartyStoragePartitioning," +) + +// withPartitioningEnabled is playwright's own list with the one feature removed, derived rather +// than written out again: the two differ by a single entry in three hundred characters, and a +// second copy is a thing to forget on the next playwright bump +func withPartitioningEnabled(t *testing.T) string { + t.Helper() + + out := strings.Replace(playwrightDisabledFeatures, partitioningFeature, "", 1) + require.NotEqual(t, playwrightDisabledFeatures, out, + "%q is no longer in playwright's default argument list, so removing it enables nothing and "+ + "this browser would block no cookie at all", partitioningFeature) + return out +} + +// The flows worth measuring in a third-party frame. OAuth is absent on purpose: the provider +// callback lands in a popup, which is a top-level context of its own, so the cookie set there is +// keyed to the auth host and never reaches the frame. Telegram is absent because the stack cannot +// answer as Telegram, see #2208. +// +// Email as well as anonymous because the widget's writer keys off the X-JWT header rather than off +// the provider, so the two are expected to behave alike. Expected is not measured, and email is the +// flow an operator running off-domain actually reaches for +var httpsFlows = []struct { + name string + signIn func(t *testing.T, page playwright.Page, frame playwright.FrameLocator) +}{ + {"anonymous", func(t *testing.T, page playwright.Page, frame playwright.FrameLocator) { + signInAnon(t, page, frame, anonName("httpsreader")) + }}, + {"email", func(t *testing.T, page playwright.Page, frame playwright.FrameLocator) { + // the address decides the user id and the mailbox this reads back, so it carries the run + // id and the case: mailpit keeps everything, and a shared address would let one case read + // the token another asked for + signInEmail(t, page, frame, "httpsemail", + fmt.Sprintf("https-email-%s-%s@example.com", strings.ReplaceAll(t.Name(), "/", "-"), runID)) + }}, +} + +// httpsThread is a thread on the TLS host page, which is a different site from the widget's own +// origin, so the widget's cookies there are genuinely third-party +func httpsThread(t *testing.T, label string) string { + t.Helper() + return fmt.Sprintf("%s/post-https.html?e2e=%s-%s", httpsHostSiteURL, label, runID) +} + +// TestHTTPS_CrossOriginSignInSurvivesAReload is what the http cross-origin case cannot assert. +// The widget keeps its token in memory for the life of a page, so signing in and posting proves +// nothing about persistence: only a reload asks whether the cookie was delivered, stored under a +// name the backend reads, and sent back from a third-party frame. On http that cookie cannot even +// be written, since the third-party form requires Secure. +// +// This runs under the browser's default policy, where third-party cookies are allowed. +// TestHTTPS_SessionSurvivesThirdPartyCookieBlocking is the same question with them blocked. +func TestHTTPS_CrossOriginSignInSurvivesAReload(t *testing.T) { + for _, flow := range httpsFlows { + t.Run(flow.name, func(t *testing.T) { + page := newPage(t) + + pauseForAuthLimit() + _, err := page.Goto(httpsThread(t, "https-signin-"+flow.name)) + require.NoError(t, err) + + frame := widget(t, page) + flow.signIn(t, page, frame) + + text := "posted over tls by " + flow.name + " " + runID + postComment(t, frame, text) + + // the assertion the whole file exists for + pauseForAuthLimit() + _, err = page.Reload() + require.NoError(t, err) + + frame = widget(t, page) + assertSignedIn(t, page, frame) + waitVisible(t, comment(frame, text)) + }) + } +} + +// TestHTTPS_AuthCookiesCarryTheThirdPartyForm reads the cookies the browser actually stored for +// the widget's origin while it is embedded elsewhere. A cookie the browser refused is not in this +// list at all, and one the browser kept but will not send from a frame is worse than useless, so +// the attributes are the assertion and not the sign-in they enable. +// +// Both cookies are checked and not the JWT alone: the fetcher reads XSRF-TOKEN and returns its +// value as a header, and go-pkgz/auth refuses a cookie-borne token whose header does not match, so +// a JWT arriving beside an XSRF cookie the frame cannot receive authenticates nobody. +func TestHTTPS_AuthCookiesCarryTheThirdPartyForm(t *testing.T) { + page := newPage(t) + + pauseForAuthLimit() + _, err := page.Goto(httpsThread(t, "https-cookies")) + require.NoError(t, err) + + frame := widget(t, page) + signInAnon(t, page, frame, anonName("httpscookies")) + + cookies, err := page.Context().Cookies() + require.NoError(t, err) + + // both writers are in play here: the backend sets its own pair, and the widget sets a + // partitioned pair of the same names through setAuthCookie. Every copy has to be usable from + // a third-party frame, and the partitioned one has to exist, or the assertions below would be + // reading the server's cookie and saying nothing about the client's + for _, name := range []string{"JWT", "XSRF-TOKEN"} { + var partitioned int + var seen int + for _, c := range cookies { + if c.Name != name { + continue + } + seen++ + assert.True(t, c.Secure, "a %s cookie is not Secure, so a third-party frame can never receive it", name) + require.NotNil(t, c.SameSite, "a %s cookie carries no SameSite at all", name) + assert.Equal(t, *playwright.SameSiteAttributeNone, *c.SameSite, + "a %s cookie is not SameSite=None, and SameSite is judged against the top-level site, so "+ + "anything stricter is never sent from an embedded widget", name) + if c.PartitionKey != nil { + partitioned++ + } + } + require.NotZero(t, seen, "no %s cookie was stored at all, so the browser refused what was sent: %v", + name, cookieNames(cookies)) + assert.NotZero(t, partitioned, + "no partitioned %s cookie was stored, so setAuthCookie either did not run or chose attributes "+ + "a blocking browser will drop", name) + } + + // the XSRF value has to be readable by the widget's own script, which is what puts it in the + // header the backend matches the token against + for _, c := range cookies { + if c.Name == "XSRF-TOKEN" { + assert.False(t, c.HttpOnly, "the XSRF cookie has to be readable by the widget's own script") + } + } + + // no name the server does not read. A __Host- prefixed cookie is a perfectly valid cookie the + // browser stores happily, which is why writing one is a defect nothing rejects: the backend + // looks for JWT and the fetcher reads XSRF-TOKEN, and neither finds a decorated name. + // + // this can only fail because the instance runs with AUTH_SEND_JWT_HEADER, which is what makes + // the widget write cookies of its own instead of leaving the server's pair alone. + for _, c := range cookies { + assert.NotContains(t, c.Name, "__Host-", + "a __Host- prefixed cookie was stored, and nothing on either side reads that name") + } +} + +// TestHTTPS_SessionSurvivesThirdPartyCookieBlocking is the reload question under the policy that +// makes it hard. With storage partitioning enforced an ordinary third-party cookie is dropped, so +// the session survives only because the widget's own cookies carry Partitioned; the server's pair +// does not, and vanishes. +// +// The control comes first and is not decoration: playwright's default arguments disable the policy +// outright, so a run configured wrongly keeps every third-party cookie and this case would pass +// while asserting nothing at all. +func TestHTTPS_SessionSurvivesThirdPartyCookieBlocking(t *testing.T) { + blocking, err := pw.Chromium.Launch(playwright.BrowserTypeLaunchOptions{ + Headless: playwright.Bool(os.Getenv("E2E_HEADLESS") != "false"), + IgnoreDefaultArgs: []string{playwrightDisabledFeatures}, + Args: []string{ + withPartitioningEnabled(t), + "--test-third-party-cookie-phaseout", + "--host-resolver-rules=MAP remark42-https 127.0.0.1, MAP host-site-https 127.0.0.1", + }, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = blocking.Close() }) + + for _, flow := range httpsFlows { + t.Run(flow.name, func(t *testing.T) { + // a context of its own, so neither case inherits what the other stored + page := newPageOn(t, blocking) + + pauseForAuthLimit() + _, err := page.Goto(httpsThread(t, "https-blocked-"+flow.name)) + require.NoError(t, err) + frame := widget(t, page) + + assertPartitioningEnforced(t, page) + + flow.signIn(t, page, frame) + + // the chain this case rests on, asserted before the reload can fail on the end of it: + // the server sends X-JWT only under AUTH_SEND_JWT_HEADER, setAuthCookie writes its own + // pair only when the fetcher sees that header, and only that pair carries Partitioned, + // since the server's own cookies carry none. drop the flag from the service and the + // reload below signs nobody in, which reads as a defect in cookie handling instead of a + // stack that cannot test it + written, err := page.Context().Cookies() + require.NoError(t, err) + require.True(t, slices.ContainsFunc(written, func(c playwright.Cookie) bool { + return c.Name == "JWT" && c.PartitionKey != nil + }), "no partitioned JWT cookie was written before the reload, so the widget's own writer never ran: "+ + "remark42-https is most likely no longer configured with AUTH_SEND_JWT_HEADER. stored: %v", + cookieNames(written)) + + pauseForAuthLimit() + _, err = page.Reload() + require.NoError(t, err) + + frame = widget(t, page) + assertSignedIn(t, page, frame) + + // and it survived because the widget's own cookies are partitioned, which is the only + // form a blocking browser keeps + cookies, err := page.Context().Cookies() + require.NoError(t, err) + for _, name := range []string{"JWT", "XSRF-TOKEN"} { + var stored *playwright.Cookie + for i, c := range cookies { + if c.Name == name { + stored = &cookies[i] + break + } + } + require.NotNil(t, stored, "no %s cookie survived the blocking browser: %v", name, cookieNames(cookies)) + assert.NotNil(t, stored.PartitionKey, + "the %s cookie is not partitioned, so it only survived because the browser was not blocking", name) + } + }) + } +} + +// assertPartitioningEnforced proves the browser refuses an ordinary third-party cookie before any +// case leans on it. Both cookies are written and read back inside a single frame evaluate, and the +// read is `document.cookie` rather than the context's cookie list, because the two are different +// channels with no ordering between them. +// +// Pinned to a build rather than stated as a rule: playwright 1.62.1 ships chromium 151.0.7922.34, +// where Blink's CookieJar::SetCookie queues the write and returns without a completion callback, +// and the renderer's own document.cookie getter is the barrier that forces the pending write to +// settle. Playwright's Context().Cookies() is a browser-session Storage.getCookies that never +// touches that frame's jar, so reading the control through it can find the name absent because the +// write has not landed yet, which is precisely the outcome the control exists to rule out. An +// implementation is free to serialize both calls in the browser process, so this is what to +// re-check on a playwright bump. +// +// The sentinel is the positive half and it has to come first: a valid third-party cookie proves the +// write path and the partitioned path are both live, and only then does the control's absence mean +// the browser turned it down rather than that nothing was written at all +func assertPartitioningEnforced(t *testing.T, page playwright.Page) { + t.Helper() + + const ( + sentinel = "e2esentinel" + control = "e2econtrol" + ) + + // one evaluate: both writes, then the getter that settles them, returned as separate answers so + // a missing sentinel and a surviving control stay distinguishable + raw, err := page.FrameLocator("#remark42 iframe").Locator("body").Evaluate(`() => { + document.cookie = 'e2econtrol=1; Path=/; Secure; SameSite=None'; + document.cookie = 'e2esentinel=1; Path=/; Secure; SameSite=None; Partitioned'; + const names = document.cookie.split(';').map((c) => c.trim().split('=')[0]); + return { sentinel: names.includes('e2esentinel'), control: names.includes('e2econtrol'), all: names }; + }`, nil) + require.NoError(t, err) + + got, ok := raw.(map[string]any) + require.True(t, ok, "expected an object from the frame, got %T (%v)", raw, raw) + + require.Equal(t, true, got["sentinel"], + "a %s cookie in the third-party form the browser still accepts was not stored, so nothing was "+ + "written at all and the absence of %s below would mean nothing. cookies in the frame: %v", + sentinel, control, got["all"]) + + require.Equal(t, false, got["control"], + "an ordinary third-party %s cookie survived alongside the partitioned %s, so this browser is "+ + "not partitioning storage and nothing here is being tested. playwright's default argument "+ + "list has most likely changed: see playwrightDisabledFeatures. cookies in the frame: %v", + control, sentinel, got["all"]) +} + +func cookieNames(cookies []playwright.Cookie) []string { + out := make([]string, 0, len(cookies)) + for _, c := range cookies { + out = append(out, c.Name) + } + return out +} diff --git a/e2e/tls/generate.sh b/e2e/tls/generate.sh new file mode 100755 index 00000000..14fd9166 --- /dev/null +++ b/e2e/tls/generate.sh @@ -0,0 +1,28 @@ +#!/bin/sh +# Self-signed certificate for the https services in the e2e stack. +# +# The suite passes IgnoreHTTPSErrors, so the certificate is never validated and its contents do +# not have to satisfy anything. It carries the right names anyway, which removes the name mismatch +# from the list of complaints when somebody looks at the stack by hand. The issuer is still unknown, +# so curl needs -k and a browser needs an exception either way. +# +# Regenerated only when missing: a fresh certificate on every stack-up would give the running +# services one the containers were not started with, since nginx and remark42 read it once. +set -eu + +cd "$(dirname "$0")" + +if [ -f cert.pem ] && [ -f key.pem ]; then + exit 0 +fi + +# stdout only. openssl writes its progress to stderr and its errors there too, and dropping both +# leaves every caller with a bare exit code: the CI step, `make e2e-up` and ensureStack all print +# nothing about why the certificate could not be made +openssl req -x509 -newkey rsa:2048 -nodes -days 3650 \ + -keyout key.pem -out cert.pem \ + -subj "/CN=remark42-e2e" \ + -addext "subjectAltName=DNS:remark42-https,DNS:host-site-https,DNS:localhost,IP:127.0.0.1" \ + >/dev/null + +chmod 644 key.pem cert.pem diff --git a/e2e/tls/nginx-tls.conf b/e2e/tls/nginx-tls.conf new file mode 100644 index 00000000..e12e2333 --- /dev/null +++ b/e2e/tls/nginx-tls.conf @@ -0,0 +1,22 @@ +# the published listener, which is what the suite and the browser use +server { + listen 443 ssl; + server_name host-site-https; + + ssl_certificate /etc/nginx/tls/cert.pem; + ssl_certificate_key /etc/nginx/tls/key.pem; + + root /usr/share/nginx/html; + index post-https.html; +} + +# for the container healthcheck only, and not published. busybox wget, which is all this image +# carries, has no TLS, so a check against the listener above cannot tell "not started yet" from +# "cannot speak https at all" +server { + listen 80; + server_name localhost; + + root /usr/share/nginx/html; + index post-https.html; +}