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') +})