Fix logout to not use side-effect-GETs

docs/auth-cross-host.md

@@ -54,10 +54,30 @@ auth, root beacon, custom org domains) stores `login_id` in its scs `data` blob
 and is validated against the parent on each request.
 
 - **Logout = one write:** set `revoked_at` on the `logins` row. Every host drops
-  to anonymous on its next request — including custom org domains, which the old
+  to anonymous on its next request (`ValidateSession` destroys any session whose
+  backing login is revoked) — including custom org domains, which the old
   redirect-chain logout never covered.
 - Per-device logout = revoke one row; "log out everywhere" = revoke all of a
   user's rows.
 - Do **not** add per-host token columns to the scs `sessions` table — scs
   resolves a cookie by its single `token` PK, and the host set is unbounded
   (every custom domain is another host).
+
+### Logout is a per-host POST, never a cross-host GET chain
+
+Sign-out is a **POST** on the host that holds the session (app, auth, or a custom
+org domain). That handler revokes the shared login row and lands the visitor on
+the public root. Because a single real sign-in maps to exactly one login row
+(the handoff callbacks reuse it via `loginWithID`), revoking it once on *any* of
+those hosts drops all of them on their next request — so there is **no** redirect
+chain from the app host to an auth-host `/logout`. The auth host's own POST
+`/logout` still exists to cover the auth-local case: a visitor who authenticated
+on the auth host (e.g. for account settings) with no app session signs out there
+directly.
+
+`/logout` is **POST-only** on every host — state-changing actions are POST
+(rule 3's spirit applied everywhere), so a forged cross-site GET like
+`<img src=".../logout">` cannot sign anyone out. The template picks the right
+target via `LogoutURL` (relative `/logout` on mutating surfaces); the **root
+host** is side-effect-free GET and therefore has no `/logout` at all — its chrome
+hides the sign-out control and the user signs out from the app dashboard.

internal/auth/web.go

@@ -43,8 +43,10 @@ func (s *Handlers) Routes() chi.Router {
 	s.baseMW(r)
 	s.SharedRoutes(r)
 	s.credentialRoutes(r)
-	// Single logout: clears the SSO session (chained to from the app host).
-	r.Get("/logout", s.handleAuthLogout)
+	// Logout: revokes the login row (dropping every host to anonymous on its next
+	// request) and clears this host's SSO session. POST-only — sign-out is a state
+	// change, so a forged cross-site GET can't trigger it.
+	r.Post("/logout", s.handleAuthLogout)
 
 	// Account/credential management lives here so credentials are created on the
 	// same origin used to sign in. Guarded by the SSO session.
@@ -118,16 +120,11 @@ func (s *Handlers) pageSignup(w http.ResponseWriter, r *http.Request) {
 	})
 }
 
-// handleAuthLogout clears the auth host's SSO session and lands the visitor on
-// the public root (or the app in narrower configs).
+// handleAuthLogout revokes the login row backing this session and lands the
+// visitor on the public root. Revoking the shared row is what signs the user out
+// of every host (app, root beacon, custom org domains) on their next request; it
+// also directly clears an auth-local SSO session that never had an app session.
 func (s *Handlers) handleAuthLogout(w http.ResponseWriter, r *http.Request) {
 	_ = s.Auth.Logout(r.Context())
-	switch {
-	case s.Cfg.RootHost != "":
-		http.Redirect(w, r, s.Origin(r, s.Cfg.RootHost)+"/", http.StatusSeeOther) //nolint:gosec // G710: local path or config-pinned origin
-	case s.Cfg.PrimaryHost != "":
-		http.Redirect(w, r, s.Origin(r, s.Cfg.PrimaryHost)+"/", http.StatusSeeOther) //nolint:gosec // G710: local path or config-pinned origin
-	default:
-		http.Redirect(w, r, "/login", http.StatusSeeOther)
-	}
+	s.RedirectAfterLogout(w, r)
 }

