From 6b4d238e51b74027d01eeb9fa696d842bfcc1158 Mon Sep 17 00:00:00 2001 From: SimpleTest Date: Wed, 12 Aug 2026 21:41:01 +0300 Subject: [PATCH] Harden transactional email envelopes --- Dockerfile | 1 + config/runtime.exs | 16 +++++--- docs/verification.md | 16 ++++++++ lib/who_need_help/email_delivery.ex | 40 +++++++++++++------ .../mail/content_removal_email_worker.ex | 4 ++ .../mail/support_confirmation_worker.ex | 1 + .../mail/support_operator_alert_worker.ex | 5 ++- .../mail/support_update_worker.ex | 1 + scripts/validate-production-env.sh | 31 +++++++++++--- scripts/validate-test-env.sh | 27 ++++++++++++- test/who_need_help/email_address_test.exs | 33 +++++++++++++++ .../support_and_content_removal_test.exs | 22 ++++++++++ 12 files changed, 173 insertions(+), 24 deletions(-) diff --git a/Dockerfile b/Dockerfile index 4040880..babbd41 100644 --- a/Dockerfile +++ b/Dockerfile @@ -133,6 +133,7 @@ RUN apt-get update \ && apt-get install -y --no-install-recommends \ build-essential=12.12 \ ca-certificates=20250419 \ + cmake=3.31.6-2 \ git=1:2.47.3-0+deb13u1 \ && rm -rf /var/lib/apt/lists/* diff --git a/config/runtime.exs b/config/runtime.exs index 8126ef1..19afac1 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -727,9 +727,16 @@ if config_env() == :prod do config :who_need_help, :handover_secret, handover_secret + email_from_name = System.get_env("EMAIL_FROM_NAME", "Who Need Help") + email_from_address = System.get_env("EMAIL_FROM_ADDRESS", "contact@example.com") + + unless WhoNeedHelp.EmailAddress.valid?(email_from_address) do + raise "EMAIL_FROM_ADDRESS must be a single SMTP-safe mailbox address." + end + config :who_need_help, :mailer_from, - name: System.get_env("EMAIL_FROM_NAME", "Who Need Help"), - address: System.get_env("EMAIL_FROM_ADDRESS", "contact@example.com") + name: email_from_name, + address: email_from_address support_inbox_address = case System.get_env("SUPPORT_INBOX_ADDRESS") do @@ -737,9 +744,8 @@ if config_env() == :prod do value -> value end - if support_inbox_address && - not Regex.match?(~r/^[^@,;\s]+@[^@,;\s]+$/, support_inbox_address) do - raise "SUPPORT_INBOX_ADDRESS must be a single email address without spaces." + if support_inbox_address && not WhoNeedHelp.EmailAddress.valid?(support_inbox_address) do + raise "SUPPORT_INBOX_ADDRESS must be a single SMTP-safe mailbox address." end config :who_need_help, :support_inbox_address, support_inbox_address diff --git a/docs/verification.md b/docs/verification.md index 5d5c287..a15ecc1 100644 --- a/docs/verification.md +++ b/docs/verification.md @@ -5,6 +5,16 @@ results from product limits and unknown production properties. ## Current launch-boundary and transactional-email proof on 2026-08-12 +- A follow-up SMTP-envelope regression run completed successfully in isolated + user-systemd unit + `codex-heavy-wnh-email-full-final-20260812-213848-711438.service`. + ExUnit reported 468 passing tests with seed `954756`. The shared mailbox + validator now covers model input, production/test environment preflight, and + every outbound envelope; malformed historical recipients cancel their exact + Oban mail job instead of reaching the SMTP adapter or retrying indefinitely. + A separately built production image loaded the same runtime validation + successfully in unit + `codex-heavy-wnh-release-runtime-email-20260812-213723-667643.service`. - The final email-boundary quality rerun completed successfully in isolated user-systemd unit `codex-heavy-wnh-email-quality-final-20260812-184719-3589051.service`. @@ -77,6 +87,12 @@ results from product limits and unknown production properties. - `SUPPORT_OPERATOR_EMAIL_MODE` was absent in the production environment and the deployed default was `disabled`; verified support requests therefore did not generate an operator-inbox alert in that observed configuration. +- Four legacy SMTP exceptions were reproduced from stored recipients whose + local part began or ended with a dot or contained consecutive dots. The + deployed revision accepted those forms before the SMTP adapter rejected + them. This establishes the failure class, but the legacy aggregate lacks + recipient and purpose labels, so it does not prove which historical rows + produced each of the four process-level exceptions. - The workstation-scheduled encrypted off-site backup completed successfully at 2026-08-12 00:04:48 EEST, including its isolated restore drill. The new restore-verified heartbeat implementation was committed later that day, so diff --git a/lib/who_need_help/email_delivery.ex b/lib/who_need_help/email_delivery.ex index 45e2036..1fe77e7 100644 --- a/lib/who_need_help/email_delivery.ex +++ b/lib/who_need_help/email_delivery.ex @@ -6,6 +6,7 @@ defmodule WhoNeedHelp.EmailDelivery do subjects, bodies, references, and other personal data are never attached. """ + alias WhoNeedHelp.EmailAddress alias WhoNeedHelp.Mailer @kinds [ @@ -26,18 +27,23 @@ defmodule WhoNeedHelp.EmailDelivery do def deliver(%Swoosh.Email{} = email, kind) when kind in @kinds do started_at = System.monotonic_time() - try do - result = Mailer.deliver(email) - emit(started_at, kind, result_status(result)) - result - rescue - exception -> - emit(started_at, kind, :exception) - reraise exception, __STACKTRACE__ - catch - class, reason -> - emit(started_at, kind, :exception) - :erlang.raise(class, reason, __STACKTRACE__) + if valid_envelope?(email) do + try do + result = Mailer.deliver(email) + emit(started_at, kind, result_status(result)) + result + rescue + exception -> + emit(started_at, kind, :exception) + reraise exception, __STACKTRACE__ + catch + class, reason -> + emit(started_at, kind, :exception) + :erlang.raise(class, reason, __STACKTRACE__) + end + else + emit(started_at, kind, :error) + {:error, :invalid_email_address} end end @@ -46,6 +52,16 @@ defmodule WhoNeedHelp.EmailDelivery do defp result_status({:ok, _metadata}), do: :ok defp result_status({:error, _reason}), do: :error + defp valid_envelope?(email) do + [email.from, email.reply_to | email.to ++ email.cc ++ email.bcc] + |> List.flatten() + |> Enum.reject(&is_nil/1) + |> Enum.all?(fn + {_name, address} -> EmailAddress.valid?(address) + _invalid_recipient -> false + end) + end + defp emit(started_at, kind, status) do :telemetry.execute( [:who_need_help, :email, :delivery], diff --git a/lib/who_need_help/mail/content_removal_email_worker.ex b/lib/who_need_help/mail/content_removal_email_worker.ex index 24eeb3a..02bbcb8 100644 --- a/lib/who_need_help/mail/content_removal_email_worker.ex +++ b/lib/who_need_help/mail/content_removal_email_worker.ex @@ -54,6 +54,7 @@ defmodule WhoNeedHelp.Mail.ContentRemovalEmailWorker do defp deliver(%Notice{} = notice, "operator") do case Notifier.deliver_operator_alert(notice) do {:ok, _metadata} -> :ok + {:error, :invalid_email_address} -> {:cancel, :invalid_email_address} {:error, reason} -> {:error, reason} end end @@ -70,5 +71,8 @@ defmodule WhoNeedHelp.Mail.ContentRemovalEmailWorker do end end + defp mark_sent({:error, :invalid_email_address}, _notice, _field), + do: {:cancel, :invalid_email_address} + defp mark_sent({:error, reason}, _notice, _field), do: {:error, reason} end diff --git a/lib/who_need_help/mail/support_confirmation_worker.ex b/lib/who_need_help/mail/support_confirmation_worker.ex index 8f528c7..fa12d6c 100644 --- a/lib/who_need_help/mail/support_confirmation_worker.ex +++ b/lib/who_need_help/mail/support_confirmation_worker.ex @@ -27,6 +27,7 @@ defmodule WhoNeedHelp.Mail.SupportConfirmationWorker do %SupportRequest{} = request -> case Notifier.deliver_confirmation(request, Support.confirmation_url(request)) do {:ok, _metadata} -> :ok + {:error, :invalid_email_address} -> {:cancel, :invalid_email_address} {:error, reason} -> {:error, reason} end end diff --git a/lib/who_need_help/mail/support_operator_alert_worker.ex b/lib/who_need_help/mail/support_operator_alert_worker.ex index bf0acfd..1218467 100644 --- a/lib/who_need_help/mail/support_operator_alert_worker.ex +++ b/lib/who_need_help/mail/support_operator_alert_worker.ex @@ -24,7 +24,10 @@ defmodule WhoNeedHelp.Mail.SupportOperatorAlertWorker do {:cancel, :contact_not_verified} %SupportRequest{} = request -> - Notifier.deliver_operator_alert(request) + case Notifier.deliver_operator_alert(request) do + {:error, :invalid_email_address} -> {:cancel, :invalid_email_address} + result -> result + end end end end diff --git a/lib/who_need_help/mail/support_update_worker.ex b/lib/who_need_help/mail/support_update_worker.ex index 0a0cba9..e9402ea 100644 --- a/lib/who_need_help/mail/support_update_worker.ex +++ b/lib/who_need_help/mail/support_update_worker.ex @@ -27,6 +27,7 @@ defmodule WhoNeedHelp.Mail.SupportUpdateWorker do %SupportRequest{} = request -> case Notifier.deliver_decision(request, Support.status_url(request)) do {:ok, _metadata} -> mark_sent(request) + {:error, :invalid_email_address} -> {:cancel, :invalid_email_address} {:error, reason} -> {:error, reason} end end diff --git a/scripts/validate-production-env.sh b/scripts/validate-production-env.sh index 209e5a2..c6ca207 100755 --- a/scripts/validate-production-env.sh +++ b/scripts/validate-production-env.sh @@ -92,6 +92,27 @@ valid_nonempty_csv() { done } +valid_mailbox_address() { + local address=$1 local_part domain label + local -a labels + + [[ "$address" =~ ^[A-Za-z0-9.!#\$%\&\'*+/=\?\^_\`\{\|\}~-]+@[A-Za-z0-9.-]+$ ]] || + return 1 + + local_part=${address%@*} + domain=${address#*@} + [[ -n "$local_part" && "$local_part" != .* && "$local_part" != *. && + "$local_part" != *..* ]] || return 1 + + IFS='.' read -r -a labels <<<"$domain" + [[ ${#labels[@]} -ge 2 ]] || return 1 + + for label in "${labels[@]}"; do + [[ -n "$label" && "$label" =~ ^[A-Za-z0-9-]+$ && + "$label" != -* && "$label" != *- ]] || return 1 + done +} + valid_public_rate_limit_policy() { local json=$1 @@ -397,13 +418,13 @@ case "$email_delivery_provider" in exit 1 ;; esac -[[ "$email_from_address" == *@* ]] || { - echo "EMAIL_FROM_ADDRESS is not an email address." >&2 +valid_mailbox_address "$email_from_address" || { + echo "EMAIL_FROM_ADDRESS must be a single SMTP-safe mailbox address." >&2 exit 1 } -if [[ -n "$support_inbox_address" && - ! "$support_inbox_address" =~ ^[^@,\;[:space:]]+@[^@,\;[:space:]]+$ ]]; then - echo "SUPPORT_INBOX_ADDRESS is not an email address." >&2 +if [[ -n "$support_inbox_address" ]] && + ! valid_mailbox_address "$support_inbox_address"; then + echo "SUPPORT_INBOX_ADDRESS must be a single SMTP-safe mailbox address." >&2 exit 1 fi diff --git a/scripts/validate-test-env.sh b/scripts/validate-test-env.sh index 9c3c70e..3e9025a 100755 --- a/scripts/validate-test-env.sh +++ b/scripts/validate-test-env.sh @@ -63,6 +63,27 @@ valid_nonempty_csv() { done } +valid_mailbox_address() { + local address=$1 local_part domain label + local -a labels + + [[ "$address" =~ ^[A-Za-z0-9.!#\$%\&\'*+/=\?\^_\`\{\|\}~-]+@[A-Za-z0-9.-]+$ ]] || + return 1 + + local_part=${address%@*} + domain=${address#*@} + [[ -n "$local_part" && "$local_part" != .* && "$local_part" != *. && + "$local_part" != *..* ]] || return 1 + + IFS='.' read -r -a labels <<<"$domain" + [[ ${#labels[@]} -ge 2 ]] || return 1 + + for label in "${labels[@]}"; do + [[ -n "$label" && "$label" =~ ^[A-Za-z0-9-]+$ && + "$label" != -* && "$label" != *- ]] || return 1 + done +} + [[ "$(require_value DEPLOYMENT_ENV)" == test ]] || { echo "Test validation requires DEPLOYMENT_ENV=test." >&2 exit 1 @@ -126,7 +147,11 @@ case "$email_delivery_provider" in exit 1 ;; esac -require_value EMAIL_FROM_ADDRESS >/dev/null +email_from_address=$(require_value EMAIL_FROM_ADDRESS) +valid_mailbox_address "$email_from_address" || { + echo "EMAIL_FROM_ADDRESS must be a single SMTP-safe mailbox address." >&2 + exit 1 +} [[ "$(require_value PHX_HOST)" == "$expected_domain" && "$(require_value WNH_BASE_URL)" == "https://$expected_domain" ]] || { echo "The test public origin does not match EXPECTED_DOMAIN." >&2 diff --git a/test/who_need_help/email_address_test.exs b/test/who_need_help/email_address_test.exs index 157bd26..7e11372 100644 --- a/test/who_need_help/email_address_test.exs +++ b/test/who_need_help/email_address_test.exs @@ -1,7 +1,11 @@ defmodule WhoNeedHelp.EmailAddressTest do use ExUnit.Case, async: true + import Swoosh.Email + alias WhoNeedHelp.EmailAddress + alias WhoNeedHelp.EmailDelivery + alias Swoosh.Adapters.SMTP.Helpers test "accepts ordinary mailbox addresses used by transactional email" do assert EmailAddress.valid?("person@example.com") @@ -9,6 +13,8 @@ defmodule WhoNeedHelp.EmailAddressTest do end test "rejects addresses that the SMTP adapter cannot safely parse" do + refute EmailAddress.valid?(".person@example.com") + refute EmailAddress.valid?("person.@example.com") refute EmailAddress.valid?("two..dots@example.com") refute EmailAddress.valid?("person@example") refute EmailAddress.valid?("person@-example.com") @@ -16,4 +22,31 @@ defmodule WhoNeedHelp.EmailAddressTest do refute EmailAddress.valid?("person@exam ple.com") refute EmailAddress.valid?("first@example.com,second@example.com") end + + test "every accepted address can be encoded by the configured SMTP implementation" do + for address <- ["person@example.com", "person+alerts@example.co.uk"] do + encoded = + new() + |> from({"Who Need Help", "contact@example.com"}) + |> to(address) + |> reply_to("support@example.com") + |> subject("Email address compatibility") + |> text_body("Compatibility check") + |> Helpers.body([]) + + assert is_binary(encoded) + end + end + + test "delivery rejects an invalid envelope before invoking the mail adapter" do + email = + new() + |> from({"Who Need Help", "contact@example.com"}) + |> to(".person@example.com") + |> subject("Invalid envelope check") + |> text_body("This message must not be delivered") + + assert {:error, :invalid_email_address} = + EmailDelivery.deliver(email, :auth_login) + end end diff --git a/test/who_need_help/support_and_content_removal_test.exs b/test/who_need_help/support_and_content_removal_test.exs index 2d62088..0e06c86 100644 --- a/test/who_need_help/support_and_content_removal_test.exs +++ b/test/who_need_help/support_and_content_removal_test.exs @@ -98,6 +98,28 @@ defmodule WhoNeedHelp.SupportAndContentRemovalTest do refute_receive {:email, _duplicate_received_email}, 50 end + test "legacy support jobs with an invalid stored mailbox are cancelled without delivery" do + assert {:ok, request} = + Support.create_request(nil, %{ + "kind" => "technical_issue", + "contact_email" => "legacy@example.com", + "subject" => "Legacy queued support message", + "details" => + "This request simulates malformed historical data already stored before validation." + }) + + {1, nil} = + Repo.update_all( + from(stored_request in SupportRequest, where: stored_request.id == ^request.id), + set: [contact_email: ".legacy@example.com"] + ) + + assert {:cancel, :invalid_email_address} = + perform_job(SupportConfirmationWorker, %{"request_id" => request.id}) + + refute_receive {:email, _email}, 50 + end + test "operator alert is sent only after public contact verification" do previous = Application.get_env(:who_need_help, :support_inbox_address) previous_mode = Application.get_env(:who_need_help, :support_operator_email_mode)