From 85a07d660a0beddfcdaceebecf54989228cde55f Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 24 Aug 2026 09:59:12 -0500 Subject: [PATCH] appview: give the O(n) bcrypt scan a guard that can actually fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestDeviceStore_ValidateDoesNotScanIndexedRows is documented as "the regression guard for the O(n) bcrypt scan", but it does not observe whether a scan happened. It asserts that every row is indexed and that an unknown secret errors, and both hold with or without the fix. Deleting the `WHERE secret_lookup IS NULL` filter from the fallback query — which is the defect 08121f3 removed — leaves it green. That matters more here than elsewhere in the batch. No backfill of the production table is possible (plaintext is not recoverable from bcrypt), so all 244 devices are legacy on day one and migrate lazily on first auth. A regression on this path locks out every existing user while new devices keep working, which is the failure mode least likely to show up in a smoke test. The scan is only observable in time, so the new guard makes one comparison expensive (bcrypt cost 13, ~300ms here) and asserts a deadline. Six seeded rows cost ~1.8s to scan and ~0ms to skip. Only one hash is generated: the others are copies with a mutated final byte, which bcrypt still runs the full key derivation over before rejecting, so setup stays at a single 300ms hash. Mutation-verified in a worktree by removing the filter: the old guard reports ok, the new one fails at 1.84s against a 400ms budget. Both halves are asserted — an unknown secret (the old code's worst case, where nothing matches and every row is compared) and a known indexed one — because only the first catches the missing filter and only the second catches the index being bypassed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh --- pkg/appview/db/device_store_scan_test.go | 118 +++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 pkg/appview/db/device_store_scan_test.go diff --git a/pkg/appview/db/device_store_scan_test.go b/pkg/appview/db/device_store_scan_test.go new file mode 100644 index 0000000..cba420d --- /dev/null +++ b/pkg/appview/db/device_store_scan_test.go @@ -0,0 +1,118 @@ +package db + +import ( + "fmt" + "testing" + "time" + + "golang.org/x/crypto/bcrypt" +) + +// scanGuardCost makes a single bcrypt comparison expensive enough to be visible +// in wall-clock time. The defect being guarded is O(n) bcrypt over the devices +// table, so the only way to observe it is to make each comparison cost +// something; at the production cost of 10 the difference between "scanned six +// rows" and "scanned none" is lost in test noise. +const scanGuardCost = 13 + +// seedIndexedDeviceRows inserts n devices that are already indexed +// (secret_lookup populated) and whose bcrypt hashes are deliberately expensive. +// +// Only one hash is generated: the rest are copies with a mutated final +// character. bcrypt reads its cost from the hash prefix and runs the full key +// derivation before comparing, so a mutated tail costs exactly as much as a +// genuine hash and then fails to match — which is what a scan over non-matching +// rows does anyway. +func seedIndexedDeviceRows(t *testing.T, store *DeviceStore, did, handle string, n int) { + t.Helper() + + base, err := bcrypt.GenerateFromPassword([]byte("filler-secret"), scanGuardCost) + if err != nil { + t.Fatalf("GenerateFromPassword: %v", err) + } + + for i := 0; i < n; i++ { + hash := append([]byte(nil), base...) + // Vary the tail so secret_hash stays UNIQUE, keeping the base64 alphabet. + hash[len(hash)-1] = byte('a' + (i % 26)) + if _, err := store.db.Exec(` + INSERT INTO devices (id, did, handle, name, secret_hash, secret_lookup, + ip_address, user_agent, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, datetime('now')) + `, + fmt.Sprintf("filler-device-%d", i), did, handle, fmt.Sprintf("Filler %d", i), + string(hash), deviceSecretLookup(fmt.Sprintf("atcr_device_filler_%d", i)), + "192.168.1.1", "Test Agent", + ); err != nil { + t.Fatalf("seed device %d: %v", i, err) + } + } +} + +// TestDeviceStore_IndexedRowsAreNeverBcryptCompared is the real regression guard +// for the O(n) bcrypt scan. +// +// Its sibling TestDeviceStore_ValidateDoesNotScanIndexedRows does not catch the +// defect: it asserts every row is indexed and that an unknown secret errors, +// and both of those hold whether or not the code scans. Removing the +// `WHERE secret_lookup IS NULL` filter from the fallback query leaves it green. +// +// What actually has to be true is that indexed rows are never handed to bcrypt. +// That is only observable in time, so the assertion is a deadline: six expensive +// rows cost ~1.8s to scan and ~0ms to skip. +func TestDeviceStore_IndexedRowsAreNeverBcryptCompared(t *testing.T) { + const fillerRows = 6 + + store := setupTestDB(t) + createTestUser(t, store, "did:plc:alice123", "alice.bsky.social") + seedIndexedDeviceRows(t, store, "did:plc:alice123", "alice.bsky.social", fillerRows) + + // A real device, indexed at creation like every post-0028 row. + secret := newDeviceForTest(t, store, "did:plc:alice123", "alice.bsky.social", "My Device") + + var unindexed int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM devices WHERE secret_lookup IS NULL OR secret_lookup = ''`, + ).Scan(&unindexed); err != nil { + t.Fatalf("count unindexed: %v", err) + } + if unindexed != 0 { + t.Fatalf("fixture is wrong: %d rows unindexed, so the scan has legitimate work to do", unindexed) + } + + // Anything above one expensive comparison means rows were scanned. The + // budget is generous against a ~1.8s full scan so this does not flake on a + // loaded machine. + const budget = 400 * time.Millisecond + + t.Run("unknown secret", func(t *testing.T) { + start := time.Now() + _, err := store.ValidateDeviceSecret("atcr_device_no_such_secret") + elapsed := time.Since(start) + + if err == nil { + t.Error("expected an error for an unknown secret") + } + // The worst case for the old code: no row ever matches, so it paid for + // every bcrypt comparison in the table before giving up. + if elapsed > budget { + t.Errorf("rejecting an unknown secret took %v (budget %v) — indexed rows are being bcrypt-compared, which is the O(n) scan this fix removed", elapsed, budget) + } + }) + + t.Run("known secret", func(t *testing.T) { + start := time.Now() + device, err := store.ValidateDeviceSecret(secret) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("ValidateDeviceSecret() error = %v", err) + } + if device.Name != "My Device" { + t.Errorf("Name = %v, want My Device", device.Name) + } + if elapsed > budget { + t.Errorf("resolving an indexed secret took %v (budget %v) — it is not using the index", elapsed, budget) + } + }) +}