From f20a9e97ddded1b4c11fae00a52ec1ab7a00b939 Mon Sep 17 00:00:00 2001 From: Musiker15 Date: Sat, 20 Jun 2026 19:39:00 +0200 Subject: [PATCH] refactor: dedupe layout-type/answer-format logic; test rate-limit & captcha Maintainability follow-up from the review (PR B): - Replace the three copies of the full layout-type list (submit route, field-input, answer-summary) with the shared `isLayoutField`. Rename the builder's narrower subset to `BUILDER_LAYOUT_TYPES` to make clear it is a different concept (only the layout types the builder offers). - Extract a framework-agnostic `formatAnswerValue` into @msk-forms/shared and use it from both the reviewer/status answer summary and the bot review-embed preview, so the two formatters can't drift. - Add unit tests for `rateLimit` (fail-open: no Redis / command error / limit boundaries) and `verifyCaptcha` (pass-through when off, fail-closed on missing token / failure / HTTP error / thrown fetch). A vitest `server-only` stub alias makes these server-side libs testable. --- .../src/app/api/forms/[slug]/submit/route.ts | 23 ++--- apps/web/src/components/form/field-input.tsx | 10 --- .../web/src/components/form/form-renderer.tsx | 6 +- .../components/submission/answer-summary.tsx | 22 ++--- apps/web/src/lib/builder-fields.ts | 6 +- apps/web/src/lib/captcha.test.ts | 85 +++++++++++++++++++ apps/web/src/lib/rate-limit.test.ts | 63 ++++++++++++++ apps/web/src/test/server-only.stub.ts | 4 + apps/web/vitest.config.ts | 12 +++ packages/shared/src/form-spec.ts | 33 +++++++ 10 files changed, 218 insertions(+), 46 deletions(-) create mode 100644 apps/web/src/lib/captcha.test.ts create mode 100644 apps/web/src/lib/rate-limit.test.ts create mode 100644 apps/web/src/test/server-only.stub.ts create mode 100644 apps/web/vitest.config.ts 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 5ebb729..68663d9 100644 --- a/apps/web/src/app/api/forms/[slug]/submit/route.ts +++ b/apps/web/src/app/api/forms/[slug]/submit/route.ts @@ -2,8 +2,9 @@ import { prisma, type Prisma } from "@msk-forms/db"; import { buildAnswerSchema, FILE_FIELD_TYPES, + formatAnswerValue, + isLayoutField, type FileAnswer, - type FormField, type FormSpec, type SubmissionReviewNotification, } from "@msk-forms/shared"; @@ -22,29 +23,15 @@ export const dynamic = "force-dynamic"; const SUBMIT_LIMIT = 8; const SUBMIT_WINDOW_SECONDS = 60; -const LAYOUT_TYPES = ["section_break", "heading", "paragraph", "image_block", "divider", "spacer"]; +const PREVIEW_LABELS = { empty: "—", yes: "Yes", no: "No" } as const; /** Short "Label: value" lines for the bot's review embed (first few fields). */ function buildPreview(spec: FormSpec, data: Record): string[] { - const labelFor = (field: FormField, v: string) => - field.options?.find((o) => o.value === v)?.label ?? v; - const valueOf = (field: FormField, value: unknown): string => { - if (value === undefined || value === null || value === "") return "—"; - if (typeof value === "boolean") return value ? "Yes" : "No"; - if (Array.isArray(value)) return value.map((v) => labelFor(field, String(v))).join(", "); - // null/undefined already returned above, so a plain object check suffices. - if (typeof value === "object" && "name" in value) { - return String((value as { name: unknown }).name); - } - if (field.options) return labelFor(field, String(value)); - return String(value); - }; - return spec.pages .flatMap((p) => p.fields) - .filter((f) => !LAYOUT_TYPES.includes(f.type)) + .filter((f) => !isLayoutField(f.type)) .slice(0, 6) - .map((f) => `${f.label ?? f.id}: ${valueOf(f, data[f.id]).slice(0, 100)}`); + .map((f) => `${f.label ?? f.id}: ${formatAnswerValue(f, data[f.id], PREVIEW_LABELS).slice(0, 100)}`); } /** diff --git a/apps/web/src/components/form/field-input.tsx b/apps/web/src/components/form/field-input.tsx index 93a1bb2..a49741a 100644 --- a/apps/web/src/components/form/field-input.tsx +++ b/apps/web/src/components/form/field-input.tsx @@ -14,16 +14,6 @@ import { FileField, type FileFieldLabels } from "./file-field"; export type FieldValue = string | number | boolean | string[] | FileAnswer | undefined; -/** Field types that carry no answer value (rendered as layout only). */ -export const LAYOUT_TYPES = [ - "section_break", - "heading", - "paragraph", - "image_block", - "divider", - "spacer", -] as const; - const YES_NO_OPTIONS = [ { value: "yes", label: "Yes" }, { value: "no", label: "No" }, diff --git a/apps/web/src/components/form/form-renderer.tsx b/apps/web/src/components/form/form-renderer.tsx index 44dc1ec..0b9326c 100644 --- a/apps/web/src/components/form/form-renderer.tsx +++ b/apps/web/src/components/form/form-renderer.tsx @@ -1,19 +1,19 @@ "use client"; -import type { FormField, FormSpec } from "@msk-forms/shared"; +import { isLayoutField, type FormField, type FormSpec } from "@msk-forms/shared"; import { Button, Field } from "@msk-forms/ui"; import type { Route } from "next"; import { useRouter } from "next/navigation"; import { useState } from "react"; -import { FieldInput, LAYOUT_TYPES, type FieldValue } from "./field-input"; +import { FieldInput, type FieldValue } from "./field-input"; import { LayoutBlock } from "./layout-block"; import { TurnstileWidget } from "./turnstile-widget"; type Answers = Record; function isLayout(field: FormField): boolean { - return (LAYOUT_TYPES as readonly string[]).includes(field.type); + return isLayoutField(field.type); } /** True when a required field has no meaningful answer. */ diff --git a/apps/web/src/components/submission/answer-summary.tsx b/apps/web/src/components/submission/answer-summary.tsx index 1baa11d..19c9f38 100644 --- a/apps/web/src/components/submission/answer-summary.tsx +++ b/apps/web/src/components/submission/answer-summary.tsx @@ -1,4 +1,10 @@ -import { FILE_FIELD_TYPES, type FormField, type FormSpec } from "@msk-forms/shared"; +import { + FILE_FIELD_TYPES, + formatAnswerValue, + isLayoutField, + type FormField, + type FormSpec, +} from "@msk-forms/shared"; /** Labels needed to format answers, shared by the public + reviewer views. */ export interface AnswerLabels { @@ -14,18 +20,8 @@ export interface SubmissionFile { filename: string; } -/** Layout-only field types carry no answer and are skipped in the summary. */ -const LAYOUT = ["section_break", "heading", "paragraph", "image_block", "divider", "spacer"]; - function formatAnswer(field: FormField, value: unknown, t: AnswerLabels): string { - if (value === undefined || value === null || value === "") return t.notAnswered; - if (typeof value === "boolean") return value ? t.yes : t.no; - - const labelFor = (v: string) => field.options?.find((o) => o.value === v)?.label ?? v; - - if (Array.isArray(value)) return value.map((v) => labelFor(String(v))).join(", "); - if (field.options) return labelFor(String(value)); - return String(value); + return formatAnswerValue(field, value, { empty: t.notAnswered, yes: t.yes, no: t.no }); } /** Read-only definition list of a submission's answers, by form field. */ @@ -40,7 +36,7 @@ export function AnswerSummary({ labels: AnswerLabels; files?: SubmissionFile[]; }) { - const fields = spec.pages.flatMap((p) => p.fields).filter((f) => !LAYOUT.includes(f.type)); + const fields = spec.pages.flatMap((p) => p.fields).filter((f) => !isLayoutField(f.type)); const isFileField = (f: FormField) => (FILE_FIELD_TYPES as readonly string[]).includes(f.type); return ( diff --git a/apps/web/src/lib/builder-fields.ts b/apps/web/src/lib/builder-fields.ts index c1130e6..d60d1a7 100644 --- a/apps/web/src/lib/builder-fields.ts +++ b/apps/web/src/lib/builder-fields.ts @@ -22,10 +22,12 @@ export const BUILDER_FIELDS: { type: FieldType; label: string }[] = [ ]; const CHOICE_TYPES: FieldType[] = ["single_choice", "dropdown", "multi_choice"]; -const LAYOUT_TYPES: FieldType[] = ["heading", "paragraph", "divider"]; +// Distinct from shared LAYOUT_FIELD_TYPES: only the layout types the builder +// currently offers (not every layout type the renderer can display). +const BUILDER_LAYOUT_TYPES: FieldType[] = ["heading", "paragraph", "divider"]; export const fieldTypeLabel = (type: FieldType): string => BUILDER_FIELDS.find((f) => f.type === type)?.label ?? type; export const needsOptions = (type: FieldType): boolean => CHOICE_TYPES.includes(type); -export const isLayoutType = (type: FieldType): boolean => LAYOUT_TYPES.includes(type); +export const isLayoutType = (type: FieldType): boolean => BUILDER_LAYOUT_TYPES.includes(type); diff --git a/apps/web/src/lib/captcha.test.ts b/apps/web/src/lib/captcha.test.ts new file mode 100644 index 0000000..d7442d5 --- /dev/null +++ b/apps/web/src/lib/captcha.test.ts @@ -0,0 +1,85 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { captchaEnabled, captchaSiteKey, verifyCaptcha } from "./captcha"; + +const ENV = { site: process.env.CAPTCHA_SITE_KEY, secret: process.env.CAPTCHA_SECRET_KEY }; + +afterEach(() => { + process.env.CAPTCHA_SITE_KEY = ENV.site; + process.env.CAPTCHA_SECRET_KEY = ENV.secret; + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +beforeEach(() => { + delete process.env.CAPTCHA_SITE_KEY; + delete process.env.CAPTCHA_SECRET_KEY; +}); + +describe("captchaEnabled / captchaSiteKey", () => { + it("is off unless BOTH keys are set", () => { + expect(captchaEnabled()).toBe(false); + process.env.CAPTCHA_SITE_KEY = "site"; + expect(captchaEnabled()).toBe(false); + expect(captchaSiteKey()).toBe("site"); + process.env.CAPTCHA_SECRET_KEY = "secret"; + expect(captchaEnabled()).toBe(true); + }); +}); + +describe("verifyCaptcha", () => { + it("passes through when captcha is not configured (nothing to enforce)", async () => { + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + expect(await verifyCaptcha(undefined)).toBe(true); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("fails closed on a missing token when enabled", async () => { + process.env.CAPTCHA_SECRET_KEY = "secret"; + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + expect(await verifyCaptcha(undefined)).toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("returns true only when Cloudflare reports success", async () => { + process.env.CAPTCHA_SECRET_KEY = "secret"; + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: true, json: async () => ({ success: true }) })), + ); + expect(await verifyCaptcha("token")).toBe(true); + }); + + it("fails closed when Cloudflare reports failure", async () => { + process.env.CAPTCHA_SECRET_KEY = "secret"; + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: true, json: async () => ({ success: false }) })), + ); + expect(await verifyCaptcha("token")).toBe(false); + }); + + it("fails closed on a non-OK HTTP response", async () => { + process.env.CAPTCHA_SECRET_KEY = "secret"; + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: false, status: 500, json: async () => ({}) })), + ); + expect(await verifyCaptcha("token")).toBe(false); + }); + + it("fails closed when fetch throws", async () => { + process.env.CAPTCHA_SECRET_KEY = "secret"; + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("network down"); + }), + ); + expect(await verifyCaptcha("token")).toBe(false); + }); +}); diff --git a/apps/web/src/lib/rate-limit.test.ts b/apps/web/src/lib/rate-limit.test.ts new file mode 100644 index 0000000..ca7263e --- /dev/null +++ b/apps/web/src/lib/rate-limit.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { getRedis } from "./redis"; +import { rateLimit } from "./rate-limit"; + +// Control the Redis singleton without a real connection. +vi.mock("./redis", () => ({ getRedis: vi.fn() })); + +const mockedGetRedis = vi.mocked(getRedis); + +afterEach(() => { + vi.restoreAllMocks(); + mockedGetRedis.mockReset(); +}); + +describe("rateLimit", () => { + it("fails open (allows) when Redis is not configured", async () => { + mockedGetRedis.mockReturnValue(null); + const r = await rateLimit("k", 5, 60); + expect(r.allowed).toBe(true); + expect(r.remaining).toBe(5); + }); + + it("fails open when the Redis command throws", async () => { + vi.spyOn(console, "error").mockImplementation(() => {}); + mockedGetRedis.mockReturnValue({ + eval: vi.fn(async () => { + throw new Error("redis down"); + }), + } as never); + const r = await rateLimit("k", 5, 60); + expect(r.allowed).toBe(true); + }); + + it("allows while under the limit and reports remaining", async () => { + mockedGetRedis.mockReturnValue({ eval: vi.fn(async () => [3, 42]) } as never); + const r = await rateLimit("k", 5, 60); + expect(r.allowed).toBe(true); + expect(r.remaining).toBe(2); + expect(r.retryAfter).toBe(42); + }); + + it("blocks once the count exceeds the limit", async () => { + mockedGetRedis.mockReturnValue({ eval: vi.fn(async () => [6, 30]) } as never); + const r = await rateLimit("k", 5, 60); + expect(r.allowed).toBe(false); + expect(r.remaining).toBe(0); + expect(r.retryAfter).toBe(30); + }); + + it("allows exactly at the limit boundary", async () => { + mockedGetRedis.mockReturnValue({ eval: vi.fn(async () => [5, 10]) } as never); + const r = await rateLimit("k", 5, 60); + expect(r.allowed).toBe(true); + expect(r.remaining).toBe(0); + }); + + it("falls back to the window when TTL is unavailable", async () => { + mockedGetRedis.mockReturnValue({ eval: vi.fn(async () => [6, -1]) } as never); + const r = await rateLimit("k", 5, 90); + expect(r.retryAfter).toBe(90); + }); +}); diff --git a/apps/web/src/test/server-only.stub.ts b/apps/web/src/test/server-only.stub.ts new file mode 100644 index 0000000..3ffb07b --- /dev/null +++ b/apps/web/src/test/server-only.stub.ts @@ -0,0 +1,4 @@ +// Vitest stub for Next.js's `server-only` guard, which only resolves inside the +// Next build. Aliased in vitest.config.ts so server-side libs (redis, captcha, +// rate-limit) can be unit-tested. +export {}; diff --git a/apps/web/vitest.config.ts b/apps/web/vitest.config.ts new file mode 100644 index 0000000..be6455b --- /dev/null +++ b/apps/web/vitest.config.ts @@ -0,0 +1,12 @@ +import { fileURLToPath } from "node:url"; + +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + resolve: { + alias: { + // `server-only` only resolves inside the Next build; stub it for unit tests. + "server-only": fileURLToPath(new URL("./src/test/server-only.stub.ts", import.meta.url)), + }, + }, +}); diff --git a/packages/shared/src/form-spec.ts b/packages/shared/src/form-spec.ts index aa5ba88..d2d9c18 100644 --- a/packages/shared/src/form-spec.ts +++ b/packages/shared/src/form-spec.ts @@ -152,6 +152,39 @@ export function isLayoutField(type: FieldType): boolean { return (LAYOUT_FIELD_TYPES as readonly string[]).includes(type); } +/** Caller-supplied labels for {@link formatAnswerValue} (lets each surface i18n). */ +export interface AnswerValueLabels { + /** Shown for an empty/missing answer. */ + empty: string; + yes: string; + no: string; +} + +/** + * Framework-agnostic, human-readable rendering of a single answer value: maps + * option values to their labels, joins arrays, formats booleans, and shows a + * file-descriptor answer by its filename. Shared by the reviewer/status answer + * summary and the bot's review-embed preview so they never drift. + */ +export function formatAnswerValue( + field: FormField, + value: unknown, + labels: AnswerValueLabels, +): string { + if (value === undefined || value === null || value === "") return labels.empty; + if (typeof value === "boolean") return value ? labels.yes : labels.no; + + const labelFor = (v: string) => field.options?.find((o) => o.value === v)?.label ?? v; + + if (Array.isArray(value)) return value.map((v) => labelFor(String(v))).join(", "); + // File-descriptor answer ({ key, name, size, mime }) — show the filename. + if (typeof value === "object" && "name" in value) { + return String((value as { name: unknown }).name); + } + if (field.options) return labelFor(String(value)); + return String(value); +} + const NUMBER_TYPES = ["number", "slider", "nps", "rating_stars"]; const MULTI_TYPES = ["multi_choice", "multi_select", "ranking"]; const BOOLEAN_TYPES = ["yes_no", "consent", "age_check"];