Merge pull request 'feat: let QA send real email, guarded by a recipient allowlist (#87)' (#88) from feature/87-qa-mail-allowlist into main

Reviewed-on: #88

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
+      # SMTP *is* configured here, unlike PayPal above, because the four mail
-      # unconfigured: it logs a warning and skips sending. That is the desired
+      # flows — verification, password reset, favorite-sold alerts and the
-      # behaviour here — a QA run must not be able to email real customers if
+      # cart-reminder cron — cannot be regression tested without it. See #87.
-      # a fixture ever contains a real address.
+      #
+      # 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}