From b9db178e1ed2ff600560b7fe85b98bcb90ba3cc6 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 14 Oct 2026 08:16:14 -0400 Subject: [PATCH] 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 --- .../bug3668_credential_lookup_index_test.go | 96 +++++++++++++++++++ .../146_api_tokens_token_hash_index.sql | 9 ++ .../116_api_tokens_token_hash_index.sql | 3 + 3 files changed, 108 insertions(+) create mode 100644 internal/store/bug3668_credential_lookup_index_test.go create mode 100644 internal/store/migrations/146_api_tokens_token_hash_index.sql create mode 100644 internal/store/pgmigrations/116_api_tokens_token_hash_index.sql diff --git a/internal/store/bug3668_credential_lookup_index_test.go b/internal/store/bug3668_credential_lookup_index_test.go new file mode 100644 index 000000000..6f07bbd14 --- /dev/null +++ b/internal/store/bug3668_credential_lookup_index_test.go @@ -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") +} diff --git a/internal/store/migrations/146_api_tokens_token_hash_index.sql b/internal/store/migrations/146_api_tokens_token_hash_index.sql new file mode 100644 index 000000000..647bc1861 --- /dev/null +++ b/internal/store/migrations/146_api_tokens_token_hash_index.sql @@ -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); diff --git a/internal/store/pgmigrations/116_api_tokens_token_hash_index.sql b/internal/store/pgmigrations/116_api_tokens_token_hash_index.sql new file mode 100644 index 000000000..bcd85cef7 --- /dev/null +++ b/internal/store/pgmigrations/116_api_tokens_token_hash_index.sql @@ -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);