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);