novox/mesh-catalog
13
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

Bounce once through this origin before sign-in, so a Strict cookie is not lost crossing from Keycloak or another site (issue 1263)

Browse Source
This commit is contained in:
jochen
2026-10-19 23:18:48 +02:00
parent 5b57422523
commit 3fd1b47fe7
2 changed files with 212 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/mesh-mail-web/cmd/mesh-mail-web/auth.go
+72 -4
View File
@@ -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(`<!DOCTYPE html>
<html><head><title>Mail</title><meta http-equiv="refresh" content="0; url={{.}}"></head>
<body><p><a href="{{.}}">Continue</a></p></body></html>`))
// 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)
}
modules/mesh-mail-web/cmd/mesh-mail-web/auth_test.go
+140 -22
View File
@@ -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 "&amp;" 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.<node>.internal/signed-in.
// challenge, and the redirect https://mail.<node>.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 {