From a0681dda754adf6b1626daa57a214bb79a5633f4 Mon Sep 17 00:00:00 2001 From: Musiker15 Date: Sat, 20 Jun 2026 19:45:24 +0200 Subject: [PATCH] harden: low-severity bundle (rate-limit, TOCTOU, erasure order, outbox) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundled low-severity findings from the review: - Rate-limit the self-service export endpoint (10/min/IP) for parity with withdraw/delete — the submission UUID is the only credential. - Withdraw is now race-safe: read + guarded `updateMany (status = current)` in one transaction; terminal statuses (incl. an existing withdrawal) return 409. - Erasure (DELETE) removes the row first (cascade) so it's durable even if object storage is down, then best-effort purges the stored objects. - `GET /api/files/[id]`: set Content-Length on `contentLength !== undefined` (0-byte files were silently dropping the header). - Outbox delivery: deliver each row under its own try/catch (one bad row no longer aborts the batch) and mark the delivered rows read in a single `updateMany`. Add a partial index on `notifications(created_at) WHERE read_at IS NULL` (hand-written migration) to keep the poller scan cheap. - Cap `maxFileSizeMb` at a hard server ceiling (`MAX_FILE_SIZE_MB = 25`) in the spec schema and clamp it again in the upload route; document the bounded in-memory buffering. - Move a misplaced doc comment onto the function it describes (deliverOne). --- apps/bot/src/notifications.ts | 28 +++++++---- apps/web/src/app/api/files/[id]/route.ts | 4 +- .../src/app/api/forms/[slug]/upload/route.ts | 9 +++- .../app/api/submissions/[id]/export/route.ts | 9 +++- .../web/src/app/api/submissions/[id]/route.ts | 7 ++- .../api/submissions/[id]/withdraw/route.ts | 46 ++++++++++++------- .../migration.sql | 8 ++++ packages/shared/src/form-spec.ts | 6 ++- 8 files changed, 84 insertions(+), 33 deletions(-) create mode 100644 packages/db/prisma/migrations/20260620180000_notification_pending_index/migration.sql diff --git a/apps/bot/src/notifications.ts b/apps/bot/src/notifications.ts index 2dbb532..b30b4e9 100644 --- a/apps/bot/src/notifications.ts +++ b/apps/bot/src/notifications.ts @@ -64,11 +64,6 @@ function buildMessage(row: PendingRow): { return { embeds: [embed], components: [button] }; } -/** - * Try to deliver one notification. Returns true when the row should be marked - * read — on success, when there's no recipient, or on a permanent "can't DM" - * error. Returns false for transient failures so the next tick retries. - */ /** * Post the "new submission" review embed to the guild's configured review * channel. Drops (marks read) when there's no guild, no configured channel, or @@ -128,6 +123,11 @@ async function deliverReview(client: Client, row: PendingRow): Promise } } +/** + * Try to deliver one notification. Returns true when the row should be marked + * read — on success, when there's no recipient, or on a permanent "can't DM" + * error. Returns false for transient failures so the next tick retries. + */ async function deliverOne(client: Client, row: PendingRow): Promise { if (row.type === "submission_review") return deliverReview(client, row); @@ -172,14 +172,22 @@ export async function deliverPendingNotifications(client: Client): Promise }, })) as PendingRow[]; + // Deliver each row independently (one failure must not abort the batch), + // collect the ones to retire, then mark them all read in a single write. + const deliveredIds: string[] = []; for (const row of pending) { - if (await deliverOne(client, row)) { - await prisma.notification.update({ - where: { id: row.id }, - data: { readAt: new Date() }, - }); + try { + if (await deliverOne(client, row)) deliveredIds.push(row.id); + } catch (err) { + console.error(`[bot] delivery failed for notification ${row.id}:`, err); } } + if (deliveredIds.length > 0) { + await prisma.notification.updateMany({ + where: { id: { in: deliveredIds } }, + data: { readAt: new Date() }, + }); + } } catch (err) { console.error("[bot] notification delivery error:", err); } finally { diff --git a/apps/web/src/app/api/files/[id]/route.ts b/apps/web/src/app/api/files/[id]/route.ts index db46fd3..2549cdb 100644 --- a/apps/web/src/app/api/files/[id]/route.ts +++ b/apps/web/src/app/api/files/[id]/route.ts @@ -41,7 +41,9 @@ export async function GET( "Cache-Control": "private, max-age=0, no-store", "X-Content-Type-Options": "nosniff", }); - if (object.contentLength) headers.set("Content-Length", String(object.contentLength)); + if (object.contentLength !== undefined) { + headers.set("Content-Length", String(object.contentLength)); + } return new NextResponse(object.body, { headers }); } diff --git a/apps/web/src/app/api/forms/[slug]/upload/route.ts b/apps/web/src/app/api/forms/[slug]/upload/route.ts index a65f67c..074a9f2 100644 --- a/apps/web/src/app/api/forms/[slug]/upload/route.ts +++ b/apps/web/src/app/api/forms/[slug]/upload/route.ts @@ -1,5 +1,5 @@ import { prisma } from "@msk-forms/db"; -import { FILE_FIELD_TYPES } from "@msk-forms/shared"; +import { FILE_FIELD_TYPES, MAX_FILE_SIZE_MB } from "@msk-forms/shared"; import { NextResponse, type NextRequest } from "next/server"; import { parseFormSpec } from "@/lib/forms"; @@ -59,7 +59,10 @@ export async function POST( return NextResponse.json({ error: "Not a file field." }, { status: 400 }); } - const maxBytes = (field.validation.maxFileSizeMb ?? DEFAULT_MAX_FILE_MB) * 1024 * 1024; + // Clamp the field's configured limit to the hard server ceiling (defense in + // depth — the spec schema also caps it, but never trust a stored value). + const limitMb = Math.min(field.validation.maxFileSizeMb ?? DEFAULT_MAX_FILE_MB, MAX_FILE_SIZE_MB); + const maxBytes = limitMb * 1024 * 1024; if (file.size === 0 || file.size > maxBytes) { return NextResponse.json( { error: `File must be between 1 byte and ${Math.round(maxBytes / 1024 / 1024)} MB.` }, @@ -80,6 +83,8 @@ export async function POST( const key = `uploads/${form.id}/${crypto.randomUUID()}`; try { + // Buffer fully into memory — bounded by the size check above (≤ MAX_FILE_SIZE_MB), + // so this is safe; streaming would only matter for much larger uploads. await putObject(key, new Uint8Array(await file.arrayBuffer()), mime); } catch (err) { console.error("[upload] storage error:", (err as Error).message); diff --git a/apps/web/src/app/api/submissions/[id]/export/route.ts b/apps/web/src/app/api/submissions/[id]/export/route.ts index 1e65f18..3b98172 100644 --- a/apps/web/src/app/api/submissions/[id]/export/route.ts +++ b/apps/web/src/app/api/submissions/[id]/export/route.ts @@ -1,6 +1,8 @@ import { prisma } from "@msk-forms/db"; import { NextResponse, type NextRequest } from "next/server"; +import { clientIp, rateLimit } from "@/lib/rate-limit"; + export const runtime = "nodejs"; export const dynamic = "force-dynamic"; @@ -9,10 +11,15 @@ export const dynamic = "force-dynamic"; * Capability model: access by the submission UUID. */ export async function GET( - _request: NextRequest, + request: NextRequest, { params }: { params: Promise<{ id: string }> }, ) { const { id } = await params; + // Throttle for parity with withdraw/delete (the UUID is the only credential). + const rl = await rateLimit(`export:${clientIp(request.headers)}`, 10, 60); + if (!rl.allowed) { + return NextResponse.json({ error: "Too many requests." }, { status: 429 }); + } const submission = await prisma.submission.findUnique({ where: { id }, diff --git a/apps/web/src/app/api/submissions/[id]/route.ts b/apps/web/src/app/api/submissions/[id]/route.ts index 6813b2b..3c6d08c 100644 --- a/apps/web/src/app/api/submissions/[id]/route.ts +++ b/apps/web/src/app/api/submissions/[id]/route.ts @@ -28,9 +28,12 @@ export async function DELETE( }); if (!submission) return NextResponse.json({ error: "Not found." }, { status: 404 }); - // Remove stored files first (best-effort), then the row (cascades events/files). - await Promise.all(submission.files.map((f) => deleteObject(f.storageKey))); + // Delete the row first (cascades events + file rows) so the erasure is durable + // even if object storage is down; then best-effort purge the stored objects. + // The storage keys were captured above before the row is gone. + const keys = submission.files.map((f) => f.storageKey); await prisma.submission.delete({ where: { id } }); + await Promise.all(keys.map((key) => deleteObject(key))); return NextResponse.json({ ok: true }); } diff --git a/apps/web/src/app/api/submissions/[id]/withdraw/route.ts b/apps/web/src/app/api/submissions/[id]/withdraw/route.ts index 0eb5b6a..7c7f895 100644 --- a/apps/web/src/app/api/submissions/[id]/withdraw/route.ts +++ b/apps/web/src/app/api/submissions/[id]/withdraw/route.ts @@ -23,22 +23,25 @@ export async function POST( return NextResponse.json({ error: "Too many requests." }, { status: 429 }); } - const submission = await prisma.submission.findUnique({ - where: { id }, - select: { status: true }, - }); - if (!submission) return NextResponse.json({ error: "Not found." }, { status: 404 }); + // Read + guarded write in one transaction so two concurrent withdraws (or a + // withdraw racing a reviewer decision) can't both record the change. The + // `updateMany ... status = current` only succeeds while the status is still + // what we read; terminal statuses (incl. an existing withdrawal) are rejected. + const outcome = await prisma.$transaction(async (tx) => { + const submission = await tx.submission.findUnique({ + where: { id }, + select: { status: true }, + }); + if (!submission) return { code: 404 as const }; + if (TERMINAL.has(submission.status)) return { code: 409 as const }; - if (TERMINAL.has(submission.status)) { - return NextResponse.json( - { error: "This submission can no longer be withdrawn." }, - { status: 409 }, - ); - } + const updated = await tx.submission.updateMany({ + where: { id, status: submission.status }, + data: { status: "withdrawn" }, + }); + if (updated.count === 0) return { code: 409 as const }; - await prisma.$transaction([ - prisma.submission.update({ where: { id }, data: { status: "withdrawn" } }), - prisma.submissionEvent.create({ + await tx.submissionEvent.create({ data: { submissionId: id, type: "status_change", @@ -46,7 +49,18 @@ export async function POST( toStatus: "withdrawn", visibility: "public", }, - }), - ]); + }); + return { code: 200 as const }; + }); + + if (outcome.code === 404) { + return NextResponse.json({ error: "Not found." }, { status: 404 }); + } + if (outcome.code === 409) { + return NextResponse.json( + { error: "This submission can no longer be withdrawn." }, + { status: 409 }, + ); + } return NextResponse.json({ ok: true }); } diff --git a/packages/db/prisma/migrations/20260620180000_notification_pending_index/migration.sql b/packages/db/prisma/migrations/20260620180000_notification_pending_index/migration.sql new file mode 100644 index 0000000..ec32916 --- /dev/null +++ b/packages/db/prisma/migrations/20260620180000_notification_pending_index/migration.sql @@ -0,0 +1,8 @@ +-- The outbox poller scans for undelivered rows ordered by creation time: +-- WHERE read_at IS NULL ORDER BY created_at ASC +-- A partial index over just the unread rows keeps that scan cheap even as the +-- delivered-notification table grows. (Partial indexes aren't expressible in the +-- Prisma schema, so this is a hand-written migration.) +CREATE INDEX IF NOT EXISTS "notifications_pending_idx" + ON "notifications" ("created_at") + WHERE "read_at" IS NULL; diff --git a/packages/shared/src/form-spec.ts b/packages/shared/src/form-spec.ts index d2d9c18..8b8c426 100644 --- a/packages/shared/src/form-spec.ts +++ b/packages/shared/src/form-spec.ts @@ -76,6 +76,9 @@ export const conditionRuleSchema = z.object({ target: z.string().optional(), }); +/** Absolute server-side ceiling for a single upload, regardless of field config. */ +export const MAX_FILE_SIZE_MB = 25; + export const fieldValidationSchema = z.object({ required: z.boolean().default(false), min: z.number().optional(), @@ -84,7 +87,8 @@ export const fieldValidationSchema = z.object({ maxLength: z.number().optional(), pattern: z.string().optional(), allowedMimeTypes: z.array(z.string()).optional(), - maxFileSizeMb: z.number().optional(), + // Capped at the hard server limit so a crafted spec can't raise it. + maxFileSizeMb: z.number().positive().max(MAX_FILE_SIZE_MB).optional(), }); export const fieldOptionSchema = z.object({