fix(store): index api_tokens.token_hash on both backends (BUG-3668) (#2157)

* fix(store): index api_tokens.token_hash on both backends (BUG-3668)

Every token-authenticated request looks its token up by token_hash, and
nothing indexed it, so each lookup scanned api_tokens, and a well-formed
invalid token scanned all of it. Migration 146 (SQLite) and 116 (Postgres) add
a plain index, as sessions and the other token tables have; UNIQUE would fail
the migration on a database already holding a duplicate.

TestBUG3668_CredentialLookupsAreIndexed is the census of the class: every
request-path credential lookup column, planned on the migrated schema.
Before: SQLite "SCAN api_tokens", Postgres "Seq Scan on api_tokens" (read
with enable_seqscan off). After: "SEARCH api_tokens USING INDEX
idx_api_tokens_token_hash" and "Index Scan using idx_api_tokens_token_hash".
The other eleven lookups were already indexed on both.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* test(store): the credential-lookup census covers the app code tables and requires an index, not just no named scan (BUG-3668, codex r1)

Adds app_install_codes.code_sha256 and app_context_codes.code_sha256 (both
already indexed by their keys on both backends). The plan must now show an
index search on SQLite and an index scan on Postgres. Red on both backends
with the two migrations removed.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* style(store): gofmt the credential-lookup census (BUG-3668)

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS
This commit is contained in:
xarmian
2026-10-14 08:16:14 -04:00
committed by GitHub
parent 87101002a2
commit b9db178e1e
3 changed files with 108 additions and 0 deletions
@@ -0,0 +1,96 @@
package store
import (
"regexp"
"strings"
"testing"
)
// BUG-3668: api_tokens.token_hash had no index, so every token-authenticated
// request scanned the table, and a well-formed invalid token had to exhaust
// the scan. This is the census of the class: every column a request-path
// credential lookup filters on, each checked on the fully migrated schema
// for an index search. The question is whether the column is indexed, so
// each query is the lookup's predicate alone, not the full statement.
//
// SQLite: EXPLAIN QUERY PLAN must report a SEARCH of the table using an
// index. Postgres plans a seq scan on a tiny table even with an index, so the
// plan is read with enable_seqscan off: an index scan is then chosen iff one
// exists.
func TestBUG3668_CredentialLookupsAreIndexed(t *testing.T) {
s := testStore(t)
census := []struct{ lookup, table, query string }{
{"API token (REST and MCP auth)", "api_tokens", `SELECT id FROM api_tokens WHERE token_hash = ?`},
{"session cookie", "sessions", `SELECT user_id FROM sessions WHERE token_hash = ?`},
{"password reset", "password_reset_tokens", `SELECT user_id FROM password_reset_tokens WHERE token_hash = ?`},
{"email verification and account claim", "email_verification_tokens", `SELECT user_id FROM email_verification_tokens WHERE token_hash = ?`},
{"share link", "share_links", `SELECT id FROM share_links WHERE token_hash = ?`},
{"invitation by code hash", "workspace_invitations", `SELECT id FROM workspace_invitations WHERE code_hash = ?`},
{"invitation by code", "workspace_invitations", `SELECT id FROM workspace_invitations WHERE code = ?`},
{"CLI auth session", "cli_auth_sessions", `SELECT code FROM cli_auth_sessions WHERE code = ?`},
{"OAuth authorization code", "oauth_authorization_codes", `SELECT signature FROM oauth_authorization_codes WHERE signature = ?`},
{"OAuth access token", "oauth_access_tokens", `SELECT signature FROM oauth_access_tokens WHERE signature = ?`},
{"OAuth refresh token", "oauth_refresh_tokens", `SELECT signature FROM oauth_refresh_tokens WHERE signature = ?`},
{"OAuth PKCE request", "oauth_pkce_requests", `SELECT signature FROM oauth_pkce_requests WHERE signature = ?`},
{"app install code", "app_install_codes", `SELECT install_id FROM app_install_codes WHERE code_sha256 = ?`},
{"app context code", "app_context_codes", `SELECT install_id FROM app_context_codes WHERE code_sha256 = ?`},
}
for _, c := range census {
plan := explainPlan(t, s, c.query, "x")
// Positive: the plan must read the table through an index, not
// merely avoid one named scan node (codex r1).
var indexed bool
if s.dialect.Driver() == DriverPostgres {
indexed = regexp.MustCompile(`(Index Scan|Index Only Scan) using \S+ on `+c.table+`\b|Bitmap Index Scan on `).MatchString(plan) &&
!strings.Contains(plan, "Seq Scan on "+c.table)
} else {
indexed = regexp.MustCompile(`SEARCH ` + c.table + ` USING (COVERING )?INDEX `).MatchString(plan)
}
if !indexed {
t.Errorf("%s: the lookup on %s does not use an index:\n%s", c.lookup, c.table, plan)
} else {
t.Logf("%s: %s", c.lookup, strings.ReplaceAll(strings.TrimSpace(plan), "\n", " | "))
}
}
}
func explainPlan(t *testing.T, s *Store, query string, args ...any) string {
t.Helper()
tx, err := s.db.Begin()
if err != nil {
t.Fatal(err)
}
defer func() { _ = tx.Rollback() }()
prefix := "EXPLAIN QUERY PLAN "
if s.dialect.Driver() == DriverPostgres {
if _, err := tx.Exec(`SET LOCAL enable_seqscan = off`); err != nil {
t.Fatal(err)
}
prefix = "EXPLAIN "
}
rows, err := tx.Query(s.q(prefix+query), args...)
if err != nil {
t.Fatalf("explain %q: %v", query, err)
}
defer rows.Close()
cols, _ := rows.Columns()
var lines []string
for rows.Next() {
vals := make([]any, len(cols))
ptrs := make([]any, len(cols))
for i := range vals {
ptrs[i] = &vals[i]
}
if err := rows.Scan(ptrs...); err != nil {
t.Fatal(err)
}
// The plan text is the last column on both backends.
switch v := vals[len(vals)-1].(type) {
case string:
lines = append(lines, v)
case []byte:
lines = append(lines, string(v))
}
}
return strings.Join(lines, "\n")
}
@@ -0,0 +1,9 @@
-- Migration 146: index api_tokens.token_hash (BUG-3668).
--
-- Every token-authenticated request (REST and MCP) looks its token up by
-- token_hash, and nothing indexed it, so each lookup scanned the table; an
-- absent hash, which every well-formed invalid token is, had to scan all of
-- it. A plain index, as sessions, password resets and email verification
-- tokens have: a UNIQUE one would fail this migration on a database that
-- already held a duplicate, and the lookup does not depend on uniqueness.
CREATE INDEX IF NOT EXISTS idx_api_tokens_token_hash ON api_tokens(token_hash);
@@ -0,0 +1,3 @@
-- Migration 116 (Postgres): index api_tokens.token_hash (BUG-3668).
-- Postgres counterpart to migrations/146_api_tokens_token_hash_index.sql; the rationale is there.
CREATE INDEX IF NOT EXISTS idx_api_tokens_token_hash ON api_tokens(token_hash);