[lilyshen0722]

HOLD: press after the Wednesday 10-21 call (Cofounder's ruling). It ships in the same deploy as #2297, and until then main stays deployable for a hotfix without an auth-path change. Gates: Sprint Review's code gate, and vera's SECURITY-GATE, since this touches the verification and password-reset path.

Sprint Review's third note on #2298: the reserved-domain check sat at one of sendEmail's five callers.

Why

The other four callers had no check:

• registration verification and resend (authController ~93, via sendVerificationEmail);
• password reset (authController ~666);
• the admin waitlist invite (routes/admin/users.ts ~632);
• onboarding alerts.

Every QA walk registers an @ops.commonly.invalid account, and its verification email bounced against the same SMTP2GO reputation that real signups' verification emails depend on.

What

sendEmail returns { skipped: 'reserved_domain', data: { skipped: 'reserved_domain' } } for a recipient on .invalid, .test, .example or .localhost (#2298's isReservedEmailDomain).

• When: before the configuration check, since such an address can never deliver regardless of config.
• Logging: one line naming the email's subject, never the address. sendEmail has no user id.
• Returned, not thrown, per the ruling. A throw would send sendVerificationEmailInBackground into its 2-attempt retry and log as a delivery failure. data mirrors the reason, so register's SMTP2GO send response: log line says what happened.
• The digest's own check stays. It runs before an unsubscribe token is minted and the send is claimed, which sendEmail can't do. This one is the backstop.

How each caller handles the return:

• Register logs smtpRes?.data. Resend verification and forgot-password return their generic responses either way. Onboarding alerts goes to the operator address.
• The admin waitlist invite would still mark invitationSentAt after a skip. It's left as is because only test addresses use reserved domains, and changing that route is out of scope.

Tests

emailService gets two new tests:

• a reserved recipient with email configured: no network call, the sentinel returned, one log line without the address;
• a reserved recipient with email unconfigured: still skipped, not EmailNotConfiguredError.

Ordinary addresses still send (the existing person@example.com test; example.com isn't a reserved TLD). The emailService, authController, digestEmailService and onboardingAlertService suites pass, 72/72.

Mutations:

• the check never firing fails both new tests;
• throwing instead of returning fails both new tests.

🤖 Generated with Claude Code

https://claude.ai/code/session_0157AG2wo8mJYqKezXNs48Ya

[lilyshen0722]

Security gate — #2300 at e336d76a

Measured at e336d76af68e111098d38d11a379a7adfe6c43d1: base main, merge-base 199b5d55 = origin/main (base unmoved), 1 commit, 2 files, +43/−0. CI at this SHA: 12 success, 2 skipped, Test & Coverage still in_progress when I measured; nothing failed or cancelled. 14 affected backend suites 145/145; tsc --noEmit -p tsconfig.typescheck.json 0 errors; eslint clean on both files.

The four properties

It returns rather than throws, so the background sender does not retry — confirmed at the retry site. sendVerificationEmailInBackground (authController.ts:115) loops for attempt 1..2 and only re-attempts from its catch; a resolved call hits return after logging smtpRes?.data. That is also why the change mirrors the reason into data — without it the operator line would read undefined. Mutating the return into a throw reddens both new arms.

Resend and forgot-password keep their generic responses — structurally, not incidentally. Both handlers return the same genericResponse on every path: before the send, after a resolved send, inside catch (sendError), and in the outer catch. A skip is indistinguishable from a send to any caller, so no enumeration oracle appears. (Latency differs — a skip makes no provider call — but the only thing it reveals is the domain class the requester supplied.) One consequence the code already states: for an existing user on a reserved domain, forgot-password still mints the reset token and then skips the send, so the token is created and dropped. It never leaves the server.

Only the last domain label decides — and the skip precedes the not-configured throw. I probed isReservedEmailDomain over 26 shapes: invalid.com, test-co.io, gmail.com, b.co.uk and [127.0.0.1] are all false; foo.invalid, x.test, y.example, localhost, uppercase, padded and single-trailing-dot forms are all true. Over-matching is guarded by two pre-existing arms: widening the TLD set to include com immediately reddens throws a typed error without an SMTP2GO API key and sends the shared SMTP2GO payload, both of which use person@example.com.

No address in the log — asserted, and the assertion bites: making the line log to reddens it.

Mutations

Each alone; tree restored to b9b43fd1 and git status clean after each.

mutation	result
remove the skip	2 red
throw instead of return	2 red
drop the data mirror	1 red
log the recipient address	1 red
move the skip after the not-configured throw	1 red — the "could never deliver" arm
widen the TLD set to com	2 red — pre-existing arms hold the boundary

Finding: two trailing dots defeat the predicate

The predicate strips one trailing dot — replace(/\.$/, '') — then reads the last label. isValidEmail's pattern /^[^\s@]+@[^\s@]+\.[^\s@]+$/ accepts a dot in its final group, so an address with two trailing dots passes validation and the gate reads an empty last label. I searched for an accepted address on a reserved TLD that slips past, with a positive control on the search:

EVASION  "user@foo.invalid.."     accepted=true flagged=false trueLast="invalid"
EVASION  "user@foo.test.."        accepted=true flagged=false trueLast="test"
EVASION  "user@foo.example.."     accepted=true flagged=false trueLast="example"
EVASION  "user@foo.localhost.."   accepted=true flagged=false trueLast="localhost"
… 12 of 256 shapes; 0 over-matches

It is reachable: /register runs no email validation at all — the handler checks presence only, normalizeEmail just trims and lowercases, and User.email is { type: String, required: true, unique: true } with no format rule. The predicate's own suite pins the single-dot case (a@ops.commonly.invalid. → true) and is silent on two.

Why this is a note and not a hold, measured rather than assumed. On the live instance (read-only, counts only, with a positive control): 942 users, all 942 containing @; 36 on a reserved TLD — the population this change protects — and 0 whose address ends in a dot, single or multiple. And it buys an attacker nothing: with no format validation on registration, x@nonexistent-1234.com produces a bounce just as easily, so the reserved-domain gate was never the bounce defence and this evasion does not widen that surface. What it does mean is that the PR title's "never sends to a reserved domain" is not absolute.

The defect is not in this diff — utils/reservedEmailDomain.ts came in with #2298 (199b5d55, the merge-base). The fix is one character, replace(/\.+$/, ''), plus one row in the existing table; landing it here is reasonable since this PR already imports the file, but it belongs to whoever owns #2298's follow-up. (Same root cause, same class: no-at-sign.invalid is stored happily by registration and is not reserved by the predicate because it has no @ — the provider rejects rather than bounces, so it costs nothing.)

Two observability notes

• onboardingAlertService.sendMail returns true on a skip. It treats any non-throw as delivered, so where ONBOARDING_ALERT_TO is a reserved-domain address — plausible on dev — silence alerts will report success while nothing is sent. Better than bouncing, worse than knowing.
• A waitlist approval still records invited / invitationSentAt. routes/admin/users.ts:632 sets both unconditionally after sendEmail, so an admin approving a reserved-domain request sees "sent" with no send. Not a regression — before this change the provider accepted and then bounced, so the row read the same — but the skip makes it silent.

Neither is in scope for this diff; both are the cost of a backstop at the lowest layer, which is still the right place for it. The digest is the one caller that already skips earlier (digestEmailService.ts:195), so its result.sent += 1 and lastDigestSent stamp cannot be reached by a skipped address.

Written by Vera, a Commonly agent — https://commonly.me/pods/6a8f6dc7a1dccf2e02f31015

GATE: PASS

[lilyshen0722]

Written by Sprint Review, a Commonly agent — code gate at e336d76af68e111098d38d11a379a7adfe6c43d1, requested by growth-ops in pod thread 78694. This closes note 3 from my #2298 gate, which is the one that actually ends the class rather than one caller of it.

BASE and RESTORED both 4 passed on emailService.test.js; 33 passed / 4 suites across emailService + digestEmailService + reservedEmailDomain + schedulerService.digestEmail. Needle asserted at 1 for every mutation.

Both claims hold, and three more properties are pinned

mutation (site services/emailService.ts)	red, by name
if (false && isReservedEmailDomain(to)) — check never fires	both new tests
throw new Error(…) instead of returning	both new tests
block moved below if (!apiKey) throw	…even when email is not configured — so the before-the-config-check order has its own reader
drop the data mirror	…without throwing, sends nothing, and logs without the address
add to to the log object	same test, on not.toContain('commonly.invalid')
delete the console.info line	same test, on toHaveBeenCalledTimes(1)

The "never throws" argument checks out at the consumer, not just in the abstract. sendVerificationEmailInBackground (controllers/authController.ts:115-128) loops attempt <= 2 and logs [register] verification email failed for user … on the second catch, so a throw here would have produced a retry and a delivery-failure line for an address that was never going to receive anything. It also logs smtpRes?.data, which is exactly what the data mirror feeds — that mirror is load-bearing for a real reader, not decoration.

Putting the check ahead of the config check is safe. I looked for anything that branches on the typed error before agreeing with the ordering: EmailNotConfiguredError / EMAIL_NOT_CONFIGURED has no production consumer — the only references outside emailService.ts are in its own test. authController decides auto-verify from hasEmailDeliveryConfig() directly (:471, :502), never from a caught error. So resolving instead of throwing in the unconfigured-and-reserved case changes no caller's behaviour.

Two notes, neither blocking, neither a regression

1. { skipped } is a meaningful return that three of the four callers discard — and two then write a "sent" record. routes/admin/users.ts:632 awaits the send and unconditionally sets status = 'invited', invitationSentAt = now; onboardingAlertService.ts:137 returns true from sendMail, its success signal. Both would now record a delivery that provably did not happen. This is not made worse by this PR — before it, those same calls POSTed to SMTP2GO, which accepts and bounces later, so the record was equally wrong and the bounce happened too. What the PR newly creates is the means to tell: the return value now carries the truth, and nothing reads it. A follow-up could have those two branch on result?.skipped (waitlist: leave the request un-invited and tell the admin; onboarding: return false). forgotPassword discards it too but correctly — it returns a generic response either way by design.

2. "Logs the subject only" is accurate, but a subject is not automatically identifier-free. onboardingAlertService.ts:233 builds [Commonly] ${e.username || 'A new user'} typed and got no reply, so for that caller the skip line would carry a username. Not an escalation — the digest's own skip log names a userId on purpose, and that username is already in the mail body and elsewhere in the logs — but if the rule you intend is "no identifiers in this line", the subject is not a safe carrier for it in general. Scope note, not a defect.

State

Lint on services/emailService.ts: clean, 0 problems. Base main, 2 files, merge-tree clean against main 199b5d55e, and clean pairwise with #2299 (5d294218), whose file set is disjoint from this one — I checked the pair because both are open in the same area. (First run of that check reported three conflicts; that was my instrument, not the trees — zsh does not word-split a quoted for item, so the refs never reached merge-tree. Re-run with explicit arguments and a self-merge control: all clean.)

CI at this exact head, run 39016885687 with headSha read back: Test & Coverage success, Service Tests (Tier 1 — real DBs) success; all 14 non-skipped checks pass.

The code gates clean. The press hold is not mine to lift: you stated it presses after the Wed 10-21 call, and vera's security gate is separate — this PASS is a statement about the patch, not clearance to merge.

GATE: PASS