1 Commits

Author SHA1 Message Date
618b07ca0f Align session codec max-age with the 7-day cap (closes #108)
All checks were successful
check / check (push) Successful in 3m5s
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.

#108
2026-08-12 10:00:50 +00:00
17 changed files with 298 additions and 431 deletions

View File

@@ -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

View File

@@ -46,19 +46,8 @@ func (r *RetentionReaper) ExportStart() {
}
// ExportStop stops the reaper's background loop for tests.
func (r *RetentionReaper) ExportStop(ctx context.Context) error {
return r.stop(ctx)
}
// ExportWedgeLoop adds a goroutine to the reaper's WaitGroup that
// never observes cancellation and returns only when release is
// closed. It stands in for a sweep stuck on a locked database.
func (r *RetentionReaper) ExportWedgeLoop(
release <-chan struct{},
) {
r.wg.Go(func() {
<-release
})
func (r *RetentionReaper) ExportStop() {
r.stop()
}
// ExportSetInterval overrides the sweep interval for tests.

View File

@@ -10,7 +10,6 @@ import (
"go.uber.org/fx"
"gorm.io/gorm"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/lifecycle"
"sneak.berlin/go/webhooker/internal/logger"
)
@@ -63,9 +62,8 @@ func NewRetentionReaper(
}
// registerHooks wires the reaper's start and stop into the fx
// lifecycle. The start hook's context is deliberately ignored (see
// start for why the sweep loop must not inherit it); the stop hook's
// context is honoured (see stop).
// lifecycle. The start hook's context is deliberately ignored: see
// start for why the sweep loop must not inherit it.
func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
lc.Append(fx.Hook{
//nolint:contextcheck // Not inheriting the hook context is
@@ -75,8 +73,10 @@ func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
return nil
},
OnStop: func(ctx context.Context) error {
return r.stop(ctx)
OnStop: func(_ context.Context) error {
r.stop()
return nil
},
})
}
@@ -105,27 +105,15 @@ func (r *RetentionReaper) start() {
)
}
// stop cancels the sweep loop's context and waits for it to
// exit, bounded by the stop hook's context: a sweep wedged on a
// locked database must not hang the process past fx's stop
// timeout.
func (r *RetentionReaper) stop(ctx context.Context) error {
func (r *RetentionReaper) stop() {
r.log.Info("retention reaper stopping")
if r.cancel != nil {
r.cancel()
}
err := lifecycle.WaitForShutdown(
ctx, r.log, "retention reaper", &r.wg,
)
if err != nil {
return err
}
r.wg.Wait()
r.log.Info("retention reaper stopped")
return nil
}
func (r *RetentionReaper) run(ctx context.Context) {

View File

@@ -26,13 +26,6 @@ const (
// reaperTestRetentionDays is the retention policy the lifecycle
// tests give their webhook.
reaperTestRetentionDays = 30
// reaperWedgeStopTimeout is the stop timeout the wedged-shutdown
// test hands OnStop, standing in for fx's StopTimeout. The test
// asserts only that the hook returns at all, and allows it
// reaperStopTimeout — forty times this budget — to do so, so no
// assertion races the wall clock.
reaperWedgeStopTimeout = 250 * time.Millisecond
)
// recordingLifecycle is a minimal fx.Lifecycle that records the
@@ -214,59 +207,3 @@ func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
"a stopped reaper must not sweep anything",
)
}
// TestRetentionReaper_StopHookHonoursStopTimeout is the
// regression test for a shutdown that could never complete. fx
// hands OnStop a context carrying the application's stop timeout;
// an OnStop that discards it and calls wg.Wait() bare hangs the
// process forever on a sweep blocked on a locked SQLite database
// — precisely when a bounded shutdown matters most.
//
// The wedged goroutine here never observes cancellation, so the
// hook can only return by honouring its context, and it must say
// so rather than reporting a clean stop.
func TestRetentionReaper_StopHookHonoursStopTimeout(
t *testing.T,
) {
t.Parallel()
env := setupRetentionTest(t)
env.reaper.ExportSetInterval(reaperTestInterval)
lc := startReaperViaHook(t, env.reaper)
release := make(chan struct{})
t.Cleanup(func() { close(release) })
env.reaper.ExportWedgeLoop(release)
stopCtx, cancel := context.WithTimeout(
context.Background(), reaperWedgeStopTimeout,
)
defer cancel()
var stopErr error
stopped := make(chan struct{})
go func() {
defer close(stopped)
stopErr = lc.hooks[0].OnStop(stopCtx)
}()
select {
case <-stopped:
case <-time.After(reaperStopTimeout):
t.Fatal(
"OnStop did not return: it discarded the stop " +
"context and is waiting on a wedged goroutine " +
"that will never observe cancellation",
)
}
require.ErrorIs(t, stopErr, context.DeadlineExceeded)
require.ErrorContains(t, stopErr, "retention reaper")
}

