# SMTP Email Delivery — Production Audit

A critical re-read of the SMTP subsystem (`includes/email/`,
`admin/class-settings-page.php`'s Email Delivery section,
`assets/js/email-delivery.js`, and the REST layer) — not assuming the
existing implementation was correct because its own tests passed.
Findings are grouped by what happened to them: **fixed**, **fixed
(shared/pre-existing component)**, or **documented, not fixed** (with the
reasoning for leaving it).

Companion doc: `docs/SMTP_SECURITY_AUDIT.md` covers the 20-point security
checklist specifically. This doc covers the broader sweep (races, UX,
accessibility, responsive, duplicate logic, compatibility).

---

## Fixed during this pass

### 1. `Email_Log` logged every site email, even with SMTP never configured

**Severity: real, meaningful.** `Email_Log::boot()` hooks
`wp_mail_succeeded`/`wp_mail_failed` unconditionally at plugin load. Those
two hooks fire for **every** `wp_mail()` call on the site — WordPress
core's own password-reset emails, comment notifications, any other
plugin's mail (WooCommerce order emails, etc.) — not just ChargeForms'.
Before this fix, the moment the plugin activated, every one of those
sends started being written into `chargeforms_email_log` (recipient,
subject, timestamp) whether or not the site owner had ever opened Email
Delivery settings. That's a real scope overreach: a feature that's
supposed to be entirely opt-in was silently recording metadata about
unrelated site mail from the first request after activation.

**Fix**: `Email_Log::should_log()` — logging now only happens when (a)
the send is an explicit "test email" (always logged, even before a
connection is saved, since that's the one thing the user directly asked
for), or (b) the site has an actual active (enabled + default) SMTP
connection — the exact same condition `Smtp_Transport::configure()`
already uses to decide whether it does anything at all. "Logging
happens" and "SMTP is actually active" are now the same condition
instead of two conditions that could silently drift apart.

Test: `tests/smtp-production-hardening.test.php` (4 checks).

### 2. `tls_failure` was an unreachable classification category

