Compare expiry timestamps on the same clock, and guard against the next one

internal/sqliteguard/doc.go

@@ -0,0 +1,6 @@
+// Package sqliteguard holds a static check over the SQLite store code.
+//
+// It has no runtime code. The check lives in the test file next to this one
+// so it runs with the ordinary suite, needs no linter plugin, and fails in
+// the same place a developer is already looking.
+package sqliteguard

internal/sqliteguard/utcbind_test.go

@@ -0,0 +1,154 @@
+package sqliteguard
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// timestampComparison matches a WHERE fragment that compares a timestamp
+// column against a bound parameter: `expires_at > ?`, `received_at >= ?`,
+// `deleted_at < ?`. It deliberately does not match equality, which is not
+// affected by ordering, or a Set clause, which writes rather than compares.
+var timestampComparison = regexp.MustCompile(`\b\w*(_at|expires\w*)\s*[<>]=?\s*\?`)
+
+// TestTimestampComparisonsBindUTC is the guard on a bug this repo has now hit
+// five times.
+//
+// SQLite has no timestamp type. These schemas store timestamps as TEXT, so a
+// predicate like `expires_at > ?` is a string comparison, and it is only
+// correct while both sides carry the same wall clock. Bind a local-zone
+// time.Time against a stored UTC value and the answer is wrong by the zone
+// offset, in a direction that depends on which side of UTC the process runs.
+// West of UTC an expiry check keeps expired rows alive. East of it a rate
+// limit stops counting.
+//
+// None of those failures look like a timezone bug from the outside, they look
+// like a token that would not die or a limiter that would not trip, so this
+// asserts the invariant rather than trusting anybody to remember it: every
+// timestamp comparison in a SQLite store binds a value that has been put on
+// UTC, either with .UTC() or through the local utc() helper.
+//
+// A Go-side comparison is not affected and is not checked. time.Time compares
+// instants, not text, so two correctly constructed times compare correctly
+// whatever zone they carry. The bug only exists where the comparison happens
+// inside the database.
+func TestTimestampComparisonsBindUTC(t *testing.T) {
+	root := repoRoot(t)
+
+	files := []string{}
+	sqliteDir := filepath.Join(root, "store", "sqlite")
+	entries, err := os.ReadDir(sqliteDir)
+	require.NoError(t, err, "read store/sqlite")
+	for _, e := range entries {
+		if strings.HasSuffix(e.Name(), ".go") && !strings.HasSuffix(e.Name(), "_test.go") {
+			files = append(files, filepath.Join(sqliteDir, e.Name()))
+		}
+	}
+	plugins, err := filepath.Glob(filepath.Join(root, "plugins", "*", "store_sqlite.go"))
+	require.NoError(t, err)
+	files = append(files, plugins...)
+
+	require.NotEmpty(t, files, "found no sqlite store files to check; has the layout moved?")
+
+	var violations []string
+	fset := token.NewFileSet()
+	for _, path := range files {
+		f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution)
+		require.NoError(t, err, "parse %s", path)
+
+		ast.Inspect(f, func(n ast.Node) bool {
+			call, ok := n.(*ast.CallExpr)
+			if !ok || len(call.Args) < 2 {
+				return true
+			}
+			sel, ok := call.Fun.(*ast.SelectorExpr)
+			if !ok || sel.Sel.Name != "Where" {
+				return true
+			}
+			lit, ok := call.Args[0].(*ast.BasicLit)
+			if !ok || lit.Kind != token.STRING {
+				return true
+			}
+			clause, err := strconv.Unquote(lit.Value)
+			if err != nil || !timestampComparison.MatchString(clause) {
+				return true
+			}
+			if isUTCNormalized(call.Args[1]) {
+				return true
+			}
+			pos := fset.Position(call.Pos())
+			violations = append(violations, "  "+relTo(root, pos.Filename)+":"+
+				strconv.Itoa(pos.Line)+"  Where("+strconv.Quote(clause)+", "+
+				exprString(call.Args[1])+")")
+			return true
+		})
+	}
+
+	if len(violations) > 0 {
+		t.Fatalf("timestamp comparisons in SQLite stores must bind a UTC value, "+
+			"because these columns are TEXT and the comparison is a string sort.\n"+
+			"Wrap the bound value in .UTC(), or utc() where the package has one:\n%s",
+			strings.Join(violations, "\n"))
+	}
+}
+
+// isUTCNormalized reports whether an expression has been put on UTC: either
+// something.UTC() or a call to the package-local utc() helper.
+func isUTCNormalized(e ast.Expr) bool {
+	call, ok := e.(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+	switch fn := call.Fun.(type) {
+	case *ast.SelectorExpr:
+		return fn.Sel.Name == "UTC"
+	case *ast.Ident:
+		return fn.Name == "utc"
+	}
+	return false
+}
+
+func exprString(e ast.Expr) string {
+	switch v := e.(type) {
+	case *ast.Ident:
+		return v.Name
+	case *ast.SelectorExpr:
+		return exprString(v.X) + "." + v.Sel.Name
+	case *ast.CallExpr:
+		return exprString(v.Fun) + "(...)"
+	}
+	return "the bound value"
+}
+
+func relTo(root, path string) string {
+	if rel, err := filepath.Rel(root, path); err == nil {
+		return rel
+	}
+	return path
+}
+
+// repoRoot walks up from the test's own directory to the module root.
+func repoRoot(t *testing.T) string {
+	t.Helper()
+	dir, err := os.Getwd()
+	require.NoError(t, err)
+	for range 10 {
+		if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
+			return dir
+		}
+		parent := filepath.Dir(dir)
+		require.NotEqual(t, parent, dir, "walked past the filesystem root without finding go.mod")
+		dir = parent
+	}
+	t.Fatal("could not locate the module root")
+	return ""
+}

