From c208543044154f2b89e6c96c68689b0059608311 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Mon, 31 Aug 2026 10:47:34 -0500 Subject: [PATCH] =?UTF-8?q?feat(engagement):=20deliverability=20=E2=80=94?= =?UTF-8?q?=20suppression,=20bounces=20and=20the=20verification=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ENGAGEMENT.md Phase 9, closing gap G16. Two mechanisms decide that somebody in a rule's audience does not get the mail, and they sit at deliberately different points in the pipeline. `engagement_suppressions` is checked at DELIVERY: an outbox row can sit through a rule's `delay_seconds` grace window and an address can bounce inside it, so the only correct check is the one taken immediately before the transport call — which is also what produces the `status='suppressed'` row with no transport call at all. The Phase 1b verification gate is applied at ENQUEUE, through a new optional `registerDeliveryChannel({ eligible })` that only `email` declares. Filtering the shared audience would have silenced the wrong sink: a rule spanning email and in-app must still put an item in an unverified user's inbox. The excluded counts reach `summary.ineligible` and the admin reach preview, which until now reported an audience size that was never the number of people who would be mailed. `bounceClassify.js` is the only thing that may write a `bounce` row, and it is deliberately NOT `mailer.PERMANENT_CODES`. That set answers "is retrying pointless?" and contains EAUTH and 554 — an auth failure and a relay-wide policy refusal, neither of which is a fact about the recipient. Reusing it would mean one stale SMTP password suppressing every address the worker touched, silently. The classifier reads the RFC 3463 enhanced status first, falls back to a phrase match only past a veto list and only for 550/551/553, and does not suppress anything it is unsure about. Scope is engagement rules only: resets, invites, verification and the contact form still attempt, matching the posture passwordReset.controller.js already stated. Found on the live rig, against a real MariaDB and a real SMTP conversation: a hard bounce was being recorded as `failed`, so the Send Log's "Bounced" filter — a status `engagement_sends` has carried since §4.5 — matched nothing and always would have. It is now its own outcome; the outbox row stays `failed`, since that ENUM has no `bounced` and a bounced row is one that finished unsuccessfully. `address_masked` is this phase's one addition to §4.5's DDL. A hash-only table cannot be operated — an operator cannot tell three typos from a whole domain refusing mail — and the domain survives while the local part is destroyed, so the column can never be read back as an address book. - schema: `engagement_suppressions` (+ `address_masked`, `created_by`) - `GET/POST/DELETE /api/v1/admin/engagement/suppressions`, and Admin → Engagement → Suppressions, the only way out of the list - `sendNotification` returns `smtp: { code, responseCode, response }` - 26 new tests; swagger, routes manifest and guards regenerated Docs: RunicGateway/docs#191. Co-Authored-By: Claude --- client/src/App.jsx | 2 + client/src/api/client.js | 19 + client/src/routes/admin/AdminLayout.jsx | 11 +- .../admin/views/EngagementSuppressions.jsx | 259 +++++++++++ server/db/schema.sql | 42 ++ server/routes.guards.json | 27 ++ server/routes.manifest.json | 12 + server/src/engagement/bounceClassify.js | 216 +++++++++ server/src/engagement/channels.js | 52 ++- server/src/engagement/coreChannels.js | 4 + server/src/engagement/emailChannel.js | 116 ++++- server/src/engagement/engine.js | 25 +- server/src/engagement/suppressions.js | 160 +++++++ .../engagement/engagementRecipients.db.js | 74 ++- .../engagement/engagementSuppressions.db.js | 139 ++++++ .../router/v1/admin/engagement.controller.js | 151 +++++++ .../src/router/v1/admin/engagement.router.js | 50 +++ server/src/utils/engagementWorker.js | 33 +- server/src/utils/mailer.js | 27 +- server/swagger/swagger-output.json | 307 +++++++++++++ server/test/engagementAdmin.test.js | 98 +++- server/test/engagementDeliverability.test.js | 423 ++++++++++++++++++ 22 files changed, 2210 insertions(+), 37 deletions(-) create mode 100644 client/src/routes/admin/views/EngagementSuppressions.jsx create mode 100644 server/src/engagement/bounceClassify.js create mode 100644 server/src/engagement/suppressions.js create mode 100644 server/src/model/engagement/engagementSuppressions.db.js create mode 100644 server/test/engagementDeliverability.test.js diff --git a/client/src/App.jsx b/client/src/App.jsx index c347a5c..48aa783 100644 --- a/client/src/App.jsx +++ b/client/src/App.jsx @@ -47,6 +47,7 @@ import EngagementAudiences from './routes/admin/views/EngagementAudiences.jsx' import EngagementTemplates from './routes/admin/views/EngagementTemplates.jsx' import EngagementTriggers from './routes/admin/views/EngagementTriggers.jsx' import EngagementSendLog from './routes/admin/views/EngagementSendLog.jsx' +import EngagementSuppressions from './routes/admin/views/EngagementSuppressions.jsx' import TeamsAdmin from './routes/admin/views/TeamsAdmin.jsx' import AccountAdmin from './routes/admin/views/AccountAdmin.jsx' import Moderation from './routes/admin/views/Moderation.jsx' @@ -208,6 +209,7 @@ export default function App() { } /> } /> } /> + } /> } /> {/* Staff have an inbox and channel preferences like anyone else — diff --git a/client/src/api/client.js b/client/src/api/client.js index 7ff2d61..d732f14 100644 --- a/client/src/api/client.js +++ b/client/src/api/client.js @@ -434,6 +434,25 @@ export const api = { return req(`/admin/engagement/sends${withQs(qs.toString())}`) }, + // Suppressions (Phase 9). `unsuppressAddress` sends the address in the BODY + // of a DELETE rather than in the path, and that is not style: a path + // parameter lands in the access log, the browser history and every proxy in + // front of the deployment, and this one is a real person's address. The list + // never returns a hash to use instead. + listEngagementSuppressions: ({ limit, offset, reason, channel, search } = {}) => { + const qs = new URLSearchParams() + if (limit) qs.set('limit', String(limit)) + if (offset) qs.set('offset', String(offset)) + if (reason) qs.set('reason', reason) + if (channel) qs.set('channel', channel) + if (search) qs.set('search', search) + return req(`/admin/engagement/suppressions${withQs(qs.toString())}`) + }, + suppressAddress: (address, detail) => + req('/admin/engagement/suppressions', { method: 'POST', body: { address, detail } }), + unsuppressAddress: (address, channel) => + req('/admin/engagement/suppressions', { method: 'DELETE', body: { address, channel } }), + // Teams (docs/website/TEAMS.md §2.11). Three of these mean something // different depending on who calls them: for a moderator, unhide and // setTeamDisplayName file a request and the response says `pending: true`. diff --git a/client/src/routes/admin/AdminLayout.jsx b/client/src/routes/admin/AdminLayout.jsx index 84339e2..e8ec10c 100644 --- a/client/src/routes/admin/AdminLayout.jsx +++ b/client/src/routes/admin/AdminLayout.jsx @@ -98,9 +98,9 @@ export const NAV = [ }, { // Its own top-level group (ENGAGEMENT.md §7.1 Q4), not a section of - // Settings. Settings is already one long page of sections, and these five - // screens are two editors, a catalog and a paged table, none of which is a - // settings section. Email Delivery stays under Settings: configuring a + // Settings. Settings is already one long page of sections, and these six + // screens are two editors, a catalog and two paged tables, none of which is + // a settings section. Email Delivery stays under Settings: configuring a // transport is not the same job as deciding who gets mail. title: 'Engagement', items: [ @@ -109,6 +109,10 @@ export const NAV = [ { to: '/admin/engagement/templates', label: 'Templates', icon: IconTemplate, roles: ['admin'] }, { to: '/admin/engagement/triggers', label: 'Triggers', icon: IconSpark, roles: ['admin'] }, { to: '/admin/engagement/sends', label: 'Send Log', icon: IconLog, roles: ['admin'] }, + // Beside the Send Log rather than inside it (Phase 9): the log answers + // "did that message go out", and this answers "why is this person not + // getting any" - and it is the only screen that can lift a suppression. + { to: '/admin/engagement/suppressions', label: 'Suppressions', icon: IconLog, roles: ['admin'] }, ], }, { @@ -190,6 +194,7 @@ const TITLES = { '/admin/engagement/audiences': 'Engagement Audiences', '/admin/engagement/templates': 'Message Templates', '/admin/engagement/triggers': 'Triggers', + '/admin/engagement/suppressions': 'Suppressions', '/admin/engagement/sends': 'Send Log', } diff --git a/client/src/routes/admin/views/EngagementSuppressions.jsx b/client/src/routes/admin/views/EngagementSuppressions.jsx new file mode 100644 index 0000000..ac9fc44 --- /dev/null +++ b/client/src/routes/admin/views/EngagementSuppressions.jsx @@ -0,0 +1,259 @@ +import { useCallback, useEffect, useState } from 'react' +import { Loading, ErrorState } from '../../../components/PageState.jsx' +import { api } from '../../../api/client.js' + +// Admin → Engagement → Suppressions (ENGAGEMENT.md §4.5 gap G16, Phase 9). +// +// **This screen is the only way out of the suppression list**, which is the whole +// reason it exists rather than the list living as a filter on the Send Log. A +// hard bounce is written by a background worker with no human in the loop, so +// without a lift button a mistyped-then-corrected mailbox is silenced for good +// and nobody ever finds out why that person stopped hearing from the deployment. +// +// **Addresses are shown masked, and the mask is deliberate on both ends.** The +// table holds a sha256 and an `address_masked` — `d***@example.com` — and the +// route never returns the hash, for the same reason the Send Log strips it: a +// digest of every address on the deployment, handed to a browser, is an offline +// dictionary attack waiting to be run. The domain survives because the signal an +// operator is actually hunting is domain-shaped ("everything to this company is +// bouncing" is a different problem from three people mistyping their own +// address), and the local part is destroyed rather than shortened so the list can +// never be read back as an address book. +// +// The consequence to keep in mind while reading this file: **lifting a +// suppression needs the WHOLE address typed in**, because the screen genuinely +// does not have it. That is not a rough edge to be smoothed later — it is the +// privacy design working, and the confirm dialog says so. + +const REASON_LABEL = { + bounce: 'Hard bounce', + complaint: 'Marked as spam', + manual: 'Added by an admin', + unverified: 'Unverified', +} + +const REASON_HELP = { + bounce: 'The receiving server said this mailbox does not exist.', + complaint: 'The recipient reported a message as spam.', + manual: 'Somebody here added it — usually a bounce reported another way.', + unverified: 'Reserved: the verification gate excludes these before a send is queued.', +} + +const PAGE = 50 + +export default function EngagementSuppressions() { + const [rows, setRows] = useState([]) + const [total, setTotal] = useState(0) + const [byReason, setByReason] = useState({}) + const [offset, setOffset] = useState(0) + const [reason, setReason] = useState('') + const [search, setSearch] = useState('') + // Debounced separately from `search` so typing a domain does not fire a request + // per keystroke; `search` is what the input shows, `applied` is what was asked. + const [applied, setApplied] = useState('') + const [adding, setAdding] = useState('') + const [note, setNote] = useState(null) + const [loading, setLoading] = useState(true) + const [error, setError] = useState(null) + + const load = useCallback(async (nextOffset, nextReason, nextSearch) => { + const result = await api.admin.listEngagementSuppressions({ + limit: PAGE, + offset: nextOffset, + reason: nextReason || undefined, + search: nextSearch || undefined, + }) + setRows(result.suppressions || []) + setTotal(result.total || 0) + setByReason(result.byReason || {}) + }, []) + + useEffect(() => { + const t = setTimeout(() => { setOffset(0); setApplied(search.trim()) }, 300) + return () => clearTimeout(t) + }, [search]) + + const refresh = useCallback(async () => { + setLoading(true) + try { + await load(offset, reason, applied) + setError(null) + } catch (err) { + setError(err.message) + } finally { + setLoading(false) + } + }, [load, offset, reason, applied]) + + useEffect(() => { refresh() }, [refresh]) + + async function addByHand(e) { + e.preventDefault() + const address = adding.trim() + if (!address) return + setNote(null) + try { + const result = await api.admin.suppressAddress(address) + // `created: false` is not a failure — the operator asked for the address to + // be suppressed and it is. Saying so plainly beats an error dialog for an + // outcome that is exactly what was wanted. + setNote(result.created + ? `${result.address} will no longer be mailed.` + : `${result.address} was already suppressed.`) + setAdding('') + await refresh() + } catch (err) { + setNote(err.message) + } + } + + async function lift() { + // The address cannot come from the row — the screen has only the mask. Asking + // for it in full is the cost of not storing it, and the prompt says why so it + // does not read as a missing feature. + const address = window.prompt( + 'Type the full address to let it be mailed again.\n\n' + + 'Suppressed addresses are stored one-way, so this screen never has the address itself.', + ) + if (!address || !address.trim()) return + setNote(null) + try { + await api.admin.unsuppressAddress(address.trim()) + setNote(`${address.trim()} can be mailed again.`) + await refresh() + } catch (err) { + setNote(err.message) + } + } + + if (loading && rows.length === 0 && !applied && !reason) return + if (error) return + + const to = Math.min(offset + PAGE, total) + const summary = Object.entries(byReason).filter(([, n]) => n > 0) + + return ( +
+

