diff --git a/internal/web/handlers_account.go b/internal/web/handlers_account.go index fea5e1c..845359d 100644 --- a/internal/web/handlers_account.go +++ b/internal/web/handlers_account.go @@ -142,12 +142,12 @@ func (s *Server) submitAccount(w http.ResponseWriter, r *http.Request) { // Keep this session usable under the new name, and — when the password // changed — drop every other session so a cookie captured under the old // password stops working. - if c, err := r.Cookie(sessionCookie); err == nil { + if token, ok := s.sessionToken(r); ok { if renaming { - s.sessions.Rename(c.Value, username) + s.sessions.Rename(token, username) } if repassword { - s.sessions.DestroyOthers(c.Value) + s.sessions.DestroyOthers(token) } } diff --git a/internal/web/handlers_auth.go b/internal/web/handlers_auth.go index d50b892..459541c 100644 --- a/internal/web/handlers_auth.go +++ b/internal/web/handlers_auth.go @@ -9,8 +9,81 @@ import ( "golang.org/x/crypto/bcrypt" ) -// sessionCookie is the name of the panel session cookie. -const sessionCookie = "selfpost_session" +// The panel session cookie's two possible names. In the production shape — +// TLS in front, so CookieSecure — it carries the __Host- prefix, which turns +// what the cookie's attributes merely promise into something the browser +// enforces: Secure, Path=/ and, the point of the exercise, no Domain +// attribute, so no other host may set a cookie by this name (phase 14.B). +// The prefix is only valid on a Secure cookie, so a development instance on +// plain HTTP has to keep the bare name: with the prefix the browser would +// discard the Set-Cookie outright and logging in would silently never stick. +const ( + sessionCookieBase = "selfpost_session" + sessionCookiePrefixed = "__Host-" + sessionCookieBase +) + +// sessionCookie is the session cookie's name for this deployment. +func (s *Server) sessionCookie() string { + if s.cfg.CookieSecure { + return sessionCookiePrefixed + } + return sessionCookieBase +} + +// sessionToken returns the session token the request carries, if exactly one +// cookie of that name is present. +// +// It walks r.Cookies() rather than calling r.Cookie, which silently returns +// the first match. Two cookies with the same name mean somebody other than +// this panel set one of them — a host on the same registrable domain can, +// with Domain=example.com, and the browser will then send both — and RFC 6265 +// makes the older one come first, so "the first match" is precisely the +// attacker's. The value cannot be forged into a valid session, so the effect +// is denial of service, not compromise; refusing the request and saying so in +// the log is what makes it diagnosable instead of an endless login loop. The +// __Host- prefix prevents this outright, but only where it applies — this +// check also covers the plain-HTTP development shape (phase 14.B). +func (s *Server) sessionToken(r *http.Request) (string, bool) { + name := s.sessionCookie() + var token string + var n int + for _, c := range r.Cookies() { + if c.Name == name { + n++ + token = c.Value + } + } + switch n { + case 0: + return "", false + case 1: + return token, true + default: + logf("panel: %s %s carries %d cookies named %q — treating the request as signed out; "+ + "another host on this domain is overwriting the session cookie, clear the cookies for the parent domain", + r.Method, r.URL.Path, n, name) + return "", false + } +} + +// clearSessionCookies expires the session cookie under both names, so an +// upgrade that switches to the __Host- prefix does not leave the old cookie +// sitting in the browser until it is closed. +func (s *Server) clearSessionCookies(w http.ResponseWriter) { + for _, name := range []string{sessionCookieBase, sessionCookiePrefixed} { + http.SetCookie(w, &http.Cookie{ + Name: name, + Value: "", + Path: "/", + MaxAge: -1, + HttpOnly: true, + // The prefixed name is only accepted at all when Secure is set, + // including on the expiring copy. + Secure: s.cfg.CookieSecure || name == sessionCookiePrefixed, + SameSite: http.SameSiteLaxMode, + }) + } +} // handleLogin serves the login form (GET) and authenticates (POST). Until an // administrator exists there is nobody to log in, so it points at setup. @@ -80,7 +153,7 @@ func (s *Server) submitLogin(w http.ResponseWriter, r *http.Request) { token := s.sessions.Create(admin.Username) http.SetCookie(w, &http.Cookie{ - Name: sessionCookie, + Name: s.sessionCookie(), Value: token, Path: "/", HttpOnly: true, @@ -97,17 +170,15 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } - if c, err := r.Cookie(sessionCookie); err == nil { - s.sessions.Destroy(c.Value) + // Destroy every token presented under the session cookie's name: if a + // shadowing duplicate is present (see sessionToken) one of them is the + // real session, and a value that names no session is simply not found. + name := s.sessionCookie() + for _, c := range r.Cookies() { + if c.Name == name { + s.sessions.Destroy(c.Value) + } } - http.SetCookie(w, &http.Cookie{ - Name: sessionCookie, - Value: "", - Path: "/", - MaxAge: -1, - HttpOnly: true, - Secure: s.cfg.CookieSecure, - SameSite: http.SameSiteLaxMode, - }) + s.clearSessionCookies(w) http.Redirect(w, r, "/login", http.StatusSeeOther) } diff --git a/internal/web/handlers_auth_test.go b/internal/web/handlers_auth_test.go new file mode 100644 index 0000000..8fc7599 --- /dev/null +++ b/internal/web/handlers_auth_test.go @@ -0,0 +1,112 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The __Host- prefix is only valid on a Secure cookie: getting this condition +// backwards would make the development instance fail to log in at all, and +// silently — the browser discards the Set-Cookie and the panel just shows the +// login form again. +func TestSessionCookieNameFollowsCookieSecure(t *testing.T) { + secure := &Server{cfg: Config{CookieSecure: true}} + if got := secure.sessionCookie(); got != "__Host-selfpost_session" { + t.Errorf("with TLS the cookie is named %q, want the __Host- prefixed name", got) + } + plain := &Server{cfg: Config{CookieSecure: false}} + if got := plain.sessionCookie(); got != "selfpost_session" { + t.Errorf("without TLS the cookie is named %q, want the bare name", got) + } +} + +// A neighbouring host on the same registrable domain can set a cookie by the +// same name; the browser then sends both, oldest first. Picking one at random +// would leave the administrator in a login loop with no explanation, so the +// request counts as signed out instead. +func TestSessionTokenRejectsDuplicates(t *testing.T) { + s := &Server{cfg: Config{CookieSecure: false}} + r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "planted-by-a-neighbour"}) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "the-real-session"}) + + if token, ok := s.sessionToken(r); ok { + t.Fatalf("duplicate cookies accepted, token = %q", token) + } +} + +func TestSessionTokenReadsOneCookie(t *testing.T) { + s := &Server{cfg: Config{CookieSecure: true}} + r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil) + r.AddCookie(&http.Cookie{Name: "__Host-selfpost_session", Value: "the-real-session"}) + + token, ok := s.sessionToken(r) + if !ok || token != "the-real-session" { + t.Fatalf("sessionToken = %q, %t; want the cookie's value", token, ok) + } +} + +// A cookie under the other deployment's name is not this deployment's session: +// after an upgrade the pre-14 cookie must not be honoured as if it were the +// prefixed one. +func TestSessionTokenIgnoresTheOtherName(t *testing.T) { + s := &Server{cfg: Config{CookieSecure: true}} + r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "left-over-from-an-older-build"}) + + if _, ok := s.sessionToken(r); ok { + t.Fatal("the unprefixed cookie was accepted on a TLS deployment") + } +} + +func TestRequireAuthRejectsDuplicateCookies(t *testing.T) { + s := &Server{cfg: Config{CookieSecure: false}, sessions: newSessionStore()} + token := s.sessions.Create("admin") + + reached := false + h := s.requireAuth(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true })) + + r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "planted-by-a-neighbour"}) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: token}) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, r) + + if reached { + t.Fatal("the handler ran even though the session cookie was shadowed") + } + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/login" { + t.Fatalf("status = %d, Location = %q; want a redirect to /login", rec.Code, rec.Header().Get("Location")) + } +} + +// Signing out has to expire the cookie under both names, or the cookie left +// over from a pre-__Host- build stays in the browser for the rest of its life. +func TestLogoutClearsBothCookieNames(t *testing.T) { + s := &Server{cfg: Config{CookieSecure: true}, sessions: newSessionStore()} + token := s.sessions.Create("admin") + + r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/logout", nil) + r.Host = "panel.example.com" + r.AddCookie(&http.Cookie{Name: "__Host-selfpost_session", Value: token}) + rec := httptest.NewRecorder() + s.handleLogout(rec, r) + + if _, ok := s.sessions.Lookup(token); ok { + t.Error("the session survived sign-out") + } + set := rec.Header().Values("Set-Cookie") + for _, name := range []string{"selfpost_session=", "__Host-selfpost_session="} { + var found bool + for _, c := range set { + if strings.HasPrefix(c, name) && strings.Contains(c, "Max-Age=0") { + found = true + } + } + if !found { + t.Errorf("sign-out does not expire a cookie named %q: %v", strings.TrimSuffix(name, "="), set) + } + } +} diff --git a/internal/web/middleware.go b/internal/web/middleware.go index 09b9e02..efa05ae 100644 --- a/internal/web/middleware.go +++ b/internal/web/middleware.go @@ -14,12 +14,12 @@ const usernameKey ctxKey = 0 // username is stashed in the request context for downstream handlers. func (s *Server) requireAuth(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - c, err := r.Cookie(sessionCookie) - if err != nil { + token, ok := s.sessionToken(r) + if !ok { http.Redirect(w, r, "/login", http.StatusSeeOther) return } - username, ok := s.sessions.Lookup(c.Value) + username, ok := s.sessions.Lookup(token) if !ok { http.Redirect(w, r, "/login", http.StatusSeeOther) return