From c856f10619c65cd04d3babfee8b13035f68ab7c6 Mon Sep 17 00:00:00 2001 From: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:10:18 +0200 Subject: [PATCH 1/4] fix(api): store only a SHA-256 digest of session values Sessions now store only the SHA-256 digest of the browser session value instead of the value itself, preventing a leaked database from yielding usable session cookies. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018MnGTGHzHhFQCa22wRknUS Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> --- api/internal/auth/sessions.go | 27 +- api/internal/auth/sessions_digest_test.go | 327 ++++++++++++++++++ api/internal/auth/sessions_test.go | 12 +- .../common/014_sessions_digest_reset.sql | 6 + api/internal/db/migrations_test.go | 95 +++++ api/specs.md | 9 +- docs/security.md | 2 +- 7 files changed, 461 insertions(+), 17 deletions(-) create mode 100644 api/internal/auth/sessions_digest_test.go create mode 100644 api/internal/db/migrations/common/014_sessions_digest_reset.sql diff --git a/api/internal/auth/sessions.go b/api/internal/auth/sessions.go index ddc72af33..1e7f3559d 100644 --- a/api/internal/auth/sessions.go +++ b/api/internal/auth/sessions.go @@ -3,7 +3,9 @@ package auth import ( "context" "crypto/rand" + "crypto/sha256" "encoding/base64" + "encoding/hex" "errors" "log/slog" "net/http" @@ -49,15 +51,25 @@ type SessionStore struct { // NewSessionStore returns a new SessionStore. func NewSessionStore(store *db.Store) *SessionStore { return &SessionStore{db: store} } +// sessionDigest returns the hex SHA-256 of a raw session cookie value. The +// sessions table stores only this digest (column "token"), never the cookie +// value, so a copy of the database cannot be replayed as a session cookie. +// The raw value is 256 bits of CSPRNG output, so a plain unsalted hash is +// sufficient and keeps lookup a single equality match. +func sessionDigest(raw string) string { + sum := sha256.Sum256([]byte(raw)) + return hex.EncodeToString(sum[:]) +} + // Create writes a new session row for the given user and returns the -// values that need to go back on the response (session cookie value + -// CSRF token). +// values that need to go back on the response (raw session cookie value + +// CSRF token). Only the digest of the session value is persisted. func (s *SessionStore) Create(ctx context.Context, userID int64) (session, csrf string, err error) { session = randomToken() csrf = randomToken() _, err = s.db.DB.ExecContext(ctx, `INSERT INTO sessions(token, user_id, csrf_token, expires_at) VALUES (?,?,?,?)`, - session, userID, csrf, time.Now().Add(sessionTTL).UTC().Format(time.RFC3339), + sessionDigest(session), userID, csrf, time.Now().Add(sessionTTL).UTC().Format(time.RFC3339), ) return } @@ -141,6 +153,7 @@ func (s *SessionStore) Authenticate(next http.Handler) http.Handler { } func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, error) { + digest := sessionDigest(token) var ( u User csrf string @@ -149,7 +162,7 @@ func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, err := s.db.DB.QueryRowContext(ctx, ` SELECT u.id, u.username, u.display_name, u.email, u.role, s.csrf_token, s.expires_at FROM sessions s JOIN users u ON u.id = s.user_id - WHERE s.token = ?`, token, + WHERE s.token = ?`, digest, ).Scan(&u.ID, &u.Username, &u.DisplayName, &u.Email, &u.Role, &csrf, &expires) if err != nil { return nil, "", err @@ -165,11 +178,11 @@ func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, // session from the client's perspective — delete it and make the // user log in again. Log so it's visible in ops. slog.Warn("session expires_at parse", "err", perr, "value", expires) - _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, token) + _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, digest) return nil, "", errors.New("session invalid") } if time.Now().After(exp) { - _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, token) + _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, digest) return nil, "", errors.New("session expired") } return &u, csrf, nil @@ -179,7 +192,7 @@ func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, func (s *SessionStore) HandleLogout() http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { if c, err := req.Cookie(sessionCookie); err == nil { - _, _ = s.db.DB.ExecContext(req.Context(), `DELETE FROM sessions WHERE token = ?`, c.Value) + _, _ = s.db.DB.ExecContext(req.Context(), `DELETE FROM sessions WHERE token = ?`, sessionDigest(c.Value)) } clearCookie(w, sessionCookie) // The CSRF cookie is JS-readable while set (see setCSRFCookie), but diff --git a/api/internal/auth/sessions_digest_test.go b/api/internal/auth/sessions_digest_test.go new file mode 100644 index 000000000..0d694b225 --- /dev/null +++ b/api/internal/auth/sessions_digest_test.go @@ -0,0 +1,327 @@ +package auth + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestSessionDigest_Deterministic(t *testing.T) { + a := sessionDigest("a") + if a != sessionDigest("a") { + t.Error("sessionDigest not deterministic") + } + b := sessionDigest("b") + if a == b { + t.Error("sessionDigest('a') == sessionDigest('b')") + } + if len(a) != 64 { + t.Errorf("digest length %d, want 64", len(a)) + } + expected := hex.EncodeToString(sha256.Sum256([]byte("a"))[:]) + if a != expected { + t.Errorf("sessionDigest('a') = %s, want %s", a, expected) + } +} + +func TestSessions_StoresDigestNotCookieValue(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + tok, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create: %v", err) + } + + var stored string + err = s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + if err != nil { + t.Fatalf("query token: %v", err) + } + if stored == tok { + t.Error("stored token == raw cookie value (should be digest)") + } + if stored != sessionDigest(tok) { + t.Errorf("stored token %s, want sessionDigest(%s)=%s", stored, tok, sessionDigest(tok)) + } + var count int + err = s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, tok).Scan(&count) + if err != nil { + t.Fatalf("count query: %v", err) + } + if count != 0 { + t.Errorf("count rows WHERE token=raw value: %d, want 0", count) + } +} + +func TestSessions_StoredValueCannotBeUsedAsCookie(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + tok, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create: %v", err) + } + + var stored string + err = s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + if err != nil { + t.Fatalf("query token: %v", err) + } + + called := false + handler := s.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusNoContent) + })) + + // Try with stored digest as cookie - should fail + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusUnauthorized { + t.Errorf("GET with stored digest cookie: %d, want 401", rr.Code) + } + if called { + t.Error("handler called with stored digest cookie") + } + + // Try with raw token as cookie - should succeed + req = httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusNoContent { + t.Errorf("GET with raw token cookie: %d, want 204", rr.Code) + } + if !called { + t.Error("handler not called with raw token cookie") + } + + // lookup with stored digest should error + _, _, err = s.lookup(ctx, stored) + if err == nil { + t.Error("lookup(stored digest) should error") + } + + // POST with stored digest as cookie and correct CSRF should still fail + req = httptest.NewRequest(http.MethodPost, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) + req.Header.Set(csrfHeader, "anything") // Wrong but proves CSRF isn't checked first + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusUnauthorized { + t.Errorf("POST with stored digest cookie: %d, want 401", rr.Code) + } +} + +func TestSessions_LogoutDeletesRowByDigest(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + + // Create two sessions + tokA, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create A: %v", err) + } + tokB, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create B: %v", err) + } + + // Logout with tokA + handler := s.HandleLogout() + req := httptest.NewRequest(http.MethodPost, "/logout", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tokA}) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + // Check tokA deleted, tokB still exists + var countA, countB int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)).Scan(&countA) + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)).Scan(&countB) + + if countA != 0 { + t.Errorf("tokA row count after logout: %d, want 0", countA) + } + if countB != 1 { + t.Errorf("tokB row count after logout: %d, want 1", countB) + } + + // tokB should still authenticate + _, _, err = s.lookup(ctx, tokB) + if err != nil { + t.Errorf("lookup(tokB) after logout: %v", err) + } + + // Logout presenting the stored digest as cookie should delete nothing + var stored string + s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + req = httptest.NewRequest(http.MethodPost, "/logout", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + var countUser int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE user_id=1`).Scan(&countUser) + if countUser != 1 { + t.Errorf("user 1 row count after logout with digest: %d, want 1", countUser) + } +} + +func TestSessions_ExpiredDigestRowRemovedOnLookupAndGC(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + + tokA, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create A: %v", err) + } + tokB, _, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create B: %v", err) + } + + // Set both to expired + expiredTime := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + _, err = s.db.DB.ExecContext(ctx, + `UPDATE sessions SET expires_at = ? WHERE token = ?`, + expiredTime, sessionDigest(tokA)) + if err != nil { + t.Fatalf("update tokA expiry: %v", err) + } + _, err = s.db.DB.ExecContext(ctx, + `UPDATE sessions SET expires_at = ? WHERE token = ?`, + expiredTime, sessionDigest(tokB)) + if err != nil { + t.Fatalf("update tokB expiry: %v", err) + } + + // lookup tokA should error and remove row + _, _, err = s.lookup(ctx, tokA) + if err == nil { + t.Error("lookup(expired tokA) should error") + } + var countA int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)).Scan(&countA) + if countA != 0 { + t.Errorf("tokA row count after lookup: %d, want 0", countA) + } + + // GC should remove tokB + s.gcOnce(ctx) + var countB int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)).Scan(&countB) + if countB != 0 { + t.Errorf("tokB row count after gc: %d, want 0", countB) + } + + var countAll int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions`).Scan(&countAll) + if countAll != 0 { + t.Errorf("total session count: %d, want 0", countAll) + } +} + +func TestSessions_DeleteForUserRevokesEveryDigestRow(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + seedUser(t, s, "bob", "pw", "viewer") + + // Two sessions for alice (id=1), one for bob (id=2) + aliceA, _, _ := s.Create(ctx, 1) + aliceB, _, _ := s.Create(ctx, 1) + bob, _, _ := s.Create(ctx, 2) + + // Delete alice's sessions + err := s.DeleteForUser(ctx, 1) + if err != nil { + t.Fatalf("DeleteForUser: %v", err) + } + + // alice's tokens should fail lookup + _, _, err = s.lookup(ctx, aliceA) + if err == nil { + t.Error("lookup(aliceA after delete) should error") + } + _, _, err = s.lookup(ctx, aliceB) + if err == nil { + t.Error("lookup(aliceB after delete) should error") + } + + // bob's token should still work + _, _, err = s.lookup(ctx, bob) + if err != nil { + t.Errorf("lookup(bob after alice delete): %v", err) + } + + // Check row count + var count int + s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE user_id=2`).Scan(&count) + if count != 1 { + t.Errorf("bob row count: %d, want 1", count) + } +} + +func TestSessions_CSRFStillEnforcedWithDigestStorage(t *testing.T) { + s := newAuthDB(t) + ctx := context.Background() + seedUser(t, s, "alice", "pw", "admin") + tok, csrf, err := s.Create(ctx, 1) + if err != nil { + t.Fatalf("Create: %v", err) + } + + handler := s.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + + // POST with raw cookie and no CSRF header -> 403 + req := httptest.NewRequest(http.MethodPost, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Errorf("POST no CSRF: %d, want 403", rr.Code) + } + + // POST with wrong CSRF -> 403 + req = httptest.NewRequest(http.MethodPost, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + req.Header.Set(csrfHeader, "wrong") + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Errorf("POST wrong CSRF: %d, want 403", rr.Code) + } + + // POST with correct CSRF -> 204 + req = httptest.NewRequest(http.MethodPost, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + req.Header.Set(csrfHeader, csrf) + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusNoContent { + t.Errorf("POST correct CSRF: %d, want 204", rr.Code) + } + + // POST with stored digest as CSRF header -> 403 + var stored string + s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + req = httptest.NewRequest(http.MethodPost, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + req.Header.Set(csrfHeader, stored) + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Errorf("POST with digest as CSRF: %d, want 403", rr.Code) + } +} diff --git a/api/internal/auth/sessions_test.go b/api/internal/auth/sessions_test.go index 29912d82e..ac48a40fb 100644 --- a/api/internal/auth/sessions_test.go +++ b/api/internal/auth/sessions_test.go @@ -41,7 +41,7 @@ func TestSessions_Lookup_Expired(t *testing.T) { } // Force-expire the row directly. if _, err := s.DB.ExecContext(context.Background(), `UPDATE sessions SET expires_at = ? WHERE token = ?`, - time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), tok); err != nil { + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), sessionDigest(tok)); err != nil { t.Fatalf("update: %v", err) } if _, _, err := store.lookup(context.Background(), tok); err == nil || @@ -55,7 +55,7 @@ func TestSessions_Lookup_CorruptExpires(t *testing.T) { seedUser(t, s, "alice", "pw", "admin") store := NewSessionStore(s) tok, _, _ := store.Create(context.Background(), 1) - if _, err := s.DB.ExecContext(context.Background(), `UPDATE sessions SET expires_at = 'garbage' WHERE token = ?`, tok); err != nil { + if _, err := s.DB.ExecContext(context.Background(), `UPDATE sessions SET expires_at = 'garbage' WHERE token = ?`, sessionDigest(tok)); err != nil { t.Fatalf("update: %v", err) } if _, _, err := store.lookup(context.Background(), tok); err == nil || @@ -64,7 +64,7 @@ func TestSessions_Lookup_CorruptExpires(t *testing.T) { } // Row should also be deleted. var n int - _ = s.DB.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM sessions WHERE token = ?`, tok).Scan(&n) + _ = s.DB.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tok)).Scan(&n) if n != 0 { t.Fatalf("corrupt row not removed (%d rows remaining)", n) } @@ -182,7 +182,7 @@ func TestSessions_HandleLogout(t *testing.T) { } // Row deleted. var n int - _ = s.DB.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM sessions WHERE token=?`, tok).Scan(&n) + _ = s.DB.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM sessions WHERE token=?`, sessionDigest(tok)).Scan(&n) if n != 0 { t.Fatalf("session row not deleted") } @@ -205,7 +205,7 @@ func TestSessions_GCOnce_DeletesExpired(t *testing.T) { store := NewSessionStore(s) tok, _, _ := store.Create(context.Background(), 1) _, _ = s.DB.ExecContext(context.Background(), `UPDATE sessions SET expires_at = ? WHERE token = ?`, - time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), tok) + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), sessionDigest(tok)) store.gcOnce(context.Background()) var n int _ = s.DB.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM sessions`).Scan(&n) @@ -220,7 +220,7 @@ func TestSessions_StartGC(t *testing.T) { store := NewSessionStore(s) tok, _, _ := store.Create(context.Background(), 1) _, _ = s.DB.ExecContext(context.Background(), `UPDATE sessions SET expires_at = ? WHERE token = ?`, - time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), tok) + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), sessionDigest(tok)) ctx, cancel := context.WithCancel(context.Background()) defer cancel() store.StartGC(ctx, 20*time.Millisecond) diff --git a/api/internal/db/migrations/common/014_sessions_digest_reset.sql b/api/internal/db/migrations/common/014_sessions_digest_reset.sql new file mode 100644 index 000000000..57bbd07d9 --- /dev/null +++ b/api/internal/db/migrations/common/014_sessions_digest_reset.sql @@ -0,0 +1,6 @@ +-- Sessions now store a SHA-256 digest of the browser session value instead +-- of the value itself (see auth.sessionDigest). Rows written before this +-- migration hold raw cookie values, which can no longer be matched, so they +-- are deliberately invalidated: every user signs in again once after the +-- upgrade. Rows created afterwards hold digests. +DELETE FROM sessions; diff --git a/api/internal/db/migrations_test.go b/api/internal/db/migrations_test.go index 349380375..7c0be8a81 100644 --- a/api/internal/db/migrations_test.go +++ b/api/internal/db/migrations_test.go @@ -1,6 +1,7 @@ package db import ( + "errors" "io/fs" "regexp" "strings" @@ -288,3 +289,97 @@ func TestNowTimestamp_Format(t *testing.T) { t.Fatalf("NowTimestamp() = %q is not the current UTC time", ts) } } + +func TestMigrate_SessionDigestResetDeletesExistingSessions(t *testing.T) { + s := newRBACStore(t) + ctx := t.Context() + + // Insert a test user + _, err := s.DB.ExecContext(ctx, + `INSERT INTO users(username, display_name, email, role) VALUES (?,?,?,?)`, + "sess-user", "sess-user", "sess@example.com", "viewer") + if err != nil { + t.Fatalf("insert user: %v", err) + } + + // Insert two sessions with raw-looking tokens + _, err = s.DB.ExecContext(ctx, + `INSERT INTO sessions(token, user_id, csrf_token, expires_at) VALUES (?,?,?,?)`, + "raw-1", 1, "csrf-1", "2999-01-01T00:00:00Z") + if err != nil { + t.Fatalf("insert session 1: %v", err) + } + _, err = s.DB.ExecContext(ctx, + `INSERT INTO sessions(token, user_id, csrf_token, expires_at) VALUES (?,?,?,?)`, + "raw-2", 1, "csrf-2", "2999-01-01T00:00:00Z") + if err != nil { + t.Fatalf("insert session 2: %v", err) + } + + // Delete the migration marker to re-run migration 014 + _, err = s.DB.ExecContext(ctx, `DELETE FROM schema_migrations WHERE version = ?`, "014_sessions_digest_reset.sql") + if err != nil { + t.Fatalf("delete migration marker: %v", err) + } + + // Run migrations - 014 should delete all sessions + if err := s.Migrate(ctx); err != nil { + t.Fatalf("migrate: %v", err) + } + + // Verify sessions were deleted + var count int + err = s.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions`).Scan(&count) + if err != nil { + t.Fatalf("count sessions: %v", err) + } + if count != 0 { + t.Errorf("session count after 014: %d, want 0", count) + } + + // Verify migration version was recorded + var v string + err = s.DB.QueryRowContext(ctx, `SELECT version FROM schema_migrations WHERE version = ?`, "014_sessions_digest_reset.sql").Scan(&v) + if err != nil { + t.Errorf("migration 014 not recorded: %v", err) + } + + // Insert a new session after migration + _, err = s.DB.ExecContext(ctx, + `INSERT INTO sessions(token, user_id, csrf_token, expires_at) VALUES (?,?,?,?)`, + "digest-row", 1, "csrf-3", "2999-01-01T00:00:00Z") + if err != nil { + t.Fatalf("insert session after 014: %v", err) + } + + // Run migrations again - 014 should not re-run + if err := s.Migrate(ctx); err != nil { + t.Fatalf("migrate again: %v", err) + } + + // Verify the new session still exists (migration not re-run) + err = s.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, "digest-row").Scan(&count) + if err != nil { + t.Fatalf("count new session: %v", err) + } + if count != 1 { + t.Errorf("new session count after re-run: %d, want 1", count) + } +} + +func TestSharedMigrations_IncludeSessionDigestReset(t *testing.T) { + files, err := listSQL(migrations, sharedMigrationDir) + if err != nil { + t.Fatalf("list shared migrations: %v", err) + } + var found bool + for _, f := range files { + if f.name == "014_sessions_digest_reset.sql" { + found = true + break + } + } + if !found { + t.Error("014_sessions_digest_reset.sql not found in shared migrations") + } +} diff --git a/api/specs.md b/api/specs.md index c7d8e5b1a..8b04bd25f 100644 --- a/api/specs.md +++ b/api/specs.md @@ -530,11 +530,11 @@ Verify from `/api/go.mod`. - Runtime SQL is written once with `?` placeholders; the Postgres connector rewrites them to `$n` (`db.Rebind`). Timestamps that used SQLite's `datetime('now')` are generated in Go (`db.NowTimestamp()`, same `YYYY-MM-DD HH:MM:SS` UTC text) and bound as parameters; inserted ids come from `RETURNING id` (pgx has no `LastInsertId`) - Postgres legacy migrations declare the text columns the API sorts or range-compares (`roles.name`, role-binding scope columns, `audit_events.ts`, `sessions.expires_at`, `share_links.created_at`/`expires_at`) `COLLATE "C"` so ordering matches SQLite's byte-wise collation -### Schema (migrations 001-012) +### Schema (migrations 001-012, 014) **001_init.sql:** - `users` — username (unique), email, display_name, pw_hash (argon2id), role (legacy, now via role_bindings), created_at, updated_at -- `sessions` — token (PK), user_id (FK), csrf_token, expires_at +- `sessions` — token (PK; since 014 holds the hex SHA-256 digest of the session cookie value, never the value), user_id (FK), csrf_token, expires_at - `oidc_links` — (issuer, subject) -> user_id (many-to-one); email claim - `audit_events` — ts, actor, method, path, target, status, ip - `api_tokens` — token (PK), user_id, name, last_used @@ -588,6 +588,9 @@ Verify from `/api/go.mod`. **012_account_removal_cleanup.sql:** (account removal cleanup) - One-off pass that deletes `oidc_links`, `user_preferences`, `sessions`, `api_tokens` and `user_role_bindings` rows whose user no longer exists, and revokes (sets `revoked_at`, RFC3339 UTC) active `share_links` whose creator no longer exists. Clears rows left by user deletes made before `db.Store.DeleteUser` removed them explicitly; forward-only, so a rollback needs the pre-upgrade DB snapshot +**014_sessions_digest_reset.sql:** (session digest storage) +- `DELETE FROM sessions` once. Sessions switched to storing a SHA-256 digest of the cookie value (`auth.sessionDigest`); pre-existing rows hold raw values and are deliberately invalidated, so every user signs in again once after upgrade. Numbered 014 because 013 is used by another shared migration; the runner applies unrecorded files in any order. Forward-only: a rollback to a pre-digest binary also invalidates digest-era sessions. + Foreign keys are enforced only on Postgres (modernc-sqlite runs with FK OFF); the API layer is authoritative and deletes dependent rows itself, so the Postgres cascades never change the outcome. The one exception is `share_links.created_by`: the Postgres schema declares it as a plain column with no foreign key, so deleting a user keeps that user's share links (revoked) in the audit trail on both drivers, instead of cascading them away. ## Security considerations @@ -595,7 +598,7 @@ Foreign keys are enforced only on Postgres (modernc-sqlite runs with FK OFF); th ### Authentication - **Local:** argon2id (Argon2id13, m=64MiB, t=3, p=2) with per-user random salt; ~200ms per check; operations are bounded by a 2-slot weighted semaphore that honours request context, so client disconnects abort pending argon2 operations and prevent stalling under concurrent login attempts - **OIDC:** `coreos/go-oidc/v3` discovery + `go-jose/v4` JWT validation; claims mapping (email, groups, roles) -- **Sessions:** cryptographically random token + paired CSRF token; DB-only persistence (no in-memory store), 12-hour TTL from creation, garbage-collected on an interval (`SessionStore.StartGC`) +- **Sessions:** cryptographically random token + paired CSRF token; only the SHA-256 digest (hex) of the token is persisted (the cookie carries the raw value, so a database copy cannot be replayed as a cookie; lookup, logout and cleanup hash the presented value first); DB-only persistence (no in-memory store), 12-hour TTL from creation, garbage-collected on an interval (`SessionStore.StartGC`) - **CSRF cookie is JS-readable by design:** unlike the session cookie (`HttpOnly`), the CSRF cookie is set `HttpOnly: false` so the SPA can read its value and echo it back as `X-Gameplane-CSRF` on mutating requests — the standard double-submit pattern (see `docs/security.md`). Logout's cookie-clear always sends `HttpOnly: true` regardless, since a MaxAge<0 delete carries no value and the browser matches it on Name/Domain/Path alone. - **Bootstrap:** `bootstrap-admin` subcommand hashes password same way as API - **Client IP:** `auth.ClientIPFromTrustedProxies` (`internal/auth/clientip.go`) records the client IP that the rate limiters and audit key on. A TCP peer outside `--trusted-proxies` is the client and its `X-Forwarded-For` is ignored. Behind a trusted peer the header is read right to left up to the first address outside the list, or to the leftmost address when every hop is trusted; an entry that isn't an IP address ends the walk at the last address reached. An empty list makes the peer the client. Tests: `internal/auth/clientip_test.go`. diff --git a/docs/security.md b/docs/security.md index 7c85408ef..c93dc7ff7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -15,7 +15,7 @@ point. Assume: Two modes, configurable independently: - **Local accounts** — argon2id (64 MiB, t=3, p=2) password hashing. - Session cookies are HttpOnly, Secure, SameSite=Lax. CSRF protection + Session cookies are HttpOnly, Secure, SameSite=Lax. The database stores only a SHA-256 digest of the session cookie value, so a leaked database or backup does not yield usable session cookies; the digest itself is rejected if presented as a cookie. CSRF protection via a double-submit `X-Gameplane-CSRF` header on mutating requests. - **OIDC** — Keycloak, Google, GitHub, any RFC-7519 compliant IdP. State validated through a short-lived cookie; `id_token` signature From d5897e73febf5acaa340c2ed7fe754d30e27b2df Mon Sep 17 00:00:00 2001 From: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:29:14 +0200 Subject: [PATCH 2/4] test(api): make the session digest tests compile Remove unused errors import from migrations_test.go and add the compiled sessions_digest_test.go fixture. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018MnGTGHzHhFQCa22wRknUS Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> --- api/internal/auth/sessions_digest_test.go | 452 +++++++++++----------- api/internal/db/migrations_test.go | 1 - 2 files changed, 233 insertions(+), 220 deletions(-) diff --git a/api/internal/auth/sessions_digest_test.go b/api/internal/auth/sessions_digest_test.go index 0d694b225..b256aa59d 100644 --- a/api/internal/auth/sessions_digest_test.go +++ b/api/internal/auth/sessions_digest_test.go @@ -4,324 +4,338 @@ import ( "context" "crypto/sha256" "encoding/hex" + "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "time" + + "github.com/ValgulNecron/gameplane/api/internal/db" ) +// storedSessionToken returns the sessions.token value stored for userID's +// only session. +func storedSessionToken(t *testing.T, s *db.Store, userID int64) string { + t.Helper() + var stored string + if err := s.DB.QueryRowContext(context.Background(), + `SELECT token FROM sessions WHERE user_id = ?`, userID).Scan(&stored); err != nil { + t.Fatalf("read stored session token: %v", err) + } + return stored +} + +// countSessions runs a SELECT COUNT(*) query against the sessions table. +func countSessions(t *testing.T, s *db.Store, query string, args ...any) int { + t.Helper() + var n int + if err := s.DB.QueryRowContext(context.Background(), query, args...).Scan(&n); err != nil { + t.Fatalf("count sessions: %v", err) + } + return n +} + func TestSessionDigest_Deterministic(t *testing.T) { a := sessionDigest("a") if a != sessionDigest("a") { - t.Error("sessionDigest not deterministic") + t.Fatal("sessionDigest is not deterministic") } - b := sessionDigest("b") - if a == b { - t.Error("sessionDigest('a') == sessionDigest('b')") + if a == sessionDigest("b") { + t.Fatal("sessionDigest(a) == sessionDigest(b)") } - if len(a) != 64 { - t.Errorf("digest length %d, want 64", len(a)) + sum := sha256.Sum256([]byte("a")) + if want := hex.EncodeToString(sum[:]); a != want { + t.Fatalf("sessionDigest(a) = %q, want %q", a, want) } - expected := hex.EncodeToString(sha256.Sum256([]byte("a"))[:]) - if a != expected { - t.Errorf("sessionDigest('a') = %s, want %s", a, expected) + if len(a) != 64 { + t.Fatalf("digest length = %d, want 64", len(a)) } } func TestSessions_StoresDigestNotCookieValue(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - tok, _, err := s.Create(ctx, 1) - if err != nil { - t.Fatalf("Create: %v", err) - } - - var stored string - err = s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + store := NewSessionStore(s) + tok, _, err := store.Create(context.Background(), 1) if err != nil { - t.Fatalf("query token: %v", err) + t.Fatalf("create: %v", err) } + stored := storedSessionToken(t, s, 1) if stored == tok { - t.Error("stored token == raw cookie value (should be digest)") + t.Fatal("sessions.token holds the raw cookie value") } if stored != sessionDigest(tok) { - t.Errorf("stored token %s, want sessionDigest(%s)=%s", stored, tok, sessionDigest(tok)) + t.Fatalf("sessions.token = %q, want sessionDigest(cookie) = %q", stored, sessionDigest(tok)) } - var count int - err = s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, tok).Scan(&count) - if err != nil { - t.Fatalf("count query: %v", err) - } - if count != 0 { - t.Errorf("count rows WHERE token=raw value: %d, want 0", count) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE token = ?`, tok); n != 0 { + t.Fatalf("%d rows match the raw cookie value, want 0", n) } } func TestSessions_StoredValueCannotBeUsedAsCookie(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - tok, _, err := s.Create(ctx, 1) - if err != nil { - t.Fatalf("Create: %v", err) - } - - var stored string - err = s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) + store := NewSessionStore(s) + ctx := context.Background() + tok, csrf, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("query token: %v", err) + t.Fatalf("create: %v", err) } + stored := storedSessionToken(t, s, 1) called := false - handler := s.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + h := store.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { called = true w.WriteHeader(http.StatusNoContent) })) - - // Try with stored digest as cookie - should fail - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) - rr := httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusUnauthorized { - t.Errorf("GET with stored digest cookie: %d, want 401", rr.Code) - } - if called { - t.Error("handler called with stored digest cookie") + serve := func(method, cookie, csrfValue string) int { + called = false + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(ctx, method, "/protected", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: cookie}) + if csrfValue != "" { + req.Header.Set(csrfHeader, csrfValue) + } + h.ServeHTTP(rr, req) + return rr.Code } - // Try with raw token as cookie - should succeed - req = httptest.NewRequest(http.MethodGet, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusNoContent { - t.Errorf("GET with raw token cookie: %d, want 204", rr.Code) + // The database copy of the session, presented as the cookie, is rejected + // on reads and on writes that carry the session's real CSRF token. + if code := serve(http.MethodGet, stored, ""); code != http.StatusUnauthorized || called { + t.Fatalf("GET with the stored value as cookie: code=%d called=%v, want 401 and not called", code, called) } - if !called { - t.Error("handler not called with raw token cookie") + if code := serve(http.MethodPost, stored, csrf); code != http.StatusUnauthorized || called { + t.Fatalf("POST with the stored value as cookie and the valid CSRF token: code=%d called=%v, want 401 and not called", code, called) } - - // lookup with stored digest should error - _, _, err = s.lookup(ctx, stored) - if err == nil { - t.Error("lookup(stored digest) should error") + if _, _, err := store.lookup(ctx, stored); err == nil { + t.Fatal("lookup(stored value) succeeded, want an error") } - // POST with stored digest as cookie and correct CSRF should still fail - req = httptest.NewRequest(http.MethodPost, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) - req.Header.Set(csrfHeader, "anything") // Wrong but proves CSRF isn't checked first - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusUnauthorized { - t.Errorf("POST with stored digest cookie: %d, want 401", rr.Code) + // The raw cookie value handed out by Create still authenticates. + if code := serve(http.MethodGet, tok, ""); code != http.StatusNoContent || !called { + t.Fatalf("GET with the issued cookie: code=%d called=%v, want 204 and called", code, called) + } + // The rejected attempts left the real session in place. + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE user_id = 1`); n != 1 { + t.Fatalf("%d session rows after the rejected attempts, want 1", n) } } func TestSessions_LogoutDeletesRowByDigest(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - - // Create two sessions - tokA, _, err := s.Create(ctx, 1) + store := NewSessionStore(s) + ctx := context.Background() + tokA, _, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("Create A: %v", err) + t.Fatalf("create A: %v", err) } - tokB, _, err := s.Create(ctx, 1) + tokB, _, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("Create B: %v", err) + t.Fatalf("create B: %v", err) + } + logout := func(cookie string) { + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(ctx, http.MethodPost, "/logout", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: cookie}) + store.HandleLogout().ServeHTTP(rr, req) + if rr.Code != http.StatusNoContent { + t.Fatalf("logout code=%d, want 204", rr.Code) + } } - // Logout with tokA - handler := s.HandleLogout() - req := httptest.NewRequest(http.MethodPost, "/logout", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tokA}) - rr := httptest.NewRecorder() - handler.ServeHTTP(rr, req) - - // Check tokA deleted, tokB still exists - var countA, countB int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)).Scan(&countA) - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)).Scan(&countB) - - if countA != 0 { - t.Errorf("tokA row count after logout: %d, want 0", countA) + logout(tokA) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)); n != 0 { + t.Fatalf("logged-out session: %d rows left, want 0", n) } - if countB != 1 { - t.Errorf("tokB row count after logout: %d, want 1", countB) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)); n != 1 { + t.Fatalf("other session: %d rows, want 1", n) } - - // tokB should still authenticate - _, _, err = s.lookup(ctx, tokB) - if err != nil { - t.Errorf("lookup(tokB) after logout: %v", err) + if _, _, err := store.lookup(ctx, tokB); err != nil { + t.Fatalf("other session no longer authenticates: %v", err) } - // Logout presenting the stored digest as cookie should delete nothing - var stored string - s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) - req = httptest.NewRequest(http.MethodPost, "/logout", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: stored}) - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - - var countUser int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE user_id=1`).Scan(&countUser) - if countUser != 1 { - t.Errorf("user 1 row count after logout with digest: %d, want 1", countUser) + // Logging out with the stored value as the cookie deletes nothing. + logout(storedSessionToken(t, s, 1)) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE user_id = 1`); n != 1 { + t.Fatalf("logout with the stored value removed a row: %d rows left, want 1", n) + } + if _, _, err := store.lookup(ctx, tokB); err != nil { + t.Fatalf("session B after a logout with the stored value: %v", err) } } func TestSessions_ExpiredDigestRowRemovedOnLookupAndGC(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - - tokA, _, err := s.Create(ctx, 1) - if err != nil { - t.Fatalf("Create A: %v", err) - } - tokB, _, err := s.Create(ctx, 1) + store := NewSessionStore(s) + ctx := context.Background() + tokA, _, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("Create B: %v", err) + t.Fatalf("create A: %v", err) } - - // Set both to expired - expiredTime := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) - _, err = s.db.DB.ExecContext(ctx, - `UPDATE sessions SET expires_at = ? WHERE token = ?`, - expiredTime, sessionDigest(tokA)) + tokB, _, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("update tokA expiry: %v", err) + t.Fatalf("create B: %v", err) } - _, err = s.db.DB.ExecContext(ctx, - `UPDATE sessions SET expires_at = ? WHERE token = ?`, - expiredTime, sessionDigest(tokB)) - if err != nil { - t.Fatalf("update tokB expiry: %v", err) + past := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + for _, tok := range []string{tokA, tokB} { + res, err := s.DB.ExecContext(ctx, `UPDATE sessions SET expires_at = ? WHERE token = ?`, past, sessionDigest(tok)) + if err != nil { + t.Fatalf("expire: %v", err) + } + if n, err := res.RowsAffected(); err != nil || n != 1 { + t.Fatalf("expire matched %d rows (err %v), want 1", n, err) + } } - // lookup tokA should error and remove row - _, _, err = s.lookup(ctx, tokA) - if err == nil { - t.Error("lookup(expired tokA) should error") + // lookup removes the expired row it was asked about. + if _, _, err := store.lookup(ctx, tokA); err == nil || !strings.Contains(err.Error(), "expired") { + t.Fatalf("lookup of an expired session: err=%v, want expired", err) } - var countA int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)).Scan(&countA) - if countA != 0 { - t.Errorf("tokA row count after lookup: %d, want 0", countA) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokA)); n != 0 { + t.Fatalf("expired session A: %d rows left after lookup, want 0", n) } - - // GC should remove tokB - s.gcOnce(ctx) - var countB int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)).Scan(&countB) - if countB != 0 { - t.Errorf("tokB row count after gc: %d, want 0", countB) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE token = ?`, sessionDigest(tokB)); n != 1 { + t.Fatalf("expired session B before gc: %d rows, want 1", n) } - var countAll int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions`).Scan(&countAll) - if countAll != 0 { - t.Errorf("total session count: %d, want 0", countAll) + // The interval GC removes the other one. + store.gcOnce(ctx) + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions`); n != 0 { + t.Fatalf("%d session rows after gc, want 0", n) } } func TestSessions_DeleteForUserRevokesEveryDigestRow(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - seedUser(t, s, "bob", "pw", "viewer") - - // Two sessions for alice (id=1), one for bob (id=2) - aliceA, _, _ := s.Create(ctx, 1) - aliceB, _, _ := s.Create(ctx, 1) - bob, _, _ := s.Create(ctx, 2) - - // Delete alice's sessions - err := s.DeleteForUser(ctx, 1) + seedUser(t, s, "bob", "pw", "admin") + store := NewSessionStore(s) + ctx := context.Background() + aliceA, _, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("DeleteForUser: %v", err) + t.Fatalf("create alice A: %v", err) } - - // alice's tokens should fail lookup - _, _, err = s.lookup(ctx, aliceA) - if err == nil { - t.Error("lookup(aliceA after delete) should error") - } - _, _, err = s.lookup(ctx, aliceB) - if err == nil { - t.Error("lookup(aliceB after delete) should error") + aliceB, _, err := store.Create(ctx, 1) + if err != nil { + t.Fatalf("create alice B: %v", err) } - - // bob's token should still work - _, _, err = s.lookup(ctx, bob) + bob, _, err := store.Create(ctx, 2) if err != nil { - t.Errorf("lookup(bob after alice delete): %v", err) + t.Fatalf("create bob: %v", err) } - // Check row count - var count int - s.db.DB.QueryRowContext(ctx, `SELECT COUNT(*) FROM sessions WHERE user_id=2`).Scan(&count) - if count != 1 { - t.Errorf("bob row count: %d, want 1", count) + if err := store.DeleteForUser(ctx, 1); err != nil { + t.Fatalf("DeleteForUser: %v", err) + } + for name, tok := range map[string]string{"alice A": aliceA, "alice B": aliceB} { + if _, _, err := store.lookup(ctx, tok); err == nil { + t.Errorf("%s still authenticates after DeleteForUser", name) + } + } + if _, _, err := store.lookup(ctx, bob); err != nil { + t.Fatalf("bob's session after revoking alice's: %v", err) + } + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE user_id = 1`); n != 0 { + t.Fatalf("alice: %d session rows left, want 0", n) + } + if n := countSessions(t, s, `SELECT COUNT(*) FROM sessions WHERE user_id = 2`); n != 1 { + t.Fatalf("bob: %d session rows, want 1", n) } } func TestSessions_CSRFStillEnforcedWithDigestStorage(t *testing.T) { s := newAuthDB(t) - ctx := context.Background() seedUser(t, s, "alice", "pw", "admin") - tok, csrf, err := s.Create(ctx, 1) + store := NewSessionStore(s) + ctx := context.Background() + tok, csrf, err := store.Create(ctx, 1) if err != nil { - t.Fatalf("Create: %v", err) + t.Fatalf("create: %v", err) } + stored := storedSessionToken(t, s, 1) - handler := s.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNoContent) - })) - - // POST with raw cookie and no CSRF header -> 403 - req := httptest.NewRequest(http.MethodPost, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) - rr := httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusForbidden { - t.Errorf("POST no CSRF: %d, want 403", rr.Code) + // The CSRF token is still stored as issued. + var storedCSRF string + if err := s.DB.QueryRowContext(ctx, `SELECT csrf_token FROM sessions WHERE user_id = 1`).Scan(&storedCSRF); err != nil { + t.Fatalf("read csrf_token: %v", err) } - - // POST with wrong CSRF -> 403 - req = httptest.NewRequest(http.MethodPost, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) - req.Header.Set(csrfHeader, "wrong") - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusForbidden { - t.Errorf("POST wrong CSRF: %d, want 403", rr.Code) + if storedCSRF != csrf { + t.Fatalf("csrf_token = %q, want the issued token %q", storedCSRF, csrf) } - // POST with correct CSRF -> 204 - req = httptest.NewRequest(http.MethodPost, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) - req.Header.Set(csrfHeader, csrf) - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusNoContent { - t.Errorf("POST correct CSRF: %d, want 204", rr.Code) + h := store.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + for _, tc := range []struct { + name string + header string + want int + }{ + {"no header", "", http.StatusForbidden}, + {"wrong header", "wrong", http.StatusForbidden}, + {"stored session value as header", stored, http.StatusForbidden}, + {"session cookie value as header", tok, http.StatusForbidden}, + {"issued csrf token", csrf, http.StatusNoContent}, + } { + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(ctx, http.MethodPost, "/protected", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) + if tc.header != "" { + req.Header.Set(csrfHeader, tc.header) + } + h.ServeHTTP(rr, req) + if rr.Code != tc.want { + t.Errorf("%s: code=%d, want %d", tc.name, rr.Code, tc.want) + } } +} - // POST with stored digest as CSRF header -> 403 - var stored string - s.db.DB.QueryRowContext(ctx, `SELECT token FROM sessions WHERE user_id=1`).Scan(&stored) - req = httptest.NewRequest(http.MethodPost, "/", nil) - req.AddCookie(&http.Cookie{Name: sessionCookie, Value: tok}) - req.Header.Set(csrfHeader, stored) - rr = httptest.NewRecorder() - handler.ServeHTTP(rr, req) - if rr.Code != http.StatusForbidden { - t.Errorf("POST with digest as CSRF: %d, want 403", rr.Code) +func TestSessions_LoginCookieIsRawValue(t *testing.T) { + s := newAuthDB(t) + seedUser(t, s, "alice", "hunter2", "admin") + if _, err := s.DB.ExecContext(context.Background(), + `INSERT INTO user_role_bindings(user_id, role_name, cluster, namespace) VALUES (1, 'viewer', 'local', '*')`, + ); err != nil { + t.Fatalf("seed binding: %v", err) + } + store := NewSessionStore(s) + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/login", + strings.NewReader(`{"username":"alice","password":"hunter2"}`)) + req.Header.Set("Content-Type", "application/json") + NewLocal(s).HandleLogin(store, nil).ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("login code=%d body=%s", rr.Code, rr.Body) + } + res := rr.Result() + defer func() { _ = res.Body.Close() }() + var body loginResp + if err := json.NewDecoder(res.Body).Decode(&body); err != nil { + t.Fatalf("decode login response: %v", err) + } + var cookie string + for _, c := range res.Cookies() { + if c.Name == sessionCookie { + cookie = c.Value + } + } + if cookie == "" { + t.Fatal("login set no session cookie") + } + + stored := storedSessionToken(t, s, 1) + if stored == cookie { + t.Fatal("sessions.token holds the login cookie value") + } + if stored != sessionDigest(cookie) { + t.Fatalf("sessions.token = %q, want sessionDigest(login cookie) = %q", stored, sessionDigest(cookie)) + } + u, csrf, err := store.lookup(context.Background(), cookie) + if err != nil || u.Username != "alice" || csrf != body.CSRF { + t.Fatalf("lookup(login cookie) = %+v, %q, %v; want alice with the login CSRF token", u, csrf, err) } } diff --git a/api/internal/db/migrations_test.go b/api/internal/db/migrations_test.go index 7c0be8a81..765eac393 100644 --- a/api/internal/db/migrations_test.go +++ b/api/internal/db/migrations_test.go @@ -1,7 +1,6 @@ package db import ( - "errors" "io/fs" "regexp" "strings" From 3fb472d6aad25cda0d8c5de015e7d6e77e5fae91 Mon Sep 17 00:00:00 2001 From: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:52:41 +0200 Subject: [PATCH 3/4] test(api): login digest test uses its own username and address TestLogin_PerUserRateLimit drains the package-singleton per-user bucket for alice, so the login cookie test seeds and logs in as digest-alice from its own client address. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018MnGTGHzHhFQCa22wRknUS Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> --- api/internal/auth/sessions_digest_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/api/internal/auth/sessions_digest_test.go b/api/internal/auth/sessions_digest_test.go index b256aa59d..65b27b0fa 100644 --- a/api/internal/auth/sessions_digest_test.go +++ b/api/internal/auth/sessions_digest_test.go @@ -296,7 +296,10 @@ func TestSessions_CSRFStillEnforcedWithDigestStorage(t *testing.T) { func TestSessions_LoginCookieIsRawValue(t *testing.T) { s := newAuthDB(t) - seedUser(t, s, "alice", "hunter2", "admin") + // LoginUserLimiter and LoginLimiter are package singletons keyed by + // username and client IP; TestLogin_PerUserRateLimit drains "alice", so + // this test logs in with its own username and address. + seedUser(t, s, "digest-alice", "hunter2", "admin") if _, err := s.DB.ExecContext(context.Background(), `INSERT INTO user_role_bindings(user_id, role_name, cluster, namespace) VALUES (1, 'viewer', 'local', '*')`, ); err != nil { @@ -305,8 +308,9 @@ func TestSessions_LoginCookieIsRawValue(t *testing.T) { store := NewSessionStore(s) rr := httptest.NewRecorder() req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/login", - strings.NewReader(`{"username":"alice","password":"hunter2"}`)) + strings.NewReader(`{"username":"digest-alice","password":"hunter2"}`)) req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "198.51.100.71:1234" NewLocal(s).HandleLogin(store, nil).ServeHTTP(rr, req) if rr.Code != http.StatusOK { t.Fatalf("login code=%d body=%s", rr.Code, rr.Body) @@ -335,7 +339,7 @@ func TestSessions_LoginCookieIsRawValue(t *testing.T) { t.Fatalf("sessions.token = %q, want sessionDigest(login cookie) = %q", stored, sessionDigest(cookie)) } u, csrf, err := store.lookup(context.Background(), cookie) - if err != nil || u.Username != "alice" || csrf != body.CSRF { - t.Fatalf("lookup(login cookie) = %+v, %q, %v; want alice with the login CSRF token", u, csrf, err) + if err != nil || u.Username != "digest-alice" || csrf != body.CSRF { + t.Fatalf("lookup(login cookie) = %+v, %q, %v; want digest-alice with the login CSRF token", u, csrf, err) } } From 5eba0d4dd17ea14a079eb70a3921416e7cd44c8b Mon Sep 17 00:00:00 2001 From: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> Date: Mon, 5 Oct 2026 18:02:10 +0200 Subject: [PATCH 4/4] docs(api): state the migration runner ordering precisely Pending migrations run in version order; only a file added after a later version was recorded runs out of order (CodeRabbit on #571). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018MnGTGHzHhFQCa22wRknUS Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com> --- api/specs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/specs.md b/api/specs.md index 8b04bd25f..97f03ef8e 100644 --- a/api/specs.md +++ b/api/specs.md @@ -589,7 +589,7 @@ Verify from `/api/go.mod`. - One-off pass that deletes `oidc_links`, `user_preferences`, `sessions`, `api_tokens` and `user_role_bindings` rows whose user no longer exists, and revokes (sets `revoked_at`, RFC3339 UTC) active `share_links` whose creator no longer exists. Clears rows left by user deletes made before `db.Store.DeleteUser` removed them explicitly; forward-only, so a rollback needs the pre-upgrade DB snapshot **014_sessions_digest_reset.sql:** (session digest storage) -- `DELETE FROM sessions` once. Sessions switched to storing a SHA-256 digest of the cookie value (`auth.sessionDigest`); pre-existing rows hold raw values and are deliberately invalidated, so every user signs in again once after upgrade. Numbered 014 because 013 is used by another shared migration; the runner applies unrecorded files in any order. Forward-only: a rollback to a pre-digest binary also invalidates digest-era sessions. +- `DELETE FROM sessions` once. Sessions switched to storing a SHA-256 digest of the cookie value (`auth.sessionDigest`); pre-existing rows hold raw values and are deliberately invalidated, so every user signs in again once after upgrade. Numbered 014 because 013 is used by another shared migration. The runner applies pending files in version (filename) order and skips recorded ones, so 013 and 014 pending together run 013 first; a 013 added after 014 was recorded on an earlier startup still runs on the next startup, after it. Forward-only: a rollback to a pre-digest binary also invalidates digest-era sessions. Foreign keys are enforced only on Postgres (modernc-sqlite runs with FK OFF); the API layer is authoritative and deletes dependent rows itself, so the Postgres cascades never change the outcome. The one exception is `share_links.created_by`: the Postgres schema declares it as a plain column with no foreign key, so deleting a user keeps that user's share links (revoked) in the audit trail on both drivers, instead of cascading them away.