harden: file delivery, submit metadata, client IP, status idempotency, answer validation

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<unknown>[] = [
-      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);

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));
 

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<string, unknown>;
   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();

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<unknown>[] = [
-      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 });
   }
 

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", () => {

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);
 }

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<void> {
   }
 }
 
+/**
+ * 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,

packages/db/src/index.ts

@@ -22,3 +22,4 @@ if (process.env.NODE_ENV !== "production") {
 }
 
 export * from "./generated/prisma/client";
+export * from "./submission-status";