From 8e841b84322e5b4df9cdd51c06363f5f6f579428 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Thu, 2 Jul 2026 19:46:26 -0400 Subject: [PATCH] Fix logout to not use side-effect-GETs --- docs/auth-cross-host.md | 22 +++++- internal/auth/web.go | 21 +++-- internal/core/handoff_test.go | 116 +++++++++++++++++++++------- internal/core/templates/invite.html | 3 +- internal/core/web.go | 9 ++- internal/e2e/logout_test.go | 75 ++++++++++++++++++ internal/web/env.go | 24 ++++++ internal/web/templates/base.html | 4 +- 8 files changed, 228 insertions(+), 46 deletions(-) create mode 100644 internal/e2e/logout_test.go diff --git a/docs/auth-cross-host.md b/docs/auth-cross-host.md index 3e754ab..280e262 100644 --- a/docs/auth-cross-host.md +++ b/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 +`` 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. diff --git a/internal/auth/web.go b/internal/auth/web.go index b463783..3c13372 100644 --- a/internal/auth/web.go +++ b/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) } diff --git a/internal/core/handoff_test.go b/internal/core/handoff_test.go index 6ae21f2..6dc87e4 100644 --- a/internal/core/handoff_test.go +++ b/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. ) 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) } } diff --git a/internal/core/templates/invite.html b/internal/core/templates/invite.html index 15674ce..84e009d 100644 --- a/internal/core/templates/invite.html +++ b/internal/core/templates/invite.html @@ -44,8 +44,7 @@
-
- +
diff --git a/internal/core/web.go b/internal/core/web.go index c616621..e5e1a31 100644 --- a/internal/core/web.go +++ b/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) } diff --git a/internal/e2e/logout_test.go b/internal/e2e/logout_test.go new file mode 100644 index 0000000..fa96654 --- /dev/null +++ b/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) +} diff --git a/internal/web/env.go b/internal/web/env.go index 0158ae7..1720c7d 100644 --- a/internal/web/env.go +++ b/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 diff --git a/internal/web/templates/base.html b/internal/web/templates/base.html index 8010612..9a4b29e 100644 --- a/internal/web/templates/base.html +++ b/internal/web/templates/base.html @@ -31,10 +31,12 @@
{{template "icon-settings" "dropdown-item-icon"}}Account settings + {{if .LogoutURL}}
-
+
+ {{end}}
{{else}}