diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7f119cae..8fee5197 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -76,8 +76,10 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # Not `pnpm test -- `: pnpm forwards the literal `--`, so jest + # reads the flags as path patterns, matches nothing and exits 0 (D-0100). - name: Run Jest tests - run: pnpm test -- --coverage --ci --passWithNoTests + run: pnpm run test:ci - name: Upload coverage to Codecov uses: codecov/codecov-action@v4 diff --git a/jest.config.js b/jest.config.js index e7a6f8d9..e8109f39 100644 --- a/jest.config.js +++ b/jest.config.js @@ -64,6 +64,9 @@ const customJestConfig = { // Allow next/jest SWC to transpile ESM-only unified/rehype packages transformIgnorePatterns: [`/node_modules/(?!(${esmPackages.join('|')})/)`], testMatch: ['**/__tests__/**/*.test.ts?(x)', '**/*.test.ts?(x)'], + // Ticket worktrees live under .claude/worktrees; without this a run in the + // main checkout also runs every worktree's copy of the suite. + testPathIgnorePatterns: ['/.claude/'], collectCoverageFrom: [ 'src/**/*.{ts,tsx}', '!src/**/*.d.ts', diff --git a/src/lib/api-keys/README.md b/src/lib/api-keys/README.md index 82c3d413..871ea6ac 100644 --- a/src/lib/api-keys/README.md +++ b/src/lib/api-keys/README.md @@ -18,14 +18,18 @@ Real implementation of API key management with cryptographic security, database- ## API Key Format ``` -nself_ab_1234567890123456789012345678901234567890 -└─┬─┘ └┬┘ └────────────────┬───────────────────┘ - │ │ │ - │ │ └─ Secret (40 chars, base62) - │ └───────────────────── Random suffix (2 chars) - └────────────────────────── Prefix (nself) +nself_k3x7qa2mzp_123456789012345678901234567890 +└─┬─┘ └───┬────┘ └─────────────┬──────────────┘ + │ │ │ + │ │ └─ Secret (30 chars, base62) + │ └────────────────────── Lookup prefix (10 chars, base32 a-z2-7) + └────────────────────────────── Prefix (nself) ``` +The lookup prefix only narrows the search: validation checks the key against +every stored record with that prefix by bcrypt hash. Keys issued before the +10-char prefix carry a 2-letter suffix (`nself_ab_...`) and keep working. + ## Basic Usage ### Creating an API Key diff --git a/src/lib/api-keys/__tests__/api-keys.test.ts b/src/lib/api-keys/__tests__/api-keys.test.ts index fb3d51c9..ab1e2597 100644 --- a/src/lib/api-keys/__tests__/api-keys.test.ts +++ b/src/lib/api-keys/__tests__/api-keys.test.ts @@ -4,6 +4,8 @@ */ import { getDatabase, initDatabase } from '@/lib/database' +import type { ApiKey } from '@/types/api-key' +import bcrypt from 'bcryptjs' import * as apiKeys from '../index' // Ensure database is initialized before tests @@ -28,9 +30,10 @@ describe('API Key Generation', () => { const prefix1 = apiKeys.generateKeyPrefix() const prefix2 = apiKeys.generateKeyPrefix() - expect(prefix1).toMatch(/^nself_[a-z]{2}$/) - expect(prefix2).toMatch(/^nself_[a-z]{2}$/) - // Low probability of collision but technically possible + expect(prefix1).toMatch(/^nself_[a-z2-7]{10}$/) + expect(prefix2).toMatch(/^nself_[a-z2-7]{10}$/) + // 50 random bits: a collision here means the generator is broken + expect(prefix1).not.toBe(prefix2) }) test('creates cryptographically secure API keys', async () => { @@ -42,8 +45,8 @@ describe('API Key Generation', () => { expect(result.key).toBeDefined() expect(result.secretKey).toBeDefined() - expect(result.key.keyPrefix).toMatch(/^nself_[a-z]{2}$/) - expect(result.secretKey).toMatch(/^nself_[a-z]{2}_[A-Za-z0-9]{30}$/) + expect(result.key.keyPrefix).toMatch(/^nself_[a-z2-7]{10}$/) + expect(result.secretKey).toMatch(/^nself_[a-z2-7]{10}_[A-Za-z0-9]{30}$/) expect(result.key.keyHash).toBeDefined() expect(result.key.keyHash).toMatch(/^\$2[aby]\$/) // bcrypt hash pattern }) @@ -135,6 +138,91 @@ describe('API Key Validation', () => { }) }) +// D-0527: the prefix is only a lookup hint. Records that share it (legacy +// 2-letter prefixes, or any future collision) must each validate by hash. +describe('API Key Prefix Collisions', () => { + // Synthetic test-only secrets, never real keys + const FAKE_SECRET_A = 'FAKEtestSECRETnotArealKEYaaaaa' + const FAKE_SECRET_B = 'FAKEtestSECRETnotArealKEYbbbbb' + const FAKE_SECRET_C = 'FAKEtestSECRETnotArealKEYccccc' + + async function insertKeyWithSecret( + template: ApiKey, + id: string, + secretKey: string, + status: ApiKey['status'] = 'active' + ): Promise { + const keyPrefix = secretKey.split('_').slice(0, 2).join('_') + const keyHash = await bcrypt.hash(secretKey, 4) + // Copy only data fields; LokiJS metadata ($loki, meta) must not be reused + const { + $loki: _loki, + meta: _meta, + ...fields + } = template as ApiKey & { + $loki?: number + meta?: unknown + } + getDatabase() + ?.getCollection('apiKeys') + .insert({ ...fields, id, keyPrefix, keyHash, status }) + } + + test('two keys with a forced shared prefix both validate', async () => { + const created = await apiKeys.apiKeysApi.create({ name: 'Collision A', scope: 'read' }) + const prefix = created.key.keyPrefix + const secretB = `${prefix}_${FAKE_SECRET_B}` + await insertKeyWithSecret(created.key, 'key_collision_b', secretB) + + const collection = getDatabase()?.getCollection('apiKeys') + expect(collection?.find({ keyPrefix: prefix })).toHaveLength(2) + + const first = await apiKeys.validateApiKey(created.secretKey) + expect(first.valid).toBe(true) + expect(first.key?.id).toBe(created.key.id) + + const second = await apiKeys.validateApiKey(secretB) + expect(second.valid).toBe(true) + expect(second.key?.id).toBe('key_collision_b') + }) + + test('a revoked key sharing the prefix does not block an active one', async () => { + const created = await apiKeys.apiKeysApi.create({ name: 'Collision tmpl', scope: 'read' }) + const revokedSecret = `nself_ab_${FAKE_SECRET_A}` + const activeSecret = `nself_ab_${FAKE_SECRET_B}` + await insertKeyWithSecret(created.key, 'key_legacy_revoked', revokedSecret, 'revoked') + await insertKeyWithSecret(created.key, 'key_legacy_active', activeSecret) + + const active = await apiKeys.validateApiKey(activeSecret) + expect(active.valid).toBe(true) + expect(active.key?.id).toBe('key_legacy_active') + + const revoked = await apiKeys.validateApiKey(revokedSecret) + expect(revoked.valid).toBe(false) + expect(revoked.error).toBe('API key has been revoked') + }) + + test('still accepts an existing short-prefix (legacy) key', async () => { + const created = await apiKeys.apiKeysApi.create({ name: 'Legacy tmpl', scope: 'read' }) + const legacySecret = `nself_zq_${FAKE_SECRET_A}` + await insertKeyWithSecret(created.key, 'key_legacy_short', legacySecret) + + const result = await apiKeys.validateApiKey(legacySecret) + expect(result.valid).toBe(true) + expect(result.key?.id).toBe('key_legacy_short') + }) + + test('rejects a wrong secret under a shared prefix', async () => { + const created = await apiKeys.apiKeysApi.create({ name: 'Wrong tmpl', scope: 'read' }) + await insertKeyWithSecret(created.key, 'key_legacy_one', `nself_zq_${FAKE_SECRET_A}`) + await insertKeyWithSecret(created.key, 'key_legacy_two', `nself_zq_${FAKE_SECRET_B}`) + + const result = await apiKeys.validateApiKey(`nself_zq_${FAKE_SECRET_C}`) + expect(result.valid).toBe(false) + expect(result.error).toBe('Invalid API key') + }) +}) + describe('API Key Management', () => { test('retrieves all API keys', async () => { await apiKeys.apiKeysApi.create({ diff --git a/src/lib/api-keys/index.ts b/src/lib/api-keys/index.ts index b93a6019..c932597a 100644 --- a/src/lib/api-keys/index.ts +++ b/src/lib/api-keys/index.ts @@ -92,9 +92,8 @@ async function initApiKeyCollections(): Promise { /** * Generate a cryptographically secure API key * Format: prefix_secret - * - prefix: 8 chars for identification (nself_XX) - * - secret: 40 chars cryptographically random - * Total: ~48 characters + * - prefix: nself_ + 10 random base32 chars (see generateKeyPrefix) + * - secret: 30 chars cryptographically random (base62) */ function generateSecureApiKey(prefix: string): string { // Generate 30 bytes of random data (40 chars in base62) @@ -115,16 +114,21 @@ function generateSecureApiKey(prefix: string): string { return `${prefix}_${secret}` } +const KEY_PREFIX_LENGTH = 10 + /** - * Generate an 8-character prefix for API keys - * Format: nself_XX where XX are random lowercase letters + * Generate a near-unique lookup prefix for API keys + * Format: nself_ + 10 random lowercase base32 chars (a-z, 2-7), 50 bits. + * Keys created before this format carry nself_ + 2 letters; validateApiKey + * still accepts them because it matches every record with the prefix by hash. */ export function generateKeyPrefix(): string { - const chars = 'abcdefghijklmnopqrstuvwxyz' - const randomBytes = crypto.randomBytes(2) + const chars = 'abcdefghijklmnopqrstuvwxyz234567' + const randomBytes = crypto.randomBytes(KEY_PREFIX_LENGTH) let suffix = '' - for (let i = 0; i < 2; i++) { + for (let i = 0; i < KEY_PREFIX_LENGTH; i++) { + // 256 is a multiple of 32, so the modulo is unbiased suffix += chars[(randomBytes[i] ?? 0) % chars.length] } @@ -459,16 +463,22 @@ export async function validateApiKey( const prefix = `${parts[0]}_${parts[1]}` - // Find key by prefix - const apiKey = apiKeysCollection?.findOne({ keyPrefix: prefix }) + // The prefix is a lookup hint, not an identity: several records can share + // it (legacy 2-letter prefixes collide often), so match each by hash. + const candidates = apiKeysCollection?.find({ keyPrefix: prefix }) || [] - if (!apiKey) { + if (candidates.length === 0) { return { valid: false, error: 'API key not found' } } - // Verify the hash - const isValid = await verifyApiKey(key, apiKey.keyHash) - if (!isValid) { + let apiKey: ApiKey | undefined + for (const candidate of candidates) { + if (await verifyApiKey(key, candidate.keyHash)) { + apiKey = candidate + break + } + } + if (!apiKey) { return { valid: false, error: 'Invalid API key' } }