Repository navigation
fix(auth): enforce token expiry on cache hits - #712
Conversation
VerifyToken memoized a successful lookup for up to auth.cache_ttl and the cache-hit branch checked only that entry deadline, never the token's own expires_at. A token that expired while cached kept authorizing requests until the entry aged out, while the same token was correctly rejected whenever the lookup reached SQLite — authorization depended on cache state rather than on the credential. Revoke, delete, update and rotate all invalidate the cache immediately and were never affected; only passive expiry was. The cache-hit path now rejects and evicts an entry whose token has expired, falling through to the database path that owns the decision and its log line. Cache entries are additionally never held past the token's own expiry, so the janitor and the size-eviction scan cannot prefer entries that can no longer authorize anything. Also normalizes expires_at to UTC on the standalone create and update paths. created_at is filled by SQLite's CURRENT_TIMESTAMP and is always UTC, but expires_at was text-encoded in the caller's own location, putting two timezone domains in one table and rendering the two columns in different zones over the API. The clustered apply path already normalized (#459/#460); this matches it and the "Arc-stamped timestamps are UTC" rule from #546. No token was ever accepted or rejected incorrectly — expiry is compared as parsed instants in Go, never as strings. Detail accompanies the forthcoming security advisory. Reported by @rexpository.
Review follow-up. The security fix is unchanged; these correct three things about the UTC half of the previous commit. The prior commit message and release note said created_at and expires_at were put into "one timezone domain" by normalizing the write. They are not, and the stated formats were wrong. Probed against go-sqlite3 v1.14.34, the bytes on disk are: created_at (CURRENT_TIMESTAMP) "2026-09-10 22:09:40" expires_at (UTC time.Time) "2026-09-10 21:32:45.958903+00:00" expires_at (UTC-6 time.Time) "2026-09-10 15:32:45.958903-06:00" So a correct UTC write carries an explicit +00:00, not a bare Z, and the column still differs textually from created_at after the fix — different offset suffix, different fractional-second handling. What normalizing actually removes is the offset VARIANCE: two rows holding the same instant no longer sort differently as text depending on which node wrote them. That is the real hazard, and expires_at must still be compared as a parsed instant, never as text. The comment and release note now say that. TestCreateToken_ExpiresAtStoredInUTC asserted on a value scanned into a Go string, which is the driver's RFC3339 re-rendering rather than the stored bytes — so it was testing go-sqlite3, and passed for the wrong reason. It now asserts the parsed value's zone offset is zero (encoding-independent) and additionally pins the raw bytes via CAST(expires_at AS TEXT). Re-verified against unfixed code: it fails on the parsed offset (-21600 seconds) under both TZ=UTC and TZ=Europe/Berlin, so it is a real regression test in any timezone. TestVerifyToken_ExpiresWhileCached_EndToEnd claimed to exercise the cache-hit expiry check. Instrumenting it shows the entry's capped deadline has already passed by then, so it takes a natural cache miss: it is an end-to-end proof of the deadline cap, not of the cache-hit branch. Comment corrected to say which tests cover that branch. Verified: go test -race ./internal/auth/... ok (181s); all four security regression tests still fail against unfixed code.
Review pass completeTwo reviewers per CLAUDE.md: one deep adversarial, one Security Checklist. Both independently ran revert-run-restore proofs and probed go-sqlite3 directly rather than reasoning from assumption. Security Checklist reviewer — 0 Blockers / 0 High / 0 Medium. Verdict: "the expiry bypass is fully closed." It verified the migration-safety claim empirically (a throwaway go-sqlite3 module): legacy offset-bearing rows parse to the identical instant post-change, so no existing token's lifetime shifts. It also confirmed fail-closed behaviour on a DB error, no metrics double-count, no DoS amplification (at most one extra query per expired-token request, and the deadline cap prevents an expired entry from ever being re-created), and no new logging of secrets. Deep reviewer — 0 Blockers / 2 High / 1 Medium, all on the UTC half; the security fix itself was confirmed correct, with sound concurrency and genuine pre-fix-failing tests. All three findings are fixed in 0047a21:
Worth recording from the security review: at the SQL string level the two formats genuinely diverge — its probe showed Re-verified after the fixes: |
1 participant
Closes a token-expiry gap in
AuthManager.VerifyToken, plus a timezone inconsistency found while verifying it on a live binary.Detail accompanies the forthcoming security advisory. Reported by @rexpository.
Summary
VerifyTokenmemoizes a successful lookup for up toauth.cache_ttl(default 300s). The cache-hit branch checked only that entry deadline, never the token's ownexpires_at, so a token that expired while cached kept authorizing requests until the entry aged out. The same token was correctly rejected whenever the lookup reached SQLite, so authorization depended on cache state rather than on the credential. An entry whose token has expired is now evicted and re-validated against the database, which rejects it.cleanupExpiredCacheand the size-eviction scan — both of which order by the cache deadline — cannot prefer entries that can no longer authorize anything.expires_atnow stored in UTC on the standalone create and update paths.created_atcomes from SQLite'sCURRENT_TIMESTAMP(always UTC) whileexpires_atwas text-encoded in the caller's own location, so a non-UTC deployment stored2026-09-10 15:32:45.958903-06:00beside2026-09-10 21:32:41in the same table. The clustered apply path already normalized (#459/#460); this matches it and the "Arc-stamped timestamps are UTC" rule from #546.Scope: revoke, delete, update and rotate all call
InvalidateCache()and were never affected — only passive expiry was. No token was ever accepted or rejected incorrectly by the timezone issue: expiry is compared as parsed instants in Go, never as strings, and there is no SQL-level comparison on the column. Existing rows are left as they are.Configuration matrix
auth.enabled=falseauthManager == nil, middleware callsc.Next()(middleware.go:114)auth.cache_ttl=300(default)auth.go:855entry.infonon-nil by construction (auth.go:983)auth.cache_ttl=600(NON-DEFAULT)"cache_ttl":600auth.cache_ttl=0/ negative (non-default)now.Before(now)is false) → DB lookup each timecleanupLoopclamps its ticker to 10s (auth.go:468)auth.max_cache_sizesmall (non-default)expiresAt, now capped at token expiryExpiresAt == nil)auth.go:856)auth.go:915)VerifyToken; those writes already UTC (cluster_apply.go:159)InvalidateCacheon apply unchangedGET /api/v1/auth/verifyVerifyTokenTest plan
auth.goreverted toHEAD, the four new regression tests FAIL for the right reasons —200instead of401, uncapped cache deadline, no eviction, offset-bearingexpires_at. Two negative controls pass in both states.go test -race ./internal/auth/...— ok, 179s, full packagegofmt -l ./internal/authempty;go vet ./internal/auth/...cleango build -tags=duckdb_arrow),auth.cache_ttl=600, token created viaPOST /api/v1/auth/tokenswithexpires_in:200/200/200while valid →401/401after expiry on both/api/v1/databasesand/api/v1/auth/verify. With the entry still well inside its 600s TTL, unfixed code would have returned 200 for ~10 more minutes.New tests:
TestVerifyToken_CachedTokenRejectedAfterExpiry(deterministic),TestVerifyToken_ExpiresWhileCached_EndToEnd(real timing),TestVerifyToken_CacheDeadlineCappedAtTokenExpiry,TestCreateToken_ExpiresAtStoredInUTC,TestMiddleware_CachedExpiredToken, plus negative controls for still-valid and non-expiring tokens.