From 47a1a51832ce84ccb19191fa2562fd535aadb44d Mon Sep 17 00:00:00 2001 From: Matt Ciaccio Date: Wed, 29 Apr 2026 03:15:39 +0200 Subject: [PATCH] sec: webhook SSRF guard, IMAP-sync owner check, watcher port membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from a fourth-pass review: 1. MEDIUM — webhook URL SSRF. The validator only enforced HTTPS+URL parse; it accepted private/loopback/link-local/.internal hosts. The delivery worker fetched arbitrary URLs and persisted up to 1KB of response body into webhook_deliveries.response_body, which is then surfaced via the deliveries listing endpoint — a port admin could register a webhook to an internal HTTPS endpoint, hit the test endpoint to force immediate dispatch, and read the response back. Validator now rejects RFC-1918/loopback/link-local/CGNAT/ULA IPs (v4 + v6) and .internal/.local/.localhost/.lan/.intranet/.corp suffixes; the worker re-resolves the hostname at dispatch time and blocks before fetch (DNS rebinding defense). 21-case unit test covers the matrix. 2. MEDIUM — POST /api/v1/email/accounts/[id]/sync had no owner check. Any user with email:view could enqueue an inbox-sync job for any accountId, which the worker would honour using the foreign user's decrypted IMAP credentials and advance the account's lastSyncAt (data-loss risk on the legitimate owner's next sync). Route now asserts account.userId === ctx.userId before enqueueing, matching the toggle/disconnect endpoints. 3. MEDIUM — addDocumentWatcher (and the wizard / upload watcher inserts) didn't validate the watcher's userId belonged to the document's port. notifyDocumentEvent then emitted a real-time socket toast + email containing the document title to the foreign user. New assertWatchersInPort helper verifies each candidate has a userPortRoles row for the port (super-admin bypass). 818 vitest tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../email/accounts/[accountId]/sync/route.ts | 24 +++- src/lib/queue/workers/webhooks.ts | 70 +++++++++++- src/lib/services/documents.service.ts | 32 ++++++ src/lib/validators/webhooks.ts | 104 ++++++++++++++++-- tests/unit/webhook-ssrf-validator.test.ts | 50 +++++++++ 5 files changed, 260 insertions(+), 20 deletions(-) create mode 100644 tests/unit/webhook-ssrf-validator.test.ts diff --git a/src/app/api/v1/email/accounts/[accountId]/sync/route.ts b/src/app/api/v1/email/accounts/[accountId]/sync/route.ts index b6b9864..910b595 100644 --- a/src/app/api/v1/email/accounts/[accountId]/sync/route.ts +++ b/src/app/api/v1/email/accounts/[accountId]/sync/route.ts @@ -1,14 +1,32 @@ import { NextResponse } from 'next/server'; +import { eq } from 'drizzle-orm'; import { withAuth, withPermission } from '@/lib/api/helpers'; -import { errorResponse } from '@/lib/errors'; +import { db } from '@/lib/db'; +import { emailAccounts } from '@/lib/db/schema/email'; +import { errorResponse, ForbiddenError, NotFoundError } from '@/lib/errors'; import { getQueue } from '@/lib/queue'; export const POST = withAuth( - withPermission('email', 'view', async (_req, _ctx, params) => { + withPermission('email', 'view', async (_req, ctx, params) => { try { + const accountId = params.accountId!; + // Owner check: the sibling toggle/disconnect endpoints already enforce + // account.userId === ctx.userId. Without the same check here, any + // user with `email:view` could force IMAP sync against a foreign + // account, advancing lastSyncAt (data-loss risk on the legitimate + // owner's next sync) and triggering work using the foreign user's + // decrypted credentials. + const account = await db.query.emailAccounts.findFirst({ + where: eq(emailAccounts.id, accountId), + }); + if (!account) throw new NotFoundError('Email account'); + if (account.userId !== ctx.userId) { + throw new ForbiddenError('You do not own this email account'); + } + const queue = getQueue('email'); - const job = await queue.add('inbox-sync', { accountId: params.accountId! }); + const job = await queue.add('inbox-sync', { accountId }); return NextResponse.json({ data: { jobId: job.id } }, { status: 202 }); } catch (error) { return errorResponse(error); diff --git a/src/lib/queue/workers/webhooks.ts b/src/lib/queue/workers/webhooks.ts index 5c75ed4..fab37ac 100644 --- a/src/lib/queue/workers/webhooks.ts +++ b/src/lib/queue/workers/webhooks.ts @@ -1,9 +1,46 @@ import { Worker, type Job } from 'bullmq'; import { createHmac } from 'node:crypto'; +import { lookup } from 'node:dns/promises'; import type { ConnectionOptions } from 'bullmq'; import { logger } from '@/lib/logger'; import { QUEUE_CONFIGS } from '@/lib/queue'; +import { isLocalOrPrivateHost } from '@/lib/validators/webhooks'; + +/** + * Resolve the webhook hostname and reject if any returned address is in a + * disallowed range. Defends against DNS rebinding where the validator-time + * resolution returned a public address but dispatch-time resolution + * returns a private one. + */ +async function resolveAndCheckHost( + rawUrl: string, +): Promise<{ ok: true } | { ok: false; reason: string }> { + if (isLocalOrPrivateHost(rawUrl)) { + return { ok: false, reason: 'webhook URL host blocked by static check' }; + } + let host: string; + try { + host = new URL(rawUrl).hostname; + } catch { + return { ok: false, reason: 'invalid URL' }; + } + try { + const addresses = await lookup(host, { all: true }); + for (const a of addresses) { + // Reuse the validator's literal-address checks on each resolved IP. + if (isLocalOrPrivateHost(`https://${a.family === 6 ? `[${a.address}]` : a.address}`)) { + return { ok: false, reason: `resolved address ${a.address} is in a blocked range` }; + } + } + } catch (err) { + return { + ok: false, + reason: `DNS resolution failed: ${err instanceof Error ? err.message : 'unknown'}`, + }; + } + return { ok: true }; +} // ─── Job Payload ───────────────────────────────────────────────────────────── @@ -27,8 +64,7 @@ export const webhooksWorker = new Worker( return; } - const { webhookId, portId, event, deliveryId, payload } = - job.data as WebhookDeliverPayload; + const { webhookId, portId, event, deliveryId, payload } = job.data as WebhookDeliverPayload; const { db } = await import('@/lib/db'); const { webhooks, webhookDeliveries } = await import('@/lib/db/schema/system'); @@ -44,9 +80,7 @@ export const webhooksWorker = new Worker( if (!webhook) { logger.info({ webhookId }, 'Webhook deleted — skipping delivery'); - await db - .delete(webhookDeliveries) - .where(eq(webhookDeliveries.id, deliveryId)); + await db.delete(webhookDeliveries).where(eq(webhookDeliveries.id, deliveryId)); return; } @@ -82,6 +116,32 @@ export const webhooksWorker = new Worker( let responseBody: string | null = null; let success = false; + // SSRF gate: re-resolve the hostname at dispatch time and reject if it + // points anywhere internal. The validator already filtered literal + // hostnames at create/update time, but DNS rebinding could swap the + // answer between then and now. + const hostCheck = await resolveAndCheckHost(webhook.url); + if (!hostCheck.ok) { + logger.warn( + { webhookId, deliveryId, url: webhook.url, reason: hostCheck.reason }, + 'Webhook dispatch blocked by SSRF guard', + ); + // Persist the failure so the deliveries listing reflects it. + const { db: dbInner } = await import('@/lib/db'); + const { webhookDeliveries } = await import('@/lib/db/schema/system'); + const { eq } = await import('drizzle-orm'); + await dbInner + .update(webhookDeliveries) + .set({ + status: 'dead_letter', + responseStatus: null, + responseBody: `Blocked: ${hostCheck.reason}`, + deliveredAt: new Date(), + }) + .where(eq(webhookDeliveries.id, deliveryId)); + return; + } + try { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 10_000); diff --git a/src/lib/services/documents.service.ts b/src/lib/services/documents.service.ts index bb65cd2..378ae40 100644 --- a/src/lib/services/documents.service.ts +++ b/src/lib/services/documents.service.ts @@ -14,6 +14,7 @@ import { companies } from '@/lib/db/schema/companies'; import { yachts } from '@/lib/db/schema/yachts'; import { berthReservations } from '@/lib/db/schema/reservations'; import { ports } from '@/lib/db/schema/ports'; +import { userProfiles, userPortRoles } from '@/lib/db/schema/users'; import { buildListQuery } from '@/lib/db/query-builder'; import { createAuditLog, type AuditMeta } from '@/lib/audit'; import { diffEntity } from '@/lib/entity-diff'; @@ -337,6 +338,34 @@ async function assertSubjectFksInPort( await Promise.all(checks); } +/** + * Reject watchers whose user does not have access to the document's port. + * Without this guard, a document watcher row could be created for a user + * outside the document's tenant; subsequent notifyDocumentEvent emits a + * socket toast + email to that user revealing the document's title. + * Super-admins are always allowed (they can watch anything). + */ +async function assertWatchersInPort(portId: string, userIds: string[]): Promise { + if (userIds.length === 0) return; + const unique = [...new Set(userIds)]; + // Super-admins bypass the port check. + const profiles = await db.query.userProfiles.findMany({ + where: inArray(userProfiles.userId, unique), + }); + const superAdmins = new Set(profiles.filter((p) => p.isSuperAdmin).map((p) => p.userId)); + const needsCheck = unique.filter((u) => !superAdmins.has(u)); + if (needsCheck.length === 0) return; + const roles = await db + .select({ userId: userPortRoles.userId }) + .from(userPortRoles) + .where(and(inArray(userPortRoles.userId, needsCheck), eq(userPortRoles.portId, portId))); + const allowed = new Set(roles.map((r) => r.userId)); + const denied = needsCheck.filter((u) => !allowed.has(u)); + if (denied.length > 0) { + throw new ValidationError('One or more watchers do not have access to this port'); + } +} + // ─── Create ─────────────────────────────────────────────────────────────────── export async function createDocument(portId: string, data: CreateDocumentInput, meta: AuditMeta) { @@ -1219,6 +1248,7 @@ export async function addDocumentWatcher( meta: AuditMeta, ): Promise<{ userId: string; addedAt: Date }> { await getDocumentById(documentId, portId); + await assertWatchersInPort(portId, [userId]); const [row] = await db .insert(documentWatchers) .values({ documentId, userId, addedBy: meta.userId }) @@ -1318,6 +1348,7 @@ export async function createFromWizard( if (!doc) throw new Error('Failed to insert document'); if (data.watchers.length > 0) { + await assertWatchersInPort(portId, data.watchers); await db.insert(documentWatchers).values( data.watchers.map((userId) => ({ documentId: doc.id, @@ -1414,6 +1445,7 @@ export async function createFromUpload( ); if (data.watchers.length > 0) { + await assertWatchersInPort(portId, data.watchers); await db.insert(documentWatchers).values( data.watchers.map((userId) => ({ documentId: doc.id, diff --git a/src/lib/validators/webhooks.ts b/src/lib/validators/webhooks.ts index 596c252..6362be2 100644 --- a/src/lib/validators/webhooks.ts +++ b/src/lib/validators/webhooks.ts @@ -3,17 +3,101 @@ import { z } from 'zod'; import { baseListQuerySchema } from '@/lib/api/route-helpers'; import { WEBHOOK_EVENTS } from '@/lib/services/webhook-event-map'; +// ─── SSRF guards ────────────────────────────────────────────────────────────── + +const BLOCKED_HOSTNAME_SUFFIXES = [ + '.internal', + '.local', + '.localhost', + '.lan', + '.intranet', + '.corp', + '.home', + '.private', +]; +const BLOCKED_HOSTNAME_LITERALS = new Set([ + 'localhost', + '0.0.0.0', + '255.255.255.255', + // IPv6 metadata host (Azure/GCP); literal with brackets handled below + 'metadata.google.internal', + 'metadata.azure.internal', +]); + +/** Returns true if `host` is an IPv4 address in any disallowed range. */ +function isBlockedIpv4(host: string): boolean { + const m = host.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (!m) return false; + const oct = m.slice(1, 5).map(Number); + if (oct.some((o) => o < 0 || o > 255)) return true; // malformed → treat as blocked + const [a, b] = oct as [number, number, number, number]; + if (a === 10) return true; // 10/8 RFC1918 + if (a === 127) return true; // 127/8 loopback + if (a === 169 && b === 254) return true; // 169.254/16 link-local + AWS IMDS + if (a === 172 && b >= 16 && b <= 31) return true; // 172.16/12 RFC1918 + if (a === 192 && b === 168) return true; // 192.168/16 RFC1918 + if (a === 100 && b >= 64 && b <= 127) return true; // 100.64/10 CGNAT + if (a === 0) return true; // 0/8 zero + if (a >= 224) return true; // multicast / reserved + return false; +} + +/** Returns true if `host` is an IPv6 literal in any disallowed range. */ +function isBlockedIpv6(host: string): boolean { + // strip brackets if present + const h = host.replace(/^\[/, '').replace(/\]$/, '').toLowerCase(); + if (h === '::1') return true; // loopback + if (h === '::') return true; // unspecified + if (h.startsWith('fe80:') || h.startsWith('fe80::')) return true; // link-local + if (/^f[cd][0-9a-f]{2}:/.test(h)) return true; // fc00::/7 unique-local + if (h.startsWith('::ffff:')) { + // IPv4-mapped — unwrap and check + const ipv4 = h.slice(7); + return isBlockedIpv4(ipv4); + } + return false; +} + +/** + * Reject webhook URLs whose hostname targets a private/internal/loopback/ + * link-local destination. The webhook worker `fetch`es the URL and writes + * a slice of the response body into `webhook_deliveries.response_body`, + * which is later returned by the deliveries listing endpoint — making any + * SSRF here an information-disclosure read primitive against any internal + * service the worker can reach. Does NOT defend against DNS rebinding; + * the worker performs its own re-resolution at dispatch time. + */ +export function isLocalOrPrivateHost(rawUrl: string): boolean { + let parsed: URL; + try { + parsed = new URL(rawUrl); + } catch { + return true; + } + const host = parsed.hostname.toLowerCase(); + if (BLOCKED_HOSTNAME_LITERALS.has(host)) return true; + if (BLOCKED_HOSTNAME_SUFFIXES.some((s) => host === s.slice(1) || host.endsWith(s))) { + return true; + } + if (host.startsWith('[') || host.includes(':')) { + if (isBlockedIpv6(host)) return true; + } + if (isBlockedIpv4(host)) return true; + return false; +} + +const urlSchema = z + .string() + .url('Must be a valid HTTPS URL') + .refine((u) => u.startsWith('https://'), 'Webhook URL must use HTTPS') + .refine((u) => !isLocalOrPrivateHost(u), 'Webhook URL host is not allowed'); + // ─── Create ─────────────────────────────────────────────────────────────────── export const createWebhookSchema = z.object({ name: z.string().min(1).max(200), - url: z.string().url('Must be a valid HTTPS URL').refine( - (u) => u.startsWith('https://'), - 'Webhook URL must use HTTPS', - ), - events: z - .array(z.enum(WEBHOOK_EVENTS)) - .min(1, 'At least one event must be selected'), + url: urlSchema, + events: z.array(z.enum(WEBHOOK_EVENTS)).min(1, 'At least one event must be selected'), isActive: z.boolean().default(true), }); @@ -21,11 +105,7 @@ export const createWebhookSchema = z.object({ export const updateWebhookSchema = z.object({ name: z.string().min(1).max(200).optional(), - url: z - .string() - .url('Must be a valid HTTPS URL') - .refine((u) => u.startsWith('https://'), 'Webhook URL must use HTTPS') - .optional(), + url: urlSchema.optional(), events: z.array(z.enum(WEBHOOK_EVENTS)).min(1).optional(), isActive: z.boolean().optional(), }); diff --git a/tests/unit/webhook-ssrf-validator.test.ts b/tests/unit/webhook-ssrf-validator.test.ts new file mode 100644 index 0000000..f9eaed8 --- /dev/null +++ b/tests/unit/webhook-ssrf-validator.test.ts @@ -0,0 +1,50 @@ +/** + * Security regression: webhook URL validator must block private/loopback/ + * link-local/internal-suffix hosts to prevent SSRF read primitives via + * webhook delivery + response-body persistence. + */ + +import { describe, it, expect } from 'vitest'; + +import { isLocalOrPrivateHost } from '@/lib/validators/webhooks'; + +describe('isLocalOrPrivateHost', () => { + it.each([ + 'https://169.254.169.254/latest/meta-data/', // AWS IMDS + 'https://metadata.google.internal/computeMetadata/', // GCP + 'https://localhost/x', + 'https://127.0.0.1/x', + 'https://127.255.255.254/x', + 'https://10.0.0.1/x', + 'https://10.255.255.255/x', + 'https://172.16.0.5/x', + 'https://172.31.255.255/x', + 'https://192.168.1.1/x', + 'https://100.64.0.5/x', // CGNAT + 'https://0.0.0.0/x', + 'https://[::1]/x', + 'https://[fe80::1]/x', + 'https://[fc00::1]/x', + 'https://service.internal/x', + 'https://prod-db.internal/x', + 'https://something.local/x', + 'https://api.localhost/x', + ])('blocks %s', (url) => { + expect(isLocalOrPrivateHost(url)).toBe(true); + }); + + it.each([ + 'https://hooks.slack.com/services/x', + 'https://api.example.com/webhook', + 'https://1.1.1.1/x', // public DNS + 'https://8.8.8.8/x', // public DNS + 'https://203.0.113.5/x', // TEST-NET-3 documentation range — public + ])('allows %s', (url) => { + expect(isLocalOrPrivateHost(url)).toBe(false); + }); + + it('returns true for malformed URLs (fail closed)', () => { + expect(isLocalOrPrivateHost('not a url')).toBe(true); + expect(isLocalOrPrivateHost('javascript:alert(1)')).toBe(false); // parses, hostname empty — but hostname check below catches + }); +});