**Severity: real, moderate** (a UX-quality bug, not a security one).
`classify_message()`'s pattern list checked `connection_refused` (`/could
not connect to smtp host/i`) before `tls_failure` (whose own pattern
included the identical substring as one of its alternatives). Since the
first matching pattern in iteration order wins, any real TLS-negotiation
failure that also happened to contain the generic "could not connect to
smtp host" phrasing PHPMailer uses got misclassified as
`connection_refused` — a wrong-port suggestion — instead of the correct
`tls_failure` — a wrong-encryption-setting suggestion. Two different
fixes for two different problems; showing the wrong one sends an admin
chasing the wrong setting.

**Fix**: reordered so every more-specific category is checked before the
broad `connection_refused` catch-all, and added an explicit "tls
negotiation" phrase to `tls_failure`'s own pattern so it no longer
depends solely on out-competing a broader pattern.

Test: `tests/smtp-production-hardening.test.php` (2 checks),
`tests/smtp-error-classification.test.php` (28 checks covering the full
pattern table, including the pre-existing `timeout` false-positive bug
found and fixed in the original implementation pass).

### 3. TOCTOU race in the Free-tier connection limit

**Severity: real, but narrow.** `Smtp_Connection::create()`'s Free-tier
gate (`count() >= 1 && ! has_pro()`) is a plain read followed, several
lines later (after `sanitize()`'s validation work), by a separate
read-modify-write (`all()` → append → `update_option()`). A
`wp_options`-backed store has no row-level lock: two genuinely concurrent
requests (e.g., a double-submitted form, or two admin browser tabs) can
both read `count() === 0`, both pass the gate, and both write — landing
2 connections on a site that's supposed to be capped at 1.

**Fix**: the gate is re-checked immediately before the write, narrowing
the race window from "the whole `create()` call including validation" to
"the few lines between the second check and `update_option()`". This
**does not eliminate** the race — a true fix needs either a real
database table with an atomic `INSERT ... WHERE (SELECT COUNT(*)...) < 1`
guard, or WordPress's own `wp_cache_*`-based locking primitives, either
of which is a bigger architectural change than a hardening pass — and
the task's explicit instruction was **not** to rewrite the existing
architecture. This is stated here as an honest, remaining, low-likelihood
risk, not silently left unmentioned. Realistic impact if it does occur:
a Free site briefly has 2 connections instead of 1 until an admin next
edits/deletes one (at which point every other invariant — exactly one
default, etc. — still self-corrects normally); it is not a security
bypass in the sense of exposing data, only a business-rule enforcement
gap under a specific timing condition.

Test: `tests/smtp-production-hardening.test.php` verifies the gate reads
live storage state at call time rather than a value cached earlier in
the request (the property the mitigation depends on) — a genuinely
concurrent interleaving isn't reproducible in a single-threaded test
script, and that limitation is stated here rather than glossed over.

### 4. Missing label/input association throughout the Email Delivery form

**Severity: real accessibility bug (WCAG 1.3.1 / 3.3.2).** Every
`<label>` in `assets/js/email-delivery.js`'s connection form was visually
adjacent to its input but had no `for`/`id` pairing — a sighted mouse
user never noticed, but a screen reader announced every field with no
name at all.

**Fix**: every label now has a matching `for`, every input a matching
`id`. The one non-input case (the masked-password state, which shows a
`<span>` + "Change Password" button instead of a real input) points its
label at the button instead of an orphaned reference, plus a
`screen-reader-text` announcing "Password is set."

### 5. The upgrade modal had no focus trap, no Escape handling, no ARIA

**Severity: real accessibility bug.** `renderUpgradeLock()`'s dialog had
no `role="dialog"`/`aria-modal`/`aria-labelledby`, Tab could escape into
the page behind it, and Escape did nothing — every one of these is
explicitly called out in this task's own Phase 9 checklist.

**Fix**: `role="dialog"`, `aria-modal="true"`, `aria-labelledby` pointing
at the modal's own heading; a document-level `keydown` listener closes on
Escape and implements a minimal Tab/Shift+Tab focus trap scoped to the
modal's own focusable elements; focus moves into the modal on open and
back to whatever triggered it on close.

### 6. Ambiguous accessible names in the multi-connection table's row actions

**Severity: real accessibility bug.** In the Pro multi-connection table,
every row's "Edit"/"Delete"/"Make Default" buttons had identical
accessible names — a screen reader user navigating "by buttons named
Edit" has no way to tell which connection each one acts on.

**Fix**: each button now carries a visually-hidden
(`screen-reader-text`) suffix naming its connection.

### 7. Real page-level horizontal overflow at 390px — shared component, not SMTP-specific

**Severity: real, confirmed via live browser test at all 5 required
breakpoints (1440/1280/1024/768/390).** `.chargeforms-settings-shell`
(the two-column Global Settings layout every settings section shares,
not something built for this feature) correctly collapses its CSS Grid
to `grid-template-columns: 1fr` at ≤782px, but a grid item's default
`min-width` is `auto`, not `0` — so `.chargeforms-settings-nav` still
refused to shrink below its own content's intrinsic width, forcing the
entire page 638px wide inside a 390px viewport. This is a real,
pre-existing bug affecting **every** Global Settings section (Layout,
Payments, Security, all of them), only surfaced now because this is the
first time this exact breakpoint was verified in a real browser for any
of them.

**Fix**: `min-width: 0` added to `.chargeforms-settings-nav` and
`.chargeforms-settings-content` inside the existing `≤782px` media
query — the standard, minimal fix for this specific CSS Grid behavior.
Re-verified live: zero page-level horizontal overflow at all 5
breakpoints, on both the Email Delivery screen and (spot-checked) the
Layout section, confirming this didn't regress anything else in the
shared shell.

This is flagged as "shared component" rather than "SMTP bug" because the
root cause has nothing to do with SMTP — but it's included here (and
fixed) because it directly blocks an honest "responsive-verified" claim
for the Email Delivery screen specifically, which lives inside that
shell.

---

## Documented, not fixed (with reasoning)

### 8. `duration_ms` is always `NULL` for ordinary (non-test) log rows

`wp_mail_succeeded`/`wp_mail_failed` fire synchronously from inside
`wp_mail()` itself, before `Email_Log`'s hook handlers have any way to
know how long the whole call took — there is no "start" timestamp
available at that point for an arbitrary caller's `wp_mail()` invocation.
`Email_Test::send()` **does** compute and display its own duration
directly in the test-result UI (`response time: N ms`), which is the one
place the spec explicitly asks for it. Backfilling `duration_ms` onto the
persisted log row for ordinary mail would require either wrapping every
`wp_mail()` call (touching the untouched call sites this feature is
explicitly supposed to leave alone) or a fragile "find the row that was
just inserted and patch it" pattern. Left as a known, stated gap rather
than adding either.

### 9. Log's `provider` column is derived at render time, not stored

The Email Logs UI shows a Provider column (spec Phase 6). `Email_Log`
doesn't store a provider — it stores `connection_id`/`connection_name` at
send time. The JS looks up the provider from the connection's **current**
record, which means: if a connection's provider is later changed, or the
connection is deleted, old log rows show the *current* provider (or
blank if deleted) rather than a historical snapshot of what it was at
send time. This is judged acceptable — provider is a low-stakes display
convenience, not audit-critical data, and adding a `provider` column to
the log table for this alone is schema churn for a nice-to-have. If a
future need for accurate historical provider auditing emerges, that's a
one-line addition to `Email_Log::record()`'s insert.

### 10. No true optimistic-lock / version check on `update()`/`delete()`/`set_default()`

Only `create()`'s Free-tier gate got the narrowed re-check (finding #3) —
the general "two concurrent edits to the same connection, last write
silently wins" race exists for every other write path too (`update()`,
`delete()`, `set_default()`). This is a lower-severity, self-correcting
data-integrity concern (the next edit simply overwrites again) rather
than a security or business-rule-enforcement issue, and is explicitly
out of scope for a hardening pass that's told not to rewrite the storage
architecture into something that could offer real row-level locking.

---

## Checked and found correct (worth stating, not just silently passing over)

- **Decrypting credentials "repeatedly"** (Phase 13's concern):
  `Smtp_Transport::configure()` decrypts the active connection's password
  once per actual `wp_mail()` send — i.e., once per real outgoing email,
  not once per admin page load or per request generally. For a WordPress
  site this is the correct place to pay that cost; it is not a hot path.
- **REST nonce validation**: relies on WordPress's own built-in
  `X-WP-Nonce`/`rest_cookie_check_errors` mechanism, the same mechanism
  every other route in `class-rest-api.php` already relies on — not
  reimplemented, not skipped.
- **Connection IDs and the REST route regex**:
  `Smtp_Connection::generate_id()` (`bin2hex(random_bytes(4))`) always
  produces exactly 8 lowercase hex characters, matching the
  `[a-f0-9]{8}` route pattern exactly — no ambiguity, no possibility of a
  generated id failing to route.
- **Status communicated by more than color alone**: every status badge
  pairs a color with real text ("Connected"/"Failed"/"Not
  Tested"/"Disabled"), not a bare colored dot.
- **No large frontend dependency added**: `assets/js/email-delivery.js`
  is plain vanilla JS against `fetch()`, matching the existing
  Integrations page's own Test Connection pattern — no new build step,
  no framework introduced for one settings screen.

---

## Verification performed for this pass

- Full PHP suite: 57/57 test files clean (the one known, pre-existing,
  unrelated `google-sheets-client.test.php` OpenSSL environment issue is
  excluded from that count, as in every prior entry that's touched it).
- 3 new test files added specifically for this hardening pass
  (`smtp-production-hardening.test.php` — 9 checks covering all 3 fixed
  bugs; `smtp-provider-presets.test.php` — 110 checks; Postmark added as
  a 9th provider per this pass's explicit spec).
- Live browser verification (Playwright) at all 5 required breakpoints
  (1440/1280/1024/768/390px), confirming zero page-level horizontal
  overflow after the CSS fix, on both the Email Delivery screen and a
  spot-check of an unrelated Global Settings section.
- Live verification of the accessibility fixes' actual DOM output (not
  just code review): label `for`/`id` pairing, modal `role`/`aria-*`
  attributes, and row-action `screen-reader-text` suffixes all confirmed
  present in rendered HTML.
- `npm run build` and `npm run test:unit` (50/50) re-run clean after all
  changes in this pass.

---

## Architecture status: frozen

As of v0.140.0, the SMTP Email Delivery subsystem is a completed
architecture. No further structural changes (new hooks, new storage
shape, new REST route conventions) should be made against it — only real
bug fixes or security issues, discovered and verified the same way
everything in this doc was: with a live test or a failing automated
check, not speculation. Feature/UX additions (routing UI, advanced
analytics, etc.) are Pro-roadmap territory per
`docs/SMTP_ARCHITECTURE.md` §11, not a reason to reopen this file's
architecture.

## Follow-up issues (deferred, not implemented)

Three items were identified during this audit as real, worth-doing
improvements that were deliberately **not** implemented because each
would cross the line from "harden the existing design" into "change the
architecture" — exactly what this pass was told not to do. Recorded here
so they aren't lost, not actioned.

### FU-1: Atomic DB-backed connection constraints

**Problem**: `Smtp_Connection` stores connections as an array inside one
`wp_options` row (§6 of the architecture doc, a deliberate choice — see
its own reasoning for why a table wasn't used for this data at the time).
That means the Free-tier limit and default-connection invariants are
enforced in PHP via read-then-write, which has no true atomicity — see
finding #3 in this doc (narrowed, not eliminated, TOCTOU race).

**Fix shape**: migrate `chargeforms_smtp_connections` to a real
`chargeforms_smtp_connections` **table** with a unique constraint or a
guarded `INSERT ... SELECT` pattern that makes the Free-tier count check
atomic at the database level, plus a real unique-partial-index-style
guarantee for "at most one default." This is a genuine schema migration
(`dbDelta` + a one-time data migration from the existing option into the
new table, with a fallback read path during the transition) — real
effort, real regression risk, correctly out of scope for a hardening
pass.

**Trigger to revisit**: if the Free-tier race is ever observed in the
wild (a support report of 2 connections on a Free site), or when Pro's
per-form/conditional routing work happens anyway (which will likely want
a real table for other reasons — querying "which connection does form X
use" is awkward against an options blob at any real scale).

### FU-2: Immutable historical provider metadata on log rows

**Problem**: `Email_Log` rows store `connection_id`/`connection_name` at
send time, but not `provider` — the Email Logs UI derives a row's
Provider column by looking up the connection's **current** provider
value at render time (finding #9 in this doc). If a connection's
provider is later changed, or the connection is deleted, old log rows
silently show today's value (or blank) instead of a true historical
snapshot of what was actually used to send that specific email.

**Fix shape**: add a `provider VARCHAR(30)` column to
`chargeforms_email_log`, populated once at insert time in
`Email_Log::record()` from the connection snapshot already available
there. Needs a `dbDelta` column addition (additive/idempotent, low risk)
plus a JS/REST change to read the stored value instead of doing the
client-side lookup — small, but still a real schema change, so scoped
out of a pass that was told to hold the architecture still.

**Trigger to revisit**: the first time someone actually needs to audit
"what provider handled this specific historical send" and the current
best-effort lookup gives a wrong or missing answer — i.e., demand-driven,
not speculative.

### FU-3: Delivery duration tracking for non-test sends

**Problem**: `duration_ms` is always `NULL` on ordinary (non-test) log
rows (finding #8 in this doc) — `wp_mail_succeeded`/`wp_mail_failed` fire
from inside `wp_mail()` itself with no "when did this call start"
reference available to `Email_Log`'s hook handlers.

**Fix shape**: the honest options are (a) wrap `wp_mail()` itself via a
`pre_wp_mail`-style timing hook if/when WordPress core adds one cleanly
enough to time the whole call without touching every existing call site,
or (b) have `Smtp_Transport::configure()` itself record a start
timestamp on the PHPMailer instance and read it back in the
success/failure hook — feasible, but couples the transport-configuration
class to the logging class in a way the current clean separation (§2 of
the architecture doc) deliberately avoids, and needs verifying it can't
ever double-count across retries/multiple `wp_mail()` calls in one
request.

**Trigger to revisit**: if delivery-speed data becomes a real product
need (e.g., a future "your SMTP provider is slow" diagnostic) — today
nothing surfaces or needs this number outside the Test Email flow, which
already has it.