View File

@@ -10,7 +10,6 @@ import (
"go.uber.org/fx"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/lifecycle"
"sneak.berlin/go/webhooker/internal/logger"
)
@@ -68,9 +67,10 @@ func NewArchiveSweeper(
}
// registerHooks wires the sweeper's start and stop into the fx
// lifecycle. The start hook's context is deliberately ignored
// (see start for why the background loop must not inherit it);
// the stop hook's context is honoured (see stop).
// lifecycle. Both hook contexts are deliberately ignored: see
// start for why the background loop must not inherit the start
// hook's context, and stop for why shutdown blocks on the loop
// rather than on the stop hook's deadline.
func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
lc.Append(fx.Hook{
//nolint:contextcheck // Not passing the hook context is
@@ -80,8 +80,10 @@ func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
return nil
},
OnStop: func(ctx context.Context) error {
return s.stop(ctx)
OnStop: func(_ context.Context) error {
s.stop()
return nil
},
})
}
@@ -111,27 +113,15 @@ func (s *ArchiveSweeper) start() {
)
}
// stop cancels the sweep loop's context and waits for it to
// exit, bounded by the stop hook's context: a prune wedged on a
// locked archive must not hang the process past fx's stop
// timeout.
func (s *ArchiveSweeper) stop(ctx context.Context) error {
func (s *ArchiveSweeper) stop() {
s.log.Info("archive sweeper stopping")
if s.cancel != nil {
s.cancel()
}
err := lifecycle.WaitForShutdown(
ctx, s.log, "archive sweeper", &s.wg,
)
if err != nil {
return err
}
s.wg.Wait()
s.log.Info("archive sweeper stopped")
return nil
}
func (s *ArchiveSweeper) run(ctx context.Context) {

View File

@@ -14,6 +14,7 @@ import (
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"gorm.io/driver/sqlite"
"gorm.io/gorm"
"gorm.io/gorm/clause"
@@ -225,6 +226,17 @@ func countArchivedRows(path string) (int64, error) {
return count, nil
}
// captureLifecycle is a minimal fx.Lifecycle that records the
// hooks a component registers, so a test can invoke the real
// OnStart/OnStop functions with a context of its choosing.
type captureLifecycle struct {
hooks []fx.Hook
}
func (l *captureLifecycle) Append(h fx.Hook) {
l.hooks = append(l.hooks, h)
}
// TestArchiveSweeper_LoopOutlivesStartHookContext is the
// regression test for a sweeper that never swept. fx calls
// OnStart with a context carrying the application's start
@@ -258,7 +270,7 @@ func TestArchiveSweeper_LoopOutlivesStartHookContext(
// Drive the genuine fx hooks the application registers,
// rather than a test-only entry point.
lc := &recordingLifecycle{}
lc := &captureLifecycle{}
env.sweeper.ExportRegisterHooks(lc)
require.Len(t, lc.hooks, 1)
@@ -912,36 +924,7 @@ func TestArchiveSweeper_StopsCleanly(t *testing.T) {
env.sweeper.ExportSetInterval(time.Millisecond)
env.sweeper.ExportStart()
// stop blocks on the loop's WaitGroup, so returning without
// error proves the loop observed the cancellation and exited
// well inside the stop context.
require.NoError(
t, env.sweeper.ExportStop(context.Background()),
)
}
// TestArchiveSweeper_StopHookHonoursStopTimeout is the sweeper's
// half of the same shutdown defect the engine and the retention
// reaper carried: an OnStop that discards its context and waits
// on the WaitGroup bare hangs the process forever on a prune
// wedged inside a locked archive.
func TestArchiveSweeper_StopHookHonoursStopTimeout(
t *testing.T,
) {
t.Parallel()
env := setupSweeperTest(t)
lc := &recordingLifecycle{}
env.sweeper.ExportRegisterHooks(lc)
require.Len(t, lc.hooks, 1)
require.NoError(t, lc.hooks[0].OnStart(context.Background()))
release := make(chan struct{})
t.Cleanup(func() { close(release) })
env.sweeper.ExportWedgeLoop(release)
requireStopHookExpires(t, lc.hooks[0], "archive sweeper")
// stop blocks on the loop's WaitGroup, so returning at all
// proves the loop observed the cancellation and exited.
env.sweeper.ExportStop()
}

View File

@@ -13,7 +13,6 @@ import (
"go.uber.org/fx"
"gorm.io/gorm"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/lifecycle"
"sneak.berlin/go/webhooker/internal/logger"
)
@@ -235,9 +234,8 @@ func (e *Engine) ScheduleRetry(
}
// registerHooks wires the engine's start and stop into the fx
// lifecycle. The start hook's context is deliberately ignored
// (see start for why the worker pool must not inherit it); the
// stop hook's context is honoured (see stop).
// lifecycle. The start hook's context is deliberately ignored:
// see start for why the worker pool must not inherit it.
func (e *Engine) registerHooks(lc fx.Lifecycle) {
lc.Append(fx.Hook{
//nolint:contextcheck // Not inheriting the hook context
@@ -247,8 +245,10 @@ func (e *Engine) registerHooks(lc fx.Lifecycle) {
return nil
},
OnStop: func(ctx context.Context) error {
return e.stop(ctx)
OnStop: func(_ context.Context) error {
e.stop()
return nil
},
})
}
@@ -289,26 +289,11 @@ func (e *Engine) start() {
)
}
// stop cancels the worker pool's context and waits for the pool
// to drain, bounded by the stop hook's context: a wedged worker
// must not hang the process past fx's stop timeout.
func (e *Engine) stop(ctx context.Context) error {
func (e *Engine) stop() {
e.log.Info("delivery engine stopping")
if e.cancel != nil {
e.cancel()
}
err := lifecycle.WaitForShutdown(
ctx, e.log, "delivery engine", &e.wg,
)
if err != nil {
return err
}
e.cancel()
e.wg.Wait()
e.log.Info("delivery engine stopped")
return nil
}
func (e *Engine) worker(ctx context.Context) {

View File

@@ -501,7 +501,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
iWaitForDelivered(t, s.WebhookDB, d.ID)
require.NoError(t, s.Engine.ExportStop(context.Background()))
s.Engine.ExportStop()
}
// iWaitForDelivered polls until the delivery reaches the
@@ -567,7 +567,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
iWaitForDelivered(t, s.WebhookDB, d.ID)
require.NoError(t, s.Engine.ExportStop(context.Background()))
s.Engine.ExportStop()
}
// --- processDelivery: unknown target type ---

View File

@@ -27,13 +27,6 @@ const (
// and a ready deliveryCh are chosen between at random and a
// doomed pool still delivers.
hookSettleDelay = 250 * time.Millisecond
// wedgeStopTimeout is the stop timeout a wedged-shutdown test
// hands OnStop, standing in for fx's StopTimeout. The test
// asserts only that the hook returns at all, and allows it
// hookStopTimeout — forty times this budget — to do so, so no
// assertion here races the wall clock.
wedgeStopTimeout = 250 * time.Millisecond
)
// recordingLifecycle is a minimal fx.Lifecycle that records the
@@ -47,44 +40,6 @@ func (l *recordingLifecycle) Append(h fx.Hook) {
l.hooks = append(l.hooks, h)
}
// requireStopHookExpires drives hook.OnStop with a stop context
// that expires while a wedged goroutine is still running, and
// requires the hook to return the deadline error naming
// component instead of blocking on the WaitGroup forever.
func requireStopHookExpires(
t *testing.T, hook fx.Hook, component string,
) {
t.Helper()
stopCtx, cancel := context.WithTimeout(
context.Background(), wedgeStopTimeout,
)
defer cancel()
var stopErr error
stopped := make(chan struct{})
go func() {
defer close(stopped)
stopErr = hook.OnStop(stopCtx)
}()
select {
case <-stopped:
case <-time.After(hookStopTimeout):
t.Fatal(
"OnStop did not return: it discarded the stop " +
"context and is waiting on a wedged goroutine " +
"that will never observe cancellation",
)
}
require.ErrorIs(t, stopErr, context.DeadlineExceeded)
require.ErrorContains(t, stopErr, component)
}
// startEngineViaHook drives the genuine fx hooks the application
// registers for the engine, handing OnStart a context that is
// already done, and returns only once a pool that inherited that
@@ -242,30 +197,3 @@ func TestEngine_StopHookStopsWorkers(t *testing.T) {
"a stopped engine must not deliver anything",
)
}
// TestEngine_StopHookHonoursStopTimeout is the regression test
// for a shutdown that could never complete. fx hands OnStop a
// context carrying the application's stop timeout; an OnStop
// that discards it and calls wg.Wait() bare hangs the process
// forever on a single worker stuck inside a delivery target that
// never returns — precisely when a bounded shutdown matters
// most.
//
// The wedged goroutine here never observes cancellation, so the
// hook can only return by honouring its context, and it must say
// so rather than reporting a clean stop.
func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
t.Parallel()
s := newISetup(t)
lc := startEngineViaHook(t, s.Engine)
release := make(chan struct{})
t.Cleanup(func() { close(release) })
s.Engine.ExportWedgeWorker(release)
requireStopHookExpires(t, lc.hooks[0], "delivery engine")
}

View File

@@ -216,19 +216,8 @@ func (e *Engine) ExportRegisterHooks(lc fx.Lifecycle) {
}
// ExportStop exposes stop for testing.
func (e *Engine) ExportStop(ctx context.Context) error {
return e.stop(ctx)
}
// ExportWedgeWorker adds a goroutine to the engine's WaitGroup
// that never observes cancellation and returns only when release
// is closed. It stands in for a worker stuck inside a delivery
// target that never returns, which is the only way stop can be
// made to outlast its context.
func (e *Engine) ExportWedgeWorker(release <-chan struct{}) {
e.wg.Go(func() {
<-release
})
func (e *Engine) ExportStop() {
e.stop()
}
// ExportDeliveryCh returns the delivery channel.
@@ -529,19 +518,8 @@ func (s *ArchiveSweeper) ExportRegisterHooks(lc fx.Lifecycle) {
}
// ExportStop stops the sweeper's background loop for tests.
func (s *ArchiveSweeper) ExportStop(ctx context.Context) error {
return s.stop(ctx)
}
// ExportWedgeLoop adds a goroutine to the sweeper's WaitGroup
// that never observes cancellation and returns only when release
// is closed. It stands in for a prune stuck on a locked archive.
func (s *ArchiveSweeper) ExportWedgeLoop(
release <-chan struct{},
) {
s.wg.Go(func() {
<-release
})
func (s *ArchiveSweeper) ExportStop() {
s.stop()
}
// ExportSetInterval overrides the sweep interval for tests.

View File

@@ -1,57 +0,0 @@
// Package lifecycle holds helpers shared by the components that
// register fx start and stop hooks.
package lifecycle
import (
"context"
"fmt"
"log/slog"
"sync"
)
// WaitForShutdown waits for wg to drain, bounded by ctx.
//
// fx hands OnStop a context carrying the application's stop
// timeout. A bare wg.Wait() discards that deadline, so a single
// goroutine that never observes cancellation — a delivery target
// that never returns, a SQLite operation blocked on a lock —
// hangs the process forever instead of letting it exit when the
// timeout expires, which is exactly when a clean shutdown matters
// most.
//
// On timeout it logs at error naming component and returns an
// error: the goroutines are still running, and reporting success
// would hide an unclean shutdown from the operator. The waiting
// goroutine outlives this call and exits when (if) wg drains; it
// holds nothing but the channel it closes.
func WaitForShutdown(
ctx context.Context,
log *slog.Logger,
component string,
wg *sync.WaitGroup,
) error {
done := make(chan struct{})
go func() {
defer close(done)
wg.Wait()
}()
select {
case <-done:
return nil
case <-ctx.Done():
log.Error(
"shutdown timed out, goroutines still running",
"component", component,
"error", ctx.Err(),
)
return fmt.Errorf(
"%s: shutdown timed out, "+
"goroutines still running: %w",
component, ctx.Err(),
)
}
}

View File

@@ -1,62 +0,0 @@
package lifecycle_test
import (
"context"
"log/slog"
"sync"
"testing"
"time"
"github.com/stretchr/testify/require"
"sneak.berlin/go/webhooker/internal/lifecycle"
)
// waitTimeout is the stop budget the timeout case gives a
// goroutine that never returns. The test's own patience is the
// go test deadline, so the only thing this value affects is how
// long the case takes.
const waitTimeout = 100 * time.Millisecond
func discardLogger() *slog.Logger {
return slog.New(slog.DiscardHandler)
}
func TestWaitForShutdown_DrainedGroup(t *testing.T) {
t.Parallel()
var wg sync.WaitGroup
wg.Go(func() {})
require.NoError(
t,
lifecycle.WaitForShutdown(
context.Background(), discardLogger(),
"test component", &wg,
),
)
}
func TestWaitForShutdown_ContextExpires(t *testing.T) {
t.Parallel()
release := make(chan struct{})
t.Cleanup(func() { close(release) })
var wg sync.WaitGroup
wg.Go(func() { <-release })
ctx, cancel := context.WithTimeout(
context.Background(), waitTimeout,
)
defer cancel()
err := lifecycle.WaitForShutdown(
ctx, discardLogger(), "test component", &wg,
)
require.ErrorIs(t, err, context.DeadlineExceeded)
require.ErrorContains(t, err, "test component")
}

View File

@@ -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,
)
}
}

View File

@@ -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",
)
}

View File

@@ -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)
}

View File

@@ -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
}

View File

@@ -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()