From a99ddbdcd3ebf74da9c01aede773f974ee9fb9bc Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 10:38:06 +0000 Subject: [PATCH] Let a browser with cookies from an earlier database log in (closes #359) A new database brings a new session key. A browser still holding the old session cookie got a 500 on a correct login: Session.Get returned the cookie's decode error and the login handler answered it with a 500. Get now treats a cookie that does not decode as absent, and logging in replaces it. gorilla/csrf already did the same for the CSRF cookie. A start that creates webhooker.db now logs "created a new, empty database" at WARN with its path, shortly before the first-boot banner, so an unexpectedly empty DATA_DIR is noticed. The codec tests now decode through the store, since Get no longer reports the codec's reason. Model: opus-5-5 --- README.md | 6 ++ internal/database/bootstrap_banner_test.go | 36 +++++++++++ internal/database/database.go | 14 ++++- internal/server/routes_test.go | 72 +++++++++++++++++++++- internal/session/codec_test.go | 15 ++--- internal/session/session.go | 14 ++++- 6 files changed, 145 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 0536a4e..25be1bf 100644 --- a/README.md +++ b/README.md @@ -538,6 +538,12 @@ its Argon2id hash. There is no second account and no forgot-password flow, so the banner and the reset command below are the only two ways in. +A start that finds no `webhooker.db` in `DATA_DIR` also logs +`created a new, empty database` at `WARN`, with the file's path, +shortly before the banner. On a deployment that has run before, that +line means `DATA_DIR` was empty, most often because its volume is not +mounted. + #### Recovering a lost admin password `webhooker resetpw` sets an existing account's password from the diff --git a/internal/database/bootstrap_banner_test.go b/internal/database/bootstrap_banner_test.go index f56b467..7f7860d 100644 --- a/internal/database/bootstrap_banner_test.go +++ b/internal/database/bootstrap_banner_test.go @@ -3,6 +3,8 @@ package database_test import ( "bytes" "context" + "log/slog" + "path/filepath" "strings" "testing" @@ -83,3 +85,37 @@ func TestFirstBoot_PrintsTheAdminPasswordAsABanner(t *testing.T) { t, ok, "the printed password must open the seeded account", ) } + +// TestNewDatabase_IsLoggedWithItsPath is the log half of +// https://git.eeqj.de/sneak/webhooker/issues/359. A DATA_DIR that is +// unexpectedly empty boots exactly like a first start, so the start +// that creates the database must say so, and where. Opening that +// database again must not. +func TestNewDatabase_IsLoggedWithItsPath(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + open := func() string { + var out bytes.Buffer + + db, err := database.Open(dir, slog.New(slog.NewTextHandler(&out, nil))) + require.NoError(t, err) + require.NoError(t, db.Close()) + + return out.String() + } + + const created = `level=WARN msg="created a new, empty database"` + + first := open() + second := open() + + assert.Contains( + t, first, + created+" path="+filepath.Join(dir, database.MainDBFileName), + ) + assert.NotContains( + t, second, created, "an existing database is not new", + ) +} diff --git a/internal/database/database.go b/internal/database/database.go index ba28bae..da8bd37 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "io/fs" "log/slog" "os" "path/filepath" @@ -199,6 +200,12 @@ func (d *Database) connectTo(dataDir string) error { // Construct the main application database path inside DATA_DIR. dbPath := filepath.Join(dataDir, MainDBFileName) + // Checked before opening, which creates the file. A DATA_DIR that + // is unexpectedly empty -- its volume not mounted, say -- looks + // exactly like a first start, so a new database is a warning. + _, statErr := os.Stat(dbPath) + created := errors.Is(statErr, fs.ErrNotExist) + // Opened through OpenSQLite so this handle carries the same WAL // journaling, busy timeout, immediate-transaction locking, and pool // bounds as every other database file. See sqlite_open.go. @@ -229,7 +236,12 @@ func (d *Database) connectTo(dataDir string) error { } d.db = db - d.log.Info("connected to database", "path", dbPath) + + if created { + d.log.Warn("created a new, empty database", "path", dbPath) + } else { + d.log.Info("connected to database", "path", dbPath) + } // Run migrations return d.migrate() diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 429a7f7..a06272f 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/url" "regexp" + "slices" "strconv" "strings" "testing" @@ -220,9 +221,21 @@ func (e *testEnv) csrfFrom( // out of the markup has to be unescaped before it is submitted. token := html.UnescapeString(match[1]) - combined := make([]*http.Cookie, 0, len(cookies)) - combined = append(combined, cookies...) - combined = append(combined, w.Result().Cookies()...) + // A cookie the page sets replaces the one of the same name, as in + // a browser. Sent both, the server would read the first, older one. + set := w.Result().Cookies() + combined := make([]*http.Cookie, 0, len(cookies)+len(set)) + + for _, c := range cookies { + replaced := slices.ContainsFunc(set, func(n *http.Cookie) bool { + return n.Name == c.Name + }) + if !replaced { + combined = append(combined, c) + } + } + + combined = append(combined, set...) return token, combined } @@ -614,6 +627,59 @@ func TestPagesLogin_CorrectPasswordSurvivesASpentBudget( ) } +// TestPagesLogin_CookiesFromAnEarlierDatabase is +// https://git.eeqj.de/sneak/webhooker/issues/359. A new database +// brings a new session key, and the operator's browser still holds +// the session and CSRF cookies signed with the old one. Logging in +// must work as from a fresh browser and leave cookies the new key +// accepts. +func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) { + t.Parallel() + + const ( + username = "operator" + password = "correct-horse-battery-staple" + ) + + earlier := newTestEnv(t) + earlierID, _ := earlier.seedUser(t, username, password) + _, stale := earlier.csrfFrom(t, "/pages/login", nil) + stale = append(stale, earlier.authCookies(t, earlierID, username)...) + + env := newTestEnv(t) + env.seedUser(t, username, password) + + token, cookies := env.csrfFrom(t, "/pages/login", stale) + + form := url.Values{} + form.Set("csrf_token", token) + form.Set("username", username) + form.Set("password", password) + + w := env.post("/pages/login", form, cookies) + require.Equal( + t, http.StatusSeeOther, w.Code, + "a session cookie from another key must not fail the login", + ) + + // The response deletes the old session cookie and then sets the + // new one; a browser keeps the last. + var fresh *http.Cookie + + for _, c := range w.Result().Cookies() { + if c.Name == session.SessionName { + fresh = c + } + } + + require.NotNil(t, fresh, "login must set a session cookie") + assert.Equal( + t, "/sources", + env.get("/", []*http.Cookie{fresh}).Header().Get("Location"), + "the new session cookie must authenticate", + ) +} + // --- /user/{username} group --- // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged diff --git a/internal/session/codec_test.go b/internal/session/codec_test.go index f181b25..08e5a0a 100644 --- a/internal/session/codec_test.go +++ b/internal/session/codec_test.go @@ -19,8 +19,8 @@ import ( ) // The tests below exercise the securecookie codecs underneath the -// store and nothing else: Session.Get only decodes, so no server-side -// expiry check takes part in the result. They exist because +// store and nothing else: they decode through the store itself, so no +// server-side expiry check takes part in the result. They exist because // NewCookieStore gives its codecs a 30-day max age that assigning // store.Options does not override, which would let the codec accept a // cookie weeks past the cap the cookie attribute advertises. @@ -75,10 +75,11 @@ func restamp( return base64.URLEncoding.EncodeToString(payload) } -// decodeCookie feeds value back through the store's decode path. +// decodeCookie feeds value back through the store's decode path. It +// asks the store rather than Session.Get, which treats a cookie that +// does not decode as absent and so hides the codec's reason. func decodeCookie( t *testing.T, - s *session.Session, value string, ) (*sessions.Session, error) { t.Helper() @@ -94,7 +95,7 @@ func decodeCookie( SameSite: http.SameSiteLaxMode, }) - sess, err := s.Get(req) + sess, err := session.NewStore(testKey()).Get(req, session.SessionName) require.NotNil(t, sess) return sess, err @@ -105,7 +106,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) { s := testSession(t) - sess, err := decodeCookie(t, s, restamp( + sess, err := decodeCookie(t, restamp( t, issuedCookie(t, s), time.Now().Add(-(testAbsoluteMaxAge-time.Hour)), @@ -126,7 +127,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) { s := testSession(t) - sess, err := decodeCookie(t, s, restamp( + sess, err := decodeCookie(t, restamp( t, issuedCookie(t, s), time.Now().Add(-(testAbsoluteMaxAge+time.Hour)), diff --git a/internal/session/session.go b/internal/session/session.go index 97b6664..8e97734 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -224,10 +224,22 @@ func New( } // Get retrieves a session for the request. +// +// A session cookie that does not decode -- one signed with an earlier +// session key, say, because the database was made anew -- is treated +// as absent: the caller gets a new, empty session and no error, and +// the next save replaces the cookie. func (s *Session) Get( r *http.Request, ) (*sessions.Session, error) { - return s.store.Get(r, SessionName) + sess, err := s.store.Get(r, SessionName) + if sess == nil { + return nil, err + } + + // For a cookie that does not decode, gorilla/sessions returns a + // new, empty session alongside the error that is dropped here. + return sess, nil } // GetKey returns the raw 32-byte authentication key used for