Follow real links instead of building them, and check the CSP on every answer (issue 1268)

modules/mesh-mail-web/cmd/mesh-mail-web/acceptance_integration_test.go

@@ -11,6 +11,8 @@ package main
 // lead's try runs it for real.
 
 import (
+	"encoding/json"
+	"html"
 	"io"
 	"net/http"
 	"net/url"
@@ -122,6 +124,39 @@ func (s *sendStandIn) status(map[string]any) (any, error) {
 	return map[string]any{"status": "sent", "words": "sent on the operator's Approve"}, nil
 }
 
+// expectSignIn drives one of the three requests RequireSession protects (a page, an attachment, a send) with
+// no session cookie, answering the end of the road: a GET follows RequireSession's own one-time bounce first
+// (review round 1 finding 1 — the first cookieless answer is a 200 page of this origin's own, never a
+// redirect, so this reads its own meta refresh and asks it, exactly as a browser's own would), the same as
+// followSignInBounce; a POST (a bounce would lose its own body) is sent to sign in directly, unchanged.
+func expectSignIn(t *testing.T, mw *startedMailWeb, method, path string) *http.Response {
+	t.Helper()
+	if method == http.MethodGet {
+		return followSignInBounce(t, mw, path)
+	}
+	return doWithCookie(t, mw, method, path, nil, nil)
+}
+
+// extractOlderLink reads the folder page's own "Older" link out of its rendered HTML, by its own anchor text
+// — never built by hand (review round 1 finding 3): html/template's own escaping of its href (percent-encoding
+// the date's own colons, "&" for the query's own separators) is exactly what a browser's own link carries,
+// and what this reads back.
+func extractOlderLink(t *testing.T, body string) string {
+	t.Helper()
+	marker := ">Older</a>"
+	end := strings.Index(body, marker)
+	if end < 0 {
+		t.Fatalf("no Older link in the page:\n%s", body)
+	}
+	head := body[:end]
+	hrefStart := strings.LastIndex(head, `href="`)
+	if hrefStart < 0 {
+		t.Fatalf("the Older link carries no href:\n%s", body)
+	}
+	href := strings.TrimSuffix(head[hrefStart+len(`href="`):], `"`)
+	return html.UnescapeString(href)
+}
+
 // 1268/design test 1 (AC1): no session redirects a page, an attachment and a send to sign-in; an identity
 // not in operators is 403; a send without its token is 403; the session cookie is Secure, HttpOnly,
 // SameSite=Strict, 12 hours.
@@ -132,10 +167,10 @@ func TestAcceptanceSignInRedirectsAndRefusals(t *testing.T) {
 	}
 
 	for _, path := range []string{"/", "/attachment?account=a&folder=INBOX&uid=1&key=att1", "/write"} {
-		resp := doWithCookie(t, mw, http.MethodGet, path, nil, nil)
+		resp := expectSignIn(t, mw, http.MethodGet, path)
 		_ = resp.Body.Close()
 		if resp.StatusCode != http.StatusFound {
-			t.Fatalf("%s with no session: status %d, want 302 (redirect to sign in)", path, resp.StatusCode)
+			t.Fatalf("%s with no session: status %d, want 302 (redirect to sign in, after RequireSession's own bounce)", path, resp.StatusCode)
 		}
 	}
 	sendResp := doForm(t, mw, "/send", url.Values{}, nil)
@@ -145,16 +180,11 @@ func TestAcceptanceSignInRedirectsAndRefusals(t *testing.T) {
 	}
 
 	// An identity not in operators: 403, no session cookie.
-	req, err := http.NewRequest(http.MethodGet, "http://"+mw.internalHost+"/", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	req.Host = mw.internalHost
-	resp, err := mw.client.Do(req)
-	if err != nil {
-		t.Fatal(err)
-	}
+	resp := followSignInBounce(t, mw, "/")
 	_ = resp.Body.Close()
+	if resp.StatusCode != http.StatusFound {
+		t.Fatalf("after the bounce: status %d, want 302 to sign in", resp.StatusCode)
+	}
 	loc, err := url.Parse(resp.Header.Get("Location"))
 	if err != nil {
 		t.Fatal(err)
@@ -246,28 +276,57 @@ func TestAcceptanceBrowsingAndSendingEndToEnd(t *testing.T) {
 		t.Fatalf("the accounts page shows account c's folder, though it was never granted:\n%s", accBody)
 	}
 
-	// The folder's first page: 50 mails, a next link to the 51st-55th.
+	// The folder's first page: 50 mails, a next link to the 51st-55th (review round 1 finding 3: read the
+	// "Older" link out of the page and follow it, rather than build page 2's own URL by hand — and compare the
+	// 50th mail's own date exactly, not merely up to its hour, which every one of these 55 mails shares).
 	page1 := doWithCookie(t, mw, http.MethodGet, "/folder?account=a&folder=INBOX", nil, cookie)
 	page1Body, _ := io.ReadAll(page1.Body)
 	_ = page1.Body.Close()
 	fiftieth := mails[49]["date"].(string)
-	// html/template percent-encodes an href's own query value (":" becomes "%3a" etc — issue 1264's own
-	// unit test, TestFolderPageShows50NewestAndLinksToTheNextPageByDate, hit the same shape): compared only
-	// up to the date's own colon-free prefix, never byte-for-byte against the raw RFC3339 string.
-	datePrefix := strings.SplitN(fiftieth, ":", 2)[0] // "2026-01-02T12"
-	if !strings.Contains(strings.ToLower(string(page1Body)), strings.ToLower("before="+datePrefix)) {
-		t.Fatalf("the folder's first page does not link to the next page before the 50th mail's date (%s):\n%s", fiftieth, page1Body)
+	olderLink := extractOlderLink(t, string(page1Body))
+	olderQuery, err := url.ParseQuery(strings.SplitN(olderLink, "?", 2)[1])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if olderQuery.Get("before") != fiftieth {
+		t.Fatalf("the folder's first page links to before=%q, want exactly the 50th mail's own date %q", olderQuery.Get("before"), fiftieth)
 	}
 
-	// The folder's second page: asked with that same before.
-	page2 := doWithCookie(t, mw, http.MethodGet, "/folder?account=a&folder=INBOX&before="+url.QueryEscape(fiftieth), nil, cookie)
+	// The folder's second page: followed from that same link, not rebuilt.
+	page2 := doWithCookie(t, mw, http.MethodGet, olderLink, nil, cookie)
 	page2Body, _ := io.ReadAll(page2.Body)
 	_ = page2.Body.Close()
 	if page2.StatusCode != http.StatusOK {
 		t.Fatalf("the folder's second page: status %d: %s", page2.StatusCode, page2Body)
 	}
-	if !strings.Contains(string(page2Body), "mail 5") { // the 55th-50th mails' own subjects
-		t.Fatalf("the folder's second page does not show the older mails:\n%s", page2Body)
+	// Exact subjects (">mail 5</a>", never a substring "mail 55" would also satisfy): the second page shows the
+	// 55th-51st mails' own oldest five, and none of the newer ones the first page already showed.
+	for _, subject := range []string{"mail 5", "mail 4", "mail 3", "mail 2", "mail 1"} {
+		if !strings.Contains(string(page2Body), ">"+subject+"</a>") {
+			t.Fatalf("the folder's second page does not show %q (its own exact subject):\n%s", subject, page2Body)
+		}
+	}
+	if strings.Contains(string(page2Body), ">mail 6</a>") {
+		t.Fatalf("the folder's second page repeats mail 6, which belongs on the first page:\n%s", page2Body)
+	}
+
+	// The second folder page's own mail-store.search was asked before=the 50th mail's own date, not merely
+	// trusted (finding 3).
+	var searchCalls []askRecord
+	for _, c := range mw.store.Calls() {
+		if c.Key == "seat:mail-store.search" {
+			searchCalls = append(searchCalls, c)
+		}
+	}
+	if len(searchCalls) != 2 {
+		t.Fatalf("mail-store.search was asked %d times, want exactly 2 (one per folder page)", len(searchCalls))
+	}
+	var secondSearchArgs map[string]any
+	if err := json.Unmarshal(searchCalls[1].Body, &secondSearchArgs); err != nil {
+		t.Fatal(err)
+	}
+	if secondSearchArgs["before"] != fiftieth {
+		t.Fatalf("the second folder page's own search asked before=%v, want the 50th mail's own date %q", secondSearchArgs["before"], fiftieth)
 	}
 
 	// A search.
@@ -307,7 +366,14 @@ func TestAcceptanceBrowsingAndSendingEndToEnd(t *testing.T) {
 	if sendResp.StatusCode != http.StatusOK || !strings.Contains(string(sendBody), "waiting") {
 		t.Fatalf("the send's first answer: status %d: %s", sendResp.StatusCode, sendBody)
 	}
-	statusResp := doWithCookie(t, mw, http.MethodGet, "/send-status?id=send-1", nil, cookie)
+	// Follow the send's own meta refresh (review round 1 finding 3), never ask /send-status by hand: this both
+	// exercises the real link a browser would follow and proves it names the right send (send-1, mail-store's
+	// own answer to mail_send).
+	statusURL := extractMetaRefreshURL(t, string(sendBody))
+	if statusURL != "/send-status?id=send-1" {
+		t.Fatalf("the send's own refresh: %q, want /send-status?id=send-1", statusURL)
+	}
+	statusResp := doWithCookie(t, mw, http.MethodGet, statusURL, nil, cookie)
 	statusBody, _ := io.ReadAll(statusResp.Body)
 	_ = statusResp.Body.Close()
 	if !strings.Contains(string(statusBody), "sent") {
@@ -365,9 +431,8 @@ func TestAcceptanceMailEscapingAttachmentDownloadAndCSP(t *testing.T) {
 	if !strings.Contains(body, "&lt;script&gt;") {
 		t.Fatalf("the mail's body is not shown as escaped text:\n%s", body)
 	}
-	if mailResp.Header.Get("Content-Security-Policy") != CSP {
-		t.Fatalf("the mail page's CSP: %q", mailResp.Header.Get("Content-Security-Policy"))
-	}
+	// The mail page's own CSP, and every other answer's (review round 1 finding 3): doWithCookie and doForm
+	// check it on every response they return, not only the ones a test asks for by hand.
 
 	attResp := doWithCookie(t, mw, http.MethodGet, "/attachment?account=a&folder=INBOX&uid=1&key=att1", nil, cookie)
 	attBody, _ := io.ReadAll(attResp.Body)
@@ -381,17 +446,34 @@ func TestAcceptanceMailEscapingAttachmentDownloadAndCSP(t *testing.T) {
 	if !strings.HasPrefix(attResp.Header.Get("Content-Disposition"), "attachment;") {
 		t.Fatalf("attachment Content-Disposition: %q", attResp.Header.Get("Content-Disposition"))
 	}
-	if attResp.Header.Get("Content-Security-Policy") != CSP {
-		t.Fatalf("the attachment's CSP: %q", attResp.Header.Get("Content-Security-Policy"))
-	}
 	if string(attBody) != "hello world" {
 		t.Fatalf("attachment body: %q", attBody)
 	}
 
-	// Every page answered carries the CSP: the 404 for another host does too (issue 1283/AC2).
-	otherHost := doWithCookie(t, mw, http.MethodGet, "/", nil, nil)
-	_ = otherHost.Body.Close()
-	if otherHost.Header.Get("Content-Security-Policy") != CSP {
-		t.Fatalf("the sign-in redirect's own CSP: %q", otherHost.Header.Get("Content-Security-Policy"))
+	// A cookieless GET still carries the CSP on its own bounce (checked inside doWithCookie too); and a
+	// request for a really different Host (review round 1 finding 3 — the step here previously asked the
+	// internal host by mistake, doWithCookie always setting it) is answered 404, and carries the CSP too
+	// (issue 1283/AC2).
+	bounceResp := doWithCookie(t, mw, http.MethodGet, "/", nil, nil)
+	_ = bounceResp.Body.Close()
+	if bounceResp.StatusCode != http.StatusOK {
+		t.Fatalf("a cookieless GET: status %d, want 200 (RequireSession's own one-time bounce)", bounceResp.StatusCode)
+	}
+
+	otherHostReq, err := http.NewRequest(http.MethodGet, "http://"+mw.internalHost+"/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	otherHostReq.Host = "example.com"
+	otherHostResp, err := mw.client.Do(otherHostReq)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer otherHostResp.Body.Close()
+	if otherHostResp.StatusCode != http.StatusNotFound {
+		t.Fatalf("Host example.com: status %d, want 404", otherHostResp.StatusCode)
+	}
+	if got := otherHostResp.Header.Get("Content-Security-Policy"); got != CSP {
+		t.Fatalf("Host example.com's own 404: CSP %q, want %q", got, CSP)
 	}
 }

modules/mesh-mail-web/cmd/mesh-mail-web/runtime_integration_test.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"html"
 	"io"
 	"net"
 	"net/http"
@@ -661,22 +662,34 @@ func waitForHTTP(t *testing.T, port int) {
 	t.Fatal("mesh-mail-web's HTTP endpoint did not come up within 30s")
 }
 
+// followSignInBounce is a plain GET to path carrying no session cookie, followed through RequireSession's own
+// one-time bounce (review round 1 finding 1: the first cookieless answer is a 200 page of this origin's own,
+// never a redirect — a browser's own meta refresh is what reaches the real one) exactly the way a browser
+// would: read the bounce's own refresh target and ask it. Answers that second response: the 302 to the
+// issuer, since the cookie is still missing on a request this client never carries one on.
+func followSignInBounce(t *testing.T, mw *startedMailWeb, path string) *http.Response {
+	t.Helper()
+	resp := doWithCookie(t, mw, http.MethodGet, path, nil, nil)
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("%s with no session: status %d, want 200 (RequireSession's own one-time bounce, review round 1 finding 1)", path, resp.StatusCode)
+	}
+	body, err := io.ReadAll(resp.Body)
+	_ = resp.Body.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return doWithCookie(t, mw, http.MethodGet, extractMetaRefreshURL(t, string(body)), nil, nil)
+}
+
 // signIn drives issue 1263's own flow through HTTP alone, as a browser would, never by calling a handler
-// directly: it asks path (any page behind RequireSession) with no session, reads the state the redirect to
-// the stand-in issuer carries, and calls back mesh-mail-web's own /signed-in with a fresh ID token the
-// issuer mints for username. Answers the session cookie mesh-mail-web set.
+// directly: it asks path (any page behind RequireSession) with no session, follows RequireSession's own
+// one-time bounce (review round 1 finding 1), reads the state the resulting redirect to the stand-in issuer
+// carries, and calls back mesh-mail-web's own /signed-in with a fresh ID token the issuer mints for username.
+// Answers the session cookie mesh-mail-web set — the callback's own answer is a 200 refresh page too
+// (finding 1), but Set-Cookie is read from it exactly the same way a 302's would have been.
 func signIn(t *testing.T, mw *startedMailWeb, username, path string) *http.Cookie {
 	t.Helper()
-	req, err := http.NewRequest(http.MethodGet, "http://"+mw.internalHost+path, nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	req.Host = mw.internalHost
-	resp, err := mw.client.Do(req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	_ = resp.Body.Close()
+	resp := followSignInBounce(t, mw, path)
 	if resp.StatusCode != http.StatusFound {
 		t.Fatalf("asking %s with no session: status %d, want 302 to sign in", path, resp.StatusCode)
 	}
@@ -710,8 +723,20 @@ func signIn(t *testing.T, mw *startedMailWeb, username, path string) *http.Cooki
 	return nil
 }
 
+// checkCSP is review round 1 finding 3's own strengthening: the content security policy (ADR 0440 §5) is
+// Server.ServeHTTP's own first line, set on every answer whatever else happens to it (a page, a 404, a
+// sign-in bounce or redirect alike) — so every request these tests drive through doWithCookie and doForm
+// checks it, rather than the three hand-picked answers the design test asked of it before.
+func checkCSP(t *testing.T, method, path string, resp *http.Response) {
+	t.Helper()
+	if got := resp.Header.Get("Content-Security-Policy"); got != CSP {
+		t.Fatalf("%s %s: Content-Security-Policy %q, want %q", method, path, got, CSP)
+	}
+}
+
 // doWithCookie is one request to mw's own endpoint, the session cookie attached, Host set to its internal
-// name — the shape every page, attachment and send request in these tests shares.
+// name — the shape every page, attachment and send request in these tests shares. Every answer's own CSP is
+// checked here (review round 1 finding 3), not left to a handful of tests to ask for themselves.
 func doWithCookie(t *testing.T, mw *startedMailWeb, method, path string, body io.Reader, cookie *http.Cookie) *http.Response {
 	t.Helper()
 	req, err := http.NewRequest(method, "http://"+mw.internalHost+path, body)
@@ -726,11 +751,12 @@ func doWithCookie(t *testing.T, mw *startedMailWeb, method, path string, body io
 	if err != nil {
 		t.Fatal(err)
 	}
+	checkCSP(t, method, path, resp)
 	return resp
 }
 
 // doForm is one form POST to mw's own endpoint, the session cookie attached when given — the shape issue
-// 1268's sign-in, CSRF and send scenarios share.
+// 1268's sign-in, CSRF and send scenarios share. Every answer's own CSP is checked here too (finding 3).
 func doForm(t *testing.T, mw *startedMailWeb, path string, form url.Values, cookie *http.Cookie) *http.Response {
 	t.Helper()
 	req, err := http.NewRequest(http.MethodPost, "http://"+mw.internalHost+path, strings.NewReader(form.Encode()))
@@ -746,9 +772,32 @@ func doForm(t *testing.T, mw *startedMailWeb, path string, form url.Values, cook
 	if err != nil {
 		t.Fatal(err)
 	}
+	checkCSP(t, http.MethodPost, path, resp)
 	return resp
 }
 
+// extractMetaRefreshURL reads a page's own meta refresh target out of its rendered HTML (review round 1
+// finding 1, finding 3): RequireSession's own bounce, the callback's own return, and the write page's own
+// send-status poll (write.go's sendStatusTemplate) all carry one, whatever its own delay. html/template's
+// contextual auto-escaping recognises content="...;url=..." inside a meta tag as a URL within an HTML
+// attribute, and escapes it as both (an "&" becomes "&"); html.UnescapeString reverses exactly that, so
+// this never builds the URL by hand — it reads the one the page itself carries, as a browser's own meta
+// refresh would.
+func extractMetaRefreshURL(t *testing.T, body string) string {
+	t.Helper()
+	marker := "; url="
+	i := strings.Index(body, marker)
+	if i < 0 {
+		t.Fatalf("no meta refresh in the page:\n%s", body)
+	}
+	rest := body[i+len(marker):]
+	j := strings.Index(rest, `"`)
+	if j < 0 {
+		t.Fatalf("the meta refresh's own url is not closed:\n%s", body)
+	}
+	return html.UnescapeString(rest[:j])
+}
+
 // extractHiddenValue reads a hidden input's own value out of a rendered page's HTML, by its field name —
 // issue 1284's own form token, which is never answered by any tool, only ever rendered into a page
 // (write.go's FormTokenInput).