internal/core/handoff_test.go

@@ -240,51 +240,115 @@ func TestHostRouting(t *testing.T) {
 	})
 }
 
-// TestSingleLogout verifies sign-out clears BOTH sessions: the app host's
-// /logout chains to the auth host, and the auth host's /logout destroys the SSO
-// session so it can't silently re-authenticate.
+// TestSingleLogout verifies the single-revoke logout model: one POST to a host's
+// /logout revokes the shared login row, dropping every host bound to it. An
+// auth-host sign-in is handed off to the app so both sessions share one login row;
+// logging out on the APP host then invalidates the AUTH host's SSO session on its
+// next request — with no redirect chain between them. See docs/auth-cross-host.md.
 func TestSingleLogout(t *testing.T) {
 	t.Parallel()
-	st, ctx, ts, h := splitServer(t)
+	_, _, ts, h := splitServer(t)
 
-	// An app session (via the handoff) logs out by bouncing to the auth host.
-	u, _ := st.CreateUser(ctx, "logoutuser", "")
-	loginID, _ := st.CreateLogin(ctx, u.ID)
-	code, _ := st.CreateAuthCode(ctx, u.ID, loginID, "/")
+	// Sign up on the auth host: creates login row L and the auth SSO session, then
+	// hands off to the app with a single-use code carrying L.
+	su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"sharuser"}, "password": {"supersecret"}})
+	su.Body.Close()
+	sso := cookieByName(su, "meshtender_session")
+	if sso == nil {
+		t.Fatal("expected an SSO session from signup")
+	}
+	loc, _ := url.Parse(su.Header.Get("Location"))
+	code := loc.Query().Get("code")
+	if code == "" {
+		t.Fatalf("signup did not hand off to the app (Location %q)", su.Header.Get("Location"))
+	}
+	// Redeem the code on the app host, binding its session to the SAME row L.
 	cb := do(t, ts, h.app, "/session/callback?code="+code+"&state=s1", &http.Cookie{Name: "mt_state", Value: "s1"})
 	cb.Body.Close()
 	appSess := cookieByName(cb, "meshtender_session")
 	if appSess == nil {
 		t.Fatal("expected an app session from the callback")
 	}
-	out := post(t, ts, h.app, "/logout", url.Values{}, appSess)
-	out.Body.Close()
-	if loc, _ := url.Parse(out.Header.Get("Location")); out.StatusCode != http.StatusSeeOther || loc.Host != h.auth || loc.Path != "/logout" {
-		t.Fatalf("app logout = %d %q, want 303 to auth /logout", out.StatusCode, out.Header.Get("Location"))
+
+	// Both live: auth /login re-hands-off (303) rather than showing the form.
+	live := do(t, ts, h.auth, "/login?next=%2F&state=x", sso)
+	live.Body.Close()
+	if live.StatusCode != http.StatusSeeOther {
+		t.Fatalf("pre-logout auth /login = %d, want 303 handoff (SSO live)", live.StatusCode)
 	}
 
-	// Sign up on the auth host to hold an SSO session, then confirm it is live
-	// (auth /login hands off) until /logout, after which it no longer does.
-	su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"ssouser"}, "password": {"supersecret"}})
+	// Log out on the APP host: it revokes L and lands on the public root — no hop
+	// through the auth host.
+	out := post(t, ts, h.app, "/logout", url.Values{}, appSess)
+	out.Body.Close()
+	if loc, _ := url.Parse(out.Header.Get("Location")); out.StatusCode != http.StatusSeeOther || loc.Hostname() != testRootHost {
+		t.Fatalf("app logout = %d %q, want 303 to the root host", out.StatusCode, out.Header.Get("Location"))
+	}
+
+	// The AUTH host's SSO session is now dead too: /login shows the form (200)
+	// instead of re-handing-off, because ValidateSession dropped the revoked login.
+	after := do(t, ts, h.auth, "/login", sso)
+	after.Body.Close()
+	if after.StatusCode != http.StatusOK {
+		t.Fatalf("post-logout auth /login = %d, want 200 (SSO cleared via shared-login revoke)", after.StatusCode)
+	}
+}
+
+// TestAuthLogoutClearsSSO covers the auth host's own POST /logout: a visitor who
+// authenticated on the auth host (e.g. for account settings) with no app session
+// signs out there directly, revoking and clearing the SSO session.
+func TestAuthLogoutClearsSSO(t *testing.T) {
+	t.Parallel()
+	_, _, ts, h := splitServer(t)
+
+	su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"ssoonly"}, "password": {"supersecret"}})
 	su.Body.Close()
 	sso := cookieByName(su, "meshtender_session")
 	if sso == nil {
 		t.Fatal("expected an SSO session from signup")
 	}
