diff --git a/specs/deploy/gmail-inbound-runbook.md b/specs/deploy/gmail-inbound-runbook.md index f0f2125..b0a15b3 100644 --- a/specs/deploy/gmail-inbound-runbook.md +++ b/specs/deploy/gmail-inbound-runbook.md @@ -180,8 +180,12 @@ privilege). 2. `PUBLIC_BASE_URL` = your production URL (e.g. `https://desk.resonantiq.app`), matching the OAuth redirect URI (A2.3) and the Pub/Sub push endpoint (A3.4). No trailing slash (the composition root strips one defensively either way). -3. Deploy. `vercel.json` (in the repo) declares the two Vercel Cron jobs: - - `*/1 * * * *` → `GET /api/v1/internal/queue/drain` (drain the job queue). +3. Deploy. `vercel.json` (in the repo) declares three Vercel Cron jobs: + - `*/1 * * * *` → `GET /api/v1/internal/queue/drain` (drain the job queue — + also delivers webhooks, HT-69: `WEBHOOK_DELIVERY_TOPIC` is handled here). + - `*/1 * * * *` → `GET /api/v1/internal/outbox/drain` (HT-69: turn + `event_outbox` rows into webhook-delivery queue jobs — a SEPARATE tick + from the queue drain above; that one then actually sends them). - `0 6 * * *` → `GET /api/v1/internal/cron/watch-maintenance` (daily renewal + sweep; UTC). Vercel Cron invokes these as HTTP GETs; the handlers require the `CRON_SECRET` (Vercel sends it as a bearer via the `Authorization` header on @@ -277,9 +281,10 @@ Authorization: Bearer $CRON_SECRET Answers **`200` when healthy, `503` when any alert is tripped** (body is the full JSON report either way: `ok`, `alerts[]`, and per-section detail — -queue stats, 24h ledger outcome counts, 24h forged-token aggregate, and -per-mailbox status + Gmail `watch()` expiry). Read-only and cheap — polling -every minute is fine. +queue stats, 24h ledger outcome counts, 24h forged-token aggregate, +per-mailbox status + Gmail `watch()` expiry, and — HT-69 — a `webhooks` +section: currently `auto_disabled` endpoints and 24h webhook-delivery +dead-letter count). Read-only and cheap — polling every minute is fine. **Wiring a monitor:** point any status-code poller that can send one custom header (UptimeRobot, Checkly, a `curl -fsS` in a cron you already own) at the @@ -300,6 +305,8 @@ Each `alerts[]` entry is `: `. The codes are stable: | `forged-token-burst` | ≥ threshold (default 5) stored deliveries in 24h carried reply tokens that FAILED signature verification — someone is guessing/tampering with threading tokens (threading.md §5) | Search Vercel logs for `forged_token_detected` (WARN); review `senderAddress`/`conversationId` across events. The mail itself threaded safely (a forged token never appends) | | `mailbox-needs-attention` | A mailbox is `paused` (cursor expired — gmail-push.md §5 rebaseline) or `needs_reconnect` (dead OAuth grant) — **inbound mail is not flowing** | `needs_reconnect`: re-run the Part E consent. `paused`: reconnect to rebaseline the cursor, then check for a gap | | `watch-expiring` | An active mailbox's Gmail `watch()` expires in < 72h (or was never armed) — the daily renewal has been failing for days | Function logs for `/internal/cron/watch-maintenance` (`gmail_watch_maintenance` events); a manual `GET` of that endpoint with the cron secret re-arms immediately | +| `webhook-endpoint-auto-disabled` | HT-69: a webhook endpoint hit 20 consecutive delivery failures and auto-disabled — a module (or an operator's own integration) has silently stopped receiving events | `SELECT id, url, consecutive_failures FROM webhook_endpoints WHERE status = 'auto_disabled'`; fix the receiving side, then `PATCH /api/v1/webhooks/{id}` with `{"status":"active"}` to re-enable (resets the counter) | +| `webhook-delivery-dead-letter-growth` | HT-69: a webhook delivery exhausted its retries in the last 24h (`WEBHOOK_DELIVERY_TOPIC` on `queue_jobs`) | `SELECT payload, last_error FROM queue_jobs WHERE topic = 'webhook.delivery' AND dead_lettered_at IS NOT NULL ORDER BY dead_lettered_at DESC` — `payload.endpointId` names the endpoint; this can precede (or accompany) an eventual auto-disable | ### G3. Structured log events (Vercel log search) @@ -308,7 +315,11 @@ ingest outcome: threading decision, append-fallback reason, forgedTokenCount, parse size, attachment count, ledger outcome), `forged_token_detected` (WARN — the per-message security event behind `forged-token-burst`), `queue_drain` (per drain tick that claimed work or fenced a stale worker: claimed/acked/ -retried/deadLettered/staleSkipped; quiet ticks don't log), `gmail_reconcile` +retried/deadLettered/staleSkipped; quiet ticks don't log — this is also +where webhook-delivery attempts surface, since `WEBHOOK_DELIVERY_TOPIC` is +handled by the SAME drain), `outbox_drain` (HT-69: per outbox-drain tick +that claimed at least one `event_outbox` row — claimed/enqueued/dispatched; +quiet ticks don't log, same convention as `queue_drain`), `gmail_reconcile` (per reconcile job: cursor positions, skip/retry/ack reasons), and `gmail_watch_maintenance` (the daily renewal + sweep). Correlate transport events to ingest events on `(mailboxId, providerMessageId)` diff --git a/src/api/agents.test.ts b/src/api/agents.test.ts index 9288e20..35bba15 100644 --- a/src/api/agents.test.ts +++ b/src/api/agents.test.ts @@ -8,6 +8,7 @@ * directly. */ +import { randomBytes } from 'node:crypto' import { afterEach, describe, expect, it } from 'vitest' import { mintInviteToken } from '../auth/invite-token.js' import { hashPassword } from '../auth/password-hash.js' @@ -19,8 +20,13 @@ import type { EmailSender, OutboundEmail } from '../providers/index.js' import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' import { createConversationStore } from '../store/conversations.js' import { createMailboxStore, type MailboxStore } from '../store/mailboxes.js' +import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' +import { createWebhookEndpointStore } from '../store/webhook-endpoints.js' import { createInboxApi } from './index.js' +/** None of this suite's tests exercise `/webhooks/*` — a real PGlite-backed store plus a no-op queue is just enough for `createInboxApi` to construct (HT-69's `webhooks` deps are now REQUIRED, mirroring `agents`). */ +const WEBHOOKS_ENC_KEY = randomBytes(ENCRYPTION_KEY_BYTES) + const TOKEN = 'test-token-for-the-agents-and-auth-suite' const MAIL_DOMAIN = 'mail.example.test' const SUPPORT_ADDRESS = 'support@example.test' @@ -86,6 +92,10 @@ describe('Agents & Authentication API', () => { mailboxStore, ...(overrides.uiBaseUrl !== undefined ? { uiBaseUrl: overrides.uiBaseUrl } : {}), }, + webhooks: { + store: createWebhookEndpointStore(db, WEBHOOKS_ENC_KEY), + queue: { async enqueue() {} }, + }, }) return { db, agentStore, mailboxStore, api, sent } } diff --git a/src/api/index.test.ts b/src/api/index.test.ts index af1d0fc..a1178cb 100644 --- a/src/api/index.test.ts +++ b/src/api/index.test.ts @@ -26,9 +26,11 @@ import { createGmailWatchStateStore } from '../store/gmail-watch-state.js' import { createMailboxTokenStore } from '../store/mailbox-tokens.js' import { createMailboxStore, type MailboxStore } from '../store/mailboxes.js' import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' +import { createWebhookEndpointStore } from '../store/webhook-endpoints.js' import type { AgentsApiDeps } from './agents.js' import type { GmailReconcileJob } from './gmail-webhook.js' import { createInboxApi, type InboxApiDeps } from './index.js' +import type { WebhooksApiDeps } from './webhooks.js' const TOKEN_ENC_KEY = randomBytes(ENCRYPTION_KEY_BYTES) @@ -56,6 +58,21 @@ function testAgentsDeps(db: Db): AgentsApiDeps { } } +/** + * Build the REQUIRED `webhooks` deps (HT-69) for a `createInboxApi` call + * wired to `db` — a real PGlite-backed `WebhookEndpointStore` plus a + * no-op `QueueProvider` (nothing in this suite exercises delivery; that is + * `src/webhooks/*.test.ts`'s and `src/api/webhooks.test.ts`'s job). Just + * enough for `createInboxApi` to construct and for the existing routes + * this suite covers to behave unchanged. + */ +function testWebhooksDeps(db: Db): WebhooksApiDeps { + return { + store: createWebhookEndpointStore(db, TOKEN_ENC_KEY), + queue: { async enqueue() {} }, + } +} + /** A fake `EmailSender` that records every `OutboundEmail` it's asked to send, never fails. */ function createFakeSender(): { sender: EmailSender; sent: OutboundEmail[] } { const sent: OutboundEmail[] = [] @@ -253,6 +270,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: agentsDeps, + webhooks: testWebhooksDeps(db), ...(overrides.openTracking !== undefined ? { openTracking: overrides.openTracking } : {}), ...(overrides.gmailPush !== undefined ? { gmailPush: overrides.gmailPush } : {}), ...(overrides.gmailConnect !== undefined ? { gmailConnect: overrides.gmailConnect } : {}), @@ -702,6 +720,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), }) const res = await api( @@ -809,6 +828,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), }) const res = await api( @@ -1014,6 +1034,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), }) const res = await api( @@ -1926,6 +1947,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), gmailPush: { verifySignature: async () => true, subscription: SUBSCRIPTION, @@ -1957,6 +1979,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), gmailPush: { verifySignature: async () => true, subscription: SUBSCRIPTION, @@ -2092,6 +2115,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), ...(gmailConnect !== undefined ? { gmailConnect } : {}), }) } @@ -2325,6 +2349,7 @@ describe('createInboxApi', () => { mailDomain: MAIL_DOMAIN, supportAddress: SUPPORT_ADDRESS, agents: testAgentsDeps(db), + webhooks: testWebhooksDeps(db), ...(gmailDisconnect !== undefined ? { gmailDisconnect } : {}), }) } @@ -2425,6 +2450,13 @@ describe('createInboxApi — hardening (Codex review)', () => { providers: [], mailboxStore: {} as unknown as MailboxStore, } satisfies AgentsApiDeps, + // Same "never invoked in this block" reasoning as `agents` above — these + // tests are purely about construction-time validation and the + // conversations-route error paths, never /webhooks/*. + webhooks: { + store: {} as unknown as WebhooksApiDeps['store'], + queue: {} as unknown as WebhooksApiDeps['queue'], + } satisfies WebhooksApiDeps, } it('throws at construction on an empty apiToken (fail closed — an empty token would authenticate every request)', () => { diff --git a/src/api/index.ts b/src/api/index.ts index 4f95a36..8fcb022 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -81,6 +81,14 @@ import { matchOpenTrackingPixel, matchRoute, } from './router.js' +import { + handleCreateWebhook, + handleDeleteWebhook, + handleListWebhooks, + handlePatchWebhook, + handleTestWebhook, + type WebhooksApiDeps, +} from './webhooks.js' /** * Minimum length for the service Bearer token. A short/empty token is a @@ -195,6 +203,14 @@ export interface InboxApiDeps { * for the conceded race. */ selfEchoGuard?: SelfEchoGuardDeps + /** + * The webhooks admin API (HT-69; specs/modules/substrate-v1.md §5) — + * REQUIRED, like `agents`: this is core substrate ("free forever", spec + * §1), not a deployment-specific optional feature like `openTracking`/ + * `gmailPush`. See `src/api/webhooks.ts`'s module doc for the full + * `POST`/`GET`/`PATCH`/`DELETE`/`.../test` surface this wires up. + */ + webhooks: WebhooksApiDeps } /** @@ -494,6 +510,43 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis request, deps.agents, ) + + // --- Webhooks admin API (HT-69; specs/modules/substrate-v1.md §5) --- + + case 'webhooks-list': + return await handleListWebhooks( + await resolveActingAgent(request, deps.agents.store), + deps.webhooks, + ) + + case 'webhooks-create': + return await handleCreateWebhook( + await resolveActingAgent(request, deps.agents.store), + request, + deps.webhooks, + ) + + case 'webhook-patch': + return await handlePatchWebhook( + route.id, + await resolveActingAgent(request, deps.agents.store), + request, + deps.webhooks, + ) + + case 'webhook-delete': + return await handleDeleteWebhook( + route.id, + await resolveActingAgent(request, deps.agents.store), + deps.webhooks, + ) + + case 'webhook-test': + return await handleTestWebhook( + route.id, + await resolveActingAgent(request, deps.agents.store), + deps.webhooks, + ) } } catch (err) { console.error('[inbox-api] unhandled error handling request', err) diff --git a/src/api/router.ts b/src/api/router.ts index 14647f7..93debff 100644 --- a/src/api/router.ts +++ b/src/api/router.ts @@ -164,6 +164,30 @@ const AGENT_MAILBOXES: RouteDef = { methods: ['GET', 'PUT'], } +// --- Webhooks admin API (HT-69; specs/modules/substrate-v1.md §5) ----------- +// +// Admin-only, acting-Agent header REQUIRED on every route (`src/api/ +// webhooks.ts`'s module doc) — same Bearer-gated-ordinary-route shape as +// Agents & Authentication above, no pre-auth carve-out. + +/** `/api/v1/webhooks` — list (GET) and register (POST), both admin only — spec §5. */ +const WEBHOOKS_LIST: RouteDef = { + pattern: /^\/api\/v1\/webhooks$/, + methods: ['GET', 'POST'], +} + +/** `/api/v1/webhooks/{id}` — patch/delete (admin only) — spec §5. Anchored `[^/]+$` so it never matches a `.../test` suffix, mirroring `AGENT_ITEM`'s own anchoring against `.../password`/`.../invite`/`.../mailboxes`. */ +const WEBHOOK_ITEM: RouteDef = { + pattern: /^\/api\/v1\/webhooks\/(?[^/]+)$/, + methods: ['PATCH', 'DELETE'], +} + +/** `/api/v1/webhooks/{id}/test` — fire a synthetic `test.ping` through the real delivery path (admin only) — spec §5, POST only. */ +const WEBHOOK_TEST: RouteDef = { + pattern: /^\/api\/v1\/webhooks\/(?[^/]+)\/test$/, + methods: ['POST'], +} + /** Every route this API recognizes, checked in order. */ const ROUTES: readonly RouteDef[] = [ CONVERSATIONS_LIST, @@ -185,6 +209,9 @@ const ROUTES: readonly RouteDef[] = [ MAILBOXES_LIST, AGENT_MAILBOXES, AGENT_ITEM, + WEBHOOKS_LIST, + WEBHOOK_TEST, + WEBHOOK_ITEM, ] /** The outcome of matching a `(method, pathname)` pair against {@link ROUTES}. */ @@ -214,6 +241,11 @@ export type RouteMatch = | { kind: 'mailboxes-list' } | { kind: 'agent-mailboxes-get'; id: string } | { kind: 'agent-mailboxes-put'; id: string } + | { kind: 'webhooks-list' } + | { kind: 'webhooks-create' } + | { kind: 'webhook-patch'; id: string } + | { kind: 'webhook-delete'; id: string } + | { kind: 'webhook-test'; id: string } | { kind: 'method-not-allowed'; allow: string[] } | { kind: 'not-found' } @@ -339,6 +371,9 @@ export function matchRoute(method: string, pathname: string): RouteMatch { if (route === MAILBOXES_LIST) { return { kind: 'mailboxes-list' } } + if (route === WEBHOOKS_LIST) { + return method === 'GET' ? { kind: 'webhooks-list' } : { kind: 'webhooks-create' } + } // Every remaining route guarantees a present, non-empty `id` group (per // its `[^/]+` pattern) whenever it matched. @@ -366,6 +401,13 @@ export function matchRoute(method: string, pathname: string): RouteMatch { if (method === 'GET') return { kind: 'agent-mailboxes-get', id } return { kind: 'agent-mailboxes-put', id } } + if (route === WEBHOOK_TEST) { + return { kind: 'webhook-test', id } + } + if (route === WEBHOOK_ITEM) { + if (method === 'DELETE') return { kind: 'webhook-delete', id } + return { kind: 'webhook-patch', id } + } if (route === AGENT_ITEM) { if (method === 'GET') return { kind: 'agent-item', id } if (method === 'DELETE') return { kind: 'agent-delete', id } diff --git a/src/api/webhooks.test.ts b/src/api/webhooks.test.ts new file mode 100644 index 0000000..2805fe0 --- /dev/null +++ b/src/api/webhooks.test.ts @@ -0,0 +1,381 @@ +/** + * End-to-end tests for the webhooks admin API (HT-69; specs/modules/ + * substrate-v1.md §5) — driven through the real `createInboxApi` pipeline, + * a real PGlite-backed `AgentStore` + `WebhookEndpointStore`, and a fake + * `QueueProvider` (nothing here exercises real delivery — that is + * `src/webhooks/delivery.test.ts`'s job), matching `src/api/agents.test.ts`'s + * convention of testing API handlers via the full HTTP pipeline. + */ + +import { randomBytes } from 'node:crypto' +import { afterEach, describe, expect, it } from 'vitest' +import { hashPassword } from '../auth/password-hash.js' +import { createPasswordAuthProvider } from '../auth/password-provider.js' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { Keyring } from '../mail/reply-token.js' +import type { EmailSender, EnqueueOptions, QueueProvider } from '../providers/index.js' +import { type AgentRecord, type AgentStore, createAgentStore } from '../store/agents.js' +import { createConversationStore } from '../store/conversations.js' +import { createMailboxStore } from '../store/mailboxes.js' +import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' +import { + createWebhookEndpointStore, + type WebhookEndpointStore, +} from '../store/webhook-endpoints.js' +import { WEBHOOK_DELIVERY_TOPIC } from '../webhooks/delivery.js' +import { createInboxApi } from './index.js' + +const TOKEN = 'test-token-for-the-webhooks-admin-suite' +const MAIL_DOMAIN = 'mail.example.test' +const SUPPORT_ADDRESS = 'support@example.test' +const KEYRING: Keyring = { current: { keyId: 'k1', secret: 'a'.repeat(32) } } +const AGENT_HEADER = 'X-Helpthread-Agent-Id' +const WEBHOOKS_ENC_KEY = randomBytes(ENCRYPTION_KEY_BYTES) + +function createFakeSender(): EmailSender { + return { + maxSendMs: 30_000, + async send() { + return {} + }, + } +} + +function fakeQueue(): { + queue: QueueProvider + enqueued: { topic: string; payload: unknown; opts?: EnqueueOptions }[] +} { + const enqueued: { topic: string; payload: unknown; opts?: EnqueueOptions }[] = [] + return { + queue: { + async enqueue(topic, payload, opts) { + enqueued.push({ topic, payload, opts }) + }, + }, + enqueued, + } +} + +describe('Webhooks admin API', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshApi(): Promise<{ + db: Db + agentStore: AgentStore + webhookStore: WebhookEndpointStore + api: (request: Request) => Promise + enqueued: { topic: string; payload: unknown; opts?: EnqueueOptions }[] + }> { + db = await createPgliteDb() + await migrate(db) + const agentStore = createAgentStore(db) + const webhookStore = createWebhookEndpointStore(db, WEBHOOKS_ENC_KEY) + const mailboxStore = createMailboxStore(db) + const { queue, enqueued } = fakeQueue() + const api = createInboxApi({ + store: createConversationStore(db), + apiToken: TOKEN, + sender: createFakeSender(), + keyring: KEYRING, + mailDomain: MAIL_DOMAIN, + supportAddress: SUPPORT_ADDRESS, + agents: { + store: agentStore, + providers: [createPasswordAuthProvider({ agentStore })], + mailboxStore, + }, + webhooks: { store: webhookStore, queue }, + }) + return { db, agentStore, webhookStore, api, enqueued } + } + + function req( + method: string, + path: string, + opts: { agentId?: string; body?: unknown } = {}, + ): Request { + const headers: Record = { Authorization: `Bearer ${TOKEN}` } + if (opts.agentId !== undefined) headers[AGENT_HEADER] = opts.agentId + const init: RequestInit = { method, headers } + if (opts.body !== undefined) { + headers['Content-Type'] = 'application/json' + init.body = JSON.stringify(opts.body) + } + return new Request(`https://x.example.test${path}`, init) + } + + async function createAgent( + agentStore: AgentStore, + role: 'admin' | 'agent', + email: string, + ): Promise { + const result = await agentStore.createAgent({ + name: 'Test Agent', + email, + role, + status: 'active', + passwordHash: hashPassword('correct-horse-battery'), + }) + if (!result.ok) throw new Error('expected ok') + return result.agent + } + + // --- authz: every route requires an admin acting-Agent ---------------------- + + describe('authz', () => { + it('401s with no acting-Agent header', async () => { + const { api } = await freshApi() + expect((await api(req('GET', '/api/v1/webhooks'))).status).toBe(401) + expect((await api(req('POST', '/api/v1/webhooks', { body: {} }))).status).toBe(401) + }) + + it('403s for a non-admin acting Agent', async () => { + const { api, agentStore } = await freshApi() + const agent = await createAgent(agentStore, 'agent', 'nonadmin@example.test') + const res = await api(req('GET', '/api/v1/webhooks', { agentId: agent.id })) + expect(res.status).toBe(403) + expect(await res.json()).toEqual({ + error: { code: 'forbidden', message: expect.any(String) }, + }) + }) + }) + + // --- POST/GET /api/v1/webhooks ----------------------------------------------- + + describe('POST /webhooks', () => { + it('creates an endpoint, returns the secret ONCE, and the secret never reappears on GET', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + + const created = await api( + req('POST', '/api/v1/webhooks', { + agentId: admin.id, + body: { url: 'https://hooks.example.test/receive', events: ['conversation.created'] }, + }), + ) + expect(created.status).toBe(201) + const createdBody = (await created.json()) as { webhook: Record } + expect(createdBody.webhook.secret).toEqual(expect.any(String)) + expect((createdBody.webhook.secret as string).length).toBeGreaterThan(20) + expect(createdBody.webhook.status).toBe('active') + expect(createdBody.webhook.consecutiveFailures).toBe(0) + const id = createdBody.webhook.id as string + + const listed = await api(req('GET', '/api/v1/webhooks', { agentId: admin.id })) + expect(listed.status).toBe(200) + const listedBody = (await listed.json()) as { webhooks: Record[] } + expect(listedBody.webhooks).toHaveLength(1) + expect(listedBody.webhooks[0].id).toBe(id) + expect(listedBody.webhooks[0]).not.toHaveProperty('secret') + }) + + it('events omitted defaults to [] (all events, spec §5)', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api( + req('POST', '/api/v1/webhooks', { + agentId: admin.id, + body: { url: 'https://hooks.example.test/receive' }, + }), + ) + expect(res.status).toBe(201) + const body = (await res.json()) as { webhook: { events: string[] } } + expect(body.webhook.events).toEqual([]) + }) + + it('400s on a non-https url', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api( + req('POST', '/api/v1/webhooks', { + agentId: admin.id, + body: { url: 'http://insecure.example.test/hook' }, + }), + ) + expect(res.status).toBe(400) + expect(await res.json()).toEqual({ + error: { code: 'validation_failed', message: expect.any(String) }, + }) + }) + + it('400s on an unknown event type', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api( + req('POST', '/api/v1/webhooks', { + agentId: admin.id, + body: { url: 'https://hooks.example.test/receive', events: ['not.a.real.event'] }, + }), + ) + expect(res.status).toBe(400) + }) + + it('400s on a missing url', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api(req('POST', '/api/v1/webhooks', { agentId: admin.id, body: {} })) + expect(res.status).toBe(400) + }) + }) + + // --- PATCH/DELETE /api/v1/webhooks/{id} --------------------------------------- + + describe('PATCH /webhooks/{id}', () => { + async function createEndpoint( + api: (r: Request) => Promise, + adminId: string, + ): Promise { + const res = await api( + req('POST', '/api/v1/webhooks', { + agentId: adminId, + body: { url: 'https://hooks.example.test/receive' }, + }), + ) + const body = (await res.json()) as { webhook: { id: string } } + return body.webhook.id + } + + it('updates url/events/module/status and returns the updated row (never the secret)', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const id = await createEndpoint(api, admin.id) + + const res = await api( + req('PATCH', `/api/v1/webhooks/${id}`, { + agentId: admin.id, + body: { + url: 'https://hooks.example.test/new-receiver', + events: ['conversation.reply_sent'], + module: 'draft-reply', + status: 'disabled', + }, + }), + ) + expect(res.status).toBe(200) + const body = (await res.json()) as { webhook: Record } + expect(body.webhook).not.toHaveProperty('secret') + expect(body.webhook).toMatchObject({ + url: 'https://hooks.example.test/new-receiver', + events: ['conversation.reply_sent'], + module: 'draft-reply', + status: 'disabled', + }) + }) + + it('refuses status: auto_disabled — engine-managed only, never admin-settable', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const id = await createEndpoint(api, admin.id) + + const res = await api( + req('PATCH', `/api/v1/webhooks/${id}`, { + agentId: admin.id, + body: { status: 'auto_disabled' }, + }), + ) + expect(res.status).toBe(400) + }) + + it('404s for an unknown id', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api( + req('PATCH', '/api/v1/webhooks/00000000-0000-4000-8000-000000000000', { + agentId: admin.id, + body: { status: 'disabled' }, + }), + ) + expect(res.status).toBe(404) + }) + }) + + describe('DELETE /webhooks/{id}', () => { + it('hard-deletes and returns 204; a second delete 404s', async () => { + const { api, agentStore, webhookStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const created = await webhookStore.create({ + url: 'https://hooks.example.test/hook', + secret: 's', + events: [], + }) + + const first = await api( + req('DELETE', `/api/v1/webhooks/${created.id}`, { agentId: admin.id }), + ) + expect(first.status).toBe(204) + + const second = await api( + req('DELETE', `/api/v1/webhooks/${created.id}`, { agentId: admin.id }), + ) + expect(second.status).toBe(404) + + expect(await webhookStore.list()).toEqual([]) + }) + }) + + // --- POST /api/v1/webhooks/{id}/test ------------------------------------------ + + describe('POST /webhooks/{id}/test', () => { + it('enqueues a test.ping through the real delivery topic, addressed to exactly this endpoint', async () => { + const { api, agentStore, webhookStore, enqueued } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const created = await webhookStore.create({ + url: 'https://hooks.example.test/hook', + secret: 's', + events: ['conversation.reply_sent'], // test.ping is NOT in this filter — must still fire + }) + + const res = await api( + req('POST', `/api/v1/webhooks/${created.id}/test`, { agentId: admin.id }), + ) + + expect(res.status).toBe(202) + expect(enqueued).toHaveLength(1) + expect(enqueued[0].topic).toBe(WEBHOOK_DELIVERY_TOPIC) + const payload = enqueued[0].payload as { + endpointId: string + type: string + conversationId: unknown + } + expect(payload.endpointId).toBe(created.id) + expect(payload.type).toBe('test.ping') + expect(payload.conversationId).toBeNull() + expect(enqueued[0].opts?.dedupeKey).toContain(created.id) + }) + + it('409s against a disabled endpoint — re-enable it first', async () => { + const { api, agentStore, webhookStore, enqueued } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const created = await webhookStore.create({ + url: 'https://hooks.example.test/hook', + secret: 's', + events: [], + }) + await webhookStore.patch(created.id, { status: 'disabled' }) + + const res = await api( + req('POST', `/api/v1/webhooks/${created.id}/test`, { agentId: admin.id }), + ) + + expect(res.status).toBe(409) + expect(enqueued).toHaveLength(0) + }) + + it('404s for an unknown id', async () => { + const { api, agentStore } = await freshApi() + const admin = await createAgent(agentStore, 'admin', 'admin@example.test') + const res = await api( + req('POST', '/api/v1/webhooks/00000000-0000-4000-8000-000000000000/test', { + agentId: admin.id, + }), + ) + expect(res.status).toBe(404) + }) + }) +}) diff --git a/src/api/webhooks.ts b/src/api/webhooks.ts new file mode 100644 index 0000000..5c923bd --- /dev/null +++ b/src/api/webhooks.ts @@ -0,0 +1,351 @@ +/** + * The webhook admin API (HT-69; specs/modules/substrate-v1.md §5) — + * `POST`/`GET /api/v1/webhooks`, `PATCH`/`DELETE /api/v1/webhooks/{id}`, + * `POST /api/v1/webhooks/{id}/test`. + * + * Same shape and conventions as `src/api/agents.ts` (this ticket's exact + * brief): each handler is a pure function of an already-authenticated + * (service Bearer), already-routed `Request` plus its dependencies — + * `src/api/index.ts` authenticates and routes; nothing here re-checks + * either. Every response goes through `src/api/responses.ts`'s helpers. + * Admin-only, acting-Agent header REQUIRED on every route — mirroring + * `agents.ts`'s mailbox-access endpoints (a security-sensitive admin + * surface with no self-service carve-out), not the Agent-roster endpoints + * (which allow any active Agent). + * + * ## Secret handling + * + * A fresh secret is server-generated on `POST` (never accepted from the + * caller — spec §5: "server-generated") and returned EXACTLY ONCE, in the + * `201` response body — `WebhookEndpointStore.list`'s rows (and hence + * every OTHER response this module ever sends) never carry it, mirroring + * `agents.ts`'s "never a secret_hash, a password, or a token anywhere in a + * response body" discipline for everything except that one creation + * response. + * + * ## `POST .../test` requires an ACTIVE endpoint + * + * Refused (`409 conflict`) against a `disabled`/`auto_disabled` endpoint — + * re-enable it first (`PATCH .../{id}` with `status: 'active'`). This + * keeps the invariant "only active endpoints ever receive a delivery" + * true by construction at the enqueue boundary; `src/webhooks/delivery.ts`'s + * handler ALSO re-checks status at send time (defense against the race + * where an endpoint is disabled between this enqueue and the delivery + * attempt), but this refusal is the primary gate and the honest response + * to an operator who tried to test a disabled endpoint, rather than a + * silent no-op. + */ + +import { randomBytes, randomUUID } from 'node:crypto' +import type { QueueProvider } from '../providers/queue.js' +import type { AgentRecord } from '../store/agents.js' +import type { + CreatedWebhookEndpoint, + StoredWebhookEndpoint, + WebhookEndpointStore, +} from '../store/webhook-endpoints.js' +import { WEBHOOK_DELIVERY_TOPIC, type WebhookDeliveryJob } from '../webhooks/delivery.js' +import { isEventType, TEST_PING_EVENT_TYPE } from '../webhooks/event-types.js' +import { apiError, json, noContent } from './responses.js' +import { isUuid } from './uuid.js' + +/** Dependencies every handler in this module needs. */ +export interface WebhooksApiDeps { + store: WebhookEndpointStore + queue: QueueProvider +} + +/** Length (bytes, before base64url encoding) of a freshly generated webhook secret — 256 bits, matching this codebase's other high-entropy secret sizes (`src/auth/invite-token.ts`'s `NONCE_BYTES`-scale reasoning: enough that guessing is infeasible, no further tuning needed). */ +const SECRET_BYTES = 32 + +/** `https://` only, and a sane upper bound so a pathological value can't bloat the row or a log line — matching migration 022's own `LIKE 'https://%'` CHECK, re-validated at the API layer for a clean `400` instead of a raw constraint-violation `500`. */ +const MAX_URL_LENGTH = 2048 + +function generateSecret(): string { + return randomBytes(SECRET_BYTES).toString('base64url') +} + +async function parseJsonBody( + request: Request, +): Promise<{ ok: true; value: unknown } | { ok: false }> { + try { + return { ok: true, value: await request.json() } + } catch { + return { ok: false } + } +} + +function asRecord(value: unknown): Record | null { + return typeof value === 'object' && value !== null ? (value as Record) : null +} + +/** `https://...`, ≤ {@link MAX_URL_LENGTH} chars, and a syntactically valid URL. `null` on any violation. */ +function validateUrl(raw: unknown): string | null { + if (typeof raw !== 'string' || raw.length === 0 || raw.length > MAX_URL_LENGTH) return null + if (!raw.startsWith('https://')) return null + try { + // Rejects anything `new URL` itself can't parse (e.g. no host at all); + // does NOT resolve DNS or apply the SSRF/private-range check — that is + // `src/webhooks/ssrf.ts`'s job, applied at DELIVERY time (spec §5's + // "resolve-then-connect pinning" — a hostname's resolved address can + // change after registration, so checking it once here would be a stale + // guarantee, not a real one). + new URL(raw) + return raw + } catch { + return null + } +} + +/** `body.events` must be an array of known {@link isEventType} strings, or omitted (defaults to `[]`, spec §5's "or all"). `null` on any violation. */ +function validateEvents(raw: unknown): string[] | null { + if (raw === undefined) return [] + if (!Array.isArray(raw)) return null + const events: string[] = [] + for (const entry of raw) { + if (typeof entry !== 'string' || !isEventType(entry)) return null + events.push(entry) + } + return events +} + +/** `body.module`, if present, must be a non-empty string. `undefined` (field absent) is distinct from `null` (explicit clear) — both legal; only a present-but-wrong-typed value is a violation (`'invalid'`). */ +function validateModule( + raw: unknown, +): { ok: true; value: string | null | undefined } | { ok: false } { + if (raw === undefined) return { ok: true, value: undefined } + if (raw === null) return { ok: true, value: null } + if (typeof raw === 'string' && raw.trim().length > 0) return { ok: true, value: raw } + return { ok: false } +} + +// --- wire shape -------------------------------------------------------------- + +interface WebhookJson { + id: string + url: string + events: string[] + module: string | null + status: StoredWebhookEndpoint['status'] + consecutiveFailures: number + createdAt: string + updatedAt: string +} + +function toWebhookJson(endpoint: StoredWebhookEndpoint): WebhookJson { + return { + id: endpoint.id, + url: endpoint.url, + events: endpoint.events, + module: endpoint.module, + status: endpoint.status, + consecutiveFailures: endpoint.consecutiveFailures, + createdAt: endpoint.createdAt.toISOString(), + updatedAt: endpoint.updatedAt.toISOString(), + } +} + +/** {@link toWebhookJson} plus the plaintext secret — `POST`'s response ONLY (module doc). */ +function toCreatedWebhookJson(endpoint: CreatedWebhookEndpoint): WebhookJson & { secret: string } { + return { ...toWebhookJson(endpoint), secret: endpoint.secret } +} + +const UNAUTHORIZED = () => apiError(401, 'unauthorized', 'Missing or invalid Agent identity.') +const FORBIDDEN = () => apiError(403, 'forbidden', 'Admin role required.') +const NOT_FOUND = () => apiError(404, 'not_found', 'No webhook endpoint with that id.') + +/** The one authz check every handler below starts with: acting Agent present AND admin. Returns the error `Response` to short-circuit with, or `null` if the caller may proceed. */ +function requireAdmin(actingAgent: AgentRecord | null): Response | null { + if (actingAgent === null) return UNAUTHORIZED() + if (actingAgent.role !== 'admin') return FORBIDDEN() + return null +} + +// --- GET/POST /api/v1/webhooks ------------------------------------------------ + +/** `GET /api/v1/webhooks` — admin only. The full roster, never including any secret. */ +export async function handleListWebhooks( + actingAgent: AgentRecord | null, + deps: Pick, +): Promise { + const denied = requireAdmin(actingAgent) + if (denied !== null) return denied + + const webhooks = await deps.store.list() + return json(200, { webhooks: webhooks.map(toWebhookJson) }) +} + +/** `POST /api/v1/webhooks` — admin only. `{ url, events?, module? }`. Returns the secret ONCE (module doc). */ +export async function handleCreateWebhook( + actingAgent: AgentRecord | null, + request: Request, + deps: Pick, +): Promise { + const denied = requireAdmin(actingAgent) + if (denied !== null) return denied + + const parsed = await parseJsonBody(request) + if (!parsed.ok) return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + const body = asRecord(parsed.value) + if (body === null) + return apiError(400, 'validation_failed', 'Request body must be a JSON object.') + + const url = validateUrl(body.url) + if (url === null) { + return apiError( + 400, + 'validation_failed', + `url is required, must start with https://, and be at most ${MAX_URL_LENGTH} characters.`, + ) + } + const events = validateEvents(body.events) + if (events === null) { + return apiError( + 400, + 'validation_failed', + 'events must be an array of known event type strings, or omitted for all events.', + ) + } + const module = validateModule(body.module) + if (!module.ok) { + return apiError(400, 'validation_failed', 'module must be a non-empty string or null.') + } + + const created = await deps.store.create({ + url, + secret: generateSecret(), + events, + ...(module.value !== undefined ? { module: module.value } : {}), + }) + return json(201, { webhook: toCreatedWebhookJson(created) }) +} + +// --- PATCH/DELETE /api/v1/webhooks/{id} --------------------------------------- + +const PATCHABLE_FIELDS = ['url', 'events', 'module', 'status'] + +/** `PATCH /api/v1/webhooks/{id}` — admin only. `status` may only be set to `'active'`/`'disabled'` — `'auto_disabled'` is engine-written only (`WebhookEndpointStore`'s own module doc), never admin-settable directly. */ +export async function handlePatchWebhook( + id: string, + actingAgent: AgentRecord | null, + request: Request, + deps: Pick, +): Promise { + const denied = requireAdmin(actingAgent) + if (denied !== null) return denied + if (!isUuid(id)) return NOT_FOUND() + + const parsed = await parseJsonBody(request) + if (!parsed.ok) return apiError(400, 'validation_failed', 'Request body must be valid JSON.') + const body = asRecord(parsed.value) + if (body === null) + return apiError(400, 'validation_failed', 'Request body must be a JSON object.') + + for (const key of Object.keys(body)) { + if (!PATCHABLE_FIELDS.includes(key)) { + return apiError(400, 'validation_failed', `Unknown field '${key}'.`) + } + } + + const patch: { + url?: string + events?: string[] + module?: string | null + status?: 'active' | 'disabled' + } = {} + if ('url' in body) { + const url = validateUrl(body.url) + if (url === null) { + return apiError( + 400, + 'validation_failed', + `url must start with https:// and be at most ${MAX_URL_LENGTH} characters.`, + ) + } + patch.url = url + } + if ('events' in body) { + const events = validateEvents(body.events) + if (events === null) { + return apiError( + 400, + 'validation_failed', + 'events must be an array of known event type strings.', + ) + } + patch.events = events + } + if ('module' in body) { + const module = validateModule(body.module) + if (!module.ok) { + return apiError(400, 'validation_failed', 'module must be a non-empty string or null.') + } + patch.module = module.value ?? null + } + if ('status' in body) { + if (body.status !== 'active' && body.status !== 'disabled') { + return apiError( + 400, + 'validation_failed', + "status must be 'active' or 'disabled' (auto_disabled is engine-managed).", + ) + } + patch.status = body.status + } + + const updated = await deps.store.patch(id, patch) + if (updated === null) return NOT_FOUND() + return json(200, { webhook: toWebhookJson(updated) }) +} + +/** `DELETE /api/v1/webhooks/{id}` — admin only, hard delete. */ +export async function handleDeleteWebhook( + id: string, + actingAgent: AgentRecord | null, + deps: Pick, +): Promise { + const denied = requireAdmin(actingAgent) + if (denied !== null) return denied + if (!isUuid(id)) return NOT_FOUND() + + const deleted = await deps.store.delete(id) + if (!deleted) return NOT_FOUND() + return noContent() +} + +// --- POST /api/v1/webhooks/{id}/test ------------------------------------------ + +/** `POST /api/v1/webhooks/{id}/test` — admin only. Fires a synthetic `test.ping` through the real delivery queue (`src/webhooks/delivery.ts`), addressed to exactly this endpoint regardless of its `events` filter. Refused (`409`) against a non-active endpoint (module doc). */ +export async function handleTestWebhook( + id: string, + actingAgent: AgentRecord | null, + deps: WebhooksApiDeps, +): Promise { + const denied = requireAdmin(actingAgent) + if (denied !== null) return denied + if (!isUuid(id)) return NOT_FOUND() + + const endpoints = await deps.store.list() + const target = endpoints.find((e) => e.id === id) + if (target === undefined) return NOT_FOUND() + if (target.status !== 'active') { + return apiError( + 409, + 'conflict', + 'Cannot test a disabled endpoint — set status to active first.', + ) + } + + const job: WebhookDeliveryJob = { + endpointId: target.id, + eventId: randomUUID(), + type: TEST_PING_EVENT_TYPE, + occurredAt: new Date().toISOString(), + conversationId: null, + data: {}, + } + await deps.queue.enqueue(WEBHOOK_DELIVERY_TOPIC, job, { + dedupeKey: `${job.eventId}:${target.id}`, + }) + return json(202, { status: 'queued' }) +} diff --git a/src/composition/app.test.ts b/src/composition/app.test.ts index 5fd19c0..90f5b75 100644 --- a/src/composition/app.test.ts +++ b/src/composition/app.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it, vi } from 'vitest' -import { createAppHandler, HEALTH_PATH, QUEUE_DRAIN_PATH, WATCH_MAINTENANCE_PATH } from './app.js' +import { + createAppHandler, + HEALTH_PATH, + OUTBOX_DRAIN_PATH, + QUEUE_DRAIN_PATH, + WATCH_MAINTENANCE_PATH, +} from './app.js' import type { HealthReport } from './health.js' const CRON_SECRET = 'test-cron-secret-0123456789' @@ -17,22 +23,25 @@ const HEALTHY_REPORT: HealthReport = { }, forgedTokens: { deliveriesLast24h: 0, tokensLast24h: 0, alertThreshold: 5 }, mailboxes: [], + webhooks: { autoDisabled: [], deliveryFailuresLast24h: 0 }, } /** Build a handler over spy deps; the inbox API spy returns a recognizable 299 so delegation is observable. */ function makeHandler(cronSecret: string = CRON_SECRET) { const inboxApi = vi.fn(async () => new Response('inbox', { status: 299 })) const drainQueue = vi.fn(async () => ({ claimed: 3, acked: 3 })) + const drainOutbox = vi.fn(async () => ({ claimed: 2, enqueued: 2, dispatched: 2 })) const runWatchMaintenance = vi.fn(async () => ({ total: 1, renewed: 1 })) const runHealthCheck = vi.fn(async (): Promise => HEALTHY_REPORT) const handler = createAppHandler({ inboxApi, cronSecret, drainQueue, + drainOutbox, runWatchMaintenance, runHealthCheck, }) - return { handler, inboxApi, drainQueue, runWatchMaintenance, runHealthCheck } + return { handler, inboxApi, drainQueue, drainOutbox, runWatchMaintenance, runHealthCheck } } /** A request to `path`; attaches `Authorization: Bearer ` unless `secret` is null. */ @@ -121,6 +130,7 @@ describe('createAppHandler — queue drain endpoint', () => { inboxApi, cronSecret: CRON_SECRET, drainQueue, + drainOutbox: vi.fn(async () => ({})), runWatchMaintenance, runHealthCheck: vi.fn(async () => HEALTHY_REPORT), }) @@ -136,6 +146,61 @@ describe('createAppHandler — queue drain endpoint', () => { }) }) +describe('createAppHandler — outbox drain endpoint (HT-69)', () => { + it('runs the outbox drain and returns its report on a GET with the correct cron secret — a SEPARATE endpoint from the queue drain', async () => { + const { handler, drainOutbox, drainQueue, inboxApi } = makeHandler() + + const res = await handler(req(OUTBOX_DRAIN_PATH)) + + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ + ok: true, + report: { claimed: 2, enqueued: 2, dispatched: 2 }, + }) + expect(res.headers.get('Cache-Control')).toBe('no-store') + expect(drainOutbox).toHaveBeenCalledOnce() + expect(drainQueue).not.toHaveBeenCalled() + expect(inboxApi).not.toHaveBeenCalled() + }) + + it('rejects a wrong cron secret with 401 and never runs the work', async () => { + const { handler, drainOutbox } = makeHandler() + const res = await handler(req(OUTBOX_DRAIN_PATH, { secret: 'wrong-secret-9999999999' })) + expect(res.status).toBe(401) + expect(drainOutbox).not.toHaveBeenCalled() + }) + + it('rejects a non-GET method (authenticated) with 405', async () => { + const { handler, drainOutbox } = makeHandler() + const res = await handler(req(OUTBOX_DRAIN_PATH, { method: 'POST' })) + expect(res.status).toBe(405) + expect(drainOutbox).not.toHaveBeenCalled() + }) + + it('answers a generic 500 (never the error text) when the work throws', async () => { + const inboxApi = vi.fn(async () => new Response(null, { status: 299 })) + const handler = createAppHandler({ + inboxApi, + cronSecret: CRON_SECRET, + drainQueue: vi.fn(async () => ({})), + drainOutbox: vi.fn(async () => { + throw new Error('secret-internal-detail-should-not-leak') + }), + runWatchMaintenance: vi.fn(async () => ({})), + runHealthCheck: vi.fn(async () => HEALTHY_REPORT), + }) + + const res = await handler(req(OUTBOX_DRAIN_PATH)) + const bodyText = await res.text() + + expect(res.status).toBe(500) + expect(bodyText).not.toContain('secret-internal-detail-should-not-leak') + expect(JSON.parse(bodyText)).toEqual({ + error: { code: 'server_error', message: 'Internal server error.' }, + }) + }) +}) + describe('createAppHandler — watch-maintenance endpoint', () => { it('runs the maintenance sweep and returns its report on an authenticated GET', async () => { const { handler, runWatchMaintenance } = makeHandler() diff --git a/src/composition/app.ts b/src/composition/app.ts index 3d35f6c..942a60a 100644 --- a/src/composition/app.ts +++ b/src/composition/app.ts @@ -41,6 +41,9 @@ export const QUEUE_DRAIN_PATH = '/api/v1/internal/queue/drain' /** `GET` (Vercel Cron) → daily Gmail `watch()` re-arm + reconciliation sweep (runbook Part C: daily at 06:00 UTC). */ export const WATCH_MAINTENANCE_PATH = '/api/v1/internal/cron/watch-maintenance' +/** `GET` (Vercel Cron) → drain one bounded batch of `event_outbox` into `queue_jobs` webhook-delivery fan-out (HT-69; `src/webhooks/outbox-drain.ts`; runbook Part C: every minute, same cadence as {@link QUEUE_DRAIN_PATH}). A SEPARATE endpoint from the queue drain — this one turns outbox rows into queue jobs; the queue drain is what then delivers them. */ +export const OUTBOX_DRAIN_PATH = '/api/v1/internal/outbox/drain' + /** * `GET` (an HTTP monitor, or an operator's curl) → the point-in-time * {@link HealthReport} (`./health.ts`; HT-44, runbook Part G). Same @@ -58,6 +61,8 @@ export interface AppHandlerDeps { cronSecret: string /** Drain one bounded batch of the job queue; returns a JSON-serializable report for the response body + logs. */ drainQueue: () => Promise + /** Drain one bounded batch of `event_outbox` into webhook-delivery queue jobs (HT-69, {@link OUTBOX_DRAIN_PATH}); returns a JSON-serializable report for the response body + logs. */ + drainOutbox: () => Promise /** Run one daily watch-renewal + reconciliation-sweep pass; returns a JSON-serializable report. */ runWatchMaintenance: () => Promise /** Assemble the health report (`./health.ts`) — the {@link HEALTH_PATH} endpoint's work. */ @@ -77,6 +82,9 @@ export function createAppHandler(deps: AppHandlerDeps): (request: Request) => Pr if (pathname === QUEUE_DRAIN_PATH) { return handleCronEndpoint(request, deps.cronSecret, 'queue-drain', deps.drainQueue) } + if (pathname === OUTBOX_DRAIN_PATH) { + return handleCronEndpoint(request, deps.cronSecret, 'outbox-drain', deps.drainOutbox) + } if (pathname === WATCH_MAINTENANCE_PATH) { return handleCronEndpoint( request, diff --git a/src/composition/health.test.ts b/src/composition/health.test.ts index f949e7c..3a8da84 100644 --- a/src/composition/health.test.ts +++ b/src/composition/health.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it } from 'vitest' import { createPgliteDb, type Db } from '../db/client.js' import { migrate } from '../db/migrate.js' import { createPostgresQueue } from '../providers/adapters/postgres-queue/index.js' +import { WEBHOOK_DELIVERY_TOPIC } from '../webhooks/delivery.js' import { FORGED_TOKEN_ALERT_THRESHOLD, type HealthReport, runHealthCheck } from './health.js' describe('runHealthCheck', () => { @@ -74,6 +75,32 @@ describe('runHealthCheck', () => { ) } + /** Insert a dead-lettered `WEBHOOK_DELIVERY_TOPIC` queue job (HT-69) parked `deadLetteredAgoHours` ago — distinct from {@link seedDeadLetteredJob}'s generic `'health-test'` topic, which must NOT trip the webhook-specific alert. */ + async function seedDeadLetteredWebhookJob( + database: Db, + deadLetteredAgoHours: number, + ): Promise { + await database.query( + `INSERT INTO queue_jobs (topic, payload, dead_lettered_at) + VALUES ($2, '{}'::jsonb, now() - ($1::double precision * interval '1 hour'))`, + [deadLetteredAgoHours, WEBHOOK_DELIVERY_TOPIC], + ) + } + + /** Insert a `webhook_endpoints` row directly, at a given `status`/`consecutiveFailures` — bypassing `WebhookEndpointStore` (this suite only needs raw rows for the health query, not the store's encrypt-at-rest/auto-disable behavior, which has its own tests). */ + async function seedWebhookEndpoint( + database: Db, + url: string, + status: 'active' | 'disabled' | 'auto_disabled', + consecutiveFailures = 0, + ): Promise { + await database.query( + `INSERT INTO webhook_endpoints (url, secret_ciphertext, status, consecutive_failures) + VALUES ($1, $2, $3, $4)`, + [url, new Uint8Array(28), status, consecutiveFailures], + ) + } + it('an empty (fresh-deploy) database is fully healthy: ok, no alerts, zero-filled sections', async () => { const { check } = await fresh() @@ -97,6 +124,7 @@ describe('runHealthCheck', () => { alertThreshold: FORGED_TOKEN_ALERT_THRESHOLD, }) expect(report.mailboxes).toEqual([]) + expect(report.webhooks).toEqual({ autoDisabled: [], deliveryFailuresLast24h: 0 }) expect(new Date(report.generatedAt).getTime()).not.toBeNaN() }) @@ -266,4 +294,55 @@ describe('runHealthCheck', () => { expect(report.alerts[0]).toMatch(/^queue-drain-stalled: /) expect(report.alerts[1]).toMatch(/^mailbox-needs-attention: /) }) + + describe('webhooks (HT-69)', () => { + it('an auto_disabled endpoint trips webhook-endpoint-auto-disabled and is listed; active/disabled endpoints are silent', async () => { + const { database, check } = await fresh() + await seedWebhookEndpoint(database, 'https://active.example.test/hook', 'active') + await seedWebhookEndpoint(database, 'https://disabled.example.test/hook', 'disabled') + await seedWebhookEndpoint(database, 'https://broken.example.test/hook', 'auto_disabled', 20) + + const report = await check() + + expect(report.ok).toBe(false) + expect(report.webhooks.autoDisabled).toHaveLength(1) + expect(report.webhooks.autoDisabled[0]).toMatchObject({ + url: 'https://broken.example.test/hook', + consecutiveFailures: 20, + }) + const alert = report.alerts.find((a) => a.startsWith('webhook-endpoint-auto-disabled: ')) + expect(alert).toBeDefined() + expect(alert).toContain('1 webhook endpoint(s)') + }) + + it('a webhook.delivery dead-letter in the last 24h trips webhook-delivery-dead-letter-growth; an OLDER one, or one on a DIFFERENT topic, does not', async () => { + const { database, check } = await fresh() + // A generic dead-letter on an unrelated topic must not trip this + // webhook-specific alert (queue-dead-letter-growth is the general one). + await seedDeadLetteredJob(database, 1) + + const beforeWebhookDeadLetter = await check() + expect(beforeWebhookDeadLetter.webhooks.deliveryFailuresLast24h).toBe(0) + expect( + beforeWebhookDeadLetter.alerts.some((a) => + a.startsWith('webhook-delivery-dead-letter-growth'), + ), + ).toBe(false) + + // An OLDER-than-24h webhook.delivery dead-letter is out of the window. + await seedDeadLetteredWebhookJob(database, 30) + const stillClean = await check() + expect(stillClean.webhooks.deliveryFailuresLast24h).toBe(0) + + // A recent one trips it. + await seedDeadLetteredWebhookJob(database, 1) + const report = await check() + + expect(report.ok).toBe(false) + expect(report.webhooks.deliveryFailuresLast24h).toBe(1) + const alert = report.alerts.find((a) => a.startsWith('webhook-delivery-dead-letter-growth: ')) + expect(alert).toBeDefined() + expect(alert).toContain('1 webhook delivery(ies)') + }) + }) }) diff --git a/src/composition/health.ts b/src/composition/health.ts index 7e2dc73..dbfbaaf 100644 --- a/src/composition/health.ts +++ b/src/composition/health.ts @@ -40,6 +40,21 @@ * 72h means renewal has been failing for days — caught while there is * still runway). `disconnected` mailboxes are deliberately silent: that * state is an operator's own explicit action (HT-47). + * - **Webhooks** (HT-69; specs/modules/substrate-v1.md §5: "surfaced by + * `/api/v1/internal/health` (runbook Part G gains a section)"): + * `webhook-endpoint-auto-disabled` for every `webhook_endpoints` row + * `WebhookEndpointStore.recordDeliveryFailure` flipped past the + * consecutive-failure threshold (spec §9 decision 2: 20) — spec's own + * rationale for alerting here ("conservative because a disabled endpoint + * silently stops a paid module"). `webhook-delivery-dead-letter-growth` + * for any `queue_jobs` row on `WEBHOOK_DELIVERY_TOPIC` dead-lettered in + * the last 24h — the SAME growth-not-backlog reasoning as `queue-dead- + * letter-growth`/`ingest-dead-letter-growth` above (a dead-lettered + * delivery's `webhook_endpoints.recordDeliveryFailure` write already + * happened by the time it reaches this state — `src/webhooks/ + * delivery.ts`'s module doc — so this is a SEPARATE signal from the + * auto-disable alert: an endpoint can shed individual failed deliveries + * for a while before crossing 20 consecutive and auto-disabling). * * ## What it deliberately does NOT check * @@ -63,6 +78,7 @@ import type { Db } from '../db/client.js' import type { QueueStats } from '../providers/adapters/postgres-queue/index.js' import type { InboundDeliveryStatus } from '../store/inbound-deliveries.js' +import { WEBHOOK_DELIVERY_TOPIC } from '../webhooks/delivery.js' /** Oldest-ready-job age (seconds) past which the every-minute drain is presumed stalled. */ export const QUEUE_OLDEST_READY_ALERT_SECONDS = 300 @@ -88,6 +104,13 @@ export interface MailboxHealth { watchExpiresAt: string | null } +/** One auto-disabled webhook endpoint — see the module doc's Webhooks section (HT-69). */ +export interface WebhookHealth { + id: string + url: string + consecutiveFailures: number +} + /** The report `GET /api/v1/internal/health` serves — see the module doc for each section and the alert it can trip. */ export interface HealthReport { /** `alerts.length === 0` — the endpoint's 200-vs-503 pivot. */ @@ -110,6 +133,13 @@ export interface HealthReport { alertThreshold: number } mailboxes: MailboxHealth[] + /** HT-69 (spec §5's "surfaced by /api/v1/internal/health") — see the module doc's Webhooks section. */ + webhooks: { + /** Endpoints currently `auto_disabled` — the standing set, not a 24h window (mirrors `ingest.deadLetterTotal`'s "inspect, don't page on the backlog itself" framing; the ALERT is what pages, on `.length > 0`). */ + autoDisabled: WebhookHealth[] + /** `queue_jobs` rows on `WEBHOOK_DELIVERY_TOPIC` dead-lettered in the last 24h. */ + deliveryFailuresLast24h: number + } } /** Every ledger status, for zero-filling {@link HealthReport.ingest}'s per-status map (a status with no 24h rows must still appear, as `0`). */ @@ -235,6 +265,42 @@ export async function runHealthCheck(deps: HealthCheckDeps): Promise( + `SELECT id, url, consecutive_failures FROM webhook_endpoints + WHERE status = 'auto_disabled' + ORDER BY updated_at DESC`, + ) + const autoDisabled: WebhookHealth[] = autoDisabledRows.map((row) => ({ + id: row.id, + url: row.url, + consecutiveFailures: row.consecutive_failures, + })) + if (autoDisabled.length > 0) { + alerts.push( + `webhook-endpoint-auto-disabled: ${autoDisabled.length} webhook endpoint(s) auto-disabled ` + + `after reaching the consecutive-failure threshold — inspect and re-enable via PATCH ` + + '/api/v1/webhooks/{id} once fixed (runbook Part G)', + ) + } + const deliveryFailuresRows = await deps.db.query<{ count: number }>( + `SELECT count(*)::int AS count FROM queue_jobs + WHERE topic = $1 AND dead_lettered_at > now() - interval '24 hours'`, + [WEBHOOK_DELIVERY_TOPIC], + ) + const deliveryFailuresLast24h = deliveryFailuresRows[0]?.count ?? 0 + if (deliveryFailuresLast24h > 0) { + alerts.push( + `webhook-delivery-dead-letter-growth: ${deliveryFailuresLast24h} webhook delivery(ies) ` + + 'dead-lettered in the last 24h — inspect queue_jobs.last_error for topic ' + + `'${WEBHOOK_DELIVERY_TOPIC}'`, + ) + } + return { ok: alerts.length === 0, alerts, @@ -247,6 +313,7 @@ export async function runHealthCheck(deps: HealthCheckDeps): Promise createGmailHistoryClient({ getAccessToken }), }) + // --- The webhook delivery handler the SAME queue drain also dispatches to + // (HT-69) — the outbox drain (wired below, its own cron tick) is what + // ENQUEUES onto this topic; this handler is what the existing every-minute + // `queue.drainOnce` call actually DELIVERS with. --- + const webhookDeliveryHandler = createWebhookDeliveryHandler({ + webhookEndpoints: webhookEndpointStore, + }) + // The drain's handler map is typed `QueueMessageHandler` (the queue // stores arbitrary JSON payloads); the topic string is what guarantees the - // payload is a GmailReconcileJob, so the narrowing cast at this single wiring - // point is the honest boundary between "any queued job" and "this topic's - // job shape". + // payload is a GmailReconcileJob/WebhookDeliveryJob, so the narrowing cast + // at this single wiring point is the honest boundary between "any queued + // job" and "this topic's job shape". const drainHandlers: Record> = { [GMAIL_RECONCILE_TOPIC]: (message) => reconcileHandler(message as QueueMessage), + [WEBHOOK_DELIVERY_TOPIC]: (message) => + webhookDeliveryHandler(message as QueueMessage), } // --- Watch-maintenance deps (daily re-arm + sweep). --- @@ -311,6 +342,23 @@ export async function buildApp( } return report }, + // --- Outbox drain (HT-69) — a SEPARATE cron tick from the queue drain + // above: this one turns `event_outbox` rows into `queue_jobs` fan-out + // (`drainEventOutbox`, `src/webhooks/outbox-drain.ts`); the queue drain + // above is what then actually DELIVERS them, via `webhookDeliveryHandler` + // registered on `WEBHOOK_DELIVERY_TOPIC` in `drainHandlers`. Same + // quiet-tick log suppression as `drainQueue` above. --- + drainOutbox: async () => { + const report = await drainEventOutbox({ + eventOutbox: eventOutboxStore, + webhookEndpoints: webhookEndpointStore, + queue, + }) + if (report.claimed > 0) { + console.info(JSON.stringify({ event: 'outbox_drain', ...report })) + } + return report + }, runWatchMaintenance: () => runGmailWatchMaintenance(watchMaintenanceDeps), runHealthCheck: () => runHealthCheck({ db, queue }), }) diff --git a/src/db/migrate.ts b/src/db/migrate.ts index 21ed5db..427e5b7 100644 --- a/src/db/migrate.ts +++ b/src/db/migrate.ts @@ -1224,11 +1224,17 @@ CREATE TABLE webhook_endpoints ( * table is only the DURABLE STAGING AREA between "the state change * committed" and "the drain step handed this off to the real queue" (spec * §4: "a drain step... turns outbox rows into QueueProvider deliveries"). - * Once a row is handed to `queue_jobs` (keyed by `dedupe_key = event_id`, - * so a double-enqueue from a crashed drain is harmless per migration 013's - * own dedupe precedent), ALL retry/backoff/dead-letter bookkeeping for - * actually delivering the event lives there, not here — this table needs no - * `attempts`/`last_error`/`dead_lettered_at` columns of its own. + * One outbox row FANS OUT to one `queue_jobs` row PER matching active + * webhook endpoint (spec §5's subset filter — an event can have several + * subscribers), keyed `dedupe_key = ` `` `${eventId}:${endpointId}` `` + * (HT-69, `src/webhooks/outbox-drain.ts`) — per-pair, not per-event, since + * a single shared `eventId` key would collide the first endpoint's enqueue + * against every other endpoint's and silently drop their deliveries. A + * double-enqueue of the SAME pair (a crashed drain retried on the next + * tick) is harmless per migration 013's own dedupe precedent. ALL retry/ + * backoff/dead-letter bookkeeping for actually delivering an event lives in + * `queue_jobs`, not here — this table needs no `attempts`/`last_error`/ + * `dead_lettered_at` columns of its own. * * What it DOES need, mirroring `queue_jobs`'s lease shape narrowly: `locked_ * until`, so two overlapping drain invocations (an overlapping cron tick, a diff --git a/src/mail/ingest.test.ts b/src/mail/ingest.test.ts index 0101c0c..db51028 100644 --- a/src/mail/ingest.test.ts +++ b/src/mail/ingest.test.ts @@ -695,6 +695,219 @@ describe('ingestInboundMessage', () => { expect(await countRows(db, 'conversations')).toBe(0) }) + // --- HT-69, spec §4: event emission on the ingestion append path. -------- + + describe('event emission (HT-69, spec §4)', () => { + interface OutboxEventRow { + type: string + conversation_id: string + data: Record + } + + async function outboxEventsFor( + database: Db, + conversationId: string, + ): Promise { + return database.query( + 'SELECT type, conversation_id, data FROM event_outbox WHERE conversation_id = $1 ORDER BY occurred_at, event_id', + [conversationId], + ) + } + + it('a fresh message (new conversation) fires conversation.created + conversation.message_received(reopened:false), both thin', async () => { + const { db, deps, mailboxId } = await freshDeps() + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (outcome.kind !== 'stored') throw new Error('unreachable') + + // Both rows land in the SAME transaction, so `occurred_at` (transaction + // start time in Postgres) TIES between them — spec §8 promises no + // cross-event ordering guarantee anyway, so these assertions are + // deliberately order-independent (by TYPE, not array position). + const events = await outboxEventsFor(db, outcome.conversationId) + expect(events).toHaveLength(2) + expect(events.map((e) => e.type).sort()).toEqual([ + 'conversation.created', + 'conversation.message_received', + ]) + const created = events.find((e) => e.type === 'conversation.created') + const messageReceived = events.find((e) => e.type === 'conversation.message_received') + expect(created?.data).toEqual({}) // conversation.created carries no data (spec §4) + expect(Object.keys(messageReceived?.data ?? {}).sort()).toEqual(['reopened', 'threadId']) + expect(messageReceived?.data).toEqual({ threadId: outcome.threadId, reopened: false }) + // Thin: no bodyText/subject/fromAddress anywhere in either payload. + for (const event of events) { + expect(JSON.stringify(event.data)).not.toMatch(/order|customer@example|Help with/) + } + }) + + it('a valid-token reply to an ACTIVE conversation fires only conversation.message_received(reopened:false)', async () => { + const { db, deps, mailboxId } = await freshDeps() + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + const second = await ingestInboundMessage( + inboundDelivery( + mailboxId, + 'provider-msg-2', + rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + 'In-Reply-To': replyToken, + }, + 'Still broken.', + ), + ), + deps, + ) + if (second.kind !== 'stored') throw new Error('unreachable') + + const events = await outboxEventsFor(db, first.conversationId) + // Exactly one conversation.created (the reply must NOT mint a second + // one) and two message_received (the original + the reply) — order + // is not asserted (spec §8: no cross-event ordering guarantee), so + // the reply's own event is found by its threadId, not array position. + expect(events.filter((e) => e.type === 'conversation.created')).toHaveLength(1) + expect(events.filter((e) => e.type === 'conversation.message_received')).toHaveLength(2) + const replyEvent = events.find( + (e) => e.type === 'conversation.message_received' && e.data.threadId === second.threadId, + ) + expect(replyEvent?.data).toEqual({ threadId: second.threadId, reopened: false }) + }) + + it('a valid-token reply to a CLOSED conversation reopens it and fires conversation.message_received(reopened:true)', async () => { + const { db, deps, mailboxId } = await freshDeps() + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + await db.query("UPDATE conversations SET status = 'closed' WHERE id = $1", [ + first.conversationId, + ]) + + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + const second = await ingestInboundMessage( + inboundDelivery( + mailboxId, + 'provider-msg-2', + rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + 'In-Reply-To': replyToken, + }, + 'Still broken.', + ), + ), + deps, + ) + if (second.kind !== 'stored') throw new Error('unreachable') + + const events = await outboxEventsFor(db, first.conversationId) + const reopenEvent = events.find( + (e) => e.type === 'conversation.message_received' && e.data.threadId === second.threadId, + ) + expect(reopenEvent?.data).toEqual({ threadId: second.threadId, reopened: true }) + }) + + it('the deleted/not-found append fallback fires conversation.created + conversation.message_received(reopened:false) on the FRESH conversation, not the orphaned one', async () => { + const { db, deps, mailboxId } = await freshDeps() + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + await db.query("UPDATE conversations SET status = 'deleted' WHERE id = $1", [ + first.conversationId, + ]) + + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + const second = await ingestInboundMessage( + inboundDelivery( + mailboxId, + 'provider-msg-2', + rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + 'In-Reply-To': replyToken, + }, + 'Still broken.', + ), + ), + deps, + ) + if (second.kind !== 'stored') throw new Error('unreachable') + expect(second.conversationId).not.toBe(first.conversationId) + + // The DELETED conversation's own events are untouched — still exactly + // its original two (soft-delete fires nothing, and this fallback never + // touches it). + expect(await outboxEventsFor(db, first.conversationId)).toHaveLength(2) + + // The FRESH fallback conversation gets its own created + message_received + // (same-transaction pair — order not asserted, see the first test's note). + const freshEvents = await outboxEventsFor(db, second.conversationId) + expect(freshEvents.map((e) => e.type).sort()).toEqual([ + 'conversation.created', + 'conversation.message_received', + ]) + const freshMessageReceived = freshEvents.find( + (e) => e.type === 'conversation.message_received', + ) + expect(freshMessageReceived?.data).toEqual({ threadId: second.threadId, reopened: false }) + }) + + it('a step-5 transaction rollback leaves NO event row behind, along with no conversation/thread row (same transaction)', async () => { + const { db, deps, mailboxId } = await freshDeps() + // Fails on the 2nd `.transaction()` call — claim's own transaction is + // the 1st, so this targets step 5 (storeAndMarkDelivered), the SAME + // transaction this ticket's event-emission calls run inside (module + // doc reference in ingest.test.ts's own dbFailingOnCall comment). + const faultyDb = dbFailingOnCall(db, 2) + const faultyDeps: IngestDeps = { + ...deps, + db: faultyDb, + inboundDeliveryStore: createInboundDeliveryStore(faultyDb), + } + + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + faultyDeps, + ) + expect(outcome.kind).toBe('failed') + + expect(await countRows(db, 'conversations')).toBe(0) + const allEvents = await db.query<{ count: number }>( + 'SELECT count(*)::int AS count FROM event_outbox', + ) + expect(allEvents[0].count).toBe(0) + }) + }) + // --- spec §5 / §8: the loop guard. ---------------------------------------- describe('isOwnMessageReflection (pure unit)', () => { diff --git a/src/mail/ingest.ts b/src/mail/ingest.ts index af3baf2..3ff2398 100644 --- a/src/mail/ingest.ts +++ b/src/mail/ingest.ts @@ -59,6 +59,7 @@ import type { Db, Queryable } from '../db/client.js' import type { BlobStore, RawInboundMessage, RawMessageContent } from '../providers/index.js' import { insertThreadAttachmentsInTx, type NewThreadAttachment } from '../store/attachments.js' import { appendThreadInTx, createConversationInTx, type NewThread } from '../store/conversations.js' +import { appendOutboxEventInTx } from '../store/event-outbox.js' import { type InboundDeliveryStore, LeaseLostError, @@ -665,6 +666,26 @@ async function storeAndMarkDelivered( * `appendFallback` names why (spec §6's append-fallback reason, HT-44) — * the decision itself is left untouched, so the caller can log both what * `decideThreading` decided AND what the store made of it. + * + * ## Event emission (HT-69; specs/modules/substrate-v1.md §4) + * + * This is "the ingestion append path" spec §4's vocabulary table means by + * `conversation.created`/`conversation.message_received`: both are fired + * HERE, inside the SAME `tx` this function already writes the conversation/ + * thread rows in (spec §4's transactional-outbox rule) — never inside + * `appendThreadInTx`/`createConversationInTx` themselves, which are shared + * by callers that must NOT fire these events (`src/mail/send.ts`'s outbound + * `sendReply`, and the notes handler in `src/api/conversations.ts`, both go + * through `appendThread` too, but neither is "an inbound thread stored"). + * + * A brand-new conversation's first message fires BOTH events + * (`conversation.created` with no data, then `conversation.message_ + * received` with `reopened: false` — a fresh conversation is never a + * reopen) — this covers a genuinely fresh `decision.kind === 'new'` AND the + * deleted/not-found fallback below, which also creates a fresh + * conversation. A successful `append` fires only `conversation.message_ + * received`, with `reopened` taken verbatim from `AppendResult.reopened` + * (`src/store/conversations.ts`) rather than re-derived here. */ async function writeParsedEmail( tx: Queryable, @@ -681,15 +702,22 @@ async function writeParsedEmail( } if (decision.kind === 'new') { - return createConversationInTx(tx, { + const created = await createConversationInTx(tx, { subject: parsed.subject, customerEmail: fromAddressOf(parsed), firstMessage, }) + await emitNewConversationEvents(tx, created.conversationId, created.threadId) + return created } const appended = await appendThreadInTx(tx, decision.conversationId, firstMessage) if (appended.ok) { + await appendOutboxEventInTx(tx, { + type: 'conversation.message_received', + conversationId: decision.conversationId, + data: { threadId: appended.threadId, reopened: appended.reopened }, + }) return { conversationId: decision.conversationId, threadId: appended.threadId } } @@ -701,9 +729,28 @@ async function writeParsedEmail( customerEmail: fromAddressOf(parsed), firstMessage, }) + await emitNewConversationEvents(tx, created.conversationId, created.threadId) return { ...created, appendFallback: appended.reason } } +/** Fire `conversation.created` + `conversation.message_received` (`reopened: false`) for a freshly-created conversation's first thread — the one pair {@link writeParsedEmail} emits from two call sites (a genuine `new` decision, and the deleted/not-found fallback), factored out so both stay byte-identical. */ +async function emitNewConversationEvents( + tx: Queryable, + conversationId: string, + threadId: string, +): Promise { + await appendOutboxEventInTx(tx, { + type: 'conversation.created', + conversationId, + data: {}, + }) + await appendOutboxEventInTx(tx, { + type: 'conversation.message_received', + conversationId, + data: { threadId, reopened: false }, + }) +} + /** * Emit spec §6's "structured record" for one ingest outcome: `mailboxId`, * `providerMessageId`, the threading decision, `forgedTokenCount`, diff --git a/src/store/conversations.test.ts b/src/store/conversations.test.ts index 7df0895..2356165 100644 --- a/src/store/conversations.test.ts +++ b/src/store/conversations.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it } from 'vitest' -import { createPgliteDb, type Db } from '../db/client.js' +import { createPgliteDb, type Db, type Queryable } from '../db/client.js' import { migrate } from '../db/migrate.js' import { createConversationStore, @@ -8,6 +8,49 @@ import { type SendEnvelope, } from './conversations.js' +/** Raw `event_outbox` row shape, for the event-emission assertions below (HT-69). */ +interface OutboxEventRow { + event_id: string + type: string + conversation_id: string + data: Record +} + +/** Every `event_outbox` row for `conversationId`, oldest first — a direct read, bypassing `EventOutboxStore`, since these tests assert on WHAT was written, not on the claim/drain machinery (covered by `event-outbox.test.ts`). */ +async function outboxEventsFor(db: Db, conversationId: string): Promise { + return db.query( + 'SELECT event_id, type, conversation_id, data FROM event_outbox WHERE conversation_id = $1 ORDER BY occurred_at, event_id', + [conversationId], + ) +} + +/** + * Wrap `real` so that any `INSERT INTO event_outbox` issued INSIDE a + * transaction throws — used to prove the state write and the outbox event + * genuinely share one transaction (spec §4: "an event never fires for a + * change that rolled back"), by forcing the event write to fail AFTER the + * state write already ran and confirming BOTH are rolled back, not just the + * event. + */ +function dbFailingOutboxInsert(real: Db): Db { + return { + query: (sql, params) => real.query(sql, params), + close: () => real.close(), + transaction: async (fn: (tx: Queryable) => Promise): Promise => + real.transaction(async (tx) => { + const guarded: Queryable = { + query: async (sql, params) => { + if (sql.includes('INSERT INTO event_outbox')) { + throw new Error('simulated event_outbox insert failure') + } + return tx.query(sql, params) + }, + } + return fn(guarded) + }), + } +} + // --- fixtures ---------------------------------------------------------------- const RANDOM_UUID = '00000000-0000-4000-8000-000000000000' @@ -1612,4 +1655,171 @@ describe('createConversationStore', () => { }) }) }) + + describe('event emission (HT-69, spec §4)', () => { + /** Insert a real `agents` row directly — setConversationAssignee's assignee_agent_id FKs to it. Same shape as the `tags & assignee`/`drafts` describe blocks' own local helpers (this file's convention: a small per-block helper rather than a shared one). */ + async function createTestAgent(db: Db, email = 'agent@example.test'): Promise { + const [row] = await db.query<{ id: string }>( + `INSERT INTO agents (email, name, role, status) VALUES ($1, 'Agent', 'agent', 'active') RETURNING id`, + [email], + ) + return row.id + } + + it('setConversationStatus emits conversation.status_changed with a thin {from,to} payload, in the SAME transaction — a rollback fires NOTHING', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + + const updated = await store.setConversationStatus(conversationId, 'closed') + expect(updated).not.toBeNull() + + const events = await outboxEventsFor(db, conversationId) + expect(events).toHaveLength(1) + expect(events[0].type).toBe('conversation.status_changed') + // Thin payload: exactly {from, to} — never subject/customerEmail/etc. + expect(Object.keys(events[0].data).sort()).toEqual(['from', 'to']) + expect(events[0].data).toEqual({ from: 'active', to: 'closed' }) + + // Rollback: force the event write itself to fail, and prove the STATUS + // change rolled back with it (not just that no event landed). + const failingStore = createConversationStore(dbFailingOutboxInsert(db)) + await expect(failingStore.setConversationStatus(conversationId, 'spam')).rejects.toThrow() + const afterRollback = await store.getConversation(conversationId) + expect(afterRollback?.status).toBe('closed') // unchanged — the failed attempt never committed + expect(await outboxEventsFor(db, conversationId)).toHaveLength(1) // still just the first + }) + + it('re-asserting the SAME status is not a "transition" — fires nothing', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + + await store.setConversationStatus(conversationId, 'active') // conversations start 'active' + + expect(await outboxEventsFor(db, conversationId)).toHaveLength(0) + }) + + it('setConversationTags emits conversation.tags_changed with a thin {tags} payload on every replace, even a no-op one', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + + await store.setConversationTags(conversationId, ['billing', 'urgent']) + await store.setConversationTags(conversationId, ['billing', 'urgent']) // same set again + + const events = await outboxEventsFor(db, conversationId) + expect(events).toHaveLength(2) + for (const event of events) { + expect(event.type).toBe('conversation.tags_changed') + expect(Object.keys(event.data)).toEqual(['tags']) + } + expect(events[0].data).toEqual({ tags: ['billing', 'urgent'] }) + + const failingStore = createConversationStore(dbFailingOutboxInsert(db)) + await expect( + failingStore.setConversationTags(conversationId, ['rolled-back']), + ).rejects.toThrow() + const afterRollback = await store.getConversation(conversationId) + expect(afterRollback?.tags).toEqual(['billing', 'urgent']) // unchanged + expect(await outboxEventsFor(db, conversationId)).toHaveLength(2) // still just the first two + }) + + it('setConversationAssignee emits conversation.assignee_changed with a thin {assigneeAgentId} payload, set or cleared', async () => { + const { db, store } = await freshStore() + const agentId = await createTestAgent(db) + const { conversationId } = await store.createConversation(newConversation()) + + await store.setConversationAssignee(conversationId, agentId) + await store.setConversationAssignee(conversationId, null) + + const events = await outboxEventsFor(db, conversationId) + expect(events).toHaveLength(2) + for (const event of events) { + expect(event.type).toBe('conversation.assignee_changed') + expect(Object.keys(event.data)).toEqual(['assigneeAgentId']) + } + expect(events[0].data).toEqual({ assigneeAgentId: agentId }) + expect(events[1].data).toEqual({ assigneeAgentId: null }) + }) + + it('setConversationAssignee: an invalid_agent FK failure fires no event (the UPDATE itself never committed)', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + + const result = await store.setConversationAssignee(conversationId, RANDOM_UUID) + + expect(result).toBe('invalid_agent') + expect(await outboxEventsFor(db, conversationId)).toHaveLength(0) + }) + + it("releaseThreadLease('sent') emits conversation.reply_sent with a thin {threadId,authorKind} payload; 'failed' fires nothing", async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + const sentThread = await store.appendThread(conversationId, newThread()) + if (!sentThread.ok) throw new Error('unreachable') + const failedThread = await store.appendThread( + conversationId, + newThread({ messageId: '' }), + ) + if (!failedThread.ok) throw new Error('unreachable') + + await store.claimThreadForDelivery(sentThread.threadId, 30_000) + await store.releaseThreadLease(sentThread.threadId, 'sent') + await store.claimThreadForDelivery(failedThread.threadId, 30_000) + await store.releaseThreadLease(failedThread.threadId, 'failed') + + const events = await outboxEventsFor(db, conversationId) + expect(events).toHaveLength(1) // only the 'sent' one + expect(events[0].type).toBe('conversation.reply_sent') + expect(Object.keys(events[0].data).sort()).toEqual(['authorKind', 'threadId']) + expect(events[0].data).toEqual({ threadId: sentThread.threadId, authorKind: 'agent' }) + }) + + it("a thread stranded 'pending'/claimed when its conversation is soft-deleted still delivers ('sent' recorded) but fires NO event (spec §4's absolute soft-delete exclusion, review fix)", async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + const strandedThread = await store.appendThread(conversationId, newThread()) + if (!strandedThread.ok) throw new Error('unreachable') + + // Claim the lease FIRST (as the delivery worker/keyed retry would), + // THEN the conversation is soft-deleted underneath it — mail delivery + // is not conversation-status-scoped, so this claimed thread can still + // be force-delivered after the fact. + await store.claimThreadForDelivery(strandedThread.threadId, 30_000) + const deleted = await store.deleteConversation(conversationId) + expect(deleted).toBe(true) + + await store.releaseThreadLease(strandedThread.threadId, 'sent') + + // The delivery status write itself is honest and unaffected — the + // mail really did go out (charter invariant #1). + const raw = await store.getConversation(conversationId) + const thread = raw?.threads.find((t) => t.id === strandedThread.threadId) + expect(thread?.deliveryStatus).toBe('sent') + + // But NO event fired — soft delete fires nothing, ever, with no + // exception for a delivery that happened to land after the fact. + expect(await outboxEventsFor(db, conversationId)).toHaveLength(0) + }) + + it('soft delete fires NOTHING, ever — deleting a conversation with prior events adds no new row for the deletion', async () => { + const { db, store } = await freshStore() + const { conversationId } = await store.createConversation(newConversation()) + await store.setConversationStatus(conversationId, 'closed') // one event, for the baseline + const before = await outboxEventsFor(db, conversationId) + expect(before).toHaveLength(1) + + const deleted = await store.deleteConversation(conversationId) + expect(deleted).toBe(true) + + const after = await outboxEventsFor(db, conversationId) + expect(after).toHaveLength(1) // unchanged — the delete itself added nothing + expect(after).toEqual(before) + + // And every write path on a now-deleted conversation is a no-op that + // also fires nothing (the null/invalid-agent returns are already + // covered elsewhere; this just confirms zero NEW event rows). + await store.setConversationStatus(conversationId, 'active') + await store.setConversationTags(conversationId, ['x']) + expect(await outboxEventsFor(db, conversationId)).toHaveLength(1) + }) + }) }) diff --git a/src/store/conversations.ts b/src/store/conversations.ts index eb375df..3d72b3e 100644 --- a/src/store/conversations.ts +++ b/src/store/conversations.ts @@ -141,6 +141,7 @@ */ import type { Db, Queryable, SqlValue } from '../db/client.js' +import { appendOutboxEventInTx } from './event-outbox.js' /** * A snapshot of the mail headers an outbound reply was sent with: @@ -349,9 +350,20 @@ export interface StoredConversation { * outcome of running arbitrary inbound mail through the threading decision * (see the module doc's resolution of specs/mail/threading.md §5), and * callers should handle it as ordinary control flow. + * + * `reopened` (HT-69; specs/modules/substrate-v1.md §4's `conversation. + * message_received` event, `reopened` field) is `true` exactly when THIS + * call's `created && thread.draftStatus === undefined` reopen branch fired + * (module doc: a genuinely new, non-draft, non-note thread landing on a + * `closed`/`spam` conversation) — `false` for every other case, including a + * replay (`created: false`) and a note/draft insert, which never reopen by + * construction. Exposed so a caller that fires `conversation.message_ + * received` (the inbound ingest pipeline, `src/mail/ingest.ts`) can report + * the spec's `reopened` fact without re-deriving it from a second read of + * the conversation's pre-append status. */ export type AppendResult = - | { ok: true; threadId: string; created: boolean; thread: StoredThread } + | { ok: true; threadId: string; created: boolean; thread: StoredThread; reopened: boolean } | { ok: false; reason: 'not-found' | 'deleted' } /** @@ -983,8 +995,15 @@ export async function appendThreadInTx( // than even a note. This is checked FIRST, ahead of the direction check // below, so a draft row (direction: 'outbound') never falls into the // reopen/bump branch a plain outbound send would. + // + // HT-69: `reopened` (AppendResult's own field — see its doc comment) + // mirrors this exact condition, computed here where the pre-append + // `row.status` is still in scope, rather than asking a caller to + // re-derive it from a second read. + let reopened = false if (created && thread.draftStatus === undefined) { if ((row.status === 'closed' || row.status === 'spam') && thread.direction !== 'note') { + reopened = true await tx.query( "UPDATE conversations SET status = 'active', updated_at = now() WHERE id = $1", [conversationId], @@ -994,7 +1013,7 @@ export async function appendThreadInTx( } } - return { ok: true, threadId, created, thread: toStoredThread(threadRow) } + return { ok: true, threadId, created, thread: toStoredThread(threadRow), reopened } } export function createConversationStore(db: Db): ConversationStore { @@ -1090,15 +1109,56 @@ export function createConversationStore(db: Db): ConversationStore { // (see its doc comment) — kept as a separate method rather than a // parameter there so the pre-HT-16 no-idempotency-key send path keeps // calling setThreadDeliveryStatus completely unchanged. - const updated = await db.query<{ id: string }>( - "UPDATE threads SET delivery_status = $1, claimed_until = NULL WHERE id = $2 AND direction = 'outbound' RETURNING id", - [status, threadId], - ) - if (updated.length === 0) { - throw new Error( - `releaseThreadLease: no outbound thread with id ${threadId} (wrong id, an inbound thread, or the row was deleted)`, + // + // HT-69 (spec §4's `conversation.reply_sent`, spec §9 decision 3: + // fired at 'sent' — truth, not intent): this is THE delivery-status + // transition this ticket owns (this method is shared by `sendReply`'s + // keyed-retry claim path AND `runDeliveryWorker`'s sweep, + // `src/mail/send.ts`'s `attemptDeliveryOfClaimedThread` — the ONE + // place either caller marks a claimed row `sent`/`failed`; the + // no-idempotency-key `setThreadDeliveryStatus` path above is legacy + // and unreachable from the real API, which requires `Idempotency-Key` + // on every reply — see this ticket's report for the full reasoning). + // Wrapped in a transaction so the status write and the outbox event + // commit or roll back together (spec §4: "an event never fires for a + // change that rolled back") — only on the 'sent' branch; 'failed' + // fires nothing (not in spec §4's vocabulary). + // + // Soft-delete carve-out (review fix, HT-69): mail delivery is NOT + // conversation-status-scoped — a thread claimed/leased before its + // conversation was soft-deleted can still legitimately be delivered + // and marked 'sent' here (charter invariant #1: never lose or corrupt + // customer mail; the send already happened by the time this write + // runs). But spec §4 is absolute: "No event of any type fires for a + // soft-deleted conversation after its deletion" — the SAME + // indistinguishable-from-nonexistent rule `listAwaitingDrafts` + // already enforces for drafts via its `c.status <> 'deleted'` join. + // The correlated `conversation_status` column below is read in the + // SAME statement as the delivery-status write (no separate query, no + // TOCTOU against a concurrent delete), and gates the event append — + // never the delivery-status write itself, which always proceeds. + await db.transaction(async (tx) => { + const updated = await tx.query( + `UPDATE threads SET delivery_status = $1, claimed_until = NULL + WHERE id = $2 AND direction = 'outbound' + RETURNING ${THREAD_COLUMNS}, + (SELECT status FROM conversations WHERE id = threads.conversation_id) AS conversation_status`, + [status, threadId], ) - } + if (updated.length === 0) { + throw new Error( + `releaseThreadLease: no outbound thread with id ${threadId} (wrong id, an inbound thread, or the row was deleted)`, + ) + } + if (status === 'sent' && updated[0].conversation_status !== 'deleted') { + const thread = toStoredThread(updated[0]) + await appendOutboxEventInTx(tx, { + type: 'conversation.reply_sent', + conversationId: thread.conversationId, + data: { threadId: thread.id, authorKind: thread.authorKind }, + }) + } + }) }, async listDeliverableThreads(options) { @@ -1177,15 +1237,49 @@ export function createConversationStore(db: Db): ConversationStore { }, async setConversationStatus(conversationId, status) { - const rows = await db.query( - `UPDATE conversations - SET status = $1, updated_at = now() - WHERE id = $2 AND status <> 'deleted' - ${summaryReturningSql('$2')}`, - [status, conversationId], - ) - const row = rows[0] - return row === undefined ? null : toConversationSummary(row) + // HT-69 (spec §4's `conversation.status_changed`, "status transition + // among the four API states"): the `from`/`to` payload needs the + // PRIOR status, which a plain UPDATE...RETURNING never exposes (only + // the post-update row). `SELECT ... FOR UPDATE` first — mirroring + // appendThreadInTx's own lock-then-act shape above — locks the row so + // no concurrent write can change `status` between this read and the + // UPDATE below, then the UPDATE and (when the status actually + // changed) the outbox event commit together in the SAME transaction + // (spec §4's transactional-outbox rule). `deleted` is excluded by + // TYPE (`ConversationStatus` has no `'deleted'` member) — this method + // can never be called with it, so "only the four API states fire + // this event" holds by construction, not by a runtime check. + return db.transaction(async (tx) => { + const priorRows = await tx.query<{ status: string }>( + 'SELECT status FROM conversations WHERE id = $1 FOR UPDATE', + [conversationId], + ) + const prior = priorRows[0] + if (prior === undefined || prior.status === 'deleted') return null + + const rows = await tx.query( + `UPDATE conversations + SET status = $1, updated_at = now() + WHERE id = $2 AND status <> 'deleted' + ${summaryReturningSql('$2')}`, + [status, conversationId], + ) + const row = rows[0] + if (row === undefined) return null + + // "Transition" (spec §4) — a PATCH that re-asserts the SAME status + // touches nothing new, so it fires no event (mirrors updated_at + // still bumping either way — a no-op transition is idempotent + // storage-wise but not event-worthy). + if (prior.status !== status) { + await appendOutboxEventInTx(tx, { + type: 'conversation.status_changed', + conversationId, + data: { from: prior.status, to: status }, + }) + } + return toConversationSummary(row) + }) }, async setConversationTags(conversationId, tags) { @@ -1193,28 +1287,64 @@ export function createConversationStore(db: Db): ConversationStore { // interface doc); jsonb columns take caller-serialized JSON text, the // same convention as send_envelope. No updated_at bump: metadata, not // activity (spec §4e). - const rows = await db.query( - `UPDATE conversations - SET tags = $1::jsonb - WHERE id = $2 AND status <> 'deleted' - ${summaryReturningSql('$2')}`, - [JSON.stringify(tags), conversationId], - ) - const row = rows[0] - return row === undefined ? null : toConversationSummary(row) + // + // HT-69 (spec §4's `conversation.tags_changed`, "fired when: tag set + // replaced"): unconditional on every successful replace — unlike + // status_changed's "transition" wording, this event names the ACTION + // (a PUT that replaces the set), not a before/after diff, so it fires + // even when the replacement happens to equal the prior set. Wrapped + // in a transaction so the write and the event commit together (spec + // §4's transactional-outbox rule). + return db.transaction(async (tx) => { + const rows = await tx.query( + `UPDATE conversations + SET tags = $1::jsonb + WHERE id = $2 AND status <> 'deleted' + ${summaryReturningSql('$2')}`, + [JSON.stringify(tags), conversationId], + ) + const row = rows[0] + if (row === undefined) return null + await appendOutboxEventInTx(tx, { + type: 'conversation.tags_changed', + conversationId, + data: { tags }, + }) + return toConversationSummary(row) + }) }, async setConversationAssignee(conversationId, assigneeAgentId) { // No updated_at bump: claiming is metadata, not activity (spec §4f). - let rows: ConversationSummaryRow[] + // + // HT-69 (spec §4's `conversation.assignee_changed`, "assignee + // set/cleared"): unconditional on every successful write, same + // "names the action, not a before/after diff" reasoning as + // setConversationTags above. Wrapped in a transaction so the write + // and the event commit together (spec §4's transactional-outbox + // rule); the FK-violation catch stays around the WHOLE transaction + // call (not just the UPDATE) since a rolled-back transaction due to + // the FK throw must not leave a dangling outbox row either — though + // in practice the throw happens before appendOutboxEventInTx is ever + // reached, since the UPDATE itself is what violates the FK. try { - rows = await db.query( - `UPDATE conversations - SET assignee_agent_id = $1 - WHERE id = $2 AND status <> 'deleted' - ${summaryReturningSql('$2')}`, - [assigneeAgentId, conversationId], - ) + return await db.transaction(async (tx) => { + const rows = await tx.query( + `UPDATE conversations + SET assignee_agent_id = $1 + WHERE id = $2 AND status <> 'deleted' + ${summaryReturningSql('$2')}`, + [assigneeAgentId, conversationId], + ) + const row = rows[0] + if (row === undefined) return null + await appendOutboxEventInTx(tx, { + type: 'conversation.assignee_changed', + conversationId, + data: { assigneeAgentId }, + }) + return toConversationSummary(row) + }) } catch (err) { // The Agent was deleted between the caller's existence check and this // UPDATE — the FK is the authoritative guard for that race (interface @@ -1222,8 +1352,6 @@ export function createConversationStore(db: Db): ConversationStore { if (isAssigneeFkViolation(err)) return 'invalid_agent' throw err } - const row = rows[0] - return row === undefined ? null : toConversationSummary(row) }, async recordThreadView(threadId) { diff --git a/src/webhooks/delivery.test.ts b/src/webhooks/delivery.test.ts new file mode 100644 index 0000000..de18898 --- /dev/null +++ b/src/webhooks/delivery.test.ts @@ -0,0 +1,428 @@ +import { createHmac, randomBytes } from 'node:crypto' +import { EventEmitter } from 'node:events' +import { afterEach, describe, expect, it } from 'vitest' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { QueueMessage } from '../providers/queue.js' +import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' +import { + createWebhookEndpointStore, + type WebhookEndpointStore, +} from '../store/webhook-endpoints.js' +import { + createWebhookDeliveryHandler, + type HttpsRequestFn, + sendWebhookRequest, + signWebhookPayload, + WEBHOOK_DELIVERY_MAX_ATTEMPTS, + type WebhookDeliveryJob, +} from './delivery.js' +import type { PinnedAddress } from './ssrf.js' +import { type resolveSafeAddress, SsrfRefusedError } from './ssrf.js' + +const ENC_KEY = randomBytes(ENCRYPTION_KEY_BYTES) + +// --- signWebhookPayload (consumer-side verification) ------------------------ + +describe('signWebhookPayload', () => { + it('produces the Stripe-shape t=, v1= string spec §5 requires', () => { + const sig = signWebhookPayload('shh', '{"a":1}', 1_700_000_000) + expect(sig).toMatch(/^t=1700000000, v1=[0-9a-f]{64}$/) + }) + + it('a consumer can independently recompute and verify the signature (this ticket brief\'s "consumer-side verification")', () => { + const secret = 'endpoint-secret-xyz' + const body = JSON.stringify({ eventId: 'e1', type: 'conversation.created' }) + const timestamp = 1_700_000_123 + const sig = signWebhookPayload(secret, body, timestamp) + + // Independent recomputation — a consumer's own verifier, not a call + // through this module's own function. + const expectedMac = createHmac('sha256', secret).update(`${timestamp}.${body}`).digest('hex') + expect(sig).toBe(`t=${timestamp}, v1=${expectedMac}`) + }) + + it('changing the secret, body, or timestamp changes the signature', () => { + const base = signWebhookPayload('secret-a', 'body', 1000) + expect(signWebhookPayload('secret-b', 'body', 1000)).not.toBe(base) + expect(signWebhookPayload('secret-a', 'other-body', 1000)).not.toBe(base) + expect(signWebhookPayload('secret-a', 'body', 1001)).not.toBe(base) + }) +}) + +// --- sendWebhookRequest ------------------------------------------------------ + +/** A fake `PinnedAddress` resolver that never touches real DNS. */ +function fakeResolve(address = '203.0.113.9', family: 4 | 6 = 4): typeof resolveSafeAddress { + return async () => ({ address, family }) satisfies PinnedAddress +} + +/** + * A minimal fake of `node:https.request`'s shape: an `EventEmitter`-based + * `ClientRequest`, whose `.end(body)` synchronously records the call and + * asynchronously invokes the response callback with a fake `IncomingMessage` + * (also an `EventEmitter`) carrying `statusCode`. Honors `options.signal` so + * a short injected timeout can be tested for real (no fake timers needed — + * see the timeout test below). Captures every call for assertions. + */ +function fakeHttpsRequestImpl( + behavior: (url: URL, options: unknown, body: Buffer) => { status: number } | { hang: true }, +): { requestImpl: HttpsRequestFn; calls: { url: URL; options: unknown; body: Buffer }[] } { + const calls: { url: URL; options: unknown; body: Buffer }[] = [] + const requestImpl = ((url: unknown, options: unknown, callback: unknown) => { + const req = new EventEmitter() as unknown as { + on: EventEmitter['on'] + end: (body: Buffer) => void + } + const opts = options as { signal?: AbortSignal } + if (opts.signal !== undefined) { + opts.signal.addEventListener('abort', () => { + ;(req as unknown as EventEmitter).emit('error', new Error('The operation was aborted')) + }) + } + ;(req as unknown as { end: (body: Buffer) => void }).end = (body: Buffer) => { + calls.push({ url: url as URL, options, body }) + const outcome = behavior(url as URL, options, body) + if ('hang' in outcome) return // never calls back — only the abort listener above settles it + queueMicrotask(() => { + type FakeResponse = EventEmitter & { statusCode: number; resume: () => void } + const res = new EventEmitter() as FakeResponse + res.statusCode = outcome.status + res.resume = () => {} + ;(callback as (res: FakeResponse) => void)(res) + queueMicrotask(() => res.emit('end')) + }) + } + return req + }) as unknown as HttpsRequestFn + return { requestImpl, calls } +} + +describe('sendWebhookRequest', () => { + it('refuses a non-https URL WITHOUT calling resolveSafeAddress or the transport', async () => { + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 200 })) + let resolveSafeAddressCalled = false + await expect( + sendWebhookRequest( + 'http://example.test/hook', + '{}', + {}, + { + resolveSafeAddress: async () => { + resolveSafeAddressCalled = true + return { address: '1.2.3.4', family: 4 } + }, + requestImpl, + }, + ), + ).rejects.toBeInstanceOf(SsrfRefusedError) + expect(resolveSafeAddressCalled).toBe(false) + expect(calls).toHaveLength(0) + }) + + it('propagates SsrfRefusedError from resolveSafeAddress WITHOUT calling the transport', async () => { + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 200 })) + await expect( + sendWebhookRequest( + 'https://evil.test/hook', + '{}', + {}, + { + resolveSafeAddress: async () => { + throw new SsrfRefusedError('nope') + }, + requestImpl, + }, + ), + ).rejects.toBeInstanceOf(SsrfRefusedError) + expect(calls).toHaveLength(0) + }) + + it('POSTs the body and returns the response status only', async () => { + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 204 })) + const result = await sendWebhookRequest( + 'https://example.test/hook', + '{"hello":"world"}', + { 'X-Test': 'yes' }, + { resolveSafeAddress: fakeResolve(), requestImpl }, + ) + expect(result).toEqual({ status: 204 }) + expect(calls).toHaveLength(1) + expect(calls[0].url.hostname).toBe('example.test') + expect(calls[0].body.toString('utf8')).toBe('{"hello":"world"}') + const options = calls[0].options as { method: string; headers: Record } + expect(options.method).toBe('POST') + expect(options.headers['X-Test']).toBe('yes') + expect(options.headers['Content-Type']).toBe('application/json') + }) + + it('pins the connection to the resolved address via the lookup option (resolve-then-connect)', async () => { + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 200 })) + await sendWebhookRequest( + 'https://pinned.test/hook', + '{}', + {}, + { + resolveSafeAddress: fakeResolve('198.51.100.42', 4), + requestImpl, + }, + ) + const options = calls[0].options as { + lookup: (h: string, o: { all: boolean }, cb: (...a: unknown[]) => void) => void + } + const captured: unknown[] = [] + options.lookup('pinned.test', { all: true }, (...args: unknown[]) => captured.push(args)) + expect(captured[0]).toEqual([null, [{ address: '198.51.100.42', family: 4 }]]) + }) + + it('a non-2xx status is still a resolved response (the CALLER decides retry, not this function)', async () => { + const { requestImpl } = fakeHttpsRequestImpl(() => ({ status: 500 })) + const result = await sendWebhookRequest( + 'https://example.test/hook', + '{}', + {}, + { + resolveSafeAddress: fakeResolve(), + requestImpl, + }, + ) + expect(result).toEqual({ status: 500 }) + }) + + it('rejects once the timeout elapses (a real, short wait — no fake timers)', async () => { + const { requestImpl } = fakeHttpsRequestImpl(() => ({ hang: true })) + await expect( + sendWebhookRequest( + 'https://slow.test/hook', + '{}', + {}, + { + resolveSafeAddress: fakeResolve(), + requestImpl, + timeoutMs: 50, + }, + ), + ).rejects.toThrow() + }) +}) + +// --- createWebhookDeliveryHandler -------------------------------------------- + +function message(job: WebhookDeliveryJob, attempts = 1): QueueMessage { + return { id: 'msg-1', topic: 'webhook.delivery', payload: job, attempts, enqueuedAt: new Date() } +} + +function job(overrides: Partial = {}): WebhookDeliveryJob { + return { + endpointId: overrides.endpointId ?? 'missing', + eventId: 'event-1', + type: 'conversation.created', + occurredAt: '2026-07-18T00:00:00.000Z', + conversationId: 'conv-1', + data: {}, + ...overrides, + } +} + +describe('createWebhookDeliveryHandler', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshStore(): Promise { + db = await createPgliteDb() + await migrate(db) + return createWebhookEndpointStore(db, ENC_KEY) + } + + it('a 2xx response acks and records delivery success', async () => { + const store = await freshStore() + const endpoint = await store.create({ url: 'https://ok.test/hook', secret: 's', events: [] }) + const { requestImpl } = fakeHttpsRequestImpl(() => ({ status: 200 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + const result = await handler(message(job({ endpointId: endpoint.id }))) + + expect(result).toEqual({ kind: 'ack' }) + const updated = await store.list() + expect(updated[0].consecutiveFailures).toBe(0) + }) + + it('a non-2xx response under the attempt ceiling retries WITHOUT touching the failure counter', async () => { + const store = await freshStore() + const endpoint = await store.create({ url: 'https://flaky.test/hook', secret: 's', events: [] }) + // Pre-existing failures, to prove this attempt does NOT bump them. + await store.recordDeliveryFailure(endpoint.id) + const { requestImpl } = fakeHttpsRequestImpl(() => ({ status: 500 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + const result = await handler( + message(job({ endpointId: endpoint.id }), WEBHOOK_DELIVERY_MAX_ATTEMPTS - 1), + ) + + expect(result).toEqual({ kind: 'retry' }) + const updated = await store.list() + expect(updated[0].consecutiveFailures).toBe(1) // unchanged from the pre-seed + }) + + it('a non-2xx response AT the attempt ceiling dead-letters and records exactly one failure', async () => { + const store = await freshStore() + const endpoint = await store.create({ url: 'https://dead.test/hook', secret: 's', events: [] }) + const { requestImpl } = fakeHttpsRequestImpl(() => ({ status: 503 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + const result = await handler( + message(job({ endpointId: endpoint.id }), WEBHOOK_DELIVERY_MAX_ATTEMPTS), + ) + + expect(result.kind).toBe('deadLetter') + const updated = await store.list() + expect(updated[0].consecutiveFailures).toBe(1) + }) + + it('an SSRF refusal dead-letters IMMEDIATELY (attempt 1) and records a failure — never retried', async () => { + const store = await freshStore() + const endpoint = await store.create({ url: 'https://ssrf.test/hook', secret: 's', events: [] }) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { + resolveSafeAddress: async () => { + throw new SsrfRefusedError('refused') + }, + }, + }) + + const result = await handler(message(job({ endpointId: endpoint.id }), 1)) + + expect(result.kind).toBe('deadLetter') + const updated = await store.list() + expect(updated[0].consecutiveFailures).toBe(1) + }) + + it('a deleted endpoint dead-letters without touching the store further', async () => { + const store = await freshStore() + const handler = createWebhookDeliveryHandler({ webhookEndpoints: store }) + + const result = await handler(message(job({ endpointId: 'does-not-exist' }))) + + expect(result.kind).toBe('deadLetter') + }) + + it('a non-active endpoint acks WITHOUT sending or touching the failure counter (race defense)', async () => { + const store = await freshStore() + const endpoint = await store.create({ + url: 'https://paused.test/hook', + secret: 's', + events: [], + }) + await store.patch(endpoint.id, { status: 'disabled' }) + let sent = false + const { requestImpl } = fakeHttpsRequestImpl(() => { + sent = true + return { status: 200 } + }) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + const result = await handler(message(job({ endpointId: endpoint.id }))) + + expect(result).toEqual({ kind: 'ack' }) + expect(sent).toBe(false) + }) + + it("signs the envelope with the endpoint's OWN decrypted secret — a consumer can verify it independently", async () => { + const store = await freshStore() + const endpoint = await store.create({ + url: 'https://verify.test/hook', + secret: 'the-real-secret', + events: [], + }) + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 200 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + const theJob = job({ endpointId: endpoint.id, type: 'conversation.reply_sent' }) + await handler(message(theJob)) + + expect(calls).toHaveLength(1) + const options = calls[0].options as { headers: Record } + expect(options.headers['X-Helpthread-Event']).toBe('conversation.reply_sent') + expect(options.headers['X-Helpthread-Delivery']).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, + ) + const sigHeader = options.headers['X-Helpthread-Signature'] + const match = /^t=(\d+), v1=([0-9a-f]{64})$/.exec(sigHeader) + expect(match).not.toBeNull() + const [, ts, mac] = match as unknown as [string, string, string] + const body = calls[0].body.toString('utf8') + const expectedMac = createHmac('sha256', 'the-real-secret') + .update(`${ts}.${body}`) + .digest('hex') + expect(mac).toBe(expectedMac) + + // Thin envelope, exactly spec §4's shape — no extra keys. + expect(JSON.parse(body)).toEqual({ + eventId: theJob.eventId, + type: theJob.type, + occurredAt: theJob.occurredAt, + conversationId: theJob.conversationId, + data: {}, + }) + }) + + it('a fresh X-Helpthread-Delivery id is minted on EACH invocation (redelivery), even for the same message', async () => { + const store = await freshStore() + const endpoint = await store.create({ + url: 'https://redeliver.test/hook', + secret: 's', + events: [], + }) + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 500 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + await handler(message(job({ endpointId: endpoint.id }), 1)) + await handler(message(job({ endpointId: endpoint.id }), 2)) + + const ids = calls.map( + (c) => (c.options as { headers: Record }).headers['X-Helpthread-Delivery'], + ) + expect(ids[0]).not.toBe(ids[1]) + }) + + it('a test.ping job (conversationId: null) delivers with a null conversationId in the envelope', async () => { + const store = await freshStore() + const endpoint = await store.create({ url: 'https://ping.test/hook', secret: 's', events: [] }) + const { requestImpl, calls } = fakeHttpsRequestImpl(() => ({ status: 200 })) + const handler = createWebhookDeliveryHandler({ + webhookEndpoints: store, + send: { resolveSafeAddress: fakeResolve(), requestImpl }, + }) + + await handler( + message(job({ endpointId: endpoint.id, type: 'test.ping', conversationId: null })), + ) + + const body = JSON.parse(calls[0].body.toString('utf8')) + expect(body.conversationId).toBeNull() + expect(body.type).toBe('test.ping') + }) +}) diff --git a/src/webhooks/delivery.ts b/src/webhooks/delivery.ts new file mode 100644 index 0000000..3bca1ad --- /dev/null +++ b/src/webhooks/delivery.ts @@ -0,0 +1,315 @@ +/** + * The webhook delivery queue consumer (HT-69; specs/modules/substrate-v1.md + * §5's "Delivery" bullet) — one `QueueMessageHandler` + * that POSTs a signed event envelope to one endpoint and reports the + * outcome back to `QueueProvider` (`src/providers/queue.ts`) and to the + * endpoint's own failure/success counters (`WebhookEndpointStore`, + * `src/store/webhook-endpoints.ts`). + * + * ## Where a `WebhookDeliveryJob` comes from + * + * Every job on {@link WEBHOOK_DELIVERY_TOPIC} was enqueued either by the + * outbox drain (`./outbox-drain.ts`, one job per matching active endpoint + * per real domain event) or by the admin `POST /api/v1/webhooks/{id}/test` + * handler (`src/api/webhooks.ts`, one synthetic `test.ping` job addressed + * to the one endpoint under test, bypassing `event_outbox` entirely — spec + * §4: "test.ping is a synthetic type fired only by the test endpoint"). This + * handler treats both origins identically: it has no idea which produced + * the job it's holding, and doesn't need to. + * + * ## Envelope, headers, signature (spec §4's JSON shape, §5's headers) + * + * The JSON body is exactly spec §4's envelope: `eventId`, `type`, + * `occurredAt`, `conversationId`, `data`. `conversationId` is `null` only + * for `test.ping` (not tied to any conversation — see + * `src/webhooks/event-types.ts`'s doc comment); every real domain event + * always carries one (`event_outbox.conversation_id` is `NOT NULL`). + * + * `X-Helpthread-Delivery` is freshly minted on EVERY invocation of this + * handler — including a queue-driven redelivery of the SAME + * `QueueMessage.id` — because spec §5 requires it to "differ per attempt"; + * `eventId` (in the body, and dedupe-key material at enqueue time) is the + * stable identity a consumer dedupes on, not this header. + * + * `X-Helpthread-Signature` is signed over `${unixTimestamp}.${body}` with + * HMAC-SHA256 under the endpoint's plaintext secret (decrypted per-call via + * `WebhookEndpointStore.getSecret` — never cached across deliveries, the + * same "fetch fresh, don't cache a decrypted secret" posture + * `gmail-oauth.ts`'s token service already uses for OAuth tokens). + * + * ## Retry vs. dead-letter is THIS handler's decision, not the queue's + * + * `QueueProvider`'s generic retry/backoff (`createPostgresQueue`) has no + * notion of "endpoint" or "consecutive failure count" — it is + * topic-agnostic infra. So this handler tracks its own attempt ceiling + * ({@link WEBHOOK_DELIVERY_MAX_ATTEMPTS}, matching the queue's own factory + * default so the two layers agree) against `QueueMessage.attempts`: while + * under the ceiling, a failed attempt returns `{ kind: 'retry' }` with NO + * store write (spec: "consecutive-failure counter increments... at the + * threshold" describes `WebhookEndpointStore.recordDeliveryFailure`'s OWN + * per-EVENT counter, which must not be double-incremented per HTTP attempt); + * once `attempts` reaches the ceiling, THIS attempt calls + * `recordDeliveryFailure` and returns `{ kind: 'deadLetter' }` itself, + * rather than returning a bare `retry` and hoping the queue's own + * (unrelated) ceiling eventually dead-letters the row without ever touching + * the store. An SSRF refusal ({@link SsrfRefusedError}) is dead-lettered + * IMMEDIATELY regardless of `attempts` — retrying can never change what a + * hostname is configured to resolve to, so burning the retry budget on it + * only delays the operator-visible signal. + * + * A 2xx response is the only success: `recordDeliverySuccess` + `ack`. + * Everything else — non-2xx (including a 3xx, since redirects are never + * followed — spec §5), a timeout, a connection error — is a failed + * attempt, handled by the retry/dead-letter branch above. + */ + +import { createHmac, randomUUID } from 'node:crypto' +import { request as httpsRequest, type RequestOptions } from 'node:https' +import type { QueueHandlerResult, QueueMessage, QueueMessageHandler } from '../providers/queue.js' +import type { WebhookEndpointStore } from '../store/webhook-endpoints.js' +import { resolveSafeAddress, SsrfRefusedError } from './ssrf.js' + +/** The queue topic every webhook delivery is enqueued on (both the outbox drain and the admin test endpoint). */ +export const WEBHOOK_DELIVERY_TOPIC = 'webhook.delivery' + +/** + * Attempts (`QueueMessage.attempts`, 1-indexed) at which this handler stops + * retrying and dead-letters the delivery itself (module doc). Matches + * `createPostgresQueue`'s own `DEFAULT_MAX_ATTEMPTS` (5) — not imported + * from there (that constant is private to the adapter), chosen to agree + * with it so the two independent ceilings line up rather than one firing + * before the other. + */ +export const WEBHOOK_DELIVERY_MAX_ATTEMPTS = 5 + +/** Hard deadline for the whole HTTP exchange — connect through response — per spec §5. */ +export const WEBHOOK_DELIVERY_TIMEOUT_MS = 10_000 + +/** The payload every `WEBHOOK_DELIVERY_TOPIC` job carries — enough to rebuild spec §4's envelope and address one endpoint, with nothing else re-fetched from `event_outbox` at delivery time (the outbox row may already be marked dispatched by then). */ +export interface WebhookDeliveryJob { + endpointId: string + eventId: string + type: string + /** ISO-8601 — `StoredOutboxEvent.occurredAt` serialized once at fan-out time (`./outbox-drain.ts`), never re-derived here. */ + occurredAt: string + /** `null` only for a synthetic `test.ping` (module doc). */ + conversationId: string | null + data: Record +} + +/** Spec §4's envelope — the exact JSON body of every delivery. */ +interface WebhookEnvelope { + eventId: string + type: string + occurredAt: string + conversationId: string | null + data: Record +} + +function envelopeFor(job: WebhookDeliveryJob): WebhookEnvelope { + return { + eventId: job.eventId, + type: job.type, + occurredAt: job.occurredAt, + conversationId: job.conversationId, + data: job.data, + } +} + +/** + * Compute spec §5's `X-Helpthread-Signature` value: + * `t=, v1=`. Exported so + * a test can independently recompute and verify it against a captured + * delivery (this ticket's brief: "a consumer-side verification in tests"). + */ +export function signWebhookPayload(secret: string, body: string, timestampSeconds: number): string { + const mac = createHmac('sha256', secret).update(`${timestampSeconds}.${body}`).digest('hex') + return `t=${timestampSeconds}, v1=${mac}` +} + +/** The one piece of information this handler needs back from the wire — never the body (spec has no use for it, and discarding it lets a slow/huge response never block the handler). */ +export interface WebhookHttpResponse { + status: number +} + +/** The `node:https.request`-shaped seam {@link sendWebhookRequest} calls — injectable so tests can exercise retry/ack/signature logic against a fake transport with zero real sockets, network, or TLS involved. */ +export type HttpsRequestFn = typeof httpsRequest + +/** + * POST `body` to `url` with `headers`, resolve-then-connect SSRF-pinned + * (`./ssrf.ts`), 10s hard deadline, redirects never followed (`node:https`' + * low-level `request` never auto-follows a redirect — there is no + * "disable following" flag to set because there is nothing to disable). + * Throws {@link SsrfRefusedError} for a non-`https:` URL or an unsafe + * resolved address; otherwise resolves with the response status only (the + * body is drained and discarded, never read). + */ +export async function sendWebhookRequest( + url: string, + body: string, + headers: Record, + deps: { + resolveSafeAddress?: typeof resolveSafeAddress + requestImpl?: HttpsRequestFn + timeoutMs?: number + } = {}, +): Promise { + const parsed = new URL(url) + if (parsed.protocol !== 'https:') { + throw new SsrfRefusedError(`webhook url must be https: — got '${parsed.protocol}//...'`) + } + + const resolve = deps.resolveSafeAddress ?? resolveSafeAddress + const pinned = await resolve(parsed.hostname) + const timeoutMs = deps.timeoutMs ?? WEBHOOK_DELIVERY_TIMEOUT_MS + const bodyBuffer = Buffer.from(body, 'utf8') + + const requestImpl = deps.requestImpl ?? httpsRequest + + const options: RequestOptions = { + method: 'POST', + headers: { + ...headers, + 'Content-Type': 'application/json', + 'Content-Length': bodyBuffer.length, + }, + signal: AbortSignal.timeout(timeoutMs), + // Resolve-then-connect pinning (`./ssrf.ts`'s module doc): always hand + // back the ALREADY-VALIDATED address, ignoring whatever hostname Node's + // connector re-passes in. Node's http/net internals call this with + // `options.all: true` and expect an ARRAY-shaped callback in that case + // — verified live against the installed Node version (this ticket's + // report); the single-address form is kept too for a caller that asks + // without `all`. + lookup: (_hostname, lookupOptions, callback) => { + if (typeof lookupOptions === 'function') { + ;(lookupOptions as unknown as (err: null, address: string, family: number) => void)( + null, + pinned.address, + pinned.family, + ) + return + } + if (lookupOptions.all) { + callback(null, [{ address: pinned.address, family: pinned.family }]) + return + } + callback(null, pinned.address, pinned.family) + }, + } + + return new Promise((resolvePromise, reject) => { + const req = requestImpl(parsed, options, (res) => { + // Only the status matters (module doc) — drain the body without + // parsing it so the socket is released and a large/slow response body + // can never block or OOM this handler. + res.resume() + res.on('end', () => resolvePromise({ status: res.statusCode ?? 0 })) + res.on('error', reject) + }) + req.on('error', reject) + req.end(bodyBuffer) + }) +} + +/** Dependencies {@link createWebhookDeliveryHandler} needs. */ +export interface WebhookDeliveryHandlerDeps { + webhookEndpoints: WebhookEndpointStore + /** Overrides for {@link sendWebhookRequest} — tests inject a fake transport here; production leaves it unset (real DNS + `node:https`). */ + send?: Parameters[3] +} + +/** + * Build the `QueueMessageHandler` registered for + * {@link WEBHOOK_DELIVERY_TOPIC}. See the module doc for the full retry/ + * dead-letter/signature contract. + */ +export function createWebhookDeliveryHandler( + deps: WebhookDeliveryHandlerDeps, +): QueueMessageHandler { + return async (message: QueueMessage): Promise => { + const job = message.payload + + // Both the endpoint's URL and its secret are read FRESH on every + // attempt, never cached from an earlier attempt at the same job — an + // admin editing the URL or rotating the secret mid-retry should have + // the NEXT attempt reflect that, not a stale value. + const endpoints = await deps.webhookEndpoints.list() + const target = endpoints.find((e) => e.id === job.endpointId) + if (target === undefined) { + // The endpoint was deleted between fan-out (or the test click) and + // this delivery attempt — there is no row left to record success or + // failure against, and there never will be on a later retry either. + // Not a transient condition: dead-letter immediately. + return { kind: 'deadLetter', reason: `webhook endpoint ${job.endpointId} no longer exists` } + } + if (target.status !== 'active') { + // The endpoint was disabled (manually, or auto-disabled) between + // fan-out and this attempt — a harmless drop, not a failure: nobody + // is harmed by not delivering to an endpoint that no longer wants + // deliveries, so this both skips the send and leaves the + // consecutive-failure counter untouched (only a genuine send failure + // should move it). The admin test endpoint (`src/api/webhooks.ts`) + // refuses to enqueue against a non-active endpoint in the first + // place, so this branch is defense against the race window, not the + // primary gate. + return { kind: 'ack' } + } + const secret = await deps.webhookEndpoints.getSecret(job.endpointId) + if (secret === null) { + // Deleted in the gap between the list() above and this read — same + // "gone, not transient" reasoning as the check above. + return { kind: 'deadLetter', reason: `webhook endpoint ${job.endpointId} no longer exists` } + } + + const body = JSON.stringify(envelopeFor(job)) + const timestampSeconds = Math.floor(Date.now() / 1000) + const headers = { + 'X-Helpthread-Event': job.type, + 'X-Helpthread-Delivery': randomUUID(), + 'X-Helpthread-Signature': signWebhookPayload(secret, body, timestampSeconds), + } + + try { + const response = await sendWebhookRequest(target.url, body, headers, deps.send) + if (response.status >= 200 && response.status < 300) { + await deps.webhookEndpoints.recordDeliverySuccess(job.endpointId) + return { kind: 'ack' } + } + return failOrRetry( + deps, + job.endpointId, + message.attempts, + `webhook delivery to ${target.url} failed with HTTP ${response.status}`, + ) + } catch (err) { + if (err instanceof SsrfRefusedError) { + // Never retryable (module doc) — dead-letter on the FIRST occurrence + // regardless of `message.attempts`. + await deps.webhookEndpoints.recordDeliveryFailure(job.endpointId) + return { kind: 'deadLetter', reason: err.message } + } + const detail = err instanceof Error ? err.message : String(err) + return failOrRetry( + deps, + job.endpointId, + message.attempts, + `webhook delivery to ${target.url} failed: ${detail}`, + ) + } + } +} + +/** Shared tail of both HTTP-failure branches above: retry under the ceiling, dead-letter (with the one `recordDeliveryFailure` write) at it — module doc's "Retry vs. dead-letter" section. */ +async function failOrRetry( + deps: WebhookDeliveryHandlerDeps, + endpointId: string, + attempts: number, + reason: string, +): Promise { + if (attempts < WEBHOOK_DELIVERY_MAX_ATTEMPTS) { + return { kind: 'retry' } + } + await deps.webhookEndpoints.recordDeliveryFailure(endpointId) + return { kind: 'deadLetter', reason } +} diff --git a/src/webhooks/event-types.test.ts b/src/webhooks/event-types.test.ts new file mode 100644 index 0000000..89a4029 --- /dev/null +++ b/src/webhooks/event-types.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest' +import { EVENT_TYPES, isEventType, TEST_PING_EVENT_TYPE } from './event-types.js' + +describe('isEventType', () => { + it('accepts every spec §4 vocabulary entry', () => { + for (const type of EVENT_TYPES) { + expect(isEventType(type)).toBe(true) + } + }) + + it('rejects test.ping — a synthetic type, never a subscribable filter value', () => { + expect(isEventType(TEST_PING_EVENT_TYPE)).toBe(false) + }) + + it('rejects an unknown string', () => { + expect(isEventType('conversation.bogus')).toBe(false) + expect(isEventType('')).toBe(false) + }) +}) diff --git a/src/webhooks/event-types.ts b/src/webhooks/event-types.ts new file mode 100644 index 0000000..8ca27fb --- /dev/null +++ b/src/webhooks/event-types.ts @@ -0,0 +1,42 @@ +/** + * The closed event-type vocabulary (HT-69; specs/modules/substrate-v1.md + * §4's vocabulary table). Additions are spec amendments, not a runtime + * config surface — this list is the one place both the admin API's `events` + * filter validation (`src/api/webhooks.ts`) and any future emission call + * site can check a type string against the spec'd set. + * + * `event_outbox.type` and `webhook_endpoints.events` are both untyped + * `text`/`jsonb` at the storage layer (`src/store/event-outbox.ts`, + * `src/store/webhook-endpoints.ts` — "this store does not validate against + * that list; the caller is the only writer of event types"), so this module + * is that caller-side validation, not a schema constraint. + * + * `draft.created`/`draft.resolved` are listed here even though HT-69 does + * not emit them (wave 3 owns `appendDraft`/`resolveDraft`, spec §6) — an + * admin registering a webhook today may legally subscribe to them ahead of + * time (spec §1's additive-forward rule: the substrate's surface is + * complete even where an individual emission call site lands in a later + * wave), and no schema change is needed when wave 3 starts firing them. + */ + +/** `test.ping` is a synthetic type — never persisted to `event_outbox`, fired only by `POST /api/v1/webhooks/{id}/test` (spec §5) directly through the delivery queue. Kept OUT of {@link EVENT_TYPES}/{@link isEventType}: it is not a subscribable filter value (a `test.ping` delivery always targets the one endpoint being tested, regardless of its `events` filter — see `src/api/webhooks.ts`'s `handleTestWebhook`), so it must never appear in a stored endpoint's `events` array. */ +export const TEST_PING_EVENT_TYPE = 'test.ping' + +/** Every real domain event type spec §4's vocabulary table lists, in table order. */ +export const EVENT_TYPES = [ + 'conversation.created', + 'conversation.message_received', + 'conversation.reply_sent', + 'conversation.status_changed', + 'conversation.tags_changed', + 'conversation.assignee_changed', + 'draft.created', + 'draft.resolved', +] as const + +export type EventType = (typeof EVENT_TYPES)[number] + +/** Is `value` one of {@link EVENT_TYPES}? The narrowing guard `src/api/webhooks.ts` uses to validate a `POST`/`PATCH` body's `events` array. Deliberately excludes {@link TEST_PING_EVENT_TYPE} — see its own doc comment. */ +export function isEventType(value: string): value is EventType { + return (EVENT_TYPES as readonly string[]).includes(value) +} diff --git a/src/webhooks/outbox-drain.test.ts b/src/webhooks/outbox-drain.test.ts new file mode 100644 index 0000000..959aab0 --- /dev/null +++ b/src/webhooks/outbox-drain.test.ts @@ -0,0 +1,187 @@ +import { randomBytes } from 'node:crypto' +import { afterEach, describe, expect, it } from 'vitest' +import { createPgliteDb, type Db } from '../db/client.js' +import { migrate } from '../db/migrate.js' +import type { EnqueueOptions, QueueProvider } from '../providers/queue.js' +import { + appendOutboxEventInTx, + createEventOutboxStore, + type EventOutboxStore, +} from '../store/event-outbox.js' +import { ENCRYPTION_KEY_BYTES } from '../store/token-crypto.js' +import { + createWebhookEndpointStore, + type WebhookEndpointStore, +} from '../store/webhook-endpoints.js' +import { WEBHOOK_DELIVERY_TOPIC, type WebhookDeliveryJob } from './delivery.js' +import { drainEventOutbox } from './outbox-drain.js' + +const KEY = randomBytes(ENCRYPTION_KEY_BYTES) + +/** A `QueueProvider` fake that records every enqueue call, never touching a real queue. */ +function fakeQueue(): { + queue: QueueProvider + enqueued: { topic: string; payload: unknown; opts?: EnqueueOptions }[] +} { + const enqueued: { topic: string; payload: unknown; opts?: EnqueueOptions }[] = [] + return { + queue: { + async enqueue(topic, payload, opts) { + enqueued.push({ topic, payload, opts }) + }, + }, + enqueued, + } +} + +describe('drainEventOutbox', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function fresh(): Promise<{ + db: Db + eventOutbox: EventOutboxStore + webhookEndpoints: WebhookEndpointStore + }> { + db = await createPgliteDb() + await migrate(db) + return { + db, + eventOutbox: createEventOutboxStore(db), + webhookEndpoints: createWebhookEndpointStore(db, KEY), + } + } + + async function insertConversation(database: Db): Promise { + const rows = await database.query<{ id: string }>( + "INSERT INTO conversations (customer_email) VALUES ('customer@example.test') RETURNING id", + ) + return rows[0].id + } + + it('an empty outbox drains to a report of all zeros', async () => { + const { eventOutbox, webhookEndpoints } = await fresh() + const { queue } = fakeQueue() + + const report = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + + expect(report).toEqual({ claimed: 0, enqueued: 0, dispatched: 0 }) + }) + + it('fans one event out to every matching ACTIVE endpoint, dedupeKey = eventId:endpointId, and marks the event dispatched', async () => { + const { db, eventOutbox, webhookEndpoints } = await fresh() + const conversationId = await insertConversation(db) + await db.transaction((tx) => + appendOutboxEventInTx(tx, { + type: 'conversation.created', + conversationId, + data: {}, + }), + ) + const matchAll = await webhookEndpoints.create({ + url: 'https://all.example.test/hook', + secret: 's1', + events: [], // [] means "all" (spec §5) + }) + const matchType = await webhookEndpoints.create({ + url: 'https://match.example.test/hook', + secret: 's2', + events: ['conversation.created', 'conversation.reply_sent'], + }) + await webhookEndpoints.create({ + url: 'https://nomatch.example.test/hook', + secret: 's3', + events: ['conversation.reply_sent'], // does NOT include conversation.created + }) + const { queue, enqueued } = fakeQueue() + + const report = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + + expect(report).toEqual({ claimed: 1, enqueued: 2, dispatched: 1 }) + expect(enqueued).toHaveLength(2) + const endpointIds = enqueued.map((e) => (e.payload as WebhookDeliveryJob).endpointId).sort() + expect(endpointIds).toEqual([matchAll.id, matchType.id].sort()) + for (const e of enqueued) { + expect(e.topic).toBe(WEBHOOK_DELIVERY_TOPIC) + const payload = e.payload as WebhookDeliveryJob + expect(payload.type).toBe('conversation.created') + expect(payload.conversationId).toBe(conversationId) + expect(e.opts?.dedupeKey).toBe(`${payload.eventId}:${payload.endpointId}`) + } + + // Claimed row is dispatched — a second drain finds nothing left to claim. + const secondReport = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + expect(secondReport).toEqual({ claimed: 0, enqueued: 0, dispatched: 0 }) + }) + + it('an endpoint that is disabled or auto_disabled never receives a fan-out, even if its events filter matches', async () => { + const { db, eventOutbox, webhookEndpoints } = await fresh() + const conversationId = await insertConversation(db) + await db.transaction((tx) => + appendOutboxEventInTx(tx, { type: 'conversation.created', conversationId, data: {} }), + ) + const disabled = await webhookEndpoints.create({ + url: 'https://disabled.example.test/hook', + secret: 's', + events: [], + }) + await webhookEndpoints.patch(disabled.id, { status: 'disabled' }) + const { queue, enqueued } = fakeQueue() + + const report = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + + // Still claimed and dispatched — there was simply nothing to fan out to + // (module doc: "the outbox's job is complete either way"). + expect(report).toEqual({ claimed: 1, enqueued: 0, dispatched: 1 }) + expect(enqueued).toHaveLength(0) + }) + + it('an event with zero matching endpoints is still marked dispatched (never re-claimed)', async () => { + const { db, eventOutbox, webhookEndpoints } = await fresh() + const conversationId = await insertConversation(db) + await db.transaction((tx) => + appendOutboxEventInTx(tx, { + type: 'conversation.tags_changed', + conversationId, + data: { tags: [] }, + }), + ) + // No endpoints registered at all. + const { queue } = fakeQueue() + + const first = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + expect(first).toEqual({ claimed: 1, enqueued: 0, dispatched: 1 }) + + const second = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }) + expect(second).toEqual({ claimed: 0, enqueued: 0, dispatched: 0 }) + }) + + it('multiple events each fan out independently, respecting the batch size', async () => { + const { db, eventOutbox, webhookEndpoints } = await fresh() + const conversationId = await insertConversation(db) + for (const type of [ + 'conversation.created', + 'conversation.message_received', + 'conversation.reply_sent', + ]) { + await db.transaction((tx) => appendOutboxEventInTx(tx, { type, conversationId, data: {} })) + } + await webhookEndpoints.create({ url: 'https://all.example.test/hook', secret: 's', events: [] }) + const { queue, enqueued } = fakeQueue() + + const report = await drainEventOutbox( + { eventOutbox, webhookEndpoints, queue }, + { batchSize: 2 }, + ) + + expect(report).toEqual({ claimed: 2, enqueued: 2, dispatched: 2 }) + expect(enqueued).toHaveLength(2) + + const rest = await drainEventOutbox({ eventOutbox, webhookEndpoints, queue }, { batchSize: 2 }) + expect(rest).toEqual({ claimed: 1, enqueued: 1, dispatched: 1 }) + }) +}) diff --git a/src/webhooks/outbox-drain.ts b/src/webhooks/outbox-drain.ts new file mode 100644 index 0000000..9c35d74 --- /dev/null +++ b/src/webhooks/outbox-drain.ts @@ -0,0 +1,144 @@ +/** + * The outbox drain (HT-69; specs/modules/substrate-v1.md §4: "A drain step + * — the existing queue/cron drain pattern — turns outbox rows into + * `QueueProvider` deliveries"). Mirrors `createPostgresQueue.drainOnce`'s + * own shape (`src/providers/adapters/postgres-queue/index.ts`): a periodic + * cron tick calls {@link drainEventOutbox} to pull and process one bounded + * batch, the same "Postgres itself has no way to push, so a cron tick + * drains" reasoning that adapter's module doc already states. + * + * ## Fan-out, not forward + * + * `EventOutboxStore.claimBatch` (`src/store/event-outbox.ts`) hands back + * one row per domain event — but a single event may have MULTIPLE + * subscribers (every `webhook_endpoints` row whose `events` filter matches, + * spec §5: "subset filter... or all"). This function fans each claimed + * event out to one `QueueProvider.enqueue` call PER matching active + * endpoint, `dedupeKey = ` `` `${eventId}:${endpointId}` `` — per-pair, not + * per-event (migration 023's doc comment on `event_outbox`, `src/db/ + * migrate.ts`, states this same fan-out key), because the SAME event fans + * to several endpoints and each needs its own independent delivery/retry + * lifecycle in `queue_jobs`: a single `dedupeKey` shared across every + * endpoint would collide the first enqueue against all the others for the + * same event and silently drop delivery to every endpoint but the first. + * + * An event with ZERO matching active endpoints (no subscriber cares, or + * every matching endpoint is `disabled`/`auto_disabled`) is still marked + * dispatched — the outbox's job (handing off to the queue) is complete + * either way; there is no queue work left to do for it, and leaving it + * undispatched would only make the next drain re-fetch and re-decide the + * same "nobody's listening" outcome forever. + * + * ## `test.ping` never touches this module + * + * The admin `POST /api/v1/webhooks/{id}/test` handler (`src/api/webhooks.ts`) + * enqueues its synthetic delivery directly, bypassing `event_outbox` + * entirely (spec §4: "test.ping is a synthetic type fired only by the test + * endpoint") — this module only ever drains REAL domain events. + */ + +import type { QueueProvider } from '../providers/queue.js' +import type { EventOutboxStore, StoredOutboxEvent } from '../store/event-outbox.js' +import type { WebhookEndpointStore } from '../store/webhook-endpoints.js' +import { WEBHOOK_DELIVERY_TOPIC, type WebhookDeliveryJob } from './delivery.js' + +/** Default cap on undispatched outbox rows claimed per {@link drainEventOutbox} call — a bound on one invocation's work, mirroring `createPostgresQueue`'s own `DEFAULT_BATCH_SIZE`. */ +const DEFAULT_BATCH_SIZE = 50 + +/** Default lease held on a claimed-but-not-yet-dispatched outbox row (`EventOutboxStore.claimBatch`'s `leaseMs`) — generous relative to how long fan-out + N enqueues should ever take, mirroring `EventOutboxStore`'s own module doc precedent (`markDispatched` is the terminal write; nothing here needs a short lease). */ +const DEFAULT_LEASE_MS = 60_000 + +/** Dependencies {@link drainEventOutbox} needs for one drain pass. */ +export interface OutboxDrainDeps { + eventOutbox: EventOutboxStore + webhookEndpoints: WebhookEndpointStore + queue: QueueProvider +} + +/** Tuning knobs for one drain pass; both default, so `drainEventOutbox(deps)` alone is a complete, reasonable call. */ +export interface OutboxDrainOptions { + batchSize?: number + leaseMs?: number +} + +/** What one {@link drainEventOutbox} call did, for logging/observability by whatever schedules it (mirrors `DrainReport`'s shape, `src/providers/adapters/postgres-queue/index.ts`). */ +export interface OutboxDrainReport { + /** Outbox rows claimed (leased) this pass. */ + claimed: number + /** `(event, endpoint)` pairs enqueued onto {@link WEBHOOK_DELIVERY_TOPIC} — may exceed `claimed` (fan-out) or be `0` (every claimed event had no matching active endpoint). */ + enqueued: number + /** Claimed events marked dispatched — always equals `claimed` (module doc: every claimed row is marked, whether it fanned to 0 or many endpoints). */ + dispatched: number +} + +/** Does `endpoint`'s `events` filter (spec §5: "subset filter... or all", `[]` meaning all) match `eventType`? */ +function endpointMatches(endpointEvents: string[], eventType: string): boolean { + return endpointEvents.length === 0 || endpointEvents.includes(eventType) +} + +/** + * Fan `event` out to every ACTIVE endpoint whose `events` filter matches + * it, one `queue.enqueue` call per match. Returns how many were enqueued + * (for {@link OutboxDrainReport.enqueued}). + */ +async function fanOutEvent( + event: StoredOutboxEvent, + endpoints: { id: string; events: string[]; status: string }[], + queue: QueueProvider, +): Promise { + const matches = endpoints.filter( + (e) => e.status === 'active' && endpointMatches(e.events, event.type), + ) + for (const endpoint of matches) { + const job: WebhookDeliveryJob = { + endpointId: endpoint.id, + eventId: event.eventId, + type: event.type, + occurredAt: event.occurredAt.toISOString(), + conversationId: event.conversationId, + data: event.data, + } + await queue.enqueue(WEBHOOK_DELIVERY_TOPIC, job, { + dedupeKey: `${event.eventId}:${endpoint.id}`, + }) + } + return matches.length +} + +/** + * Run one outbox drain pass: claim a batch of undispatched `event_outbox` + * rows, fan each out to its matching active endpoints (module doc), then + * mark every claimed row dispatched. See the module doc for the full + * fan-out and dispatched-regardless-of-match-count contract. + * + * The endpoint roster is fetched ONCE per call, up front, and reused for + * every claimed event in this batch — not re-fetched per event. Endpoint + * registration is expected to be low-cardinality (v1 has no marketplace + * scale yet, spec §1's non-goals), so this is simplicity-first, matching + * this substrate's own "v1 simplicity" posture (`WebhookEndpointStore`'s + * own module doc uses the same "fixed... not a scopes system" reasoning + * for assistants). A registration change mid-drain is picked up on the + * NEXT drain tick, one minute later at most on the deployed cron cadence. + */ +export async function drainEventOutbox( + deps: OutboxDrainDeps, + options?: OutboxDrainOptions, +): Promise { + const batchSize = options?.batchSize ?? DEFAULT_BATCH_SIZE + const leaseMs = options?.leaseMs ?? DEFAULT_LEASE_MS + + const claimed = await deps.eventOutbox.claimBatch({ batchSize, leaseMs }) + if (claimed.length === 0) { + return { claimed: 0, enqueued: 0, dispatched: 0 } + } + + const endpoints = await deps.webhookEndpoints.list() + + let enqueued = 0 + for (const event of claimed) { + enqueued += await fanOutEvent(event, endpoints, deps.queue) + await deps.eventOutbox.markDispatched(event.eventId) + } + + return { claimed: claimed.length, enqueued, dispatched: claimed.length } +} diff --git a/src/webhooks/ssrf.test.ts b/src/webhooks/ssrf.test.ts new file mode 100644 index 0000000..01d8e58 --- /dev/null +++ b/src/webhooks/ssrf.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, it } from 'vitest' +import { + expandIpv6, + isDisallowedIpv4, + isDisallowedIpv6, + type LookupAllFn, + resolveSafeAddress, + SsrfRefusedError, +} from './ssrf.js' + +describe('isDisallowedIpv4', () => { + it.each([ + ['0.0.0.0', true], + ['10.0.0.1', true], + ['10.255.255.255', true], + ['100.64.0.1', true], + ['100.127.255.255', true], + ['127.0.0.1', true], + ['169.254.1.1', true], + ['172.16.0.1', true], + ['172.31.255.255', true], + ['192.0.0.1', true], + ['192.0.2.1', true], + ['192.88.99.1', true], + ['192.168.1.1', true], + ['198.18.0.1', true], + ['198.51.100.1', true], + ['203.0.113.1', true], + ['224.0.0.1', true], + ['240.0.0.1', true], + ['255.255.255.255', true], + ])('%s is disallowed', (ip, expected) => { + expect(isDisallowedIpv4(ip)).toBe(expected) + }) + + it.each([ + ['8.8.8.8', false], + ['1.1.1.1', false], + ['172.15.255.255', false], // just below the 172.16.0.0/12 block + ['172.32.0.0', false], // just above it + ['100.63.255.255', false], // just below 100.64.0.0/10 + ['100.128.0.0', false], // just above it + ])('%s is allowed', (ip, expected) => { + expect(isDisallowedIpv4(ip)).toBe(expected) + }) +}) + +describe('expandIpv6', () => { + it('expands a fully-written address', () => { + expect(expandIpv6('2001:0db8:0000:0000:0000:0000:0000:0001')).toEqual([ + 0x2001, 0x0db8, 0, 0, 0, 0, 0, 1, + ]) + }) + + it('expands "::" (unspecified) to eight zero hextets', () => { + expect(expandIpv6('::')).toEqual([0, 0, 0, 0, 0, 0, 0, 0]) + }) + + it('expands "::1" (loopback)', () => { + expect(expandIpv6('::1')).toEqual([0, 0, 0, 0, 0, 0, 0, 1]) + }) + + it('expands a leading-compressed address', () => { + expect(expandIpv6('fe80::1')).toEqual([0xfe80, 0, 0, 0, 0, 0, 0, 1]) + }) + + it('expands a trailing-compressed address', () => { + expect(expandIpv6('fc00::')).toEqual([0xfc00, 0, 0, 0, 0, 0, 0, 0]) + }) + + it('expands an IPv4-embedded tail', () => { + expect(expandIpv6('::ffff:192.168.1.1')).toEqual([0, 0, 0, 0, 0, 0xffff, 0xc0a8, 0x0101]) + }) + + it('throws on a malformed address', () => { + expect(() => expandIpv6('not-an-address')).toThrow() + expect(() => expandIpv6('1:2:3::4:5:6:7:8')).toThrow() // too many groups with :: + expect(() => expandIpv6('gggg::1')).toThrow() // invalid hex + }) +}) + +describe('isDisallowedIpv6', () => { + it.each([ + ['::1', true], // loopback + ['::', true], // unspecified + ['fe80::1', true], // link-local + ['fe80::ffff:ffff:ffff:ffff', true], // still within fe80::/10 + ['fc00::1', true], // unique-local + ['fdff:ffff::1', true], // still within fc00::/7 + ['ff02::1', true], // multicast + ['::ffff:127.0.0.1', true], // IPv4-mapped + ['::ffff:8.8.8.8', true], // IPv4-mapped, even a public embedded address (module doc: blocked outright) + ['64:ff9b::1', true], // NAT64 + ['2001:db8::1', true], // documentation + ['2002:c000:0204::1', true], // 6to4 + ['2001:0:1::1', true], // Teredo + ])('%s is disallowed', (ip, expected) => { + expect(isDisallowedIpv6(ip)).toBe(expected) + }) + + it.each([ + ['2001:4860:4860::8888', false], // Google public DNS + ['2606:4700:4700::1111', false], // Cloudflare public DNS + ['fbff:ffff::1', false], // just below fc00::/7 + ['fe7f:ffff::1', false], // just below fe80::/10 + ])('%s is allowed', (ip, expected) => { + expect(isDisallowedIpv6(ip)).toBe(expected) + }) +}) + +describe('resolveSafeAddress', () => { + function fakeLookup(answers: { address: string; family: number }[]): LookupAllFn { + return async () => answers + } + + it('returns the first answer when every resolved address is safe', async () => { + const lookup = fakeLookup([ + { address: '93.184.216.34', family: 4 }, + { address: '93.184.216.35', family: 4 }, + ]) + const result = await resolveSafeAddress('example.test', { lookup }) + expect(result).toEqual({ address: '93.184.216.34', family: 4 }) + }) + + it('refuses when the ONLY answer is unsafe', async () => { + const lookup = fakeLookup([{ address: '127.0.0.1', family: 4 }]) + await expect(resolveSafeAddress('evil.test', { lookup })).rejects.toBeInstanceOf( + SsrfRefusedError, + ) + }) + + it('refuses when ANY answer is unsafe, even if the first is safe (no round-robin bypass)', async () => { + const lookup = fakeLookup([ + { address: '8.8.8.8', family: 4 }, + { address: '10.0.0.1', family: 4 }, + ]) + await expect(resolveSafeAddress('evil.test', { lookup })).rejects.toBeInstanceOf( + SsrfRefusedError, + ) + }) + + it('refuses when resolution returns no answers', async () => { + const lookup = fakeLookup([]) + await expect(resolveSafeAddress('nowhere.test', { lookup })).rejects.toBeInstanceOf( + SsrfRefusedError, + ) + }) + + it('refuses when the lookup itself throws (e.g. NXDOMAIN)', async () => { + const lookup: LookupAllFn = async () => { + throw new Error('getaddrinfo ENOTFOUND') + } + await expect(resolveSafeAddress('nxdomain.test', { lookup })).rejects.toBeInstanceOf( + SsrfRefusedError, + ) + }) + + it('validates an IPv6 answer using the same disallowed-range rules', async () => { + const lookup = fakeLookup([{ address: '::1', family: 6 }]) + await expect(resolveSafeAddress('evil6.test', { lookup })).rejects.toBeInstanceOf( + SsrfRefusedError, + ) + }) +}) diff --git a/src/webhooks/ssrf.ts b/src/webhooks/ssrf.ts new file mode 100644 index 0000000..10a2d2a --- /dev/null +++ b/src/webhooks/ssrf.ts @@ -0,0 +1,286 @@ +/** + * SSRF defense for the webhook delivery handler (HT-69; specs/modules/ + * substrate-v1.md §5's closing bullet: "the delivery handler refuses URLs + * resolving to private/link-local ranges (impl note: resolve-then-connect + * pinning)"). + * + * ## Resolve-then-connect pinning, and the TOCTOU it closes + * + * A webhook `url`'s hostname is attacker-influenceable (any admin, or in a + * future marketplace world, any module author, can register one) and DNS is + * not trustworthy at connect time: a hostname that resolves to a public IP + * when this module CHECKS it could resolve to `127.0.0.1` or `10.0.0.0/8` + * moments later when the HTTP client actually CONNECTS (a classic DNS- + * rebinding attack). Checking the resolved address and then handing the + * ORIGINAL HOSTNAME to an HTTP client for it to re-resolve independently + * reopens exactly that gap. + * + * {@link resolveSafeAddress} closes it structurally: it resolves the + * hostname itself, validates EVERY returned address (not just the first — + * a round-robin DNS answer could otherwise hide an unsafe address behind a + * safe one), and returns the address to pin. The caller (`./delivery.ts`) + * then hands that EXACT address to `node:https`' `lookup` option, so the + * TCP connection is forced onto the address this module already validated + * — DNS is never consulted a second time, and there is no window between + * "checked" and "connected" for the answer to change. + * + * ## What this checker does and does not cover + * + * Documented honestly, per this ticket's brief: {@link isDisallowedAddress} + * covers the IANA special-purpose registries that matter for SSRF + * (loopback, link-local, RFC 1918 + carrier-grade NAT private ranges, + * multicast, the IPv4-mapped IPv6 `::ffff:0:0/96` block, unique-local IPv6, + * and the common documentation/benchmarking ranges). It does NOT unwrap an + * IPv6 6to4 (`2002::/16`) or Teredo (`2001::/32`) address to inspect the + * IPv4 address embedded in its bits — those whole prefixes are blocked + * outright instead, which is conservative (a legitimate 6to4/Teredo-only + * webhook target would be refused) rather than under-strict. It also + * cannot defend against a target that is a genuinely public IP at + * connect-time but sits behind infrastructure (a reverse proxy, a cloud + * metadata-endpoint alias) that later forwards the request somewhere + * private — that is outside what an SSRF check at THIS layer can ever see, + * and is a residual risk of allowing operator-configured webhook URLs at + * all, not something resolve-then-connect pinning claims to close. + */ + +import { lookup as dnsLookup } from 'node:dns/promises' + +/** Thrown by {@link resolveSafeAddress} — the delivery handler treats this as an immediate, non-retryable dead-letter (retrying can never change what a hostname is configured to resolve to). */ +export class SsrfRefusedError extends Error { + constructor(message: string) { + super(message) + this.name = 'SsrfRefusedError' + } +} + +/** One resolved-and-validated address, ready to pin a connection to. */ +export interface PinnedAddress { + address: string + family: 4 | 6 +} + +// --- IPv4 --------------------------------------------------------------- + +function ipv4ToInt(ip: string): number { + const parts = ip.split('.') + if (parts.length !== 4) throw new Error(`ssrf: not a dotted-quad IPv4 address: ${ip}`) + let value = 0 + for (const part of parts) { + const n = Number(part) + if (!Number.isInteger(n) || n < 0 || n > 255) { + throw new Error(`ssrf: not a dotted-quad IPv4 address: ${ip}`) + } + value = (value << 8) | n + } + return value >>> 0 +} + +interface Ipv4Range { + /** Human label, purely for a refusal message — never load-bearing. */ + label: string + base: string + prefixLength: number +} + +/** + * IANA special-purpose IPv4 registry entries relevant to SSRF (RFC 6890 and + * successors): loopback, the three RFC 1918 private blocks, carrier-grade + * NAT (RFC 6598), link-local (RFC 3927), multicast, the documentation/ + * benchmarking TEST-NET blocks, the deprecated 6to4 relay anycast prefix, + * and the reserved/broadcast top block. + */ +const IPV4_DISALLOWED_RANGES: Ipv4Range[] = [ + { label: 'this-network', base: '0.0.0.0', prefixLength: 8 }, + { label: 'private (RFC 1918)', base: '10.0.0.0', prefixLength: 8 }, + { label: 'carrier-grade NAT (RFC 6598)', base: '100.64.0.0', prefixLength: 10 }, + { label: 'loopback', base: '127.0.0.0', prefixLength: 8 }, + { label: 'link-local (RFC 3927)', base: '169.254.0.0', prefixLength: 16 }, + { label: 'private (RFC 1918)', base: '172.16.0.0', prefixLength: 12 }, + { label: 'IETF protocol assignments', base: '192.0.0.0', prefixLength: 24 }, + { label: 'documentation (TEST-NET-1)', base: '192.0.2.0', prefixLength: 24 }, + { label: '6to4 relay anycast', base: '192.88.99.0', prefixLength: 24 }, + { label: 'private (RFC 1918)', base: '192.168.0.0', prefixLength: 16 }, + { label: 'benchmarking', base: '198.18.0.0', prefixLength: 15 }, + { label: 'documentation (TEST-NET-2)', base: '198.51.100.0', prefixLength: 24 }, + { label: 'documentation (TEST-NET-3)', base: '203.0.113.0', prefixLength: 24 }, + { label: 'multicast', base: '224.0.0.0', prefixLength: 4 }, + { label: 'reserved / broadcast', base: '240.0.0.0', prefixLength: 4 }, +] + +function ipv4InRange(ip: number, range: Ipv4Range): boolean { + const baseInt = ipv4ToInt(range.base) + const mask = range.prefixLength === 0 ? 0 : (0xffffffff << (32 - range.prefixLength)) >>> 0 + return (ip & mask) === (baseInt & mask) +} + +/** Is `ip` (a dotted-quad string) in any disallowed IPv4 range? */ +export function isDisallowedIpv4(ip: string): boolean { + const value = ipv4ToInt(ip) + return IPV4_DISALLOWED_RANGES.some((range) => ipv4InRange(value, range)) +} + +// --- IPv6 ----------------------------------------------------------------- + +/** + * Expand an IPv6 address (RFC 4291 textual form, `::`-compressed or not, + * with an optional trailing IPv4-embedded tail like `::ffff:192.168.1.1`, + * and an optional `%zone` suffix stripped) into its eight 16-bit hextets. + * Deliberately hand-rolled rather than built on `node:net`'s `BlockList`: + * verified live (see this ticket's report) that mixing an IPv4-mapped + * (`::ffff:0:0/96`) subnet rule into a `BlockList` makes EVERY plain IPv4 + * `check()` call return `true` regardless of the address checked — a + * confirmed footgun in the Node version this repo targets, not a + * theoretical concern, so this module owns its own parsing instead. + */ +export function expandIpv6(address: string): number[] { + const withoutZone = address.split('%')[0] + const halves = withoutZone.split('::') + if (halves.length > 2) { + throw new Error(`ssrf: not a valid IPv6 address: ${address}`) + } + + const parseGroups = (part: string): string[] => (part === '' ? [] : part.split(':')) + + /** A trailing dotted-quad group (`::ffff:192.168.1.1`'s `192.168.1.1`) becomes two hex hextets in place — the rest of RFC 4291's textual form is plain hex groups. */ + const expandEmbeddedIpv4Tail = (groups: string[]): string[] => { + if (groups.length === 0) return groups + const last = groups[groups.length - 1] + if (!last.includes('.')) return groups + const octets = last.split('.').map(Number) + if (octets.length !== 4 || octets.some((o) => !Number.isInteger(o) || o < 0 || o > 255)) { + throw new Error(`ssrf: invalid IPv4-embedded tail in IPv6 address: ${address}`) + } + const hi = ((octets[0] << 8) | octets[1]).toString(16) + const lo = ((octets[2] << 8) | octets[3]).toString(16) + return [...groups.slice(0, -1), hi, lo] + } + + const assertHextet = (group: string): void => { + if (!/^[0-9a-fA-F]{1,4}$/.test(group)) { + throw new Error(`ssrf: invalid IPv6 hextet '${group}' in address: ${address}`) + } + } + + if (halves.length === 1) { + const groups = expandEmbeddedIpv4Tail(parseGroups(halves[0])) + if (groups.length !== 8) { + throw new Error(`ssrf: not a valid IPv6 address (expected 8 groups): ${address}`) + } + for (const g of groups) assertHextet(g) + return groups.map((g) => Number.parseInt(g, 16)) + } + + const left = expandEmbeddedIpv4Tail(parseGroups(halves[0])) + const right = expandEmbeddedIpv4Tail(parseGroups(halves[1])) + const missing = 8 - (left.length + right.length) + // RFC 4291 §2.2: "::" stands for ONE OR MORE groups of 16 zero bits — it + // can never represent zero groups (that would just be the two halves + // written without compression at all), so `missing === 0` here is also + // invalid, not merely `missing < 0`. + if (missing < 1) { + throw new Error(`ssrf: not a valid IPv6 address (too many groups): ${address}`) + } + const groups = [...left, ...new Array(missing).fill('0'), ...right] + for (const g of groups) assertHextet(g) + return groups.map((g) => Number.parseInt(g, 16)) +} + +/** Pack an IPv6 address's eight hextets into a single 128-bit `bigint`, most-significant hextet first. */ +export function ipv6ToBigInt(address: string): bigint { + return expandIpv6(address).reduce((acc, hextet) => (acc << 16n) | BigInt(hextet), 0n) +} + +interface Ipv6Range { + label: string + base: string + prefixLength: number +} + +/** + * IANA special-purpose IPv6 registry entries relevant to SSRF: the + * unspecified and loopback addresses, unique-local (RFC 4193, the IPv6 + * analogue of RFC 1918), link-local (RFC 4291 §2.5.6), multicast, the + * NAT64 well-known prefix (RFC 6052) and IPv4-mapped block (RFC 4291 + * §2.5.5.2) — both of which embed an IPv4 address this module does not + * separately unwrap, so the WHOLE prefix is refused (module doc) — the + * discard-only range (RFC 6666), and the documentation prefix (RFC 3849). + * 6to4 (`2002::/16`) and Teredo (`2001::/32`) are refused in full for the + * same "don't unwrap, block outright" reason (module doc). + */ +const IPV6_DISALLOWED_RANGES: Ipv6Range[] = [ + { label: 'loopback', base: '::1', prefixLength: 128 }, + { label: 'unspecified', base: '::', prefixLength: 128 }, + { label: 'IPv4-mapped', base: '::ffff:0:0', prefixLength: 96 }, + { label: 'NAT64 well-known prefix', base: '64:ff9b::', prefixLength: 96 }, + { label: 'discard-only (RFC 6666)', base: '100::', prefixLength: 64 }, + { label: 'documentation (RFC 3849)', base: '2001:db8::', prefixLength: 32 }, + { label: 'Teredo', base: '2001::', prefixLength: 32 }, + { label: '6to4', base: '2002::', prefixLength: 16 }, + { label: 'unique-local (RFC 4193)', base: 'fc00::', prefixLength: 7 }, + { label: 'link-local (RFC 4291)', base: 'fe80::', prefixLength: 10 }, + { label: 'multicast', base: 'ff00::', prefixLength: 8 }, +] + +function ipv6InRange(value: bigint, range: Ipv6Range): boolean { + if (range.prefixLength === 0) return true + const shift = BigInt(128 - range.prefixLength) + return value >> shift === ipv6ToBigInt(range.base) >> shift +} + +/** Is `ip` (a textual IPv6 address) in any disallowed IPv6 range? */ +export function isDisallowedIpv6(ip: string): boolean { + const value = ipv6ToBigInt(ip) + return IPV6_DISALLOWED_RANGES.some((range) => ipv6InRange(value, range)) +} + +/** Is `address` (of `family` 4 or 6) in any disallowed range for its family? The one check {@link resolveSafeAddress} applies to every candidate DNS answer. */ +export function isDisallowedAddress(address: string, family: 4 | 6): boolean { + return family === 4 ? isDisallowedIpv4(address) : isDisallowedIpv6(address) +} + +// --- resolve + validate ----------------------------------------------------- + +/** The DNS lookup shape {@link resolveSafeAddress} needs — `node:dns/promises`' own `lookup(hostname, { all: true })` signature, injectable for tests. */ +export type LookupAllFn = ( + hostname: string, + options: { all: true; verbatim?: boolean }, +) => Promise<{ address: string; family: number }[]> + +const defaultLookupAll: LookupAllFn = (hostname, options) => dnsLookup(hostname, options) + +/** + * Resolve `hostname` and return ONE address safe to connect to (module + * doc's resolve-then-connect pinning). Every resolved candidate is + * validated — not just the one returned — so a multi-answer response + * cannot hide an unsafe address behind a safe one. Throws + * {@link SsrfRefusedError} if resolution fails, returns no answers, or ANY + * answer falls in a disallowed range. + */ +export async function resolveSafeAddress( + hostname: string, + deps: { lookup?: LookupAllFn } = {}, +): Promise { + const lookup = deps.lookup ?? defaultLookupAll + let answers: { address: string; family: number }[] + try { + answers = await lookup(hostname, { all: true, verbatim: true }) + } catch (err) { + throw new SsrfRefusedError( + `could not resolve webhook hostname '${hostname}': ${err instanceof Error ? err.message : String(err)}`, + ) + } + if (answers.length === 0) { + throw new SsrfRefusedError(`webhook hostname '${hostname}' resolved to no addresses`) + } + for (const answer of answers) { + const family = answer.family === 6 ? 6 : 4 + if (isDisallowedAddress(answer.address, family)) { + throw new SsrfRefusedError( + `webhook hostname '${hostname}' resolves to a disallowed address (${answer.address}) — ` + + 'private/link-local/loopback/multicast ranges are refused', + ) + } + } + const chosen = answers[0] + return { address: chosen.address, family: chosen.family === 6 ? 6 : 4 } +} diff --git a/vercel.json b/vercel.json index 13c4b2f..322b271 100644 --- a/vercel.json +++ b/vercel.json @@ -16,6 +16,10 @@ "path": "/api/v1/internal/queue/drain", "schedule": "*/1 * * * *" }, + { + "path": "/api/v1/internal/outbox/drain", + "schedule": "*/1 * * * *" + }, { "path": "/api/v1/internal/cron/watch-maintenance", "schedule": "0 6 * * *"