diff --git a/app/api/admin/users/route.ts b/app/api/admin/users/route.ts index 48c48db..364d563 100644 --- a/app/api/admin/users/route.ts +++ b/app/api/admin/users/route.ts @@ -25,10 +25,9 @@ import { unauthorizedResponse, } from "@/lib/api-auth"; import { apiErrorResponse } from "@/lib/api-error"; +import { isValidEmail } from "@/lib/email"; import { getRequestId, logError } from "@/lib/log"; -const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; - function isValidRole(role: unknown): role is "admin" | "superadmin" { return role === "admin" || role === "superadmin"; } @@ -78,7 +77,7 @@ export async function POST(req: NextRequest) { const email = normalizeEmail(String(body.email ?? "")); const role = body.role; - if (!email || !EMAIL_REGEX.test(email)) { + if (!email || !isValidEmail(email)) { return badRequestResponse("A valid email address is required."); } if (!isValidRole(role)) { diff --git a/app/api/trade-inquiry/route.ts b/app/api/trade-inquiry/route.ts index 1658aa6..e1bee3c 100644 --- a/app/api/trade-inquiry/route.ts +++ b/app/api/trade-inquiry/route.ts @@ -1,22 +1,15 @@ import { NextRequest, NextResponse } from "next/server"; import { getRequestId, logError } from "@/lib/log"; import { submitTradeInquiry } from "@/lib/trade-leads"; -import { tradeLeadFieldTooLong } from "@/lib/trade-leads-common"; +import { + handleTradeInquiry, + type TradeInquiryBody, +} from "@/lib/trade-leads-common"; const RATE_LIMIT_WINDOW_MS = 10 * 60 * 1000; const RATE_LIMIT_MAX = 5; const requestLogByIp = new Map(); -interface TradeInquiryBody { - businessName: string; - contactName: string; - email: string; - phoneOrWhatsapp?: string; - venueType: string; - message?: string; - website?: string; -} - function getClientIp(req: NextRequest): string { const forwardedFor = req.headers.get("x-forwarded-for"); if (forwardedFor) { @@ -57,70 +50,12 @@ export async function POST(req: NextRequest) { ); } - const businessName = body.businessName?.trim() ?? ""; - const contactName = body.contactName?.trim() ?? ""; - const email = body.email?.trim() ?? ""; - const phoneOrWhatsapp = body.phoneOrWhatsapp?.trim() ?? ""; - const venueType = body.venueType?.trim() ?? ""; - const message = body.message?.trim() ?? ""; - const website = body.website?.trim() ?? ""; - - if (!businessName || !contactName || !email || !venueType) { - return NextResponse.json( - { - ok: false, - error: - "Missing required fields: businessName, contactName, email, venueType.", - }, - { status: 400 } - ); - } - - const oversized = tradeLeadFieldTooLong({ - businessName, - contactName, - email, - phoneOrWhatsapp, - venueType, - message, - }); - if (oversized) { - return NextResponse.json( - { ok: false, error: `Field exceeds maximum length: ${oversized}.` }, - { status: 400 } - ); - } - - // Honeypot: pretend success for bots, but persist nothing and send no email. - if (website) { - return NextResponse.json({ ok: true }); - } - - const clientIp = getClientIp(req); - if (isRateLimited(clientIp)) { - return NextResponse.json( - { ok: false, error: "Too many requests. Please try again later." }, - { status: 429 } - ); - } - - // Firestore is the system of record: the inquiry must be persisted before - // we claim success. The Resend notification is best-effort inside - // submitTradeInquiry — its failure is logged, not surfaced to the customer. - const outcome = await submitTradeInquiry( - { businessName, contactName, email, phoneOrWhatsapp, venueType, message }, - requestId + const result = await handleTradeInquiry( + body, + { clientIp: getClientIp(req), requestId }, + { isRateLimited, submit: submitTradeInquiry } ); - if (!outcome.ok) { - // Persistence failure was already logged as - // trade_inquiry.persistence_failed — respond generically. - return NextResponse.json( - { ok: false, error: "Failed to submit inquiry." }, - { status: 500 } - ); - } - - return NextResponse.json({ ok: true }); + return NextResponse.json(result.body, { status: result.status }); } catch (error) { logError("trade_inquiry.unexpected", error, { requestId }); return NextResponse.json( diff --git a/components/admin-access.tsx b/components/admin-access.tsx index 043d747..7394c54 100644 --- a/components/admin-access.tsx +++ b/components/admin-access.tsx @@ -4,6 +4,7 @@ import { useCallback, useEffect, useState } from "react"; import { Button } from "@/components/ui/button"; import { Badge } from "@/components/ui/badge"; import { formatAdminDate, formatAdminDateTime } from "@/lib/admin-format"; +import { isValidEmail } from "@/lib/email"; import type { AdminRole, AdminUserView, AdminInvitationView } from "@/lib/types"; // The panel only needs a token source — the real Firebase `User` satisfies @@ -24,8 +25,6 @@ interface AdminListResponse { error?: string; } -const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; - export function AdminAccessPanel({ user, onStatusMessage }: AdminAccessPanelProps) { const [admins, setAdmins] = useState([]); const [invitations, setInvitations] = useState([]); @@ -63,7 +62,7 @@ export function AdminAccessPanel({ user, onStatusMessage }: AdminAccessPanelProp async function invite(e: React.FormEvent) { e.preventDefault(); const email = inviteEmail.trim().toLowerCase(); - if (!EMAIL_REGEX.test(email)) { + if (!isValidEmail(email)) { onStatusMessage("Please enter a valid email address."); return; } diff --git a/docs/TECHNICAL.md b/docs/TECHNICAL.md index 6e9f903..a78ccd4 100644 --- a/docs/TECHNICAL.md +++ b/docs/TECHNICAL.md @@ -123,7 +123,7 @@ client SDK writes (content management) or through Admin-SDK-backed API routes | `app/api/` | Server API routes: `admin/bootstrap`, `admin/invitations/accept`, `admin/invitations/[id]/resend`, `admin/me`, `admin/rebuild`, `admin/users` (GET list + POST create-invitation), `admin/users/[uid]` (PATCH/DELETE), and `trade-inquiry`. All are Admin-SDK-protected except `trade-inquiry`. | | `components/` | App components: header/footer, home sections, cards, carousel/filter grid, analytics trackers, `admin-dashboard.tsx` (auth + data orchestration), `admin-workspace.tsx` (props-driven authenticated view shared with `/admin-fixture`), `admin-access.tsx`, `admin-fixture.tsx` (test-only data), `trade-inquiry-form.tsx`, `mdx-layout.tsx`. | | `components/ui/` | shadcn/ui primitives (Radix-based) configured by `components.json`. | -| `lib/` | Shared logic. Client-safe: `firebase.ts`, `beers.ts`, `venues.ts`, `analytics.ts`, `types.ts`, `utils.ts`, `trade-leads-common.ts`, admin `*-common`/`admin-format.ts` helpers. Server-only (`import "server-only"`): `firebase-admin.ts`, `admin-auth.ts`, `admin-users.ts`, `admin-invitations.ts`, `admin-invitation-email.ts`, `admin-invitation-resend-core.ts`, `admin-audit.ts`, `trade-leads.ts`. Policy/serialization helpers shared by both: `admin-policy.ts`, `admin-serializers.ts`, `admin-invitation-policy.ts`, `admin-invitation-resend-policy.ts`, `admin-types.ts`. | +| `lib/` | Shared logic. Client-safe: `firebase.ts`, `beers.ts`, `venues.ts`, `analytics.ts`, `types.ts`, `utils.ts`, `email.ts`, `trade-leads-common.ts`, admin `*-common`/`admin-format.ts` helpers. Server-only (`import "server-only"`): `firebase-admin.ts`, `admin-auth.ts`, `admin-users.ts`, `admin-invitations.ts`, `admin-invitation-email.ts`, `admin-invitation-resend-core.ts`, `admin-audit.ts`, `trade-leads.ts`. Policy/serialization helpers shared by both: `admin-policy.ts`, `admin-serializers.ts`, `admin-invitation-policy.ts`, `admin-invitation-resend-policy.ts`, `admin-types.ts`. | | `tests/` | Node `node:test` unit tests (`tsx` loader) for admin/auth/invitation/audit helpers and for the *contents* of `firestore.rules` and `storage.rules`. | | `rules-tests/` | Emulator-backed security-rules tests (`@firebase/rules-unit-testing` against the Firestore/Storage emulators). Run via `npm run test:rules`, which wraps `firebase emulators:exec`; each file uses its own `demo-*` project so parallel `node:test` files stay isolated. | | `scripts/` | Local/manual tooling: Playwright diagnostics (`*-check.mjs`, `hero-video-network.mjs`), `check-md-links.mjs`, `check-react-versions.mjs`, `optimize-assets.mjs`, `bootstrap-superadmin.ts`, `prune-trade-leads.ts`, `seed-beers.ts`, `seed-venues.ts`. `check-md-links.mjs` and `check-react-versions.mjs` run in CI; the Playwright diagnostics and prune script do not (CI browser coverage lives in `smoke-tests/`). | @@ -509,14 +509,18 @@ notification.** (Owner decision, #57.) Client-side state drives start/success/error analytics events (`trade_form_start`, `trade_form_success`, `trade_form_error`, category `conversion`). -2. **API route:** `app/api/trade-inquiry/route.ts` validates and rate-limits, - then delegates to `submitTradeInquiry()` in `lib/trade-leads.ts` - (server-only). The route: +2. **API route:** `app/api/trade-inquiry/route.ts` delegates to + `handleTradeInquiry()` in `lib/trade-leads-common.ts` (injectable and + unit-tested), which validates and rate-limits before calling + `submitTradeInquiry()` in `lib/trade-leads.ts` (server-only). The pipeline: - requires `businessName`, `contactName`, `email`, `venueType` (400 on missing); - - rejects oversized fields with 400 — `TRADE_LEAD_FIELD_LIMITS` in - `lib/trade-leads-common.ts` bounds each persisted field so a huge - submission is invalid input rather than a Firestore write failure; + - rejects oversized fields with 400 — `TRADE_LEAD_FIELD_LIMITS` bounds + each persisted field so a huge submission is invalid input rather than + a Firestore write failure; + - rejects a malformed `email` with 400 via the shared `isValidEmail()` + (`lib/email.ts`, also used by the admin invitation code) — before any + lead is persisted or notification attempted; - treats a filled `website` honeypot as spam and **returns fake `ok: true` without persisting or notifying**; - applies an in-memory rate limit — max 5 requests per client IP per diff --git a/lib/email.ts b/lib/email.ts new file mode 100644 index 0000000..91e2cdc --- /dev/null +++ b/lib/email.ts @@ -0,0 +1,10 @@ +// Canonical email-format check, shared by server routes and client +// components. Deliberately pragmatic — one "@", no whitespace, and a dot in +// the domain — not a full RFC 5322 parser. Dependency-free so client +// components can import it without pulling server modules into public +// bundles. +const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + +export function isValidEmail(email: string): boolean { + return EMAIL_REGEX.test(email); +} diff --git a/lib/trade-leads-common.ts b/lib/trade-leads-common.ts index d6f8dd2..e22916c 100644 --- a/lib/trade-leads-common.ts +++ b/lib/trade-leads-common.ts @@ -1,7 +1,8 @@ -// Shared, dependency-free trade-inquiry logic. The persistence/notification -// orchestration lives here (with injected collaborators) so it is unit-testable -// without Firebase Admin or Resend; `lib/trade-leads.ts` wires the real -// server-only dependencies. +// Shared, dependency-free trade-inquiry logic. The request-validation and +// persistence/notification orchestration lives here (with injected +// collaborators) so it is unit-testable without Firebase Admin or Resend; +// `lib/trade-leads.ts` wires the real server-only dependencies. +import { isValidEmail } from "@/lib/email"; import type { LogContext } from "@/lib/log"; export const TRADE_LEADS_COLLECTION = "tradeLeads"; @@ -121,6 +122,113 @@ export type TradeInquiryOutcome = | { ok: true; leadId: string } | { ok: false }; +// --- POST /api/trade-inquiry request pipeline --- + +// Untrusted JSON body shape; every field is optional at this boundary. +export interface TradeInquiryBody { + businessName?: string; + contactName?: string; + email?: string; + phoneOrWhatsapp?: string; + venueType?: string; + message?: string; + website?: string; +} + +export interface TradeInquiryRouteResult { + status: number; + body: { ok: boolean; error?: string }; +} + +export interface TradeInquiryRouteDeps { + isRateLimited: (clientIp: string) => boolean; + submit: ( + input: TradeLeadInput, + requestId: string + ) => Promise; +} + +// Everything between "JSON parsed" and "respond": trim → required fields → +// field-length bounds → email format → honeypot → rate limit → submit. +// All validation runs before the honeypot reply and before `submit`, so a +// malformed email can never reach Firestore or Resend. +export async function handleTradeInquiry( + body: TradeInquiryBody, + context: { clientIp: string; requestId: string }, + deps: TradeInquiryRouteDeps +): Promise { + const businessName = body.businessName?.trim() ?? ""; + const contactName = body.contactName?.trim() ?? ""; + const email = body.email?.trim() ?? ""; + const phoneOrWhatsapp = body.phoneOrWhatsapp?.trim() ?? ""; + const venueType = body.venueType?.trim() ?? ""; + const message = body.message?.trim() ?? ""; + const website = body.website?.trim() ?? ""; + + if (!businessName || !contactName || !email || !venueType) { + return { + status: 400, + body: { + ok: false, + error: + "Missing required fields: businessName, contactName, email, venueType.", + }, + }; + } + + const oversized = tradeLeadFieldTooLong({ + businessName, + contactName, + email, + phoneOrWhatsapp, + venueType, + message, + }); + if (oversized) { + return { + status: 400, + body: { ok: false, error: `Field exceeds maximum length: ${oversized}.` }, + }; + } + + if (!isValidEmail(email)) { + return { + status: 400, + body: { ok: false, error: "Please enter a valid email address." }, + }; + } + + // Honeypot: pretend success for bots, but persist nothing and send no email. + if (website) { + return { status: 200, body: { ok: true } }; + } + + if (deps.isRateLimited(context.clientIp)) { + return { + status: 429, + body: { ok: false, error: "Too many requests. Please try again later." }, + }; + } + + // Firestore is the system of record: the inquiry must be persisted before + // we claim success. The Resend notification is best-effort inside submit — + // its failure is logged, not surfaced to the customer. + const outcome = await deps.submit( + { businessName, contactName, email, phoneOrWhatsapp, venueType, message }, + context.requestId + ); + if (!outcome.ok) { + // Persistence failure was already logged as + // trade_inquiry.persistence_failed — respond generically. + return { + status: 500, + body: { ok: false, error: "Failed to submit inquiry." }, + }; + } + + return { status: 200, body: { ok: true } }; +} + // Persists the inquiry, then attempts the notification email. Persistence is // the durability boundary: a notification failure is logged but does not fail // the submission, while a persistence failure yields `{ ok: false }` so the diff --git a/tests/lib/email.test.ts b/tests/lib/email.test.ts new file mode 100644 index 0000000..175c471 --- /dev/null +++ b/tests/lib/email.test.ts @@ -0,0 +1,44 @@ +import { describe, it } from "node:test"; +import assert from "node:assert"; +import { isValidEmail } from "@/lib/email"; + +// The canonical policy is deliberately pragmatic — not RFC 5322. It accepts +// ordinary addresses (plus tags, subdomains, dotted local parts) and rejects +// input that could never reach a mailbox. +describe("isValidEmail", () => { + it("accepts ordinary addresses", () => { + for (const email of [ + "customer@example.com", + "first.last@example.com", + "customer+trade@example.com", + "sales@subdomain.example.com", + "sales@wholesale.example.com", + ]) { + assert.ok(isValidEmail(email), `expected valid: ${email}`); + } + }); + + it("rejects malformed addresses", () => { + for (const email of [ + "not-an-email", + "customer@", + "@example.com", + "customer@example", + "customer @example.com", + "customer@exam ple.com", + "customer@@example.com", + "customer@example@com", + "", + " ", + ]) { + assert.ok(!isValidEmail(email), `expected invalid: ${email}`); + } + }); + + // Callers normalize (trim) before validating — the check itself treats + // surrounding whitespace as part of the candidate and rejects it. + it("rejects surrounding whitespace rather than repairing it", () => { + assert.ok(!isValidEmail(" customer@example.com")); + assert.ok(!isValidEmail("customer@example.com ")); + }); +}); diff --git a/tests/lib/trade-leads.test.ts b/tests/lib/trade-leads.test.ts index 1af4f81..fbf0ade 100644 --- a/tests/lib/trade-leads.test.ts +++ b/tests/lib/trade-leads.test.ts @@ -1,12 +1,17 @@ import { describe, it } from "node:test"; import assert from "node:assert"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; import { buildTradeLeadRecord, + handleTradeInquiry, processTradeInquiry, tradeLeadFieldTooLong, TRADE_LEAD_FIELD_LIMITS, TRADE_LEADS_COLLECTION, type TradeInquiryDeps, + type TradeInquiryRouteDeps, + type TradeInquiryBody, type TradeLeadInput, } from "@/lib/trade-leads-common"; import type { LogContext } from "@/lib/log"; @@ -175,3 +180,204 @@ describe("processTradeInquiry", () => { } }); }); + +// The POST /api/trade-inquiry pipeline (Issue #117). `submit` is the single +// entry point to persistence + notification (submitTradeInquiry → +// processTradeInquiry → persist + notify), so "submit never called" proves a +// rejected request neither writes a tradeLeads record nor attempts a Resend +// send. +describe("handleTradeInquiry", () => { + const body: TradeInquiryBody = { + businessName: "Test Venue", + contactName: "Test Contact", + email: "venue@example.com", + phoneOrWhatsapp: "+599 000 0000", + venueType: "bar", + message: "We would like to carry your beer.", + }; + + function routeDeps(overrides: { + isRateLimited?: TradeInquiryRouteDeps["isRateLimited"]; + submit?: TradeInquiryRouteDeps["submit"]; + } = {}) { + const calls: string[] = []; + const submitted: TradeLeadInput[] = []; + const deps: TradeInquiryRouteDeps = { + isRateLimited: + overrides.isRateLimited ?? + (() => { + calls.push("isRateLimited"); + return false; + }), + submit: + overrides.submit ?? + (async (submittedInput) => { + calls.push("submit"); + submitted.push(submittedInput); + return { ok: true, leadId: "lead-123" }; + }), + }; + return { deps, calls, submitted }; + } + + const context = { clientIp: "203.0.113.10", requestId: "req-1" }; + + it("rejects malformed email addresses before persistence or notification", async () => { + for (const email of [ + "not-an-email", + "customer@", + "@example.com", + "customer@example", + "custom er@example.com", + "customer@@example.com", + "customer@example@com", + ]) { + const { deps, calls } = routeDeps(); + const result = await handleTradeInquiry({ ...body, email }, context, deps); + assert.strictEqual(result.status, 400, `expected 400 for ${email}`); + assert.deepStrictEqual(result.body, { + ok: false, + error: "Please enter a valid email address.", + }); + assert.deepStrictEqual(calls, [], `submit/rate-limit ran for ${email}`); + } + }); + + it("trims surrounding whitespace before validating the email", async () => { + const { deps, submitted } = routeDeps(); + const result = await handleTradeInquiry( + { ...body, email: " venue@example.com " }, + context, + deps + ); + assert.strictEqual(result.status, 200); + assert.strictEqual(submitted[0]?.email, "venue@example.com"); + }); + + it("rejects a whitespace-only email as missing", async () => { + const { deps, calls } = routeDeps(); + const result = await handleTradeInquiry( + { ...body, email: " " }, + context, + deps + ); + assert.strictEqual(result.status, 400); + assert.match(result.body.error ?? "", /Missing required fields/); + assert.deepStrictEqual(calls, []); + }); + + it("still rejects a missing email as a required-field error", async () => { + const { deps, calls } = routeDeps(); + const result = await handleTradeInquiry( + { ...body, email: undefined }, + context, + deps + ); + assert.strictEqual(result.status, 400); + assert.match(result.body.error ?? "", /Missing required fields/); + assert.deepStrictEqual(calls, []); + }); + + it("still rejects an oversized email as a length error", async () => { + const { deps, calls } = routeDeps(); + const result = await handleTradeInquiry( + { ...body, email: `${"a".repeat(TRADE_LEAD_FIELD_LIMITS.email)}@example.com` }, + context, + deps + ); + assert.strictEqual(result.status, 400); + assert.match(result.body.error ?? "", /Field exceeds maximum length: email/); + assert.deepStrictEqual(calls, []); + }); + + it("accepts ordinary, plus-tag, and subdomain addresses", async () => { + for (const email of [ + "customer@example.com", + "first.last@example.com", + "customer+trade@example.com", + "sales@wholesale.example.com", + ]) { + const { deps, submitted } = routeDeps(); + const result = await handleTradeInquiry({ ...body, email }, context, deps); + assert.strictEqual(result.status, 200, `expected 200 for ${email}`); + assert.deepStrictEqual(result.body, { ok: true }); + assert.strictEqual(submitted[0]?.email, email); + } + }); + + it("keeps the honeypot short-circuit ahead of the rate limiter and submit", async () => { + const { deps, calls } = routeDeps(); + const result = await handleTradeInquiry( + { ...body, website: "https://spam.example" }, + context, + deps + ); + assert.deepStrictEqual(result, { status: 200, body: { ok: true } }); + assert.deepStrictEqual(calls, []); + }); + + it("returns 429 and does not submit when rate limited", async () => { + const { deps, calls } = routeDeps({ + isRateLimited: () => { + calls.push("isRateLimited"); + return true; + }, + }); + const result = await handleTradeInquiry(body, context, deps); + assert.strictEqual(result.status, 429); + assert.match(result.body.error ?? "", /Too many requests/); + assert.deepStrictEqual(calls, ["isRateLimited"]); + }); + + it("maps a failed submission to a generic 500", async () => { + const { deps } = routeDeps({ + submit: async () => ({ ok: false }), + }); + const result = await handleTradeInquiry(body, context, deps); + assert.deepStrictEqual(result, { + status: 500, + body: { ok: false, error: "Failed to submit inquiry." }, + }); + }); + + it("submits exactly the trimmed lead fields with the request id", async () => { + const { deps, submitted } = routeDeps(); + await handleTradeInquiry( + { ...body, businessName: " Test Venue ", website: "" }, + { clientIp: "198.51.100.7", requestId: "req-9" }, + deps + ); + assert.deepStrictEqual(submitted, [ + { + businessName: "Test Venue", + contactName: "Test Contact", + email: "venue@example.com", + phoneOrWhatsapp: "+599 000 0000", + venueType: "bar", + message: "We would like to carry your beer.", + }, + ]); + }); +}); + +// The route module cannot be imported here (it pulls in server-only +// modules), so the wiring between POST and the tested pipeline is asserted +// on the source — the same pattern as tests/lib/admin-session-refresh.test.ts. +describe("trade-inquiry route wiring", () => { + const route = readFileSync( + join(process.cwd(), "app/api/trade-inquiry/route.ts"), + "utf8" + ); + + it("delegates to handleTradeInquiry with the real submitter and rate limiter", () => { + assert.ok(route.includes("handleTradeInquiry(")); + assert.ok(route.includes("submit: submitTradeInquiry")); + assert.ok(route.includes("isRateLimited")); + }); + + it("returns the pipeline result verbatim as the JSON response", () => { + assert.ok( + route.includes("NextResponse.json(result.body, { status: result.status })") + ); + }); +});