feat(email): engagement Phase 1 — remove Gmail OAuth2, SMTP behind a transport registry #165

Merged

whitlocktech merged 2 commits from feat/engagement-phase-1-smtp into edge 2026-08-29 02:09:46 +00:00

Conversation 0 Commits 2 Files Changed 30 +1599 -594

wtclaude commented 2026-08-29 02:02:11 +00:00

Member

Copy Link

What & why

Engagement Phase 1 (ENGAGEMENT.md §1.2a, §3.1, §3.2 — scope decision 4). A subtraction and a replacement in one PR, because leaving the OAuth2 flow half-wired across a release is worse than either end state.

Stacked on #164. That PR fixes npm run swagger, which cannot run on edge at all. Merge it first; this branch contains its commit and the diff collapses once it lands.

Companion docs PRs: docs#178 and runicgateway.com#21. Land together.

Deleted — §1.2a's inventory in full

Routes	GET /admin/email/connect/start, GET /admin/email/connect/callback
Controller	connectStart / connectCallback, the email_oauth_tx signed cookie, the PKCE verifier and CSRF nonce plumbing, EMAIL_SCOPE, googleClient() — about half the file
Transport	the OAuth2 nodemailer transport and its smtp.gmail.com / 465 literals
Model	the refreshToken decrypt
Client	the Connect Gmail button and handler, the ?email_connected / ?email_error banner, six Gmail-specific error strings

provider and refresh_token_enc stay as columns under the additive-only discipline, unread as configuration.

SSO is unaffected, and one real coupling goes with this: email borrowed the google provider's client id and secret, so rotating the SSO secret silently broke outbound mail with nothing on either screen relating the two.

Added

A mail transport registry (server/src/engagement/transports/) with smtp as the sole registration. credentialFields is the single declaration that the admin form renders, the request sanitizer filters against, and the "is this secret" answer comes from — so adding a relay is a registration, not four edits across a form, a validator, a column set and a model.

Only that half of §3.1 is built. registerDeliveryChannel is not here: Phase 1 has no consumer for addressFor/render/deliver (that is the Phase 4 engine), and a registered channel nothing calls is a shape frozen before anything tried to use it.

email_config gains transport, credential_enc and reply_to. The credential is one encrypted JSON blob, not a column per field, because the field list belongs to the transport — a column per union member would make each new transport a schema change. A blob that will not decrypt reads as absent rather than raising, so a rotated SECRET_ENC_KEY lands an admin on an "unconfigured" screen instead of a 500 that takes the contact form with it.

Send test becomes the real verification. Under the consent flow the sender came back from Google's userinfo and was guaranteed to be a mailbox the credential owned. Operator-typed, it can be refused — a silent SPF/DMARC failure, not an error — so failures now name the sender and the likely cause instead of passing a bare 550 through.

One deliberate behaviour change

enabled now gates every sender. buildTransport() used to test 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 five direct senders bypassed it. The connect flow used to set enabled as a side effect of a consent redirect; with a credential form it has to mean what it says, so the gate moved onto the one path every sender shares. Flagged because it is a change, not a refactor.

G22 — the silent degradation, and the warning it earns

On upgrade transport backfills to smtp with no credentials, and every sink politely does nothing: contact → mailto, invites → copyable link, resets → generic 200, Team notifications logged and dropped. Nothing breaks loudly, which is exactly the risk.

So the admin dashboard now returns warnings[], with one entry today. The condition is narrow on purpose: the deprecated Gmail token present and no replacement credential — the one deployment this happens to. A fresh install has never had mail and is not nagged; a banner people learn to ignore is worth nothing, and this one has to be believed exactly once. It clears itself when a credential is saved.

Also: a one-time, self-limiting UPDATE corrects the status = 'connected' that an upgraded row would otherwise still be claiming.

Guardrails

• npm run check:hosts is new — §3.2 rule 4. No transport may ship a default host, endpoint or sender. It reads code, not prose (documentation naming smtp.gmail.com is fine; a literal in a transport is not) and has its own self-test that feeds it code it must reject, the discipline checkModuleIdentifiers.test.js established. Wired into pr-checks before the install, alongside check:modules.
• Swagger regenerated (2 routes removed, 3 annotations rewritten, warnings[] documented); routes.manifest.json and routes.guards.json regenerated, delta exactly −2 routes.

How it was tested

• npm test --prefix server — 1203 passing, 0 failing. 31 new tests across mailTransports.test.js (new), mailer.test.js and emailConfig.model.test.js (rewritten around the transport seam, keeping every failure-contract assertion), and checkNoExternalHosts.test.js (new).
  • One flake worth naming: an earlier full run reported mailTransports.test.js as a bare file-level 'test failed' with no failing subtest, while a Docker container, an Astro build and a Vite build were running alongside it. Not reproducible — green in isolation, green in a four-file group, and green on a clean re-run of the whole suite. Reporting it rather than quietly re-running.
