feat(notification): support implicit-TLS SMTP email channels - #271
feat(notification): support implicit-TLS SMTP email channels#271excniesNIED wants to merge 14 commits into
Conversation
Email notification channels gain an smtp_encryption field (ssl|tls|smtps|implicit) that switches sendEmail to a direct TLS dial for mail servers that only expose SMTP over implicit-TLS ports such as 465/994. Default STARTTLS behavior on port 587 is unchanged.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at 8a94c30. The direct-TLS dial path preserves the legacy STARTTLS behavior when smtp_encryption is absent, and the SMTP command flow is appropriate for implicit TLS.
[P1] Make the new implicit-TLS regression self-contained and runnable on the supported CI runners (backend/internal/server/smtp_direct_tls_test.go:79-83). The test writes the generated certificate to SSL_CERT_FILE and assumes Go's crypto/x509 will load that environment variable, but the exact test fails in the isolated macOS worktree with x509: certificate signed by unknown authority; the test therefore records a failed delivery rather than exercising SMTP. Pass a RootCAs pool explicitly to the production dial configuration through a test seam, or use a certificate trusted by the test process, and keep the end-to-end assertion green.
[P1] Required CI is missing. GitHub currently reports UNSTABLE with an empty status check rollup, so this head cannot be approved or considered merge-ready. Publish the required Backend/Frontend/Deployment/Repository gates and aggregate CI, then request re-review on the exact head.
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Follow-up re-review of exact head 8a94c30d73a44f8468ed42cab9d1c9e20ce98650: all seven GitHub checks are now SUCCESS and the merge state is MERGEABLE/CLEAN. The direct implicit-TLS implementation and backward-compatible STARTTLS path remain sound.
The portability blocker remains in the new end-to-end regression: on the isolated macOS runner, TestEmailDeliveryViaImplicitTLSSMTP still fails with x509: certificate signed by unknown authority because the test relies on SSL_CERT_FILE rather than explicitly wiring the generated CA into the TLS configuration. go vet ./... passes and GitHub Linux CI is green, but the test is not self-contained across supported development environments. This remains comment-only; make the test trust setup explicit and request another review.
The end-to-end implicit-TLS test previously trusted its throwaway certificate through the SSL_CERT_FILE env var, which Go's crypto/x509 does not load in every environment (it fails in isolated macOS worktrees). Add a Server.smtpRootCAs test seam that sendEmail wires into tls.Config.RootCAs on the direct-TLS dial, and have the test inject the fake server's certificate through that seam instead. The backward-compatible STARTTLS path is unchanged.
|
Fixed in Verified: |
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 4353a80. The RootCAs test seam fixes the macOS portability blocker, and the full backend suite, go vet, repository tests and gates, git diff checks, and all GitHub checks pass.
[P1] The feature still cannot be configured through the admin console promised by the PR. notificationChannelConfig has no smtp_encryption field, notificationChannelPayload does not send it, and the edit form does not round-trip it. An operator using the documented UI therefore cannot enable implicit TLS; only a direct API or database edit can. Please add the email-only encryption selector, preserve it through create/edit payloads, add the required English/Japanese translations, and cover the form/payload round trip.
Also replace port 994 in all three administrator guides: 994 is assigned to IRC over TLS, while implicit-TLS SMTP submission uses port 465.
Please request another review after the console path and documentation are corrected.
The email notification channel form now has an SMTP encryption selector (starttls default, or ssl for implicit TLS) that is preserved through the create/update payload and round-tripped when editing, so operators can enable implicit-TLS delivery from the admin console without direct API or database edits. The three-language administrator guide documents the selector.
|
Addressed in Verified: frontend lint, |
tls.DialWithDialer only honours the net.Dialer timeout, so a cancelled alert or password-reset request could keep blocking on the implicit-TLS path until the 5s dial timeout. Switch to tls.Dialer.DialContext so the smtp_encryption=ssl path preserves the same cancellation semantics as the STARTTLS branch, and lock it in with a context-cancellation test.
The generic field UI renders label and help through tx(), so English and Japanese admin console users would have seen the new SMTP encryption copy in Chinese. Add matching entries to both translation catalogs.
CompleteImageJob commits the completed status before observeGatewayCall publishes the counters, so polling only the job status raced the metrics assertion and failed intermittently under CI load (got 0). Also wait for the overhead series — the last one ObserveGatewayCall writes — so the assertions run after publication.
|
Both P1s and the CI failure are fixed at
Verified: |
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 4477770d7f63dd0b00e149913d5493e7d32f13f9. The context-aware implicit-TLS dial and the English/Japanese catalog additions fix both inline findings, and I resolved those two threads. Focused SMTP and metrics tests, including the race run, go vet, frontend lint/tests, translation and environment gates, and diff checks pass.
Approval remains blocked:
[P1] Required CI is missing. GitHub reports no checks for this head.
[P1] npm run typecheck and the repository source-line gate fail because frontend/features/admin/i18n/en.tsx and ja.tsx are each 1502 lines, above the 1500-line limit. Move the new keys into an appropriate split catalog so the required gate passes.
The previously requested port 994 correction also remains open: the current UI help and all three administrator guides still advertise it for implicit SMTP. Please keep implicit SMTP guidance on port 465.
This remains comment-only.
Adding the two keys pushed en.tsx and ja.tsx past the 1500-line source gate. Move them into a notifications catalog and merge it in translations.tsx, following the existing per-domain split pattern.
The UI help text, all three administrator guides, and a test comment advertised 994 alongside 465 for implicit-TLS SMTP. Standardize on 465, the registered smtps port, so the guidance names one canonical port.
|
Both blockers fixed at
On CI: run Verified: |
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 04deae237a9907cb6ca377b8e06a36dd7a0496d2. The split notification catalog fixes the source-line gate, and the UI help, synchronized administrator guides, and test comment now consistently use port 465. Standards and spec reviews are otherwise clean. Full local backend tests and go vet, repeated focused race tests, frontend lint/typecheck/tests/build/E2E, all repository tests and gates, the explicit base/head documentation translation check, and git diff --check pass.
Approval remains blocked:
[P1] GitHub reports this PR as CONFLICTING/DIRTY against main. Resolve the merge conflict, preserving both the catalogs added on main and this PR’s notificationTranslations merge, then request review on the resulting head.
[P1] The exact current head has no CI checks. The most recent listed workflow is action_required on the previous 4477770 head, and there is no required-check result for 04deae2. The required checks must run and pass on the post-conflict head before approval.
[P2] The PR Summary still says implicit-TLS SMTP uses “465 or 994”, even though the code and guides were corrected. Remove the remaining 994 reference from the PR description so it matches the corrected guidance.
This remains comment-only; no workflow was triggered.
# Conflicts: # frontend/features/admin/i18n/translations.tsx
|
Merge conflict resolved at Verified on the merge head: source-line gate / lint / vitest / next build, doc-translation check ( CI note: run |
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
I verified that the current head resolves the previous merge conflict and preserves the intended translation catalog changes. The focused backend checks, go vet, frontend build, repository gates, and explicit base-to-head translation checks pass locally.
I cannot approve this head yet because GitHub reports no CI checks for the exact commit and the merge state remains UNSTABLE; the workflow is action_required. Please obtain trusted CI results for this exact head. I did not trigger or approve the workflow.
|
Thanks for approving the workflow and verifying the merge locally — CI is now running on For the record: because this PR comes from an external fork, each commit's Actions run lands in Will follow up once Backend finishes — should be ready for final approval if everything stays green. |
|
CI is now green for 8f7c3f5. Run 32505020693 passed all 7 checks (Backend, Backend PostgreSQL, N-1 contract, Deployment, Frontend, Frontend browser smoke, and Repository gates), and the merge state is CLEAN. All blockers from the previous review are cleared. Requesting final approval and merge on this head. |
Channels created before the smtp_encryption field existed have no value, so toForm returned an empty string and the select showed the blank placeholder, and saving wrote an empty value back. Round-trip the backend's effective default by falling back to starttls.
|
Fixed at Verified: |
This flaky-test fix is unrelated to the implicit-TLS SMTP feature and will be resubmitted as its own PR.
Notification channel fields are free-form, so a pre-existing generic encryption field must not silently flip an email channel to direct TLS. Keep the predicate scoped to smtp_encryption to preserve the documented opt-in boundary.
|
Both points addressed at
Verified: |
astaxie
left a comment
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 95e92a0fb8d9d8af3371d7c1d1ae9c5eb14a5c8c. The generic encryption alias is removed, the unrelated metrics change is split into #285, all prior threads are resolved, and all GitHub CI checks are green.
One security-sensitive behavior still blocks approval: an explicitly configured smtp_encryption: "starttls" remains opportunistic. At admin_notifications_http.go:687-693, if the SMTP server does not advertise STARTTLS, TokenHub continues and sends the message over the plaintext connection. smtp.PlainAuth itself refuses non-local plaintext authentication, but the alert or password-reset email body can still be transmitted; the latter contains a live reset token. This contradicts the UI/docs contract that selecting STARTTLS means STARTTLS and makes downgrade or server misconfiguration unsafe.
Please fail closed when smtp_encryption is explicitly starttls and the extension is absent. If preserving the legacy unset-field behavior is required, that compatibility path can remain opportunistic, but an explicit secure mode must be enforced. Add an in-process SMTP regression that omits STARTTLS and verifies no MAIL FROM/message body is sent.
Focused SMTP/notification tests, go vet ./..., frontend lint/typecheck/tests/build, the serial browser smoke suite (3/3), and git diff --check passed locally. The first browser run collided with another review worktree on the suite's fixed port; the isolated rerun passed.
Explicit smtp_encryption "starttls" must not downgrade to plaintext when the server does not advertise STARTTLS: the alert or password-reset body could be transmitted in the clear. Return an error instead; legacy channels without the field keep the opportunistic STARTTLS path. Add a regression that runs an in-process server without STARTTLS and asserts no MAIL FROM or message body is sent.
|
Fixed at Added Verified: |
| whatsapp_api_version: stringifyValue(item.fields?.whatsapp_api_version || item.fields?.api_version || "v20.0"), | ||
| smtp_host: stringifyValue(item.fields?.smtp_host), | ||
| smtp_port: stringifyValue(item.fields?.smtp_port), | ||
| smtp_encryption: stringifyValue(item.fields?.smtp_encryption) || "starttls", |
There was a problem hiding this comment.
[P1] This default now changes legacy channel behavior on any edit. A channel created before smtp_encryption existed has the legacy opportunistic STARTTLS semantics, but toForm turns the missing field into "starttls", and notificationChannelPayload writes that value back on save. With the new fail-closed handling for explicit STARTTLS, an unrelated UI edit can convert a previously working plaintext-fallback channel into one that fails whenever the server does not advertise STARTTLS. Please preserve the unset value through edit/update, or otherwise distinguish the displayed default from the persisted explicit starttls value.
| // advertise the extension: downgrading to plaintext would transmit the alert | ||
| // or password-reset body in the clear. The plain serveTestSMTPConnection helper | ||
| // never advertises STARTTLS, so no MAIL FROM or message body may be sent. | ||
| func TestExplicitStartTLSFailsClosedWhenExtensionAbsent(t *testing.T) { |
There was a problem hiding this comment.
[P3] The regression only proves no DATA body is accepted, not the full no-downgrade boundary requested in the previous review. serveTestSMTPConnection records completed message bodies, so this test would still pass if a future refactor sent MAIL FROM before discovering STARTTLS was unavailable. Please have the fake server record commands and assert that no MAIL FROM (and therefore no DATA body) is sent before the explicit STARTTLS failure.
|
The current PR body still under-reports verification for this head. The diff changes frontend resources/i18n files, but the Verification section only lists backend Go checks, |
…ression Only persist smtp_encryption when the channel already has an explicit value or the user picked a non-default option, so an unrelated edit cannot silently upgrade a legacy channel to the fail-closed mode. Rewrite the explicit-STARTTLS regression to record every command and assert no MAIL FROM is sent, not just that no message body arrives.
|
Addressed all three points at
Verification (this head):
New head will need the usual "Approve and run" on its workflow (maintainer-only for fork PRs). |
| let smtpEncryption: string | undefined; | ||
| if (existingEncryption) { | ||
| smtpEncryption = values.smtp_encryption; | ||
| } else if (values.smtp_encryption && values.smtp_encryption !== "starttls") { |
There was a problem hiding this comment.
[P1] This makes the UI unable to save explicit fail-closed STARTTLS for the default path. On create, existing is absent and the form default is starttls, so the payload omits smtp_encryption; the backend then treats the channel as legacy/opportunistic STARTTLS and may continue over plaintext when the server lacks STARTTLS. The same happens for legacy channels with no existing field, where the selector displays starttls but saving keeps it unset, so an operator cannot opt into the explicit starttls behavior from this form. Please distinguish the legacy/opportunistic state from explicit STARTTLS (for example, persist starttls on create and expose a separate legacy/auto value or dirty-state handling for existing unset channels), and keep the docs aligned with whichever behavior is intended.
Summary
Email notification channels previously could only use plaintext-plus-STARTTLS (typically port 587). Many SMTP relays expose SMTP exclusively over an implicit-TLS port such as 465, so
sendEmailwould open a plain TCP connection, the server would reset it waiting for a TLS ClientHello, and alert delivery failed. This change adds asmtp_encryptionfield to email notification channels that switchessendEmailto a direct TLS dial (implicit TLS) for such servers, exposes the field in the admin console, and documents it in all three administrator guides.Related Issue
N/A
Changes
backend/internal/server/admin_notifications_http.go:sendEmailopens an implicit-TLS connection whensmtp_encryptionisssl,tls,smtps, orimplicit; otherwise the previous STARTTLS path is unchanged.smtp_encryptionis the only opt-in switch; the genericencryptionfield is ignored so a free-form channel keeps its documented STARTTLS behavior.smtp_encryption: "starttls"fails closed when the server does not advertise STARTTLS instead of downgrading to plaintext; legacy channels without the field keep the opportunistic path.tls.Dialer.DialContext.backend/internal/server/smtp_direct_tls_test.go: in-process implicit-TLS SMTP delivery test,directSMTPTLSEnabledunit test, context-cancellation test, and a command-recording regression proving explicit STARTTLS fails closed before anyMAIL FROM.frontend/features/admin/resources/settings-config.tsx+payloads.tsx:smtp_encryptionselector (starttls/ssl) with the field round-tripped on edit; legacy channels without the field are saved unset so their opportunistic semantics survive unrelated edits.frontend/features/admin/i18n/notifications.tsx(+en.tsx/ja.tsxsplit): EN/JA translations for the new field.docs/administrator-guide.md,docs/zh-CN/administrator-guide.md,docs/ja/administrator-guide.md: documentsmtp_encryptionwith implicit TLS guidance on port 465.Type of Change
Verification
go vet ./...; SMTP tests (TestEmailDeliveryViaImplicitTLSSMTP,TestDirectSMTPTLSEnabled,TestDirectTLSSMTPDialHonorsContextCancellation,TestExplicitStartTLSFailsClosedWhenExtensionAbsent); fullgo test ./internal/server/ -count=1— all passed.npm run lint,npm run typecheck(incl. source-line gate, 582 files),npm test(6 files / 11 tests),npm run build— all passed.node tools/check-doc-translations.mjs --base origin/main --head HEAD,git diff --check— passed.Compatibility, Security, and Operations
smtp_encryptionis a runtime notification-channel field configured in the admin console, so.env.example/Compose/start.share untouched.smtp_encryptionkeep the existing STARTTLS behavior, and legacy channels are never silently upgraded to the fail-closed mode by an unrelated edit.starttlsrefuses to downgrade to plaintext (protects password-reset and alert bodies); implicit-TLS dials require TLS 1.2+ and honour caller cancellation.Checklist
.envfiles, databases, backups, or runtime logs are included.start.sh, and deployment documentation where applicable (none in this PR).data/model-catalog.yamlremains tracked and catalog changes were reviewed where applicable (none).git diff --checkpasses.