plugins/sharedsignals/ssftest/event_cases.go

@@ -124,15 +124,25 @@ func testListReceivedEventsRespectsWindow(t *testing.T, f Fixture) {
 	// [base+1m, base+2m) must hold exactly the middle event.
 	got, err := f.Store.ListReceivedEvents(ctx, f.AppID, ssf.ReceivedEventFilter{
 		StreamID: s.ID,
-		Since:    base.Add(time.Minute),
-		Until:    base.Add(2 * time.Minute),
+		Since:    base.Add(time.Minute).Local(),
+		Until:    base.Add(2 * time.Minute).Local(),
 	})
 	require.NoError(t, err)
 	assert.Len(t, got, 1, "the window is half-open: Since is inclusive and Until is exclusive")
 
 	all, err := f.Store.ListReceivedEvents(ctx, f.AppID, ssf.ReceivedEventFilter{StreamID: s.ID})
 	require.NoError(t, err)
 	assert.Len(t, all, 3, "an unbounded window must return every row on the stream")
+
+	// Newest first, as the interface documents. Worth asserting rather than
+	// assuming: on a backend ordering a text timestamp column this is a
+	// string sort, and it only agrees with chronological order while every
+	// stored value is on the same clock.
+	for i := 1; i < len(all); i++ {
+		assert.False(t, all[i].ReceivedAt.After(all[i-1].ReceivedAt),
+			"audit rows came back out of order: %v then %v",
+			all[i-1].ReceivedAt, all[i].ReceivedAt)
+	}
 }
 
 // testListReceivedEventsClampsLimit covers the two documented bounds: no
@@ -193,7 +203,10 @@ func testCountEventsSince(t *testing.T, f Fixture) {
 		require.NoError(t, f.Store.InsertReceivedEvent(ctx, e))
 	}
 
-	n, err := f.Store.CountEventsSince(ctx, s.ID, now().Add(-time.Minute))
+	// time.Now() unqualified, matching the breaker's own call in actions.go.
+	// A UTC bound value here would pass on every backend and hide the fact
+	// that the comparison happens in the database against a text column.
+	n, err := f.Store.CountEventsSince(ctx, s.ID, time.Now().Add(-time.Minute))
 	require.NoError(t, err)
 	assert.Equal(t, 3, n, "the breaker count must see recent events and not the older one")
 
@@ -205,7 +218,50 @@ func testCountEventsSince(t *testing.T, f Fixture) {
 	signal.ReceivedAt = now()
 	require.NoError(t, f.Store.InsertReceivedEvent(ctx, signal))
 
-	n, err = f.Store.CountEventsSince(ctx, s.ID, now().Add(-time.Minute))
+	n, err = f.Store.CountEventsSince(ctx, s.ID, time.Now().Add(-time.Minute))
 	require.NoError(t, err)
 	assert.Equal(t, 4, n, "the breaker must count events that took no action, not just the ones that did")
 }