-	live := do(t, ts, h.auth, "/login?next=%2F&state=x", sso)
-	live.Body.Close()
-	if live.StatusCode != http.StatusSeeOther {
-		t.Fatalf("authed /login = %d, want 303 handoff (SSO live)", live.StatusCode)
-	}
-	lo := do(t, ts, h.auth, "/logout", sso)
-	lo.Body.Close()
-	if lo.StatusCode != http.StatusSeeOther {
-		t.Fatalf("auth /logout = %d, want 303", lo.StatusCode)
+
+	out := post(t, ts, h.auth, "/logout", url.Values{}, sso)
+	out.Body.Close()
+	if loc, _ := url.Parse(out.Header.Get("Location")); out.StatusCode != http.StatusSeeOther || loc.Hostname() != testRootHost {
+		t.Fatalf("auth logout = %d %q, want 303 to the root host", out.StatusCode, out.Header.Get("Location"))
 	}
+
 	after := do(t, ts, h.auth, "/login", sso)
-	defer after.Body.Close()
+	after.Body.Close()
 	if after.StatusCode != http.StatusOK {
-		t.Fatalf("post-logout /login = %d, want 200 (SSO cleared, no re-handoff)", after.StatusCode)
+		t.Fatalf("post-logout auth /login = %d, want 200 (SSO cleared)", after.StatusCode)
+	}
+}
+
+// TestLogoutRejectsGet locks in that /logout is POST-only on both the auth and app
+// hosts: a forged cross-site GET (e.g. <img src=".../logout">) must not sign
+// anyone out. This is the rule (state-changing actions are POST) the old
+// side-effecting GET /logout violated.
+func TestLogoutRejectsGet(t *testing.T) {
+	t.Parallel()
+	_, _, ts, h := splitServer(t)
+
+	su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"getlogout"}, "password": {"supersecret"}})
+	su.Body.Close()
+	sso := cookieByName(su, "meshtender_session")
+	if sso == nil {
+		t.Fatal("expected an SSO session from signup")
+	}
+
+	for _, host := range []string{h.auth, h.app} {
+		g := do(t, ts, host, "/logout", sso)
+		g.Body.Close()
+		if g.StatusCode != http.StatusMethodNotAllowed {
+			t.Fatalf("GET %s/logout = %d, want 405 (POST-only)", host, g.StatusCode)
+		}
+	}
+
+	// The GET left the session untouched: auth /login still re-hands-off (SSO live).
+	after := do(t, ts, h.auth, "/login?next=%2F&state=x", sso)
+	after.Body.Close()
+	if after.StatusCode != http.StatusSeeOther {
+		t.Fatalf("auth /login after GET /logout = %d, want 303 (SSO still live)", after.StatusCode)
 	}
 }
 

internal/core/templates/invite.html

@@ -44,8 +44,7 @@
             <form method="post" action="/invite/{{.Token}}/accept" class="m-0">
               <button type="submit" class="btn btn-primary">Accept as {{.UserName}}</button>
             </form>
