From 9977b0bc0d9164ae9c46924405ac7667386f73a8 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 20:59:22 -0500 Subject: [PATCH 1/3] fix(sqlite): compare expiry timestamps on the same clock Two lookups bound a local-zone time into a comparison against a TEXT timestamp column. On SQLite that predicate is a string comparison, so both sides have to be on the same clock, and they were not. Which way it breaks depends on where the process runs, and neither direction is acceptable. GetActiveEmailVerification in the core store is the serious one. West of UTC the bound value renders as local wall-clock text, which sorts below the stored UTC text, so the query matches rows it should have excluded and a verification token keeps working for the length of the zone offset after it expires. On a US Pacific deployment that is eight hours. The token in a verification email is a bearer credential, so this is a credential outliving the window it was issued for. East of UTC the same bug rejects tokens that are still valid. ListActiveSignals in the shared signals plugin has the same fault from the other end: it takes the caller's clock as an argument and risk.go hands it a bare time.Now(). Risk signals are what decide whether a past event still constrains a sign-in, so west of UTC an expired signal keeps applying and east of UTC a live one stops early. That second direction fails open on a security control. Both are fixed on both sides. The comparisons bind a UTC value, and the model converters normalise on the way in, so a caller passing a local-zone timestamp cannot reintroduce the mismatch from the storage direction. Guarded by two new cases in the shared contract suites rather than in sqlite tests, so all four backends assert the boundary. Each fails on sqlite before this change and passes after; memory, postgres and mongo were always right, because their expiry columns hold a real instant rather than text, which is exactly why a sqlite-only test would have been the wrong place for them. Both cases also assert that a live row is still returned, so a fix that simply rejected everything would not pass. An earlier version of this commit claimed the verification lookup was the only remaining case and that the plugin stores were already covered. Neither was true: the plugin suites had no signals case at all, which is why nothing caught this one until I went looking. --- plugins/sharedsignals/ssftest/event_cases.go | 43 ++++++++++++++++ plugins/sharedsignals/ssftest/ssftest.go | 1 + plugins/sharedsignals/store_models.go | 6 +-- plugins/sharedsignals/store_sqlite.go | 10 +++- store/sqlite/models.go | 4 +- store/sqlite/store.go | 7 ++- store/storetest/storetest.go | 54 ++++++++++++++++++++ 7 files changed, 117 insertions(+), 8 deletions(-) diff --git a/plugins/sharedsignals/ssftest/event_cases.go b/plugins/sharedsignals/ssftest/event_cases.go index 81189db1..13d52c30 100644 --- a/plugins/sharedsignals/ssftest/event_cases.go +++ b/plugins/sharedsignals/ssftest/event_cases.go @@ -209,3 +209,46 @@ func testCountEventsSince(t *testing.T, f Fixture) { 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") +} diff --git a/plugins/sharedsignals/ssftest/ssftest.go b/plugins/sharedsignals/ssftest/ssftest.go index f1449f68..3dadb600 100644 --- a/plugins/sharedsignals/ssftest/ssftest.go +++ b/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] { diff --git a/plugins/sharedsignals/store_models.go b/plugins/sharedsignals/store_models.go index 525a83a7..1ed788eb 100644 --- a/plugins/sharedsignals/store_models.go +++ b/plugins/sharedsignals/store_models.go @@ -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(), } } diff --git a/plugins/sharedsignals/store_sqlite.go b/plugins/sharedsignals/store_sqlite.go index 3afe2c82..7197c2cf 100644 --- a/plugins/sharedsignals/store_sqlite.go +++ b/plugins/sharedsignals/store_sqlite.go @@ -231,7 +231,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 +244,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) diff --git a/store/sqlite/models.go b/store/sqlite/models.go index 112f0c9a..ced47f3d 100644 --- a/store/sqlite/models.go +++ b/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(), } } diff --git a/store/sqlite/store.go b/store/sqlite/store.go index 5fd83268..5b91c9ba 100644 --- a/store/sqlite/store.go +++ b/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) diff --git a/store/storetest/storetest.go b/store/storetest/storetest.go index 402cc497..d8280b73 100644 --- a/store/storetest/storetest.go +++ b/store/storetest/storetest.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/xraph/authsome/account" "github.com/xraph/authsome/app" "github.com/xraph/authsome/environment" "github.com/xraph/authsome/id" @@ -76,6 +77,7 @@ func RunConformance(t *testing.T, newStore Factory, skip ...string) { {"SessionActorChainRoundTrip", testSessionActorChainRoundTrip}, {"ServiceAccountKindDefaultsToService", testServiceAccountKindDefaultsToService}, {"SessionDPoPJKTRoundTrip", testSessionDPoPJKTRoundTrip}, + {"ExpiredEmailVerificationIsNotActive", testExpiredEmailVerificationIsNotActive}, } for _, tc := range cases { tc := tc @@ -1520,3 +1522,55 @@ func SeedUser(t *testing.T, s store.Store, tn Tenant, email string) id.UserID { t.Helper() return seedUser(t, s, tenant(tn), email).ID } + +// testExpiredEmailVerificationIsNotActive checks the expiry boundary on the +// active-verification lookup. The token in a verification email is a bearer +// credential, so one that outlives its expiry is a credential that works +// after it was supposed to stop. +// +// It is a boundary worth testing on every backend rather than trusting, +// because the comparison happens in the database and not in Go: SQLite stores +// these timestamps as TEXT and compares them as strings, so a query that +// binds a differently-zoned value than the one it stored gets an answer that +// is wrong by the offset in whichever direction the zone runs. +func testExpiredEmailVerificationIsNotActive(t *testing.T, s store.Store) { + ctx := context.Background() + tn := seedTenant(t, s) + u := seedUser(t, s, tn, "verify-expiry-"+suffix(tn.AppID.String())+"@example.test") + + expired := &account.Verification{ + ID: id.NewVerificationID(), + AppID: tn.AppID, + EnvID: tn.EnvID, + UserID: u.ID, + Token: "expired-" + suffix(tn.AppID.String()), + Type: account.VerificationEmail, + ExpiresAt: now().Add(-time.Hour), + CreatedAt: now().Add(-2 * time.Hour), + } + require.NoError(t, s.CreateVerification(ctx, expired)) + + got, err := s.GetActiveEmailVerification(ctx, u.ID) + if err == nil { + assert.NotEqual(t, expired.ID, got.ID, + "a verification that expired an hour ago is still being served as active") + } + + // And the live one is found, so the boundary is not simply rejecting + // everything. + live := &account.Verification{ + ID: id.NewVerificationID(), + AppID: tn.AppID, + EnvID: tn.EnvID, + UserID: u.ID, + Token: "live-" + suffix(tn.AppID.String()), + Type: account.VerificationEmail, + ExpiresAt: now().Add(time.Hour), + CreatedAt: now(), + } + require.NoError(t, s.CreateVerification(ctx, live)) + + got, err = s.GetActiveEmailVerification(ctx, u.ID) + require.NoError(t, err, "a verification with an hour left must be found") + assert.Equal(t, live.ID, got.ID, "the active lookup returned the wrong verification") +} From 0a9ec57b91e2386ce22e6c79dae34aa872782904 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 21:43:49 -0500 Subject: [PATCH 2/3] fix(sharedsignals/sqlite): put the audit window and breaker count on UTC Two more comparisons against a TEXT timestamp column bound the caller's clock unnormalised, and both had conformance cases that passed anyway because those cases were the ones passing UTC. CountEventsSince is the one that matters. actions.go calls it with a bare time.Now() and it backs the circuit breaker, so west of UTC the count sweeps in events from further back than the window and the breaker trips early, while east of UTC it sees fewer than it should and does not trip at all. A rate limit that quietly stops limiting is the failure worth avoiding here. ListReceivedEvents has the same problem on both bounds of its window, which skews the audit trail by the zone offset in whichever direction the zone runs. Both binds are now .UTC(), and fromReceivedEvent normalises ReceivedAt on the way in so the stored side cannot drift either. The cases now pass a local-zone time on purpose, matching what the real callers do. That is the actual lesson from this one: a conformance case that constructs its own tidy UTC input tests the store against a caller that does not exist. Both cases fail on sqlite before this change and pass after. Also added the newest-first assertion ListReceivedEvents documents but nothing checked. Ordering a text timestamp column is a string sort, so it only agrees with chronological order while every stored value is on one clock, which is now true going forward. --- plugins/sharedsignals/ssftest/event_cases.go | 21 ++++++++++++++++---- plugins/sharedsignals/store_models.go | 2 +- plugins/sharedsignals/store_sqlite.go | 13 +++++++++--- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/plugins/sharedsignals/ssftest/event_cases.go b/plugins/sharedsignals/ssftest/event_cases.go index 13d52c30..6f007c7b 100644 --- a/plugins/sharedsignals/ssftest/event_cases.go +++ b/plugins/sharedsignals/ssftest/event_cases.go @@ -124,8 +124,8 @@ 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") @@ -133,6 +133,16 @@ func testListReceivedEventsRespectsWindow(t *testing.T, f Fixture) { 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,7 @@ 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") } diff --git a/plugins/sharedsignals/store_models.go b/plugins/sharedsignals/store_models.go index 1ed788eb..5242b86f 100644 --- a/plugins/sharedsignals/store_models.go +++ b/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(), } } diff --git a/plugins/sharedsignals/store_sqlite.go b/plugins/sharedsignals/store_sqlite.go index 7197c2cf..bb328ea8 100644 --- a/plugins/sharedsignals/store_sqlite.go +++ b/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) From 57c68fe65210cce221b9d8ac810f3d3114f1cb90 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:05:48 -0500 Subject: [PATCH 3/3] test(sqlite): fail the build on an unnormalised timestamp comparison This is the fifth and sixth time this repo has hit the same fault, so it is worth a check rather than another round of greps. internal/sqliteguard parses every SQLite store file, finds each Where clause that compares a timestamp column against a bound parameter, and requires the bound value to have been put on UTC. It is a plain test, so it runs with the ordinary suite, needs no linter plugin, and fails where a developer is already looking. A Go-side comparison is not checked and does not need to be: time.Time compares instants rather than text, so the bug only exists where the comparison happens inside the database. It earned its place immediately. It found a sixth instance that my own grep had missed, because the grep expected the column at the start of the clause and this one does not have it there: store/sqlite/principal.go Where("(expires_at IS NULL OR expires_at >= ?)", q.ActiveAsOf) That gates whether a service account or agent principal is still active, and engine_principal.go passes a bare time.Now(). West of UTC an expired principal keeps authenticating for the length of the zone offset. It is the same severity as the verification token, and it survived a manual sweep that I had already described as complete. Fixed, and proved rather than assumed: EphemeralPrincipalExpiry now passes ActiveAsOf on the local clock like its real caller does, and it fails with the fix reverted. The previous version of that case constructed a UTC value and passed either way, which is the same blind spot the shared signals cases had. Worth reading store/sqlite/principal.go's note on FindActiveDelegation next to this. Somebody hit a nastier version of the same thing there, where the column's NUMERIC affinity made the predicate constant-true, and solved it by filtering expiry in Go instead. That is a stronger fix than normalising, and it is the right answer wherever the row count allows it. --- internal/sqliteguard/doc.go | 6 ++ internal/sqliteguard/utcbind_test.go | 154 +++++++++++++++++++++++++++ store/sqlite/principal.go | 7 +- store/storetest/storetest.go | 5 +- 4 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 internal/sqliteguard/doc.go create mode 100644 internal/sqliteguard/utcbind_test.go diff --git a/internal/sqliteguard/doc.go b/internal/sqliteguard/doc.go new file mode 100644 index 00000000..1bddc842 --- /dev/null +++ b/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 diff --git a/internal/sqliteguard/utcbind_test.go b/internal/sqliteguard/utcbind_test.go new file mode 100644 index 00000000..e912f8b0 --- /dev/null +++ b/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 "" +} diff --git a/store/sqlite/principal.go b/store/sqlite/principal.go index 3a7b5bdc..ef9f551c 100644 --- a/store/sqlite/principal.go +++ b/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) diff --git a/store/storetest/storetest.go b/store/storetest/storetest.go index d8280b73..6bdad8d5 100644 --- a/store/storetest/storetest.go +++ b/store/storetest/storetest.go @@ -1144,8 +1144,11 @@ func testEphemeralPrincipalExpiry(t *testing.T, s store.Store) { assert.Equal(t, parent.ID.String(), gotLapsed.Parent.ID) assert.False(t, gotLapsed.IsActive(now()), "an expired principal must not read as active") + // ActiveAsOf on the local clock, matching engine_principal.go's own call. + // A UTC value here passes on every backend and hides that the filtering + // happens in the database against a text column. active, err := s.ListPrincipals(ctx, &principal.Query{ - AppID: tn.AppID, Kind: principal.KindAgent, ActiveOnly: true, ActiveAsOf: now(), + AppID: tn.AppID, Kind: principal.KindAgent, ActiveOnly: true, ActiveAsOf: time.Now(), }) require.NoError(t, err) ids := make([]string, 0, len(active))