+
+// testExpiredSignalIsNotActive checks the expiry boundary on the risk-signal
+// lookup, which is what decides whether a past event still constrains a
+// sign-in now. It takes the caller's clock as an argument, so the case passes
+// a local-zone time on purpose: that is what the risk path actually does, and
+// on a backend storing expires_at as text the comparison is only correct if
+// the store normalises what it is handed.
+func testExpiredSignalIsNotActive(t *testing.T, f Fixture) {
+	ctx := context.Background()
+	s := seedStream(t, f)
+
+	expired := &ssf.Signal{
+		ID: id.NewSSFSignalID(), AppID: f.AppID, EnvID: f.EnvID, UserID: f.UserID,
+		StreamID: s.ID, EventType: sessionRevoked, Severity: 5, Reason: "expired",
+		EventAt: now().Add(-2 * time.Hour), ExpiresAt: now().Add(-time.Hour),
+		CreatedAt: now().Add(-2 * time.Hour),
+	}
+	require.NoError(t, f.Store.CreateSignal(ctx, expired))
+
+	live := &ssf.Signal{
+		ID: id.NewSSFSignalID(), AppID: f.AppID, EnvID: f.EnvID, UserID: f.UserID,
+		StreamID: s.ID, EventType: sessionRevoked, Severity: 7, Reason: "live",
+		EventAt: now(), ExpiresAt: now().Add(time.Hour), CreatedAt: now(),
+	}
+	require.NoError(t, f.Store.CreateSignal(ctx, live))
+
+	// time.Now() unqualified, matching the risk path's own call.
+	got, err := f.Store.ListActiveSignals(ctx, f.AppID, f.EnvID, f.UserID, time.Now())
+	require.NoError(t, err)
+
+	var sawExpired, sawLive bool
+	for _, sig := range got {
+		if sig.ID == expired.ID {
+			sawExpired = true
+		}
+		if sig.ID == live.ID {
+			sawLive = true
+		}
+	}
+	assert.False(t, sawExpired,
+		"a risk signal that expired an hour ago is still constraining sign-in")
+	assert.True(t, sawLive, "a signal with an hour left must still apply")
+}

plugins/sharedsignals/ssftest/ssftest.go

@@ -63,6 +63,7 @@ func RunConformance(t *testing.T, newFixture Factory, skip ...string) {
 		{"ListReceivedEventsClampsLimit", testListReceivedEventsClampsLimit},
 		{"DeleteReceivedEventFreesTheJTI", testDeleteReceivedEventFreesTheJTI},
 		{"CountEventsSince", testCountEventsSince},
+		{"ExpiredSignalIsNotActive", testExpiredSignalIsNotActive},
 	}
 	for _, tc := range cases {
 		if skipSet[tc.name] {

plugins/sharedsignals/store_models.go

@@ -270,7 +270,7 @@ func fromReceivedEvent(e *ReceivedEvent) *receivedEventModel {
 		Outcome:        e.Outcome,
 		ActionTaken:    e.ActionTaken,
 		Error:          e.Error,
-		ReceivedAt:     e.ReceivedAt,
+		ReceivedAt:     e.ReceivedAt.UTC(),
 	}
 }
 
@@ -310,9 +310,9 @@ func fromSignal(s *Signal) *signalModel {
 		EventType: s.EventType,
 		Severity:  s.Severity,
 		Reason:    s.Reason,
-		EventAt:   s.EventAt,
-		ExpiresAt: s.ExpiresAt,
-		CreatedAt: s.CreatedAt,
+		EventAt:   s.EventAt.UTC(),
+		ExpiresAt: s.ExpiresAt.UTC(),
+		CreatedAt: s.CreatedAt.UTC(),
 	}
 }
 

plugins/sharedsignals/store_sqlite.go