-            <form method="post" action="/logout" class="m-0">
-              <input type="hidden" name="next" value="/invite/{{.Token}}">
+            <form method="post" action="{{.LogoutURL}}" class="m-0">
               <button type="submit" class="btn btn-link">Switch account</button>
             </form>
           </div>

internal/core/web.go

@@ -366,10 +366,11 @@ func splitOwnedShared(repeaters []*store.Repeater) (owned, shared []*store.Repea
 	return owned, shared
 }
 
-// handleLogout signs the user out of the app host, then chains to the auth host's
-// /logout to clear the SSO session too — otherwise the surviving SSO session
-// would silently re-authenticate on the next request.
+// handleLogout signs the user out. It revokes the login row backing the session,
+// which drops every host sharing it (auth SSO, root beacon, custom org domains)
+// to anonymous on their next request — so no redirect chain to the auth host is
+// needed. See docs/auth-cross-host.md's global logout model.
 func (s *Handlers) handleLogout(w http.ResponseWriter, r *http.Request) {
 	_ = s.Auth.Logout(r.Context())
-	http.Redirect(w, r, s.Origin(r, s.Cfg.AuthHost)+"/logout", http.StatusSeeOther) //nolint:gosec // G710: local path or config-pinned origin
+	s.RedirectAfterLogout(w, r)
 }

internal/e2e/logout_test.go

@@ -0,0 +1,75 @@
+//go:build browser
+
+package e2e
+
+import (
+	"strings"
+	"testing"
+
+	cdplog "github.com/chromedp/cdproto/log"
+	"github.com/chromedp/cdproto/network"
+	"github.com/chromedp/cdproto/runtime"
+	"github.com/chromedp/chromedp"
+)
+
+// TestE2ELogout proves sign-out works in a real browser: the signed-in page
+// chrome carries a POST /logout form, submitting it clears the session, and a
+// protected page then bounces the (now anonymous) browser to sign-in. Driving the
+// endpoint with same-origin fetch (rather than clicking through) keeps the test
+// independent of where the single-host harness lands after logout, while still
+// exercising real browser cookies and the strict CSP (connect-src 'self').
+func TestE2ELogout(t *testing.T) {
+	srv := newE2EServer(t)
+	_, cookie := srv.login(t, "logoutuser")
+
+	bctx, cancel, watch := startBrowser(t)
+	defer cancel()
+
+	dash := srv.browserURL + "/"
+
+	// Log out, then probe a protected page — both with redirect:'manual' so a
+	// sign-in bounce surfaces as an opaqueredirect instead of being followed.
+	const logoutAndProbe = `(async () => {
+		const lo = await fetch('/logout', { method: 'POST', redirect: 'manual' });
+		const prot = await fetch('/repeaters', { redirect: 'manual' });
+		return { logoutType: lo.type, protType: prot.type, protStatus: prot.status };
+	})()`
+	awaitPromise := func(p *runtime.EvaluateParams) *runtime.EvaluateParams {
+		return p.WithAwaitPromise(true)
+	}
+
+	var formAction string
+	var result struct {
+		LogoutType string `json:"logoutType"`
+		ProtType   string `json:"protType"`
+		ProtStatus int    `json:"protStatus"`
+	}
+	if err := chromedp.Run(bctx,
+		network.Enable(),
+		cdplog.Enable(),
+		setSessionCookie(cookie),
+		chromedp.Navigate(dash),
+		// The sign-out form lives in the user-menu dropdown (present in the DOM even
+		// while the dropdown is collapsed).
+		chromedp.WaitReady(`[data-testid="logout-form"]`, chromedp.ByQuery),
+		chromedp.AttributeValue(`[data-testid="logout-form"]`, "action", &formAction, nil),
+		chromedp.Evaluate(logoutAndProbe, &result, awaitPromise),
+	); err != nil {
+		t.Fatalf("browser run against %s: %v", dash, err)
+	}
+
+	if !strings.HasSuffix(formAction, "/logout") {
+		t.Fatalf("sign-out form action = %q, want it to POST to /logout", formAction)
+	}
+	// The POST was accepted and redirected (not 405/blocked): a manual-redirect
+	// fetch reports an opaqueredirect for the 303.
+	if result.LogoutType != "opaqueredirect" {
+		t.Fatalf("POST /logout fetch type = %q, want opaqueredirect (accepted + redirected)", result.LogoutType)
+	}
+	// After logout the protected page no longer returns 200 — the browser is
+	// bounced to sign-in (another opaqueredirect), proving the session was cleared.
+	if result.ProtType != "opaqueredirect" {
+		t.Fatalf("post-logout /repeaters type = %q (status %d), want a sign-in redirect", result.ProtType, result.ProtStatus)
+	}
+	watch.assertClean(t)
+}

