From fe3a32c63dee3240796857992246617c46009553 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 13:55:25 +0000 Subject: [PATCH] Test that logging out needs the form's CSRF token (part of #256) A GET /logout, or a POST /logout without the CSRF token, must end no login session; the generator page must carry a logout form with the token; and after a logout from that form, a login opens the generator page while a cookie copied before it is refused, also after a restart. Model: opus-5-5 --- .../handlers/auth_session_internal_test.go | 14 +- internal/server/logout_internal_test.go | 164 +++++++++++++++--- internal/session/session_logout_test.go | 5 +- 3 files changed, 151 insertions(+), 32 deletions(-) diff --git a/internal/handlers/auth_session_internal_test.go b/internal/handlers/auth_session_internal_test.go index c0451c4..250d627 100644 --- a/internal/handlers/auth_session_internal_test.go +++ b/internal/handlers/auth_session_internal_test.go @@ -185,7 +185,7 @@ func TestHandleLogout_ClearsSessionCookie(t *testing.T) { } } -// TestHandleLogout_EndsCopiedSession verifies that after GET /logout a copy +// TestHandleLogout_EndsCopiedSession verifies that after POST /logout a copy // of the session cookie taken before it shows the login page, not the // generator page. func TestHandleLogout_EndsCopiedSession(t *testing.T) { @@ -196,14 +196,14 @@ func TestHandleLogout_EndsCopiedSession(t *testing.T) { copied := newSessionCookie(t, h) req := httptest.NewRequestWithContext( - t.Context(), http.MethodGet, "/logout", nil) + t.Context(), http.MethodPost, "/logout", nil) req.AddCookie(copied) rec := httptest.NewRecorder() h.HandleLogout().ServeHTTP(rec, req) if rec.Code != http.StatusSeeOther { - t.Fatalf("GET /logout status = %d, want %d", + t.Fatalf("POST /logout status = %d, want %d", rec.Code, http.StatusSeeOther) } @@ -223,7 +223,7 @@ func TestHandleLogout_EndsCopiedSession(t *testing.T) { } // TestHandleLogout_SaveFails_Answers500 verifies that when the logout cannot -// be saved in the database, GET /logout answers 500, leaves the session +// be saved in the database, POST /logout answers 500, leaves the session // cookie in place and ends no session. func TestHandleLogout_SaveFails_Answers500(t *testing.T) { t.Parallel() @@ -248,17 +248,17 @@ func TestHandleLogout_SaveFails_Answers500(t *testing.T) { } req := httptest.NewRequestWithContext( - t.Context(), http.MethodGet, "/logout", nil) + t.Context(), http.MethodPost, "/logout", nil) req.AddCookie(cookie) rec := httptest.NewRecorder() h.HandleLogout().ServeHTTP(rec, req) - t.Logf("GET /logout: status %d, Set-Cookie %q", + t.Logf("POST /logout: status %d, Set-Cookie %q", rec.Code, rec.Header().Get("Set-Cookie")) if rec.Code != http.StatusInternalServerError { - t.Errorf("GET /logout status = %d, want %d", + t.Errorf("POST /logout status = %d, want %d", rec.Code, http.StatusInternalServerError) } diff --git a/internal/server/logout_internal_test.go b/internal/server/logout_internal_test.go index 360112b..e473301 100644 --- a/internal/server/logout_internal_test.go +++ b/internal/server/logout_internal_test.go @@ -3,6 +3,8 @@ package server import ( "net/http" "net/http/httptest" + "net/url" + "regexp" "strings" "testing" @@ -19,6 +21,11 @@ import ( "sneak.berlin/go/pixa/internal/session" ) +// logoutFormPattern extracts the CSRF token rendered into the generator +// page's logout form. +var logoutFormPattern = regexp.MustCompile( + `(?s)action="/logout".*?name="gorilla\.csrf\.Token" value="([^"]+)"`) + // startPixa starts the components pixad's fx app builds, from cfg, and // returns the server with its routes, as newTestServer does. func startPixa(t *testing.T, cfg *config.Config) *Server { @@ -50,6 +57,24 @@ func startPixa(t *testing.T, cfg *config.Config) *Server { return s } +// logIn logs in to s with the signing key, as postLogin does, and returns +// the session cookie it sets. +func logIn(t *testing.T, s *Server) *http.Cookie { + t.Helper() + + rec := postLogin(t, s, firstClient, "", testSigningKey) + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName { + return c + } + } + + t.Fatal("logging in set no session cookie") + + return nil +} + // opensGeneratorPage reports whether GET / with cookie shows s's generator // page, which only a valid login session opens. func opensGeneratorPage(t *testing.T, s *Server, cookie *http.Cookie) bool { @@ -65,37 +90,132 @@ func opensGeneratorPage(t *testing.T, s *Server, cookie *http.Cookie) bool { strings.Contains(rec.Body.String(), `action="/generate"`) } +// postLogout loads the generator page with GET / and sessionCookie and +// submits its logout form with POST /logout, as a browser does. +func postLogout( + t *testing.T, s *Server, sessionCookie *http.Cookie, +) *httptest.ResponseRecorder { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.AddCookie(sessionCookie) + + page := httptest.NewRecorder() + s.ServeHTTP(page, req) + + match := logoutFormPattern.FindStringSubmatch(page.Body.String()) + if match == nil { + t.Fatal("the generator page has no logout form with a CSRF token") + } + + form := url.Values{"gorilla.csrf.Token": {match[1]}} + req = httptest.NewRequestWithContext(t.Context(), http.MethodPost, + "/logout", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(sessionCookie) + + for _, c := range page.Result().Cookies() { + req.AddCookie(c) + } + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + return rec +} + +// TestLogoutWithoutCSRFTokenEndsNoSession verifies that a request to /logout +// carrying the session cookie but not the logout form's CSRF token ends no +// session: GET /logout, as an image on another page of the same site makes a +// browser send, answers 405, and POST /logout without the token answers 403. +func TestLogoutWithoutCSRFTokenEndsNoSession(t *testing.T) { + t.Parallel() + + tests := []struct { + method string + want int + }{ + {http.MethodGet, http.StatusMethodNotAllowed}, + {http.MethodPost, http.StatusForbidden}, + } + + for _, tc := range tests { + t.Run(tc.method, func(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + cookie := logIn(t, s) + + req := httptest.NewRequestWithContext( + t.Context(), tc.method, "/logout", nil) + req.AddCookie(cookie) + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + t.Logf("%s /logout without the CSRF token: status %d", + tc.method, rec.Code) + + if rec.Code != tc.want { + t.Errorf("%s /logout without the CSRF token status = %d, "+ + "want %d", tc.method, rec.Code, tc.want) + } + + if !opensGeneratorPage(t, s, cookie) { + t.Errorf("%s /logout without the CSRF token ended the "+ + "login session", tc.method) + } + }) + } +} + +// TestLoginAfterLogoutOpensGeneratorPage verifies that after a logout from +// the generator page a copy of the session cookie taken before it is +// refused, and that a login after it opens the generator page. +func TestLoginAfterLogoutOpensGeneratorPage(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + copied := logIn(t, s) + if !opensGeneratorPage(t, s, copied) { + t.Fatal("logging in did not open the generator page") + } + + logout := postLogout(t, s, copied) + if logout.Code != http.StatusSeeOther { + t.Fatalf("POST /logout status = %d, want %d", + logout.Code, http.StatusSeeOther) + } + + if opensGeneratorPage(t, s, copied) { + t.Error("a cookie copied before the logout still opens the " + + "generator page") + } + + opens := opensGeneratorPage(t, s, logIn(t, s)) + t.Logf("a login after the logout opens the generator page: %v", opens) + + if !opens { + t.Error("a login after the logout does not open the generator page") + } +} + // TestLogoutOutlastsRestart verifies that a copy of the session cookie taken -// before /logout is still refused by a pixa started again on the same state +// before a logout is still refused by a pixa started again on the same state // directory, as pixad is after a restart. func TestLogoutOutlastsRestart(t *testing.T) { t.Parallel() s := newTestServer(t) - login := postLogin(t, s, firstClient, "", testSigningKey) - - var copied *http.Cookie - - for _, c := range login.Result().Cookies() { - if c.Name == session.CookieName { - copied = c - } + copied := logIn(t, s) + if !opensGeneratorPage(t, s, copied) { + t.Fatal("logging in did not open the generator page") } - if copied == nil || !opensGeneratorPage(t, s, copied) { - t.Fatal("logging in gave no session cookie that opens the generator page") - } - - req := httptest.NewRequestWithContext( - t.Context(), http.MethodGet, "/logout", nil) - req.AddCookie(copied) - - logout := httptest.NewRecorder() - s.ServeHTTP(logout, req) - + logout := postLogout(t, s, copied) if logout.Code != http.StatusSeeOther { - t.Fatalf("GET /logout status = %d, want %d", + t.Fatalf("POST /logout status = %d, want %d", logout.Code, http.StatusSeeOther) } @@ -106,7 +226,7 @@ func TestLogoutOutlastsRestart(t *testing.T) { opens) if opens { - t.Error("after a restart, a cookie copied before /logout " + + t.Error("after a restart, a cookie copied before the logout " + "still opens the generator page") } } diff --git a/internal/session/session_logout_test.go b/internal/session/session_logout_test.go index 668513a..b86dbb7 100644 --- a/internal/session/session_logout_test.go +++ b/internal/session/session_logout_test.go @@ -124,9 +124,8 @@ func TestLogout_EndsEverySessionMadeBeforeIt(t *testing.T) { } // TestLogout_WithoutSession_EndsNoSession verifies that a logout from a -// request without a session cookie ends no session. Another site can make a -// browser request /logout, but the browser leaves the SameSite=Strict -// session cookie out of that request. +// request without a session cookie ends no session, so someone who has not +// logged in cannot end the sessions of those who have. func TestLogout_WithoutSession_EndsNoSession(t *testing.T) { t.Parallel()