From 618b07ca0fa6098eb096bfa380f73a46b2fea0c7 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 12 Aug 2026 09:46:00 +0000 Subject: [PATCH] Align session codec max-age with the 7-day cap (closes #108) sessions.NewCookieStore gives its securecookie codecs a 30-day max age, and assigning store.Options never touches Codecs. The store therefore decoded a cookie up to 30 days old while the cookie attribute and the server-side expiry check both said 7 days, so a refactor of that check -- or a second decode path that skips IsAuthenticated -- would silently reopen a 30-day window. Build the store through store.MaxAge, which sets Options.MaxAge and propagates to every codec. The store is now built by newStore, the one place a store is constructed; Regenerate shares its cookie attributes via cookieOptions. Tests: - Two codec tests decode a re-stamped cookie an hour inside and an hour outside the cap. They only exercise Session.Get, so they pin the codec: reverting the fix fails the rejection test whether or not the server-side expiry check is present. - The lazy-refresh bound is pinned two-sidedly, so the documented "expires up to 10% early, never late" guarantee now fails the suite if idleRefreshDivisor drifts in either direction. Also scopes the RequireAuth save error to a distinct saveErr variable instead of reusing the outer err. It is a plain assignment rather than an inline "if saveErr := ...; saveErr != nil", because the repo's noinlineerr linter rejects that form. Behaviour is unchanged; this is scoping hygiene, not a bug fix. Two README claims are corrected as well: the idle timeout is disabled by any non-positive value, not only 0, and deploying the two-clock expiry logs existing sessions out once because they carry no timestamps. https://git.eeqj.de/sneak/webhooker/issues/108 --- README.md | 12 ++- internal/middleware/middleware.go | 6 +- internal/session/codec_test.go | 150 ++++++++++++++++++++++++++++++ internal/session/export_test.go | 10 ++ internal/session/session.go | 51 ++++++---- internal/session/session_test.go | 57 +++++++++--- 6 files changed, 247 insertions(+), 39 deletions(-) create mode 100644 internal/session/codec_test.go create mode 100644 internal/session/export_test.go diff --git a/README.md b/README.md index d089ea3..7000740 100644 --- a/README.md +++ b/README.md @@ -150,9 +150,10 @@ one runs out first: - **Idle expiry** (`SESSION_IDLE_TIMEOUT`, default `24h`) is a sliding window. Every authenticated request pushes it forward, so a session in continuous use never hits it, while an abandoned one expires a day - after its last use. Set it to `0` to disable idle expiry entirely; - the absolute cap below still applies. A set-but-unparseable value - aborts startup rather than silently falling back to the default. + after its last use. Any non-positive value (`0`, or a negative + duration such as `-1s`) disables idle expiry entirely; the absolute + cap below still applies. A set-but-unparseable value aborts startup + rather than silently falling back to the default. - **Absolute expiry** is a fixed 7 days from login. Activity does **not** extend it: after a week, every session ends and the user authenticates again. @@ -164,6 +165,11 @@ idle window rather than on every request, which means a session may expire up to 10% early relative to the user's true last request, but never late. +Both clocks are anchored by timestamps stored in the session cookie. +Sessions issued before this feature existed carry neither, so they are +treated as expired: upgrading to a build that has it logs every +existing session out once, and those users sign in again. + #### Invalid values abort startup The defaults above apply **only** to variables that are unset (or set diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index add73df..975ecc0 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -214,11 +214,11 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler { // handler runs, while the headers are still ours to // write. if s.session.Touch(sess) { - err = s.session.Save(r, w, sess) - if err != nil { + saveErr := s.session.Save(r, w, sess) + if saveErr != nil { s.log.Error( "auth middleware: failed to refresh session", - "error", err, + "error", saveErr, ) } } diff --git a/internal/session/codec_test.go b/internal/session/codec_test.go new file mode 100644 index 0000000..f181b25 --- /dev/null +++ b/internal/session/codec_test.go @@ -0,0 +1,150 @@ +package session_test + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gorilla/sessions" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/session" +) + +// 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 +// 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. + +// issuedCookie returns a session cookie the store itself wrote. +func issuedCookie(t *testing.T, s *session.Session) string { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil) + w := httptest.NewRecorder() + + sess, err := s.Get(req) + require.NoError(t, err) + + sess.Values["probe"] = "value" + require.NoError(t, s.Save(req, w, sess)) + + cookies := w.Result().Cookies() + require.Len(t, cookies, 1) + + return cookies[0].Value +} + +// restamp rewrites the timestamp inside an encoded session cookie and +// re-signs it, yielding the cookie the store would have written at +// that instant. securecookie stamps the encoding time itself and +// exposes no seam to move it, so its wire format is reproduced here: +// the base64url payload is "date|value|mac", where mac is HMAC-SHA256 +// of "name|date|value" under the store's key. +func restamp( + t *testing.T, + encoded string, + at time.Time, +) string { + t.Helper() + + raw, err := base64.URLEncoding.DecodeString(encoded) + require.NoError(t, err) + + parts := strings.SplitN(string(raw), "|", 3) + require.Len(t, parts, 3) + + stamped := fmt.Sprintf("%d|%s", at.Unix(), parts[1]) + + mac := hmac.New(sha256.New, testKey()) + _, err = mac.Write([]byte(session.SessionName + "|" + stamped)) + require.NoError(t, err) + + payload := append([]byte(stamped+"|"), mac.Sum(nil)...) + + return base64.URLEncoding.EncodeToString(payload) +} + +// decodeCookie feeds value back through the store's decode path. +func decodeCookie( + t *testing.T, + s *session.Session, + value string, +) (*sessions.Session, error) { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{ + Name: session.SessionName, + Value: value, + Path: "/", + HttpOnly: true, + Secure: true, + SameSite: http.SameSiteLaxMode, + }) + + sess, err := s.Get(req) + require.NotNil(t, sess) + + return sess, err +} + +func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) { + t.Parallel() + + s := testSession(t) + + sess, err := decodeCookie(t, s, restamp( + t, + issuedCookie(t, s), + time.Now().Add(-(testAbsoluteMaxAge-time.Hour)), + )) + require.NoError(t, err) + assert.False( + t, sess.IsNew, + "a cookie inside the cap must still decode", + ) + assert.Equal( + t, "value", sess.Values["probe"], + "decoding must yield the values that were saved", + ) +} + +func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) { + t.Parallel() + + s := testSession(t) + + sess, err := decodeCookie(t, s, restamp( + t, + issuedCookie(t, s), + time.Now().Add(-(testAbsoluteMaxAge+time.Hour)), + )) + require.Error( + t, err, + "the codec must refuse a cookie older than the cap", + ) + assert.Contains( + t, err.Error(), "expired timestamp", + "rejection must come from the codec's age check", + ) + assert.True( + t, sess.IsNew, + "a cookie past the cap must not populate a session", + ) + assert.Nil( + t, sess.Values["probe"], + "a cookie past the cap must not yield its values", + ) +} diff --git a/internal/session/export_test.go b/internal/session/export_test.go new file mode 100644 index 0000000..626a6fd --- /dev/null +++ b/internal/session/export_test.go @@ -0,0 +1,10 @@ +package session + +import "github.com/gorilla/sessions" + +// NewStore exposes the production cookie-store constructor so tests +// exercise the store the application actually runs with, rather than a +// lookalike assembled in the test. +func NewStore(key []byte, secure bool) *sessions.CookieStore { + return newStore(key, secure) +} diff --git a/internal/session/session.go b/internal/session/session.go index 1a2d2e1..4269f10 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -100,6 +100,35 @@ type Session struct { now func() time.Time } +// cookieOptions returns the cookie attributes used for every session +// cookie. MaxAge is deliberately left at its zero value: for a store +// it is set through CookieStore.MaxAge (see newStore), and for a +// single session it is copied from the store's options. +func cookieOptions(secure bool) *sessions.Options { + return &sessions.Options{ + Path: "/", + HttpOnly: true, + Secure: secure, + SameSite: http.SameSiteLaxMode, + } +} + +// newStore builds the session cookie store. +// +// The absolute cap MUST be applied with store.MaxAge and not by +// assigning store.Options.MaxAge. NewCookieStore gives the underlying +// securecookie codecs a 30-day max age of their own, and assigning +// Options never touches Codecs -- so a store configured that way still +// decodes a 30-day-old cookie, leaving the cookie attribute and the +// codec disagreeing about the same policy. store.MaxAge sets both. +func newStore(key []byte, secure bool) *sessions.CookieStore { + store := sessions.NewCookieStore(key) + store.Options = cookieOptions(secure) + store.MaxAge(secondsPerDay * sessionMaxAgeDays) + + return store +} + // New creates a new session manager. The cookie store is // initialized during the fx OnStart phase after the database is // connected, using a session key that is auto-generated and stored @@ -142,19 +171,8 @@ func New( ) } - store := sessions.NewCookieStore(keyBytes) - - // Configure cookie options for security - store.Options = &sessions.Options{ - Path: "/", - MaxAge: secondsPerDay * sessionMaxAgeDays, - HttpOnly: true, - Secure: !params.Config.IsDev(), - SameSite: http.SameSiteLaxMode, - } - s.key = keyBytes - s.store = store + s.store = newStore(keyBytes, !params.Config.IsDev()) s.log.Info("session manager initialized") return nil @@ -350,13 +368,8 @@ func (s *Session) Regenerate( // Apply the standard session options (the destroyed old // session had MaxAge = -1, which store.New might inherit // from the cookie). - newSess.Options = &sessions.Options{ - Path: "/", - MaxAge: secondsPerDay * sessionMaxAgeDays, - HttpOnly: true, - Secure: !s.config.IsDev(), - SameSite: http.SameSiteLaxMode, - } + newSess.Options = cookieOptions(!s.config.IsDev()) + newSess.Options.MaxAge = secondsPerDay * sessionMaxAgeDays return newSess, nil } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index f269149..31bb472 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -39,6 +39,19 @@ func (c *fakeClock) Advance(d time.Duration) { c.t = c.t.Add(d) } +// testKey returns the fixed session key the tests sign with. The +// codec tests re-sign cookies with it, so it must be the same key the +// store was built from. +func testKey() []byte { + key := make([]byte, testKeySize) + + for i := range key { + key[i] = byte(i + 42) + } + + return key +} + // testSession creates a Session with a real cookie store and the // real clock. func testSession(t *testing.T) *session.Session { @@ -59,20 +72,8 @@ func testSessionWithClock( ) (*session.Session, *fakeClock) { t.Helper() - key := make([]byte, testKeySize) - - for i := range key { - key[i] = byte(i + 42) - } - - store := sessions.NewCookieStore(key) - store.Options = &sessions.Options{ - Path: "/", - MaxAge: 86400 * 7, - HttpOnly: true, - Secure: false, - SameSite: http.SameSiteLaxMode, - } + key := testKey() + store := session.NewStore(key, false) cfg := &config.Config{ Environment: config.EnvironmentDev, @@ -645,6 +646,34 @@ func TestTouch_LazyBelowRefreshThreshold(t *testing.T) { ) } +func TestTouch_RefreshThresholdIsOneTenthOfIdleWindow(t *testing.T) { + t.Parallel() + + // testRefreshDivisor restates the documented bound independently + // of the implementation constant: the idle timestamp is rewritten + // once it is a tenth of the idle window old, which is what makes + // "expires up to 10% early, never late" true. Both assertions are + // needed to pin it -- a larger divisor fails the first, a smaller + // one fails the second. + const testRefreshDivisor = 10 + + threshold := testIdleTimeout / testRefreshDivisor + + s, sess, clock := authenticatedSession(t, testIdleTimeout) + + clock.Advance(threshold - time.Second) + assert.False( + t, s.Touch(sess), + "Touch must not rewrite the session below a tenth of the window", + ) + + clock.Advance(time.Second) + assert.True( + t, s.Touch(sess), + "Touch must rewrite the session at a tenth of the window", + ) +} + func TestTouch_UnauthenticatedSessionIsNotRefreshed(t *testing.T) { t.Parallel()