+ Addresses this deployment has stopped mailing. Engagement rules skip them; password resets, + invites and verification mails still go out, because those are asked for by the person + themselves. Addresses are stored one-way and shown masked. +

+ + {summary.length > 0 && ( +
+ {summary.map(([r, n]) => ( +
+
{n}
+
+ {REASON_LABEL[r] || r} +
+
+ ))} +
+ )} + +
+ + +
+ + +
+ +
+ + {note && ( +

{note}

+ )} + + {total === 0 ? ( +

+ {reason || applied ? 'Nothing matches that filter.' : 'No addresses are suppressed.'} +

+ ) : ( + <> +
+ + + + + + + + + + + + {rows.map((r) => ( + + + + + + + + ))} + +
AddressReasonDetailChannelSince
+ {r.address_masked + ? {r.address_masked} + : not recorded} + + {REASON_LABEL[r.reason] || r.reason} + + {r.detail || ''} + {r.channel} + {new Date(r.created_at).toLocaleString()} +
+
+ +
+ + {offset + 1}–{to} of {total} + +
+ + +
+
+ + )} +
+ ) +} diff --git a/server/db/schema.sql b/server/db/schema.sql index 840e1c2..4b1555d 100644 --- a/server/db/schema.sql +++ b/server/db/schema.sql @@ -1994,3 +1994,45 @@ CREATE TABLE IF NOT EXISTS user_notifications ( -- notification this deployment has ever written. INDEX idx_un_prune (created_at) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; + +-- ── Deliverability: suppression and bounces (ENGAGEMENT.md §4.5 G16 — Phase 9) ── + +-- The addresses this deployment has stopped mailing, and why. +-- +-- **Keyed on the ADDRESS, not the user** (§4.5), and after Phase 1b that is a +-- deliberate choice rather than a workaround for a missing unique index. Two +-- accounts can no longer share an address, but a bounce arrives as an ADDRESS — +-- it does not know which account was behind it, and it stays true after the +-- account that held it changed its address or was deleted. Keying on the user +-- would forget a dead mailbox the moment anybody moved. +-- +-- `address_masked` is Phase 9's one addition to §4.5's DDL, and it exists because +-- the hash-only table cannot be operated. An operator looking at a screen of +-- sha256 digests cannot tell whether the list is three typos or a whole domain +-- refusing mail, and un-suppressing somebody who fixed their mailbox is the one +-- action this table has to support. `d***@example.com` is enough to act on and to +-- see a domain-wide pattern in, and — the reason it is safe — the local part is +-- destroyed rather than shortened, so the column is not an address book and +-- cannot be turned back into one. It is NULLable because a row written from a +-- correlation that only ever held a hash has nothing to mask. +-- +-- **`reason` is not a synonym for "the send failed".** `mailer.PERMANENT_CODES` +-- classifies a failure as not-worth-retrying, and that set contains EAUTH and 554 +-- — an authentication failure and a relay-wide policy refusal, neither of which +-- is a fact about the recipient. Writing a suppression on every terminal failure +-- would mean one wrong SMTP password suppresses every address the worker touches +-- before anyone notices. Only recipient-scoped evidence reaches this table; see +-- `src/engagement/bounceClassify.js`. +CREATE TABLE IF NOT EXISTS engagement_suppressions ( + address_hash CHAR(64) NOT NULL PRIMARY KEY, -- sha256 of the lowercased address + address_masked VARCHAR(190) NULL, -- d***@example.com; never the local part + channel VARCHAR(32) NOT NULL DEFAULT 'email', + reason ENUM('bounce','complaint','manual','unverified') NOT NULL, + detail VARCHAR(500) NULL, + created_by INT NULL, -- the admin, for a manual row; NULL for automatic + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + CONSTRAINT fk_engsup_user FOREIGN KEY (created_by) REFERENCES users(id) ON DELETE SET NULL, + -- The screen's two orderings: newest first, and filtered by reason. + INDEX idx_engsup_created (created_at), + INDEX idx_engsup_reason (reason, created_at) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/server/routes.guards.json b/server/routes.guards.json index 08e139e..65b2ac2 100644 --- a/server/routes.guards.json +++ b/server/routes.guards.json @@ -293,6 +293,33 @@ "requireAuth" ] }, + { + "method": "DELETE", + "path": "/api/v1/admin/engagement/suppressions", + "handlers": 2, + "gates": [ + "noindex", + "requireAuth" + ] + }, + { + "method": "GET", + "path": "/api/v1/admin/engagement/suppressions", + "handlers": 2, + "gates": [ + "noindex", + "requireAuth" + ] + }, + { + "method": "POST", + "path": "/api/v1/admin/engagement/suppressions", + "handlers": 2, + "gates": [ + "noindex", + "requireAuth" + ] + }, { "method": "GET", "path": "/api/v1/admin/engagement/templates", diff --git a/server/routes.manifest.json b/server/routes.manifest.json index 20706a8..6767767 100644 --- a/server/routes.manifest.json +++ b/server/routes.manifest.json @@ -129,6 +129,18 @@ "method": "GET", "path": "/api/v1/admin/engagement/sends" }, + { + "method": "DELETE", + "path": "/api/v1/admin/engagement/suppressions" + }, + { + "method": "GET", + "path": "/api/v1/admin/engagement/suppressions" + }, + { + "method": "POST", + "path": "/api/v1/admin/engagement/suppressions" + }, { "method": "GET", "path": "/api/v1/admin/engagement/templates" diff --git a/server/src/engagement/bounceClassify.js b/server/src/engagement/bounceClassify.js new file mode 100644 index 0000000..b2a1203 --- /dev/null +++ b/server/src/engagement/bounceClassify.js @@ -0,0 +1,216 @@ +// ── Which send failures are facts about the RECIPIENT ─────────────────────── +// +// ENGAGEMENT.md Phase 9. The suppression list's whole value is that an address on +// it is genuinely undeliverable; the moment it fills with addresses that were +// fine, an operator learns to ignore it and it may as well not exist. This file +// is the one place that judgement is made. +// +// **It is deliberately NOT `mailer.PERMANENT_CODES`, and reusing that set would +// have been a mass-suppression bug.** That set answers "is retrying pointless?" +// and holds `EAUTH` and `554` alongside `550` — an authentication failure and a +// relay-wide policy refusal. Both are permanent and neither says anything about +// the person: one wrong SMTP password would suppress every address the outbox +// worker touched before anybody noticed the mail had stopped. "Do not retry" and +// "this mailbox does not exist" are different questions, and this file only +// answers the second. +// +// **The primary signal is the enhanced status code (RFC 3463), not the reply +// code.** `550` alone is the catch-all every refusal arrives as; `5.1.1` means +// one specific thing — no such mailbox. Every relay worth configuring emits an +// enhanced code, so it is read first and, when present, decides on its own. +// +// **The fallback is narrow on purpose.** Without an enhanced code a phrase match +// is all that is left, and phrase matching is how a classifier quietly starts +// suppressing everything. So it applies only after the reply code has already +// narrowed the failure to the recipient address — 550, 551 and 553 are RFC 5321's +// recipient-address codes — and only for phrases that cannot mean anything else, +// with a veto list checked first. `552` (storage exceeded) and `554` (transaction +// failed) are excluded from even that: a full mailbox gets emptied, and a generic +// transaction failure is generic. +// +// Anything this file is unsure about is NOT suppressed. The cost of a false +// negative is mailing a dead address again next month; the cost of a false +// positive is a person who silently stops hearing from the deployment and has no +// way to find out. + +// RFC 3463 subject.detail pairs that mean "this address will not accept mail, +// today or ever". Kept as strings because `5.1.10` and `5.1.1` are different +// codes and numeric parsing loses that. +const PERMANENT_RECIPIENT = new Set([ + '1.1', // bad destination mailbox address — no such user + '1.2', // bad destination system address — the domain does not take mail + '1.3', // bad destination mailbox address syntax + '1.6', // mailbox has moved, no forwarding address + '1.10', // recipient address has a null MX (RFC 7505) + '2.1', // mailbox disabled, not accepting messages +]) + +// Enhanced subjects that are permanent but are NOT about the recipient. Listed +// rather than merely omitted, because each is a plausible-looking 5.x.y that a +// later edit would otherwise be tempted to add: +// 2.2 — mailbox full. Permanent-coded by some relays, emptied by every user. +// 7.x — policy. Our sending reputation, our SPF, our content; the recipient is +// the one party it is not about. +// 3.x — the destination MAIL SYSTEM is full or refusing. Not the mailbox. +// 5.x — protocol failure. A bug at one end or the other. +const NEVER_RECIPIENT_SUBJECTS = new Set(['3', '5', '7']) + +// RFC 5321 reply codes that name the recipient address specifically. 554 is +// absent deliberately: "transaction failed" is what a relay reaches for when it +// does not want to say why, and it is the commonest shape of a content or policy +// rejection. +const RECIPIENT_REPLY_CODES = new Set([550, 551, 553]) + +// Phrases that only ever mean "no such mailbox", checked only once a reply code +// above has established the failure is about the address. Each is a substring of +// a real refusal from a widely deployed MTA (Postfix, Exim, Exchange, Google, +// Microsoft 365). +const NO_SUCH_MAILBOX = [ + 'user unknown', + 'unknown user', + 'no such user', + 'no such recipient', + 'unknown recipient', + 'invalid recipient', + 'recipient address rejected', + 'recipient not found', + 'address does not exist', + 'does not exist', + 'mailbox unavailable', + 'mailbox not found', + 'no mailbox', + 'user does not exist', + 'address rejected', +] + +// Phrases that appear alongside the ones above and mean the opposite, checked +// FIRST. "Mailbox unavailable" is a substring of the sentence a relay sends when +// a mailbox is merely full, so a substring match with no veto list would read a +// temporary condition as a dead address. +const NOT_A_DEAD_MAILBOX = [ + 'full', + 'quota', + 'storage', + 'temporar', + 'try again', + 'greylist', + 'rate limit', + 'too many', + 'spam', + 'blocked', + 'blacklist', + 'blocklist', + 'reputation', + 'policy', + 'authentication', + 'not authorized', +] + +/** + * The enhanced status code in an SMTP response, as `{ class, subject, detail }`, + * or null. + * + * Anchored to the start of the line rather than searched for anywhere in it: a + * bounce that quotes another server's answer ("...said: 550 5.1.1...") contains + * two, and the one that matters is the one this relay just gave us. A free search + * finds whichever comes first, which is not the same thing. + */ +function parseEnhanced(response) { + if (!response) return null + const m = /^\s*(\d{3})[\s-]+(\d)\.(\d{1,3})\.(\d{1,3})\b/.exec(String(response)) + if (!m) return null + return { class: m[2], subject: m[3], detail: m[4] } +} + +/** The three-digit reply code, off the error object or out of the response text. */ +function replyCode(err) { + const direct = Number(err && err.responseCode) + if (Number.isInteger(direct) && direct >= 400 && direct <= 599) return direct + const m = /^\s*(\d{3})\b/.exec(String((err && err.response) || '')) + return m ? Number(m[1]) : null +} + +const lower = (s) => String(s || '').toLowerCase() + +/** + * Should this send failure suppress the address? + * + * @param {object} err the error a transport's send threw, or an object carrying + * the `responseCode` / `response` / `code` lifted off one + * @returns {{ suppress: boolean, reason: string, evidence: string|null }} + * + * `reason` is populated on a refusal too, and that is not decoration: it becomes + * the send log's `detail`, so "not suppressed: 554 does not name the recipient + * address" is the line that stops somebody re-deriving this decision from an + * unexplained non-event six months from now. + */ +function classify(err) { + const e = err || {} + const response = e.response || e.message || '' + const enhanced = parseEnhanced(response) + const code = replyCode(e) + + // No reply code at all means the failure happened before or outside the SMTP + // transaction: the connection, the credentials, the socket. Never the + // recipient. `EAUTH` lands here, which is the whole reason this file exists. + if (!code) { + return { + suppress: false, + reason: `no SMTP reply code (${e.code || 'transport failure'}); not a recipient failure`, + evidence: null, + } + } + + if (code < 500) { + return { suppress: false, reason: `${code} is a temporary failure`, evidence: null } + } + + if (enhanced) { + const pair = `${enhanced.subject}.${enhanced.detail}` + if (enhanced.class !== '5') { + return { suppress: false, reason: `enhanced status ${enhanced.class}.${pair} is not permanent`, evidence: null } + } + if (PERMANENT_RECIPIENT.has(pair)) { + return { suppress: true, reason: 'bounce', evidence: `5.${pair}` } + } + if (NEVER_RECIPIENT_SUBJECTS.has(enhanced.subject)) { + return { + suppress: false, + reason: `5.${pair} is about the server or our standing with it, not the address`, + evidence: `5.${pair}`, + } + } + // A permanent 5.x.y this file has no opinion on. Unknown means no. + return { + suppress: false, + reason: `5.${pair} is not a known recipient failure`, + evidence: `5.${pair}`, + } + } + + // No enhanced code: the narrow fallback. + if (!RECIPIENT_REPLY_CODES.has(code)) { + return { suppress: false, reason: `${code} does not name the recipient address`, evidence: null } + } + const text = lower(response) + const veto = NOT_A_DEAD_MAILBOX.find((p) => text.includes(p)) + if (veto) { + return { suppress: false, reason: `${code}, but the response says "${veto}"`, evidence: null } + } + const hit = NO_SUCH_MAILBOX.find((p) => text.includes(p)) + if (hit) { + return { suppress: true, reason: 'bounce', evidence: `${code} "${hit}"` } + } + return { suppress: false, reason: `${code} with no enhanced status and no recognised reason`, evidence: null } +} + +module.exports = { + classify, + parseEnhanced, + replyCode, + PERMANENT_RECIPIENT, + NEVER_RECIPIENT_SUBJECTS, + RECIPIENT_REPLY_CODES, + NO_SUCH_MAILBOX, + NOT_A_DEAD_MAILBOX, +} diff --git a/server/src/engagement/channels.js b/server/src/engagement/channels.js index 0f551c3..da80f56 100644 --- a/server/src/engagement/channels.js +++ b/server/src/engagement/channels.js @@ -53,6 +53,24 @@ const isMode = (value) => MODES.includes(value) * without one is declared but not yet deliverable, which is exactly what * `inapp` is until Phase 7; the worker finishes such a row `failed` and * says so in the send log rather than pretending it was sent. + * @param {(userIds: number[]) => Promise<{userIds: number[], excluded: object}>} [def.eligible] + * Phase 9. Narrow an already-resolved audience to the users this channel + * may write an outbox row for, and say how many it dropped and why. + * + * **It exists so the engine can stay channel-agnostic.** The verification + * gate is an email fact — an unverified address is a reason not to mail + * somebody and no reason at all not to put an item in their inbox — and a + * rule may name both channels. Filtering the shared audience before the + * per-channel loop would have silenced the wrong sink; an `if (channel === + * 'email')` in `engine.js` would have put a channel's rule inside the + * generic engine. This is the seam that is neither. + * + * Distinct from `deliver`'s refusals on purpose: this runs at ENQUEUE and + * is for standing properties of a user (is this address verified), which + * are stable across a delay window and are worth not writing a row for. + * A suppression is not one of those — it can appear between the enqueue + * and the send — so it is checked in `deliver`, where it produces a + * `suppressed` row in the send log the acceptance criterion asks for. */ function registerDeliveryChannel(def) { if (!def || typeof def !== 'object') throw new Error('registerDeliveryChannel: definition required') @@ -81,7 +99,7 @@ function registerDeliveryChannel(def) { // silently never delivers — the failure Phase 3 deferred the whole behavioural // half to avoid freezing, and the one the worker's "no delivery implementation // yet" branch would report as if it were by design. - for (const fn of ['addressFor', 'deliver']) { + for (const fn of ['addressFor', 'deliver', 'eligible']) { if (def[fn] !== undefined && typeof def[fn] !== 'function') { throw new Error(`registerDeliveryChannel(${id}): ${fn} must be a function`) } @@ -96,6 +114,7 @@ function registerDeliveryChannel(def) { supportsDigest, addressFor: def.addressFor, deliver: def.deliver, + eligible: def.eligible, }) return id } @@ -103,13 +122,14 @@ function registerDeliveryChannel(def) { /** * Every channel, in registration order. The preferences screen's column set. * - * **Declarative fields only** — `addressFor` and `deliver` are stripped. This is - * what a route serializes, and a function on an object bound for `res.json` is a - * key that silently disappears rather than an error; keeping the boundary here - * means the API shape is decided in one place instead of by JSON.stringify. + * **Declarative fields only** — `addressFor`, `deliver` and `eligible` are + * stripped. This is what a route serializes, and a function on an object bound + * for `res.json` is a key that silently disappears rather than an error; keeping + * the boundary here means the API shape is decided in one place instead of by + * JSON.stringify. */ const all = () => - [...channels.values()].map(({ addressFor, deliver, ...declared }) => ({ ...declared })) + [...channels.values()].map(({ addressFor, deliver, eligible, ...declared }) => ({ ...declared })) /** Just the ids. */ const ids = () => [...channels.keys()] @@ -137,6 +157,25 @@ const modesFor = (id) => { /** Is `mode` a mode this channel accepts? The gate on every preference write. */ const acceptsMode = (id, mode) => modesFor(id).includes(mode) +/** + * Narrow an audience to the users this channel may enqueue for (Phase 9). + * + * The default for a channel that declares no `eligible` is "everyone the + * audience resolved to", which is what every channel but email does. It lives + * here rather than at each call site so the two consumers — the engine and the + * admin reach preview — cannot answer the question differently, which is exactly + * how a preview comes to promise a number the engine will not deliver. + */ +async function eligibleFor(id, userIds) { + const c = channels.get(id) + if (!c || typeof c.eligible !== 'function') return { userIds: userIds.slice(), excluded: {} } + const result = await c.eligible(userIds) + return { + userIds: (result && result.userIds) || [], + excluded: (result && result.excluded) || {}, + } +} + // Test-only: the registry is module-level state. function _reset() { channels.clear() @@ -153,5 +192,6 @@ module.exports = { defaultMode, modesFor, acceptsMode, + eligibleFor, _reset, } diff --git a/server/src/engagement/coreChannels.js b/server/src/engagement/coreChannels.js index 59d3efd..ab2d2e0 100644 --- a/server/src/engagement/coreChannels.js +++ b/server/src/engagement/coreChannels.js @@ -66,6 +66,10 @@ const CHANNELS = [ // time (§4.2b), which is a different delivery path rather than a batched one. addressFor: emailChannel.addressFor, deliver: emailChannel.deliver, + // Phase 9: the only channel that declares one. The Phase 1b verification + // gate is an email fact, and this is the seam that keeps it out of the + // generic engine - see channels.js's `eligible` docs. + eligible: emailChannel.eligible, }, { id: 'inapp', diff --git a/server/src/engagement/emailChannel.js b/server/src/engagement/emailChannel.js index f801665..6f56a92 100644 --- a/server/src/engagement/emailChannel.js +++ b/server/src/engagement/emailChannel.js @@ -29,11 +29,12 @@ // `teamName` (a display string the cooldown keys on) and the scope is `team:12`. // A Team renamed between the mail and the click must not orphan the link in it. -const crypto = require('crypto') const rulesDb = require('../model/engagement/engagementRules.db') const recipients = require('../model/engagement/engagementRecipients.db') const templates = require('./templates') const projection = require('./projection') +const suppressions = require('./suppressions') +const settings = require('../model/settings/settings.model') const unsubscribeToken = require('../utils/unsubscribeToken') const log = require('../utils/logger')('engagement') @@ -54,12 +55,10 @@ const DEFAULT_TEMPLATE = 'notify.event' const baseUrl = () => templates.baseUrl() -// Lower-cased first: a bounce reported for "Darrow@example.com" has to match the -// row written for "darrow@example.com", and a hash of two spellings is two -// different rows. The local part is technically case-sensitive per RFC 5321 and -// no relay anybody deploys treats it that way. -const hashAddress = (address) => - crypto.createHash('sha256').update(String(address).trim().toLowerCase()).digest('hex') +// Re-exported rather than defined here since Phase 9: the send log's hash and +// the suppression list's key have to be the same function or a bounce never finds +// the row it belongs to. `suppressions.js` owns it, next to the masking. +const hashAddress = suppressions.hashAddress /** * The two unsubscribe URLs for one recipient of one scope, or nulls. @@ -94,6 +93,52 @@ function unsubscribeUrls(userId, scopeKey) { /** Where this channel would send to, or null. */ const addressFor = (userId) => recipients.addressFor(userId) +/** + * Narrow an audience to the users this channel may write an outbox row for + * (Phase 9, decision 4). + * + * **One gate, and it is the Phase 1b verification setting.** With + * `email_verification_required` on, a user whose address is unverified is + * excluded here rather than refused at delivery, and the org lead settled it that + * way for two reasons. It is a STANDING property — unlike a suppression, which + * can appear inside a `delay_seconds` window and therefore has to be re-checked + * at send time — so the outbox row would be written only to be thrown away. And a + * deployment that upgraded before verifying anybody has an audience that is + * almost entirely unverified: excluding at delivery would write a `suppressed` + * row per person per rule firing, which is a send log nobody can read. + * + * The count comes back so the admin reach preview can say "1,204 excluded: + * unverified" instead of quietly promising a number the engine will not deliver. + * + * **It fails OPEN, and the try/catch is load-bearing rather than defensive + * habit.** `settings.isEmailVerificationRequired` swallows its own errors and + * answers `off`, but `unverifiedAmong` does not, and an uncaught throw here does + * not fail one recipient — `applyRule` awaits this before the per-user loop, so + * it would abandon the whole rule for every channel it names. A database having + * a bad minute would become a rule that silently sent nothing, with a clean send + * log and nothing in the outbox to retry. Same direction as the suppression + * check, for the same reason: the recoverable mistake is mail going out, not mail + * silently stopping. + */ +async function eligible(userIds) { + const list = userIds || [] + if (!list.length) return { userIds: [], excluded: {} } + try { + if (!(await settings.isEmailVerificationRequired())) { + return { userIds: list.slice(), excluded: {} } + } + const unverified = await recipients.unverifiedAmong(list) + if (!unverified.size) return { userIds: list.slice(), excluded: {} } + return { + userIds: list.filter((id) => !unverified.has(Number(id))), + excluded: { unverified: unverified.size }, + } + } catch (err) { + log.error('verification gate could not be evaluated; not excluding anyone', { message: err.message }) + return { userIds: list.slice(), excluded: {} } + } +} + /** * Deliver one claimed outbox row. * @@ -130,11 +175,60 @@ async function deliver(row) { log.debug('template variables had no value', { key, missing: rendered.missing }) } + // **The suppression check is HERE and not at enqueue** (Phase 9). An outbox + // row can sit through a `delay_seconds` grace window, and an address can hard + // bounce inside it — so the only check that can be correct is the one taken + // immediately before the transport call. It is also the check the acceptance + // criterion describes: a `suppressed` row in the send log, and no transport + // call at all. + const blocked = await suppressions.isSuppressed(to.address) + if (blocked) { + return { + ok: false, + suppressed: true, + detail: `address is suppressed (${blocked.reason})`, + addressHash: hashAddress(to.address), + } + } + const result = await mailer().sendNotification({ to: to.address, rendered, ...unsub }) + + // A failed send is where a hard bounce enters the system on SMTP, and the + // reason it is worth catching rather than waiting for an API provider: a + // single-recipient send refused at RCPT TO is a synchronous 5.1.1, which is + // the most valuable deliverability signal there is and it was already being + // thrown away. `considerFailure` is narrow — see bounceClassify.js — and its + // note goes into the log on BOTH outcomes, so "this failed and was not + // suppressed" says why. + if (result && !result.ok && result.smtp) { + const verdict = await suppressions.considerFailure({ address: to.address, error: result.smtp }) + // A bounce is terminal by definition. Overriding `retry` matters because + // `PERMANENT_CODES` does not contain every code that can carry a 5.1.x, so + // without this a genuine dead mailbox could still be retried four more + // times — each one another refusal on our record with the relay. + if (verdict.suppressed) { + return { + ok: false, + retry: false, + // `engagement_sends.status` has carried 'bounced' since §4.5 and + // nothing wrote it until here, so the Send Log's "Bounced" filter + // matched nothing — the live rig is what showed that. It is a distinct + // status rather than a flavour of 'failed' because the two need + // different actions: a failure means look at the relay, and a bounce + // means that person's address is gone. + bounced: true, + transport: result.transport, + detail: `${result.detail} — ${verdict.note}`, + addressHash: hashAddress(to.address), + } + } + return { ...result, detail: `${result.detail} — ${verdict.note}`, addressHash: hashAddress(to.address) } + } + // The send log stores a sha256 of the address and never the address itself - // (schema.sql): enough to correlate a bounce in Phase 9, useless as a mailing - // list. Attached on every outcome, because a failure is exactly the row a - // bounce would need to be matched against. + // (schema.sql): enough to correlate a bounce, useless as a mailing list. + // Attached on every outcome, because a failure is exactly the row a bounce + // would need to be matched against. return { ...result, addressHash: hashAddress(to.address) } } catch (err) { // See the header: a throw here would be retried as if it were the relay's @@ -144,4 +238,4 @@ async function deliver(row) { } } -module.exports = { addressFor, deliver, unsubscribeUrls, hashAddress, DEFAULT_TEMPLATE } +module.exports = { addressFor, eligible, deliver, unsubscribeUrls, hashAddress, DEFAULT_TEMPLATE } diff --git a/server/src/engagement/engine.js b/server/src/engagement/engine.js index 6bfcd9b..e93f563 100644 --- a/server/src/engagement/engine.js +++ b/server/src/engagement/engine.js @@ -120,7 +120,18 @@ async function subscribedTo(userIds, streamId, channel, scopeKey = null) { * for tests; it is not read by the caller for control flow. */ async function applyRule(rule, event, now) { - const summary = { ruleId: rule.id, enqueued: 0, deduped: 0, cooled: 0, capped: 0, skipped: null } + const summary = { + ruleId: rule.id, + enqueued: 0, + deduped: 0, + cooled: 0, + capped: 0, + // Phase 9: people a CHANNEL refused to enqueue for, by reason. Counted + // separately from `cooled` and `capped` because those are the engine holding + // a message back and this is a channel saying it cannot carry one at all. + ineligible: {}, + skipped: null, + } if (!conditions.evaluate(rule.conditions, event.data)) { summary.skipped = 'conditions' @@ -176,7 +187,17 @@ async function applyRule(rule, event, now) { const dueAt = new Date(now.getTime() + Math.max(0, rule.delay_seconds) * 1000) for (const channel of live) { - const eligible = await subscribedTo(resolved.userIds, event.triggerId, channel, event.scopeKey) + // Phase 9, and it runs BEFORE the preference filter rather than after. Both + // orders reach the same recipients; this one costs one query against the + // narrower set only when the channel declares an `eligible` at all, and it + // means `summary.ineligible` counts people the CHANNEL cannot reach rather + // than people who happened to also be opted in. Channels that declare none — + // push and in-app — pass straight through. + const gated = await channels.eligibleFor(channel, resolved.userIds) + for (const [why, n] of Object.entries(gated.excluded)) { + summary.ineligible[why] = (summary.ineligible[why] || 0) + n + } + const eligible = await subscribedTo(gated.userIds, event.triggerId, channel, event.scopeKey) for (const userId of eligible) { if (budget <= 0) { summary.capped += 1 diff --git a/server/src/engagement/suppressions.js b/server/src/engagement/suppressions.js new file mode 100644 index 0000000..b87c9ac --- /dev/null +++ b/server/src/engagement/suppressions.js @@ -0,0 +1,160 @@ +// ── The suppression list ─────────────────────────────────────────────────── +// +// ENGAGEMENT.md G16, Phase 9. Addresses this deployment has stopped mailing, +// and the two questions asked of them: "may I send to this one?" at delivery +// time, and "why did this one stop?" on the admin screen. +// +// **Scope: the engagement email channel only** (Phase 9 decision 2). A password +// reset, an invite, a verification mail and the contact form are all +// user-INITIATED and still attempt, exactly as they still attempt to an +// unverified address (`passwordReset.controller.js`). The posture is the same one +// that file already states: a background system's opinion about an address must +// not be able to lock somebody out of their own account. One reset to a dead +// mailbox is not a reputation problem; a rule mailing three thousand people every +// week is, and that is what this list guards. +// +// **The table holds a hash and a mask, never an address.** The hash is what +// correlates a bounce back to an `engagement_sends` row (Phase 6 was already +// writing `address_hash` on every outcome for this). The mask — +// `d***@example.com` — is Phase 9's one addition to §4.5's DDL and exists because +// a screen of sha256 digests cannot be operated: an operator has to be able to +// see that a whole domain is refusing mail, and to find the person who fixed +// their mailbox and let them back in. The local part is DESTROYED rather than +// shortened, so the column cannot be turned back into an address book. +// +// **Nothing here writes a suppression from "the send failed".** What may write +// one is `bounceClassify.classify`, which is a much narrower question — see that +// file's header for why reusing `mailer.PERMANENT_CODES` would have suppressed +// every address the moment an SMTP password went stale. + +const crypto = require('crypto') + +const db = require('../model/engagement/engagementSuppressions.db') +const bounceClassify = require('./bounceClassify') +const log = require('../utils/logger')('engagement') + +const REASONS = ['bounce', 'complaint', 'manual', 'unverified'] + +/** + * The key an address is stored under. + * + * Lower-cased first, and that matters more here than anywhere else in the + * subsystem: a bounce reported for `Darrow@example.com` has to find the row + * written for `darrow@example.com`, and a hash of two spellings is two rows that + * never meet. RFC 5321 says the local part is technically case-sensitive; no + * relay anybody deploys treats it that way. + */ +const hashAddress = (address) => + crypto.createHash('sha256').update(String(address).trim().toLowerCase()).digest('hex') + +/** + * `darrow@example.com` → `d***@example.com`. Null for anything that is not an + * address. + * + * The domain survives intact because domain-level patterns are the signal an + * operator is actually looking for — "everything to this company is bouncing" is + * a different problem from three people mistyping their own address, and only the + * domain distinguishes them. + * + * The first character of the local part survives only when there are at least + * three, which is not fussiness: for a two-letter local part, one revealed + * character plus the domain is most of the address. + */ +function maskAddress(address) { + const s = String(address || '').trim() + const at = s.lastIndexOf('@') + if (at <= 0 || at === s.length - 1) return null + const local = s.slice(0, at) + const domain = s.slice(at + 1).toLowerCase() + const head = local.length >= 3 ? local[0].toLowerCase() : '' + return `${head}***@${domain}`.slice(0, 190) +} + +/** Is this address suppressed on this channel? */ +async function isSuppressed(address, channel = 'email') { + if (!address) return null + try { + return await db.get(hashAddress(address), channel) + } catch (err) { + // Fail OPEN, and the direction is deliberate. A database that cannot answer + // "is this suppressed" must not stop the deployment's mail; the failure mode + // it would otherwise produce is total silence with a clean send log, which is + // exactly G22's shape. Mailing one dead address during an outage is the + // cheaper mistake. + log.error('suppression check failed; sending anyway', { message: err.message }) + return null + } +} + +/** + * Suppress an address. Returns true when this call created the row. + * + * `reason` is validated rather than trusted: it is an ENUM in the schema, so an + * unknown value is a 500 from the driver at the worst possible moment (inside a + * failure handler), and the callers include an admin route. + */ +async function suppress({ address, reason, detail = null, channel = 'email', createdBy = null }) { + if (!address) return false + if (!REASONS.includes(reason)) throw new Error(`suppress: unknown reason "${reason}"`) + const created = await db.add({ + address_hash: hashAddress(address), + address_masked: maskAddress(address), + channel, + reason, + detail, + created_by: createdBy, + }) + if (created) { + // Masked, never the address — the same rule every other log line in this + // subsystem follows. It is logged at all because an address dropping off the + // mailing list is the kind of change an operator finds out about weeks later + // otherwise. + log.info('address suppressed', { address: maskAddress(address), reason, channel }) + } + return created +} + +/** Un-suppress. Returns true when a row was removed. */ +async function unsuppress(address, channel = 'email') { + if (!address) return false + const removed = await db.remove(hashAddress(address), channel) + if (removed) log.info('suppression lifted', { address: maskAddress(address), channel }) + return removed +} + +/** + * Consider a failed send for suppression, and say what was decided. + * + * The seam between a delivery failure and this list, and the only one — nothing + * else in the codebase writes a `bounce` row. Called from `emailChannel.deliver` + * with the error the transport threw. + * + * @returns {Promise<{suppressed: boolean, note: string}>} `note` goes into the + * send log's detail, on both outcomes. + */ +async function considerFailure({ address, error, channel = 'email' }) { + const verdict = bounceClassify.classify(error) + if (!verdict.suppress) { + return { suppressed: false, note: `not suppressed (${verdict.reason})` } + } + try { + const detail = verdict.evidence ? `hard bounce: ${verdict.evidence}` : 'hard bounce' + await suppress({ address, reason: 'bounce', detail, channel }) + return { suppressed: true, note: detail } + } catch (err) { + // A failure to record the suppression must not change how the send itself is + // reported. The mail failed either way, and that is the row the log owes. + log.error('could not record a suppression', { message: err.message }) + return { suppressed: false, note: `hard bounce, not recorded: ${err.message}` } + } +} + +module.exports = { + hashAddress, + maskAddress, + isSuppressed, + suppress, + unsuppress, + considerFailure, + REASONS, +} diff --git a/server/src/model/engagement/engagementRecipients.db.js b/server/src/model/engagement/engagementRecipients.db.js index f942615..6be43a5 100644 --- a/server/src/model/engagement/engagementRecipients.db.js +++ b/server/src/model/engagement/engagementRecipients.db.js @@ -132,11 +132,14 @@ const storedModes = async (userIds, streamId, channel) => { * between the emit and the send is exactly the case this catches. The cost is one * primary-key lookup on a path that is about to open an SMTP conversation. * - * **It does not gate on `email_verified`.** Whether an unverified address may - * receive opt-in mail is §7.1 Q1's narrower half, and it is a Phase 9 decision - * with the suppression list in front of it; deciding it here by accident would - * mean every deployment that upgraded before verifying its users stopped mailing - * them. + * **It still does not gate on `email_verified`, and Phase 9 kept it that way.** + * §7.1 Q1's narrower half was settled by the org lead 2026-08-31: the gate + * excludes an unverified user at ENQUEUE, in `emailChannel.eligible`, so no + * outbox row is written and the admin reach preview can say how many were + * dropped. Adding the same condition here as well would look like defence in + * depth and would in fact be a second, invisible answer to the question — this + * function is also what a password reset would reach if it ever routed through + * the channel, and reset mail is deliberately ungated (`passwordReset.controller.js`). */ const addressFor = async (userId) => { const rows = await query( @@ -147,4 +150,63 @@ const addressFor = async (userId) => { return rows.length ? { address: rows[0].email } : null } -module.exports = { active, staff, subscribers, filterActive, storedModes, addressFor, MAX_AUDIENCE } +/** + * Which of these users hold an UNVERIFIED address - the set the Phase 1b gate + * excludes when it is on (Phase 9). + * + * A user with no address at all is in this set, and that is not incidental: the + * gate's question is "may we mail this person", and nowhere to send is a stronger + * no than an unconfirmed somewhere. `addressFor` refuses them at delivery either + * way; including them here is what stops an outbox row being written for a send + * that is already known to be impossible. + * + * One query for a whole audience. The engine calls it once per rule per event + * with up to MAX_AUDIENCE ids, so a per-user lookup would be five thousand round + * trips on the path that is supposed to be the cheap one. + */ +const unverifiedAmong = async (userIds) => { + const wanted = [...new Set(userIds.map(Number).filter((n) => Number.isInteger(n) && n > 0))] + if (!wanted.length) return new Set() + const capped = wanted.slice(0, MAX_AUDIENCE) + const rows = await query( + `SELECT id FROM users + WHERE id IN (${marks(capped)}) + AND (email_verified = 0 OR email IS NULL OR email = '')`, + capped, + ) + return new Set(ids(rows)) +} + +/** + * The addresses for a set of active users, as a Map - what the admin reach + * preview hashes to count how many of them are suppressed. + * + * It returns PLAINTEXT, which is the one thing this subsystem otherwise avoids, + * and there is no way around it: a suppression is keyed on the sha256 of an + * address, so answering "how many of these people are suppressed" requires + * hashing each one. The caller (`engagement.controller`) hashes immediately and + * returns only a count - no route ever serializes what this returns. + */ +const addressesFor = async (userIds) => { + const wanted = [...new Set(userIds.map(Number).filter((n) => Number.isInteger(n) && n > 0))] + if (!wanted.length) return new Map() + const capped = wanted.slice(0, MAX_AUDIENCE) + const rows = await query( + `SELECT id, email FROM users + WHERE id IN (${marks(capped)}) AND status = 'active' AND email IS NOT NULL AND email <> ''`, + capped, + ) + return new Map(rows.map((r) => [Number(r.id), r.email])) +} + +module.exports = { + active, + staff, + subscribers, + filterActive, + storedModes, + addressFor, + unverifiedAmong, + addressesFor, + MAX_AUDIENCE, +} diff --git a/server/src/model/engagement/engagementSuppressions.db.js b/server/src/model/engagement/engagementSuppressions.db.js new file mode 100644 index 0000000..5e68570 --- /dev/null +++ b/server/src/model/engagement/engagementSuppressions.db.js @@ -0,0 +1,139 @@ +const { query } = require('../../utils/db') + +/** + * The suppression list (ENGAGEMENT.md G16, Phase 9). + * + * Every function here takes an ALREADY HASHED address. Hashing lives in + * `engagement/suppressions.js` beside the masking, so the two can never disagree + * about what a row for one address looks like; this file only reads and writes. + */ + +/** Is this address suppressed on this channel? The row, or null. */ +const get = async (addressHash, channel = 'email') => { + const rows = await query( + 'SELECT * FROM engagement_suppressions WHERE address_hash = ? AND channel = ?', + [addressHash, channel], + ) + return rows.length ? rows[0] : null +} + +/** + * Suppress an address, or leave an existing row exactly as it is. + * + * `INSERT IGNORE`, deliberately, rather than an upsert. The FIRST reason an + * address was suppressed is the true one and the one an operator needs: an + * address that hard-bounced in March and was then manually re-added in June + * should still read `bounce`, because that is the fact that explains the mail + * stopping. An upsert would let the most recent write overwrite the diagnosis. + * + * @returns {Promise} true when this call created the row + */ +const add = async (entry) => { + const result = await query( + `INSERT IGNORE INTO engagement_suppressions + (address_hash, address_masked, channel, reason, detail, created_by) + VALUES (?, ?, ?, ?, ?, ?)`, + [ + entry.address_hash, + entry.address_masked ?? null, + entry.channel || 'email', + entry.reason, + entry.detail ? String(entry.detail).slice(0, 500) : null, + entry.created_by ?? null, + ], + ) + return Number(result.affectedRows || 0) > 0 +} + +/** + * Un-suppress an address. The one way out of this table, which is why the admin + * screen exists at all (Phase 9 decision 3). + * + * @returns {Promise} true when a row was removed + */ +const remove = async (addressHash, channel = 'email') => { + const result = await query( + 'DELETE FROM engagement_suppressions WHERE address_hash = ? AND channel = ?', + [addressHash, channel], + ) + return Number(result.affectedRows || 0) > 0 +} + +/** WHERE-clause builder shared by `list` and `count`, so the two cannot disagree. */ +const filters = ({ reason = null, channel = null, search = null } = {}) => { + const where = [] + const params = [] + if (reason) { + where.push('reason = ?') + params.push(reason) + } + if (channel) { + where.push('channel = ?') + params.push(channel) + } + if (search) { + // Against the MASKED column only. Searching the hash would need the caller to + // hash first, which makes a partial search impossible, and there is no + // plaintext column to search — that is the point of the table. A domain + // ("example.com") is what an operator actually types here, and the mask keeps + // the domain intact precisely so this works. + where.push('address_masked LIKE ?') + params.push(`%${String(search).slice(0, 120)}%`) + } + return { clause: where.length ? `WHERE ${where.join(' AND ')}` : '', params } +} + +/** The admin list, newest first. */ +const list = (opts = {}) => { + const { clause, params } = filters(opts) + return query( + `SELECT * FROM engagement_suppressions ${clause} ORDER BY created_at DESC, address_hash LIMIT ? OFFSET ?`, + [...params, opts.limit || 50, opts.offset || 0], + ) +} + +/** How many rows match the same filters — the paged screen's total. */ +const count = async (opts = {}) => { + const { clause, params } = filters(opts) + const [row] = await query(`SELECT COUNT(*) AS n FROM engagement_suppressions ${clause}`, params) + return Number(row?.n || 0) +} + +/** + * How many suppressions per reason — the summary the screen leads with. + * + * A count by reason is the difference between "eleven addresses are suppressed" + * and "eleven addresses hard-bounced", and only the second tells an operator + * whether to go and look at their relay. + */ +const countsByReason = async () => { + const rows = await query( + 'SELECT reason, COUNT(*) AS n FROM engagement_suppressions GROUP BY reason', + ) + const out = {} + for (const r of rows) out[r.reason] = Number(r.n || 0) + return out +} + +/** + * The subset of these hashes that is suppressed, as a Set. + * + * One query for a whole audience rather than one per recipient. Not used by the + * delivery path — that checks a single address at send time, after the delay + * window, which is the only check that can be correct — but by the admin reach + * preview, which is asked about thousands of users at once and must not become + * thousands of round trips. + */ +const suppressedAmong = async (addressHashes, channel = 'email') => { + const wanted = [...new Set((addressHashes || []).filter(Boolean))] + if (!wanted.length) return new Set() + const marks = wanted.map(() => '?').join(', ') + const rows = await query( + `SELECT address_hash FROM engagement_suppressions + WHERE channel = ? AND address_hash IN (${marks})`, + [channel, ...wanted], + ) + return new Set(rows.map((r) => r.address_hash)) +} + +module.exports = { get, add, remove, list, count, countsByReason, suppressedAmong } diff --git a/server/src/router/v1/admin/engagement.controller.js b/server/src/router/v1/admin/engagement.controller.js index 4f9e403..a90d201 100644 --- a/server/src/router/v1/admin/engagement.controller.js +++ b/server/src/router/v1/admin/engagement.controller.js @@ -32,6 +32,8 @@ const segments = require('../../../model/engagement/engagementSegments.model') const recipients = require('../../../model/engagement/engagementRecipients.db') const templates = require('../../../model/engagement/engagementTemplates.model') const sendsDb = require('../../../model/engagement/engagementSends.db') +const suppressionsDb = require('../../../model/engagement/engagementSuppressions.db') +const suppressions = require('../../../engagement/suppressions') // The lattice, flattened for a client: for each ceiling, the ones a rule may // choose under it. Served with the catalog rather than hardcoded in the admin @@ -245,6 +247,37 @@ exports.deleteSegment = async (req, res, next) => { // ── Reach preview ────────────────────────────────────────────────────────── +/** + * How many of a resolved audience would actually receive an email, and what + * removed the rest (Phase 9). + * + * The two mechanisms are asked in the order the engine applies them, and the + * order is what makes the numbers add up: the verification gate runs at enqueue, + * so a user it excludes is never checked for suppression, and counting both + * independently would double-count anybody who is unverified AND bounced. + * + * **It never returns an address.** Addresses are read only to hash them, and only + * the counts leave this function — the send log route already refuses to return + * `address_hash` for exactly this reason, and a reach preview that leaked a list + * would be the same hole through a different door. + */ +async function emailReach(userIds) { + const gated = await channels.eligibleFor('email', userIds) + const addresses = await recipients.addressesFor(gated.userIds) + const hashes = [...addresses.values()].map((a) => suppressions.hashAddress(a)) + const blocked = await suppressionsDb.suppressedAmong(hashes) + return { + // Everyone the audience resolved to who is not excluded by the gate and is + // not suppressed. Users with no address at all are already out: the gate + // drops them when it is on, and `addressesFor` does not return them when it + // is off, so they never reach the count either way. + deliverable: Math.max(0, addresses.size - blocked.size), + excluded: gated.excluded, + suppressed: blocked.size, + } +} + + /** * GET /api/v1/admin/engagement/audience-preview * @@ -270,6 +303,16 @@ exports.deleteSegment = async (req, res, next) => { * at all. Without it the editor shows a healthy count beside a save the server * will refuse, which reads as a bug in the save rather than as the G24 ceiling * doing its job. + * + * **A fourth arrived with Phase 9: `email`.** `count` is how many people the + * audience resolves to, and that has never been how many will get a mail — the + * verification gate drops unverified users at enqueue and the suppression list + * drops bounced addresses at send. An operator reading "3,000" beside a rule that + * will mail 1,796 people has been told something false by a screen whose only job + * is that number, so the breakdown is computed the same way the engine computes + * it: `channels.eligibleFor('email', ...)` is the identical call `applyRule` + * makes. It is reported for the email channel only because it is the only channel + * either mechanism applies to. */ exports.previewAudience = async (req, res, next) => { try { @@ -297,6 +340,7 @@ exports.previewAudience = async (req, res, next) => { dormant: resolved.dormant, reason: resolved.reason, permitted: triggerId && resolved.ceiling ? audiences.permitted(triggerId, resolved.ceiling) : null, + email: await emailReach(resolved.userIds), }) } catch (err) { next(err) @@ -444,3 +488,110 @@ exports.listSends = async (req, res, next) => { next(err) } } + +// ── Suppressions (Phase 9) ───────────────────────────────────────────────── +// +// The one surface that can take an address OUT of the suppression list, which is +// why it exists at all: a hard bounce is written by a background worker with no +// human in the loop, and without a way back a mistyped-then-corrected mailbox is +// silenced permanently. +// +// **The list returns `address_masked`, never `address_hash`.** The send log route +// above strips the hash for a stated reason — shipping a sha256 of every address +// on the deployment to a browser is an offline dictionary attack waiting to be +// run — and the same reasoning applies twice over here, where the rows are +// exactly the addresses somebody would most want to confirm. The mask is what an +// operator can act on and is not reversible. + +/** GET /api/v1/admin/engagement/suppressions */ +exports.listSuppressions = async (req, res, next) => { + try { + const limit = Math.min(Math.max(Number(req.query.limit) || 50, 1), 200) + const offset = Math.max(Number(req.query.offset) || 0, 0) + const reason = typeof req.query.reason === 'string' ? req.query.reason : null + if (reason && !suppressions.REASONS.includes(reason)) { + return res.status(400).json({ message: `reason must be one of ${suppressions.REASONS.join(', ')}` }) + } + const filters = { + reason, + channel: typeof req.query.channel === 'string' ? req.query.channel : null, + search: typeof req.query.search === 'string' ? req.query.search.trim() || null : null, + } + const [rows, total, byReason] = await Promise.all([ + suppressionsDb.list({ ...filters, limit, offset }), + suppressionsDb.count(filters), + // Unfiltered on purpose: it is the summary strip above the table, and a + // count that moved with the filter would say "0 bounces" while the operator + // was looking at the manual ones. + suppressionsDb.countsByReason(), + ]) + res.json({ + suppressions: rows.map(({ address_hash: _hash, ...row }) => row), + total, + limit, + offset, + byReason, + reasons: suppressions.REASONS, + }) + } catch (err) { + next(err) + } +} + +/** + * POST /api/v1/admin/engagement/suppressions + * + * Suppress an address by hand — the operator-side half of a bounce they were + * told about out of band (a person emailing to say "stop", a relay's dashboard). + * + * The reason is forced to `manual` rather than taken from the body. An admin + * typing an address is not evidence of a bounce or a complaint, and a list where + * `reason` sometimes means "the relay said so" and sometimes means "somebody + * chose this word" cannot be used to diagnose anything. + */ +exports.createSuppression = async (req, res, next) => { + try { + const address = typeof req.body?.address === 'string' ? req.body.address.trim() : '' + // The same shape check the rest of the codebase uses for an address, and no + // more: this is not a deliverability test, it is a guard against storing a + // hash of a typo that can never be matched or found again. + if (!address || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(address)) { + return res.status(400).json({ message: 'A valid email address is required' }) + } + const detail = typeof req.body?.detail === 'string' ? req.body.detail.slice(0, 500) : null + const created = await suppressions.suppress({ + address, + reason: 'manual', + detail, + createdBy: req.user?.id ?? null, + }) + // 200 rather than 409 for an address already on the list: the operator asked + // for it to be suppressed and it is, which is the outcome they wanted. `created` + // says which of the two happened, so the screen can say "already suppressed" + // without it reading as a failure. + res.status(created ? 201 : 200).json({ created, address: suppressions.maskAddress(address) }) + } catch (err) { + next(err) + } +} + +/** + * DELETE /api/v1/admin/engagement/suppressions + * + * Un-suppress. The address goes in the BODY rather than the path, and that is + * not a REST preference: a path parameter lands in the access log, the browser's + * history and any proxy in front of the deployment, and this one is a real + * address belonging to a real person. The hash cannot be used instead — the + * screen never receives one. + */ +exports.deleteSuppression = async (req, res, next) => { + try { + const address = typeof req.body?.address === 'string' ? req.body.address.trim() : '' + if (!address) return res.status(400).json({ message: 'An email address is required' }) + const removed = await suppressions.unsuppress(address, req.body?.channel || 'email') + if (!removed) return res.status(404).json({ message: 'That address is not suppressed' }) + res.json({ removed: true }) + } catch (err) { + next(err) + } +} diff --git a/server/src/router/v1/admin/engagement.router.js b/server/src/router/v1/admin/engagement.router.js index 0a5fd86..6ed6865 100644 --- a/server/src/router/v1/admin/engagement.router.js +++ b/server/src/router/v1/admin/engagement.router.js @@ -331,4 +331,54 @@ engagementRouter.get( controller.listSends, ) +// -- Suppressions (Phase 9) ------------------------------------------------ + +engagementRouter.get( + '/suppressions', + // #swagger.tags = ['Admin - Engagement'] + // #swagger.summary = 'Addresses this deployment has stopped mailing' + // #swagger.description = 'G16. Rows carry `address_masked` (`d***@example.com`) and never `address_hash` - the same rule the send log follows, and for the same reason: a sha256 of every address on the deployment, handed to a browser, is an offline dictionary attack. The mask keeps the domain intact so a whole-domain delivery failure is visible, and destroys the local part so the list cannot be turned back into an address book. `byReason` is deliberately unfiltered - it is the summary strip above the table.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + // #swagger.parameters['limit'] = { in: 'query', description: 'Page size, 1-200 (default 50)', required: false, schema: { type: 'integer' } } + // #swagger.parameters['offset'] = { in: 'query', description: 'Rows to skip', required: false, schema: { type: 'integer' } } + // #swagger.parameters['reason'] = { in: 'query', description: 'bounce, complaint, manual or unverified', required: false, schema: { type: 'string' } } + // #swagger.parameters['channel'] = { in: 'query', description: 'Only this channel (default: all)', required: false, schema: { type: 'string' } } + // #swagger.parameters['search'] = { in: 'query', description: 'Substring of the masked address - a domain is what this is for', required: false, schema: { type: 'string' } } + /* #swagger.responses[200] = { description: 'One page of the list, with per-reason totals', content: { "application/json": { schema: { type: "object", properties: { suppressions: { type: "array", items: { type: "object", additionalProperties: true } }, total: { type: "integer" }, limit: { type: "integer" }, offset: { type: "integer" }, byReason: { type: "object", additionalProperties: { type: "integer" } }, reasons: { type: "array", items: { type: "string" } } } } } } } */ + /* #swagger.responses[400] = { description: 'Unknown reason', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[403] = { description: 'Not an admin', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + adminOnly, + controller.listSuppressions, +) + +engagementRouter.post( + '/suppressions', + // #swagger.tags = ['Admin - Engagement'] + // #swagger.summary = 'Suppress an address by hand' + // #swagger.description = 'For a bounce or a complaint reported out of band. The reason is forced to `manual` rather than read from the body: an admin typing an address is not evidence of a bounce, and a `reason` column that sometimes means "the relay said so" and sometimes means "somebody chose this word" cannot diagnose anything. An address already on the list answers 200 with `created: false` rather than 409 - the operator asked for it to be suppressed and it is.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.requestBody = { required: true, content: { "application/json": { schema: { type: "object", properties: { address: { type: "string" }, detail: { type: "string", nullable: true } }, required: ["address"] } } } } */ + /* #swagger.responses[201] = { description: 'Suppressed', content: { "application/json": { schema: { type: "object", properties: { created: { type: "boolean" }, address: { type: "string", nullable: true } } } } } } */ + /* #swagger.responses[200] = { description: 'Already suppressed; nothing changed', content: { "application/json": { schema: { type: "object", properties: { created: { type: "boolean" }, address: { type: "string", nullable: true } } } } } } */ + /* #swagger.responses[400] = { description: 'Not a valid address', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[403] = { description: 'Not an admin', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + adminOnly, + controller.createSuppression, +) + +engagementRouter.delete( + '/suppressions', + // #swagger.tags = ['Admin - Engagement'] + // #swagger.summary = 'Lift a suppression' + // #swagger.description = 'The only way out of the list, and the reason the screen exists: a hard bounce is written by a background worker with no human in the loop, so a mistyped-then-corrected mailbox would otherwise be silenced permanently. The address goes in the BODY, not the path - a path parameter lands in the access log, the browser history and every proxy in front of the deployment, and this one belongs to a real person. The hash cannot be used instead because the screen is never given one.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.requestBody = { required: true, content: { "application/json": { schema: { type: "object", properties: { address: { type: "string" }, channel: { type: "string", nullable: true } }, required: ["address"] } } } } */ + /* #swagger.responses[200] = { description: 'Lifted', content: { "application/json": { schema: { type: "object", properties: { removed: { type: "boolean" } } } } } } */ + /* #swagger.responses[400] = { description: 'No address given', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[403] = { description: 'Not an admin', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[404] = { description: 'That address is not suppressed', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + adminOnly, + controller.deleteSuppression, +) + module.exports = engagementRouter diff --git a/server/src/utils/engagementWorker.js b/server/src/utils/engagementWorker.js index d486f13..25669e6 100644 --- a/server/src/utils/engagementWorker.js +++ b/server/src/utils/engagementWorker.js @@ -56,7 +56,7 @@ const STALE_MS = 15 * 60 * 1000 /** * Deliver one claimed row. * - * @returns {{ outcome: 'sent'|'retry'|'terminal', detail?: string, transport?: string, addressHash?: string }} + * @returns {{ outcome: 'sent'|'retry'|'terminal'|'suppressed'|'bounced', detail?: string, transport?: string, addressHash?: string }} */ async function deliver(row) { const channel = channels.get(row.channel) @@ -78,6 +78,21 @@ async function deliver(row) { if (result && result.ok) { return { outcome: 'sent', transport: result.transport, detail: result.detail, addressHash: hash } } + // Its own outcome rather than a flavour of 'terminal' (Phase 9). Both statuses + // the ENUMs already carried for it say something a 'failed' row cannot: the + // outbox row was not attempted, and the send log's `suppressed` is the + // difference between "we tried and the relay refused" and "we declined to + // try". An operator reading a screen of failures needs those separated, and + // so does anybody counting deliverability. + if (result && result.suppressed) { + return { outcome: 'suppressed', detail: result.detail || 'suppressed', addressHash: hash } + } + // A hard bounce. Terminal like any other refusal, but recorded under its own + // name: "the relay would not take this" and "this mailbox does not exist" + // send an operator to two different places. + if (result && result.bounced) { + return { outcome: 'bounced', detail: result.detail || 'hard bounce', addressHash: hash } + } if (result && result.retry) { return { outcome: 'retry', detail: result.detail || 'transient failure', addressHash: hash } } @@ -107,8 +122,18 @@ async function processRow(row, now = new Date(), deliverFn = deliver) { return 'retry' } - const status = result.outcome === 'sent' ? 'sent' : 'failed' - await outboxDb.finish(row.id, status, status === 'failed' ? result.detail : null) + // **The two tables diverge here, deliberately.** `engagement_outbox.status` is + // the ROW's lifecycle and its ENUM has no 'bounced' - from the queue's point of + // view a bounced message is a row that finished unsuccessfully, which is + // 'failed'. `engagement_sends.status` is what happened to the MESSAGE, and + // there 'bounced' is the whole point: it is the difference between "look at + // your relay" and "this person's mailbox is gone". + let status = 'failed' + if (result.outcome === 'sent') status = 'sent' + else if (result.outcome === 'suppressed') status = 'suppressed' + else if (result.outcome === 'bounced') status = 'bounced' + const outboxStatus = status === 'bounced' ? 'failed' : status + await outboxDb.finish(row.id, outboxStatus, status === 'sent' ? null : result.detail) // The send log is written for every terminal outcome, not only success. G15's // question is "did user X get the mail?", and "no, and here is why" is an // answer that table has to be able to give. @@ -142,7 +167,7 @@ async function tick(now = new Date()) { } if (!due || !due.length) return - const counts = { sent: 0, failed: 0, retry: 0, taken: 0 } + const counts = { sent: 0, failed: 0, suppressed: 0, bounced: 0, retry: 0, taken: 0 } for (const row of due) { try { const outcome = await processRow(row, now) diff --git a/server/src/utils/mailer.js b/server/src/utils/mailer.js index bdd3a5d..523ec5c 100644 --- a/server/src/utils/mailer.js +++ b/server/src/utils/mailer.js @@ -411,7 +411,17 @@ const PERMANENT_CODES = new Set([550, 553, 554, 'EENVELOPE', 'EAUTH']) * itself. A FAILURE is still recorded, because a relay that has started refusing * mail is precisely what that screen exists to show. * - * @returns {Promise<{ok: boolean, retry?: boolean, transport?: string, detail?: string}>} + * **`smtp` carries the refusal itself, and Phase 9 is why.** `retry` says whether + * to try again; `detail` is a sentence for a human. Neither can answer "was this + * the recipient's fault?", which is the question the suppression list turns on — + * `550 5.1.1` and `550 5.7.1` produce an identical `retry: false` and mean + * completely different things. So the reply code, the enhanced status and the + * error code ride back untouched for `bounceClassify` to read. Three scalar + * fields rather than the error object: an `Error` from nodemailer carries the + * whole failed message, envelope included, and this return value is logged. + * + * @returns {Promise<{ok: boolean, retry?: boolean, transport?: string, detail?: string, + * smtp?: {code: string|null, responseCode: number|null, response: string|null}}>} */ async function sendNotification({ to, rendered, unsubscribeUrl, unsubscribeApiUrl }) { const built = await buildTransport() @@ -445,7 +455,20 @@ async function sendNotification({ to, rendered, unsubscribeUrl, unsubscribeApiUr const code = err && (err.responseCode || err.code) log.warn('engagement send failed', { message: err.message }) await emailConfig.recordStatus({ status: 'error', statusDetail: detail }).catch(() => {}) - return { ok: false, retry: !PERMANENT_CODES.has(code), transport: config.transport, detail } + return { + ok: false, + retry: !PERMANENT_CODES.has(code), + transport: config.transport, + detail, + smtp: { + code: err && err.code ? String(err.code) : null, + responseCode: err && Number.isInteger(err.responseCode) ? err.responseCode : null, + // Truncated: a relay may answer with a multi-line essay, and this string + // reaches a 500-character log column. The reply code and the enhanced + // status are both at the front, which is where the classifier reads them. + response: err && err.response ? String(err.response).slice(0, 400) : null, + }, + } } } diff --git a/server/swagger/swagger-output.json b/server/swagger/swagger-output.json index 8e385d4..7da8651 100644 --- a/server/swagger/swagger-output.json +++ b/server/swagger/swagger-output.json @@ -2183,6 +2183,313 @@ ] } }, + "/api/v1/admin/engagement/suppressions": { + "get": { + "tags": [ + "Admin - Engagement" + ], + "summary": "Addresses this deployment has stopped mailing", + "description": "G16. Rows carry `address_masked` (`d***@example.com`) and never `address_hash` - the same rule the send log follows, and for the same reason: a sha256 of every address on the deployment, handed to a browser, is an offline dictionary attack. The mask keeps the domain intact so a whole-domain delivery failure is visible, and destroys the local part so the list cannot be turned back into an address book. `byReason` is deliberately unfiltered - it is the summary strip above the table.", + "parameters": [ + { + "name": "limit", + "in": "query", + "description": "Page size, 1-200 (default 50)", + "required": false, + "schema": { + "type": "integer" + } + }, + { + "name": "offset", + "in": "query", + "description": "Rows to skip", + "required": false, + "schema": { + "type": "integer" + } + }, + { + "name": "reason", + "in": "query", + "description": "bounce, complaint, manual or unverified", + "required": false, + "schema": { + "type": "string" + } + }, + { + "name": "channel", + "in": "query", + "description": "Only this channel (default: all)", + "required": false, + "schema": { + "type": "string" + } + }, + { + "name": "search", + "in": "query", + "description": "Substring of the masked address - a domain is what this is for", + "required": false, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "One page of the list, with per-reason totals", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "suppressions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": true + } + }, + "total": { + "type": "integer" + }, + "limit": { + "type": "integer" + }, + "offset": { + "type": "integer" + }, + "byReason": { + "type": "object", + "additionalProperties": { + "type": "integer" + } + }, + "reasons": { + "type": "array", + "items": { + "type": "string" + } + } + } + } + } + } + }, + "400": { + "description": "Unknown reason", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Not an admin", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ] + }, + "post": { + "tags": [ + "Admin - Engagement" + ], + "summary": "Suppress an address by hand", + "description": "For a bounce or a complaint reported out of band. The reason is forced to `manual` rather than read from the body: an admin typing an address is not evidence of a bounce, and a `reason` column that sometimes means \"the relay said so\" and sometimes means \"somebody chose this word\" cannot diagnose anything. An address already on the list answers 200 with `created: false` rather than 409 - the operator asked for it to be suppressed and it is.", + "responses": { + "200": { + "description": "Already suppressed; nothing changed", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "created": { + "type": "boolean" + }, + "address": { + "type": "string", + "nullable": true + } + } + } + } + } + }, + "201": { + "description": "Suppressed", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "created": { + "type": "boolean" + }, + "address": { + "type": "string", + "nullable": true + } + } + } + } + } + }, + "400": { + "description": "Not a valid address", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Not an admin", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "address": { + "type": "string" + }, + "detail": { + "type": "string", + "nullable": true + } + }, + "required": [ + "address" + ] + } + } + } + } + }, + "delete": { + "tags": [ + "Admin - Engagement" + ], + "summary": "Lift a suppression", + "description": "The only way out of the list, and the reason the screen exists: a hard bounce is written by a background worker with no human in the loop, so a mistyped-then-corrected mailbox would otherwise be silenced permanently. The address goes in the BODY, not the path - a path parameter lands in the access log, the browser history and every proxy in front of the deployment, and this one belongs to a real person. The hash cannot be used instead because the screen is never given one.", + "responses": { + "200": { + "description": "Lifted", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "removed": { + "type": "boolean" + } + } + } + } + } + }, + "400": { + "description": "No address given", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Not an admin", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "404": { + "description": "That address is not suppressed", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "address": { + "type": "string" + }, + "channel": { + "type": "string", + "nullable": true + } + }, + "required": [ + "address" + ] + } + } + } + } + } + }, "/api/v1/admin/engagement/templates": { "get": { "tags": [ diff --git a/server/test/engagementAdmin.test.js b/server/test/engagementAdmin.test.js index f0b5b56..2dd6dd0 100644 --- a/server/test/engagementAdmin.test.js +++ b/server/test/engagementAdmin.test.js @@ -40,6 +40,9 @@ const ctrl = require('../src/router/v1/admin/engagement.controller') const rulesDb = require('../src/model/engagement/engagementRules.db') const segmentsDb = require('../src/model/engagement/engagementSegments.db') const recipients = require('../src/model/engagement/engagementRecipients.db') +const suppressionsDb = require('../src/model/engagement/engagementSuppressions.db') +const suppressions = require('../src/engagement/suppressions') +const settings = require('../src/model/settings/settings.model') const db = require('../src/utils/db') after(() => db.close()) @@ -48,7 +51,13 @@ after(() => db.close()) let store const originals = {} -for (const [name, mod] of [['rulesDb', rulesDb], ['segmentsDb', segmentsDb], ['recipients', recipients]]) { +for (const [name, mod] of [ + ['rulesDb', rulesDb], + ['segmentsDb', segmentsDb], + ['recipients', recipients], + ['suppressionsDb', suppressionsDb], + ['settings', settings], +]) { originals[name] = { mod, fns: { ...mod } } } const restoreOriginals = () => { @@ -56,7 +65,16 @@ const restoreOriginals = () => { } function installStubs() { - store = { rules: new Map(), segments: new Map(), users: new Map(), nextRule: 1, nextSegment: 1 } + store = { + rules: new Map(), + segments: new Map(), + users: new Map(), + // Phase 9: address hashes, and the verification gate's setting. + suppressed: new Set(), + verificationRequired: false, + nextRule: 1, + nextSegment: 1, + } rulesDb.list = async () => [...store.rules.values()].map((r) => ({ ...r })) rulesDb.getById = async (id) => (store.rules.has(id) ? { ...store.rules.get(id) } : null) @@ -102,11 +120,40 @@ function installStubs() { recipients.subscribers = async () => [] recipients.filterActive = async (ids) => [...new Set(ids)].filter((id) => store.users.get(id)?.status === 'active') + + // Phase 9: the reach preview now asks the email channel what it would actually + // deliver, so the fake world has to be able to answer the two questions that + // makes it ask - who is unverified, and who is suppressed. + recipients.unverifiedAmong = async (ids) => + new Set(ids.filter((id) => store.users.get(Number(id))?.email_verified === 0)) + recipients.addressesFor = async (ids) => + new Map( + ids + .filter((id) => store.users.get(Number(id))?.status === 'active') + .map((id) => [Number(id), store.users.get(Number(id)).email]), + ) + suppressionsDb.suppressedAmong = async (hashes) => + new Set(hashes.filter((h) => store.suppressed.has(h))) + settings.isEmailVerificationRequired = async () => store.verificationRequired } // ── Fixtures ─────────────────────────────────────────────────────────────── -const addUser = (id, over = {}) => store.users.set(id, { id, role: 'player', status: 'active', ...over }) +const addUser = (id, over = {}) => + store.users.set(id, { + id, + role: 'player', + status: 'active', + // Verified with an address by default: the reach preview counts deliverable + // people, and a fixture that was unverified by accident would make every + // count in this file read as zero. + email: `u${id}@example.test`, + email_verified: 1, + ...over, + }) + +const suppressUser = (id) => + store.suppressed.add(suppressions.hashAddress(store.users.get(id).email)) function register(owner, fn) { const api = registries.stage(owner) @@ -438,6 +485,51 @@ test('an `owner` audience previews as 0 with the reason, because it resolves per assert.equal(res.body.permitted, true) }) +// Phase 9. `count` has never been how many people get a mail, and after this +// phase there are two mechanisms that make the gap real. An operator reading +// "3,000" beside a rule that will mail 1,796 people has been told something false +// by the one screen whose entire job is that number. +test('the preview says how many would actually be mailed, not just how many resolved', async () => { + addUser(1) + addUser(2) + addUser(3) + suppressUser(2) + + const res = await call(ctrl.previewAudience, { query: { audience: 'authenticated' } }) + + assert.equal(res.body.count, 3) + assert.equal(res.body.email.deliverable, 2) + assert.equal(res.body.email.suppressed, 1) +}) + +test('with the verification gate on, the preview counts the exclusion rather than hiding it', async () => { + addUser(1) + addUser(2, { email_verified: 0 }) + store.verificationRequired = true + + const res = await call(ctrl.previewAudience, { query: { audience: 'authenticated' } }) + + assert.equal(res.body.count, 2) + assert.deepEqual(res.body.email.excluded, { unverified: 1 }) + assert.equal(res.body.email.deliverable, 1) +}) + +// The two mechanisms are asked in the order the engine applies them, so somebody +// who is both unverified and suppressed is removed once. Counting them +// independently would report more exclusions than there are people. +test('a user who is both unverified and suppressed is not counted twice', async () => { + addUser(1) + addUser(2, { email_verified: 0 }) + suppressUser(2) + store.verificationRequired = true + + const res = await call(ctrl.previewAudience, { query: { audience: 'authenticated' } }) + + assert.equal(res.body.email.deliverable, 1) + assert.deepEqual(res.body.email.excluded, { unverified: 1 }) + assert.equal(res.body.email.suppressed, 0, 'the gate already removed them') +}) + test('the preview reports when the trigger ceiling would refuse what it just counted', async () => { addUser(1, { role: 'admin' }) diff --git a/server/test/engagementDeliverability.test.js b/server/test/engagementDeliverability.test.js new file mode 100644 index 0000000..67e8596 --- /dev/null +++ b/server/test/engagementDeliverability.test.js @@ -0,0 +1,423 @@ +// ── Deliverability: suppression and bounces (ENGAGEMENT.md Phase 9) ──────── +// +// The phase's acceptance criteria, plus the two things building it showed were +// worth pinning because getting either wrong is silent: +// +// • **the classifier is not `PERMANENT_CODES`** — an EAUTH or a 5.7.1 policy +// refusal must not suppress anybody. Reusing that set would have meant one +// stale SMTP password suppressing every address the worker touched, and +// nothing would have said so. +// • **the verification gate excludes at ENQUEUE, on the email channel only** — +// a rule spanning email and in-app must still reach an unverified user's +// inbox, which is the failure a shared audience filter would have shipped. +// +// Point the DB at a closed port before requiring anything: the registries reach +// utils/discordAnnounce, which builds the pool at require time. +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, beforeEach, afterEach, after } = require('node:test') +const assert = require('node:assert/strict') + +const registries = require('../src/modules/registries') +const channels = require('../src/engagement/channels') +const engine = require('../src/engagement/engine') +const emailChannel = require('../src/engagement/emailChannel') +const bounceClassify = require('../src/engagement/bounceClassify') +const suppressions = require('../src/engagement/suppressions') +const templates = require('../src/engagement/templates') +const audiences = require('../src/engagement/audiences') +const worker = require('../src/utils/engagementWorker') +const mailer = require('../src/utils/mailer') +const rulesDb = require('../src/model/engagement/engagementRules.db') +const recipients = require('../src/model/engagement/engagementRecipients.db') +const suppressionsDb = require('../src/model/engagement/engagementSuppressions.db') +const settings = require('../src/model/settings/settings.model') +const db = require('../src/utils/db') + +require('../src/engagement') +registries.registerCore() + +after(() => db.close()) + +const saved = new Map() +function patch(mod, name, fn) { + if (!saved.has(mod)) saved.set(mod, new Map()) + if (!saved.get(mod).has(name)) saved.get(mod).set(name, mod[name]) + mod[name] = fn +} +function restore() { + for (const [mod, names] of saved) for (const [name, fn] of names) mod[name] = fn + saved.clear() +} + +const TRIGGER = 'team.forum.post' + +let world + +beforeEach(() => { + world = { + mails: [], + // address_hash → row + suppressed: new Map(), + verificationRequired: false, + unverified: new Set(), + sendResult: { ok: true, transport: 'smtp' }, + } + + patch(mailer, 'sendNotification', async (msg) => { + world.mails.push(msg) + return world.sendResult + }) + patch(templates, 'renderByKey', async (key) => ({ + subject: `[${key}]`, + html: '

x

', + text: 'x', + missing: [], + values: {}, + })) + patch(recipients, 'addressFor', async (userId) => ({ address: `u${userId}@example.test` })) + patch(recipients, 'filterActive', async (ids) => ids) + patch(recipients, 'storedModes', async () => new Map()) + patch(recipients, 'unverifiedAmong', async (ids) => + new Set(ids.map(Number).filter((id) => world.unverified.has(id)))) + patch(recipients, 'addressesFor', async (ids) => + new Map(ids.map((id) => [Number(id), `u${id}@example.test`]))) + patch(rulesDb, 'getById', async () => ({ id: 1, trigger_id: TRIGGER, template_keys: {} })) + patch(settings, 'isEmailVerificationRequired', async () => world.verificationRequired) + + patch(suppressionsDb, 'get', async (hash) => world.suppressed.get(hash) || null) + patch(suppressionsDb, 'add', async (entry) => { + if (world.suppressed.has(entry.address_hash)) return false + world.suppressed.set(entry.address_hash, entry) + return true + }) + patch(suppressionsDb, 'remove', async (hash) => world.suppressed.delete(hash)) + patch(suppressionsDb, 'suppressedAmong', async (hashes) => + new Set(hashes.filter((h) => world.suppressed.has(h)))) +}) +afterEach(restore) + +const outboxRow = (over = {}) => ({ + id: 1, + rule_id: 1, + trigger_id: TRIGGER, + user_id: 11, + channel: 'email', + subject_key: 'The Silver Hand', + scope_key: 'team:1', + payload: {}, + attempts: 0, + ...over, +}) + +const suppress = (address, reason = 'bounce') => + suppressions.suppress({ address, reason, detail: 'seeded by the test' }) + +// ── The classifier: what counts as the recipient's fault ─────────────────── +// +// The single most important group in this file. Everything below it assumes +// `classify` is right about which failures are about a person. + +test('a 5.1.1 is a hard bounce', () => { + const v = bounceClassify.classify({ + responseCode: 550, + response: '550 5.1.1 : Recipient address rejected: User unknown', + }) + assert.equal(v.suppress, true) + assert.equal(v.reason, 'bounce') +}) + +// The bug this whole file exists to prevent. `mailer.PERMANENT_CODES` contains +// EAUTH, so "terminal failure → suppress" would have emptied the mailing list +// the first time an operator's SMTP password expired — with a clean send log and +// no warning anywhere. +test('an authentication failure suppresses nobody', () => { + const v = bounceClassify.classify({ code: 'EAUTH', message: 'Invalid login: 535 5.7.8' }) + assert.equal(v.suppress, false) + assert.match(v.reason, /not a recipient failure/) +}) + +test('a policy refusal suppresses nobody — 5.7.x is about us, not the address', () => { + const v = bounceClassify.classify({ responseCode: 554, response: '554 5.7.1 Message rejected by policy' }) + assert.equal(v.suppress, false) +}) + +test('a full mailbox is not a dead one, with or without an enhanced code', () => { + assert.equal(bounceClassify.classify({ responseCode: 552, response: '552 5.2.2 Mailbox full' }).suppress, false) + // The veto list earning its place: "mailbox unavailable" is in the + // no-such-mailbox phrases, and this sentence contains it. + assert.equal( + bounceClassify.classify({ responseCode: 550, response: '550 Mailbox unavailable: mailbox is full' }).suppress, + false, + ) +}) + +test('a temporary failure suppresses nobody', () => { + assert.equal( + bounceClassify.classify({ responseCode: 451, response: '451 4.7.1 Greylisted, try again later' }).suppress, + false, + ) +}) + +test('a bare 550 with an unambiguous reason still bounces', () => { + assert.equal(bounceClassify.classify({ responseCode: 550, response: '550 No such user here' }).suppress, true) +}) + +test('a bare 554 does not, because "transaction failed" means nothing in particular', () => { + assert.equal(bounceClassify.classify({ responseCode: 554, response: '554 Transaction failed' }).suppress, false) +}) + +// A refusal this file has no opinion about must be a refusal, not a guess. The +// asymmetry is deliberate: mailing a dead address again costs a retry, and +// suppressing a live one costs a person who silently stops hearing from us. +test('an unrecognised permanent code is not suppressed', () => { + const v = bounceClassify.classify({ responseCode: 550, response: '550 5.4.1 Access denied' }) + assert.equal(v.suppress, false) + assert.match(v.reason, /not a known recipient failure/) +}) + +// A bounce that quotes another relay's answer carries two enhanced codes. The +// one that matters is the one THIS relay gave us, which is the one at the front. +test('the enhanced code is read from the front of the response, not found anywhere in it', () => { + const v = bounceClassify.classify({ + responseCode: 554, + response: '554 5.7.1 rejected; remote host said: 550 5.1.1 user unknown', + }) + assert.equal(v.suppress, false) +}) + +// ── The address key ──────────────────────────────────────────────────────── + +test('the hash is case- and whitespace-folded, so a bounce finds the row it belongs to', () => { + assert.equal(suppressions.hashAddress(' Darrow@Example.TEST '), suppressions.hashAddress('darrow@example.test')) +}) + +test('the mask keeps the domain and destroys the local part', () => { + assert.equal(suppressions.maskAddress('darrow@example.test'), 'd***@example.test') + // Two characters is most of a two-character local part, so nothing is kept. + assert.equal(suppressions.maskAddress('ab@example.test'), '***@example.test') + assert.equal(suppressions.maskAddress('not-an-address'), null) +}) + +// ── The acceptance criteria ──────────────────────────────────────────────── + +test('a suppressed address is skipped with no transport call at all', async () => { + await suppress('u11@example.test') + const result = await emailChannel.deliver(outboxRow()) + assert.equal(result.ok, false) + assert.equal(result.suppressed, true) + assert.equal(world.mails.length, 0, 'the transport must not be called') + assert.match(result.detail, /suppressed \(bounce\)/) +}) + +test("the worker writes that as status='suppressed' in both tables, not as a failure", async () => { + const finished = [] + const logged = [] + const outboxDb = require('../src/model/engagement/engagementOutbox.db') + const sendsDb = require('../src/model/engagement/engagementSends.db') + patch(outboxDb, 'claim', async () => true) + patch(outboxDb, 'finish', async (id, status, detail) => finished.push({ id, status, detail })) + patch(sendsDb, 'record', async (entry) => logged.push(entry)) + + await suppress('u11@example.test') + const status = await worker.processRow(outboxRow()) + + assert.equal(status, 'suppressed') + assert.equal(finished[0].status, 'suppressed') + assert.equal(logged[0].status, 'suppressed') + // The correlation key rides along even here: it is what a later manual audit + // matches a relay's own bounce report against. + assert.match(logged[0].address_hash, /^[0-9a-f]{64}$/) +}) + +test('a hard bounce suppresses the address, and the next send never reaches the relay', async () => { + world.sendResult = { + ok: false, + retry: false, + transport: 'smtp', + detail: 'the relay refused the message', + smtp: { code: 'EENVELOPE', responseCode: 550, response: '550 5.1.1 User unknown' }, + } + const first = await emailChannel.deliver(outboxRow()) + assert.equal(first.ok, false) + assert.equal(world.mails.length, 1, 'the first attempt does reach the transport') + assert.match(first.detail, /hard bounce: 5\.1\.1/) + + world.sendResult = { ok: true, transport: 'smtp' } + const second = await emailChannel.deliver(outboxRow()) + assert.equal(second.suppressed, true) + assert.equal(world.mails.length, 1, 'the second attempt does not') +}) + +// Without this a genuine dead mailbox could be retried four more times, because +// PERMANENT_CODES does not contain every reply code that can carry a 5.1.x — +// each retry another refusal on our record with the relay. +// `engagement_sends.status` has carried 'bounced' since §4.5 and nothing wrote +// it until this phase, so the Send Log's "Bounced" filter matched nothing. The +// live rig is what showed that; this is what keeps it fixed. The outbox row is +// still 'failed' — that ENUM has no 'bounced', and from the queue's point of +// view a bounced row is simply one that finished unsuccessfully. +test("a hard bounce is logged as 'bounced', while the outbox row is 'failed'", async () => { + const finished = [] + const logged = [] + const outboxDb = require('../src/model/engagement/engagementOutbox.db') + const sendsDb = require('../src/model/engagement/engagementSends.db') + patch(outboxDb, 'claim', async () => true) + patch(outboxDb, 'finish', async (id, status) => finished.push(status)) + patch(sendsDb, 'record', async (entry) => logged.push(entry)) + + world.sendResult = { + ok: false, + retry: true, + transport: 'smtp', + detail: 'refused', + smtp: { code: 'EENVELOPE', responseCode: 550, response: '550 5.1.1 User unknown' }, + } + const status = await worker.processRow(outboxRow()) + + assert.equal(status, 'bounced') + assert.equal(logged[0].status, 'bounced') + assert.equal(finished[0], 'failed') +}) + +test('a bounce is terminal even when the mailer called the failure retryable', async () => { + world.sendResult = { + ok: false, + retry: true, + transport: 'smtp', + detail: 'refused', + smtp: { code: null, responseCode: 550, response: '550 5.1.1 User unknown' }, + } + const result = await emailChannel.deliver(outboxRow()) + assert.equal(result.retry, false) +}) + +test('a failure that is NOT a bounce stays retryable and suppresses nobody', async () => { + world.sendResult = { + ok: false, + retry: true, + transport: 'smtp', + detail: 'connection refused', + smtp: { code: 'ECONNECTION', responseCode: null, response: null }, + } + const result = await emailChannel.deliver(outboxRow()) + assert.equal(result.retry, true) + assert.equal(world.suppressed.size, 0) + // The send log says why it was not suppressed, so the non-event is explained + // rather than merely absent. + assert.match(result.detail, /not suppressed/) +}) + +// The suppression list must never be able to stop mail going out. A database +// that cannot answer "is this suppressed" is a database problem, and turning it +// into total silence with a clean send log is G22's shape all over again. +test('a suppression check that throws sends the mail anyway', async () => { + patch(suppressionsDb, 'get', async () => { throw new Error('db is down') }) + const result = await emailChannel.deliver(outboxRow()) + assert.equal(result.ok, true) + assert.equal(world.mails.length, 1) +}) + +test('the first reason an address was suppressed is the one that survives', async () => { + await suppress('u11@example.test', 'bounce') + await suppressions.suppress({ address: 'u11@example.test', reason: 'manual' }) + const row = world.suppressed.get(suppressions.hashAddress('u11@example.test')) + assert.equal(row.reason, 'bounce') +}) + +test('un-suppressing is the way back, and it is the only one', async () => { + await suppress('u11@example.test') + assert.equal(await suppressions.unsuppress('u11@example.test'), true) + const result = await emailChannel.deliver(outboxRow()) + assert.equal(result.ok, true) +}) + +// ── The verification gate (§7.1 Q1's narrower half) ──────────────────────── + +test('with the gate off, an unverified address is mailed', async () => { + world.unverified.add(11) + const gated = await channels.eligibleFor('email', [11, 12]) + assert.deepEqual(gated.userIds, [11, 12]) + assert.deepEqual(gated.excluded, {}) +}) + +test('with the gate on, an unverified user is excluded and counted', async () => { + world.verificationRequired = true + world.unverified.add(11) + const gated = await channels.eligibleFor('email', [11, 12]) + assert.deepEqual(gated.userIds, [12]) + assert.deepEqual(gated.excluded, { unverified: 1 }) +}) + +// The reason the gate is a CHANNEL hook rather than a filter on the shared +// audience. A rule spanning both channels must still put an item in an +// unverified user's inbox — being unverified is a reason not to mail somebody +// and no reason at all to hide their notifications from them. +test('the gate is email-only: in-app still reaches an unverified user', async () => { + world.verificationRequired = true + world.unverified.add(11) + const inapp = await channels.eligibleFor('inapp', [11, 12]) + assert.deepEqual(inapp.userIds, [11, 12]) + const push = await channels.eligibleFor('push', [11, 12]) + assert.deepEqual(push.userIds, [11, 12]) +}) + +// The gate must not be able to stop mail by failing, and the blast radius is +// wider than one recipient: `applyRule` awaits this before the per-user loop, so +// a throw would abandon the whole rule for every channel it names — a rule that +// silently sent nothing, with a clean send log and an empty outbox. +test('a gate that cannot be read is off, for every recipient and every channel', async () => { + patch(settings, 'isEmailVerificationRequired', async () => { throw new Error('db is down') }) + const gated = await channels.eligibleFor('email', [11, 12]) + assert.deepEqual(gated.userIds, [11, 12]) + assert.deepEqual(gated.excluded, {}) +}) + +test('and so is a gate whose user lookup fails', async () => { + world.verificationRequired = true + patch(recipients, 'unverifiedAmong', async () => { throw new Error('db is down') }) + const gated = await channels.eligibleFor('email', [11, 12]) + assert.deepEqual(gated.userIds, [11, 12]) +}) + +// ── The engine end to end ────────────────────────────────────────────────── + +test('the engine writes no outbox row for an excluded user, and counts the exclusion', async () => { + const enqueued = [] + const outboxDb = require('../src/model/engagement/engagementOutbox.db') + const cooldownsDb = require('../src/model/engagement/engagementCooldowns.db') + const sendsDb = require('../src/model/engagement/engagementSends.db') + patch(outboxDb, 'enqueue', async (row) => { enqueued.push(row); return enqueued.length }) + patch(cooldownsDb, 'claim', async () => true) + patch(sendsDb, 'countSentSince', async () => 0) + patch(audiences, 'resolveForRule', async () => ({ + userIds: [11, 12], + ceiling: 'authenticated', + dormant: false, + })) + patch(audiences, 'permitted', () => true) + // Both channels default to a mode that enqueues only where the user opted in; + // email is opt-in, so say so for both users. + patch(recipients, 'storedModes', async (ids) => new Map(ids.map((id) => [Number(id), 'instant']))) + + world.verificationRequired = true + world.unverified.add(11) + + const rule = { + id: 1, + trigger_id: TRIGGER, + channels: ['email', 'inapp'], + max_sends_per_hour: 100, + cooldown_seconds: 0, + delay_seconds: 0, + conditions: null, + } + const summary = await engine.applyRule(rule, { triggerId: TRIGGER, data: {}, subject: 's' }, new Date()) + + assert.deepEqual(summary.ineligible, { unverified: 1 }) + const emailRows = enqueued.filter((r) => r.channel === 'email').map((r) => r.user_id) + const inappRows = enqueued.filter((r) => r.channel === 'inapp').map((r) => r.user_id) + assert.deepEqual(emailRows, [12], 'the unverified user gets no mail') + assert.deepEqual(inappRows.sort(), [11, 12], 'and still gets an inbox item') +})