diff --git a/backend/src/mailer.ts b/backend/src/mailer.ts index 12d10d7..31a40a9 100755 --- a/backend/src/mailer.ts +++ b/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 { 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, diff --git a/backend/tests/unit/mailAllowlist.test.ts b/backend/tests/unit/mailAllowlist.test.ts new file mode 100644 index 0000000..192e1bc --- /dev/null +++ b/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); + }); + }); +}); diff --git a/docker-compose.qa.yml b/docker-compose.qa.yml index 26376e0..65a1d78 100644 --- a/docker-compose.qa.yml +++ b/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}