From 3fd1b47fe7a047d7c4ded4d1d4bda128855a9966 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 23:18:48 +0200 Subject: [PATCH] Bounce once through this origin before sign-in, so a Strict cookie is not lost crossing from Keycloak or another site (issue 1263) --- .../mesh-mail-web/cmd/mesh-mail-web/auth.go | 76 +++++++- .../cmd/mesh-mail-web/auth_test.go | 162 +++++++++++++++--- 2 files changed, 212 insertions(+), 26 deletions(-) diff --git a/modules/mesh-mail-web/cmd/mesh-mail-web/auth.go b/modules/mesh-mail-web/cmd/mesh-mail-web/auth.go index 92ee1a97..4a43544a 100644 --- a/modules/mesh-mail-web/cmd/mesh-mail-web/auth.go +++ b/modules/mesh-mail-web/cmd/mesh-mail-web/auth.go @@ -14,6 +14,7 @@ import ( "encoding/json" "errors" "fmt" + "html/template" "net/http" "os" "strings" @@ -81,6 +82,54 @@ func NewAuthenticator(ctx context.Context, cfg OIDCConfig, redirectURL string) ( }, nil } +// BounceParam marks a request that already spent RequireSession's own one-time bounce (review round 1 of +// issue 1263, finding 1): a browser arriving with no session cookie at all may simply have none yet, or may +// hold a live one that this particular navigation does not carry — exactly what a cookie marked +// SameSite=Strict looks like on a navigation this browser does not count as same-site, which both Keycloak's +// own redirect to the callback and a link clicked from another site (Telegram) are, even across an otherwise +// live session. A page this module's own origin serves, whose own follow-on navigation (a meta refresh, not an +// HTTP redirect) is same-site, carries the cookie if there is one; a second cookieless arrival, marked by this +// parameter, means there truly is none, and goes to sign-in instead. Stripped from the return URL a sign-in +// keeps (returnURL below), so it never leaks into the page the browser ends up on. +const BounceParam = "mww-bounced" + +// bounceTemplate is both RequireSession's own one-time bounce and Callback's own return to the page first +// asked: the same shape, a 200 page this origin serves whose own navigation carries a Strict cookie that a +// redirect begun elsewhere (Keycloak's 302 to this callback) would not. +var bounceTemplate = template.Must(template.New("bounce").Parse(` +Mail +

Continue