• npm test --prefix client — 288 passing. npm run build --prefix client — clean.
• npm run check:hosts, npm run check:modules, npm run routes:manifest -- --check — all clean.
• The five failure contracts are asserted individually, unconfigured and configured: mailto fallback, invite link, generic 200, never-throws, and the one sender that does throw.

Not yet exercised against a live SMTP server. The transport build is asserted against a stubbed nodemailer; a real send is a rig walk and is worth doing before this reaches the deployment.

Checklist

• I have read CONTRIBUTING.md.
• The change builds and existing tests/checks pass locally.
• I have added or updated tests/docs where it makes sense.
• My commits are reasonably scoped with clear messages.

AI-assisted contributions (required)

• AI tools were used. Tool(s): Claude Code. I have reviewed and understand every change, and take responsibility for it. AI-authored commits are marked with a Co-Authored-By trailer.

License

• I agree that my contribution is licensed under this project's license (GNU GPL v3.0 or later), and I have the right to contribute it.

## What & why Engagement **Phase 1** ([`ENGAGEMENT.md`](https://gitea.whitlocktech.com/RunicGateway/docs/src/branch/edge/website/ENGAGEMENT.md) §1.2a, §3.1, §3.2 — scope decision 4). A subtraction and a replacement in one PR, because leaving the OAuth2 flow half-wired across a release is worse than either end state. > **Stacked on #164.** That PR fixes `npm run swagger`, which cannot run on `edge` at all. Merge it first; this branch contains its commit and the diff collapses once it lands. > > **Companion docs PRs: docs#178 and runicgateway.com#21.** Land together. ### Deleted — §1.2a's inventory in full | | | | --- | --- | | Routes | `GET /admin/email/connect/start`, `GET /admin/email/connect/callback` | | Controller | `connectStart` / `connectCallback`, the `email_oauth_tx` signed cookie, the PKCE verifier and CSRF nonce plumbing, `EMAIL_SCOPE`, `googleClient()` — about half the file | | Transport | the OAuth2 nodemailer transport and its `smtp.gmail.com` / `465` literals | | Model | the `refreshToken` decrypt | | Client | the Connect Gmail button and handler, the `?email_connected` / `?email_error` banner, six Gmail-specific error strings | `provider` and `refresh_token_enc` stay as columns under the additive-only discipline, unread as configuration. **SSO is unaffected**, and one real coupling goes with this: email *borrowed* the `google` provider's client id and secret, so rotating the SSO secret silently broke outbound mail with nothing on either screen relating the two. ### Added **A mail transport registry** (`server/src/engagement/transports/`) with `smtp` as the sole registration. `credentialFields` is the single declaration that the admin form renders, the request sanitizer filters against, and the "is this secret" answer comes from — so adding a relay is a registration, not four edits across a form, a validator, a column set and a model. **Only that half of §3.1 is built.** `registerDeliveryChannel` is not here: Phase 1 has no consumer for `addressFor`/`render`/`deliver` (that is the Phase 4 engine), and a registered channel nothing calls is a shape frozen before anything tried to use it. `email_config` gains `transport`, `credential_enc` and `reply_to`. The credential is **one encrypted JSON blob**, not a column per field, because the field list belongs to the transport — a column per union member would make each new transport a schema change. A blob that will not decrypt reads as **absent** rather than raising, so a rotated `SECRET_ENC_KEY` lands an admin on an "unconfigured" screen instead of a 500 that takes the contact form with it. **Send test becomes the real verification.** Under the consent flow the sender came back from Google's userinfo and was guaranteed to be a mailbox the credential owned. Operator-typed, it can be refused — a silent SPF/DMARC failure, not an error — so failures now name the sender and the likely cause instead of passing a bare `550` through. ### One deliberate behaviour change **`enabled` now gates every sender.** `buildTransport()` used to test 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 five direct senders bypassed it. The connect flow used to set `enabled` as a side effect of a consent redirect; with a credential form it has to mean what it says, so the gate moved onto the one path every sender shares. Flagged because it is a change, not a refactor. ### G22 — the silent degradation, and the warning it earns On upgrade `transport` backfills to `smtp` with no credentials, and every sink politely does nothing: contact → `mailto`, invites → copyable link, resets → generic 200, Team notifications logged and dropped. **Nothing breaks loudly, which is exactly the risk.** So the admin dashboard now returns `warnings[]`, with one entry today. The condition is narrow on purpose: the deprecated Gmail token present **and** no replacement credential — the one deployment this happens to. A fresh install has never had mail and is not nagged; a banner people learn to ignore is worth nothing, and this one has to be believed exactly once. It clears itself when a credential is saved. Also: a one-time, self-limiting `UPDATE` corrects the `status = 'connected'` that an upgraded row would otherwise still be claiming. ### Guardrails - **`npm run check:hosts`** is new — §3.2 rule 4. No transport may ship a default host, endpoint or sender. It reads code, not prose (documentation naming `smtp.gmail.com` is fine; a literal in a transport is not) and has its own self-test that feeds it code it must reject, the discipline `checkModuleIdentifiers.test.js` established. Wired into `pr-checks` before the install, alongside `check:modules`. - Swagger regenerated (2 routes removed, 3 annotations rewritten, `warnings[]` documented); `routes.manifest.json` and `routes.guards.json` regenerated, delta exactly −2 routes. ## How it was tested - `npm test --prefix server` — **1203 passing, 0 failing.** 31 new tests across `mailTransports.test.js` (new), `mailer.test.js` and `emailConfig.model.test.js` (rewritten around the transport seam, keeping every failure-contract assertion), and `checkNoExternalHosts.test.js` (new). - *One flake worth naming:* an earlier full run reported `mailTransports.test.js` as a bare file-level `'test failed'` with no failing subtest, while a Docker container, an Astro build and a Vite build were running alongside it. Not reproducible — green in isolation, green in a four-file group, and green on a clean re-run of the whole suite. Reporting it rather than quietly re-running. - `npm test --prefix client` — 288 passing. `npm run build --prefix client` — clean. - `npm run check:hosts`, `npm run check:modules`, `npm run routes:manifest -- --check` — all clean. - The five failure contracts are asserted individually, unconfigured and configured: mailto fallback, invite link, generic 200, never-throws, and the one sender that *does* throw. **Not yet exercised against a live SMTP server.** The transport build is asserted against a stubbed nodemailer; a real send is a rig walk and is worth doing before this reaches the deployment. ## Checklist - [x] I have read [CONTRIBUTING.md](CONTRIBUTING.md). - [x] The change builds and existing tests/checks pass locally. - [x] I have added or updated tests/docs where it makes sense. - [x] My commits are reasonably scoped with clear messages. ## AI-assisted contributions (required) - [x] AI tools were used. Tool(s): `Claude Code`. I have reviewed and understand every change, and take responsibility for it. AI-authored commits are marked with a `Co-Authored-By` trailer. ## License - [x] I agree that my contribution is licensed under this project's license (**GNU GPL v3.0 or later**), and I have the right to contribute it.

