diff --git a/apps/bot/src/review-actions.ts b/apps/bot/src/review-actions.ts index cbdd309..8a74ef8 100644 --- a/apps/bot/src/review-actions.ts +++ b/apps/bot/src/review-actions.ts @@ -1,4 +1,4 @@ -import { Prisma, prisma } from "@msk-forms/db"; +import { changeSubmissionStatus, prisma } from "@msk-forms/db"; import { DEFAULT_STATUSES, parseBotConfig, @@ -64,42 +64,27 @@ export async function handleReviewButton(interaction: ButtonInteraction): Promis const toStatus = ACTION_STATUS[action]; - if (submission.status !== toStatus) { - const ops: Prisma.PrismaPromise[] = [ - prisma.submission.update({ where: { id: submissionId }, data: { status: toStatus } }), - prisma.submissionEvent.create({ - data: { - submissionId, - type: "status_change", - fromStatus: submission.status, - toStatus, - visibility: "public", - }, - }), - ]; - if (submission.userId) { - const payload: StatusChangeNotification = { + // Idempotent, race-safe transition shared with the web review route. + const notify: StatusChangeNotification | null = submission.userId + ? { submissionId, formTitle: submission.form.title, toStatus, toStatusLabel: statusLabel(toStatus), - }; - ops.push( - prisma.notification.create({ - data: { - userId: submission.userId, - type: "status_change", - payload: payload as unknown as Prisma.InputJsonValue, - }, - }), - ); - } - await prisma.$transaction(ops); + } + : null; + const { changed } = await changeSubmissionStatus({ + submissionId, + toStatus, + notify: + submission.userId && notify + ? { userId: submission.userId, type: "status_change", payload: notify } + : null, + }); - if (action === "accept" && submission.userId) { - const roleId = parseBotConfig(submission.guild.botConfig).acceptedRoleId; - if (roleId) await grantRole(interaction, submission.userId, roleId); - } + if (changed && action === "accept" && submission.userId) { + const roleId = parseBotConfig(submission.guild.botConfig).acceptedRoleId; + if (roleId) await grantRole(interaction, submission.userId, roleId); } await updateMessage(interaction, submission.guildId, submissionId, toStatus); diff --git a/apps/web/src/app/api/files/[id]/route.ts b/apps/web/src/app/api/files/[id]/route.ts index 058947a..db46fd3 100644 --- a/apps/web/src/app/api/files/[id]/route.ts +++ b/apps/web/src/app/api/files/[id]/route.ts @@ -19,7 +19,7 @@ export async function GET( const file = await prisma.fileUpload.findUnique({ where: { id }, - select: { filename: true, mime: true, storageKey: true }, + select: { filename: true, storageKey: true }, }); if (!file) { return NextResponse.json({ error: "File not found." }, { status: 404 }); @@ -30,10 +30,16 @@ export async function GET( return NextResponse.json({ error: "File not found." }, { status: 404 }); } + // Serve as an opaque download, never inline. A user-uploaded file could be + // HTML/SVG that would execute script if rendered same-origin; forcing + // `attachment` + `application/octet-stream` makes the browser save it instead + // of interpreting it (nosniff is also set globally in proxy.ts). The filename + // is the applicant's display name only and never reaches the served body. const headers = new Headers({ - "Content-Type": file.mime || object.contentType, - "Content-Disposition": `inline; filename*=UTF-8''${encodeURIComponent(file.filename)}`, + "Content-Type": "application/octet-stream", + "Content-Disposition": `attachment; filename*=UTF-8''${encodeURIComponent(file.filename)}`, "Cache-Control": "private, max-age=0, no-store", + "X-Content-Type-Options": "nosniff", }); if (object.contentLength) headers.set("Content-Length", String(object.contentLength)); diff --git a/apps/web/src/app/api/forms/[slug]/submit/route.ts b/apps/web/src/app/api/forms/[slug]/submit/route.ts index de1fcc4..5ebb729 100644 --- a/apps/web/src/app/api/forms/[slug]/submit/route.ts +++ b/apps/web/src/app/api/forms/[slug]/submit/route.ts @@ -13,6 +13,7 @@ import { getCurrentUser } from "@/lib/auth"; import { captchaEnabled, verifyCaptcha } from "@/lib/captcha"; import { parseFormSpec } from "@/lib/forms"; import { clientIp, rateLimit } from "@/lib/rate-limit"; +import { headObject } from "@/lib/s3"; export const runtime = "nodejs"; export const dynamic = "force-dynamic"; @@ -115,7 +116,10 @@ export async function POST( } // Turn file-field answers into FileUpload rows. Keys must live under this - // form's upload prefix — that's what ties an uploaded object to this form. + // form's upload prefix — that's what ties an uploaded object to this form — + // and the object must actually exist in storage. We trust the *server-stored* + // size/type (via HEAD), not the client-supplied descriptor; only the display + // filename comes from the client (and is never used to address the object). const data = result.data as Record; const fileFields = spec.pages .flatMap((p) => p.fields) @@ -127,7 +131,17 @@ export async function POST( if (!v.key.startsWith(`uploads/${form.id}/`)) { return NextResponse.json({ error: "Invalid file reference." }, { status: 400 }); } - fileRows.push({ fieldId: f.id, filename: v.name, mime: v.mime, size: v.size, storageKey: v.key }); + const head = await headObject(v.key); + if (!head) { + return NextResponse.json({ error: "Invalid file reference." }, { status: 400 }); + } + fileRows.push({ + fieldId: f.id, + filename: v.name, + mime: head.contentType, + size: head.contentLength, + storageKey: v.key, + }); } const user = await getCurrentUser(); diff --git a/apps/web/src/app/api/guilds/[guildId]/submissions/[id]/events/route.ts b/apps/web/src/app/api/guilds/[guildId]/submissions/[id]/events/route.ts index d24ac20..21292dc 100644 --- a/apps/web/src/app/api/guilds/[guildId]/submissions/[id]/events/route.ts +++ b/apps/web/src/app/api/guilds/[guildId]/submissions/[id]/events/route.ts @@ -1,4 +1,4 @@ -import { Prisma, prisma } from "@msk-forms/db"; +import { changeSubmissionStatus, Prisma, prisma } from "@msk-forms/db"; import { DEFAULT_STATUSES, type MessageNotification, @@ -54,11 +54,6 @@ export async function POST( const action = parsed.data; if (action.kind === "status") { - // Nothing to do if the status is unchanged. - if (action.status === submission.status) { - return NextResponse.json({ ok: true }); - } - // Only allow statuses from the default pipeline or the guild's own defs. const defs = await prisma.formStatusDef.findMany({ where: { guildId }, @@ -72,42 +67,24 @@ export async function POST( return NextResponse.json({ error: "Unknown status." }, { status: 422 }); } - const ops: Prisma.PrismaPromise[] = [ - prisma.submission.update({ - where: { id }, - data: { status: action.status }, - }), - prisma.submissionEvent.create({ - data: { + // Idempotent, race-safe transition + event + applicant DM (skip anonymous). + const notify: StatusChangeNotification | null = submission.userId + ? { submissionId: id, - actorUserId: user.id, - type: "status_change", - fromStatus: submission.status, + formTitle: submission.form.title, toStatus: action.status, - visibility: "public", - }, - }), - ]; - // Queue an outbox DM for the applicant (the bot delivers it). Skip - // anonymous submissions — there's no Discord user to notify. - if (submission.userId) { - const payload: StatusChangeNotification = { - submissionId: id, - formTitle: submission.form.title, - toStatus: action.status, - toStatusLabel: resolveStatus(action.status, defs).label, - }; - ops.push( - prisma.notification.create({ - data: { - userId: submission.userId, - type: "status_change", - payload: payload as unknown as Prisma.InputJsonValue, - }, - }), - ); - } - await prisma.$transaction(ops); + toStatusLabel: resolveStatus(action.status, defs).label, + } + : null; + await changeSubmissionStatus({ + submissionId: id, + toStatus: action.status, + actorUserId: user.id, + notify: + submission.userId && notify + ? { userId: submission.userId, type: "status_change", payload: notify } + : null, + }); return NextResponse.json({ ok: true }); } diff --git a/apps/web/src/lib/client-ip.test.ts b/apps/web/src/lib/client-ip.test.ts index 358a096..d72a65c 100644 --- a/apps/web/src/lib/client-ip.test.ts +++ b/apps/web/src/lib/client-ip.test.ts @@ -3,9 +3,29 @@ import { describe, expect, it } from "vitest"; import { clientIp } from "./client-ip"; describe("clientIp", () => { - it("takes the first hop of X-Forwarded-For", () => { - const h = new Headers({ "x-forwarded-for": "203.0.113.5, 10.0.0.1, 127.0.0.1" }); - expect(clientIp(h)).toBe("203.0.113.5"); + it("takes the last hop of X-Forwarded-For (the one our trusted proxy appended)", () => { + const h = new Headers({ "x-forwarded-for": "203.0.113.5, 10.0.0.1, 198.51.100.7" }); + expect(clientIp(h)).toBe("198.51.100.7"); + }); + + it("ignores a client-spoofed left entry", () => { + // Attacker sends "X-Forwarded-For: 1.2.3.4"; Apache appends the real peer. + const h = new Headers({ "x-forwarded-for": "1.2.3.4, 203.0.113.9" }); + expect(clientIp(h)).toBe("203.0.113.9"); + }); + + it("handles a single entry", () => { + expect(clientIp(new Headers({ "x-forwarded-for": "203.0.113.5" }))).toBe("203.0.113.5"); + }); + + it("accepts IPv6 addresses", () => { + const h = new Headers({ "x-forwarded-for": "2001:db8::1" }); + expect(clientIp(h)).toBe("2001:db8::1"); + }); + + it("rejects a malformed last entry and falls through", () => { + const h = new Headers({ "x-forwarded-for": "203.0.113.5, not-an-ip" }); + expect(clientIp(h)).toBe("unknown"); }); it("falls back to x-real-ip", () => { diff --git a/apps/web/src/lib/client-ip.ts b/apps/web/src/lib/client-ip.ts index a2163db..fbfcfe9 100644 --- a/apps/web/src/lib/client-ip.ts +++ b/apps/web/src/lib/client-ip.ts @@ -1,16 +1,35 @@ /** * Best-effort client IP. Behind the Apache reverse proxy the socket peer is - * always 127.0.0.1, so the real client is in `X-Forwarded-For` (mod_proxy adds - * it by default). Falls back to `x-real-ip`, then a constant (which buckets all - * callers together — acceptable degradation if no proxy header is present). + * always 127.0.0.1, so the real client is in `X-Forwarded-For`. + * + * `X-Forwarded-For` is `client, proxy1, proxy2, …` — appended left-to-right as + * the request passes through each hop. A client can spoof the *left* part by + * sending its own header; our trusted proxy (Apache) appends the real peer at + * the **right**. With exactly one trusted proxy in front, the **last** entry is + * the only trustworthy one, so we key the rate limiter on it. Falls back to + * `x-real-ip`, then a constant (which buckets all callers together — acceptable + * degradation if no proxy header is present). * * Pure (no server-only / Redis imports) so it stays unit-testable. */ export function clientIp(headers: Headers): string { const xff = headers.get("x-forwarded-for"); if (xff) { - const first = xff.split(",")[0]?.trim(); - if (first) return first; + const parts = xff.split(",").map((p) => p.trim()); + const last = parts[parts.length - 1]; + if (last && isIpish(last)) return last; } - return headers.get("x-real-ip")?.trim() || "unknown"; + const real = headers.get("x-real-ip")?.trim(); + if (real && isIpish(real)) return real; + return "unknown"; +} + +/** + * Loose IPv4/IPv6 shape check. Not full validation — just enough to reject + * obvious junk so a malformed header can't pollute the rate-limit keyspace. + */ +function isIpish(value: string): boolean { + if (value.length > 45) return false; // longest possible IPv6 textual form + // IPv4 dotted-quad, or IPv6 (hex groups + colons, optional zone/embedded v4). + return /^\d{1,3}(\.\d{1,3}){3}$/.test(value) || /^[0-9a-fA-F:]+(%[0-9a-zA-Z]+)?$/.test(value); } diff --git a/apps/web/src/lib/s3.ts b/apps/web/src/lib/s3.ts index 0926f18..966976e 100644 --- a/apps/web/src/lib/s3.ts +++ b/apps/web/src/lib/s3.ts @@ -3,6 +3,7 @@ import "server-only"; import { DeleteObjectCommand, GetObjectCommand, + HeadObjectCommand, PutObjectCommand, S3Client, } from "@aws-sdk/client-s3"; @@ -61,6 +62,28 @@ export async function deleteObject(key: string): Promise { } } +/** + * Look up an object's stored metadata without downloading it, or null if it's + * missing/unconfigured. Used on submit to confirm an uploaded object actually + * exists and to read its *server-recorded* size/type instead of trusting the + * client-supplied descriptor. + */ +export async function headObject( + key: string, +): Promise<{ contentType: string; contentLength: number } | null> { + const s3 = getS3(); + if (!s3) return null; + try { + const out = await s3.send(new HeadObjectCommand({ Bucket: s3Bucket(), Key: key })); + return { + contentType: out.ContentType ?? "application/octet-stream", + contentLength: out.ContentLength ?? 0, + }; + } catch { + return null; + } +} + /** Fetch an object as a web stream plus its metadata, or null if missing. */ export async function getObject( key: string, diff --git a/packages/db/src/index.ts b/packages/db/src/index.ts index d112edb..27a8f19 100644 --- a/packages/db/src/index.ts +++ b/packages/db/src/index.ts @@ -22,3 +22,4 @@ if (process.env.NODE_ENV !== "production") { } export * from "./generated/prisma/client"; +export * from "./submission-status"; diff --git a/packages/db/src/submission-status.ts b/packages/db/src/submission-status.ts new file mode 100644 index 0000000..9cd3afb --- /dev/null +++ b/packages/db/src/submission-status.ts @@ -0,0 +1,83 @@ +import { Prisma, prisma } from "./index"; + +export interface StatusChangeOutboxNotification { + /** Discord-linked applicant to DM (omit for anonymous submissions). */ + userId: string; + /** Notification type, e.g. "status_change". */ + type: string; + /** Serialized notification payload (shape owned by @msk-forms/shared). */ + payload: unknown; +} + +export interface ChangeSubmissionStatusArgs { + submissionId: string; + /** Target status key (already validated against the allowed set by the caller). */ + toStatus: string; + /** Reviewer who triggered the change, or null for bot/system actions. */ + actorUserId?: string | null; + /** When set, queue an outbox DM for the applicant in the same transaction. */ + notify?: StatusChangeOutboxNotification | null; +} + +export interface ChangeSubmissionStatusResult { + /** False if the submission was already at `toStatus` (or lost a race). */ + changed: boolean; + /** The status before the change (only meaningful when `changed` is true). */ + fromStatus: string | null; +} + +/** + * Idempotent, race-safe status transition shared by the web review route and + * the bot's Accept/Reject buttons. Reads the current status, then writes guarded + * by that exact value (`updateMany ... where status = current`) so two concurrent + * reviewers can't both record the transition. Writes the status-change event and + * (optionally) an outbox notification in the same transaction. A no-op when the + * submission is already at `toStatus`. + */ +export async function changeSubmissionStatus( + args: ChangeSubmissionStatusArgs, +): Promise { + const { submissionId, toStatus, actorUserId = null, notify = null } = args; + + return prisma.$transaction(async (tx) => { + const current = await tx.submission.findUnique({ + where: { id: submissionId }, + select: { status: true }, + }); + if (!current || current.status === toStatus) { + return { changed: false, fromStatus: current?.status ?? null }; + } + + // Guarded write: only succeeds if the status is still what we just read. + const updated = await tx.submission.updateMany({ + where: { id: submissionId, status: current.status }, + data: { status: toStatus }, + }); + if (updated.count === 0) { + return { changed: false, fromStatus: null }; // lost the race to another reviewer + } + + await tx.submissionEvent.create({ + data: { + submissionId, + actorUserId, + type: "status_change", + fromStatus: current.status, + toStatus, + visibility: "public", + }, + }); + + if (notify) { + await tx.notification.create({ + data: { + userId: notify.userId, + type: notify.type, + payload: notify.payload as Prisma.InputJsonValue, + }, + }); + } + + return { changed: true, fromStatus: current.status }; + }); +} diff --git a/packages/shared/src/form-spec.test.ts b/packages/shared/src/form-spec.test.ts index 58c4ec9..d8a82df 100644 --- a/packages/shared/src/form-spec.test.ts +++ b/packages/shared/src/form-spec.test.ts @@ -1,5 +1,24 @@ import { describe, expect, it } from "vitest"; -import { buildAnswerSchema, formSpecSchema, type FormSpec } from "./form-spec.js"; +import { + buildAnswerSchema, + formSpecSchema, + isLayoutField, + type FormField, + type FormSpec, +} from "./form-spec.js"; + +/** Build a one-field spec to exercise `buildFieldSchema` via the public API. */ +function specWith(field: Partial & Pick): FormSpec { + return { + version: 1, + pages: [ + { + id: "p1", + fields: [{ width: "full", validation: { required: false }, conditional: [], ...field }], + }, + ], + }; +} const spec: FormSpec = { version: 1, @@ -26,10 +45,95 @@ describe("formSpecSchema", () => { }); }); +describe("isLayoutField", () => { + it("classifies layout vs answer fields", () => { + expect(isLayoutField("heading")).toBe(true); + expect(isLayoutField("divider")).toBe(true); + expect(isLayoutField("short_text")).toBe(false); + expect(isLayoutField("file_upload")).toBe(false); + }); +}); + describe("buildAnswerSchema", () => { it("enforces required fields and ignores layout fields", () => { const answerSchema = buildAnswerSchema(spec); expect(answerSchema.safeParse({ name: "Moritz", age: 30 }).success).toBe(true); expect(answerSchema.safeParse({ age: 30 }).success).toBe(false); // name missing }); + + it("rejects an empty string for a required text field", () => { + const s = buildAnswerSchema(specWith({ id: "x", type: "short_text", validation: { required: true } })); + expect(s.safeParse({ x: "" }).success).toBe(false); + expect(s.safeParse({ x: "ok" }).success).toBe(true); + }); + + it("enforces number min/max", () => { + const s = buildAnswerSchema( + specWith({ id: "n", type: "number", validation: { required: true, min: 18, max: 99 } }), + ); + expect(s.safeParse({ n: 17 }).success).toBe(false); + expect(s.safeParse({ n: 100 }).success).toBe(false); + expect(s.safeParse({ n: 30 }).success).toBe(true); + }); + + it("enforces string length and pattern", () => { + const s = buildAnswerSchema( + specWith({ + id: "t", + type: "short_text", + validation: { required: true, minLength: 2, maxLength: 5, pattern: "^[a-z]+$" }, + }), + ); + expect(s.safeParse({ t: "a" }).success).toBe(false); // too short + expect(s.safeParse({ t: "abcdef" }).success).toBe(false); // too long + expect(s.safeParse({ t: "AB" }).success).toBe(false); // pattern + expect(s.safeParse({ t: "abc" }).success).toBe(true); + }); + + it("validates the email format for email fields", () => { + const s = buildAnswerSchema(specWith({ id: "e", type: "email", validation: { required: true } })); + expect(s.safeParse({ e: "not-an-email" }).success).toBe(false); + expect(s.safeParse({ e: "a@b.com" }).success).toBe(true); + }); + + it("restricts single-choice answers to the option set", () => { + const s = buildAnswerSchema( + specWith({ + id: "c", + type: "single_choice", + validation: { required: true }, + options: [ + { value: "a", label: "A" }, + { value: "b", label: "B" }, + ], + }), + ); + expect(s.safeParse({ c: "z" }).success).toBe(false); + expect(s.safeParse({ c: "a" }).success).toBe(true); + }); + + it("restricts multi-choice answers to the option set and count", () => { + const s = buildAnswerSchema( + specWith({ + id: "m", + type: "multi_choice", + validation: { required: true, max: 2 }, + options: [ + { value: "a", label: "A" }, + { value: "b", label: "B" }, + { value: "c", label: "C" }, + ], + }), + ); + expect(s.safeParse({ m: ["a", "z"] }).success).toBe(false); // unknown option + expect(s.safeParse({ m: ["a", "b", "c"] }).success).toBe(false); // over max + expect(s.safeParse({ m: ["a", "b"] }).success).toBe(true); + }); + + it("ignores an invalid stored regex pattern instead of rejecting everything", () => { + const s = buildAnswerSchema( + specWith({ id: "t", type: "short_text", validation: { required: true, pattern: "(" } }), + ); + expect(s.safeParse({ t: "anything" }).success).toBe(true); + }); }); diff --git a/packages/shared/src/form-spec.ts b/packages/shared/src/form-spec.ts index c064ed1..aa5ba88 100644 --- a/packages/shared/src/form-spec.ts +++ b/packages/shared/src/form-spec.ts @@ -137,26 +137,87 @@ export const fileAnswerSchema = z.object({ }); export type FileAnswer = z.infer; -/** Builds a dynamic zod schema to validate a submission against a form spec. */ +/** Field types that carry no answer (pure layout/markup). */ +export const LAYOUT_FIELD_TYPES = [ + "section_break", + "heading", + "paragraph", + "image_block", + "divider", + "spacer", +] as const; + +/** True for layout-only fields, which never produce a submission answer. */ +export function isLayoutField(type: FieldType): boolean { + return (LAYOUT_FIELD_TYPES as readonly string[]).includes(type); +} + +const NUMBER_TYPES = ["number", "slider", "nps", "rating_stars"]; +const MULTI_TYPES = ["multi_choice", "multi_select", "ranking"]; +const BOOLEAN_TYPES = ["yes_no", "consent", "age_check"]; +const SINGLE_CHOICE_TYPES = ["single_choice", "dropdown"]; + +/** + * Builds a dynamic zod schema to validate a submission against a form spec. + * Enforces the full `field.validation` contract server-side — not just + * `required` — so the API never trusts the client to honor min/max, lengths, + * patterns, formats, or the allowed option set. + */ export function buildAnswerSchema(spec: FormSpec): z.ZodTypeAny { const shape: Record = {}; for (const page of spec.pages) { for (const field of page.fields) { - if (["section_break", "heading", "paragraph", "image_block", "divider", "spacer"].includes(field.type)) { - continue; - } - let base: z.ZodTypeAny = z.string(); - if (field.type === "number" || field.type === "slider" || field.type === "nps" || field.type === "rating_stars") { - base = z.number(); - } else if (field.type === "multi_choice" || field.type === "multi_select" || field.type === "ranking") { - base = z.array(z.string()); - } else if (field.type === "yes_no" || field.type === "consent" || field.type === "age_check") { - base = z.boolean(); - } else if ((FILE_FIELD_TYPES as readonly string[]).includes(field.type)) { - base = fileAnswerSchema; - } - shape[field.id] = field.validation.required ? base : base.optional(); + if (isLayoutField(field.type)) continue; + shape[field.id] = buildFieldSchema(field); } } return z.object(shape); } + +function buildFieldSchema(field: FormField): z.ZodTypeAny { + const v = field.validation; + const optionValues = field.options?.map((o) => o.value) ?? []; + let base: z.ZodTypeAny; + + if (NUMBER_TYPES.includes(field.type)) { + let n = z.number(); + if (v.min !== undefined) n = n.min(v.min); + if (v.max !== undefined) n = n.max(v.max); + base = n; + } else if (MULTI_TYPES.includes(field.type)) { + let arr = z.array(z.string()); + if (v.min !== undefined) arr = arr.min(v.min); + if (v.max !== undefined) arr = arr.max(v.max); + base = + optionValues.length > 0 + ? arr.refine((vals) => vals.every((x) => optionValues.includes(x)), "Invalid option.") + : arr; + } else if (BOOLEAN_TYPES.includes(field.type)) { + base = z.boolean(); + } else if ((FILE_FIELD_TYPES as readonly string[]).includes(field.type)) { + base = fileAnswerSchema; + } else if (SINGLE_CHOICE_TYPES.includes(field.type)) { + base = optionValues.length > 0 ? z.enum(optionValues as [string, ...string[]]) : z.string(); + } else { + // Text-like fields (short_text, long_text, email, url, phone, etc.). + let s = z.string(); + if (v.minLength !== undefined) s = s.min(v.minLength); + if (v.maxLength !== undefined) s = s.max(v.maxLength); + if (field.type === "email") s = s.email(); + if (field.type === "url") s = s.url(); + if (v.pattern) { + try { + const re = new RegExp(v.pattern); + s = s.regex(re, "Invalid format."); + } catch { + // Ignore an invalid stored pattern rather than rejecting everything. + } + } + base = s; + } + + if (!v.required) return base.optional(); + // A required field must be present; for text, also reject the empty string. + if (base instanceof z.ZodString) return (base as z.ZodString).min(v.minLength ?? 1); + return base; +}