QA cannot send email at all, so the four mail flows have never been regression tested #87

New Issue

2026-08-21T09:08:07-05:00

bermudalamb commented

2026-08-21 09:08:07 -05:00

QA is deliberately configured never to send mail, and that now blocks regression testing of every flow that sends any.

Why nothing arrives today

docker-compose.qa.yml sets no SMTP variables, with the reason stated inline:

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.

And mailer.ts does exactly that:

if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) {
  console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`);
  return;
}

Working as designed. The design is now the problem.

What is untestable as a result

Five call sites, four customer-visible flows:

Flow	Source
Email verification at signup	`routes/customers.ts:77`
Password reset	`routes/customers.ts:128`
Favorite-sold alerts	`favoriteAlerts.ts:47`
Daily cart reminders (`node-cron`, 9am container time)	`server.ts:43`

None of these has ever been exercised end to end anywhere but production. The cart reminder is the worst of them — it fires from a cron inside the app process, and the project context already flags that if the container restarts frequently the reminders could stop silently with no alerting on that failure mode.

Worth noting how this surfaced: a password-reset test in QA produced no email and nothing in the UI said why, because the endpoint returns {status: 'sent'} whether or not mail was attempted — deliberately, so it cannot be used to enumerate accounts. The time went on establishing that the feature was fine and the environment was muted.

Decision: real SMTP, with a recipient allowlist

Chosen over two alternatives. A mail catcher such as Mailpit was rejected because it cannot catch a rendering or deliverability problem in a real client, which is part of what regression testing these mails is for. Redirecting every recipient to one address was rejected because it discards the question of whether the right person gets the mail.

MAIL_ALLOWLIST, a new environment variable read in mailer.ts:

Unset — no restriction. This is production's behaviour and must stay that way.
Set — only matching recipients are sent to. Anything else is skipped with a loud, greppable log line naming the blocked recipient and subject, and the caller proceeds as though mail succeeded. That is the same degradation shape the unconfigured-SMTP path already has, so no call site changes and no flow under test breaks.

Matching is case-insensitive, and strips a +suffix from the local part before comparing, because plus-addressing is how these tests get written:

Entry	Matches
`someone@gmail.com`	that address, and `someone+anything@gmail.com`
`@example.com`	any address at that domain

The guard lives in mailer.ts rather than at the call sites, so all five senders are covered by construction and a sixth added later cannot bypass it.

This inverts the failure mode, which is worth stating plainly

Today QA physically cannot email a real customer. After this it can, and the safety property depends on the allowlist being present and correct. Three things hold it up:

MAIL_ALLOWLIST is hardcoded in docker-compose.qa.yml, not read from ${VAR}. A safety property should not depend on remembering to set a stack variable.
The matching function is pure and unit tested, including the cases that would be dangerous if wrong.
The comment next to it says that removing the variable means unrestricted sending, so nobody deletes it while tidying.

Production is a separate Portainer stack that does not read this compose file, so it stays unrestricted by default and needs no change.

SMTP settings: Brevo

The existing Brevo account will be reused rather than a separate QA sending identity. Recorded as a deliberate choice: it means QA regression volume shares production's sending reputation and plan quota. Acceptable for now; worth revisiting if QA traffic grows.

Note the mailer's defaults were written for Gmail — SMTP_HOST falls back to smtp.gmail.com, SMTP_PORT to 465, and secure is true unless SMTP_SECURE=false. Brevo's relay is a different host and typically port 587 with STARTTLS, so QA has to set host, port and SMTP_SECURE explicitly rather than inheriting those fallbacks. Getting that wrong fails at send time, not at boot — which is #64's territory.

Verification

Unit tests on the matching function, which is where a mistake would actually be dangerous: an allowed address, a plus-variant of one, a domain entry, a non-matching address, a lookalike domain that must not match, and an unset allowlist meaning unrestricted. Then a real send from QA to a plus-addressed allowlisted inbox, and a confirmed refusal for an address not on the list.

Severity

Medium. Nothing is broken, but four customer-facing mail flows have no test coverage in any environment short of production, and one of them is a cron job with a known silent failure mode.

QA is deliberately configured never to send mail, and that now blocks regression testing of every flow that sends any. ## Why nothing arrives today `docker-compose.qa.yml` sets no SMTP variables, with the reason stated inline: > 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. And `mailer.ts` does exactly that: ```ts if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) { console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`); return; } ``` Working as designed. The design is now the problem. ## What is untestable as a result Five call sites, four customer-visible flows: | Flow | Source | | --- | --- | | Email verification at signup | `routes/customers.ts:77` | | Password reset | `routes/customers.ts:128` | | Favorite-sold alerts | `favoriteAlerts.ts:47` | | Daily cart reminders (`node-cron`, 9am container time) | `server.ts:43` | None of these has ever been exercised end to end anywhere but production. The cart reminder is the worst of them — it fires from a cron inside the app process, and the project context already flags that if the container restarts frequently the reminders could stop silently with no alerting on that failure mode. Worth noting how this surfaced: a password-reset test in QA produced no email and nothing in the UI said why, because the endpoint returns `{status: 'sent'}` whether or not mail was attempted — deliberately, so it cannot be used to enumerate accounts. The time went on establishing that the feature was fine and the environment was muted. ## Decision: real SMTP, with a recipient allowlist Chosen over two alternatives. A mail catcher such as Mailpit was rejected because it cannot catch a rendering or deliverability problem in a real client, which is part of what regression testing these mails is for. Redirecting every recipient to one address was rejected because it discards the question of whether the *right* person gets the mail. **`MAIL_ALLOWLIST`**, a new environment variable read in `mailer.ts`: - **Unset** — no restriction. This is production's behaviour and must stay that way. - **Set** — only matching recipients are sent to. Anything else is skipped with a loud, greppable log line naming the blocked recipient and subject, and the caller proceeds as though mail succeeded. That is the same degradation shape the unconfigured-SMTP path already has, so no call site changes and no flow under test breaks. Matching is case-insensitive, and strips a `+suffix` from the local part before comparing, because plus-addressing is how these tests get written: | Entry | Matches | | --- | --- | | `someone@gmail.com` | that address, and `someone+anything@gmail.com` | | `@example.com` | any address at that domain | The guard lives in `mailer.ts` rather than at the call sites, so all five senders are covered by construction and a sixth added later cannot bypass it. ## This inverts the failure mode, which is worth stating plainly Today QA physically cannot email a real customer. After this it can, and the safety property depends on the allowlist being present and correct. Three things hold it up: 1. `MAIL_ALLOWLIST` is **hardcoded in `docker-compose.qa.yml`**, not read from `${VAR}`. A safety property should not depend on remembering to set a stack variable. 2. The matching function is pure and unit tested, including the cases that would be dangerous if wrong. 3. The comment next to it says that removing the variable means unrestricted sending, so nobody deletes it while tidying. Production is a separate Portainer stack that does not read this compose file, so it stays unrestricted by default and needs no change. ## SMTP settings: Brevo The existing Brevo account will be reused rather than a separate QA sending identity. Recorded as a deliberate choice: it means QA regression volume shares production's sending reputation and plan quota. Acceptable for now; worth revisiting if QA traffic grows. Note the mailer's defaults were written for Gmail — `SMTP_HOST` falls back to `smtp.gmail.com`, `SMTP_PORT` to `465`, and `secure` is true unless `SMTP_SECURE=false`. Brevo's relay is a different host and typically port 587 with STARTTLS, so QA has to set host, port and `SMTP_SECURE` explicitly rather than inheriting those fallbacks. Getting that wrong fails at send time, not at boot — which is #64's territory. ## Verification Unit tests on the matching function, which is where a mistake would actually be dangerous: an allowed address, a plus-variant of one, a domain entry, a non-matching address, a lookalike domain that must not match, and an unset allowlist meaning unrestricted. Then a real send from QA to a plus-addressed allowlisted inbox, and a confirmed refusal for an address not on the list. ## Severity Medium. Nothing is broken, but four customer-facing mail flows have no test coverage in any environment short of production, and one of them is a cron job with a known silent failure mode.

bermudalamb commented

2026-08-21 09:15:59 -05:00

Implemented on `feature/87-qa-mail-allowlist` — one commit, not pushed

MAIL_ALLOWLIST is read in mailer.ts, guarding all four flows from one place.

Three things you need to set in the QA Portainer stack

The branch is inert until these exist — with them missing the mailer takes the older path and skips sending as "SMTP not configured", so the symptom would look unchanged.

Variable	Value
`QA_SMTP_USER`	Brevo SMTP login
`QA_SMTP_PASSWORD`	Brevo SMTP key
`QA_SMTP_FROM`	a sender address verified in Brevo — an unverified one is accepted at connect time and rejected at send

They are named QA_-prefixed for the same reason QA_DB_PASSWORD is: pasting production's stack variables into QA must not silently work.

Host, port and secure are pinned in the compose file rather than left to you: smtp-relay.brevo.com, 587, SMTP_SECURE=false. The mailer's fallbacks are Gmail's (smtp.gmail.com, 465, TLS), and Brevo needs STARTTLS on 587 — inheriting the fallbacks would connect to the wrong provider entirely.

MAIL_ALLOWLIST is hardcoded to thomlamb@gmail.com in docker-compose.qa.yml, not read from a stack variable, so the safety property cannot be lost by omission. If that is the wrong address, change it in the file — a wrong one fails closed, so the symptom is blocked mail rather than mail to a stranger.

What the guard does

Recipient	Result
`thomlamb@gmail.com`	delivered
`thomlamb+favtest1@gmail.com`	delivered — an entry covers all its plus-variants, so no editing to invent a suffix
`someone.else@gmail.com`	blocked, `[mail-blocked]` logged with the address and subject
`thomlamb@gmail.com.evil.example`	blocked

Two decisions worth knowing because they are the difference between a guard and the appearance of one:

Comparison is exact equality on both halves, never a suffix test. A suffix test would let any attacker-controlled domain ending in an allowed one through, and there is a test asserting the lookalike case specifically.

A present-but-empty value refuses everyone, rather than falling back to unrestricted. 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.

The guard lives in the mailer rather than at the four call sites, so a fifth sender added later cannot bypass it by forgetting. It skips rather than throws, matching the existing unconfigured-SMTP behaviour — three of the callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 during signup. The flow under test finishes and the log says why nothing arrived, which is the part that was missing when QA was simply muted.

Verification

12 new unit tests on the matching function — the allowed address, its plus-variants, a domain entry, a different mailbox at an allowed domain, an unlisted address, a lookalike domain, a lookalike local part, case handling on both sides, padding and empty entries, an unusable recipient, an absent allowlist meaning unrestricted, and an empty one meaning refuse.

98 unit (86 + 12), 144 integration, backend lint 0 errors and 8 warnings unchanged, build clean, and docker compose config renders the expected values.

Deliberately not unit-tested: the send itself. Asserting the guard through a real transport would need either a live SMTP connection or a mock of nodemailer that proves nothing about the rule. The rule is pure and is where a mistake would be dangerous.

Still to confirm, and it needs the stack variables first

The end-to-end half of this issue's verification is untouched: a real send from QA to a plus-addressed allowlisted inbox, and a confirmed [mail-blocked] line for an address not on the list. I cannot do either until the Brevo variables exist in the stack. Once they do, requesting a password reset for thomlamb+favtest1@gmail.com should deliver, and the same for any other address should log [mail-blocked].

Note on scope

No separate design document. The design is this issue, and duplicating it into docs/superpowers/specs/ would create two records of the same decisions that can drift — the change is one guard function, its tests, and compose configuration.

Not pushed, per the usual arrangement.

## Implemented on `feature/87-qa-mail-allowlist` — one commit, not pushed `MAIL_ALLOWLIST` is read in `mailer.ts`, guarding all four flows from one place. ## Three things you need to set in the QA Portainer stack The branch is inert until these exist — with them missing the mailer takes the older path and skips sending as "SMTP not configured", so the symptom would look unchanged. | Variable | Value | | --- | --- | | `QA_SMTP_USER` | Brevo SMTP login | | `QA_SMTP_PASSWORD` | Brevo SMTP key | | `QA_SMTP_FROM` | a sender address **verified in Brevo** — an unverified one is accepted at connect time and rejected at send | They are named `QA_`-prefixed for the same reason `QA_DB_PASSWORD` is: pasting production's stack variables into QA must not silently work. Host, port and secure are pinned in the compose file rather than left to you: `smtp-relay.brevo.com`, `587`, `SMTP_SECURE=false`. The mailer's fallbacks are Gmail's (`smtp.gmail.com`, `465`, TLS), and Brevo needs STARTTLS on 587 — inheriting the fallbacks would connect to the wrong provider entirely. `MAIL_ALLOWLIST` is **hardcoded to `thomlamb@gmail.com`** in `docker-compose.qa.yml`, not read from a stack variable, so the safety property cannot be lost by omission. If that is the wrong address, change it in the file — a wrong one fails closed, so the symptom is blocked mail rather than mail to a stranger. ## What the guard does | Recipient | Result | | --- | --- | | `thomlamb@gmail.com` | delivered | | `thomlamb+favtest1@gmail.com` | delivered — an entry covers all its plus-variants, so no editing to invent a suffix | | `someone.else@gmail.com` | blocked, `[mail-blocked]` logged with the address and subject | | `thomlamb@gmail.com.evil.example` | blocked | Two decisions worth knowing because they are the difference between a guard and the appearance of one: **Comparison is exact equality on both halves**, never a suffix test. A suffix test would let any attacker-controlled domain ending in an allowed one through, and there is a test asserting the lookalike case specifically. **A present-but-empty value refuses everyone**, rather than falling back to unrestricted. 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. The guard lives in the mailer rather than at the four call sites, so a fifth sender added later cannot bypass it by forgetting. It skips rather than throws, matching the existing unconfigured-SMTP behaviour — three of the callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 during signup. The flow under test finishes and the log says why nothing arrived, which is the part that was missing when QA was simply muted. ## Verification 12 new unit tests on the matching function — the allowed address, its plus-variants, a domain entry, a different mailbox at an allowed domain, an unlisted address, a lookalike domain, a lookalike local part, case handling on both sides, padding and empty entries, an unusable recipient, an absent allowlist meaning unrestricted, and an empty one meaning refuse. **98 unit** (86 + 12), **144 integration**, backend lint 0 errors and 8 warnings unchanged, build clean, and `docker compose config` renders the expected values. Deliberately not unit-tested: the send itself. Asserting the guard through a real transport would need either a live SMTP connection or a mock of nodemailer that proves nothing about the rule. The rule is pure and is where a mistake would be dangerous. ## Still to confirm, and it needs the stack variables first The end-to-end half of this issue's verification is untouched: a real send from QA to a plus-addressed allowlisted inbox, and a confirmed `[mail-blocked]` line for an address not on the list. I cannot do either until the Brevo variables exist in the stack. Once they do, requesting a password reset for `thomlamb+favtest1@gmail.com` should deliver, and the same for any other address should log `[mail-blocked]`. ## Note on scope No separate design document. The design is this issue, and duplicating it into `docs/superpowers/specs/` would create two records of the same decisions that can drift — the change is one guard function, its tests, and compose configuration. Not pushed, per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-21 09:38:29 -05:00

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

bermudalamb referenced this issue

2026-08-21 09:38:51 -05:00

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

bermudalamb added reference feature/87-qa-mail-allowlist

2026-08-21 09:39:51 -05:00

bermudalamb added this to the Code Quality and Hardening project 2026-08-21 09:40:00 -05:00

bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-21 09:40:13 -05:00

bermudalamb self-assigned this 2026-08-21 09:40:17 -05:00

bermudalamb referenced this issue from a commit