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/plugins/sharedsignals/ssftest/event_cases.go b/plugins/sharedsignals/ssftest/event_cases.go index 81189db1..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,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") +} 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..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(), } } @@ -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..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) @@ -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) 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/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/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..6bdad8d5 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 @@ -1142,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)) @@ -1520,3 +1525,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") +}