fix(hub): disambiguate supervisor auth failure reasons

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<VerifyApiKeyResult> {
   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 ───────────────────────────────────────────────────────────────

hub/src/ws/agent.ts

@@ -127,9 +127,34 @@ export async function handleAgentMessage(ws: ServerWebSocket<AgentWsData>, 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

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)
+  })
+})