`)) + +// renderBounce answers a 200 page whose own navigation goes to url, next (RequireSession's bounce, or +// Callback's own return once signed in). +func renderBounce(w http.ResponseWriter, url string) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _ = bounceTemplate.Execute(w, url) +} + +// returnURL is the page a sign-in should return to once it succeeds, or RequireSession's own bounce should +// repeat: r's own request URI, with BounceParam stripped (it is this module's own bookkeeping, never the page +// the browser should end up on). +func returnURL(r *http.Request) string { + u := *r.URL + if u.RawQuery != "" { + q := u.Query() + q.Del(BounceParam) + u.RawQuery = q.Encode() + } + return u.RequestURI() +} + +// bouncedURL is r's own request URI with BounceParam added, so a second cookieless arrival at the same page is +// told apart from the first. +func bouncedURL(r *http.Request) string { + u := *r.URL + q := u.Query() + q.Set(BounceParam, "1") + u.RawQuery = q.Encode() + return u.RequestURI() +} + // Auth wires sign-in into a Server: RequireSession protects a page, an attachment or a send (issue 1263/AC1), // and Callback is /signed-in. type Auth struct { @@ -109,14 +158,29 @@ func SessionFrom(r *http.Request) (Session, bool) { // RequireSession is issue 1263/AC1, AC5: no session, or one that ended 12 hours ago, is answered a redirect to // the issuer's authorisation endpoint (state, PKCE challenge, this module's own callback) rather than the page, // attachment or send asked for; a live session is handed on to next. +// +// A GET that carries no session cookie at all (review round 1 finding 1) is bounced once through this origin's +// own page first, rather than sent to sign in at once: a cookie marked SameSite=Strict is withheld from +// exactly this shape of request when the live session's own cookie would otherwise have been there (Keycloak's +// redirect to the callback, a link from another site), and this origin's own follow-on navigation is where it +// reappears. A cookie that did reach this request but names no live session (it expired, or never existed) is +// not that case — the browser did carry it — so it goes to sign in directly, as does any non-GET (a bounce +// would lose its body) and a second cookieless GET already marked by BounceParam. func (a *Auth) RequireSession(next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if cookie, err := r.Cookie(SessionCookie); err == nil { + cookie, err := r.Cookie(SessionCookie) + if err == nil { if sess, ok := a.Sessions.Get(cookie.Value); ok { ctx := context.WithValue(r.Context(), sessionKey{}, sess) next(w, r.WithContext(ctx)) return } + a.redirectToSignIn(w, r) + return + } + if r.Method == http.MethodGet && r.URL.Query().Get(BounceParam) == "" { + renderBounce(w, bouncedURL(r)) + return } a.redirectToSignIn(w, r) } @@ -124,7 +188,7 @@ func (a *Auth) RequireSession(next http.HandlerFunc) http.HandlerFunc { func (a *Auth) redirectToSignIn(w http.ResponseWriter, r *http.Request) { verifier := oauth2.GenerateVerifier() - state, err := a.Pending.Create(verifier, r.URL.RequestURI()) + state, err := a.Pending.Create(verifier, returnURL(r)) if err != nil { http.Error(w, "sign-in could not be started", http.StatusInternalServerError) return @@ -136,7 +200,11 @@ func (a *Auth) redirectToSignIn(w http.ResponseWriter, r *http.Request) { // Callback is /signed-in (issue 1263/AC2, AC3, AC4): a state this module did not issue, an exchange that // fails, an ID token of another audience, signed by another key, or expired, or an identity operators does not // name, is answered 403 and makes no session. Only once the ID token verifies and names an operator is a -// session made, its cookie set, and the browser sent back to the page it first asked for. +// session made, its cookie set, and the browser sent back to the page it first asked for — through a 200 page +// of this origin's own, not a 302 (review round 1 finding 1): the navigation that landed here started at +// Keycloak, so a redirect onward from it is still, in the browser's own eyes, a navigation Keycloak began, and +// a cookie marked SameSite=Strict (just set by this very answer) is not sent on it or on anything it redirects +// to in turn. A follow-on navigation this page itself starts has no such history, so the cookie is sent on it. func (a *Auth) Callback(w http.ResponseWriter, r *http.Request) { refuse := func(why string) { a.logf("[mesh-mail-web] sign-in refused: %s", why) @@ -203,5 +271,5 @@ func (a *Auth) Callback(w http.ResponseWriter, r *http.Request) { if returnTo == "" { returnTo = "/" } - http.Redirect(w, r, returnTo, http.StatusFound) + renderBounce(w, returnTo) } diff --git a/modules/mesh-mail-web/cmd/mesh-mail-web/auth_test.go b/modules/mesh-mail-web/cmd/mesh-mail-web/auth_test.go index bec8539f..2112e601 100644 --- a/modules/mesh-mail-web/cmd/mesh-mail-web/auth_test.go +++ b/modules/mesh-mail-web/cmd/mesh-mail-web/auth_test.go @@ -12,6 +12,7 @@ import ( "crypto/sha256" "encoding/base64" "encoding/json" + "html" "math/big" "net/http" "net/http/httptest" @@ -140,8 +141,45 @@ func operatorsFile(t *testing.T, names ...string) string { return path } +// extractRefreshURL reads a bounce or callback page's own meta refresh target out of its rendered HTML +// (review round 1 finding 1): html/template's own attribute escaping turns "&" into "&" in the URL it +// carries (its contextual auto-escaping recognises content="...;url=..." as a URL inside an HTML attribute, +// and escapes it as both), so this reverses exactly that, never by building the URL by hand. +func extractRefreshURL(t *testing.T, body string) string { + t.Helper() + marker := `content="0; 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]) +} + +// followBounce is RequireSession's own one-time bounce (review round 1 finding 1), followed the way a +// browser's own meta refresh would: w is the first, cookieless answer (a 200 page of this origin's own, +// never a redirect); this reads its refresh target and asks it, answering that second response — the real +// 302 to the issuer, since the cookie is still missing on a request this player never carries one on. +func followBounce(t *testing.T, s *Server, w *httptest.ResponseRecorder, host string) *httptest.ResponseRecorder { + t.Helper() + if w.Code != http.StatusOK { + t.Fatalf("the bounce: status %d, want 200 (a page of this origin's own, review round 1 finding 1): %s", w.Code, w.Body.String()) + } + refreshURL := extractRefreshURL(t, w.Body.String()) + w2 := httptest.NewRecorder() + r2 := httptest.NewRequest(http.MethodGet, refreshURL, nil) + r2.Host = host + s.ServeHTTP(w2, r2) + return w2 +} + // 1263/AC1: no session: a redirect to the issuer's authorisation endpoint with the client id, a state, a PKCE -// challenge, and the redirect https://mail..internal/signed-in. +// challenge, and the redirect https://mail..internal/signed-in — reached only after RequireSession's own +// one-time bounce (review round 1 finding 1), as a browser would, never asked for directly. func TestNoSessionRedirectsToTheIssuersAuthorisationEndpoint(t *testing.T) { fi := newFakeIssuer(t) auth := testAuth(t, fi, operatorsFile(t, "alice"), nil) @@ -151,11 +189,12 @@ func TestNoSessionRedirectsToTheIssuersAuthorisationEndpoint(t *testing.T) { r := httptest.NewRequest(http.MethodGet, "/", nil) r.Host = InternalHost("g14") s.ServeHTTP(w, r) + w2 := followBounce(t, s, w, InternalHost("g14")) - if w.Code != http.StatusFound { - t.Fatalf("status %d, want 302", w.Code) + if w2.Code != http.StatusFound { + t.Fatalf("status %d, want 302", w2.Code) } - loc, err := url.Parse(w.Header().Get("Location")) + loc, err := url.Parse(w2.Header().Get("Location")) if err != nil { t.Fatal(err) } @@ -177,15 +216,89 @@ func TestNoSessionRedirectsToTheIssuersAuthorisationEndpoint(t *testing.T) { } } -// startSignIn drives the redirect step for a test, answering the state and PKCE verifier it issued (read back -// from the pending store by the state the redirect carried), as the next call (the callback) needs them. -func startSignIn(t *testing.T, s *Server, auth *Auth) (state string) { - t.Helper() +// review round 1 finding 1: a GET with no session cookie is bounced once through this origin's own page (a +// 200 with a meta refresh back to the very page asked, marked by BounceParam) rather than sent to the issuer +// at once; a second cookieless arrival at that same marked URL goes to sign in directly — no loop — and the +// sign-in's own kept return is the page first asked, with the bounce's own marker stripped so it never leaks +// into where the browser ends up. A POST with no session cookie is never bounced (a bounce would lose its +// body): it goes to sign in directly, the same as it always has. +func TestRequireSessionBouncesOnceThenSignsIn(t *testing.T) { + fi := newFakeIssuer(t) + auth := testAuth(t, fi, operatorsFile(t, "alice"), nil) + s := NewServer(InternalHost("g14"), auth, nil) + w := httptest.NewRecorder() - r := httptest.NewRequest(http.MethodGet, "/", nil) + r := httptest.NewRequest(http.MethodGet, "/folder?account=a&folder=INBOX", nil) r.Host = InternalHost("g14") s.ServeHTTP(w, r) - loc, err := url.Parse(w.Header().Get("Location")) + if w.Code != http.StatusOK { + t.Fatalf("the first cookieless GET: status %d, want 200 (the bounce)", w.Code) + } + refreshURL := extractRefreshURL(t, w.Body.String()) + if !strings.HasPrefix(refreshURL, "/folder?") { + t.Fatalf("the bounce does not return to the same page: %q", refreshURL) + } + q, err := url.ParseQuery(strings.SplitN(refreshURL, "?", 2)[1]) + if err != nil { + t.Fatal(err) + } + if q.Get(BounceParam) == "" { + t.Fatalf("the bounce's own refresh does not mark itself: %q", refreshURL) + } + if q.Get("account") != "a" || q.Get("folder") != "INBOX" { + t.Fatalf("the bounce lost the page's own query: %q", refreshURL) + } + + // A second cookieless arrival, at the marked URL: sign in directly, no further bounce (no loop). + w2 := httptest.NewRecorder() + r2 := httptest.NewRequest(http.MethodGet, refreshURL, nil) + r2.Host = InternalHost("g14") + s.ServeHTTP(w2, r2) + if w2.Code != http.StatusFound { + t.Fatalf("the second cookieless GET: status %d, want 302 to sign in (no loop)", w2.Code) + } + loc, err := url.Parse(w2.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(loc.String(), fi.Server.URL+"/authorize") { + t.Fatalf("the second cookieless GET did not go to the issuer: %s", loc) + } + + // The sign-in's own kept return is the page first asked, the bounce's own marker gone. + pending, ok := auth.Pending.Take(loc.Query().Get("state")) + if !ok { + t.Fatal("no pending sign-in kept under that state") + } + if pending.Return != "/folder?account=a&folder=INBOX" { + t.Fatalf("the sign-in's own return: %q, want the page first asked with no bounce marker", pending.Return) + } + + // A POST with no session cookie: sign in directly, never bounced. + w3 := httptest.NewRecorder() + r3 := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("")) + r3.Host = InternalHost("g14") + s.ServeHTTP(w3, r3) + if w3.Code != http.StatusFound { + t.Fatalf("a POST with no session cookie: status %d, want 302 (no bounce)", w3.Code) + } +} + +// startSignIn drives the real shape of starting a sign-in from path, as a browser would (review round 1 +// finding 2): the first cookieless GET is bounced once (RequireSession's own fix, finding 1); following that +// bounce — reading its own meta refresh and asking it, never building the issuer's URL by hand — reaches the +// real 302 to the issuer, whose state and PKCE verifier the callback needs. +func startSignIn(t *testing.T, s *Server, auth *Auth, path string) (state string) { + t.Helper() + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, path, nil) + r.Host = InternalHost("g14") + s.ServeHTTP(w, r) + w2 := followBounce(t, s, w, InternalHost("g14")) + if w2.Code != http.StatusFound { + t.Fatalf("after the bounce: status %d, want 302 to the issuer", w2.Code) + } + loc, err := url.Parse(w2.Header().Get("Location")) if err != nil { t.Fatal(err) } @@ -201,25 +314,30 @@ func callback(s *Server, state, code string) *httptest.ResponseRecorder { } // 1263/AC2: a valid callback whose ID token names an operator sets a session cookie Secure, HttpOnly, -// SameSite=Strict, lasting 12 hours, and sends the browser back to the page it first asked for. +// SameSite=Strict, lasting 12 hours, and sends the browser back to the page it first asked for — a page the +// test starts sign-in from that is not also the no-pending-return fallback (review round 1 finding 2: "/" is +// both, so a return to it would pass even with pending.Return ignored), through a 200 refresh page, not a 302 +// (finding 1: a 302 here would carry the Strict cookie just set on a navigation the browser still counts as +// begun at Keycloak). func TestACallbackWithAValidIDTokenNamingAnOperatorSetsTheSessionCookie(t *testing.T) { fi := newFakeIssuer(t) now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) auth := testAuth(t, fi, operatorsFile(t, "alice"), func() time.Time { return now }) s := NewServer(InternalHost("g14"), auth, nil) - state := startSignIn(t, s, auth) + const returnPath = "/folder?account=a&folder=INBOX" + state := startSignIn(t, s, auth, returnPath) // The session clock (now) is mocked to test the 12-hour bound (TestASessionTwelveHoursOldIsTreatedAsNoSession // below); the ID token's own iat/exp are judged by go-oidc against the real wall clock, so they are minted // from it, not from the mocked now. fi.nextIDToken = fi.idToken("mail-web", "alice", time.Now(), nil, fi.key, fi.kid) w := callback(s, state, "some-code") - if w.Code != http.StatusFound { - t.Fatalf("status %d, want 302: %s", w.Code, w.Body.String()) + if w.Code != http.StatusOK { + t.Fatalf("status %d, want 200 (a refresh page, not a redirect): %s", w.Code, w.Body.String()) } - if loc := w.Header().Get("Location"); loc != "/" { - t.Fatalf("returned to %q, want the page first asked for (/)", loc) + if refreshURL := extractRefreshURL(t, w.Body.String()); refreshURL != returnPath { + t.Fatalf("returned to %q, want the page first asked for (%s)", refreshURL, returnPath) } var found *http.Cookie for _, c := range w.Result().Cookies() { @@ -245,7 +363,7 @@ func TestACallbackForAnIdentityNotInOperatorsIsRefused(t *testing.T) { auth := testAuth(t, fi, operatorsFile(t, "alice"), func() time.Time { return now }) s := NewServer(InternalHost("g14"), auth, nil) - state := startSignIn(t, s, auth) + state := startSignIn(t, s, auth, "/") fi.nextIDToken = fi.idToken("mail-web", "mallory", now, nil, fi.key, fi.kid) w := callback(s, state, "some-code") @@ -274,11 +392,11 @@ func TestACallbackIsRefusedOnAnUnknownStateOrAnInvalidIDToken(t *testing.T) { }{ {"unknown state", func(auth *Auth, s *Server) string { return "a-state-nobody-issued" }, func() string { return fi.idToken("mail-web", "alice", now, nil, fi.key, fi.kid) }}, - {"wrong audience", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth) }, + {"wrong audience", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth, "/") }, func() string { return fi.idToken("someone-else", "alice", now, nil, fi.key, fi.kid) }}, - {"wrong signing key", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth) }, + {"wrong signing key", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth, "/") }, func() string { return fi.idToken("mail-web", "alice", now, nil, otherKey, fi.kid) }}, - {"expired", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth) }, + {"expired", func(auth *Auth, s *Server) string { return startSignIn(t, s, auth, "/") }, func() string { return fi.idToken("mail-web", "alice", now, map[string]any{"exp": now.Add(-time.Hour).Unix()}, fi.key, fi.kid) }}, @@ -309,7 +427,7 @@ func TestASessionTwelveHoursOldIsTreatedAsNoSession(t *testing.T) { auth := testAuth(t, fi, operatorsFile(t, "alice"), func() time.Time { return now }) s := NewServer(InternalHost("g14"), auth, nil) - state := startSignIn(t, s, auth) + state := startSignIn(t, s, auth, "/") // The ID token's own iat/exp are judged against the real wall clock (see the comment in // TestACallbackWithAValidIDTokenNamingAnOperatorSetsTheSessionCookie above); the session's own age is what // this test mocks, through the session clock (now). @@ -355,7 +473,7 @@ func TestCallbackWithEmptyOperatorsLetsNobodyIn(t *testing.T) { auth := testAuth(t, fi, operatorsFile(t), func() time.Time { return now }) s := NewServer(InternalHost("g14"), auth, nil) - state := startSignIn(t, s, auth) + state := startSignIn(t, s, auth, "/") fi.nextIDToken = fi.idToken("mail-web", "alice", now, nil, fi.key, fi.kid) w := callback(s, state, "some-code") if w.Code != http.StatusForbidden {