From 9f3a8ee391013593c378a1080d4ba998cc798bdf Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 23:18:53 +0200 Subject: [PATCH] Follow real links instead of building them, and check the CSP on every answer (issue 1268) --- .../acceptance_integration_test.go | 150 ++++++++++++++---- .../mesh-mail-web/runtime_integration_test.go | 79 +++++++-- 2 files changed, 180 insertions(+), 49 deletions(-) diff --git a/modules/mesh-mail-web/cmd/mesh-mail-web/acceptance_integration_test.go b/modules/mesh-mail-web/cmd/mesh-mail-web/acceptance_integration_test.go index 2b8aa626..4d48c17e 100644 --- a/modules/mesh-mail-web/cmd/mesh-mail-web/acceptance_integration_test.go +++ b/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" + 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", 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+"") { + 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") { + 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, "<script>") { 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) } } diff --git a/modules/mesh-mail-web/cmd/mesh-mail-web/runtime_integration_test.go b/modules/mesh-mail-web/cmd/mesh-mail-web/runtime_integration_test.go index a1c2417e..ae281ee6 100644 --- a/modules/mesh-mail-web/cmd/mesh-mail-web/runtime_integration_test.go +++ b/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).