Reject malformed emails on trade inquiries (#117)

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

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<string, number[]>();
 
-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(

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<AdminUserView[]>([]);
   const [invitations, setInvitations] = useState<AdminInvitationView[]>([]);
@@ -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;
     }

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

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

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<TradeInquiryOutcome>;
+}
+
+// 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<TradeInquiryRouteResult> {
+  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

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