From 0df7b0d8c296d707a022bdaecf293cf9ccd0c569 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 02:51:01 +0000 Subject: [PATCH 1/9] Test that logging out ends a copied login session (part of #256) A copy of the session cookie taken before /logout must be refused after it, a login after it must work, and both must still hold for a session manager made again on the same database, as after a restart. A logout from a request without a session cookie must end no session. These fail until the fix: the session tests do not compile yet. Model: opus-5-5 --- .../handlers/auth_session_internal_test.go | 37 +++++ internal/session/session_logout_test.go | 149 ++++++++++++++++++ 2 files changed, 186 insertions(+) create mode 100644 internal/session/session_logout_test.go diff --git a/internal/handlers/auth_session_internal_test.go b/internal/handlers/auth_session_internal_test.go index 883e434..bf95d9d 100644 --- a/internal/handlers/auth_session_internal_test.go +++ b/internal/handlers/auth_session_internal_test.go @@ -185,6 +185,43 @@ func TestHandleLogout_ClearsSessionCookie(t *testing.T) { } } +// TestHandleLogout_EndsCopiedSession verifies that after GET /logout a copy +// of the session cookie taken before it shows the login page, not the +// generator page. +func TestHandleLogout_EndsCopiedSession(t *testing.T) { + t.Parallel() + + h, srv := newCSRFTestRouter(t) + + copied := newSessionCookie(t, h) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/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", + rec.Code, http.StatusSeeOther) + } + + req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.AddCookie(copied) + + rec = httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + body := rec.Body.String() + t.Logf("GET / with the copied cookie: status %d, login page %v", + rec.Code, strings.Contains(body, loginForm)) + + if !strings.Contains(body, loginForm) || strings.Contains(body, generatorForm) { + t.Error("a cookie copied before /logout still opens the generator page") + } +} + // TestGeneratePost_NoSession_RedirectsToLogin verifies that POST /generate // with a valid CSRF token but no login session answers 303 to / and makes no // URL. diff --git a/internal/session/session_logout_test.go b/internal/session/session_logout_test.go new file mode 100644 index 0000000..668513a --- /dev/null +++ b/internal/session/session_logout_test.go @@ -0,0 +1,149 @@ +package session_test + +import ( + "database/sql" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "sneak.berlin/go/pixa/internal/database" + "sneak.berlin/go/pixa/internal/session" +) + +const logoutTestSigningKey = "test-signing-key-12345" + +// newTestDatabase opens an empty in-memory database with pixa's schema. It +// has one connection, as pixad's has: each connection to ":memory:" would +// open a database of its own. +func newTestDatabase(t *testing.T) *sql.DB { + t.Helper() + + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("sql.Open() error = %v", err) + } + + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + err = database.ApplyMigrations(t.Context(), db, nil) + if err != nil { + t.Fatalf("ApplyMigrations() error = %v", err) + } + + return db +} + +// createSessionCookie logs in with mgr and returns the session cookie it +// sets. +func createSessionCookie(t *testing.T, mgr *session.Manager) *http.Cookie { + t.Helper() + + rec := httptest.NewRecorder() + + err := mgr.CreateSession(rec) + if err != nil { + t.Fatalf("CreateSession() error = %v", err) + } + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName { + return c + } + } + + t.Fatalf("CreateSession() did not set a %q cookie", session.CookieName) + + return nil +} + +// requestWithCookie returns a GET request carrying cookie, or no cookie when +// cookie is nil. +func requestWithCookie(t *testing.T, cookie *http.Cookie) *http.Request { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + if cookie != nil { + req.AddCookie(cookie) + } + + return req +} + +// TestLogout_EndsEverySessionMadeBeforeIt verifies that after a logout a +// copy of the session cookie taken before it is refused and a login after it +// works, and that both still hold for a new Manager on the same database, as +// after a restart. +func TestLogout_EndsEverySessionMadeBeforeIt(t *testing.T) { + t.Parallel() + + db := newTestDatabase(t) + + mgr, err := session.NewManagerWithDatabase( + t.Context(), logoutTestSigningKey, db) + if err != nil { + t.Fatalf("NewManagerWithDatabase() error = %v", err) + } + + copied := createSessionCookie(t, mgr) + + err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, copied)) + if err != nil { + t.Fatalf("Logout() error = %v", err) + } + + _, err = mgr.ValidateSession(requestWithCookie(t, copied)) + t.Logf("cookie copied before the logout: ValidateSession() error = %v", err) + + if !errors.Is(err, session.ErrInvalidSession) { + t.Errorf("cookie copied before the logout: error = %v, want %v", + err, session.ErrInvalidSession) + } + + loginAfter := createSessionCookie(t, mgr) + + _, err = mgr.ValidateSession(requestWithCookie(t, loginAfter)) + if err != nil { + t.Errorf("login after the logout: ValidateSession() error = %v", err) + } + + restarted, err := session.NewManagerWithDatabase( + t.Context(), logoutTestSigningKey, db) + if err != nil { + t.Fatalf("NewManagerWithDatabase() after restart error = %v", err) + } + + if restarted.IsAuthenticated(requestWithCookie(t, copied)) { + t.Error("after a restart, a cookie copied before the logout is accepted") + } + + if !restarted.IsAuthenticated(requestWithCookie(t, loginAfter)) { + t.Error("after a restart, a login made after the logout is refused") + } +} + +// 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. +func TestLogout_WithoutSession_EndsNoSession(t *testing.T) { + t.Parallel() + + mgr, err := session.NewManagerWithDatabase( + t.Context(), logoutTestSigningKey, newTestDatabase(t)) + if err != nil { + t.Fatalf("NewManagerWithDatabase() error = %v", err) + } + + cookie := createSessionCookie(t, mgr) + + err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, nil)) + if err != nil { + t.Fatalf("Logout() error = %v", err) + } + + if !mgr.IsAuthenticated(requestWithCookie(t, cookie)) { + t.Error("a logout without a session cookie ended a session") + } +} -- 2.54.0 From 9a88a695265362c591178d13f6b5852befc53945 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 03:03:52 +0000 Subject: [PATCH 2/9] Logging out ends every login session (closes #256) A login session was a cookie with nothing kept on the server, so /logout removed it from one browser and a copy kept working for 30 days. Every login is made with the one signing key, so /logout now ends them all: the session manager keeps the time of the latest logout, saves it in the new logout table so it outlasts a restart, and refuses a session made before it. A /logout without a valid session ends none and only removes the cookie, so another site cannot log everyone out. A logout that cannot be saved answers 500. session.NewManager, which existing tests use, keeps logouts in memory only; pixad uses NewManagerWithDatabase. Model: opus-5-5 --- README.md | 11 +++- TODO.md | 11 ++++ internal/db/migrations/001_schema.sql | 7 ++ internal/handlers/auth.go | 12 +++- internal/handlers/handlers.go | 4 +- internal/session/session.go | 94 +++++++++++++++++++++++++++ 6 files changed, 134 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 61fc6c9..435e558 100644 --- a/README.md +++ b/README.md @@ -196,7 +196,10 @@ path under `/v1/` answers 200, in maintenance mode too. session it answers 303 to `/`. Answers: 200 with the page showing the URL; 400 with the page naming a field that is not valid; 500 when the URL cannot be made. -- `GET /logout` — end the login session. Needs: nothing. Answers: 303 to `/`. +- `GET /logout` — end every login session, in every browser (see Encrypted + URLs), and remove the session cookie. Needs: a login session; without one it + only removes the cookie. Answers: 303 to `/`; 500 when the logout cannot be + saved in the database. - `GET` or `HEAD` `/v1/image///.`, or `/v1/image///` with no format — an image, fetched, resized and converted (below). Needs: a signature, unless the host is allowlisted (see @@ -382,7 +385,11 @@ nor change what it asks for. 1. Open `/` in a browser over HTTPS (or over plain HTTP while `debug` is on, see Routes) and log in with the signing key (`signing_key`). The login session - lasts 30 days, or until `/logout`. + lasts 30 days, or until someone logs out at `/logout`. Every login is made + with the one signing key, so logging out ends every login session, in every + browser: pixa saves the time of the latest logout in its database and refuses + a login session made before it, a copy of the session cookie included, also + after a restart. 2. On the generator page, give the source image's URL, the width and height, the format, quality and fit, and how long the URL lasts, then submit the form (`POST /generate`). The format is JPEG XL unless another is chosen; a form diff --git a/TODO.md b/TODO.md index a9f3843..f3fd828 100644 --- a/TODO.md +++ b/TODO.md @@ -133,6 +133,17 @@ P2: security: per-origin rate limiting answered 500. Once the limit is used up, the fetcher now reads one more byte: if the body has ended it is accepted, and if there is more it is refused with `ErrResponseTooLarge`, as before. +- 2026-10-15 logging out ends every login session (closes #256): a login session + was a cookie with nothing kept on the server, so `/logout` removed it from one + browser and a copy of it kept working for its 30 days. Every login is made + with the one signing key, so `/logout` now ends them all: the session manager + keeps the time of the latest logout, saves it in the new `logout` table so + that it outlasts a restart, and refuses a session made before it. A `/logout` + without a valid login session, such as one another site makes a browser send + without the `SameSite=Strict` cookie, ends none and only removes the cookie. A + logout that cannot be saved answers 500. `session.NewManager`, which the tests + use, keeps logouts in memory only; pixad uses + `session.NewManagerWithDatabase`. - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side (closes #257): it scales the image to cover the requested size, and the side scaled past that size was held to no maximum, so `1x8192?fit=outside` of a diff --git a/internal/db/migrations/001_schema.sql b/internal/db/migrations/001_schema.sql index 68f3a32..d4788b9 100644 --- a/internal/db/migrations/001_schema.sql +++ b/internal/db/migrations/001_schema.sql @@ -176,3 +176,10 @@ CREATE TABLE IF NOT EXISTS cache_stats ( last_updated_at DATETIME DEFAULT CURRENT_TIMESTAMP ); INSERT OR IGNORE INTO cache_stats (id) VALUES (1); + +-- Time of the latest logout. A login session made before it is refused, +-- so logging out ends every login session. No row: nobody has logged out. +CREATE TABLE IF NOT EXISTS logout ( + id INTEGER PRIMARY KEY CHECK (id = 1), + logged_out_at DATETIME NOT NULL +); diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 052ccd2..dff2763 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -89,10 +89,18 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/", http.StatusSeeOther) } -// HandleLogout clears the session and redirects to login. +// HandleLogout logs out, which ends every login session (see +// session.Manager.Logout), and redirects to login. func (s *Handlers) HandleLogout() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - s.sessMgr.ClearSession(w) + err := s.sessMgr.Logout(w, r) + if err != nil { + s.log.Error("failed to log out", "error", err) + http.Error(w, "Failed to log out", http.StatusInternalServerError) + + return + } + http.Redirect(w, r, "/", http.StatusSeeOther) } } diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index da9572f..6e191d1 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -164,7 +164,9 @@ func (s *Handlers) initImageService() error { // Initialize session manager (signing key is validated at config load // time). Session cookies are always Secure/HttpOnly/SameSite=Strict. - sessMgr, err := session.NewManager(s.config.SigningKey) + // Logouts are saved in the database, so they outlast a restart. + sessMgr, err := session.NewManagerWithDatabase( + context.Background(), s.config.SigningKey, s.db.DB()) if err != nil { return err } diff --git a/internal/session/session.go b/internal/session/session.go index 7233d9e..c4ea3d2 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -2,8 +2,12 @@ package session import ( + "context" + "database/sql" "errors" + "fmt" "net/http" + "sync" "time" "github.com/gorilla/securecookie" @@ -35,11 +39,25 @@ type Data struct { } // Manager handles session creation and validation using encrypted cookies. +// +// Every login is made with the one signing key, so a logout ends every +// session made before it, in every browser. The Manager keeps the time of +// the latest logout in memory, so checking a session reads no database; a +// Manager from NewManagerWithDatabase also saves it in the database, so that +// it outlasts a restart. type Manager struct { sc *securecookie.SecureCookie + db *sql.DB + + // mu guards latestLogout, and is held while a logout is saved, so that + // the database and latestLogout agree. + mu sync.Mutex + latestLogout time.Time } // NewManager creates a session manager with keys derived from the signing key. +// It keeps logouts in memory only, so a restart forgets them; +// NewManagerWithDatabase keeps them in the database. // // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict // attributes; this cannot be configured. Browsers treat http://localhost as a @@ -67,6 +85,29 @@ func NewManager(signingKey string) (*Manager, error) { }, nil } +// NewManagerWithDatabase creates a session manager like NewManager that saves +// each logout in db, which must have pixa's schema, and starts from the +// latest logout saved there. +func NewManagerWithDatabase( + ctx context.Context, signingKey string, db *sql.DB, +) (*Manager, error) { + m, err := NewManager(signingKey) + if err != nil { + return nil, err + } + + err = db.QueryRowContext(ctx, + "SELECT logged_out_at FROM logout WHERE id = 1", + ).Scan(&m.latestLogout) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return nil, fmt.Errorf("failed to read the latest logout: %w", err) + } + + m.db = db + + return m, nil +} + // CreateSession creates a new authenticated session and sets the cookie. func (m *Manager) CreateSession(w http.ResponseWriter) error { now := time.Now() @@ -122,9 +163,31 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) { return nil, ErrInvalidSession } + // A session made before the latest logout has ended. + if !data.CreatedAt.After(m.latestLogoutTime()) { + return nil, ErrInvalidSession + } + return &data, nil } +// Logout removes the session cookie and, when r has a valid session, ends +// every session made until now. A request without one ends no session: +// another site can make a browser request /logout, but the browser leaves +// the SameSite=Strict session cookie out of that request. +func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) error { + if m.IsAuthenticated(r) { + err := m.saveLogout(r.Context()) + if err != nil { + return err + } + } + + m.ClearSession(w) + + return nil +} + // ClearSession removes the session cookie. func (m *Manager) ClearSession(w http.ResponseWriter) { http.SetCookie(w, &http.Cookie{ @@ -145,3 +208,34 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool { return err == nil && data != nil && data.Authenticated } + +// saveLogout makes now the time of the latest logout, saving it in the +// database first when the Manager has one. +func (m *Manager) saveLogout(ctx context.Context) error { + m.mu.Lock() + defer m.mu.Unlock() + + now := time.Now().UTC() + + if m.db != nil { + _, err := m.db.ExecContext(ctx, + "INSERT OR REPLACE INTO logout (id, logged_out_at) VALUES (1, ?)", + now) + if err != nil { + return fmt.Errorf("failed to save the logout: %w", err) + } + } + + m.latestLogout = now + + return nil +} + +// latestLogoutTime returns the time of the latest logout, or the zero time +// when there has been none. +func (m *Manager) latestLogoutTime() time.Time { + m.mu.Lock() + defer m.mu.Unlock() + + return m.latestLogout +} -- 2.54.0 From 8efc87bc8df98c83a3ad7c9d22f7b09f671f9ff6 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 05:17:58 +0000 Subject: [PATCH 3/9] Test a logout after a restart and one that cannot be saved (part of #256) The new server test logs in and out on pixa built as pixad builds it, starts pixa again on the same state directory, and checks that a cookie copied before the logout is refused, which fails if pixad keeps logouts in memory only. The new handler test closes the database so the logout cannot be saved, and checks for 500, no session cookie set, and the session still valid. Model: opus-5-5 --- .../handlers/auth_session_internal_test.go | 55 +++++++++ internal/server/logout_internal_test.go | 112 ++++++++++++++++++ 2 files changed, 167 insertions(+) create mode 100644 internal/server/logout_internal_test.go diff --git a/internal/handlers/auth_session_internal_test.go b/internal/handlers/auth_session_internal_test.go index bf95d9d..c0451c4 100644 --- a/internal/handlers/auth_session_internal_test.go +++ b/internal/handlers/auth_session_internal_test.go @@ -222,6 +222,61 @@ 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 +// cookie in place and ends no session. +func TestHandleLogout_SaveFails_Answers500(t *testing.T) { + t.Parallel() + + h, srv := newCSRFTestRouter(t) + + db := setupTestDB(t) + + sessMgr, err := session.NewManagerWithDatabase( + t.Context(), testSigningKey, db) + if err != nil { + t.Fatalf("session.NewManagerWithDatabase() error = %v", err) + } + + h.sessMgr = sessMgr + cookie := newSessionCookie(t, h) + + // Saving the logout fails on a closed database. + err = db.Close() + if err != nil { + t.Fatalf("closing the database: %v", err) + } + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/logout", nil) + req.AddCookie(cookie) + + rec := httptest.NewRecorder() + h.HandleLogout().ServeHTTP(rec, req) + + t.Logf("GET /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", + rec.Code, http.StatusInternalServerError) + } + + if c := findSessionCookie(rec); c != nil { + t.Errorf("a logout that failed set the session cookie: %s", c) + } + + req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.AddCookie(cookie) + + rec = httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + if !strings.Contains(rec.Body.String(), generatorForm) { + t.Error("a logout that failed ended the session") + } +} + // TestGeneratePost_NoSession_RedirectsToLogin verifies that POST /generate // with a valid CSRF token but no login session answers 303 to / and makes no // URL. diff --git a/internal/server/logout_internal_test.go b/internal/server/logout_internal_test.go new file mode 100644 index 0000000..360112b --- /dev/null +++ b/internal/server/logout_internal_test.go @@ -0,0 +1,112 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "go.uber.org/fx" + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/database" + "sneak.berlin/go/pixa/internal/globals" + "sneak.berlin/go/pixa/internal/handlers" + "sneak.berlin/go/pixa/internal/healthcheck" + "sneak.berlin/go/pixa/internal/logger" + "sneak.berlin/go/pixa/internal/middleware" + "sneak.berlin/go/pixa/internal/session" +) + +// 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 { + t.Helper() + + var ( + h *handlers.Handlers + mw *middleware.Middleware + ) + + app := fxtest.New(t, + fx.Supply(cfg), + fx.Provide( + globals.New, + logger.New, + database.New, + healthcheck.New, + handlers.New, + middleware.New, + ), + fx.Populate(&h, &mw), + ) + app.RequireStart() + t.Cleanup(app.RequireStop) + + s := &Server{config: cfg, mw: mw, h: h} + s.SetupRoutes() + + return s +} + +// 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 { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.AddCookie(cookie) + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + return rec.Code == http.StatusOK && + strings.Contains(rec.Body.String(), `action="/generate"`) +} + +// TestLogoutOutlastsRestart verifies that a copy of the session cookie taken +// before /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 + } + } + + 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) + + if logout.Code != http.StatusSeeOther { + t.Fatalf("GET /logout status = %d, want %d", + logout.Code, http.StatusSeeOther) + } + + restarted := startPixa(t, s.config) + + opens := opensGeneratorPage(t, restarted, copied) + t.Logf("after the restart, the copied cookie opens the generator page: %v", + opens) + + if opens { + t.Error("after a restart, a cookie copied before /logout " + + "still opens the generator page") + } +} -- 2.54.0 From e108839bf825226aba8508b550dfe2c2bfc3367b Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 05:17:58 +0000 Subject: [PATCH 4/9] Say that losing the state directory forgets the latest logout (part of #256) The state directory bullet in README.md and the session package doc now say that the time of the latest logout is kept in the database, which a restart without a persistent volume loses. Model: opus-5-5 --- README.md | 9 +++++---- TODO.md | 23 ++++++++++++----------- internal/session/session.go | 4 +++- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 435e558..6c868ee 100644 --- a/README.md +++ b/README.md @@ -71,9 +71,10 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the database and the disk cache: - It needs a persistent volume: without one, every restart starts with an empty - cache. In the container, the startup script gives the directory to the user - pixa runs as (uid 65532) and sets its mode to `750`; outside it, that user - must be able to write the directory. + cache and forgets the latest logout, so the login sessions that logout ended + work again (see Encrypted URLs). In the container, the startup script gives + the directory to the user pixa runs as (uid 65532) and sets its mode to `750`; + outside it, that user must be able to write the directory. - `cache_max_bytes` limits the source and transformed images together. The database, the metadata files, the `.meta` file beside each transformed image and files still being written come on top, and eviction runs in the @@ -389,7 +390,7 @@ nor change what it asks for. with the one signing key, so logging out ends every login session, in every browser: pixa saves the time of the latest logout in its database and refuses a login session made before it, a copy of the session cookie included, also - after a restart. + after a restart that keeps the state directory (see Deployment). 2. On the generator page, give the source image's URL, the width and height, the format, quality and fit, and how long the URL lasts, then submit the form (`POST /generate`). The format is JPEG XL unless another is chosen; a form diff --git a/TODO.md b/TODO.md index f3fd828..93f3fc7 100644 --- a/TODO.md +++ b/TODO.md @@ -116,6 +116,18 @@ P2: security: per-origin rate limiting and the query now have each `%` written as `%25` and each `:` as `%3A`; a path and query holding neither are signed and keyed as before. Signature Specification in `README.md` says so. +- 2026-10-15 logging out ends every login session (closes #256): a login session + was a cookie with nothing kept on the server, so `/logout` removed it from one + browser and a copy of it kept working for its 30 days. Every login is made + with the one signing key, so `/logout` now ends them all: the session manager + keeps the time of the latest logout, saves it in the new `logout` table so + that it outlasts a restart, and refuses a session made before it. A restart + without a persistent volume for the state directory forgets it. A `/logout` + without a valid login session, such as one another site makes a browser send + without the `SameSite=Strict` cookie, ends none and only removes the cookie. A + logout that cannot be saved answers 500. `session.NewManager`, which the tests + use, keeps logouts in memory only; pixad uses + `session.NewManagerWithDatabase`. - 2026-10-15 the example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to @@ -133,17 +145,6 @@ P2: security: per-origin rate limiting answered 500. Once the limit is used up, the fetcher now reads one more byte: if the body has ended it is accepted, and if there is more it is refused with `ErrResponseTooLarge`, as before. -- 2026-10-15 logging out ends every login session (closes #256): a login session - was a cookie with nothing kept on the server, so `/logout` removed it from one - browser and a copy of it kept working for its 30 days. Every login is made - with the one signing key, so `/logout` now ends them all: the session manager - keeps the time of the latest logout, saves it in the new `logout` table so - that it outlasts a restart, and refuses a session made before it. A `/logout` - without a valid login session, such as one another site makes a browser send - without the `SameSite=Strict` cookie, ends none and only removes the cookie. A - logout that cannot be saved answers 500. `session.NewManager`, which the tests - use, keeps logouts in memory only; pixad uses - `session.NewManagerWithDatabase`. - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side (closes #257): it scales the image to cover the requested size, and the side scaled past that size was held to no maximum, so `1x8192?fit=outside` of a diff --git a/internal/session/session.go b/internal/session/session.go index c4ea3d2..0097dd4 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -1,4 +1,6 @@ -// Package session provides encrypted session cookie management. +// Package session provides encrypted session cookie management, and keeps the +// time of the latest logout in the database so that a logout ends every +// session made before it. package session import ( -- 2.54.0 From 42f6b2324439e457abd17dd1b0ea7c0b4db637c0 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 13:55:25 +0000 Subject: [PATCH 5/9] 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() -- 2.54.0 From a33cc6861bcd4bf947e3b372d199e414e18733db Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 14:00:58 +0000 Subject: [PATCH 6/9] Log out with a form and count logouts instead of timing them (part of #256) Logging out is now a POST in the CSRF-protected route group, sent by a form on the generator page, so a link or image on another page of the same site cannot end every login session. The session manager keeps a count of logouts instead of the time of the latest one: each session carries the count at its login and is refused once it has changed. No clock is compared, so a login after a logout always works. Model: opus-5-5 --- README.md | 32 +++++++------ TODO.md | 28 ++++++----- internal/db/migrations/001_schema.sql | 8 ++-- internal/handlers/handlers.go | 3 +- internal/server/routes.go | 3 +- internal/session/session.go | 67 ++++++++++++++------------- internal/static/style.css | 12 ++++- internal/templates/generator.html | 9 ++-- 8 files changed, 93 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 6c868ee..e152dea 100644 --- a/README.md +++ b/README.md @@ -71,10 +71,10 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the database and the disk cache: - It needs a persistent volume: without one, every restart starts with an empty - cache and forgets the latest logout, so the login sessions that logout ended - work again (see Encrypted URLs). In the container, the startup script gives - the directory to the user pixa runs as (uid 65532) and sets its mode to `750`; - outside it, that user must be able to write the directory. + cache and forgets the count of logouts, so login sessions that a logout ended + can work again (see Encrypted URLs). In the container, the startup script + gives the directory to the user pixa runs as (uid 65532) and sets its mode to + `750`; outside it, that user must be able to write the directory. - `cache_max_bytes` limits the source and transformed images together. The database, the metadata files, the `.meta` file beside each transformed image and files still being written come on top, and eviction runs in the @@ -197,10 +197,11 @@ path under `/v1/` answers 200, in maintenance mode too. session it answers 303 to `/`. Answers: 200 with the page showing the URL; 400 with the page naming a field that is not valid; 500 when the URL cannot be made. -- `GET /logout` — end every login session, in every browser (see Encrypted - URLs), and remove the session cookie. Needs: a login session; without one it - only removes the cookie. Answers: 303 to `/`; 500 when the logout cannot be - saved in the database. +- `POST /logout` — log out with the generator page's Logout button: end every + login session, in every browser (see Encrypted URLs), and remove the session + cookie. Needs: a login session and the generator page's form (below); without + a login session it only removes the cookie. Answers: 303 to `/`; 500 when the + logout cannot be saved in the database. - `GET` or `HEAD` `/v1/image///.`, or `/v1/image///` with no format — an image, fetched, resized and converted (below). Needs: a signature, unless the host is allowlisted (see @@ -248,7 +249,7 @@ the request carries the same ID as `request_id`, and so do the lines it logs when it fetches, converts and serves an image; the fetch sends it to the upstream host as `X-Request-ID`. -Both `POST` routes accept only a form that pixa's own page served: the page puts +The `POST` routes accept only a form that pixa's own page served: the page puts a token in the form and sets a cookie to match, and a request without both is refused with 403, so another site cannot submit the form from a visitor's browser. The login and generator pages are meant to be opened over HTTPS: while @@ -386,11 +387,14 @@ nor change what it asks for. 1. Open `/` in a browser over HTTPS (or over plain HTTP while `debug` is on, see Routes) and log in with the signing key (`signing_key`). The login session - lasts 30 days, or until someone logs out at `/logout`. Every login is made - with the one signing key, so logging out ends every login session, in every - browser: pixa saves the time of the latest logout in its database and refuses - a login session made before it, a copy of the session cookie included, also - after a restart that keeps the state directory (see Deployment). + lasts 30 days, or until someone logs out with the generator page's Logout + button. Every login is made with the one signing key, so logging out ends + every login session, in every browser, a copy of the session cookie included. + pixa keeps a count of logouts in its database: each logout adds one, a login + session carries the count at its login, and a login session whose count is + not the current one is refused, also after a restart that keeps the state + directory (see Deployment). No time is compared, so a login after a logout + works whatever the host's clock says. 2. On the generator page, give the source image's URL, the width and height, the format, quality and fit, and how long the URL lasts, then submit the form (`POST /generate`). The format is JPEG XL unless another is chosen; a form diff --git a/TODO.md b/TODO.md index 93f3fc7..7b977ba 100644 --- a/TODO.md +++ b/TODO.md @@ -76,6 +76,22 @@ P2: security: per-origin rate limiting 405, or a CORS preflight. The metrics library takes the label before the request is routed, so the metrics middleware asks the router which route matches. +- 2026-10-15 logging out ends every login session (closes #256): a login session + was a cookie with nothing kept on the server, so `/logout` removed it from one + browser and a copy of it kept working for its 30 days. Every login is made + with the one signing key, so logging out now ends them all: the session + manager counts logouts, saves the count in the new `logout` table so that it + outlasts a restart, puts the count at login in each session, and refuses a + session whose count is not the current one. No time is compared, so a login + after a logout works whatever the host's clock does. A restart without a + persistent volume for the state directory forgets the count. Logging out is + now `POST /logout` from a form on the generator page, behind the CSRF check + the other forms have, so a link or image elsewhere on the same site cannot end + the sessions. A logout without a valid login session ends none and only + removes the cookie, as anyone can take a CSRF token from the login page. A + logout that cannot be saved answers 500. `session.NewManager`, which the tests + use, keeps the count in memory only; pixad uses + `session.NewManagerWithDatabase`. - 2026-10-15 expired negative cache rows are removed on a schedule (closes #244): the row of a failed fetch, which makes pixa answer 502 for that source URL for 5 minutes, was deleted only when the same URL was requested again @@ -116,18 +132,6 @@ P2: security: per-origin rate limiting and the query now have each `%` written as `%25` and each `:` as `%3A`; a path and query holding neither are signed and keyed as before. Signature Specification in `README.md` says so. -- 2026-10-15 logging out ends every login session (closes #256): a login session - was a cookie with nothing kept on the server, so `/logout` removed it from one - browser and a copy of it kept working for its 30 days. Every login is made - with the one signing key, so `/logout` now ends them all: the session manager - keeps the time of the latest logout, saves it in the new `logout` table so - that it outlasts a restart, and refuses a session made before it. A restart - without a persistent volume for the state directory forgets it. A `/logout` - without a valid login session, such as one another site makes a browser send - without the `SameSite=Strict` cookie, ends none and only removes the cookie. A - logout that cannot be saved answers 500. `session.NewManager`, which the tests - use, keeps logouts in memory only; pixad uses - `session.NewManagerWithDatabase`. - 2026-10-15 the example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to diff --git a/internal/db/migrations/001_schema.sql b/internal/db/migrations/001_schema.sql index d4788b9..2e018af 100644 --- a/internal/db/migrations/001_schema.sql +++ b/internal/db/migrations/001_schema.sql @@ -177,9 +177,11 @@ CREATE TABLE IF NOT EXISTS cache_stats ( ); INSERT OR IGNORE INTO cache_stats (id) VALUES (1); --- Time of the latest logout. A login session made before it is refused, --- so logging out ends every login session. No row: nobody has logged out. +-- Count of logouts. Each logout adds one; a login session carries the count +-- at its login and is refused once the count has changed, so logging out ends +-- every login session. CREATE TABLE IF NOT EXISTS logout ( id INTEGER PRIMARY KEY CHECK (id = 1), - logged_out_at DATETIME NOT NULL + logout_count INTEGER NOT NULL DEFAULT 0 ); +INSERT OR IGNORE INTO logout (id) VALUES (1); diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 6e191d1..c82b621 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -164,7 +164,8 @@ func (s *Handlers) initImageService() error { // Initialize session manager (signing key is validated at config load // time). Session cookies are always Secure/HttpOnly/SameSite=Strict. - // Logouts are saved in the database, so they outlast a restart. + // The count of logouts is saved in the database, so it outlasts a + // restart. sessMgr, err := session.NewManagerWithDatabase( context.Background(), s.config.SigningKey, s.db.DB()) if err != nil { diff --git a/internal/server/routes.go b/internal/server/routes.go index 104f2e6..b7ed1a2 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -69,10 +69,9 @@ func (s *Server) SetupRoutes() { r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)). Post("/", s.h.HandleRoot()) r.Post("/generate", s.h.HandleGenerateURL()) + r.Post("/logout", s.h.HandleLogout()) }) - s.router.Get("/logout", s.h.HandleLogout()) - // Image routes, the only ones that send CORS headers, as pages on other // sites read them. They are a subrouter rather than a group: a group's // middleware runs only for a request that matches one of its routes, diff --git a/internal/session/session.go b/internal/session/session.go index 0097dd4..06c77bd 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -1,6 +1,6 @@ -// Package session provides encrypted session cookie management, and keeps the -// time of the latest logout in the database so that a logout ends every -// session made before it. +// Package session provides encrypted session cookie management, and keeps a +// count of logouts in the database so that a logout ends every session made +// before it. package session import ( @@ -38,28 +38,34 @@ type Data struct { Authenticated bool `json:"auth"` CreatedAt time.Time `json:"created"` ExpiresAt time.Time `json:"expires"` + + // LogoutCount is the count of logouts when the session was made. + LogoutCount int64 `json:"logoutCount"` } // Manager handles session creation and validation using encrypted cookies. // // Every login is made with the one signing key, so a logout ends every -// session made before it, in every browser. The Manager keeps the time of -// the latest logout in memory, so checking a session reads no database; a +// session made before it, in every browser. The Manager counts logouts: each +// logout adds one, a session carries the count at its login, and a session +// whose count is not the current one is refused. No clock is compared, so a +// login after a logout works whatever the host's clock does. The Manager +// keeps the count in memory, so checking a session reads no database; a // Manager from NewManagerWithDatabase also saves it in the database, so that // it outlasts a restart. type Manager struct { sc *securecookie.SecureCookie db *sql.DB - // mu guards latestLogout, and is held while a logout is saved, so that - // the database and latestLogout agree. - mu sync.Mutex - latestLogout time.Time + // mu guards logoutCount, and is held while a logout is saved, so that + // the database and logoutCount agree. + mu sync.Mutex + logoutCount int64 } // NewManager creates a session manager with keys derived from the signing key. -// It keeps logouts in memory only, so a restart forgets them; -// NewManagerWithDatabase keeps them in the database. +// It keeps the count of logouts in memory only, so a restart forgets it; +// NewManagerWithDatabase keeps it in the database. // // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict // attributes; this cannot be configured. Browsers treat http://localhost as a @@ -88,8 +94,8 @@ func NewManager(signingKey string) (*Manager, error) { } // NewManagerWithDatabase creates a session manager like NewManager that saves -// each logout in db, which must have pixa's schema, and starts from the -// latest logout saved there. +// the count of logouts in db, which must have pixa's schema, and starts from +// the count saved there. func NewManagerWithDatabase( ctx context.Context, signingKey string, db *sql.DB, ) (*Manager, error) { @@ -99,10 +105,10 @@ func NewManagerWithDatabase( } err = db.QueryRowContext(ctx, - "SELECT logged_out_at FROM logout WHERE id = 1", - ).Scan(&m.latestLogout) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - return nil, fmt.Errorf("failed to read the latest logout: %w", err) + "SELECT logout_count FROM logout WHERE id = 1", + ).Scan(&m.logoutCount) + if err != nil { + return nil, fmt.Errorf("failed to read the count of logouts: %w", err) } m.db = db @@ -117,6 +123,7 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error { Authenticated: true, CreatedAt: now, ExpiresAt: now.Add(SessionTTL), + LogoutCount: m.currentLogoutCount(), } encoded, err := m.sc.Encode(CookieName, data) @@ -165,8 +172,8 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) { return nil, ErrInvalidSession } - // A session made before the latest logout has ended. - if !data.CreatedAt.After(m.latestLogoutTime()) { + // A logout since the session was made has ended it. + if data.LogoutCount != m.currentLogoutCount() { return nil, ErrInvalidSession } @@ -174,9 +181,8 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) { } // Logout removes the session cookie and, when r has a valid session, ends -// every session made until now. A request without one ends no session: -// another site can make a browser request /logout, but the browser leaves -// the SameSite=Strict session cookie out of that request. +// every session made until now. A request without one ends no session, so +// someone who has not logged in cannot end the sessions of those who have. func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) error { if m.IsAuthenticated(r) { err := m.saveLogout(r.Context()) @@ -211,33 +217,30 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool { return err == nil && data != nil && data.Authenticated } -// saveLogout makes now the time of the latest logout, saving it in the +// saveLogout adds one to the count of logouts, saving the new count in the // database first when the Manager has one. func (m *Manager) saveLogout(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() - now := time.Now().UTC() - if m.db != nil { _, err := m.db.ExecContext(ctx, - "INSERT OR REPLACE INTO logout (id, logged_out_at) VALUES (1, ?)", - now) + "UPDATE logout SET logout_count = ? WHERE id = 1", + m.logoutCount+1) if err != nil { return fmt.Errorf("failed to save the logout: %w", err) } } - m.latestLogout = now + m.logoutCount++ return nil } -// latestLogoutTime returns the time of the latest logout, or the zero time -// when there has been none. -func (m *Manager) latestLogoutTime() time.Time { +// currentLogoutCount returns the count of logouts. +func (m *Manager) currentLogoutCount() int64 { m.mu.Lock() defer m.mu.Unlock() - return m.latestLogout + return m.logoutCount } diff --git a/internal/static/style.css b/internal/static/style.css index da22318..407c46e 100644 --- a/internal/static/style.css +++ b/internal/static/style.css @@ -122,12 +122,20 @@ header { margin-bottom: 2rem; } -header a { +/* The logout button, drawn as a link. */ + +header button { + width: auto; + margin: 0; + padding: 0; + background: none; font-size: 0.875rem; color: #4b5563; + text-decoration: underline; } -header a:hover { +header button:hover { + background: none; color: #1f2937; } diff --git a/internal/templates/generator.html b/internal/templates/generator.html index 2caaaac..6d679d2 100644 --- a/internal/templates/generator.html +++ b/internal/templates/generator.html @@ -10,9 +10,12 @@

Pixa URL Generator

- - Logout - +
+ {{ .CSRFField }} + +
{{if .GeneratedURL}} -- 2.54.0 From 066811580d10657cab82623fcb2ee1e893a0ace5 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 16:36:52 +0000 Subject: [PATCH 7/9] Test that losing the state directory ends every login session (part of #256) A pixa started on an empty state directory must refuse a session cookie from before, and a login on it must open the generator page. With the count of logouts, the new pixa starts from 0 again and accepts the old cookie, so this test fails until the count is replaced. Model: opus-5-5 --- internal/server/logout_internal_test.go | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/internal/server/logout_internal_test.go b/internal/server/logout_internal_test.go index e473301..2d46d76 100644 --- a/internal/server/logout_internal_test.go +++ b/internal/server/logout_internal_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "path/filepath" "regexp" "strings" "testing" @@ -230,3 +231,39 @@ func TestLogoutOutlastsRestart(t *testing.T) { "still opens the generator page") } } + +// TestLostStateDirectoryEndsEveryLoginSession verifies that a pixa started on +// an empty state directory, as pixad is after a restart that lost its state +// directory, refuses a session cookie from before, and that a login on it +// opens the generator page. +func TestLostStateDirectoryEndsEveryLoginSession(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + before := logIn(t, s) + if !opensGeneratorPage(t, s, before) { + t.Fatal("logging in did not open the generator page") + } + + emptyStateDir := *s.config + emptyStateDir.StateDir = t.TempDir() + emptyStateDir.DBURL = "file:" + + filepath.Join(emptyStateDir.StateDir, "state.sqlite3") + + restarted := startPixa(t, &emptyStateDir) + + opens := opensGeneratorPage(t, restarted, before) + t.Logf("on an empty state directory, the cookie from before opens the "+ + "generator page: %v", opens) + + if opens { + t.Error("on an empty state directory, a cookie from before still " + + "opens the generator page") + } + + if !opensGeneratorPage(t, restarted, logIn(t, restarted)) { + t.Error("a login on an empty state directory does not open the " + + "generator page") + } +} -- 2.54.0 From df3bff6c6bc5e5e1b6943ba7c6449f47957ed4f8 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 17:00:58 +0000 Subject: [PATCH 8/9] Replace the count of logouts with a random value (part of #256) The session manager now keeps a random value from crypto/rand in the logout table, made when the table has none and replaced by each logout. A session carries the value current at its login and is refused when it differs. A lost state directory now ends every login session, and no logout can make an ended one work again, as a count starting from 0 could. README.md and the Manager doc comment say the check for a logout compares no time, while a session's expiry still compares the clock. Model: opus-5-5 --- README.md | 19 +++--- TODO.md | 33 +++++------ internal/db/migrations/001_schema.sql | 9 ++- internal/handlers/handlers.go | 4 +- internal/session/session.go | 83 ++++++++++++++++----------- 5 files changed, 82 insertions(+), 66 deletions(-) diff --git a/README.md b/README.md index e152dea..87d3838 100644 --- a/README.md +++ b/README.md @@ -71,10 +71,10 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the database and the disk cache: - It needs a persistent volume: without one, every restart starts with an empty - cache and forgets the count of logouts, so login sessions that a logout ended - can work again (see Encrypted URLs). In the container, the startup script - gives the directory to the user pixa runs as (uid 65532) and sets its mode to - `750`; outside it, that user must be able to write the directory. + cache. Losing the directory also ends every login session (see Encrypted + URLs). In the container, the startup script gives the directory to the user + pixa runs as (uid 65532) and sets its mode to `750`; outside it, that user + must be able to write the directory. - `cache_max_bytes` limits the source and transformed images together. The database, the metadata files, the `.meta` file beside each transformed image and files still being written come on top, and eviction runs in the @@ -390,11 +390,12 @@ nor change what it asks for. lasts 30 days, or until someone logs out with the generator page's Logout button. Every login is made with the one signing key, so logging out ends every login session, in every browser, a copy of the session cookie included. - pixa keeps a count of logouts in its database: each logout adds one, a login - session carries the count at its login, and a login session whose count is - not the current one is refused, also after a restart that keeps the state - directory (see Deployment). No time is compared, so a login after a logout - works whatever the host's clock says. + pixa keeps a random value in its database, which it makes when the database + has none and replaces at each logout. A login session carries the value + current at its login and is refused once the value has changed, also after a + restart (see Deployment). The check for a logout compares no time, so a login + after a logout works whatever the host's clock says; the 30 days are still + measured by the host's clock. 2. On the generator page, give the source image's URL, the width and height, the format, quality and fit, and how long the URL lasts, then submit the form (`POST /generate`). The format is JPEG XL unless another is chosen; a form diff --git a/TODO.md b/TODO.md index 7b977ba..200293b 100644 --- a/TODO.md +++ b/TODO.md @@ -42,6 +42,23 @@ P2: security: per-origin rate limiting sizes. Storing a source over a file of another size replaces that file. A source that fails to decode is not removed for that alone: a damaged upstream image would then be fetched on every request. +- 2026-10-15 logging out ends every login session (closes #256): a login session + was a cookie with nothing kept on the server, so `/logout` removed it from one + browser and a copy of it kept working for its 30 days. Every login is made + with the one signing key, so logging out now ends them all: the session + manager keeps a random value, made with `crypto/rand` when the new `logout` + table has none and replaced by each logout, saves it there so that it outlasts + a restart, puts the value at login in each session, and refuses a session + carrying another value. The check for a logout compares no time, so a login + after a logout works whatever the host's clock does; a session's 30 days are + still checked against the clock. Losing the state directory ends every login + session, and no logout can make an ended one work again. Logging out is now + `POST /logout` from a form on the generator page, behind the CSRF check the + other forms have, so a link or image elsewhere on the same site cannot end the + sessions. A logout without a valid login session ends none and only removes + the cookie, as anyone can take a CSRF token from the login page. A logout that + cannot be saved answers 500. `session.NewManager`, which the tests use, keeps + the value in memory only; pixad uses `session.NewManagerWithDatabase`. - 2026-10-15 a transcode stops once its request's `downstream_timeout` has ended, unless it has begun encoding (closes #243): the image processor looked at its context only while it waited for a processing slot, so an image was @@ -76,22 +93,6 @@ P2: security: per-origin rate limiting 405, or a CORS preflight. The metrics library takes the label before the request is routed, so the metrics middleware asks the router which route matches. -- 2026-10-15 logging out ends every login session (closes #256): a login session - was a cookie with nothing kept on the server, so `/logout` removed it from one - browser and a copy of it kept working for its 30 days. Every login is made - with the one signing key, so logging out now ends them all: the session - manager counts logouts, saves the count in the new `logout` table so that it - outlasts a restart, puts the count at login in each session, and refuses a - session whose count is not the current one. No time is compared, so a login - after a logout works whatever the host's clock does. A restart without a - persistent volume for the state directory forgets the count. Logging out is - now `POST /logout` from a form on the generator page, behind the CSRF check - the other forms have, so a link or image elsewhere on the same site cannot end - the sessions. A logout without a valid login session ends none and only - removes the cookie, as anyone can take a CSRF token from the login page. A - logout that cannot be saved answers 500. `session.NewManager`, which the tests - use, keeps the count in memory only; pixad uses - `session.NewManagerWithDatabase`. - 2026-10-15 expired negative cache rows are removed on a schedule (closes #244): the row of a failed fetch, which makes pixa answer 502 for that source URL for 5 minutes, was deleted only when the same URL was requested again diff --git a/internal/db/migrations/001_schema.sql b/internal/db/migrations/001_schema.sql index 2e018af..234a141 100644 --- a/internal/db/migrations/001_schema.sql +++ b/internal/db/migrations/001_schema.sql @@ -177,11 +177,10 @@ CREATE TABLE IF NOT EXISTS cache_stats ( ); INSERT OR IGNORE INTO cache_stats (id) VALUES (1); --- Count of logouts. Each logout adds one; a login session carries the count --- at its login and is refused once the count has changed, so logging out ends --- every login session. +-- A random value that a login session carries from its login. pixa makes it +-- when the table has no row, and each logout replaces it; a login session +-- carrying another value is refused, so logging out ends every login session. CREATE TABLE IF NOT EXISTS logout ( id INTEGER PRIMARY KEY CHECK (id = 1), - logout_count INTEGER NOT NULL DEFAULT 0 + logout_value TEXT NOT NULL ); -INSERT OR IGNORE INTO logout (id) VALUES (1); diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index c82b621..9cfc3bb 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -164,8 +164,8 @@ func (s *Handlers) initImageService() error { // Initialize session manager (signing key is validated at config load // time). Session cookies are always Secure/HttpOnly/SameSite=Strict. - // The count of logouts is saved in the database, so it outlasts a - // restart. + // The random value that each logout replaces is saved in the database, + // so it outlasts a restart. sessMgr, err := session.NewManagerWithDatabase( context.Background(), s.config.SigningKey, s.db.DB()) if err != nil { diff --git a/internal/session/session.go b/internal/session/session.go index 06c77bd..788c114 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -1,10 +1,11 @@ -// Package session provides encrypted session cookie management, and keeps a -// count of logouts in the database so that a logout ends every session made -// before it. +// Package session provides encrypted session cookie management, and keeps in +// the database a random value that each logout replaces, so that a logout +// ends every session made before it. package session import ( "context" + "crypto/rand" "database/sql" "errors" "fmt" @@ -39,33 +40,35 @@ type Data struct { CreatedAt time.Time `json:"created"` ExpiresAt time.Time `json:"expires"` - // LogoutCount is the count of logouts when the session was made. - LogoutCount int64 `json:"logoutCount"` + // LogoutValue is the Manager's random value, which each logout + // replaces, when the session was made. + LogoutValue string `json:"logoutValue"` } // Manager handles session creation and validation using encrypted cookies. // // Every login is made with the one signing key, so a logout ends every -// session made before it, in every browser. The Manager counts logouts: each -// logout adds one, a session carries the count at its login, and a session -// whose count is not the current one is refused. No clock is compared, so a -// login after a logout works whatever the host's clock does. The Manager -// keeps the count in memory, so checking a session reads no database; a -// Manager from NewManagerWithDatabase also saves it in the database, so that -// it outlasts a restart. +// session made before it, in every browser. The Manager keeps a random +// value, which each logout replaces: a session carries the value current at +// its login, and a session carrying another value is refused. The check for +// a logout compares no time, so a login after a logout works whatever the +// host's clock does; the session's expiry is still checked against the +// clock. The Manager keeps the value in memory, so checking a session reads +// no database; a Manager from NewManagerWithDatabase also saves it in the +// database, so that it outlasts a restart. type Manager struct { sc *securecookie.SecureCookie db *sql.DB - // mu guards logoutCount, and is held while a logout is saved, so that - // the database and logoutCount agree. + // mu guards logoutValue, and is held while a logout is saved, so that + // the database and logoutValue agree. mu sync.Mutex - logoutCount int64 + logoutValue string } // NewManager creates a session manager with keys derived from the signing key. -// It keeps the count of logouts in memory only, so a restart forgets it; -// NewManagerWithDatabase keeps it in the database. +// It makes a new random value and keeps it in memory only, so a restart ends +// every session; NewManagerWithDatabase keeps the value in the database. // // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict // attributes; this cannot be configured. Browsers treat http://localhost as a @@ -89,13 +92,15 @@ func NewManager(signingKey string) (*Manager, error) { sc.MaxAge(int(SessionTTL.Seconds())) return &Manager{ - sc: sc, + sc: sc, + logoutValue: rand.Text(), }, nil } -// NewManagerWithDatabase creates a session manager like NewManager that saves -// the count of logouts in db, which must have pixa's schema, and starts from -// the count saved there. +// NewManagerWithDatabase creates a session manager like NewManager that keeps +// its random value in db, which must have pixa's schema: it saves the value +// NewManager made when db has none, and otherwise starts from the one saved +// there. func NewManagerWithDatabase( ctx context.Context, signingKey string, db *sql.DB, ) (*Manager, error) { @@ -104,11 +109,20 @@ func NewManagerWithDatabase( return nil, err } - err = db.QueryRowContext(ctx, - "SELECT logout_count FROM logout WHERE id = 1", - ).Scan(&m.logoutCount) + _, err = db.ExecContext(ctx, + "INSERT OR IGNORE INTO logout (id, logout_value) VALUES (1, ?)", + m.logoutValue) if err != nil { - return nil, fmt.Errorf("failed to read the count of logouts: %w", err) + return nil, fmt.Errorf( + "failed to save the random value in the logout table: %w", err) + } + + err = db.QueryRowContext(ctx, + "SELECT logout_value FROM logout WHERE id = 1", + ).Scan(&m.logoutValue) + if err != nil { + return nil, fmt.Errorf( + "failed to read the random value in the logout table: %w", err) } m.db = db @@ -123,7 +137,7 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error { Authenticated: true, CreatedAt: now, ExpiresAt: now.Add(SessionTTL), - LogoutCount: m.currentLogoutCount(), + LogoutValue: m.currentLogoutValue(), } encoded, err := m.sc.Encode(CookieName, data) @@ -173,7 +187,7 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) { } // A logout since the session was made has ended it. - if data.LogoutCount != m.currentLogoutCount() { + if data.LogoutValue != m.currentLogoutValue() { return nil, ErrInvalidSession } @@ -217,30 +231,31 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool { return err == nil && data != nil && data.Authenticated } -// saveLogout adds one to the count of logouts, saving the new count in the +// saveLogout replaces the random value with a new one, saving it in the // database first when the Manager has one. func (m *Manager) saveLogout(ctx context.Context) error { + value := rand.Text() + m.mu.Lock() defer m.mu.Unlock() if m.db != nil { _, err := m.db.ExecContext(ctx, - "UPDATE logout SET logout_count = ? WHERE id = 1", - m.logoutCount+1) + "UPDATE logout SET logout_value = ? WHERE id = 1", value) if err != nil { return fmt.Errorf("failed to save the logout: %w", err) } } - m.logoutCount++ + m.logoutValue = value return nil } -// currentLogoutCount returns the count of logouts. -func (m *Manager) currentLogoutCount() int64 { +// currentLogoutValue returns the Manager's random value. +func (m *Manager) currentLogoutValue() string { m.mu.Lock() defer m.mu.Unlock() - return m.logoutCount + return m.logoutValue } -- 2.54.0 From c69bd4c5bd2da27880b80eb52ee64b717718c172 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 18:16:42 +0000 Subject: [PATCH 9/9] Move the logout entry to the top of Completed Steps (part of #256) The rebase onto next union-merged TODO.md and left this entry below the one next added. Model: opus-5-5 --- TODO.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/TODO.md b/TODO.md index 200293b..724a404 100644 --- a/TODO.md +++ b/TODO.md @@ -30,18 +30,6 @@ P2: security: per-origin rate limiting # Completed Steps -- 2026-10-15 a cached file left empty or partial is no longer served, or fetched - again on every request (closes #252). Source and transformed image files, and - each transformed image's `.meta` file, are synced to disk before they are - renamed into place, and their directory after; the `.meta` file is written - before the image, and a failed directory sync removes the renamed file. The - reconciliation pass, at startup and every five minutes, removes a cached - source or transformed image whose file is empty, or not the size the database - records for it, with its rows and a warning naming the file, so the next - request fetches the source or converts the image again; requests compare no - sizes. Storing a source over a file of another size replaces that file. A - source that fails to decode is not removed for that alone: a damaged upstream - image would then be fetched on every request. - 2026-10-15 logging out ends every login session (closes #256): a login session was a cookie with nothing kept on the server, so `/logout` removed it from one browser and a copy of it kept working for its 30 days. Every login is made @@ -59,6 +47,18 @@ P2: security: per-origin rate limiting the cookie, as anyone can take a CSRF token from the login page. A logout that cannot be saved answers 500. `session.NewManager`, which the tests use, keeps the value in memory only; pixad uses `session.NewManagerWithDatabase`. +- 2026-10-15 a cached file left empty or partial is no longer served, or fetched + again on every request (closes #252). Source and transformed image files, and + each transformed image's `.meta` file, are synced to disk before they are + renamed into place, and their directory after; the `.meta` file is written + before the image, and a failed directory sync removes the renamed file. The + reconciliation pass, at startup and every five minutes, removes a cached + source or transformed image whose file is empty, or not the size the database + records for it, with its rows and a warning naming the file, so the next + request fetches the source or converts the image again; requests compare no + sizes. Storing a source over a file of another size replaces that file. A + source that fails to decode is not removed for that alone: a damaged upstream + image would then be fetched on every request. - 2026-10-15 a transcode stops once its request's `downstream_timeout` has ended, unless it has begun encoding (closes #243): the image processor looked at its context only while it waited for a processing slot, so an image was -- 2.54.0