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

[juicycleff]

Six 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 fails depends on where the process runs

West of UTC it fails open. The bound value renders as local wall-clock text, which sorts below the stored UTC text, so the comparison 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 fails closed, rejecting tokens that are still valid. That one is only annoying.

Reproduced on this machine at UTC-5: a verification created with an expiry an hour in the past was still returned as the active one.

The fix

Both sides on UTC, in both places. 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.

This is the same fault as the oauth2 device code sweep fixed in #88.

The second one is ListActiveSignals in the shared signals plugin, which has the 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 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.

Then two more in the same plugin, and these are the interesting ones because they already had conformance cases that passed. CountEventsSince backs the circuit breaker and actions.go calls it with a bare time.Now(); ListReceivedEvents has it on both bounds of its audit window. West of UTC the breaker counts too far back and trips early. East of UTC it counts too little and does not trip at all, which is a rate limit quietly ceasing to limit.

The cases passed because I had written them to construct tidy UTC inputs. That is the real lesson from this branch: a conformance case that supplies its own well-formed input tests the store against a caller that does not exist. Those cases now pass a local-zone time on purpose, matching what the callers actually do, and they fail on sqlite before this change.

An earlier version of this branch claimed the verification lookup was the only remaining case and that the plugin suites covered the rest. That was wrong twice over, and it is why the commit history here has a correction in it.

Guarded, not just fixed

Both new cases live 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 correct, because their expiry columns hold a real instant rather than text, and that is precisely why sqlite-only tests would have been the wrong place to put them.

Each case also checks that a live row is still returned, so a fix that simply rejected everything would not pass.

A check, instead of another sweep

I said last time that a manual sweep had covered everything. It had not, so this branch also adds internal/sqliteguard: a plain test that parses every SQLite store file, finds each Where clause comparing a timestamp column against a bound parameter, and requires the bound value to be on UTC. No linter plugin, runs with the ordinary suite, fails where you are already looking.

It found a sixth instance on its first run, one my grep had missed because the column is not at the start of the clause:

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. Same severity as the verification token, and it survived a sweep I had already called complete.

Go-side comparisons are not checked and do not need to be. time.Time compares instants rather than text, so the bug only exists where the comparison happens inside the database.

ListReceivedEvents also now asserts the newest-first ordering it documents and 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.

Worth reading store/sqlite/principal.go's note on FindActiveDelegation alongside this. Somebody hit a nastier version of the same fault there, where the column's NUMERIC affinity made the predicate constant-true, and solved it by filtering expiry in Go rather than in SQL. That is a stronger fix than normalising, and it is the better answer wherever the row count allows it.

Checks

All four backends green on the full contract suite. The exact CI conformance command green end to end, full suite 96 packages green, lint clean at 2.12.2 and 2.13.1, goimports clean.