feat: let QA send real email, guarded by a recipient allowlist (#87)

QA has never been able to send mail. The compose file set no SMTP variables and the mailer skips sending when it finds none, which was deliberate — a QA run must not be able to email a real customer if a fixture ever holds a real address. The cost is that four customer-facing flows have never been exercised anywhere but production: verification, password reset, favorite-sold alerts, and the cart-reminder cron that already has a known silent failure mode.

MAIL_ALLOWLIST replaces the blanket mute. Unset means unrestricted, which is production and must stay so. Set means only matching recipients are delivered to; anything else is skipped with a [mail-blocked] warning naming the address and subject. An entry is either a full address, which also covers its plus-suffixed variants, or @domain for every mailbox there — plus-addressing is how these tests get written, and nobody should have to edit an allowlist to invent a new suffix mid-run.

The guard sits in the mailer, not at the four call sites, so every sender is covered by construction and a fifth added later cannot bypass it by forgetting. It skips rather than throws: three callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 on the signup path. The flow under test finishes and the log says why no mail arrived, which is exactly what was missing when QA was simply muted.

Two details are load-bearing enough to state. Comparison is exact equality on both halves of the address rather than a suffix test, so a lookalike domain ending in an allowed one cannot get through — there is a test for that specifically. And a present-but-empty value refuses everyone rather than allowing everyone: writing MAIL_ALLOWLIST= expresses an intent to restrict, and reading it as "no restriction" would turn a typo into an outbound mail incident.

This inverts the failure mode, so the allowlist is hardcoded in docker-compose.qa.yml rather than read from a stack variable. The safety property must not depend on remembering to set something in Portainer, where an omission would mean unrestricted sending from an environment full of fixtures. The comment says removing the line disables the restriction rather than the mail.

QA points at Brevo, reusing the existing account rather than a separate QA sender — a deliberate choice that puts QA volume behind production's sending reputation and quota, acceptable for now. Host, port and secure are pinned in the compose because the mailer's fallbacks are Gmail's and Brevo needs 587 with STARTTLS; that mismatch fails at send time rather than at boot, which is #64's territory.

Verified: 12 new unit tests on the matching function, which is where a mistake would actually be dangerous — 98 unit and 144 integration passing, lint 0 errors and 8 warnings unchanged, and the compose renders the expected values under docker compose config.

Refs #87
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/mailer.ts

@@ -1,5 +1,9 @@
 import nodemailer from 'nodemailer';
 
+// Defaults written for Gmail. An environment on a different provider — QA is on
+// Brevo — has to set host, port and SMTP_SECURE explicitly rather than
+// inheriting these, and getting that wrong fails at send time rather than at
+// boot. See #64 on validating this at startup instead.
 const transporter = nodemailer.createTransport({
   host: process.env.SMTP_HOST || 'smtp.gmail.com',
   port: parseInt(process.env.SMTP_PORT || '465', 10),
@@ -10,11 +14,94 @@ const transporter = nodemailer.createTransport({
   }
 });
 
+interface ParsedAddress {
+  local: string;
+  domain: string;
+}
+
+// Lowercased, and with any `+suffix` removed from the local part. Returns null
+// for anything that is not a usable address, so a caller can refuse rather than
+// compare nonsense.
+function parseAddress(address: string): ParsedAddress | null {
+  const trimmed = address.trim().toLowerCase();
+  const at = trimmed.lastIndexOf('@');
+  // Needs something on both sides of a single trailing @.
+  if (at <= 0 || at === trimmed.length - 1) {
+    return null;
+  }
+  const localWithSuffix = trimmed.slice(0, at);
+  return {
+    local: localWithSuffix.split('+')[0],
+    domain: trimmed.slice(at + 1)
+  };
+}
+
+/**
+ * Whether this environment is permitted to email this recipient.
+ *
+ * `allowlist` is the raw MAIL_ALLOWLIST value: a comma-separated list where an
+ * entry is either a full address, which also covers its `+suffix` variants, or
+ * `@domain`, which covers every mailbox there.
+ *
+ * Undefined means unrestricted, which is production — it must be able to mail
+ * real customers. Present but empty means refuse everyone: someone writing
+ * `MAIL_ALLOWLIST=` is expressing an intent to restrict, and reading that as
+ * "no restriction" would turn a typo into an outbound mail incident.
+ *
+ * Comparison is by exact equality on both halves, never a suffix test, so a
+ * lookalike domain ending in an allowed one does not get through.
+ *
+ * Exported for its unit test. This function is the entire safety property of
+ * mail in a non-production environment, and it is pure, so it is worth testing
+ * directly rather than through a send.
+ */
+export function isAllowedRecipient(to: string, allowlist: string | undefined): boolean {
+  if (allowlist === undefined) {
+    return true;
+  }
+
+  const entries = allowlist
+    .split(',')
+    .map((entry) => entry.trim().toLowerCase())
+    .filter((entry) => entry !== '');
+
+  if (entries.length === 0) {
+    return false;
+  }
+
+  const recipient = parseAddress(to);
+  if (!recipient) {
+    return false;
+  }
+
+  return entries.some((entry) => {
+    if (entry.startsWith('@')) {
+      return recipient.domain === entry.slice(1);
+    }
+    const allowed = parseAddress(entry);
+    return allowed !== null && allowed.local === recipient.local && allowed.domain === recipient.domain;
+  });
+}
+
 export async function sendMail(to: string, subject: string, html: string): Promise<void> {
   if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) {
     console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`);
     return;
   }
+
+  // Guarded here rather than at the four call sites, so every sender is covered
+  // by construction and a fifth added later cannot bypass it by forgetting.
+  //
+  // Skipping rather than throwing, and returning as though it sent: three of
+  // the callers already swallow send failures into a log, so throwing would
+  // mostly be caught and logged anyway while risking a 500 on the signup path.
+  // The flow under test finishes, and the log says why no mail arrived — which
+  // is the part that was missing when QA was simply muted.
+  if (!isAllowedRecipient(to, process.env.MAIL_ALLOWLIST)) {
+    console.warn(`[mail-blocked] ${to} is not on MAIL_ALLOWLIST — skipping "${subject}"`);
+    return;
+  }
+
   await transporter.sendMail({
     from: process.env.SMTP_FROM || process.env.SMTP_USER,
     to,

backend/tests/unit/mailAllowlist.test.ts

@@ -0,0 +1,80 @@
+import { isAllowedRecipient } from '../../src/mailer';
+
+// This function is the whole safety property of QA mail. If it says yes when it
+// should say no, a QA run emails a real customer; if it says no when it should
+// say yes, the regression test it was written for silently proves nothing. Both
+// directions are tested, and the dangerous direction is tested hardest.
+describe('isAllowedRecipient', () => {
+  describe('with no allowlist configured', () => {
+    // Production. It must send to whoever it is told to, or every customer
+    // email in the product stops working.
+    it('allows anyone when the variable is absent', () => {
+      expect(isAllowedRecipient('anyone@example.com', undefined)).toBe(true);
+    });
+  });
+
+  describe('with an allowlist configured', () => {
+    const ALLOW = 'someone@gmail.com, @internal.example';
+
+    it('allows an address listed exactly', () => {
+      expect(isAllowedRecipient('someone@gmail.com', ALLOW)).toBe(true);
+    });
+
+    // The reason this feature exists: a tester invents a new plus-suffix per
+    // run and must not have to edit the allowlist each time.
+    it('allows any plus-suffixed variant of a listed address', () => {
+      expect(isAllowedRecipient('someone+favtest1@gmail.com', ALLOW)).toBe(true);
+      expect(isAllowedRecipient('someone+anything-at-all@gmail.com', ALLOW)).toBe(true);
+    });
+
+    it('allows any address at a listed domain', () => {
+      expect(isAllowedRecipient('whoever@internal.example', ALLOW)).toBe(true);
+    });
+
+    it('refuses a different mailbox at a listed address domain', () => {
+      expect(isAllowedRecipient('someone.else@gmail.com', ALLOW)).toBe(false);
+    });
+
+    it('refuses an address that is not listed at all', () => {
+      expect(isAllowedRecipient('realcustomer@example.com', ALLOW)).toBe(false);
+    });
+
+    // The dangerous case. A suffix match rather than an equality check would
+    // let an attacker-controlled domain ending in a listed one through.
+    it('refuses a lookalike domain that merely ends with a listed one', () => {
+      expect(isAllowedRecipient('someone@gmail.com.evil.example', ALLOW)).toBe(false);
+      expect(isAllowedRecipient('whoever@not-internal.example', ALLOW)).toBe(false);
+    });
+
+    // And the reverse of the same mistake, on the local part.
+    it('refuses a local part that merely ends with a listed one', () => {
+      expect(isAllowedRecipient('notsomeone@gmail.com', ALLOW)).toBe(false);
+    });
+
+    it('ignores case on both sides', () => {
+      expect(isAllowedRecipient('SomeOne+Test@GMAIL.com', 'someone@gmail.com')).toBe(true);
+      expect(isAllowedRecipient('someone@gmail.com', 'SOMEONE@GMAIL.COM')).toBe(true);
+    });
+
+    it('tolerates padding and empty entries in the list', () => {
+      expect(isAllowedRecipient('someone@gmail.com', '  someone@gmail.com ,, ')).toBe(true);
+    });
+
+    it('refuses a recipient that is not a usable address', () => {
+      expect(isAllowedRecipient('not-an-address', ALLOW)).toBe(false);
+      expect(isAllowedRecipient('', ALLOW)).toBe(false);
+      expect(isAllowedRecipient('@nolocalpart.example', ALLOW)).toBe(false);
+    });
+  });
+
+  // Fails closed rather than open. Someone who writes MAIL_ALLOWLIST= into a
+  // compose file is expressing an intent to restrict, and reading that as
+  // "unrestricted" would turn a typo into an outbound mail incident.
+  describe('with the variable present but empty', () => {
+    it('refuses everyone', () => {
+      expect(isAllowedRecipient('someone@gmail.com', '')).toBe(false);
+      expect(isAllowedRecipient('someone@gmail.com', '   ')).toBe(false);
+      expect(isAllowedRecipient('someone@gmail.com', ' , , ')).toBe(false);
+    });
+  });
+});

docker-compose.qa.yml

@@ -34,6 +34,9 @@
 #                     production stack's variables here does nothing silently.
 #   PUBLIC_URL      — the QA hostname, e.g.
 #                     https://qa-redefined-designs.bermudalamb.synology.me
+#   QA_SMTP_USER    — Brevo SMTP login. Named QA_ for the same reason as the
+#   QA_SMTP_PASSWORD  database password: pasting production's variables in here
+#   QA_SMTP_FROM      must not silently work.
 
 services:
   redefined-designs-qa:
@@ -63,10 +66,35 @@ services:
       # PAYPAL_ENV=sandbox — never the live ones.
       - DEMO_MODE=true
 
-      # No SMTP configuration either. The mailer degrades gracefully when
-      # unconfigured: it logs a warning and skips sending. That is the desired
-      # behaviour here — a QA run must not be able to email real customers if
-      # a fixture ever contains a real address.
+      # SMTP *is* configured here, unlike PayPal above, because the four mail
+      # flows — verification, password reset, favorite-sold alerts and the
+      # cart-reminder cron — cannot be regression tested without it. See #87.
+      #
+      # Host, port and secure are not secrets and are pinned here rather than
+      # inherited: the mailer's fallbacks are Gmail's (smtp.gmail.com, 465,
+      # TLS) and Brevo needs 587 with STARTTLS, which is why SMTP_SECURE is
+      # false. Getting these wrong fails at send time, not at boot.
+      - SMTP_HOST=smtp-relay.brevo.com
+      - SMTP_PORT=587
+      - SMTP_SECURE=false
+      - SMTP_USER=${QA_SMTP_USER}
+      - SMTP_PASSWORD=${QA_SMTP_PASSWORD}
+      - SMTP_FROM=${QA_SMTP_FROM}
+
+      # What keeps a QA run from emailing a real customer now that it *can*
+      # send. Only these recipients are ever delivered to; anything else is
+      # skipped with a [mail-blocked] warning naming the address.
+      #
+      # Hardcoded rather than read from a stack variable, deliberately. This is
+      # the entire safety property, and it must not depend on somebody
+      # remembering to set something in Portainer — an unset variable would
+      # mean unrestricted sending from an environment full of test fixtures.
+      #
+      # An entry covers its plus-suffixed variants, so `+whatever` addresses
+      # work without editing this. Removing the line does NOT disable mail; it
+      # disables the restriction. Production is a separate stack that does not
+      # read this file, which is why it is unrestricted and correct to be.
+      - MAIL_ALLOWLIST=thomlamb@gmail.com
       - SITE_CURRENCY=USD
       - RESERVATION_MINUTES=15
       - PUBLIC_URL=${PUBLIC_URL}