wtclaude added 3 commits 2026-08-29 02:02:13 +00:00

fix(swagger): hoist the one inline predicate that makes the generator run away ... e25e7ade80

`npm run swagger` cannot run on this tree. It dies with swagger-autogen's
"FATAL ERROR: invalid array length - Allocation failed", generating
nothing, and it reproduces on a pristine checkout under both Node 20 and
Node 24 — so the committed spec cannot be regenerated by anyone, and any
PR that adds or changes a route is unable to meet the standing obligation
to update it.

Bisected to one statement in `teams.router.js`:

  param('teamId').custom((v) => v === 'default' || TEAM_ID.test(v))

Hoisting that arrow to a named const fixes it outright. Nothing else
changes and the regenerated spec is byte-identical to the committed one,
so this is a generator fix, not a spec change.

The diagnosis worth keeping, because the file's own comment recorded a
different one. Phase 8 shipped a bare regex LITERAL before `.test(` and
phase 9 hoisted the regex, blaming a per-file route limit measured at
twenty statements; the file has sat at nineteen ever since on the theory
that it was one under the edge. That theory is wrong. Probing every
router file individually, `teams.router.js` at nineteen statements dies
while a THREE-route file carrying only this one route also dies — so the
trigger is the inline arrow reaching `.test(`, not the count. Hoisting
the regex was half the fix; the predicate around it needed hoisting too.

The comments in `teams.router.js`, `teamsVoice.router.js` and
`admin/index.js` are corrected to say so, since all three currently tell
the next person to keep counting statements.

Co-Authored-By: Claude <noreply@anthropic.com>

feat(email): remove Gmail OAuth2, put SMTP behind a transport registry ... 47c8b37d45

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 <noreply@anthropic.com>

docs(email): SMTP setup, the three postures, and the upgrade note ... c4ab8b9b9d

The operator-facing half of engagement Phase 1. README's stack table and
security section, plus both .env.example files, all pointed at the
removed Connect Gmail flow.

The env comments now name the three supported postures rather than one
provider — a relay as the recommendation, smtp.gmail.com:587 with an app
password as the shortest migration, an unauthenticated local MTA as the
third — and point at docs/website/UPGRADE_NOTES.md for the deployment
this actually happens to.

The OpenAPI spec is regenerated: two routes gone, three annotations
rewritten, and the dashboard's new warnings[] documented.

Co-Authored-By: Claude <noreply@anthropic.com>

whitlocktech merged commit f5aa32e0ed into edge 2026-08-29 02:09:46 +00:00

whitlocktech deleted branch feat/engagement-phase-1-smtp 2026-08-29 02:09:47 +00:00

whitlocktech referenced this issue from a commit 2026-08-29 02:09:49 +00:00

Merge pull request 'feat(email): engagement Phase 1 — remove Gmail OAuth2, SMTP behind a transport registry' (#165) from feat/engagement-phase-1-smtp into edge

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

severity:critical

Security audit: critical severity

severity:high

Security audit: high severity

severity:low

Security audit: low severity

severity:medium

Security audit: medium severity

wontfix

This won't be fixed

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

ksfixitman whitlocktech wtclaude

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: RunicGateway/website#165

No description provided.