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 {