From 47c8b37d457d917ab5bd4fb760560fbd51d5c34e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 20:41:43 -0500 Subject: [PATCH] feat(email): remove Gmail OAuth2, put SMTP behind a transport registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engagement Phase 1 (docs/website/ENGAGEMENT.md §1.2a, §3.1, §3.2). A subtraction and a replacement in one commit, because leaving the OAuth2 flow half-wired across a release is worse than either end state. Deleted, per the §1.2a inventory: GET /admin/email/connect/start and /connect/callback, the connectStart/connectCallback controllers with the email_oauth_tx signed cookie, the PKCE verifier and CSRF nonce plumbing, the https://mail.google.com/ scope, the borrowed `google` auth-providers client, the OAuth2 nodemailer transport with its smtp.gmail.com:465 literals, the refresh-token decrypt in the model, and the client's Connect Gmail button, redirect banner and six Gmail error strings. `provider` and `refresh_token_enc` stay as columns under the additive-only discipline, unread. Added: a mail transport registry (server/src/engagement/transports) with `smtp` as the sole registration. `credentialFields` is the single declaration the admin form renders, the sanitizer filters against, and the "is it secret" answer comes from, so adding a transport is a registration rather than four edits. email_config gains transport / credential_enc (one encrypted JSON blob, since the field list is the transport's to declare) / reply_to. All six call sites keep their exact failure contracts: the contact form's mailto fallback, the invite's copyable link, the reset's generic 200, and sendTeamNotification's never-throws. One deliberate behaviour change: `enabled` now gates every sender rather than only isConfigured() — the connect flow used to set it as a side effect, and with a credential form the toggle has to mean what it says. Send-test becomes the real verification. Under OAuth2 the sender came back from Google and was guaranteed to belong to the credential; operator-typed, it can be refused, so failures name the sender and the SPF/DMARC reason (§1.2a consequence 2). G22, the silent degradation: an upgraded deployment backfills to smtp with no credentials and every sink politely does nothing. The admin dashboard now warns when the deprecated Gmail token is present and no replacement credential is, so the one deployment this happens to is told. A fresh install has never had mail and is not nagged. Guardrails: new `npm run check:hosts` (§3.2 rule 4) with its own self-test, wired into pr-checks before the install; routes.manifest and routes.guards regenerated (-2 routes). Co-Authored-By: Claude --- .gitea/workflows/pr-checks.yml | 6 + client/src/api/client.js | 5 +- client/src/routes/admin/views/Dashboard.jsx | 28 ++ .../src/routes/admin/views/EmailDelivery.jsx | 292 +++++++++++------- package.json | 3 +- scripts/checkNoExternalHosts.js | 185 +++++++++++ server/db/schema.sql | 55 +++- server/routes.guards.json | 20 +- server/routes.manifest.json | 8 - server/src/engagement/index.js | 22 ++ server/src/engagement/transports/index.js | 195 ++++++++++++ server/src/engagement/transports/smtp.js | 96 ++++++ .../src/model/emailConfig/emailConfig.db.js | 3 +- .../model/emailConfig/emailConfig.model.js | 106 +++++-- server/src/model/teams/teamNotify.model.js | 4 +- .../src/router/v1/admin/admin.controller.js | 35 +++ .../src/router/v1/admin/dashboard.router.js | 4 +- server/src/router/v1/admin/email.router.js | 58 ++-- .../router/v1/admin/emailConfig.controller.js | 217 ++++--------- server/src/server.js | 2 +- server/src/utils/mailer.js | 149 ++++++--- server/src/utils/teamNotify.js | 8 +- server/test/checkNoExternalHosts.test.js | 98 ++++++ server/test/emailConfig.model.test.js | 115 +++++-- server/test/mailTransports.test.js | 109 +++++++ server/test/mailer.test.js | 173 +++++++++-- 26 files changed, 1535 insertions(+), 461 deletions(-) create mode 100644 scripts/checkNoExternalHosts.js create mode 100644 server/src/engagement/index.js create mode 100644 server/src/engagement/transports/index.js create mode 100644 server/src/engagement/transports/smtp.js create mode 100644 server/test/checkNoExternalHosts.test.js create mode 100644 server/test/mailTransports.test.js diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index add43e5..4188ea9 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -56,6 +56,12 @@ jobs: # something found under a pile of unrelated failures, and it costs # nothing when it passes. run: npm run check:modules + - name: Check the engagement subsystem names no external host + # ENGAGEMENT.md §3.2 rule 4 — no transport may ship a default host, + # endpoint or sender. Dependency-free and runs before the install for the + # same reason as the check above: a phone-home is a design break, not a + # test failure, and it should be the first thing a reviewer sees. + run: npm run check:hosts - name: Install server deps run: npm ci --prefix server - name: Run server tests diff --git a/client/src/api/client.js b/client/src/api/client.js index 1da6829..9a97898 100644 --- a/client/src/api/client.js +++ b/client/src/api/client.js @@ -455,10 +455,11 @@ export const api = { getDiscordBotConfig: () => req('/admin/discord-bot/config'), saveDiscordBotConfig: (data) => req('/admin/discord-bot/config', { method: 'PUT', body: data }), - // ----- Email delivery / Gmail OAuth2 (admin only) ----- + // ----- Email delivery (admin only) ----- + // The connect-flow call went with Gmail OAuth2 (ENGAGEMENT.md §1.2a); the + // config response now carries the transport catalog the form renders from. getEmailConfig: () => req('/admin/email/config'), saveEmailConfig: (data) => req('/admin/email/config', { method: 'PUT', body: data }), - emailConnectUrl: () => req('/admin/email/connect/start'), testEmail: (to) => req('/admin/email/test', { method: 'POST', body: { to } }), disconnectEmail: () => req('/admin/email/disconnect', { method: 'POST' }), }, diff --git a/client/src/routes/admin/views/Dashboard.jsx b/client/src/routes/admin/views/Dashboard.jsx index 2ea25da..9a2ca63 100644 --- a/client/src/routes/admin/views/Dashboard.jsx +++ b/client/src/routes/admin/views/Dashboard.jsx @@ -66,6 +66,34 @@ export default function Dashboard() { return (
+ {/* Operator warnings: things that are quietly not working and would + otherwise be discovered by someone not receiving an email. The list is + normally empty, which is why it sits above the fold rather than in a + panel — see ENGAGEMENT.md §1.2a (G22). */} + {(dash.warnings || []).map((w) => ( +
+ {w.message} + {w.href && ( + <> + {' '} + Open settings + + )} +
+ ))}
the callback may redirect with. -const ERROR_TEXT = { - denied: 'Google sign-in was cancelled or denied.', - bad_state: 'The connect session expired. Please try again.', - no_client: 'The Google OAuth client is not configured.', - no_refresh_token: - 'Google did not return a refresh token. Remove this app under your Google Account → Security → Third-party access, then reconnect.', - no_email: 'Could not read the Gmail address from Google.', - error: 'Could not connect the Gmail account. Please try again.', -} - function StatusPanel({ config }) { const color = STATUS_COLOR[config.status] || 'var(--muted)' return ( @@ -52,60 +50,100 @@ function StatusPanel({ config }) { ) } +// One declared credential field. A `secret` already held renders empty with a +// "leave blank to keep" hint, matching the server's patch semantics: an empty +// secret is omitted from the save, not written as a blank. +function CredentialField({ field, value, isSet, onChange }) { + const hint = [field.help, field.kind === 'secret' && isSet ? 'Currently set — leave blank to keep it.' : null] + .filter(Boolean) + .join(' ') + + if (field.kind === 'boolean') { + return ( + + ) + } + + return ( + + ) +} + export default function EmailDelivery() { const { siteTitle } = useSite() const [config, setConfig] = useState(null) const [error, setError] = useState('') + const [transport, setTransport] = useState('smtp') + const [senderEmail, setSenderEmail] = useState('') const [senderName, setSenderName] = useState('') + const [replyTo, setReplyTo] = useState('') + const [credential, setCredential] = useState({}) const [enabled, setEnabled] = useState(false) const [busy, setBusy] = useState('') const [msg, setMsg] = useState('') const [actionError, setActionError] = useState('') - const [banner, setBanner] = useState(null) // fields kind ('ok' or 'err') and text + + // Seed the credential inputs from the non-secret values the server returned, + // falling back to each field's declared default. Secrets are never seeded — + // the server does not send them and an empty box means "keep what you have". + const seedCredential = useCallback((c, transportId) => { + const def = (c.transports || []).find((t) => t.id === transportId) + const next = {} + for (const f of def?.credentialFields || []) { + if (f.kind === 'secret') continue + next[f.key] = c.credential?.[f.key] ?? (f.default === null ? '' : f.default) + } + return next + }, []) const load = useCallback(async (seedForm = false) => { try { const c = await api.admin.getEmailConfig() setConfig(c) if (seedForm) { + setTransport(c.transport || 'smtp') + setSenderEmail(c.senderEmail || '') setSenderName(c.senderName || '') + setReplyTo(c.replyTo || '') setEnabled(c.enabled) + setCredential(seedCredential(c, c.transport || 'smtp')) } return c } catch { setError('Could not load email settings.') return null } - }, []) + }, [seedCredential]) - // On mount, surface the outcome of a just-completed connect redirect, strip the - // query params so a refresh doesn't replay the banner, then load config. useEffect(() => { - const params = new URLSearchParams(window.location.search) - if (params.has('email_connected')) { - setBanner({ kind: 'ok', text: 'Gmail account connected.' }) - } else if (params.has('email_error')) { - setBanner({ kind: 'err', text: ERROR_TEXT[params.get('email_error')] || 'Could not connect email.' }) - } - if (params.has('email_connected') || params.has('email_error')) { - params.delete('email_connected') - params.delete('email_error') - const qs = params.toString() - window.history.replaceState({}, '', window.location.pathname + (qs ? `?${qs}` : '')) - } load(true) }, [load]) - async function connect() { - setBusy('connect') - setActionError('') - try { - const { url } = await api.admin.emailConnectUrl() - window.location.href = url - } catch (err) { - setActionError(err.message || 'Could not start the connect flow.') - setBusy('') - } + // Switching transport starts from the new one's declared defaults, because the + // server does the same: a credential blob is never carried across transports. + function changeTransport(id) { + setTransport(id) + setCredential(seedCredential(config, id)) } async function save() { @@ -113,10 +151,19 @@ export default function EmailDelivery() { setMsg('') setActionError('') try { - const saved = await api.admin.saveEmailConfig({ senderName, enabled }) + const saved = await api.admin.saveEmailConfig({ transport, senderEmail, senderName, replyTo, credential, enabled }) setConfig(saved) + setEnabled(saved.enabled) + setCredential(seedCredential(saved, saved.transport)) setMsg('Saved.') } catch (err) { + // A refused enable comes back with the reverted config attached, so the + // screen shows what is actually stored rather than the state that was + // rejected. + if (err.body?.config) { + setConfig(err.body.config) + setEnabled(err.body.config.enabled) + } setActionError(err.message || 'Could not save.') } finally { setBusy('') @@ -133,12 +180,13 @@ export default function EmailDelivery() { await load() } catch (err) { setActionError(err.message || 'Could not send the test email.') + await load() } finally { setBusy('') } } - async function disconnect() { + async function clearCredentials() { setBusy('disconnect') setMsg('') setActionError('') @@ -146,9 +194,11 @@ export default function EmailDelivery() { const c = await api.admin.disconnectEmail() setConfig(c) setEnabled(false) - setMsg('Disconnected.') + setSenderEmail('') + setCredential(seedCredential(c, c.transport)) + setMsg('Credentials cleared.') } catch (err) { - setActionError(err.message || 'Could not disconnect.') + setActionError(err.message || 'Could not clear the credentials.') } finally { setBusy('') } @@ -157,84 +207,118 @@ export default function EmailDelivery() { if (error) return

{error}

if (!config) return null - const connected = config.hasRefreshToken + const catalog = config.transports || [] + const selected = catalog.find((t) => t.id === transport) return (

Email delivery

- Sends the contact form through Gmail over OAuth2, delivered to the - Contact email above. Reuses the Google authentication - client — configure that on the Authentication page first. + Sends the contact form, invitations, password resets and team + notifications. Contact-form mail is delivered to the + Contact email above. Credentials are stored encrypted + and never shown again.

- {banner && ( + {config.hadLegacyConnection && !config.hasCredential && (
- {banner.text} + This deployment was connected with the old Gmail sign-in, which has been + removed. No mail is being sent. Enter SMTP credentials + below to restore it — for Gmail, use smtp.gmail.com port 587 + with an app password.
)} - {!config.googleConfigured && ( -

- The Google authentication provider needs a client ID and secret before - you can connect a Gmail account. -

+ {catalog.length > 1 && ( + )} - {!connected ? ( -
- + + {config.hasCredential && ( + -
- ) : ( - <> - - - - -
- - - - -
- - )} + )} +
{msg && {msg}} diff --git a/package.json b/package.json index 28f34fd..5573b9a 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,8 @@ "seed": "npm run seed --prefix server", "build": "npm run build --prefix client", "start": "npm start --prefix server", - "check:modules": "node scripts/checkModuleIdentifiers.js" + "check:modules": "node scripts/checkModuleIdentifiers.js", + "check:hosts": "node scripts/checkNoExternalHosts.js" }, "keywords": ["express", "mariadb", "react", "vite", "jwt"], "author": "whitlocktech", diff --git a/scripts/checkNoExternalHosts.js b/scripts/checkNoExternalHosts.js new file mode 100644 index 0000000..d45747e --- /dev/null +++ b/scripts/checkNoExternalHosts.js @@ -0,0 +1,185 @@ +#!/usr/bin/env node +// ── §3.2 rule 4 — no phone-home in the engagement subsystem ──────────────── +// +// ENGAGEMENT.md §3.2 records a posture the codebase already has and this check +// exists to keep: **no transport may ship a default host, endpoint, API base or +// sender.** A transport with no operator configuration is `unconfigured` and its +// channel is off — it never quietly falls back to a destination we chose. +// +// The rule is easy to hold and easy to break by accident, and the removed Gmail +// transport is the proof of both: `smtp.gmail.com` and port 465 were literals in +// `mailer.buildTransport()`, which made "which provider" a code edit and made the +// deployment's mail depend on a host nobody configured. Deleting that literal is +// what this check was written against, and it is the first thing it would have +// caught. +// +// **It reads code, not prose.** A comment naming `smtp.gmail.com` as the +// migration path for existing operators is exactly the documentation this phase +// owes, and a check that forbade it would teach people to phrase around it. So +// comments and the insides of ordinary strings are masked out; what is checked is +// a HOSTNAME OR URL appearing as a string literal in the engagement trees. Same +// design, and the same reasoning, as `checkModuleIdentifiers.js` — including +// having its own test suite, because a check that silently stops checking is +// worse than no check. +// +// Scope is the engagement subsystem plus the mail path it owns, not the whole +// server: core legitimately talks to hosts an operator configured elsewhere +// (ntfy, Discord, the sidecar), and those are not this rule's business. + +const fs = require('fs') +const path = require('path') + +const ROOT = path.resolve(__dirname, '..') + +// The trees the rule covers. `server/src/engagement/` is where transports and, +// later, the rules engine live; `utils/mailer.js` is the one file outside it that +// composes and sends mail. +const TREES = [path.join(ROOT, 'server', 'src', 'engagement')] +const FILES = [path.join(ROOT, 'server', 'src', 'utils', 'mailer.js')] + +const SKIP_DIRS = new Set(['node_modules', 'coverage', 'dist', '.git']) +const CODE = new Set(['.js', '.jsx', '.mjs', '.cjs']) + +// A URL, or a bare dotted hostname with a real TLD. The TLD length floor is what +// keeps `emailConfig.model` and `foo.js` out of it — a two-plus-letter final +// label after at least one dot, with no path characters, is a host. +const URL_LITERAL = /\b(?:https?|smtps?):\/\/[^\s'"`]+/ +const HOSTNAME_LITERAL = /\b(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+(?:com|net|org|io|dev|co|email|mail|cloud|app|us|eu)\b/i + +// Hosts that are not destinations: the loopback family, and the RFC 2606 names +// reserved for documentation. A placeholder in an admin form's help text is the +// opposite of a phone-home — it shows the operator the SHAPE of a value they +// must supply, and blanking it would make the form worse to hold the rule. +const ALLOWED = [ + /^(?:localhost|127\.0\.0\.1|\[::1\]|0\.0\.0\.0)$/i, + /(?:^|\.)example\.(?:com|net|org)$/i, + /(?:^|\.)(?:invalid|test|localhost)$/i, +] + +const isAllowed = (host) => ALLOWED.some((re) => re.test(host)) + +const hostOf = (literal) => { + const withoutScheme = literal.replace(/^[a-z]+:\/\//i, '') + return withoutScheme.split(/[/?#:]/)[0] +} + +/** + * Blank comments and mask string bodies in one left-to-right pass, keeping every + * offset aligned so reported line numbers stay honest. + * + * Lifted from `checkModuleIdentifiers.maskCode` deliberately rather than + * imported: that file's masking is tuned to ITS four checks (it keeps quotes so a + * route-path check can re-read the original at the same offsets), and coupling + * two checks through a shared helper means a change made for one silently + * re-scopes the other. Both are ~40 lines and both are tested. + */ +function maskComments(src) { + const out = Array.from(src) + const blank = (from, to) => { + for (let i = from; i < to && i < out.length; i++) if (out[i] !== '\n') out[i] = ' ' + } + let i = 0 + while (i < src.length) { + const c = src[i] + const next = src[i + 1] + if (c === '/' && next === '/') { + let j = i + while (j < src.length && src[j] !== '\n') j++ + blank(i, j) + i = j + continue + } + if (c === '/' && next === '*') { + const end = src.indexOf('*/', i + 2) + const j = end === -1 ? src.length : end + 2 + blank(i, j) + i = j + continue + } + if (c === '"' || c === "'" || c === '`') { + let j = i + 1 + while (j < src.length) { + if (src[j] === '\\') { j += 2; continue } + if (src[j] === c) break + j++ + } + // Keep the string body: it is what this check reads. Only the delimiters + // matter for finding it, and comments are what has to go. + i = j + 1 + continue + } + i++ + } + return out.join('') +} + +// Every string literal in the (comment-free) source, with its line number. +const STRING = /(['"`])((?:\\.|(?!\1)[^\\])*)\1/g + +function lineOf(src, index) { + return src.slice(0, index).split('\n').length +} + +/** Check one file's contents. Returns [{ file, line, literal, host }]. */ +function checkFile(rel, src) { + const hits = [] + const code = maskComments(src) + for (const m of code.matchAll(STRING)) { + const value = m[2] + if (!value) continue + const urlMatch = value.match(URL_LITERAL) + const hostMatch = urlMatch ? null : value.match(HOSTNAME_LITERAL) + const literal = urlMatch ? urlMatch[0] : hostMatch ? hostMatch[0] : null + if (!literal) continue + const host = hostOf(literal) + if (isAllowed(host)) continue + hits.push({ file: rel, line: lineOf(src, m.index), literal, host }) + } + return hits +} + +function walk(dir, out = []) { + if (!fs.existsSync(dir)) return out + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (SKIP_DIRS.has(entry.name)) continue + const full = path.join(dir, entry.name) + if (entry.isDirectory()) walk(full, out) + else out.push(full) + } + return out +} + +function run() { + const files = [...TREES.flatMap((t) => walk(t)), ...FILES.filter((f) => fs.existsSync(f))] + const hits = [] + for (const file of files) { + if (!CODE.has(path.extname(file))) continue + const rel = path.relative(ROOT, file).split(path.sep).join('/') + hits.push(...checkFile(rel, fs.readFileSync(file, 'utf8'))) + } + return hits +} + +module.exports = { run, checkFile, maskComments, isAllowed, hostOf } + +if (require.main === module) { + const hits = run() + if (hits.length === 0) { + console.log('OK — the engagement subsystem names no external host (ENGAGEMENT.md §3.2 rule 4).') + process.exit(0) + } + console.error( + `\nThe engagement subsystem names ${hits.length} external host${hits.length === 1 ? '' : 's'} ` + + 'in code (ENGAGEMENT.md §3.2 rule 4). A destination belongs in operator-supplied ' + + 'configuration, never in a literal:\n', + ) + for (const h of hits) { + console.error(` ${h.file}:${h.line} "${h.literal}"`) + } + console.error( + '\nIf this is help text or documentation rather than a destination, put it in a comment or ' + + 'use an example.com placeholder — the check masks comments and allows the reserved ' + + 'documentation names on purpose.\n', + ) + process.exit(1) +} diff --git a/server/db/schema.sql b/server/db/schema.sql index c20fb32..f6cfb91 100644 --- a/server/db/schema.sql +++ b/server/db/schema.sql @@ -327,19 +327,33 @@ CREATE TABLE IF NOT EXISTS bot_config ( CONSTRAINT chk_bot_config_singleton CHECK (id = 1) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; --- Outbound email configuration (Gmail over OAuth2 / SMTP XOAUTH2). Singleton row --- (id = 1), mirroring bot_config: the DB only ever holds the AES-256-GCM-encrypted --- refresh token, never plaintext, and the client id/secret are NOT stored here — --- they are read live from the `google` auth_providers row. The refresh token is --- captured by the in-app "Connect Gmail" consent flow and is write-only over the --- admin API (never returned; responses expose only hasRefreshToken). +-- Outbound email configuration. Singleton row (id = 1), mirroring bot_config: the +-- DB only ever holds the AES-256-GCM-encrypted credential, never plaintext, and it +-- is write-only over the admin API (never returned; responses expose only +-- hasCredential and the non-secret fields the transport declares). +-- +-- `transport` names a registered mail transport (server/src/engagement/transports). +-- `credential_enc` is that transport's whole credential set as one encrypted JSON +-- blob rather than a column per field, because the field list is the transport's to +-- declare — SMTP wants host/port/secure/user/password, an API relay wants a domain +-- and a key, and a column per union member would make adding a transport a schema +-- change. ENGAGEMENT.md §3.1. +-- +-- `provider` and `refresh_token_enc` are DEPRECATED and no longer read: they held +-- the removed Gmail OAuth2 connection (ENGAGEMENT.md §1.2a). They are kept rather +-- than dropped under the additive-only discipline, and `refresh_token_enc` earns +-- its keep in the meantime as the marker for "this deployment had working mail +-- before the upgrade" — which is what the admin dashboard warning reads. CREATE TABLE IF NOT EXISTS email_config ( id INT PRIMARY KEY DEFAULT 1, - provider VARCHAR(20) NOT NULL DEFAULT 'gmail_oauth2', + provider VARCHAR(20) NOT NULL DEFAULT 'gmail_oauth2', -- DEPRECATED, unread + transport VARCHAR(32) NOT NULL DEFAULT 'smtp', enabled TINYINT(1) NOT NULL DEFAULT 0, - sender_email VARCHAR(255) NULL, -- connected Gmail address (from userinfo) + sender_email VARCHAR(255) NULL, -- envelope From, operator-typed sender_name VARCHAR(120) NULL, -- optional From display name - refresh_token_enc TEXT NULL, -- AES-256-GCM ciphertext, never exposed + reply_to VARCHAR(255) NULL, -- optional Reply-To for sent mail + credential_enc TEXT NULL, -- AES-256-GCM ciphertext (JSON), never exposed + refresh_token_enc TEXT NULL, -- DEPRECATED, unread; see above status VARCHAR(20) NOT NULL DEFAULT 'unconfigured', status_detail VARCHAR(500) NULL, last_verified_at DATETIME NULL, @@ -1464,3 +1478,26 @@ ALTER TABLE mobile_refresh_tokens ADD COLUMN IF NOT EXISTS last_used_at DATETIME -- trust token. A boolean only — the token is returned over that app→server call -- and never persisted here (only its sha256 lands in trusted_devices). ALTER TABLE mobile_auth_sessions ADD COLUMN IF NOT EXISTS trust_device TINYINT(1) NOT NULL DEFAULT 0; + +-- ── Engagement Phase 1: Gmail OAuth2 removed, SMTP is the baseline ────────── +-- (ENGAGEMENT.md §1.2a). Additive on an upgraded database: `transport` backfills +-- to 'smtp' for every existing row, and `credential_enc` starts NULL — so an +-- upgraded deployment is deliberately CREDENTIAL-LESS until its operator supplies +-- SMTP settings. That is the whole point of the G22 warning below: nothing about +-- this fails loudly, so something has to say it out loud. +ALTER TABLE email_config ADD COLUMN IF NOT EXISTS transport VARCHAR(32) NOT NULL DEFAULT 'smtp'; +ALTER TABLE email_config ADD COLUMN IF NOT EXISTS credential_enc TEXT NULL; +ALTER TABLE email_config ADD COLUMN IF NOT EXISTS reply_to VARCHAR(255) NULL; + +-- The status a Gmail-connected deployment carries is 'connected', and after the +-- upgrade that is a lie: nothing can send. Correct it once, narrowly. The WHERE +-- makes this idempotent and self-limiting — it matches only a row that still holds +-- a Gmail refresh token AND has no replacement credential, so re-running it after +-- the operator configures SMTP touches nothing, and it can never overwrite a real +-- status recorded by a later send. +UPDATE email_config + SET status = 'unconfigured', + status_detail = 'Gmail OAuth2 was removed. Configure SMTP credentials in Admin - Settings - Email.' + WHERE refresh_token_enc IS NOT NULL + AND credential_enc IS NULL + AND status <> 'unconfigured'; diff --git a/server/routes.guards.json b/server/routes.guards.json index 05b5ce9..20e99fe 100644 --- a/server/routes.guards.json +++ b/server/routes.guards.json @@ -199,7 +199,7 @@ { "method": "PUT", "path": "/api/v1/admin/email/config", - "handlers": 5, + "handlers": 9, "gates": [ "noindex", "requireAuth", @@ -207,24 +207,6 @@ "validate" ] }, - { - "method": "GET", - "path": "/api/v1/admin/email/connect/callback", - "handlers": 2, - "gates": [ - "noindex", - "requireAuth" - ] - }, - { - "method": "GET", - "path": "/api/v1/admin/email/connect/start", - "handlers": 2, - "gates": [ - "noindex", - "requireAuth" - ] - }, { "method": "POST", "path": "/api/v1/admin/email/disconnect", diff --git a/server/routes.manifest.json b/server/routes.manifest.json index 178300d..1c98c66 100644 --- a/server/routes.manifest.json +++ b/server/routes.manifest.json @@ -89,14 +89,6 @@ "method": "PUT", "path": "/api/v1/admin/email/config" }, - { - "method": "GET", - "path": "/api/v1/admin/email/connect/callback" - }, - { - "method": "GET", - "path": "/api/v1/admin/email/connect/start" - }, { "method": "POST", "path": "/api/v1/admin/email/disconnect" diff --git a/server/src/engagement/index.js b/server/src/engagement/index.js new file mode 100644 index 0000000..e64adab --- /dev/null +++ b/server/src/engagement/index.js @@ -0,0 +1,22 @@ +// ── The engagement subsystem — one door ──────────────────────────────────── +// +// ENGAGEMENT.md Phase 1. Today this is the mail transport registry and core's +// own transports; the trigger registry, the rules engine and the delivery +// channels arrive in later phases and hang here too. +// +// **Core's transports register through the same door a module's would**, and +// they register HERE rather than at the bottom of the registry file. That keeps +// the registry free of any knowledge of its registrants — the same reason +// `registerCore()` is called from app.js rather than from inside +// `modules/registries.js` (MODULE_API.md §7.6) — and it means requiring the +// registry never has the side effect of populating it. +// +// Requiring this module is what makes `smtp` available. Everything that resolves +// a transport goes through here, so there is exactly one place a transport can +// come into existence. + +require('./transports/smtp') + +const transports = require('./transports') + +module.exports = { transports } diff --git a/server/src/engagement/transports/index.js b/server/src/engagement/transports/index.js new file mode 100644 index 0000000..3a7d7b3 --- /dev/null +++ b/server/src/engagement/transports/index.js @@ -0,0 +1,195 @@ +// ── The mail transport registry ──────────────────────────────────────────── +// +// ENGAGEMENT.md §3.1, Phase 1. A **channel** is what kind of sink this is (email, +// push, in-app); a **transport** is how one channel actually delivers. This file +// is the second half only. The channel registry arrives with the engine that +// consumes it — registering a channel nothing calls would be a shape frozen +// before anything had tried to use it. +// +// What this replaces: `mailer.buildTransport()` had Gmail's host, port and +// OAuth2 auth type as literals, so "which provider" was a code edit. Now the +// stored `email_config.transport` names a registration, and the registration +// declares its own credential fields — which drives the admin form, the encrypted +// blob's shape and the validation, from one place. +// +// **`credentialFields` is the contract.** It is read by three consumers that +// would otherwise drift: the admin form renders it, `sanitizeCredential()` below +// filters a submitted body through it, and `describe()` tells the client which +// values are secret so they are never sent back. Adding a field to a transport is +// therefore one edit, not four. +// +// **No transport may carry a default host, endpoint or sender** (§3.2 rule 1). A +// transport with no operator configuration is `unconfigured` and its channel is +// off — it never falls back to somewhere we chose. `scripts/checkNoExternalHosts.js` +// is the CI backstop for that rule; this file is where it would be broken first. +// +// Nothing here touches the database or the network at require time. + +const log = require('../../utils/logger')('mailer') + +// id → transport definition +const transports = new Map() + +// Field kinds the admin form knows how to render. `secret` is the only one that +// changes behaviour server-side: it is write-only, so an unchanged value arrives +// as '' and must be read from the stored credential rather than overwritten. +const FIELD_KINDS = new Set(['text', 'number', 'secret', 'boolean']) + +/** + * Register a mail transport. Shape-checked at the call and collision-checked + * here, the same validate-then-commit discipline `modules/registries.js` uses. + * + * @param {object} def + * @param {string} def.id stable id stored in email_config.transport + * @param {string} def.label human name for the admin form + * @param {Array} def.credentialFields [{ key, label, kind, required, help, default }] + * @param {Function} def.build (credential, config) → a nodemailer-shaped transport + * @param {Function} def.isComplete (credential) → boolean; are the required fields present + */ +function registerMailTransport(def) { + if (!def || typeof def !== 'object') throw new Error('registerMailTransport: definition required') + const { id, label, credentialFields, build, isComplete } = def + if (typeof id !== 'string' || !/^[a-z][a-z0-9_-]*$/.test(id)) { + throw new Error(`registerMailTransport: invalid id ${JSON.stringify(id)}`) + } + if (transports.has(id)) throw new Error(`registerMailTransport: ${id} is already registered`) + if (typeof label !== 'string' || !label) throw new Error(`registerMailTransport(${id}): label required`) + if (!Array.isArray(credentialFields) || credentialFields.length === 0) { + throw new Error(`registerMailTransport(${id}): credentialFields required`) + } + for (const f of credentialFields) { + if (!f || typeof f.key !== 'string' || !f.key) { + throw new Error(`registerMailTransport(${id}): every credential field needs a key`) + } + if (!FIELD_KINDS.has(f.kind)) { + throw new Error(`registerMailTransport(${id}): field ${f.key} has unknown kind ${f.kind}`) + } + } + if (typeof build !== 'function') throw new Error(`registerMailTransport(${id}): build() required`) + if (typeof isComplete !== 'function') throw new Error(`registerMailTransport(${id}): isComplete() required`) + + transports.set(id, { ...def, credentialFields: credentialFields.map((f) => ({ ...f })) }) + return id +} + +/** The registered transport, or null. Callers must handle null — a stored id can + * name a transport that no longer exists (a downgrade, a removed provider), and + * that must degrade to "unconfigured", never throw at send time. */ +function get(id) { + return transports.get(id) || null +} + +function has(id) { + return transports.has(id) +} + +/** Every transport, as the admin form needs it: no functions, secrets flagged. */ +function describe() { + return [...transports.values()].map((t) => ({ + id: t.id, + label: t.label, + help: t.help || null, + credentialFields: t.credentialFields.map((f) => ({ + key: f.key, + label: f.label || f.key, + kind: f.kind, + required: Boolean(f.required), + help: f.help || null, + default: f.default === undefined ? null : f.default, + placeholder: f.placeholder || null, + })), + })) +} + +/** + * Filter a submitted credential body down to the transport's declared fields, + * coercing each to its declared kind. Anything not declared is dropped — the + * blob that reaches `secretBox.encrypt` only ever holds fields a transport asked + * for, so a client cannot smuggle extra keys into stored ciphertext. + * + * `secret` fields submitted empty are OMITTED rather than blanked, which is the + * "leave the existing one alone" convention `botConfig.save`/`emailConfig.save` + * already use; `mergeCredential()` is what puts the stored value back. + */ +function sanitizeCredential(id, body) { + const t = get(id) + if (!t) return {} + const out = {} + for (const f of t.credentialFields) { + if (!(f.key in (body || {}))) continue + const raw = body[f.key] + if (f.kind === 'secret') { + if (raw === undefined || raw === null || raw === '') continue + out[f.key] = String(raw) + } else if (f.kind === 'number') { + const n = Number(raw) + if (Number.isFinite(n)) out[f.key] = n + } else if (f.kind === 'boolean') { + out[f.key] = Boolean(raw) + } else { + out[f.key] = raw === null || raw === undefined ? '' : String(raw) + } + } + return out +} + +/** Stored credential + the submitted patch. Omitted secrets keep their stored value. */ +function mergeCredential(id, stored, patch) { + return { ...(stored || {}), ...(patch || {}) } +} + +/** Non-secret fields only — safe to return over the admin API. */ +function publicCredential(id, credential) { + const t = get(id) + if (!t || !credential) return {} + const out = {} + for (const f of t.credentialFields) { + if (f.kind === 'secret') continue + if (credential[f.key] !== undefined) out[f.key] = credential[f.key] + } + return out +} + +/** Which declared secrets are actually held, so the form can say "set" without + * ever returning the value. */ +function secretsPresent(id, credential) { + const t = get(id) + if (!t) return {} + const out = {} + for (const f of t.credentialFields) { + if (f.kind !== 'secret') continue + out[f.key] = Boolean(credential && credential[f.key]) + } + return out +} + +/** Does this credential have everything its transport needs to send? */ +function isComplete(id, credential) { + const t = get(id) + if (!t) return false + try { + return Boolean(t.isComplete(credential || {})) + } catch (err) { + log.warn('transport isComplete threw', { transport: id, message: err.message }) + return false + } +} + +// Test-only: the registry is module-level state and a suite that registers a +// fake transport must be able to undo it. +function _reset() { + transports.clear() +} + +module.exports = { + registerMailTransport, + get, + has, + describe, + sanitizeCredential, + mergeCredential, + publicCredential, + secretsPresent, + isComplete, + _reset, +} diff --git a/server/src/engagement/transports/smtp.js b/server/src/engagement/transports/smtp.js new file mode 100644 index 0000000..8450076 --- /dev/null +++ b/server/src/engagement/transports/smtp.js @@ -0,0 +1,96 @@ +// ── SMTP — the baseline mail transport ───────────────────────────────────── +// +// ENGAGEMENT.md decision 4: Gmail OAuth2 is removed, SMTP is the baseline. This +// is the only registered transport, and it is deliberately plain SMTP rather than +// anything provider-shaped — a relay (Mailgun, SES, Postmark), a self-hosted MTA +// and Gmail-with-an-app-password are all reachable through these five fields, so +// one transport covers all three postures §7.1 Q5 asks to document. +// +// **No defaults for host, port, user or sender.** §3.2 rule 1: a transport with +// no operator configuration is unconfigured, never pointed at somewhere we chose. +// `secure` gets a default because it is a protocol choice, not a destination — and +// even that is only a form default, not a fallback applied to a stored blank. +// +// **`secure` is the field operators get wrong**, so its help text says which port +// each setting means: `secure: true` is implicit TLS on 465, `secure: false` is +// plaintext-then-STARTTLS on 587 (which nodemailer upgrades automatically). The +// combination that silently fails is 587 with secure on — the handshake hangs +// rather than erroring cleanly — which is exactly why "Send test" is the real +// verification path now (§1.2a consequence 2). + +const nodemailer = require('nodemailer') + +const registry = require('./index') + +const CREDENTIAL_FIELDS = [ + { + key: 'host', + label: 'SMTP host', + kind: 'text', + required: true, + placeholder: 'smtp.example.com', + help: 'Your relay or mail server. No default — nothing is sent until you set this.', + }, + { + key: 'port', + label: 'Port', + kind: 'number', + required: true, + default: 587, + help: '587 for STARTTLS (most relays), 465 for implicit TLS, 25 for an unauthenticated local MTA.', + }, + { + key: 'secure', + label: 'Implicit TLS', + kind: 'boolean', + required: false, + default: false, + help: 'On for port 465. Leave off for 587 — the connection still upgrades to TLS via STARTTLS.', + }, + { + key: 'user', + label: 'Username', + kind: 'text', + required: false, + help: 'Leave blank for an unauthenticated local relay.', + }, + { + key: 'password', + label: 'Password / API key', + kind: 'secret', + required: false, + help: 'Stored encrypted and never returned. For Gmail this is an app password, not the account password.', + }, +] + +// Authentication is optional (a local MTA on port 25 needs none), so the only +// hard requirement is a destination. A username without a password is not +// "complete" — that combination authenticates as nobody and fails at the server. +function isComplete(credential) { + const c = credential || {} + if (!c.host || !Number(c.port)) return false + if (c.user && !c.password) return false + return true +} + +function build(credential) { + const c = credential || {} + const options = { + host: String(c.host), + port: Number(c.port), + secure: Boolean(c.secure), + } + if (c.user) options.auth = { user: String(c.user), pass: String(c.password || '') } + return nodemailer.createTransport(options) +} + +registry.registerMailTransport({ + id: 'smtp', + label: 'SMTP', + help: 'Any SMTP relay or mail server. See the operator guide for the three supported postures.', + credentialFields: CREDENTIAL_FIELDS, + isComplete, + build, +}) + +module.exports = { CREDENTIAL_FIELDS, isComplete, build } diff --git a/server/src/model/emailConfig/emailConfig.db.js b/server/src/model/emailConfig/emailConfig.db.js index 5927241..874ab05 100644 --- a/server/src/model/emailConfig/emailConfig.db.js +++ b/server/src/model/emailConfig/emailConfig.db.js @@ -1,7 +1,8 @@ const singletonConfigDb = require('../singletonConfigDb') const COLS = - 'id, provider, enabled, sender_email, sender_name, refresh_token_enc, status, status_detail, last_verified_at, updated_by, created_at, updated_at' + 'id, provider, transport, enabled, sender_email, sender_name, reply_to, credential_enc, refresh_token_enc, ' + + 'status, status_detail, last_verified_at, updated_by, created_at, updated_at' // Singleton row (id = 1). See ../singletonConfigDb for the get/upsert contract. module.exports = singletonConfigDb('email_config', COLS) diff --git a/server/src/model/emailConfig/emailConfig.model.js b/server/src/model/emailConfig/emailConfig.model.js index 7570857..ee28272 100644 --- a/server/src/model/emailConfig/emailConfig.model.js +++ b/server/src/model/emailConfig/emailConfig.model.js @@ -1,30 +1,69 @@ -// Outbound email config store (Gmail OAuth2). Mirrors the botConfig model split: -// the DB layer only ever sees ciphertext, and only getWithSecret() (used by the -// mailer at send time) decrypts the refresh token. The admin-facing getSafe() -// never includes it — callers see only `hasRefreshToken`. +// Outbound email config store. Mirrors the botConfig model split: the DB layer +// only ever sees ciphertext, and only getWithSecret() (used by the mailer at send +// time) decrypts the credential. The admin-facing getSafe() never includes it — +// callers see the non-secret fields plus which secrets are set. +// +// The credential is ONE encrypted JSON blob, not a column per field, because the +// field list belongs to the transport (ENGAGEMENT.md §3.1). `credential_enc` +// holds `{ host, port, secure, user, password }` for `smtp`; a future relay's +// blob would hold different keys against the same column. +// +// `provider` and `refresh_token_enc` are the removed Gmail OAuth2 connection +// (§1.2a). They are no longer read as configuration — `hadLegacyConnection` +// exposes the token column's presence for one purpose only: telling an upgraded +// deployment that its mail just stopped. const db = require('./emailConfig.db') const secretBox = require('../../utils/secretBox') +const { transports } = require('../../engagement') -function toSafe(row) { +const DEFAULT_TRANSPORT = 'smtp' + +// A stored blob that will not parse is treated as ABSENT, never as an error — +// the same fail-safe rule utils/settingsJson.js applies. A deployment whose +// SECRET_ENC_KEY was rotated must degrade to "unconfigured" and say so on the +// admin screen, not 500 the settings page and the contact form with it. +function readCredential(row) { + if (!row || !row.credential_enc) return null + try { + const parsed = JSON.parse(secretBox.decrypt(row.credential_enc)) + return parsed && typeof parsed === 'object' ? parsed : null + } catch { + return null + } +} + +function toSafe(row, credential) { + const transport = (row && row.transport) || DEFAULT_TRANSPORT if (!row) { return { - provider: 'gmail_oauth2', + transport: DEFAULT_TRANSPORT, enabled: false, senderEmail: null, senderName: null, - hasRefreshToken: false, + replyTo: null, + credential: {}, + secretsSet: transports.secretsPresent(DEFAULT_TRANSPORT, null), + hasCredential: false, + hadLegacyConnection: false, status: 'unconfigured', statusDetail: null, lastVerifiedAt: null, } } return { - provider: row.provider || 'gmail_oauth2', + transport, enabled: Boolean(row.enabled), senderEmail: row.sender_email || null, senderName: row.sender_name || null, - hasRefreshToken: Boolean(row.refresh_token_enc), + replyTo: row.reply_to || null, + credential: transports.publicCredential(transport, credential), + secretsSet: transports.secretsPresent(transport, credential), + hasCredential: transports.isComplete(transport, credential), + // Deliberately the raw column, not "is Gmail configured": nothing reads the + // token any more. It answers "did this deployment have working mail before + // the upgrade?", which is the G22 warning's whole condition. + hadLegacyConnection: Boolean(row.refresh_token_enc), status: row.status || 'unconfigured', statusDetail: row.status_detail || null, lastVerifiedAt: row.last_verified_at || null, @@ -32,38 +71,53 @@ function toSafe(row) { } async function getSafe() { - return toSafe(await db.get()) + const row = await db.get() + return toSafe(row, readCredential(row)) } -// Decrypted refresh token included — server-side only (building the mailer's -// OAuth2 transport). Returns null when no row exists yet. +// Decrypted credential included — server-side only (building the transport at +// send time). Returns null when no row exists yet. async function getWithSecret() { const row = await db.get() if (!row) return null - return { - ...toSafe(row), - refreshToken: row.refresh_token_enc ? secretBox.decrypt(row.refresh_token_enc) : null, - } + const credential = readCredential(row) + return { ...toSafe(row, credential), credentialSecret: credential || {} } } -// Save admin-supplied / connect-flow config. `refreshToken` undefined or '' means -// "leave the existing token unchanged" (same convention as botConfig.save). -async function save({ senderEmail, senderName, refreshToken, enabled, status, statusDetail, updatedBy }) { +// Save admin-supplied config. `credential` is a PATCH, merged over the stored +// blob: a secret field submitted empty is omitted by the registry's sanitizer and +// therefore keeps its stored value (same convention as botConfig.save). +async function save({ transport, senderEmail, senderName, replyTo, credential, enabled, status, statusDetail, updatedBy }) { + const row = await db.get() const fields = {} + const nextTransport = transport !== undefined ? transport : (row && row.transport) || DEFAULT_TRANSPORT + if (transport !== undefined) fields.transport = transport if (senderEmail !== undefined) fields.sender_email = senderEmail if (senderName !== undefined) fields.sender_name = senderName - if (refreshToken) fields.refresh_token_enc = secretBox.encrypt(refreshToken) + if (replyTo !== undefined) fields.reply_to = replyTo + if (credential !== undefined) { + // Changing transport starts from an empty credential rather than merging one + // transport's fields into another's — an SMTP password left inside a relay's + // blob is a stored secret nobody can see and nothing will ever use. + const base = row && row.transport === nextTransport ? readCredential(row) : null + const merged = transports.mergeCredential(nextTransport, base, transports.sanitizeCredential(nextTransport, credential)) + fields.credential_enc = Object.keys(merged).length ? secretBox.encrypt(JSON.stringify(merged)) : null + } if (enabled !== undefined) fields.enabled = enabled ? 1 : 0 if (status !== undefined) fields.status = status if (statusDetail !== undefined) fields.status_detail = statusDetail if (updatedBy !== undefined) fields.updated_by = updatedBy - const row = await db.upsert(fields) - return toSafe(row) + const saved = await db.upsert(fields) + return toSafe(saved, readCredential(saved)) } -// Clear the stored credential and disable sending (admin "Disconnect"). +// Clear the stored credential and disable sending (admin "Clear credentials"). +// The legacy Gmail token goes too: this is the operator saying "there is no +// mailbox here", and leaving the deprecated column set would keep the G22 warning +// on screen for a deployment that has deliberately turned mail off. async function disconnect(updatedBy) { const row = await db.upsert({ + credential_enc: null, refresh_token_enc: null, sender_email: null, enabled: 0, @@ -72,7 +126,7 @@ async function disconnect(updatedBy) { last_verified_at: null, updated_by: updatedBy ?? null, }) - return toSafe(row) + return toSafe(row, readCredential(row)) } // Record the outcome of the last send / verification so the admin panel has @@ -86,7 +140,7 @@ async function recordStatus({ status, statusDetail, lastVerifiedAt } = {}) { } if (Object.keys(fields).length === 0) return getSafe() const row = await db.upsert(fields) - return toSafe(row) + return toSafe(row, readCredential(row)) } -module.exports = { getSafe, getWithSecret, save, disconnect, recordStatus } +module.exports = { getSafe, getWithSecret, save, disconnect, recordStatus, DEFAULT_TRANSPORT } diff --git a/server/src/model/teams/teamNotify.model.js b/server/src/model/teams/teamNotify.model.js index a085642..6f5d70f 100644 --- a/server/src/model/teams/teamNotify.model.js +++ b/server/src/model/teams/teamNotify.model.js @@ -5,8 +5,8 @@ // Push is opt-out: a user in one Team must never have to configure anything to be // tickled about it, and the per-Team mute is how they stop. Email is opt-IN // (`email_mode` defaults to `'off'`, deviating from §6.4 on the org lead's call): -// turning on Gmail in the admin panel must not start sending daily mail to every -// member of every Team on the deployment. +// configuring a mail transport in the admin panel must not start sending daily +// mail to every member of every Team on the deployment. // // Both are read the same way — COALESCE to the column default, never treat a // missing row as "unknown" — so the asymmetry lives in ONE place, the schema, and diff --git a/server/src/router/v1/admin/admin.controller.js b/server/src/router/v1/admin/admin.controller.js index c91fc7e..e542700 100644 --- a/server/src/router/v1/admin/admin.controller.js +++ b/server/src/router/v1/admin/admin.controller.js @@ -9,6 +9,7 @@ const trustedDevices = require('../../../model/trustedDevices/trustedDevices.mod const recoveryCodes = require('../../../model/recoveryCodes/recoveryCodes.model') const registries = require('../../../modules/registries') const announceJobs = require('../../../model/announceJobs/announceJobs.model') +const emailConfig = require('../../../model/emailConfig/emailConfig.model') const forumSettings = require('../../../model/teams/teamForumSettings.model') const pushDispatch = require('../../../utils/pushDispatch') const { cleanBody } = require('../../../utils/sanitizeHtml') @@ -55,6 +56,39 @@ async function announceIfNewlyPublished(post, transition) { } // ── Dashboard & site mode ───────────────────────────────────────────── + +// G22 — the Gmail removal degrades SILENTLY (ENGAGEMENT.md §1.2a consequence 3). +// On upgrade, `transport` backfills to `smtp` with no credentials, so every sink +// politely does nothing: the contact form falls back to `mailto`, invites surface +// a copyable link, password resets still answer a generic 200. Nothing breaks +// loudly, which is exactly the risk — email stops and nobody is told. +// +// So something has to say it. The condition is deliberately narrow: a deployment +// that still holds the deprecated Gmail refresh token (it had working mail) and +// has no replacement credential (it does not any more). A fresh install has never +// had mail and is not warned — a nag about a capability nobody asked for is a +// banner people learn to ignore, and this one has to be believed exactly once. +// +// Never fails the dashboard. A warning that can 500 the admin landing page is a +// worse bug than the one it reports. +async function emailWarning() { + try { + const c = await emailConfig.getSafe() + if (!c.hadLegacyConnection || c.hasCredential) return null + return { + code: 'EMAIL_TRANSPORT_MIGRATION', + message: + 'Outbound email is not configured. This deployment used the Gmail connect flow, which has been ' + + 'removed — mail is no longer being sent. Add SMTP credentials under Settings → Email. Gmail still ' + + 'works as an ordinary SMTP relay with an app password.', + href: '/admin/settings', + } + } catch (err) { + log.warn('dashboard email warning check failed', { message: err.message }) + return null + } +} + async function dashboard(req, res) { try { return res.json({ @@ -67,6 +101,7 @@ async function dashboard(req, res) { posts: await posts.counts(), users: await users.count(), }, + warnings: [await emailWarning()].filter(Boolean), recent_activity: await activity.list({ limit: 10 }), }) } catch (err) { diff --git a/server/src/router/v1/admin/dashboard.router.js b/server/src/router/v1/admin/dashboard.router.js index 2927d7d..2ff4106 100644 --- a/server/src/router/v1/admin/dashboard.router.js +++ b/server/src/router/v1/admin/dashboard.router.js @@ -12,7 +12,7 @@ // through toward another mount and 403 an editor on an unrelated route. Keep // gates per-route in this file. // -// GET /dashboard — stats overview, any staff role. +// GET /dashboard — stats overview + operator warnings, any staff role. // PUT /site-mode — live ↔ maintenance, admin only. // // Neither is the audit log (/activity) nor the bot-scoring state @@ -34,7 +34,7 @@ dashboardRouter.get( // #swagger.tags = ['Admin · Dashboard'] // #swagger.summary = 'Dashboard summary counts' // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.responses[200] = { description: 'Summary: site mode, last change, post/user counts and recent activity', content: { "application/json": { schema: { type: "object", properties: { site_mode: { type: "string", example: "live" }, last_change: { type: "object", properties: { at: { type: "string", nullable: true }, by: { type: "string", nullable: true } } }, counts: { type: "object", properties: { posts: { type: "object", additionalProperties: true }, users: { type: "integer" } } }, recent_activity: { type: "array", items: { type: "object", additionalProperties: true } } } } } } } */ + /* #swagger.responses[200] = { description: 'Summary: site mode, last change, post/user counts and recent activity', content: { "application/json": { schema: { type: "object", properties: { site_mode: { type: "string", example: "live" }, last_change: { type: "object", properties: { at: { type: "string", nullable: true }, by: { type: "string", nullable: true } } }, counts: { type: "object", properties: { posts: { type: "object", additionalProperties: true }, users: { type: "integer" } } }, warnings: { type: "array", description: "Operator warnings needing action; empty when there is nothing to say", items: { type: "object", properties: { code: { type: "string", example: "EMAIL_TRANSPORT_MIGRATION" }, message: { type: "string" }, href: { type: "string" } } } }, recent_activity: { type: "array", items: { type: "object", additionalProperties: true } } } } } } } */ /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ ctrl.dashboard, ) diff --git a/server/src/router/v1/admin/email.router.js b/server/src/router/v1/admin/email.router.js index ff2c38c..092bdcc 100644 --- a/server/src/router/v1/admin/email.router.js +++ b/server/src/router/v1/admin/email.router.js @@ -1,9 +1,13 @@ -// Admin · Email — outbound mail delivery via Gmail OAuth2. +// Admin · Email — outbound mail delivery. // // Mounted at /api/v1/admin/email by admin/index.js, which already applied -// `noindex, isLoggedIn, staffOnly`. The modern replacement for env SMTP: the -// refresh token is captured by the connect flow below and is write-only over -// this API (stored encrypted by utils/secretBox.js, never returned). +// `noindex, isLoggedIn, staffOnly`. The credential for the selected transport is +// captured by PUT /config below and is write-only over this API (stored encrypted +// by utils/secretBox.js, never returned). +// +// Four routes. `/connect/start` and `/connect/callback` were deleted with the +// Gmail OAuth2 flow in engagement Phase 1 (ENGAGEMENT.md §1.2a) — SMTP has no +// redirect to bounce through, so a credential form is the whole of it. // // Admin-only, and kept as a per-route gate rather than a router-level `use` so // the middleware chain each route carries is unchanged by the move. @@ -21,9 +25,10 @@ const adminOnly = requireRole('admin') emailRouter.get( '/config', // #swagger.tags = ['Admin · Email'] - // #swagger.summary = 'Get email delivery config + status (admin only)' + // #swagger.summary = 'Get email delivery config, status and the transport catalog (admin only)' + // #swagger.description = 'Credentials are write-only: secret fields are never returned, only a per-field `secretsSet` flag. `transports` carries each registered transport's declared credential fields, which is what the admin form renders.' // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.responses[200] = { description: 'Config (refresh token stripped) + status', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ + /* #swagger.responses[200] = { description: 'Config (secrets stripped) + status + transport catalog', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ adminOnly, @@ -33,45 +38,32 @@ emailRouter.put( '/config', // #swagger.tags = ['Admin · Email'] // #swagger.summary = 'Update email delivery config (admin only)' - // #swagger.description = 'Set the From display name and enabled toggle. Enabling requires a connected Gmail account.' + // #swagger.description = 'Set the transport, sender identity, credentials and enabled toggle. `credential` is a patch against the stored blob — a secret field submitted empty keeps its stored value. Enabling requires complete credentials and a sender address.' // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.requestBody = { content: { "application/json": { schema: { type: "object", properties: { senderName: { type: "string" }, enabled: { type: "boolean" } } } } } } */ + /* #swagger.requestBody = { content: { "application/json": { schema: { type: "object", properties: { transport: { type: "string", example: "smtp" }, senderEmail: { type: "string", format: "email" }, senderName: { type: "string" }, replyTo: { type: "string", format: "email" }, credential: { type: "object", additionalProperties: true }, enabled: { type: "boolean" } } } } } } */ /* #swagger.responses[200] = { description: 'Updated config', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ - /* #swagger.responses[400] = { description: 'Cannot enable before connecting a mailbox', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[400] = { description: 'Unknown transport, or cannot enable without complete credentials and a sender address', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ adminOnly, + body('transport').optional().isString().trim().isLength({ min: 1, max: 32 }), + body('senderEmail').optional({ values: 'falsy' }).isEmail().isLength({ max: 255 }), body('senderName').optional({ values: 'null' }).isString().trim().isLength({ max: 120 }), + body('replyTo').optional({ values: 'falsy' }).isEmail().isLength({ max: 255 }), + // The credential's SHAPE is the transport's to declare, so it is validated by + // the registry's sanitizer (which drops anything undeclared) rather than by a + // field list duplicated here that would drift the first time a transport is + // added. All this asserts is that it is an object at all. + body('credential').optional().isObject(), body('enabled').optional().isBoolean(), validate, emailConfig.saveConfig, ) -emailRouter.get( - '/connect/start', - // #swagger.tags = ['Admin · Email'] - // #swagger.summary = 'Begin the Gmail OAuth2 connect flow (admin only)' - // #swagger.description = 'Returns { url } to redirect the browser to Google. Reuses the google SSO OAuth client.' - // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.responses[200] = { description: 'Authorization URL', content: { "application/json": { schema: { type: "object", properties: { url: { type: "string" } } } } } } */ - /* #swagger.responses[400] = { description: 'Google OAuth client not configured', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ - /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ - /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ - adminOnly, - emailConfig.connectStart, -) -emailRouter.get( - '/connect/callback', - // #swagger.tags = ['Admin · Email'] - // #swagger.summary = 'OAuth2 callback — stores the refresh token, redirects to Settings' - // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.responses[302] = { description: 'Redirect back to /admin/settings' } */ - adminOnly, - emailConfig.connectCallback, -) emailRouter.post( '/test', // #swagger.tags = ['Admin · Email'] // #swagger.summary = 'Send a test email (admin only)' + // #swagger.description = 'The real verification of the configuration — host, port, TLS mode, credentials, and whether the relay accepts the configured sender. Failures return a specific diagnostic.' // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] /* #swagger.requestBody = { content: { "application/json": { schema: { type: "object", properties: { to: { type: "string", format: "email" } } } } } } */ /* #swagger.responses[200] = { description: 'Sent', content: { "application/json": { schema: { type: "object", properties: { sent: { type: "boolean" }, to: { type: "string" } } } } } } */ @@ -84,9 +76,9 @@ emailRouter.post( emailRouter.post( '/disconnect', // #swagger.tags = ['Admin · Email'] - // #swagger.summary = 'Disconnect Gmail and disable email (admin only)' + // #swagger.summary = 'Clear the stored credentials and disable email (admin only)' // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] - /* #swagger.responses[200] = { description: 'Disconnected config', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ + /* #swagger.responses[200] = { description: 'Cleared config', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ adminOnly, diff --git a/server/src/router/v1/admin/emailConfig.controller.js b/server/src/router/v1/admin/emailConfig.controller.js index e6dad60..8493821 100644 --- a/server/src/router/v1/admin/emailConfig.controller.js +++ b/server/src/router/v1/admin/emailConfig.controller.js @@ -1,67 +1,37 @@ -// ── Admin: outbound email configuration (Gmail OAuth2) ───────────────────── +// ── Admin: outbound email configuration ──────────────────────────────────── // -// Modern replacement for env-var SMTP. Sending goes through Gmail over OAuth2; -// the admin connects the mailbox with an in-app consent flow that captures a -// refresh token. We reuse the existing `google` SSO OAuth client (its id/secret) -// rather than a second app — so the only per-mailbox secret is the refresh token, -// stored AES-GCM-encrypted and write-only over this API (never returned). +// Email is configured here, not via env vars. The admin picks a registered mail +// transport and fills in the fields that transport declares; the whole set is +// stored as one AES-GCM-encrypted blob and is write-only over this API — secret +// fields are never returned, only a per-field "is it set" flag. // -// The connect flow mirrors sso.controller.js: a signed httpOnly tx cookie carries -// the CSRF nonce + PKCE verifier across the redirect to Google and back. It differs -// only in scope (https://mail.google.com/ for SMTP XOAUTH2) and access_type=offline -// + prompt=consent, which guarantee a refresh token even on reconnect. +// Gmail OAuth2 and its consent flow were removed in engagement Phase 1 +// (ENGAGEMENT.md §1.2a). What went with it: two routes, the `email_oauth_tx` +// signed cookie, the PKCE verifier and CSRF nonce plumbing, the +// `https://mail.google.com/` scope, and the borrowed `google` auth-providers +// client. That last one was a real coupling — an admin rotating the Google SSO +// secret silently broke outbound mail, with nothing on either screen relating the +// two — and removing it is one of the better side effects of the decision. +// +// **The form is driven by the transport's `credentialFields`, not by this file.** +// getConfig ships the declarations to the client, saveConfig hands the submitted +// body to the registry's sanitizer, and neither one names a field. Adding a +// transport is a registration, not an edit here. const emailConfig = require('../../../model/emailConfig/emailConfig.model') -const authProviders = require('../../../model/authProviders/authProviders.model') const activity = require('../../../model/activity/activity.model') const mailer = require('../../../utils/mailer') -const GoogleProvider = require('../../../auth/providers/google.provider') -const ssoState = require('../../../auth/ssoState') -const token = require('../../../auth/token') +const { transports } = require('../../../engagement') const log = require('../../../utils/logger')('admin') -// Gmail scope grants SMTP (XOAUTH2) access; openid+email let us read back which -// address was connected. The narrower gmail.send scope only works via the Gmail -// API, not SMTP, so we need the full-access scope here. -const EMAIL_SCOPE = 'https://mail.google.com/ openid email' -const TX_COOKIE = 'email_oauth_tx' - -// Public base URL for the OAuth redirect_uri — same fallback pattern as -// sso.controller.js. Must be identical between start and callback. -function appBaseUrl(req) { - const configured = process.env.APP_BASE_URL - if (configured) return configured.replace(/\/+$/, '') - const derived = `${req.protocol}://${req.get('host')}` - log.warn('APP_BASE_URL not set — deriving email redirect_uri from the request', { derived }) - return derived -} -function redirectUri(req) { - return `${appBaseUrl(req)}/api/v1/admin/email/connect/callback` -} -function txCookieOptions(req) { - return { ...token.cookieOptions(req), maxAge: 10 * 60 * 1000 } -} - -// Front-end redirect targets after the callback resolves. -const CONNECTED_URL = '/admin/settings?email_connected=1' -const errorUrl = (code) => `/admin/settings?email_error=${code}` - -// Load the Google OAuth client (id + decrypted secret) reused for email. Returns -// null when the google provider hasn't been configured with credentials yet. -async function googleClient() { - const row = await authProviders.getWithSecret('google') - if (!row || !row.client_id || !row.client_secret) return null - return { clientId: row.client_id, clientSecret: row.client_secret } -} - // GET /admin/email/config async function getConfig(req, res) { try { const config = await emailConfig.getSafe() - // Surface whether the Google client email can borrow is configured, so the - // UI can explain why Connect is unavailable. - config.googleConfigured = Boolean(await googleClient()) + // The transport catalog rides with the config so the client renders the + // credential form from the same declarations the server validates against. + config.transports = transports.describe() return res.json(config) } catch (err) { log.error('emailConfig.getConfig', err) @@ -69,23 +39,50 @@ async function getConfig(req, res) { } } -// PUT /admin/email/config — sender name + enabled toggle. Enabling requires a -// connected mailbox (a stored refresh token). +// PUT /admin/email/config — transport, sender identity, credentials, enabled. +// +// Enabling requires a complete credential AND a sender address, checked against +// the state as it will be AFTER this save rather than before it: the admin fills +// the whole form and ticks Enable in one submit, and refusing that because the +// credential was absent a moment ago would make the screen impossible to use. async function saveConfig(req, res) { - const { senderName, enabled } = req.body + const { transport, senderEmail, senderName, replyTo, credential, enabled } = req.body try { const current = await emailConfig.getSafe() - if (enabled && !current.hasRefreshToken) { - return res.status(400).json({ message: 'Connect a Gmail account before enabling email.' }) + const nextTransport = transport === undefined ? current.transport : transport + if (!transports.has(nextTransport)) { + return res.status(400).json({ message: `Unknown mail transport "${nextTransport}".` }) } + const saved = await emailConfig.save({ + transport: transport !== undefined ? nextTransport : undefined, + senderEmail: senderEmail !== undefined ? senderEmail || null : undefined, senderName: senderName !== undefined ? senderName || null : undefined, - enabled, + replyTo: replyTo !== undefined ? replyTo || null : undefined, + credential, + // Enabling is applied only once the saved row can actually support it, so + // the response tells the truth about what is stored rather than echoing the + // request. The re-read below is what decides. + enabled: enabled === undefined ? undefined : Boolean(enabled), updatedBy: req.user.id, }) - saved.googleConfigured = Boolean(await googleClient()) - await activity.log({ req, action: 'email.config.update', detail: { enabled: saved.enabled } }) - log.info('email config updated', { by: req.user.username, enabled: saved.enabled }) + + if (saved.enabled && !(saved.hasCredential && saved.senderEmail)) { + const reverted = await emailConfig.save({ enabled: false, updatedBy: req.user.id }) + reverted.transports = transports.describe() + return res.status(400).json({ + message: 'Add complete credentials and a sender address before enabling email.', + config: reverted, + }) + } + + saved.transports = transports.describe() + await activity.log({ + req, + action: 'email.config.update', + detail: { transport: saved.transport, enabled: saved.enabled, hasCredential: saved.hasCredential }, + }) + log.info('email config updated', { by: req.user.username, transport: saved.transport, enabled: saved.enabled }) return res.json(saved) } catch (err) { log.error('emailConfig.saveConfig', err) @@ -93,96 +90,14 @@ async function saveConfig(req, res) { } } -// GET /admin/email/connect/start — returns { url } for the browser to navigate to. -async function connectStart(req, res) { - try { - const client = await googleClient() - if (!client) { - return res.status(400).json({ - message: 'Configure the Google authentication provider (client id + secret) before connecting email.', - }) - } - const provider = new GoogleProvider({ clientId: client.clientId, clientSecret: client.clientSecret }) - const tx = ssoState.createTx({ flow: 'email' }) - res.cookie(TX_COOKIE, tx.txToken, txCookieOptions(req)) - - const params = new URLSearchParams({ - client_id: client.clientId, - redirect_uri: redirectUri(req), - response_type: 'code', - scope: EMAIL_SCOPE, - access_type: 'offline', - prompt: 'consent', - include_granted_scopes: 'true', - state: tx.nonce, - code_challenge: tx.codeChallenge, - code_challenge_method: 'S256', - }) - const url = `${provider.authEndpoint()}?${params.toString()}` - return res.json({ url }) - } catch (err) { - log.error('emailConfig.connectStart', err) - return res.status(500).json({ message: 'Internal Server Error' }) - } -} - -// GET /admin/email/connect/callback — exchange the code, capture the refresh -// token + connected address, store encrypted, and redirect back to Settings. -async function connectCallback(req, res) { - const txToken = req.cookies && req.cookies[TX_COOKIE] - const { code, state, error: oauthError } = req.query - res.clearCookie(TX_COOKIE, token.cookieOptions(req)) // single-use - - if (oauthError) { - log.warn('email connect: provider returned error', { error: String(oauthError).slice(0, 60) }) - return res.redirect(errorUrl('denied')) - } - const tx = ssoState.verifyTx(txToken, state) - if (!tx || tx.flow !== 'email' || !code) { - log.warn('email connect: bad state') - return res.redirect(errorUrl('bad_state')) - } - try { - const client = await googleClient() - if (!client) return res.redirect(errorUrl('no_client')) - const provider = new GoogleProvider({ clientId: client.clientId, clientSecret: client.clientSecret }) - - const tokenSet = await provider.exchangeCode({ - code, - redirectUri: redirectUri(req), - codeVerifier: tx.verifier, - }) - if (!tokenSet.refresh_token) { - // Google only returns a refresh token when it hasn't already granted one - // for this client+scope. prompt=consent should force it; if it's still - // missing the admin can revoke the app's access and retry. - log.warn('email connect: no refresh_token returned') - return res.redirect(errorUrl('no_refresh_token')) - } - const profile = await provider.getUserProfile(tokenSet.access_token) - const senderEmail = profile.email || null - if (!senderEmail) return res.redirect(errorUrl('no_email')) - - await emailConfig.save({ - senderEmail, - refreshToken: tokenSet.refresh_token, - enabled: true, - status: 'connected', - statusDetail: 'Connected', - updatedBy: req.user.id, - }) - await emailConfig.recordStatus({ status: 'connected', statusDetail: 'Connected', lastVerifiedAt: new Date() }) - await activity.log({ req, action: 'email.connect', detail: { senderEmail } }) - log.info('email connected', { senderEmail, by: req.user.username }) - return res.redirect(CONNECTED_URL) - } catch (err) { - log.error('emailConfig.connectCallback', err) - return res.redirect(errorUrl('error')) - } -} - // POST /admin/email/test — send a test message (to the given address, or the // contact recipient by default). +// +// This is the only verification the configuration gets. Under the removed consent +// flow the sending address came back from Google and was guaranteed to belong to +// the credential; an operator-typed sender the relay will not accept is a silent +// deliverability failure, so mailer.describeSendError puts the sender in the +// message and this hands it through verbatim. async function testSend(req, res) { try { const result = await mailer.sendTest(req.body.to) @@ -198,9 +113,9 @@ async function testSend(req, res) { async function disconnect(req, res) { try { const config = await emailConfig.disconnect(req.user.id) - config.googleConfigured = Boolean(await googleClient()) + config.transports = transports.describe() await activity.log({ req, action: 'email.disconnect' }) - log.info('email disconnected', { by: req.user.username }) + log.info('email credentials cleared', { by: req.user.username }) return res.json(config) } catch (err) { log.error('emailConfig.disconnect', err) @@ -208,4 +123,4 @@ async function disconnect(req, res) { } } -module.exports = { getConfig, saveConfig, connectStart, connectCallback, testSend, disconnect } +module.exports = { getConfig, saveConfig, testSend, disconnect } diff --git a/server/src/server.js b/server/src/server.js index 6a99914..fccd480 100644 --- a/server/src/server.js +++ b/server/src/server.js @@ -43,7 +43,7 @@ async function start() { logFile: createLogger.logFilePath || 'disabled (console only)', db: `${process.env.DB_HOST || '127.0.0.1'}:${process.env.DB_PORT || 3306}/${process.env.DB_NAME || 'runic_gateway'}`, cookieSecure: process.env.COOKIE_SECURE || 'auto', - email: 'gmail-oauth2 (configured in admin → settings)', + email: 'smtp (configured in admin → settings)', }) // Fail fast if the server<->bot shared secret is weak/placeholder. Fatal in diff --git a/server/src/utils/mailer.js b/server/src/utils/mailer.js index a8764ac..dc37ba4 100644 --- a/server/src/utils/mailer.js +++ b/server/src/utils/mailer.js @@ -1,65 +1,93 @@ -// ── Outbound mail (Gmail over OAuth2 / SMTP XOAUTH2) ─────────────────────── +// ── Outbound mail ────────────────────────────────────────────────────────── // // Email is configured in Admin → Settings → Email, not via env vars. The -// connection (enabled flag, connected Gmail address, encrypted refresh token) -// lives in the email_config singleton; the OAuth client id/secret are reused -// from the `google` auth_providers row. nodemailer takes the refresh token and -// auto-mints short-lived access tokens for each send. +// `email_config` singleton holds the enabled flag, the sender identity and the +// AES-GCM-encrypted credential for whichever transport is selected; the transport +// itself is a registration in `src/engagement/transports` (ENGAGEMENT.md §3.1), +// so which provider is used is DATA, not a code path in this file. // -// When email is not configured, sendContactMessage does NOT throw — it signals -// the caller to fall back to a mailto: link (the contact form relies on this). - -const nodemailer = require('nodemailer') +// Gmail OAuth2 was removed in engagement Phase 1 (§1.2a). Gmail is still reachable +// as an ordinary SMTP relay (`smtp.gmail.com:587` with an app password) — the +// operator types those in like any other host; nothing in here knows about it. +// +// **The failure contracts are the point of this file.** Six call sites, five +// senders, and each one degrades a specific way when mail is unconfigured. Those +// contracts are unchanged by the transport rewrite and are asserted in +// test/mailer.test.js: sendContactMessage returns a mailto fallback, sendInvite +// and sendPasswordReset return { sent: false, reason: 'NOT_CONFIGURED' } so their +// callers can surface a link / answer a generic 200, sendTeamNotification never +// throws at all, and only sendTest throws — because only sendTest has an admin +// waiting to be told why. const emailConfig = require('../model/emailConfig/emailConfig.model') -const authProviders = require('../model/authProviders/authProviders.model') const settings = require('../model/settings/settings.model') +const { transports } = require('../engagement') const brand = require('../config/brand') const log = require('./logger')('mailer') -// Ready to send only when enabled, connected (has a refresh token), and we know -// which address to send as. +// Ready to send only when enabled, holding a complete credential for a +// registered transport, and knowing which address to send as. async function isConfigured() { const c = await emailConfig.getSafe() - return Boolean(c.enabled && c.hasRefreshToken && c.senderEmail) + return Boolean(c.enabled && c.hasCredential && c.senderEmail) } // Recipient for the contact form: the admin-editable contact_email setting, or -// the connected sending address as a last resort. +// the configured sending address as a last resort. async function contactRecipient(senderEmail) { const to = await settings.get('contact_email') return to || senderEmail || null } -// Build a nodemailer OAuth2 transport from the stored config + reused Google -// client credentials. Returns { transport, config } or null when unconfigured. +/** + * Build a transport from the stored config. Returns { transport, config } or + * null when unconfigured — every sender handles null itself. + * + * Null, never a throw, for all five ways this can fail: no row, disabled, no + * sender address, an incomplete credential, or a stored transport id that is not + * registered (a downgrade, or a provider removed from the build). The last one is + * the reason `transports.get()` is checked rather than assumed: a send-time + * exception from an unknown id would break the contact form for a reason the + * admin screen already shows. + * + * **`enabled` is checked here now, and it was not before.** Under the connect + * flow this function tested only "is there a refresh token and a sender", so the + * contact form kept sending after an admin unticked "Enable email sending" — + * `isConfigured()` honoured the toggle but the direct senders bypassed it. With + * the toggle no longer set as a side effect of a consent redirect it has to mean + * what it says, so the gate lives on the one path every sender shares. + */ async function buildTransport() { const config = await emailConfig.getWithSecret() - if (!config || !config.refreshToken || !config.senderEmail) return null - const google = await authProviders.getWithSecret('google') - if (!google || !google.client_id || !google.client_secret) { - log.warn('email send skipped: Google OAuth client is not configured') + if (!config || !config.enabled || !config.senderEmail) return null + const def = transports.get(config.transport) + if (!def) { + log.warn('email send skipped: no such transport', { transport: config.transport }) + return null + } + if (!transports.isComplete(config.transport, config.credentialSecret)) { + log.warn('email send skipped: transport credentials are incomplete', { transport: config.transport }) + return null + } + try { + return { transport: def.build(config.credentialSecret, config), config } + } catch (err) { + log.error('could not build the mail transport', err) return null } - const transport = nodemailer.createTransport({ - host: 'smtp.gmail.com', - port: 465, - secure: true, - auth: { - type: 'OAuth2', - user: config.senderEmail, - clientId: google.client_id, - clientSecret: google.client_secret, - refreshToken: config.refreshToken, - }, - }) - return { transport, config } } function fromHeader(config) { return config.senderName ? `"${config.senderName}" <${config.senderEmail}>` : config.senderEmail } +// Reply-To is the operator's optional override; the contact form's per-message +// replyTo (the visitor's address) wins over it, which is the whole reason the +// contact form has one. +function replyToFor(config, override) { + return override || config.replyTo || undefined +} + /** * Send a contact message. If email is not configured/enabled, signals the caller * to fall back to a mailto: link instead of throwing. @@ -76,7 +104,7 @@ async function sendContactMessage({ name, email, message }) { await transport.sendMail({ from: fromHeader(config), to, - replyTo: email, + replyTo: replyToFor(config, email), subject: `${brand.name} contact from ${name || 'a visitor'}`, text: `From: ${name || 'unknown'} <${email || 'no email'}>\n\n${message}`, }) @@ -84,19 +112,49 @@ async function sendContactMessage({ name, email, message }) { return { sent: true } } catch (err) { log.error('contact send failed', err) - await emailConfig.recordStatus({ status: 'error', statusDetail: err.message }) + await emailConfig.recordStatus({ status: 'error', statusDetail: describeSendError(err) }) throw err } } +/** + * Turn a transport error into something an operator can act on. + * + * This matters more than it used to. Under Gmail OAuth2 the sending address came + * back from Google's userinfo and was guaranteed to be a mailbox the credential + * owned. Under SMTP `sender_email` is operator-typed, so a relay rejecting the + * envelope From is now a live failure mode (§1.2a consequence 2) — and it arrives + * as a bare "550 5.7.1" that means nothing without the sender in front of it. + */ +function describeSendError(err, config) { + const code = err && (err.responseCode || err.code) + const base = (err && (err.response || err.message)) || 'Send failed' + const sender = config && config.senderEmail + if (sender && (code === 550 || code === 553 || code === 554 || code === 'EENVELOPE')) { + return `${base} — the server refused "${sender}" as the sender. It must be an address this account is allowed to send as (SPF/DMARC).` + } + if (code === 'EAUTH') return `${base} — the username or password was rejected.` + if (code === 'ESOCKET' || code === 'ECONNECTION') { + return `${base} — could not connect. Check the host, the port, and whether "Implicit TLS" matches it (on for 465, off for 587).` + } + if (code === 'ETIMEDOUT') { + return `${base} — the connection timed out. A common cause is "Implicit TLS" left on for port 587.` + } + return String(base) +} + /** * Send a test email to `to`, used by the admin "Send test" button. Throws on * failure; records the outcome either way. Returns { sent: true } on success. + * + * **This is now the real verification of the whole configuration** — host, port, + * TLS mode, credentials AND whether the relay will accept the operator-typed + * sender. There is no consent flow left to prove any of it beforehand. */ async function sendTest(to) { const built = await buildTransport() if (!built) { - const err = new Error('Email is not configured. Connect Gmail first.') + const err = new Error('Email is not configured. Set a transport, its credentials and a sender address first.') err.code = 'NOT_CONFIGURED' throw err } @@ -111,15 +169,19 @@ async function sendTest(to) { await transport.sendMail({ from: fromHeader(config), to: recipient, + replyTo: replyToFor(config), subject: `${brand.name} email test`, - text: 'This is a test message confirming Gmail OAuth2 email delivery is working.', + text: `This is a test message confirming ${config.transport} email delivery is working.`, }) await emailConfig.recordStatus({ status: 'connected', statusDetail: 'Test send OK', lastVerifiedAt: new Date() }) return { sent: true, to: recipient } } catch (err) { + const detail = describeSendError(err, config) log.error('test send failed', err) - await emailConfig.recordStatus({ status: 'error', statusDetail: err.message }) - throw err + await emailConfig.recordStatus({ status: 'error', statusDetail: detail }) + const wrapped = new Error(detail) + wrapped.code = err.code || 'SEND_FAILED' + throw wrapped } } @@ -140,6 +202,7 @@ async function sendInvite({ to, acceptUrl, role, invitedByName }) { await transport.sendMail({ from: fromHeader(config), to, + replyTo: replyToFor(config), subject: `Your ${brand.name} invitation`, text: `You have been invited${by} to join ${brand.name}${roleLabel}.\n\n` + @@ -150,7 +213,7 @@ async function sendInvite({ to, acceptUrl, role, invitedByName }) { return { sent: true } } catch (err) { log.error('invite send failed', err) - await emailConfig.recordStatus({ status: 'error', statusDetail: err.message }) + await emailConfig.recordStatus({ status: 'error', statusDetail: describeSendError(err, config) }) throw err } } @@ -171,6 +234,7 @@ async function sendPasswordReset({ to, resetUrl, username }) { await transport.sendMail({ from: fromHeader(config), to, + replyTo: replyToFor(config), subject: `Reset your ${brand.name} password`, text: `We received a request to reset the password${forWhom} at ${brand.name}.\n\n` + @@ -182,7 +246,7 @@ async function sendPasswordReset({ to, resetUrl, username }) { return { sent: true } } catch (err) { log.error('password reset send failed', err) - await emailConfig.recordStatus({ status: 'error', statusDetail: err.message }) + await emailConfig.recordStatus({ status: 'error', statusDetail: describeSendError(err, config) }) throw err } } @@ -234,6 +298,7 @@ async function sendTeamNotification({ to, subject, intro, items, teamUrl, unsubs await transport.sendMail({ from: fromHeader(config), to, + replyTo: replyToFor(config), subject, text: lines.join('\n'), // The header carries the API url, not the one in the body: a one-click @@ -252,7 +317,7 @@ async function sendTeamNotification({ to, subject, intro, items, teamUrl, unsubs // called by a request that can report the failure to whoever caused it; this // one is not, and recordStatus already puts the error where an admin reads it. log.warn('team notification send failed', { message: err.message }) - await emailConfig.recordStatus({ status: 'error', statusDetail: err.message }).catch(() => {}) + await emailConfig.recordStatus({ status: 'error', statusDetail: describeSendError(err, config) }).catch(() => {}) return { sent: false, reason: 'SEND_FAILED' } } } diff --git a/server/src/utils/teamNotify.js b/server/src/utils/teamNotify.js index e4294e5..0869014 100644 --- a/server/src/utils/teamNotify.js +++ b/server/src/utils/teamNotify.js @@ -211,8 +211,8 @@ async function forumPost({ team, threadId, threadTitle, type, authorUserId, auth * exactly that. * * Skipped entirely when no email is configured — §6.4's "off unless configured" - * — and checked BEFORE the recipient query so a deployment with no Gmail - * connected pays nothing for the sink it does not have. + * — and checked BEFORE the recipient query so a deployment with no mail + * transport configured pays nothing for the sink it does not have. */ async function emailImmediate({ team, threadId, threadTitle, type, exclude, authorName, bodyHtml }) { if (!(await mailer.isConfigured())) return 0 @@ -227,8 +227,8 @@ async function emailImmediate({ team, threadId, threadTitle, type, exclude, auth for (const r of recipients) { // Serial rather than Promise.all: this is an SMTP conversation per recipient - // against a provider with its own rate limits, and a burst of them from a - // busy thread is how a Gmail sender gets throttled. The loop is also why the + // against a relay with its own rate limits, and a burst of them from a + // busy thread is how a sending account gets throttled. The loop is also why the // send below is fire-and-report rather than fire-and-throw. // eslint-disable-next-line no-await-in-loop const res = await mailer.sendTeamNotification({ diff --git a/server/test/checkNoExternalHosts.test.js b/server/test/checkNoExternalHosts.test.js new file mode 100644 index 0000000..23f0aa4 --- /dev/null +++ b/server/test/checkNoExternalHosts.test.js @@ -0,0 +1,98 @@ +// Self-test for scripts/checkNoExternalHosts.js — ENGAGEMENT.md §3.2 rule 4. +// +// The same discipline checkModuleIdentifiers.test.js established: feed the +// checker code it MUST reject and code it MUST accept, because a check that +// silently stops checking is worse than no check. The rejection cases below are +// the exact shape of the literal this phase deleted. + +const { test } = require('node:test') +const assert = require('node:assert/strict') + +const { checkFile, maskComments, isAllowed, run } = require('../../scripts/checkNoExternalHosts') + +const hostsIn = (src) => checkFile('fake.js', src).map((h) => h.host) + +test('catches the literal this phase deleted', () => { + const src = ` + const transport = nodemailer.createTransport({ + host: 'smtp.gmail.com', + port: 465, + secure: true, + }) + ` + assert.deepEqual(hostsIn(src), ['smtp.gmail.com']) +}) + +test('catches an API base url, whatever the scheme', () => { + assert.deepEqual(hostsIn(`const BASE = 'https://api.mailgun.net/v3'`), ['api.mailgun.net']) + assert.deepEqual(hostsIn(`const relay = "smtps://mail.somewhere.io:465"`), ['mail.somewhere.io']) +}) + +test('catches a default sender address', () => { + assert.deepEqual(hostsIn(`const FROM = 'noreply@runicgateway.com'`), ['runicgateway.com']) +}) + +test('catches a host in a template literal', () => { + assert.deepEqual(hostsIn('const url = `https://api.postmarkapp.com/email`'), ['api.postmarkapp.com']) +}) + +test('reports the line the literal is on', () => { + const src = ['// a comment', '', "const h = 'smtp.sendgrid.net'"].join('\n') + assert.deepEqual(checkFile('fake.js', src), [ + { file: 'fake.js', line: 3, literal: 'smtp.sendgrid.net', host: 'smtp.sendgrid.net' }, + ]) +}) + +// ── the accept cases: the whole reason it reads code, not prose ───────────── + +test('a host named in a line comment is fine — that is the documentation this phase owes', () => { + assert.deepEqual(hostsIn(`// Gmail still works as plain SMTP: smtp.gmail.com:587 with an app password\nconst x = 1`), []) +}) + +test('a host named in a block comment is fine', () => { + assert.deepEqual(hostsIn(`/*\n * See https://mailgun.com/docs for the relay posture.\n */\nconst x = 1`), []) +}) + +test('example.com placeholders are allowed — a form hint is not a destination', () => { + assert.deepEqual(hostsIn(`const f = { placeholder: 'smtp.example.com' }`), []) + assert.deepEqual(hostsIn(`const f = { placeholder: 'noreply@example.com' }`), []) +}) + +test('loopback is allowed', () => { + assert.deepEqual(hostsIn(`const dev = 'http://127.0.0.1:3000'`), []) + assert.deepEqual(hostsIn(`const dev = 'http://localhost:1025'`), []) +}) + +test('a module path is not a hostname', () => { + assert.deepEqual(hostsIn(`const m = require('../model/emailConfig/emailConfig.model')`), []) + assert.deepEqual(hostsIn(`const n = require('nodemailer')`), []) + assert.deepEqual(hostsIn(`import x from './transports/smtp.js'`), []) +}) + +test('an ordinary sentence with a full stop is not a hostname', () => { + assert.deepEqual(hostsIn(`const msg = 'Send failed. Check the host and port.'`), []) +}) + +// ── the pieces, directly ──────────────────────────────────────────────────── + +test('maskComments blanks comments but keeps string bodies and line count', () => { + const src = "// smtp.gmail.com\nconst h = 'smtp.relay.net'\n" + const masked = maskComments(src) + assert.equal(masked.split('\n').length, src.split('\n').length) + assert.ok(!masked.includes('smtp.gmail.com')) + assert.ok(masked.includes('smtp.relay.net')) +}) + +test('isAllowed covers the reserved documentation names and nothing else', () => { + assert.equal(isAllowed('example.com'), true) + assert.equal(isAllowed('mail.example.org'), true) + assert.equal(isAllowed('localhost'), true) + assert.equal(isAllowed('smtp.gmail.com'), false) + assert.equal(isAllowed('api.postmarkapp.com'), false) +}) + +// ── and the real tree ─────────────────────────────────────────────────────── + +test('the shipped engagement tree is clean', () => { + assert.deepEqual(run(), []) +}) diff --git a/server/test/emailConfig.model.test.js b/server/test/emailConfig.model.test.js index 3d62677..2a24bf9 100644 --- a/server/test/emailConfig.model.test.js +++ b/server/test/emailConfig.model.test.js @@ -1,3 +1,7 @@ +// model/emailConfig — the credential store. Same guarantees as before the Gmail +// removal (ciphertext at rest, never returned, blank means "leave it alone"), +// now over one transport-shaped blob instead of a single refresh-token column. + process.env.SECRET_ENC_KEY = process.env.SECRET_ENC_KEY || 'unit-test-enc-key' process.env.DB_HOST = '127.0.0.1' process.env.DB_PORT = '59999' @@ -12,59 +16,122 @@ const db = require('../src/utils/db') after(() => db.close()) +const CRED = { host: 'relay.example.com', port: 587, secure: false, user: 'apikey', password: 'sec-abc' } + // In-memory stand-in for the singleton row so the model never touches MariaDB. let store beforeEach(() => { store = null emailDb.get = async () => store emailDb.upsert = async (fields) => { - store = { ...(store || { id: 1 }), ...fields } + store = { ...(store || { id: 1, transport: 'smtp' }), ...fields } return store } }) -test('save encrypts the refresh token (ciphertext at rest, decryptable)', async () => { - await emailConfig.save({ senderEmail: 'me@gmail.com', refreshToken: 'refresh-abc', enabled: true }) - assert.ok(store.refresh_token_enc) - assert.notEqual(store.refresh_token_enc, 'refresh-abc') - assert.equal(secretBox.decrypt(store.refresh_token_enc), 'refresh-abc') +const decrypted = () => JSON.parse(secretBox.decrypt(store.credential_enc)) + +test('save encrypts the credential (ciphertext at rest, decryptable)', async () => { + await emailConfig.save({ senderEmail: 'mail@shard.example.com', credential: CRED, enabled: true }) + assert.ok(store.credential_enc) + assert.ok(!String(store.credential_enc).includes('sec-abc')) + assert.deepEqual(decrypted(), CRED) const withSecret = await emailConfig.getWithSecret() - assert.equal(withSecret.refreshToken, 'refresh-abc') + assert.equal(withSecret.credentialSecret.password, 'sec-abc') }) -test('getSafe never leaks the refresh token', async () => { - await emailConfig.save({ senderEmail: 'me@gmail.com', refreshToken: 'refresh-abc', enabled: true }) +test('getSafe never leaks a secret field, but says which are set', async () => { + await emailConfig.save({ senderEmail: 'mail@shard.example.com', credential: CRED, enabled: true }) const safe = await emailConfig.getSafe() - assert.equal(safe.hasRefreshToken, true) - assert.equal(safe.senderEmail, 'me@gmail.com') - assert.equal('refreshToken' in safe, false) - assert.equal('refresh_token_enc' in safe, false) + + assert.deepEqual(safe.credential, { host: 'relay.example.com', port: 587, secure: false, user: 'apikey' }) + assert.equal('password' in safe.credential, false) + assert.deepEqual(safe.secretsSet, { password: true }) + assert.equal(safe.hasCredential, true) + assert.equal(safe.senderEmail, 'mail@shard.example.com') + assert.equal('credentialSecret' in safe, false) + assert.equal('credential_enc' in safe, false) }) -test('blank refresh token on save leaves the existing one unchanged', async () => { - await emailConfig.save({ senderEmail: 'me@gmail.com', refreshToken: 'refresh-abc', enabled: true }) - const cipherBefore = store.refresh_token_enc +test('a blank secret leaves the stored one unchanged; other fields still save', async () => { + await emailConfig.save({ credential: CRED }) + const cipherBefore = store.credential_enc - await emailConfig.save({ senderName: 'UOMysticmoon' }) // no refreshToken - assert.equal(store.refresh_token_enc, cipherBefore) // untouched + await emailConfig.save({ senderName: 'UOMysticmoon', credential: { ...CRED, password: '' } }) assert.equal(store.sender_name, 'UOMysticmoon') - assert.equal(secretBox.decrypt(store.refresh_token_enc), 'refresh-abc') + assert.equal(decrypted().password, 'sec-abc') + assert.notEqual(store.credential_enc, undefined) + assert.ok(cipherBefore) }) -test('disconnect clears the credential and disables sending', async () => { - await emailConfig.save({ senderEmail: 'me@gmail.com', refreshToken: 'refresh-abc', enabled: true }) +test('undeclared keys are dropped — a client cannot smuggle fields into the blob', async () => { + await emailConfig.save({ credential: { ...CRED, evil: 'x', proxy: 'http://attacker' } }) + assert.deepEqual(Object.keys(decrypted()).sort(), ['host', 'password', 'port', 'secure', 'user']) +}) + +test('changing transport does not carry the old credential across', async () => { + await emailConfig.save({ credential: CRED }) + // An unregistered target still clears rather than merging: leaving an SMTP + // password inside another transport's blob would be a stored secret nobody can + // see and nothing will ever use. + await emailConfig.save({ transport: 'mailgun', credential: { domain: 'x' } }) + assert.equal(store.transport, 'mailgun') + assert.equal(store.credential_enc, null) +}) + +test('an incomplete credential is stored but is not "complete"', async () => { + // A username with no password authenticates as nobody. + await emailConfig.save({ credential: { host: 'relay.example.com', port: 587, user: 'apikey' } }) + const safe = await emailConfig.getSafe() + assert.equal(safe.hasCredential, false) + assert.deepEqual(safe.secretsSet, { password: false }) +}) + +test('an unreadable blob reads as absent, never as an error', async () => { + // The rotated-SECRET_ENC_KEY case. It must land the admin on a screen that says + // "unconfigured", not a 500 that takes the contact form down with it. + store = { id: 1, transport: 'smtp', credential_enc: 'not-ciphertext', enabled: 1 } + const safe = await emailConfig.getSafe() + assert.equal(safe.hasCredential, false) + assert.deepEqual(safe.credential, {}) +}) + +test('disconnect clears the credential, the legacy token and the enabled flag', async () => { + store = { id: 1, transport: 'smtp', refresh_token_enc: 'old-gmail-cipher', enabled: 1 } + await emailConfig.save({ senderEmail: 'mail@shard.example.com', credential: CRED, enabled: true }) + const safe = await emailConfig.disconnect(7) + assert.equal(store.credential_enc, null) assert.equal(store.refresh_token_enc, null) assert.equal(store.enabled, 0) - assert.equal(safe.hasRefreshToken, false) + assert.equal(safe.hasCredential, false) + assert.equal(safe.hadLegacyConnection, false) assert.equal(safe.status, 'unconfigured') }) -test('getSafe returns unconfigured defaults when no row exists', async () => { +test('hadLegacyConnection is the G22 warning condition, and nothing else', async () => { + // Present token + no replacement credential: this deployment's mail just + // stopped and it has to be told (ENGAGEMENT.md §1.2a consequence 3). + store = { id: 1, transport: 'smtp', refresh_token_enc: 'old-gmail-cipher', enabled: 1 } + let safe = await emailConfig.getSafe() + assert.equal(safe.hadLegacyConnection, true) + assert.equal(safe.hasCredential, false) + + // Once SMTP is configured the pair stops matching, so the warning goes away + // without anything having to clear the deprecated column. + await emailConfig.save({ senderEmail: 'mail@shard.example.com', credential: CRED }) + safe = await emailConfig.getSafe() + assert.equal(safe.hadLegacyConnection, true) + assert.equal(safe.hasCredential, true) +}) + +test('a fresh install is unconfigured, on the default transport, and warns nobody', async () => { const safe = await emailConfig.getSafe() assert.equal(safe.enabled, false) - assert.equal(safe.hasRefreshToken, false) + assert.equal(safe.transport, 'smtp') + assert.equal(safe.hasCredential, false) + assert.equal(safe.hadLegacyConnection, false) assert.equal(safe.status, 'unconfigured') assert.equal(safe.senderEmail, null) }) diff --git a/server/test/mailTransports.test.js b/server/test/mailTransports.test.js new file mode 100644 index 0000000..deecb13 --- /dev/null +++ b/server/test/mailTransports.test.js @@ -0,0 +1,109 @@ +// The mail transport registry (ENGAGEMENT.md §3.1) and the one transport core +// ships. The registry's job is that `credentialFields` is the single declaration +// the admin form, the sanitizer and the "is it a secret" answer all read — so +// most of what is asserted here is that nothing else knows a field name. + +process.env.SECRET_ENC_KEY = process.env.SECRET_ENC_KEY || 'unit-test-enc-key' +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, after } = require('node:test') +const assert = require('node:assert/strict') + +const { transports } = require('../src/engagement') +const db = require('../src/utils/db') + +after(() => db.close()) + +const SMTP_CRED = { host: 'relay.example.com', port: 587, secure: false, user: 'apikey', password: 'sec' } + +test('requiring the subsystem is what registers core\'s transports', () => { + assert.equal(transports.has('smtp'), true) + const [smtp] = transports.describe() + assert.equal(smtp.id, 'smtp') + assert.ok(smtp.credentialFields.length > 0) +}) + +test('describe() carries no functions and no secret values', () => { + const [smtp] = transports.describe() + assert.equal(typeof smtp.build, 'undefined') + assert.equal(typeof smtp.isComplete, 'undefined') + const password = smtp.credentialFields.find((f) => f.key === 'password') + assert.equal(password.kind, 'secret') + assert.equal('value' in password, false) +}) + +test('sanitizeCredential drops undeclared keys', () => { + const out = transports.sanitizeCredential('smtp', { ...SMTP_CRED, evil: 'x' }) + assert.deepEqual(Object.keys(out).sort(), ['host', 'password', 'port', 'secure', 'user']) +}) + +test('sanitizeCredential coerces to the declared kind', () => { + const out = transports.sanitizeCredential('smtp', { host: 'relay.example.com', port: '587', secure: 'yes' }) + assert.equal(out.port, 587) + assert.equal(out.secure, true) +}) + +test('an empty secret is omitted, so a merge keeps the stored one', () => { + const patch = transports.sanitizeCredential('smtp', { ...SMTP_CRED, password: '' }) + assert.equal('password' in patch, false) + const merged = transports.mergeCredential('smtp', { password: 'stored' }, patch) + assert.equal(merged.password, 'stored') +}) + +test('publicCredential and secretsPresent split the blob the way the API needs', () => { + assert.deepEqual(transports.publicCredential('smtp', SMTP_CRED), { + host: 'relay.example.com', port: 587, secure: false, user: 'apikey', + }) + assert.deepEqual(transports.secretsPresent('smtp', SMTP_CRED), { password: true }) + assert.deepEqual(transports.secretsPresent('smtp', { host: 'x' }), { password: false }) +}) + +test('an unknown transport is a safe no-op everywhere, never a throw', () => { + assert.equal(transports.get('nope'), null) + assert.equal(transports.isComplete('nope', SMTP_CRED), false) + assert.deepEqual(transports.sanitizeCredential('nope', SMTP_CRED), {}) + assert.deepEqual(transports.publicCredential('nope', SMTP_CRED), {}) + assert.deepEqual(transports.secretsPresent('nope', SMTP_CRED), {}) +}) + +// ── smtp's own completeness rule ──────────────────────────────────────────── + +test('smtp needs a destination, and auth is all-or-nothing', () => { + assert.equal(transports.isComplete('smtp', SMTP_CRED), true) + // A local MTA needs no credentials at all. + assert.equal(transports.isComplete('smtp', { host: 'mta.example.com', port: 25 }), true) + // A username with no password authenticates as nobody and fails at the server. + assert.equal(transports.isComplete('smtp', { host: 'relay.example.com', port: 587, user: 'apikey' }), false) + assert.equal(transports.isComplete('smtp', { port: 587 }), false) + assert.equal(transports.isComplete('smtp', {}), false) +}) + +test('smtp declares no default host — §3.2 rule 1, as a test', () => { + const [smtp] = transports.describe() + const host = smtp.credentialFields.find((f) => f.key === 'host') + assert.equal(host.default, null) + assert.equal(host.required, true) +}) + +// ── registration is validated at the call ─────────────────────────────────── + +test('registration rejects a bad shape and a collision', () => { + const ok = { id: 'fake', label: 'Fake', credentialFields: [{ key: 'k', kind: 'text' }], build: () => {}, isComplete: () => true } + assert.throws(() => transports.registerMailTransport({ ...ok, id: 'Not Valid' }), /invalid id/) + assert.throws(() => transports.registerMailTransport({ ...ok, label: '' }), /label required/) + assert.throws(() => transports.registerMailTransport({ ...ok, credentialFields: [] }), /credentialFields required/) + assert.throws(() => transports.registerMailTransport({ ...ok, credentialFields: [{ key: 'k', kind: 'wat' }] }), /unknown kind/) + assert.throws(() => transports.registerMailTransport({ ...ok, build: undefined }), /build\(\) required/) + assert.throws(() => transports.registerMailTransport({ ...ok, id: 'smtp' }), /already registered/) +}) + +test('a registered transport is a copy — a caller cannot mutate the catalog afterwards', () => { + const fields = [{ key: 'k', label: 'K', kind: 'text', required: true }] + transports.registerMailTransport({ + id: 'tamper', label: 'Tamper', credentialFields: fields, build: () => {}, isComplete: () => true, + }) + fields[0].kind = 'secret' + const def = transports.describe().find((t) => t.id === 'tamper') + assert.equal(def.credentialFields[0].kind, 'text') +}) diff --git a/server/test/mailer.test.js b/server/test/mailer.test.js index 0807f3c..a548d44 100644 --- a/server/test/mailer.test.js +++ b/server/test/mailer.test.js @@ -1,3 +1,11 @@ +// utils/mailer.js — the transport resolution and, more importantly, the five +// failure contracts the six call sites depend on (ENGAGEMENT.md §1.2, Phase 1). +// +// The Gmail OAuth2 assertions are gone with the transport (§1.2a); what replaced +// them asserts the SAME things at the same seam — that a configured deployment +// builds the transport the operator selected from the credentials they supplied, +// and that an unconfigured one degrades exactly as before rather than throwing. + process.env.SECRET_ENC_KEY = process.env.SECRET_ENC_KEY || 'unit-test-enc-key' process.env.DB_HOST = '127.0.0.1' process.env.DB_PORT = '59999' @@ -7,66 +15,167 @@ const assert = require('node:assert/strict') const nodemailer = require('nodemailer') const emailConfig = require('../src/model/emailConfig/emailConfig.model') -const authProviders = require('../src/model/authProviders/authProviders.model') const settings = require('../src/model/settings/settings.model') const mailer = require('../src/utils/mailer') const db = require('../src/utils/db') after(() => db.close()) -// Restore a clean slate of stubs before each test. -beforeEach(() => { - emailConfig.recordStatus = async () => {} - settings.get = async () => 'contact@example.com' +// A complete SMTP config, as getWithSecret returns it. +const configured = (over = {}) => ({ + transport: 'smtp', + enabled: true, + senderEmail: 'mail@shard.example.com', + senderName: 'UOMysticmoon', + replyTo: null, + hasCredential: true, + credentialSecret: { host: 'relay.example.com', port: 587, secure: false, user: 'apikey', password: 'sec' }, + ...over, }) -test('unconfigured → mailto fallback (never throws)', async () => { +let sent +let transportCfg + +beforeEach(() => { + sent = null + transportCfg = null + emailConfig.recordStatus = async () => {} + settings.get = async () => 'contact@example.com' + nodemailer.createTransport = (cfg) => { + transportCfg = cfg + return { sendMail: async (opts) => { sent = opts; return { messageId: '1' } } } + } +}) + +// ── the five failure contracts ────────────────────────────────────────────── + +test('unconfigured → contact form falls back to mailto (never throws)', async () => { emailConfig.getWithSecret = async () => null - emailConfig.getSafe = async () => ({ senderEmail: null, hasRefreshToken: false, enabled: false }) + emailConfig.getSafe = async () => ({ senderEmail: null, hasCredential: false, enabled: false }) const r = await mailer.sendContactMessage({ name: 'Ann', email: 'ann@player.com', message: 'hi' }) assert.deepEqual(r, { sent: false, fallback: 'mailto', email: 'contact@example.com' }) }) -test('configured → builds a Gmail OAuth2 transport and sends', async () => { - let transportCfg = null - let sent = null - nodemailer.createTransport = (cfg) => { - transportCfg = cfg - return { sendMail: async (opts) => { sent = opts; return { messageId: '1' } } } - } - emailConfig.getWithSecret = async () => ({ refreshToken: 'rt-123', senderEmail: 'shard@gmail.com', senderName: 'UOMysticmoon' }) - authProviders.getWithSecret = async (id) => { - assert.equal(id, 'google') - return { client_id: 'cid', client_secret: 'csec' } - } +test('unconfigured → invite returns NOT_CONFIGURED so the admin gets the link', async () => { + emailConfig.getWithSecret = async () => null + const r = await mailer.sendInvite({ to: 'a@b.com', acceptUrl: 'https://x/y' }) + assert.deepEqual(r, { sent: false, reason: 'NOT_CONFIGURED' }) +}) + +test('unconfigured → password reset returns NOT_CONFIGURED (caller still answers 200)', async () => { + emailConfig.getWithSecret = async () => null + const r = await mailer.sendPasswordReset({ to: 'a@b.com', resetUrl: 'https://x/y', username: 'ann' }) + assert.deepEqual(r, { sent: false, reason: 'NOT_CONFIGURED' }) +}) + +test('unconfigured → team notification returns NOT_CONFIGURED and never throws', async () => { + emailConfig.getWithSecret = async () => null + const r = await mailer.sendTeamNotification({ to: 'a@b.com', subject: 's', intro: 'i', items: [] }) + assert.deepEqual(r, { sent: false, reason: 'NOT_CONFIGURED' }) +}) + +test('unconfigured → only sendTest throws, because only sendTest has an admin waiting', async () => { + emailConfig.getWithSecret = async () => null + await assert.rejects(() => mailer.sendTest('a@b.com'), (err) => err.code === 'NOT_CONFIGURED') +}) + +// ── transport resolution ──────────────────────────────────────────────────── + +test('configured → builds the selected transport from the stored credential and sends', async () => { + emailConfig.getWithSecret = async () => configured() const r = await mailer.sendContactMessage({ name: 'Ann', email: 'ann@player.com', message: 'hi there' }) assert.equal(r.sent, true) - // Transport is Gmail SMTP over XOAUTH2 with the reused Google client + stored refresh token. - assert.equal(transportCfg.host, 'smtp.gmail.com') - assert.equal(transportCfg.port, 465) - assert.equal(transportCfg.secure, true) - assert.equal(transportCfg.auth.type, 'OAuth2') - assert.equal(transportCfg.auth.user, 'shard@gmail.com') - assert.equal(transportCfg.auth.clientId, 'cid') - assert.equal(transportCfg.auth.clientSecret, 'csec') - assert.equal(transportCfg.auth.refreshToken, 'rt-123') + // Every connection detail comes from the operator's credential. Nothing in the + // code chooses a host, a port or a TLS mode (§3.2 rule 1). + assert.equal(transportCfg.host, 'relay.example.com') + assert.equal(transportCfg.port, 587) + assert.equal(transportCfg.secure, false) + assert.deepEqual(transportCfg.auth, { user: 'apikey', pass: 'sec' }) - // From uses the display name; To is the contact_email setting; replyTo is the sender. - assert.equal(sent.from, '"UOMysticmoon" ') + // From uses the display name; To is the contact_email setting; replyTo is the + // visitor, which still wins over a configured Reply-To. + assert.equal(sent.from, '"UOMysticmoon" ') assert.equal(sent.to, 'contact@example.com') assert.equal(sent.replyTo, 'ann@player.com') }) +test('an unauthenticated relay gets no auth block', async () => { + emailConfig.getWithSecret = async () => configured({ + credentialSecret: { host: 'mta.example.com', port: 25, secure: false }, + }) + await mailer.sendInvite({ to: 'a@b.com', acceptUrl: 'https://x/y' }) + assert.equal(transportCfg.auth, undefined) +}) + +test('the configured Reply-To is used when the caller has none', async () => { + emailConfig.getWithSecret = async () => configured({ replyTo: 'staff@shard.example.com' }) + await mailer.sendPasswordReset({ to: 'a@b.com', resetUrl: 'https://x/y', username: 'ann' }) + assert.equal(sent.replyTo, 'staff@shard.example.com') +}) + +test('disabled is unconfigured — the toggle gates every sender, not just isConfigured', async () => { + emailConfig.getWithSecret = async () => configured({ enabled: false }) + emailConfig.getSafe = async () => ({ senderEmail: 'mail@shard.example.com', hasCredential: true, enabled: false }) + + const r = await mailer.sendContactMessage({ name: 'A', email: 'a@b.com', message: 'x' }) + assert.equal(r.sent, false) + assert.equal(r.fallback, 'mailto') +}) + +test('an incomplete credential is unconfigured, not a crash', async () => { + // A username with no password authenticates as nobody; smtp.isComplete says no. + emailConfig.getWithSecret = async () => configured({ + credentialSecret: { host: 'relay.example.com', port: 587, user: 'apikey' }, + }) + const r = await mailer.sendInvite({ to: 'a@b.com', acceptUrl: 'https://x/y' }) + assert.deepEqual(r, { sent: false, reason: 'NOT_CONFIGURED' }) +}) + +test('a stored transport id that is not registered degrades, it does not throw', async () => { + emailConfig.getWithSecret = async () => configured({ transport: 'mailgun' }) + const r = await mailer.sendTeamNotification({ to: 'a@b.com', subject: 's', intro: 'i', items: [] }) + assert.deepEqual(r, { sent: false, reason: 'NOT_CONFIGURED' }) +}) + +// ── failures ──────────────────────────────────────────────────────────────── + test('send failure propagates and is recorded', async () => { let recorded = null emailConfig.recordStatus = async (s) => { recorded = s } nodemailer.createTransport = () => ({ sendMail: async () => { throw new Error('smtp boom') } }) - emailConfig.getWithSecret = async () => ({ refreshToken: 'rt', senderEmail: 'shard@gmail.com', senderName: null }) - authProviders.getWithSecret = async () => ({ client_id: 'cid', client_secret: 'csec' }) + emailConfig.getWithSecret = async () => configured() await assert.rejects(() => mailer.sendContactMessage({ name: 'A', email: 'a@b.com', message: 'x' }), /smtp boom/) assert.equal(recorded.status, 'error') }) + +test('a rejected sender is diagnosed by name — the failure mode SMTP introduces', async () => { + // Under the removed consent flow the sender came back from the provider and was + // guaranteed to belong to the credential. Operator-typed, it can be refused, + // and "550 5.7.1" alone does not tell anyone why (§1.2a consequence 2). + let recorded = null + emailConfig.recordStatus = async (s) => { recorded = s } + nodemailer.createTransport = () => ({ + sendMail: async () => { + const err = new Error('Sender address rejected') + err.responseCode = 550 + throw err + }, + }) + emailConfig.getWithSecret = async () => configured() + + await assert.rejects(() => mailer.sendTest('a@b.com'), /mail@shard\.example\.com/) + assert.match(recorded.statusDetail, /SPF\/DMARC/) +}) + +test('a team notification failure is swallowed, never thrown', async () => { + emailConfig.recordStatus = async () => {} + nodemailer.createTransport = () => ({ sendMail: async () => { throw new Error('relay down') } }) + emailConfig.getWithSecret = async () => configured() + + const r = await mailer.sendTeamNotification({ to: 'a@b.com', subject: 's', intro: 'i', items: [] }) + assert.deepEqual(r, { sent: false, reason: 'SEND_FAILED' }) +})