internal/web/env.go

@@ -87,6 +87,21 @@ func (e *Env) Origin(r *http.Request, host string) string {
 	return originFor(e.Cfg, r, host)
 }
 
+// RedirectAfterLogout lands a signed-out visitor on the public root (or the app,
+// or the local sign-in page in narrower configs). Shared by every host's POST
+// /logout so sign-out ends in the same place regardless of which surface it was
+// triggered from.
+func (e *Env) RedirectAfterLogout(w http.ResponseWriter, r *http.Request) {
+	switch {
+	case e.Cfg.RootHost != "":
+		http.Redirect(w, r, e.Origin(r, e.Cfg.RootHost)+"/", http.StatusSeeOther) //nolint:gosec // G710: config-pinned origin
+	case e.Cfg.PrimaryHost != "":
+		http.Redirect(w, r, e.Origin(r, e.Cfg.PrimaryHost)+"/", http.StatusSeeOther) //nolint:gosec // G710: config-pinned origin
+	default:
+		http.Redirect(w, r, "/login", http.StatusSeeOther)
+	}
+}
+
 func originFor(cfg *config.Config, r *http.Request, host string) string {
 	scheme := "http"
 	if cfg.Secure {
@@ -187,6 +202,15 @@ func (rn *Renderer) Render(w http.ResponseWriter, r *http.Request, page string,
 			data["RootURL"] = originFor(rn.cfg, r, rn.cfg.RootHost)
 		}
 	}
+	// LogoutURL: sign-out is a POST that revokes the login row, so it must target a
+	// host that owns a /logout endpoint AND holds this browser's session. The app
+	// host, auth host, and custom org domains all do (relative "/logout", a
+	// same-host POST). The root host is strictly side-effect-free GET (see
+	// docs/auth-cross-host.md), so it has no logout of its own — the template hides
+	// the control there and the user signs out from the app dashboard instead.
+	if rn.cfg.RootHost == "" || HostWithoutPort(r.Host) != rn.cfg.RootHost {
+		data["LogoutURL"] = "/logout"
+	}
 	if rn.userInfo != nil {
 		if name, canAdmin, ok := rn.userInfo(r.Context()); ok {
 			data["UserName"] = name

internal/web/templates/base.html

@@ -31,10 +31,12 @@
             </a>
             <div class="dropdown-menu dropdown-menu-end dropdown-menu-arrow">
               <a class="dropdown-item" href="{{.AuthURL}}/account">{{template "icon-settings" "dropdown-item-icon"}}Account settings</a>
+              {{if .LogoutURL}}
               <div class="dropdown-divider"></div>
-              <form method="post" action="{{.AppURL}}/logout" class="m-0">
+              <form method="post" action="{{.LogoutURL}}" class="m-0" data-testid="logout-form">
                 <button type="submit" class="dropdown-item text-danger">{{template "icon-logout" "dropdown-item-icon"}}Sign out</button>
               </form>
+              {{end}}
             </div>
           </div>
           {{else}}