diff --git a/internal/web/auth.go b/internal/web/auth.go index c868e1c..cfedcbc 100644 --- a/internal/web/auth.go +++ b/internal/web/auth.go @@ -195,11 +195,23 @@ func (a *Authenticator) setCookie(w http.ResponseWriter, name, value string, ttl } func (a *Authenticator) clearCookie(w http.ResponseWriter, name string) { - http.SetCookie(w, &http.Cookie{ + // A cookie delete only lands when its Domain matches the one the cookie was + // set with. The session cookie's scope has changed over Pete's life: host-only + // before the games site, then widened to the parent domain so news and games + // could share one login. A browser may still hold it under the older scope, and + // a clear under only the current scope leaves the other in place — a stranded + // cookie that keeps someone signed in with a session logout can't reach. So + // clear both: always host-only, plus the parent domain when one is configured. + base := http.Cookie{ Name: name, Value: "", Path: "/", MaxAge: -1, - Domain: a.cookieDomain(name), HttpOnly: true, Secure: true, SameSite: http.SameSiteLaxMode, - }) + } + http.SetCookie(w, &base) // host-only (no Domain attribute) + if d := a.cookieDomain(name); d != "" { + scoped := base + scoped.Domain = d + http.SetCookie(w, &scoped) // parent-domain scope + } } // oauthFor returns the OAuth config to use for this request. The configured diff --git a/internal/web/auth_test.go b/internal/web/auth_test.go index fe97d29..ada33ca 100644 --- a/internal/web/auth_test.go +++ b/internal/web/auth_test.go @@ -8,6 +8,56 @@ import ( "golang.org/x/oauth2" ) +// TestClearCookieClearsBothScopes: when a parent cookie domain is configured, +// clearCookie must emit a delete for BOTH the parent-domain scope and the +// host-only scope. A browser holding a session under the older host-only scope +// (from before the cookie domain widened for the games site) would otherwise +// survive logout and keep the user signed in with a session logout can't reach. +func TestClearCookieClearsBothScopes(t *testing.T) { + a := &Authenticator{domain: "parodia.dev"} + rec := httptest.NewRecorder() + a.clearCookie(rec, sessionCookie) + + var hostOnly, scoped bool + for _, c := range rec.Result().Cookies() { + if c.Name != sessionCookie { + continue + } + if c.MaxAge >= 0 { + t.Errorf("clear cookie should expire the session, got MaxAge=%d", c.MaxAge) + } + switch c.Domain { + case "": + hostOnly = true + case "parodia.dev": + scoped = true + default: + t.Errorf("unexpected clear Domain %q", c.Domain) + } + } + if !hostOnly { + t.Error("missing host-only clear (no Domain) — stale host-only sessions stay stranded") + } + if !scoped { + t.Error("missing parent-domain clear (Domain=parodia.dev)") + } +} + +// With no cookie domain configured, only the host-only clear is emitted. +func TestClearCookieHostOnlyWhenNoDomain(t *testing.T) { + a := &Authenticator{} + rec := httptest.NewRecorder() + a.clearCookie(rec, sessionCookie) + + got := rec.Result().Cookies() + if len(got) != 1 { + t.Fatalf("want exactly one clear cookie, got %d", len(got)) + } + if got[0].Domain != "" { + t.Errorf("want host-only clear, got Domain=%q", got[0].Domain) + } +} + func TestSignVerifyRoundTrip(t *testing.T) { a := &Authenticator{secret: []byte("test-secret-key-at-least-16")} payload := []byte(`{"sub":"abc","exp":123}`) diff --git a/internal/web/who.go b/internal/web/who.go index 5427609..8ae990f 100644 --- a/internal/web/who.go +++ b/internal/web/who.go @@ -221,7 +221,18 @@ func (s *Server) handleAdventureWho(w http.ResponseWriter, r *http.Request) { // reversing the token, so no visitor can unlock another player's self extras. if s.auth != nil { if u := s.auth.userFromRequest(r); u != nil { - if self, ok, err := storage.PlayerDetailByOwner(buyerLocalpart(u), token); err == nil && ok { + // A signed-in visitor viewing someone else's page legitimately fails the + // ownership join (ok=false), so that alone is not worth a log. Two other + // misses are: a genuine lookup/decode error, and a session that carries no + // username (minted before the game economy existed) — for the latter the + // join can never match because buyerLocalpart is empty. Both used to fail + // silently here; surface them so a stuck owner is diagnosable. + lp := buyerLocalpart(u) + if lp == "" { + slog.Warn("who: signed-in session has no username; owner unlock skipped", "sub", u.Sub) + } else if self, ok, err := storage.PlayerDetailByOwner(lp, token); err != nil { + slog.Error("who: owner detail lookup failed", "localpart", lp, "token", token, "err", err) + } else if ok { page.HasSelf = true page.Self = self page.Worn = itemRows(self.Equipped, "worn")