fix: SMTP provider could not send (missing TLS options); Test Connection validated nothing

[timujinne]

Two defects in the e-mail integrations shipped in v1.7.190 (PR #633). One of them means the universal SMTP provider cannot send at all.

1. 🔴 SMTP could not send — the TLS options were missing

gen_smtp supplies no TLS options of its own, and OTP's :ssl now defaults to verify: :verify_peerwith no CA store. Against a real relay:

port	what happened
465 (ssl: true)	connect died: {:options, :incompatible, [verify: :verify_peer, cacerts: :undefined]}
587 (tls: :always)	STARTTLS handshake failed: :tls_failed

The earlier tls: :if_available hid this: when the handshake failed, gen_smtp silently fell back to plaintext — mail went out with the relay password on the wire. Making the transport fail-closed (#633) turned that silent downgrade into an outright failure, which is how the bug surfaced.

swoosh_config_for/1 now passes proper TLS options in both modes — system CA certs, verify_peer, SNI, and the hostname check :ssl does not perform on its own. They ride on sockopts for implicit TLS (gen_smtp hands those straight to :ssl.connect/4) and on tls_options for STARTTLS.

Verified against a live relay: 465 and 587 both connect, and a real send now succeeds over verified TLS.

2. 🟠 "Test Connection" verified nothing for aws_ses and smtp

Neither provider declared a validation, so do_validate/2 fell through to the :ok catch-all and record_validation stamped the connection "connected" without a single byte leaving the box. Paste a wrong key or a bad SMTP password → green check, failing send.

New PhoenixKit.Integrations.Validators:

• aws_ses/1 asks SES itself for the account send quota — a raw ExAws.Operation.Query, so no ex_aws_ses dependency is needed. It proves the credentials are real and authorised for SES in that region.
• smtp/1 opens a real session and authenticates. auth: :always is deliberate: with gen_smtp's default, a relay that does not demand authentication would accept a wrong password and report success. Its options come from Mailer.swoosh_config_for/1, so the check exercises exactly the transport a real send uses — one source of truth, no drift between "tested" and "sent".

Verified live with real credentials:

validate SES (real) => :ok
validate SES (bad secret) => {:error, "Invalid credentials"}
validate SMTP (real) => :ok
validate SMTP (bad pass) => {:error, "Invalid credentials"}
validate SMTP (bad host) => {:error, "Could not reach the SMTP server"}
SEND via smtp integration => {:ok, "2.0.0 OK: queued as <...>"}

Tests

The SMTP provider test used to assert :ok = validate_connection(...) on fabricated credentials — it only passed because validation was a no-op. It now asserts what actually matters (credentials stay retrievable from the "configured" status) and adds a case proving an unreachable relay is rejected.

73 tests, 0 failures across the mailer + integrations suites; --warnings-as-errors, credo --strict and dialyzer all clean (dialyzer caught a dead {:error, reason} clause — gen_smtp_client.open/1 only ever answers with a three-element error tuple).

[timujinne]

Live verification (real relay, real credentials)

Run against the running dev app, using the actual aws_ses and smtp Integrations connections:

### validate SES (real creds) => :ok
### validate SES (bad secret) => {:error, "Invalid credentials"}
### validate SMTP (real creds) => :ok
### validate SMTP (bad password)=> {:error, "Invalid credentials"}
### validate SMTP (bad host) => {:error, "Could not reach the SMTP server"}
### SEND via smtp integration => {:ok, "2.0.0 OK: queued as <90bcdfa63c38da59035d900c27cf098b@...>"}

The last line is the one that matters: sending over the universal SMTP provider works again, and now over verified TLS. Before this branch it failed on both ports — and before v1.7.190 it "worked" only because a failed TLS handshake silently fell back to plaintext, putting the relay password on the wire.

Both TLS modes were confirmed directly against the live relay:

587 STARTTLS + verified tls_options => :OK
465 implicit TLS + verified sockopts => :OK
465 implicit TLS, no tls_options => {:error, ... {:options, :incompatible,
[verify: :verify_peer, cacerts: :undefined]}}

Gates

73 tests, 0 failures (mailer + integrations). mix compile --warnings-as-errors, mix credo --strict, mix dialyzer all clean — dialyzer earned its keep here, catching a dead {:error, reason} clause (gen_smtp_client.open/1 only ever answers with a three-element error tuple).

[timujinne]

Backward compatibility: does real validation break anything that used to pass?

Short answer: no. Validation status is informational — credential retrieval, and therefore sending, does not depend on it.

Proven live, on a connection whose validation genuinely fails:

validate result => {:error, "Could not reach the SMTP server"}
stored status => "error"
get_credentials => true # still returns {:ok, creds}
connected?/1 => true

Why it holds:

• has_credentials?/1 accepts "connected"or"configured", and otherwise falls through to a field-presence check — so a connection that fails validation still yields its credentials.
• connected?/1 is defined in terms of get_credentials/1, not the stored status.

The practical consequence for the emails package: migrate_legacy/0 validates headlessly, so on a host with no outbound network at migration time the migrated SES connection may now land in status "error" instead of "connected". That is an honest signal, and sending is unaffected — the credentials are still resolved and used.

Observation (pre-existing, not introduced here)

connected?/1 returns true for a connection whose stored status is "error", because it asks "are there usable credentials?" rather than "did the last check pass?". The name reads like the latter. Worth a follow-up, but out of scope for this fix.

[timujinne]

Code review, round 2 — verdict was Needs-rework, now addressed

GLM-5.2 (our usual reviewer) was returning 529 for hours, so this round was reviewed by two independent Opus agents — a code reviewer and an architecture/security reviewer. Both read the diff, the call sites and the deps (gen_smtp_client.erl, smtp_socket.erl, ex_aws), and backed the load-bearing claims with live experiments rather than reading. Every claim below was then re-verified against the source before it was acted on.

The review's core objection was fair, and is the reason for the rework: this branch's own thesis is "a check that always says yes is worse than no check" — and the check it first shipped could say yes when it should say no, and no when it should say yes.

Said yes when it should have said no

Finding	Fix
Blank region → silently probed us-east-1.has_credentials?/1 for :key_secret only checked access_key, so a region-less SES connection reached the validator; ExAws defaults a missing region to us-east-1 and the check went green — while the send path builds email..amazonaws.com from the same blank region and raises.	The :key_secret gate now requires every field the provider declares required: true, and the validator refuses a blank region outright.
Fail-open to verify: :verify_none when no CA store is found. Latent in dev (this container has 150 CA certs) and fires exactly where nobody is looking: minimal production images. Sender and check would then trust any certificate — and the check would still show green.	A relay that expects a password now fails closed (:no_ca_store). Only a credential-less relay degrades.

Said no when it should have said yes

Finding	Fix
auth: :always rejected relays that advertise no AUTH verb (internal smarthosts authenticating by IP). Sending works there; the check went red — and the operator could not avoid it, since username/password are required fields.	gen_smtp throws {:missing_requirement, auth} for exactly that case; it is now a pass. Relays that do advertise AUTH still fail closed on a bad password.
SES AccessDenied reported as "Invalid credentials". The least-privilege policy AWS itself recommends (grant only ses:SendEmail) cannot read the send quota — so a correctly configured integration got a permanent red cross, which teaches operators to ignore the check.	The XML <Code> is decoded: signature/token errors → invalid credentials; AccessDenied → pass with a note; throttling → "busy, try again"; anything else surfaced verbatim.
A valid key could be reported invalid. Found live: after AWS rejects a bad signature, a correct request from the same key comes back SignatureDoesNotMatch for a moment — precisely what an operator produces by pasting a wrong key, fixing it, and pressing Test again.	An "invalid credentials" verdict is now confirmed with a second attempt. The adversarial sequence went from intermittently red to 6/6 green; good credentials alone: 10/10.

Could park a LiveView process for twenty minutes

timeout: bounds only gen_smtp's TCP connect. Every read after it waits on a hard-coded ?TIMEOUT of 1_200_000 ms, in the calling process (open/1 does not spawn) — and both call sites are LiveView callbacks. A tarpit relay would freeze that LiveView for twenty minutes. ExAws had a milder version (ten retries with backoff).

Both checks now run under a hard Task.yield / Task.shutdown(:brutal_kill) deadline (15 s, configurable), and ExAws is capped at two attempts with 5 s timeouts. There is a regression test that stands up a socket which accepts the connection and then says nothing.

Structure

The transport moved to PhoenixKit.Mailer.SmtpTransport — a pure function of the credentials map, depending on nothing. Mailer and Validators both use it, so tested and sent are literally the same options, and the Integrations → Validators → Mailer → Integrations cycle is gone.

Two things the reviewers got wrong — and testing caught

• depth is not cargo-cult. It was flagged as a redundant default worth deleting. Removing it breaks the handshake every time: gen_smtp's socket layer ships {depth, 0} (smtp_socket.erl:43,52) and merges it into whatever we pass — depth 0 means no intermediate CAs, so every real chain fails verification and dies with :tls_failed. Omit it → 4/4 failures; set it to anything ≥ 1 → success. It stays, with a test guarding it against a future cleanup.
• auth: :always is required, not removable. My own first instinct (and the suspicion I handed the reviewers) was that it should go, since the send path does not force it. Testing refuted that: with gen_smtp's default the AUTH exchange fails tolerantly, so a wrong password still opens a session and the check would pass. It stays — with the no-AUTH-relay case carved out instead.

Gates

86 tests, 0 failures (mailer + transport + integrations). --warnings-as-errors, credo --strict, dialyzer all clean — dialyzer twice caught dead clauses whose types had narrowed during the rework.

Live, against a real relay and a real SES account:

SES real => :ok SMTP real => :ok
SES bad secret => Invalid creds SMTP bad password => Invalid creds
SES blank region => Region required SMTP unreachable host => Could not reach
validate_connection(SES) => :ok validate_connection(SMTP) => :ok
SEND via smtp integration => {:ok, "2.0.0 OK: queued as <...>"}
SEND via SES (legacy path) => {:ok, %{id: "0110019f5fbc3ce7-..."}}

[timujinne]

Round 3 — one more bug, found in the fix for the last one

First, a correction to the round-2 note above

I filed auth: :always under "two things the reviewers got wrong". That heading was wrong, and the reviewer was right to push back on it. Its recommendation was, verbatim: "keep auth: :always, but special-case {:missing_requirement, _, :auth} → :ok" — which is exactly what shipped. The bad hypothesis (that auth: :always should be dropped, since the send path does not force it) was mine, and I handed it to the reviewers as a suspicion to check. Testing refuted my idea, not theirs.

Only depth belongs under that heading, and the reviewer has since conceded it with a better explanation than mine: smtp_socket.erl:50-56 defines ?SSL_CONNECT_OPTIONS with {depth, 0}, and ssl_connect_options/2 (:314-315) merges caller options over it via proplist_merge → lists:ukeymerge. Omit depth and you inherit 0 — "the peer certificate must be signed directly by a trusted root, zero intermediates" — which kills the handshake against essentially every real relay.

Ten of twelve findings from that review stand, and all five MUST-FIX items landed.

The new bug: a failed connection check killed the operator's page

The round-2 rework put both checks under a deadline, because :gen_smtp_client.open/1 otherwise parks the calling process for twenty minutes. The deadline was Task.async/1 + Task.yield/2.

Task.async/1 links. Every call site is a LiveView callback, and a LiveView process does not trap exits. So any crash inside the check killed the operator's page outright — and the {:exit, reason} clause that was supposed to turn a crash into a polite error message could never run, because the caller was already dead by the time Task.yield/2 would have returned it. I wrote a guard that guarded nothing.

Measured rather than reasoned about — a non-trapping caller running exactly what with_deadline/1 ran:

check does	caller
raise	DIES
exit(:boom)	DIES
killed from outside	DIES
exit(:normal)	survives → {:exit, :normal}
succeeds	survives
overruns the deadline	survives (Task.shutdown unlinks first)

The SMTP check was safe only by accident — open_smtp/1 happens to both rescue and catch :exit. The SES check was not: send_quota_request/2 only rescues, and hackney is entitled to exit underneath it.

Fix: the harness moves to PhoenixKit.Integrations.Probe, built on spawn_monitor/1 — which is what LiveView's own start_async uses, for precisely this reason (phoenix_live_view/channel.ex:337, a :DOWN handler, not a link). A crash is now an error message, and the :DOWN branch is reachable code instead of a comforting lie.

Making it a module rather than a private helper is the point: the property can now be tested. Three of its seven tests run the probe from a spawned, non-trapping caller and fail with {:caller_died, _} if anyone ever swaps Task.async back in.

Two smaller bugs fell out of the same seam:

• The check lost the operator's language. Gettext keeps the locale in the process dictionary, which a spawned process does not inherit. Errors rendered inside the check came back in the default language, while errors rendered in the caller ("Region is required", "Invalid port") came back translated — so the operator saw a mix. The locale is now carried across explicitly, with a test.
• A reply landing in the instant the deadline fired was left in the caller's mailbox. The caller is a LiveView; it would log the stray as an unexpected message. It is flushed.

Blast radius of the tightened :key_secret gate

This was the riskiest thing in the round-2 rework: it changes has_credentials?/1, which gates get_credentials/1, which the send path uses. Checked live against the dev app rather than argued:

get_credentials(existing SES) => ok emails aws_configured? => true
region-less: get_credentials => {:error, :not_configured}
region-less: send now => {:error, :not_configured}
:key_secret providers => ["aws_ses"]

The real SES connection still resolves and still sends. A region-less connection now fails cleanly instead of raising inside Swoosh's AmazonSES adapter, which interpolates the blank region straight into the hostname (email..amazonaws.com) — it could never send either way, so this turns a crash into an actionable message. And aws_ses is the only :key_secret provider, so nothing else is in range.

Timings, on a real relay and a real SES account

The confirm-retry raised a fair question: can a wrong key now overrun the 15s deadline and report "did not respond in time" instead of "invalid credentials"?

SES wrong key (incl. confirm-retry)	1.7 s → Invalid credentials
SES valid	1.1 s → ok
SES unreachable region	0.1 s → Could not reach
SMTP wrong password	0.6 s → Invalid credentials
SMTP valid	0.3 s → ok
SMTP unreachable host	0.005 s → Could not reach

No. A "wrong credentials" verdict comes from an HTTP 403, which is immediate and not retried (ExAws retries transport errors, not 403s), so the pathological 2 × 2 × 5s path needs the request to time out and return a signature error — contradictory. And the degraded case is a less precise red cross, never a false green.

Gates

93 tests / 0 failures in the affected suites (86 → 93, all seven new ones are the probe's). --warnings-as-errors, credo --strict, dialyzer and mix docs all clean. The full suite gains no failures: every failure in it also fails without this branch, and the handful that differ run-to-run pass in isolation (pre-existing sandbox contention — Activity.log/1 hits DBConnection.OwnershipError on main too).

On reviewers

GLM-5.2 was returning 529 for the whole of this work, so rounds 2 and 3 were reviewed by Opus agents instead. The second-round architecture reviewer has now hit a hard billing limit mid-run, so this round's findings are mine, verified the same way the others were: by experiment against the running app and by reading gen_smtp, ExAws and LiveView rather than trusting recollection. That is worth stating plainly rather than implying a review happened that did not.

[timujinne]

Merged main in — and it independently confirmed this PR's main finding

main had moved on by two commits (867bc5b2 lib upgrades, 303286d0 post-merge fixes for #635). Merged cleanly, no conflicts. The review for this PR is now published in the repo's own convention: dev_docs/pull_requests/2026/636-integration-connection-validators/CLAUDE_REVIEW.md.

The merge brought a nice piece of corroboration

303286d0 fixes a hazard in qr_login.ex and describes it in its own words:

# async_nolink (not Task.async): the lookup must never be able to crash the calling LiveView via a link, only ever resolve to nil on failure.

That is exactly the bug this PR's round 3 found in its own deadline harness, reached independently, in the same week, in the same codebase. Two people tripping over the same wire is a good argument that it is worth a guard — and there is now a test that fails if anyone puts Task.async back.

Probe stays on spawn_monitor/1 rather than adopting async_nolink: a connection check touches neither the repo nor a mock, so it needs nothing the supervisor offers — no $callers, no supervision — and async_nolink would cost a dialyzer suppression in new code (see below). The reasoning is in the moduledoc so the next person does not have to re-derive it.

One thing for the maintainers

mix dialyzer on main is currently red.qr_login.ex:142 trips call_without_opaque at Task.yield/2: Task.Supervisor.async_nolink/2 hands back a %Task{} that dialyzer knows structurally, while Task.yield/2 declares Task.t() opaque. It is a false positive of exactly the class the repo already suppresses for gettext.ex, and it is not a runtime bug — but it does fail the pre-commit gate. It surfaced only now because 867bc5b2's dep upgrades rebuilt the PLT.

Suppressed here (with the reasoning written down) so this branch is green; flagging it because it will block the next commit on main too.

Re-verified live after the merge

The upgrades included phoenix_live_view, so the claims this PR rests on were re-checked rather than assumed — the new LiveView channel still does not trap exits, which is what makes the unlinked probe necessary.

validate SES => :ok validate SMTP => :ok
validate_connection(SES) => :ok validate_connection(SMTP) => :ok
SES bad secret => Invalid credentials
SMTP bad password => Invalid credentials
SEND via smtp integration => {:ok, "2.0.0 OK: queued as <1e18849037...>"}

93 tests / 0 failures in the affected suites; --warnings-as-errors, credo --strict, dialyzer and mix docs clean on the merged tree.

[timujinne]

Hold — a late review landed, verdict Needs-rework

Retracting the "ready" note above until this is fixed. Two findings, both proven by execution, both mine:

1. A crash on a live path.retries: [max_attempts: 2] does not cap ExAws retries — ExAws merges the override shallowly, so it replaces the default list and drops the backoff keys, and the retry then evaluates nil * :math.pow(2, attempt). The rescue swallows the ArithmeticError, so the check does zero retries and logs an arithmetic error instead of the real cause.

2. My own Probe fix relocated the bug it fixed rather than removing it.spawn_monitor/1 watches in one direction only: the deadline lives in the caller, so when the LiveView dies mid-check nothing fires it, and the probe stays parked in gen_smtp for twenty minutes holding its socket. Linked, it would at least have been reaped.

Verifying both against source now and fixing. Do not merge this branch yet.

[timujinne]

Hold lifted — both findings fixed, and the reason they got through is fixed too

The late review's two findings were real, and I reproduced both against source and against the running app before touching anything. Details are in the published review; the short version:

1. The retry cap did not cap retries — it crashed them

retries: [max_attempts: 2] looks like a cap. ExAws.Config.build_base/2 merges overrides with Map.merge (config.ex:121) — shallow — so the list replaces the default [max_attempts: 10, base_backoff_in_ms: 10, max_backoff_in_ms: 10_000] and takes both backoff keys with it. ExAws.Request.backoff/2 then evaluates nil * :math.pow(2, attempt) and raises; my own rescue swallowed it.

before: HTTP ERROR: :nxdomain ... ATTEMPT: 1
SES connection check failed: %ArithmeticError{} <- zero retries, wrong diagnosis
after: HTTP ERROR: :nxdomain ... ATTEMPT: 1
HTTP ERROR: :nxdomain ... ATTEMPT: 2
{:error, "Could not reach AWS SES"} (278 ms)

The comment above it claimed "two attempts survives a single blip". It survived nothing.

2. My own Probe fix relocated the hang instead of removing it

Replacing Task.async/1 with spawn_monitor/1 removed the link — and the link was doing something. spawn_monitor watches in one direction only: the deadline lives in the caller's receive/after, so when the LiveView goes away mid-check (the operator hit refresh), nothing is left alive to fire it. The check stays parked in gen_smtp's 20-minute ?TIMEOUT holding its socket — and, unlinked, it is now unreachable rather than merely slow.

caller dies mid-check	check
spawn_monitor (as shipped)	still alive — socket held 20 min
spawn_link + monitor + unlink	reaped with its caller

And the moduledoc I wrote to justify it — that LiveView "monitors rather than links" — was simply wrong. I asserted it from the :DOWN handler in channel.ex without opening async.ex. LiveView does both: Task.start_link/1, a monitor on top for result delivery, and the work wrapped in try/after Process.unlink/1 so the child unlinks before it dies. The link reaps the child when the parent goes; the unlink stops the child taking the parent with it. Handling one direction and not the other is worse than handling neither. Probe now does the same, and unlinks before killing at the deadline, since :kill is untrappable.

The reason both got through, fixed at the root

The reviewer's closing line is the one worth keeping: green PRs did not catch these, because neither had a test. The new logic was the untested logic. So the seams came with the fixes:

• the CA store is a parameter of SmtpTransport.config/2 — it was always a pure function of (credentials, CA store), the store was just ambient — so the security-critical fail-closed branch is finally reachable from a test;
• request_send_quota/3 takes its requester, so the confirm-retry is tested without AWS;
• interpret_ses_error/1 is pure and public, so the AWS-error mapping is tested against real SES codes;
• a fake relay that advertises no AUTH verb proves the {:missing_requirement, _, :auth} carve-out, which had been asserted and never run;
• Probe's tests now run in both directions — the asymmetry (five tests for "check dies, caller survives", none for "caller dies, check is reaped") is exactly why the leak survived the fix.

86 → 107 tests, 0 failures.

Also in this round

AccessDenied used to pass with a "note" that went only to Logger.info — so the operator saw a bare "Connection verified", while AccessDenied proves only that the signature is valid for some AWS principal (a key from the wrong account lands there too). That was this branch's own thesis reopened at the one door it had closed. A check can now pass with something to say: the connection is connected and sends exactly as before, but the caveat is stored and rendered next to the badge. Dialyzer caught the one place the widening had not reached.

Plus: catch :exit on the SES path (hackney reaches its pool through GenServer.call, which exits); gen_smtp's default {retries, 1} disabled for the check only; InvalidAccessKeyId / ExpiredToken / TokenRefreshRequired treated as invalid credentials; the confirm-retry waits a full second, since SES throttles GetSendQuota at ~1/s; and the tarpit test no longer leaks its acceptor for the life of the VM.

Upgrade note — now in CHANGELOG

Two fixes change the send path, not just the check, and neither had been written down:

• SMTP sending stops on images with no CA bundle ({:error, :no_ca_store}) instead of proceeding with certificate verification disabled — which is why slim images (distroless, scratch, some Alpine builds) never noticed they had none. Install ca-certificates. A relay with no credentials still degrades rather than failing.
• Configured relays are no longer MX-resolved (no_mx_lookups: true). If you set host to a bare domain and relied on MX resolution, point it at the relay.

Gates

107 tests / 0 failures; --warnings-as-errors, credo --strict, dialyzer, mix docs clean; the full suite gains no failures. Live on the dev app: both integrations validate, both reject bad credentials, and a real send still goes out through the SMTP integration.

Ready to merge.

[ddon]

Please solve the conflict

[ddon]

Still this branch has conflicts

[timujinne]

Closing as superseded: every change on this branch (the SMTP transport TLS fix, the Test Connection validators, the Probe, and all their tests) landed on main via #647 and shipped in 1.7.203. After syncing this branch with main the residual diff is zero files.