Repository navigation
fix(mail): deliver account mail over STARTTLS through abnegate-notify - #114
Merged
Merged
Conversation
zone_email built SmtpTransport::relay, implicit TLS, for every port, so the default SMTP_PORT of 587 opened with a TLS ClientHello against a plaintext greeting and no verification or reset mail was ever delivered. abnegate_notify::Mailer upgrades with STARTTLS on every port but 465. zone_notify and zone_email were zone's copies of what abnegate-notify now publishes, so both crates go and zone_server depends on the crate with its smtp feature (testing for its own tests). Account mail keeps zone's wording in a new services::mail module: Template holds the two messages and AccountMail sends them through any abnegate_notify::Mail. mail::Config is the one reader of the SMTP_* variables for account mail and email notices alike, with the sender policy as the only difference, so the two can no longer drift. A value that is blank once trimmed counts as unset: docker-compose passes every SMTP_* variable through even when .env leaves it out, so a blank SMTP_FROM switched account mail off instead of falling back to the default sender, and a blank SMTP_HOST was logged as configured and then failed every send. SMTP_PASSWORD is the exception and is taken as given, because both readers accepted an empty one and a relay may too. An unreadable SMTP_PORT falls back to 587 with a warning for both, as notices already did; that delivered if the relay served 587 and failed otherwise, which beats switching account mail off. The auth handlers hand a message to a task of its own instead of awaiting it. A known address was answered a full SMTP session later than an unknown one, which told a caller which accounts exist, and a stalled relay held the request open until lettre's timeout. The token is stored either way, so a send that fails, is dropped because MAX_CONCURRENT_SENDS messages are already with the relay, or is cut off by shutdown is retried by asking again. The bound keeps the public, unthrottled endpoints from opening an SMTP session per request. Each send is also cut off after SEND_TIMEOUT: lettre's async transport bounds only the TCP connect, so a relay that stalls after it would hold a permit forever, and eight such stalls would switch account mail off until a restart. The state the server falls back to when the embedding service will not start now carries account mail. It used to drop it after logging that it was configured. AppState's three constructors built the same state three times, so they now share one assembly. The display name is sanitised before it reaches a mail client, since it is whatever the person typed. The link is left alone, because redaction would eat its token. The notification workers swap imports. Slack and Discord now get ZONE_NOTIFY_TIMEOUT_SECONDS for their own requests, as documented, and Discord keeps its Zone footer, which the crate leaves opt-in. The email channel no longer needs a Tokio runtime to be built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SMTP entries now say which port uses which TLS mode, that a blank value other than the password counts as unset, what an unreadable port falls back to, and where SMTP_FROM falls back. They no longer claim invitations send mail; they never did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clippy 1.98 flags Routing as a large enum: the Chat variant's Route makes every Routing at least 856 bytes, 216 more than the next largest variant, and clippy -D warnings fails zone_server on main. Boxing the route keeps the enum near the media variants' size; it is built once per turn, so the allocation costs nothing that matters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (55)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CodeQL's Rust sensitive-data heuristic (HeuristicNames::maybeAccountInfo) matches any call, variable or field name containing "account". Naming the constructor new_with_account_mail, the field, accessor and parameters account_mail made every AppState a sensitive source, so the GitHub API URLs built in zone_vcs::pull_request were flagged as rust/cleartext-transmission. Nothing sensitive flows there; the state only carries the mail sender. The heuristic does not read type names, so AccountMail keeps its name, and "mail" alone is not matched. The test client follows: with_mail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaks your
.envTwo edge cases change for account mail. Email notices already behaved the new way, and nothing else in
.envchanges.SMTP_PORTnaming an implicit-TLS port other than 465. Account mail used implicit TLS on every port, so such a relay worked for it. Every port except 465 now expects STARTTLS, which is how notices already connected. Fix: pointSMTP_PORTat 465, or at the relay's STARTTLS port.SMTP_USER. Account mail used to sign in with an empty user name, which only a relay that accepts any credentials took. A blank user now leaves account mail off, as it already left out email notices. Fix: setSMTP_USER.Summary
This moves zone's notifications and transactional email onto the published
abnegate-notifycrate and deletes zone's local copies of both:runner/zone_notify(3,116 lines) andrunner/zone_email(361 lines). On the way it fixes account mail, which never reached a relay on the default port.zone_notify(Fanout,Notification,Slack,Discord,Email,Report, …)abnegate-notify = "0.1"with thesmtpfeatureNotifyErrorbecomesabnegate_notify::Error, built through its constructors (Error::unreachable,Error::rejected). Everywhere else only the imports change:auto_project/{driver,mod,notification}.rs,housekeeping/*,regression/worker.rs,reports/{digest,worker}.rs.zone_email::EmailServicezone_server::services::mailoverabnegate_notify::MailerTemplateholds the verification and password-reset wording.AccountMailsends a template through anyabnegate_notify::Mail.AppState::email_service()becomesmail().zone_email::MockEmailService(#[cfg(test)]only)abnegate_notify::MockMailer(testingfeature, dev-dependency only)zone_serverno longer depends onlettredirectly. Its only copy now comes through the crate (default-features = false, tokio + native-tls).One reader for
SMTP_*.mail::Config::from_variablesnow reads the relay for account mail and for the email notice channel. The variable names live in onemail::Variableenum. The only difference between the two callers is amail::SenderPolicyargument:SenderPolicy::Default, so it falls back tonoreply@zone.appwhenSMTP_FROMis not set.SenderPolicy::Required, so they still needSMTP_FROM.Both callers now follow the same rules for blank values, trimming and an unreadable port. See below.
AppState. Its three constructors built the same state three times. They now share one privateassemble, and the newAppState::new_with_maillets the fallback state carry account mail. The constructor, field, accessor and parameters saymailrather thanaccount_mail: CodeQL's sensitive-data heuristic treats any value or call whose name containsaccountas a secret, soaccount_mailmade everyAppStatea sensitive source and raisedrust/cleartext-transmissionon the GitHub API calls inzone_vcs. The type keeps its name, since the heuristic reads only call, variable and field names.Behaviour differences
Account mail (sign-up verification, password reset)
STARTTLS on port 587 (a fix).
zone_emailbuiltSmtpTransport::relayfor every port, which is implicit TLS. The defaultSMTP_PORTis 587, and relays serve that port with STARTTLS. So zone opened with a TLS ClientHello against a plaintext220greeting, and no verification or reset mail was ever delivered.Mailernow uses implicit TLS only on 465. On every other port it greets in plaintext and upgrades with STARTTLS. It refuses to send credentials if the upgrade fails. Pinned byservices::mail::account::tests::a_submission_port_relay_is_upgraded_with_starttls_before_signing_in(see Verification).Blank
SMTP_*values count as unset (a fix).docker-compose.ymlpasses everySMTP_*variable through, blank when.envleaves it out (SMTP_FROM=${SMTP_FROM:-}). These cases used to break:SMTP_FROMwas taken as the sender address, so account mail failed to start rather than falling back tonoreply@zone.app.SMTP_HOSTwas logged as "Account mail configured", and then every send failed.A value that is blank once trimmed is now unset, and every value is trimmed, with one exception.
SMTP_PASSWORDis taken exactly as given and a blank one is still accepted, as both readers accepted it before, for a relay that takes an empty password. Pinned byservices::mail::config::tests::{a_blank_sender_falls_back_to_the_default, a_blank_host_is_named_as_missing, a_blank_user_is_named_as_missing, a_blank_password_is_kept_for_a_relay_that_takes_an_empty_one}.An unreadable
SMTP_PORTfalls back to 587. It used to switch account mail off (Invalid SMTP_PORT). Account mail now does what notices did: it warns, naming the variable, and uses 587. Pinned byservices::mail::config::tests::an_unreadable_port_falls_back_to_the_default.The response no longer waits on the relay (a fix).
resend-verificationandforgot-passwordused to run a full SMTP session before they answered, and only for a known address. Response time therefore told a caller which addresses have accounts. A stalled relay could also hold the request open until lettre's per-command timeout.AccountMail::dispatchnow sends on a task of its own, and the handler answers straight away. The token is stored either way, so a send that fails is retried by asking again.auth_console_contract_tests::a_known_address_is_answered_without_waiting_on_the_relay, which uses aMailthat never resolves.MAX_CONCURRENT_SENDS(8) messages are with the relay at once, because both endpoints are public and not rate-limited. Past that,dispatchlogs a warning naming only the subject and drops the message without spawning a task. That is safe because the token is already stored, so the person can ask again. Pinned byservices::mail::account::tests::a_message_past_the_concurrency_bound_is_dropped_without_a_task. Its relay counts the sends that reach it, and the test waits until all eight are insidesendbefore it checks the overflow.SEND_TIMEOUT(60 s), which frees its place. The relay sets no limit of its own here (see Async send). Without this bound, eight stalled sessions would switch account mail off until a restart. Pinned byservices::mail::account::tests::a_relay_that_never_answers_gives_its_permit_back_after_the_send_timeout, which runs on paused time.Account mail survives an embedding failure (a fix). When the embedding service failed to start,
main.rsfell back toAppState::new, which had no account mail. That happened after "Account mail configured" had already been logged. The fallback now usesAppState::new_with_mail. Pinned bystate::tests::the_state_without_embedding_still_carries_mail.EmailServicehad the same gap.Display name is sanitised. The
{name}in each template goes throughabnegate_secret::sanitize, which strips terminal control sequences and invisible formatting characters and redacts anything that looks like a credential. The link is left as it is, because redaction would eat its token. Pinned byservices::mail::template::tests::control_sequences_in_a_name_are_stripped_and_the_link_is_left_whole.Async send. A message now goes through lettre's async transport (
Tokio1Executor), not aspawn_blockingaround the synchronous one. A transport is built for each message, so there is no connection pool. In lettre 0.11 the async transport bounds only the TCP connect. The greeting, the TLS handshake and every command reply are unbounded; the synchronous transport timed each command out after 60 s.abnegate_notify::Mailer::sendadds no timeout either, so zone bounds the whole send withSEND_TIMEOUTinstead.Email notices are bounded the same way, by the fan-out. The crate's
Emailnotifier does not overrideNotifier::timeout, soFanout::deliverwraps each email delivery in the fan-out budget:ZONE_NOTIFY_TIMEOUT_SECONDS, default 10 s, capped at 120 s. A stalled relay is cancelled there. Every zone caller goes throughFanout::deliver(auto_project/driver.rs,regression/worker.rs,reports/worker.rs).Body whitespace. The text is the same. The old leading blank line and trailing indentation from the raw-string templates are gone.
Error and log text.
Account mail did not go out, with the subject andSMTP delivery via <host> failed: <sanitized reason>. The handler logs… email queued for user_id: …, not… email sent ….SMTP_HOST is not setrather thanEmail service not configured.Sender name. A blank
SMTP_FROM_NAMEnow falls back toZone. It used to produce" <address>".Dead code. The never-called invitation template (
send_invitation_email) is dropped.create_invitationreturns its token in the response and never sent mail.docs/CONFIGURATION.mdand.env.exampleno longer claim it does.Notifications (Slack, Discord, email, chat) (covered by the crate's suite unless noted)
ZONE_NOTIFY_TIMEOUT_SECONDSnow also bounds each Slack and Discord request. Before, the HTTP client had a fixed 10-second timeout, so a configured budget above 10 seconds was still cut off at 10.CONFIGURATION.mddocuments the variable as how long one channel is given. Pinned byworkers::notify::tests::webhook_channels_are_given_the_configured_budget.mail::Config. The rules for notices are the ones they had: blank values count as unset, a blankSMTP_PASSWORDis accepted, and an unreadableSMTP_PORTfalls back to 587 with a warning. That fallback delivers if the relay serves 587 and fails otherwise. The only change is that the warning for a skipped channel now names the missing variable. Pinned byworkers::notify::tests::{an_unreadable_port_falls_back_to_the_default_and_keeps_email, a_blank_password_still_registers_email, a_blank_host_leaves_email_out_and_is_named, a_relay_missing_its_sender_leaves_email_out, a_padded_relay_is_trimmed_and_a_blank_sender_name_falls_back}."Zone"explicitly and embeds look as they did. This one is not unit-tested: the payload is private to the crate, and a Discord channel only posts todiscord.com.Open in Zoneto the crate'sOpen, which is fixed in the crate.abnegate-notify/0.1.3, notZone/0.1.0 (+zone_notify).Endpointposts the parsed, normalised URL, with a lower-cased host and..segments resolved, not the string as configured. A URL that names a port other than the default is refused (EndpointError::Port).Error::Timeout, notUnreachable.abnegate_secret::sanitizerather thanzone_core::tools::sanitize. The new rules are stricter: they also strip invisible formatting characters (zero-width joiners, variation selectors such as U+FE0F, bidi overrides). The sanitize slice makes this zone-wide.#[test].Error::unreachable("database", …)).Verification
The commands below ran from
runner/with--no-default-features --features zone_context/test-utilsunless noted.cargo fmt --all --check: PASScargo clippy -p zone_server --all-targets -- -D warnings: PASScargo clippy -p zone_server --all-targets --all-features -- -D warnings(the CI feature set): PASScargo check --workspace --exclude zone_desktop --all-targets: PASScargo nextest run -p zone_server --lib -E 'test(/^services::mail::|^workers::notify::|^workers::reports::|^workers::regression::worker::|^workers::auto_project::notification::|^state::tests::the_state_without|^state::tests::constructors/)': 90 passedzone-verify.sh … scoped(throwaway pg16 container) with-p zone_server --lib --test email_verification_tests --test password_reset_tests --test auth_console_contract_tests -E 'test(/^state::|^services::mail::|^workers::notify::|^workers::reports::|^ws::chat::/) | kind(test)': 235 passedzone_email::EmailService:zone_emailwas restored from 1fae890 into the worktree, temporarily, and the test was run there. Both were reverted afterwards."\u{16}\u{3}\u{3}…", a TLS ClientHello, instead ofEHLO.clippy -D warningswas already red onmain. It failed onlarge_enum_variantinws/chat.rs, the lintmainhas failed since 1fae890. This branch carries the sanitize slice's fix,refactor(chat): box the route a chat turn carries, unchanged. It matches8b3bb0adonmigrate/sanitize, so the PRs merge cleanly in either order.runner/Cargo.lockwas re-resolved by cargo: it addsabnegate-notify 0.1.3and dropszone_notifyandzone_email.New tests:
services::mail::account::tests::{a_submission_port_relay_is_upgraded_with_starttls_before_signing_in, every_message_reaches_the_mailer_with_its_subject_and_body, a_dispatched_message_reaches_the_mailer_on_its_own_task, a_message_past_the_concurrency_bound_is_dropped_without_a_task, a_relay_that_never_answers_gives_its_permit_back_after_the_send_timeout}services::mail::template::tests::{each_template_has_its_subject, verification_greets_the_person_and_carries_the_link_on_its_own_line, a_password_reset_greets_the_person_and_carries_the_link_on_its_own_line, control_sequences_in_a_name_are_stripped_and_the_link_is_left_whole}services::mail::config::tests::{the_required_variables_alone_configure_a_relay_with_the_defaults, every_optional_variable_is_honoured, each_required_variable_is_named_when_it_is_missing, a_blank_host_is_named_as_missing, a_blank_user_is_named_as_missing, a_blank_password_is_kept_for_a_relay_that_takes_an_empty_one, a_blank_sender_falls_back_to_the_default, a_required_sender_is_named_when_it_is_missing_or_blank, values_are_trimmed_but_the_password_is_kept_as_given, an_unreadable_port_falls_back_to_the_default, the_password_never_appears_in_debug}services::mail::{error,variable}::tests::*workers::notify::tests::{webhook_channels_are_given_the_configured_budget, a_blank_host_leaves_email_out_and_is_named, an_unreadable_port_falls_back_to_the_default_and_keeps_email, a_blank_password_still_registers_email, a_padded_relay_is_trimmed_and_a_blank_sender_name_falls_back}state::tests::the_state_without_embedding_still_carries_mailauth_console_contract_tests::a_known_address_is_answered_without_waiting_on_the_relayNot verified:
zone-verify.sh readyrun (sqlx prepare check, cargo-deny) is left to the verifier.🤖 Generated with Claude Code