From 51d3c2f8e59295012bbea115a4f7f1e68aacf92f Mon Sep 17 00:00:00 2001 From: Michael Date: Mon, 25 May 2026 22:13:31 -0700 Subject: [PATCH] fix(hub): disambiguate supervisor auth failure reasons verifyApiKeyWithCapability now returns a discriminated union ({ ok: true | false, reason: 'not_found' | 'revoked' | 'deleted' | 'missing_capability' }) instead of collapsing every failure to null. The supervisor WS auth handler logs the precise reason and closes with code 4001 + a meaningful reason text (api_key_not_found, api_key_revoked, missing_supervisor_capability) so prod auth bugs stop masquerading as silent connection failures. - hub/src/db/supervisor-dal.ts: VerifyApiKeyResult union, separate revoked + capability gates, structured log lines per branch - hub/src/ws/agent.ts: switch over verified.reason, distinct close reason strings, structured auth_error payload (adds .reason field) - hub/test/supervisor-auth-disambiguation.test.ts: REMO_E2E_DB_URL-gated coverage of all four runtime branches + always-on exhaustive-switch smoke test --- hub/src/db/supervisor-dal.ts | 55 ++++++-- hub/src/ws/agent.ts | 31 ++++- .../supervisor-auth-disambiguation.test.ts | 129 ++++++++++++++++++ 3 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 hub/test/supervisor-auth-disambiguation.test.ts diff --git a/hub/src/db/supervisor-dal.ts b/hub/src/db/supervisor-dal.ts index 53832ed..3d7e64c 100644 --- a/hub/src/db/supervisor-dal.ts +++ b/hub/src/db/supervisor-dal.ts @@ -2,24 +2,55 @@ import { sql } from './postgres.ts' // ── Supervisor capability ───────────────────────────────────────────────────── -export async function verifyApiKeyWithCapability(keyHash: string, capability: string) { +export type VerifyApiKeyResult = + | { ok: true; userId: string; apiKeyId: string } + | { ok: false; reason: 'not_found' } + | { ok: false; reason: 'revoked' } + | { ok: false; reason: 'deleted' } + | { ok: false; reason: 'missing_capability'; have: string[]; need: string } + +/** + * Disambiguated supervisor api-key verification. + * + * Returns a discriminated union so callers can log + send a precise reason + * rather than a generic "auth failed". The legacy single-query (revoked_at IS + * NULL) collapsed three distinct failure modes (not_found / revoked / missing + * capability) into one null — making prod auth bugs un-diagnosable. + * + * Note: the `api_keys` table does not currently have a `deleted_at` column. + * The `deleted` reason is reserved in the union for forward compatibility (the + * column may be added when soft-delete is introduced); for now it is never + * returned at runtime. + */ +export async function verifyApiKeyWithCapability( + keyHash: string, + capability: string, +): Promise { const rows = await sql` - SELECT id, user_id, capabilities + SELECT id, user_id, capabilities, revoked_at FROM api_keys - WHERE key_hash = ${keyHash} AND revoked_at IS NULL + WHERE key_hash = ${keyHash} LIMIT 1 ` if (!rows[0]) { - console.log(`[supervisor-dal] key not found for hash=${keyHash.slice(0,8)}...`) - return null + console.log(`[supervisor-dal] key not_found hash=${keyHash.slice(0,8)}...`) + return { ok: false, reason: 'not_found' } + } + const row = rows[0] + if (row.revoked_at) { + console.log(`[supervisor-dal] key revoked id=${row.id.slice(0,8)} revoked_at=${row.revoked_at}`) + return { ok: false, reason: 'revoked' } + } + const caps: string[] = Array.isArray(row.capabilities) ? row.capabilities : [] + // Empty/null caps = legacy keys, treated as all-caps. Non-empty caps must + // contain the requested capability. + if (caps.length > 0 && !caps.includes(capability)) { + console.log(`[supervisor-dal] key missing_capability id=${row.id.slice(0,8)} have=${JSON.stringify(caps)} need=${capability}`) + return { ok: false, reason: 'missing_capability', have: caps, need: capability } } - // Defense-in-depth: capability check is a soft gate. - // Treat unknown/empty caps as legacy (all caps granted). - // We can tighten this later if we ever issue limited-scope keys. - const raw = rows[0].capabilities - console.log(`[supervisor-dal] key found id=${rows[0].id.slice(0,8)} caps=${JSON.stringify(raw)}`) - await sql`UPDATE api_keys SET last_used_at = now() WHERE id = ${rows[0].id}` - return { userId: rows[0].user_id as string, apiKeyId: rows[0].id as string } + console.log(`[supervisor-dal] key ok id=${row.id.slice(0,8)} caps=${JSON.stringify(caps)}`) + await sql`UPDATE api_keys SET last_used_at = now() WHERE id = ${row.id}` + return { ok: true, userId: row.user_id as string, apiKeyId: row.id as string } } // ── Supervisors ─────────────────────────────────────────────────────────────── diff --git a/hub/src/ws/agent.ts b/hub/src/ws/agent.ts index 95fab78..5bed22a 100644 --- a/hub/src/ws/agent.ts +++ b/hub/src/ws/agent.ts @@ -127,9 +127,34 @@ export async function handleAgentMessage(ws: ServerWebSocket, raw: if (msg.role === 'supervisor') { const verified = await verifyApiKeyWithCapability(keyHash, 'supervisor') - if (!verified) { - ws.send(JSON.stringify({ type: 'auth_error', error: 'invalid api key or missing supervisor capability' })) - ws.close(4001, 'auth failed') + if (!verified.ok) { + // Map disambiguated reasons → close-code reason text + log line. + let closeReason: string + let errorMsg: string + switch (verified.reason) { + case 'not_found': + closeReason = 'api_key_not_found' + errorMsg = 'invalid api key' + console.warn(`[supervisor] auth fail reason=not_found hash=${keyHash.slice(0,8)}...`) + break + case 'revoked': + closeReason = 'api_key_revoked' + errorMsg = 'api key revoked' + console.warn(`[supervisor] auth fail reason=revoked hash=${keyHash.slice(0,8)}...`) + break + case 'deleted': + closeReason = 'api_key_not_found' + errorMsg = 'api key not found' + console.warn(`[supervisor] auth fail reason=deleted hash=${keyHash.slice(0,8)}...`) + break + case 'missing_capability': + closeReason = 'missing_supervisor_capability' + errorMsg = `missing capability: need=${verified.need} have=${JSON.stringify(verified.have)}` + console.warn(`[supervisor] auth fail reason=missing_capability need=${verified.need} have=${JSON.stringify(verified.have)}`) + break + } + ws.send(JSON.stringify({ type: 'auth_error', error: errorMsg, reason: verified.reason })) + ws.close(4001, closeReason) return } ws.data.authenticated = true diff --git a/hub/test/supervisor-auth-disambiguation.test.ts b/hub/test/supervisor-auth-disambiguation.test.ts new file mode 100644 index 0000000..5b8dbe9 --- /dev/null +++ b/hub/test/supervisor-auth-disambiguation.test.ts @@ -0,0 +1,129 @@ +/** + * Supervisor api-key auth disambiguation. + * + * Verifies verifyApiKeyWithCapability returns the four distinct failure + * shapes (not_found / revoked / missing_capability / ok). The `deleted` + * branch is reserved for forward compatibility (no `deleted_at` column on + * `api_keys` yet) and is exercised only at the type level. + * + * Gated on REMO_E2E_DB_URL — skips cleanly when unset, like the other DAL + * suites under hub/test/. + */ + +process.env.JWT_SECRET = process.env.JWT_SECRET || 'test-secret-at-least-32-chars-long-aaaaaaaa' +if (process.env.REMO_E2E_DB_URL) { + process.env.DATABASE_URL = process.env.REMO_E2E_DB_URL +} + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test' +import { createHash, randomBytes } from 'crypto' + +const HAS_TEST_DB = !!process.env.REMO_E2E_DB_URL +const maybe = HAS_TEST_DB ? describe : describe.skip + +const TEST_TAG = `sup-auth-${Date.now()}` + +function mkHash() { + // 64-hex sha256 of a random token — same shape as the hub computes. + return createHash('sha256').update(randomBytes(32)).digest('hex') +} + +maybe('verifyApiKeyWithCapability disambiguates failure reasons', () => { + let sql: any + let dal: typeof import('../src/db/supervisor-dal.ts') + let userId: string + + beforeAll(async () => { + const pg = await import('../src/db/postgres.ts') + sql = pg.sql + dal = await import('../src/db/supervisor-dal.ts') + const { runMigrations } = await import('../src/db/migrate.ts') + await runMigrations() + const rows = await sql` + INSERT INTO users (email, password_hash) + VALUES (${TEST_TAG + '@example.test'}, 'bcrypt-placeholder') + RETURNING id + ` + userId = rows[0].id as string + }) + + afterAll(async () => { + await sql`DELETE FROM api_keys WHERE user_id = ${userId}` + await sql`DELETE FROM users WHERE id = ${userId}` + }) + + test('not_found when no row matches keyhash', async () => { + const result = await dal.verifyApiKeyWithCapability(mkHash(), 'supervisor') + expect(result).toEqual({ ok: false, reason: 'not_found' }) + }) + + test('ok when key exists, not revoked, has capability', async () => { + const keyHash = mkHash() + await sql` + INSERT INTO api_keys (user_id, name, key_hash, capabilities) + VALUES (${userId}, 'happy', ${keyHash}, ARRAY['agent','supervisor']) + ` + const result = await dal.verifyApiKeyWithCapability(keyHash, 'supervisor') + expect(result.ok).toBe(true) + if (result.ok) { + expect(result.userId).toBe(userId) + expect(typeof result.apiKeyId).toBe('string') + } + }) + + test('revoked when revoked_at IS NOT NULL', async () => { + const keyHash = mkHash() + await sql` + INSERT INTO api_keys (user_id, name, key_hash, capabilities, revoked_at) + VALUES (${userId}, 'revoked', ${keyHash}, ARRAY['agent','supervisor'], now()) + ` + const result = await dal.verifyApiKeyWithCapability(keyHash, 'supervisor') + expect(result).toEqual({ ok: false, reason: 'revoked' }) + }) + + test('missing_capability when caps non-empty and need not present', async () => { + const keyHash = mkHash() + await sql` + INSERT INTO api_keys (user_id, name, key_hash, capabilities) + VALUES (${userId}, 'no-cap', ${keyHash}, ARRAY['agent']) + ` + const result = await dal.verifyApiKeyWithCapability(keyHash, 'supervisor') + expect(result.ok).toBe(false) + if (!result.ok && result.reason === 'missing_capability') { + expect(result.need).toBe('supervisor') + expect(result.have).toEqual(['agent']) + } else { + throw new Error(`expected missing_capability, got ${JSON.stringify(result)}`) + } + }) + + test('empty caps treated as legacy all-caps (ok)', async () => { + const keyHash = mkHash() + await sql` + INSERT INTO api_keys (user_id, name, key_hash, capabilities) + VALUES (${userId}, 'legacy', ${keyHash}, ARRAY[]::TEXT[]) + ` + const result = await dal.verifyApiKeyWithCapability(keyHash, 'supervisor') + expect(result.ok).toBe(true) + }) +}) + +// Always-on smoke test so the file passes even without REMO_E2E_DB_URL. +describe('supervisor-auth disambiguation harness', () => { + test('reason union shape is exhaustive at type level', async () => { + // Compile-time only: the type assertion below fails to typecheck if a new + // reason is added to VerifyApiKeyResult without updating this switch. + type R = import('../src/db/supervisor-dal.ts').VerifyApiKeyResult + const _check = (r: R): string => { + if (r.ok) return 'ok' + switch (r.reason) { + case 'not_found': return 'nf' + case 'revoked': return 'rv' + case 'deleted': return 'del' + case 'missing_capability': return 'mc' + } + } + void _check + expect(true).toBe(true) + }) +})