@@ -191,11 +191,15 @@ func (s *SqliteStore) ListReceivedEvents(ctx context.Context, appID id.AppID,
 
 	var models []*receivedEventModel
 	q := s.sdb.NewSelect(&models).Where("stream_id = ?", f.StreamID.String())
+	// .UTC() on both bounds: received_at is TEXT here, so these are string
+	// comparisons and a caller passing a local-zone window would select by
+	// wall-clock text against stored UTC text, shifting the window by the
+	// zone offset.
 	if !f.Since.IsZero() {
-		q = q.Where("received_at >= ?", f.Since)
+		q = q.Where("received_at >= ?", f.Since.UTC())
 	}
 	if !f.Until.IsZero() {
-		q = q.Where("received_at < ?", f.Until)
+		q = q.Where("received_at < ?", f.Until.UTC())
 	}
 	// The id tie-break makes the page deterministic when several rows share
 	// a received_at, which one multi-event SET produces by construction.
@@ -221,7 +225,10 @@ func (s *SqliteStore) CountEventsSince(ctx context.Context,
 	// Store.CountEventsSince.
 	count, err := s.sdb.NewSelect((*receivedEventModel)(nil)).
 		Where("stream_id = ?", streamID.String()).
-		Where("received_at > ?", since).
+		// .UTC(): actions.go calls this with a bare time.Now(), and the
+		// breaker counting the wrong window either trips early or, east of
+		// UTC, fails to trip at all.
+		Where("received_at > ?", since.UTC()).
 		Count(ctx)
 	if err != nil {
 		return 0, sqlErr(err)
@@ -231,7 +238,7 @@ func (s *SqliteStore) CountEventsSince(ctx context.Context,
 
 func (s *SqliteStore) CreateSignal(ctx context.Context, sig *Signal) error {
 	if sig.CreatedAt.IsZero() {
-		sig.CreatedAt = time.Now()
+		sig.CreatedAt = time.Now().UTC()
 	}
 	_, err := s.sdb.NewInsert(fromSignal(sig)).Exec(ctx)
 	return sqlErr(err)
@@ -244,7 +251,13 @@ func (s *SqliteStore) ListActiveSignals(ctx context.Context, appID id.AppID,
 		Where("app_id = ?", appID.String()).
 		Where("env_id = ?", envID.String()).
 		Where("user_id = ?", userID.String()).
-		Where("expires_at > ?", now).
+		// now.UTC(), not the caller's clock as given: expires_at is TEXT in
+		// this schema, so this predicate is a string comparison and both
+		// sides have to agree. The risk path calls this with a bare
+		// time.Now(), and west of UTC an unnormalised bound value keeps
+		// expired signals constraining sign-in while east of it live signals
+		// stop applying early.
+		Where("expires_at > ?", now.UTC()).
 		OrderExpr("severity DESC").
 		Scan(ctx); err != nil {
 		return nil, sqlErr(err)

store/sqlite/models.go

@@ -496,9 +496,9 @@ func fromVerification(v *account.Verification) *VerificationModel {
 		Token:     v.Token,
 		Type:      string(v.Type),
 		Attempts:  v.Attempts,
-		ExpiresAt: v.ExpiresAt,
+		ExpiresAt: v.ExpiresAt.UTC(),
 		Consumed:  v.Consumed,
-		CreatedAt: v.CreatedAt,
+		CreatedAt: v.CreatedAt.UTC(),
 	}
 }
 

store/sqlite/principal.go

@@ -169,7 +169,12 @@ func (s *Store) ListPrincipals(ctx context.Context, q *principal.Query) ([]*prin
 		// disagree with the domain method every non-store caller uses.
 		query = query.
 			Where("active = TRUE").
-			Where("(expires_at IS NULL OR expires_at >= ?)", q.ActiveAsOf)
+			// .UTC(): expires_at is TEXT here, so this is a string
+			// comparison and the caller's clock has to be normalised.
+			// engine_principal.go passes a bare time.Now(), and west of UTC
+			// an unnormalised bound value keeps an expired principal
+			// authenticating for the length of the zone offset.
+			Where("(expires_at IS NULL OR expires_at >= ?)", q.ActiveAsOf.UTC())
 	}
 	if q.Limit > 0 {
 		query = query.Limit(q.Limit)

store/sqlite/store.go

@@ -528,7 +528,12 @@ func (s *Store) GetActiveEmailVerification(ctx context.Context, userID id.UserID
 		Where("user_id = ?", userID.String()).
 		Where("type = ?", string(account.VerificationEmail)).
 		Where("consumed = FALSE").
-		Where("expires_at > ?", time.Now()).
+		// .UTC(), not a bare time.Now(): expires_at is TEXT in this schema,
+		// so this predicate is a string comparison and both sides have to be
+		// on the same clock. With a local-zone bound value the answer is
+		// wrong by the offset, and west of UTC it is wrong in the direction
+		// that keeps an expired verification token working.
+		Where("expires_at > ?", time.Now().UTC()).
 		OrderExpr("created_at DESC").
 		Limit(1).
 		Scan(ctx)