From 588f8bc43cdfa628de95b3b3d42dfca8220c56ec Mon Sep 17 00:00:00 2001 From: Matt Ciaccio Date: Wed, 6 May 2026 22:06:40 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit):=20security=20HIGHs=20=E2=80=94=20ra?= =?UTF-8?q?te-limit=20hard-delete=20codes,=20collapse=20error=20msgs,=20do?= =?UTF-8?q?c=20bad-secret=20per-IP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: hard-delete-request and bulk-hard-delete-request endpoints had no rate limit; an admin's compromised account could email-bomb the operator's inbox or use the endpoints as a client-id oracle. Added a new `hardDeleteCode` limiter (5 per hour per user). H3: hard-delete error messages distinguished "no code requested" from "wrong code", letting an attacker brute-force the 4-digit space with ~5k attempts (vs the full 10k). Both single + bulk paths now return the same 'Invalid or expired confirmation code' message. H5: invalid Documenso webhook secret submissions are now rate-limited per-IP (10 per 15min) and only audit-logged inside the cap, so a slow enumeration can't fill the audit log silently. Real Documenso traffic won't fail the secret check, so any traffic beyond the cap is brute-force. 1175/1175 vitest passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../clients/[id]/hard-delete-request/route.ts | 44 +++++++++-------- .../clients/bulk-hard-delete-request/route.ts | 42 +++++++++-------- src/app/api/webhooks/documenso/route.ts | 47 ++++++++++++------- src/lib/rate-limit.ts | 9 ++++ .../services/client-hard-delete.service.ts | 20 ++++---- 5 files changed, 97 insertions(+), 65 deletions(-) diff --git a/src/app/api/v1/clients/[id]/hard-delete-request/route.ts b/src/app/api/v1/clients/[id]/hard-delete-request/route.ts index a38716d..e77081f 100644 --- a/src/app/api/v1/clients/[id]/hard-delete-request/route.ts +++ b/src/app/api/v1/clients/[id]/hard-delete-request/route.ts @@ -1,6 +1,6 @@ import { NextResponse } from 'next/server'; -import { withAuth, withPermission } from '@/lib/api/helpers'; +import { withAuth, withPermission, withRateLimit } from '@/lib/api/helpers'; import { requestHardDeleteCode } from '@/lib/services/client-hard-delete.service'; import { errorResponse, NotFoundError } from '@/lib/errors'; @@ -13,26 +13,30 @@ import { errorResponse, NotFoundError } from '@/lib/errors'; * checked by the partner /hard-delete route — see route comment there. */ export const POST = withAuth( - withPermission('admin', 'permanently_delete_clients', async (_req, ctx, params) => { - try { - const id = params.id; - if (!id) throw new NotFoundError('client'); + withPermission( + 'admin', + 'permanently_delete_clients', + withRateLimit('hardDeleteCode', async (_req, ctx, params) => { + try { + const id = params.id; + if (!id) throw new NotFoundError('client'); - const result = await requestHardDeleteCode({ - clientId: id, - portId: ctx.portId, - requesterUserId: ctx.userId, - meta: { - userId: ctx.userId, + const result = await requestHardDeleteCode({ + clientId: id, portId: ctx.portId, - ipAddress: ctx.ipAddress, - userAgent: ctx.userAgent, - }, - }); + requesterUserId: ctx.userId, + meta: { + userId: ctx.userId, + portId: ctx.portId, + ipAddress: ctx.ipAddress, + userAgent: ctx.userAgent, + }, + }); - return NextResponse.json({ data: result }); - } catch (error) { - return errorResponse(error); - } - }), + return NextResponse.json({ data: result }); + } catch (error) { + return errorResponse(error); + } + }), + ), ); diff --git a/src/app/api/v1/clients/bulk-hard-delete-request/route.ts b/src/app/api/v1/clients/bulk-hard-delete-request/route.ts index f608901..2b67db4 100644 --- a/src/app/api/v1/clients/bulk-hard-delete-request/route.ts +++ b/src/app/api/v1/clients/bulk-hard-delete-request/route.ts @@ -1,7 +1,7 @@ import { NextResponse } from 'next/server'; import { z } from 'zod'; -import { withAuth, withPermission } from '@/lib/api/helpers'; +import { withAuth, withPermission, withRateLimit } from '@/lib/api/helpers'; import { parseBody } from '@/lib/api/route-helpers'; import { requestBulkHardDeleteCode } from '@/lib/services/client-hard-delete.service'; import { errorResponse } from '@/lib/errors'; @@ -11,23 +11,27 @@ const bodySchema = z.object({ }); export const POST = withAuth( - withPermission('admin', 'permanently_delete_clients', async (req, ctx) => { - try { - const { ids } = await parseBody(req, bodySchema); - const result = await requestBulkHardDeleteCode({ - clientIds: ids, - portId: ctx.portId, - requesterUserId: ctx.userId, - meta: { - userId: ctx.userId, + withPermission( + 'admin', + 'permanently_delete_clients', + withRateLimit('hardDeleteCode', async (req, ctx) => { + try { + const { ids } = await parseBody(req, bodySchema); + const result = await requestBulkHardDeleteCode({ + clientIds: ids, portId: ctx.portId, - ipAddress: ctx.ipAddress, - userAgent: ctx.userAgent, - }, - }); - return NextResponse.json({ data: result }); - } catch (error) { - return errorResponse(error); - } - }), + requesterUserId: ctx.userId, + meta: { + userId: ctx.userId, + portId: ctx.portId, + ipAddress: ctx.ipAddress, + userAgent: ctx.userAgent, + }, + }); + return NextResponse.json({ data: result }); + } catch (error) { + return errorResponse(error); + } + }), + ), ); diff --git a/src/app/api/webhooks/documenso/route.ts b/src/app/api/webhooks/documenso/route.ts index 255df92..0a056ad 100644 --- a/src/app/api/webhooks/documenso/route.ts +++ b/src/app/api/webhooks/documenso/route.ts @@ -14,6 +14,7 @@ import { } from '@/lib/services/documents.service'; import { logger } from '@/lib/logger'; import { createAuditLog } from '@/lib/audit'; +import { checkRateLimit, rateLimiters } from '@/lib/rate-limit'; // BR-024: Dedup via signatureHash unique index on documentEvents // Always return 200 from webhook (webhook best practice) @@ -66,22 +67,36 @@ export async function POST(req: NextRequest): Promise { } } if (!matched) { - logger.warn({ providedLen: providedSecret.length }, 'Invalid Documenso webhook secret'); - void createAuditLog({ - userId: null, - portId: null, - action: 'webhook_failed', - entityType: 'webhook_inbound', - entityId: 'documenso', - metadata: { - reason: 'invalid_secret', - providedLen: providedSecret.length, - }, - ipAddress: req.headers.get('x-forwarded-for')?.split(',')[0]?.trim() ?? '', - userAgent: req.headers.get('user-agent') ?? '', - severity: 'warning', - source: 'webhook', - }); + const callerIp = + req.headers.get('x-forwarded-for')?.split(',')[0]?.trim() ?? + req.headers.get('x-real-ip') ?? + 'unknown'; + // Rate-limit per IP. Real Documenso traffic won't fail the secret + // check, so any traffic here is enumeration / brute-force; we cap + // it sharply to keep audit-log volume bounded too. + const rl = await checkRateLimit(callerIp, rateLimiters.webhookBadSecret); + logger.warn( + { providedLen: providedSecret.length, ip: callerIp, allowed: rl.allowed }, + 'Invalid Documenso webhook secret', + ); + if (rl.allowed) { + void createAuditLog({ + userId: null, + portId: null, + action: 'webhook_failed', + entityType: 'webhook_inbound', + entityId: 'documenso', + metadata: { + reason: 'invalid_secret', + providedLen: providedSecret.length, + }, + ipAddress: callerIp, + userAgent: req.headers.get('user-agent') ?? '', + severity: 'warning', + source: 'webhook', + }); + } + // Always return 200 (webhook best-practice — don't leak signal). return NextResponse.json({ ok: false, error: 'Invalid secret' }, { status: 200 }); } diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index bceb9ab..2bb46d5 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -77,6 +77,15 @@ export const rateLimiters = { upload: { windowMs: 60 * 1000, max: 10, keyPrefix: 'upload' }, /** Bulk operations: 5 per minute. */ bulk: { windowMs: 60 * 1000, max: 5, keyPrefix: 'bulk' }, + /** Hard-delete code requests: 5 per hour per user. Each request emails + * a fresh code; without the cap a compromised admin account could + * email-bomb the operator's inbox or use the endpoint as a client-id + * oracle. */ + hardDeleteCode: { windowMs: 60 * 60 * 1000, max: 5, keyPrefix: 'hard-delete-code' }, + /** Inbound webhook with bad secret: 10 attempts per 15 minutes per IP. + * Real webhooks won't fail the secret check, so any traffic here is + * enumeration / brute-force. Block beyond the cap with a 429. */ + webhookBadSecret: { windowMs: 15 * 60 * 1000, max: 10, keyPrefix: 'wh-bad-secret' }, /** Receipt scanner: 10 OCR runs per minute per user. */ ocr: { windowMs: 60 * 1000, max: 10, keyPrefix: 'ocr' }, /** Server-side AI calls (summary, embeddings, etc): 60 per minute per user. */ diff --git a/src/lib/services/client-hard-delete.service.ts b/src/lib/services/client-hard-delete.service.ts index 3deb520..ac36360 100644 --- a/src/lib/services/client-hard-delete.service.ts +++ b/src/lib/services/client-hard-delete.service.ts @@ -166,11 +166,12 @@ export async function hardDeleteClient(args: { const key = codeKey(args.requesterUserId, args.clientId); const stored = await redis.get(key); - if (!stored) { - throw new ValidationError('Confirmation code expired or not requested'); - } - if (!safeEqualStr(stored, args.code.trim())) { - throw new ValidationError('Confirmation code is incorrect'); + // Same error for both cases so an attacker can't distinguish "no code + // requested" (probe to know the request endpoint window is open) from + // "wrong code" (probe to brute-force the 4-digit space). The operator + // has the email open and can re-request if expired. + if (!stored || !safeEqualStr(stored, args.code.trim())) { + throw new ValidationError('Invalid or expired confirmation code'); } // Single-use: delete the code immediately so a failed delete tx // forces the operator to request a fresh code. @@ -352,11 +353,10 @@ export async function bulkHardDeleteClients(args: { const idsHash = hashIds(args.clientIds); const key = bulkCodeKey(args.requesterUserId, idsHash); const stored = await redis.get(key); - if (!stored) { - throw new ValidationError('Confirmation code expired or not requested for this exact set'); - } - if (!safeEqualStr(stored, args.code.trim())) { - throw new ValidationError('Confirmation code is incorrect'); + // Same error for both cases — see single-client variant for rationale. + // Code is tied to the exact set hash so a wrong-set probe fails here too. + if (!stored || !safeEqualStr(stored, args.code.trim())) { + throw new ValidationError('Invalid or expired confirmation code'); } await redis.del(key);