From b3989fa4afe2e7e5ad67485720af7a69be7e31d0 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 21 Jul 2026 00:11:40 +0900 Subject: [PATCH] fix(mail): prove SMTP deliverability before saving, and stop losing the relay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live install passed the SMTP setup screen and then failed every one-time code with a bare `internal error`. Four separate defects had to line up for that, and each is fixed here. The relay was configured with `from = noreply@` on an account authenticated as `@`. Providers that validate sender identity — Fastmail among them — answer MAIL FROM with an unconditional 250 and only refuse at end-of-DATA. Ping stopped at NOOP, so it never saw the refusal: the wizard reported success, wrote the config, rolled felis-api, and every OTP afterwards died at w.Close(). Ping now runs the same transaction a real code takes — connect, (STARTTLS,) AUTH, MAIL FROM, RCPT TO, DATA — delivering one self-test message to the From address, and SendOTP and Ping share deliver() so the check cannot drift from the thing it checks. The self-test recipient cannot cause a false negative: an authenticated submission relay accepts RCPT for any destination by definition, while the sender identity it does validate is exactly what we want tested. The setup screen now says a message will be sent, names the address it went to, and warns that From must be an address the account is allowed to send as. A relay refusal also answered 500 `internal`, which reads as a broken panel and sends the operator hunting through handler code instead of their [smtp] block. It is now 502 `mail_undeliverable`, mapped inside deliverOTP so all four doors that mail a code (onboarding, email login, op-login, migrate step-up) answer alike. The relay's own text stays out of the response — it can name the SMTP account, and these routes are reachable by any signed-in player — and goes to the log instead. writeError logged nothing when it collapsed an unmapped error to 500, so an operator holding an `internal error` had nothing to grep for and diagnosis degraded into guessing against a live install. It now logs the method, path, wrapped chain and the same request_id the caller is shown. Finally, write_felis_toml regenerated the config wholesale and never emitted [smtp], so re-running the installer — the documented way to update felis-api — silently erased a working relay and reverted OTP delivery to the no-Mailer path, logging codes instead of sending them. It now carries the block forward, cached on first read because the host toml is clobbered before the pod toml is written. Same defect family as the root_domain loss fixed in ecbeb20: a generated file holding a hand-set value with no carry-forward. Tests cover the case a MAIL FROM probe cannot see: a fake relay that answers 250 to MAIL FROM and 550 at end-of-DATA must fail both Ping and SendOTP, and the 502 must carry a distinct machine code without leaking the relay's text. --- cmd/felis/tui_smtp.go | 24 +++-- deploy/bootstrap.sh | 38 ++++++- docs/openapi.yaml | 20 ++++ internal/api/errors.go | 10 ++ internal/api/handlers_email_otp.go | 16 ++- internal/api/handlers_email_otp_test.go | 37 +++++++ internal/mail/mail.go | 103 +++++++++++++----- internal/mail/mail_test.go | 133 ++++++++++++++++++++++++ 8 files changed, 346 insertions(+), 35 deletions(-) diff --git a/cmd/felis/tui_smtp.go b/cmd/felis/tui_smtp.go index a3ccb42..ff7e729 100644 --- a/cmd/felis/tui_smtp.go +++ b/cmd/felis/tui_smtp.go @@ -89,7 +89,7 @@ func (m *smtpModel) build() *huh.Form { return m.sized(newFelisForm(huh.NewGroup( huh.NewNote(). Title("Email (SMTP)"). - Description("The relay Felis mails one-time codes through — email verification, email login and operator sign-in all need it. The password goes into a Kubernetes Secret; only the other fields are written to felis.toml."), + Description("The relay Felis mails one-time codes through — email verification, email login and operator sign-in all need it. The password goes into a Kubernetes Secret; only the other fields are written to felis.toml. Saving sends one self-test message to the From address: nothing is written unless it is delivered."), huh.NewInput(). Title("SMTP host"). Description("Your provider's relay, e.g. smtp.gmail.com or smtp.mailgun.org."). @@ -102,7 +102,7 @@ func (m *smtpModel) build() *huh.Form { Validate(validateSMTPPort), huh.NewInput(). Title("From address"). - Description("The sender codes are mailed as, e.g. felis@your-domain."). + Description("The sender codes are mailed as, e.g. felis@your-domain. It must be an address this account is allowed to send as — providers reject a From on a domain you have not verified with them, and they usually do it only after the message body, not when you connect."). Value(&m.in.from). Validate(validateSMTPFrom), huh.NewInput(). @@ -225,11 +225,14 @@ func (m *smtpModel) normalizeInputs() { func (m *smtpModel) View() string { switch m.step { case esWorking: - return " " + m.sp.View() + " " + tuiHint.Render("Verifying the relay, saving email settings and rolling the API…") + "\n" + return " " + m.sp.View() + " " + tuiHint.Render("Delivering a self-test message, saving email settings and rolling the API…") + "\n" case esDone: var b strings.Builder b.WriteString(tuiSuccessBanner("Email configured — codes are now mailed.") + "\n\n") b.WriteString(tuiInfo("Relay → "+smtpDetail(m.in)) + "\n") + // Named because it is checkable: the operator can open that inbox and see the + // proof, rather than taking "configured" on faith. + b.WriteString(tuiHint.Render("A self-test message was delivered to "+m.in.from+".") + "\n") b.WriteString("\n" + tuiAction("enter", "continue")) return b.String() case esError: @@ -290,9 +293,18 @@ func currentSMTPInputs() smtpInputs { } // applySMTPConfig proves the relay works, then persists it and rolls felis-api: -// Ping (connect/STARTTLS/AUTH, no mail sent) → [smtp] into both config files → -// the felis-smtp Secret → the config Secret → rollout. A failed Ping leaves the -// install untouched, so a typo dies at the keyboard, not at a player's OTP. +// Ping (a full transaction — connect/STARTTLS/AUTH/MAIL FROM/RCPT/DATA, which +// delivers one self-test message to the From address) → [smtp] into both config +// files → the felis-smtp Secret → the config Secret → rollout. A failed Ping +// leaves the install untouched, so a bad relay dies at the keyboard, not at a +// player's OTP. +// +// Ping really sends, because a cheaper probe cannot answer the question this +// screen exists to answer. Relays that validate sender identity — Fastmail, and +// it is not alone — return an unconditional 250 to MAIL FROM and only refuse at +// end-of-DATA. The earlier connect/AUTH/NOOP check therefore accepted a From on +// a domain the account could not send as, wrote the config, and left every OTP +// failing afterwards with this screen reporting success. func applySMTPConfig(ctx context.Context, in smtpInputs) error { port, err := strconv.Atoi(in.port) if err != nil { diff --git a/deploy/bootstrap.sh b/deploy/bootstrap.sh index 06b9bc7..676b84b 100644 --- a/deploy/bootstrap.sh +++ b/deploy/bootstrap.sh @@ -1668,8 +1668,43 @@ EOF ok "panel TLS certificate ready (${PANEL_TLS_CERT})" } +# persisted_smtp_block echoes the [smtp] section an earlier run left behind, or +# nothing. Unlike every other value in the generated toml, [smtp] is not derived +# from this script's inputs -- `felis setup`'s SMTP screen writes it, after +# proving the relay works. A wholesale `cat >` therefore erases it on every +# re-run, and since re-running the installer is the documented way to update +# felis-api, an operator who updates loses mail: OTP delivery silently reverts +# to the no-Mailer path and every code is logged instead of sent. Same defect +# family as the root_domain loss fixed in ecbeb20 -- generated file, hand-set +# value, no carry-forward. +# +# Cached on first call because write_felis_toml clobbers felis.host.toml before +# it is called again for felis.pod.toml: by then the file this would read from +# no longer has the block. The pod toml is the fallback for exactly that window. +persisted_smtp_block() { + if [ -z "${SMTP_BLOCK_CACHED:-}" ]; then + SMTP_BLOCK_CACHED=1 + SMTP_BLOCK="" + local f + for f in "${STATE_DIR}/felis.host.toml" "${STATE_DIR}/felis.pod.toml"; do + [ -r "$f" ] || continue + # Print from [smtp] up to (not including) the next section header. + SMTP_BLOCK="$(awk '/^[[:space:]]*\[smtp\]/ { f=1 } + f && /^[[:space:]]*\[/ && !/^[[:space:]]*\[smtp\]/ { exit } + f { print }' "$f")" + [ -n "$SMTP_BLOCK" ] && break + done + fi + printf '%s' "$SMTP_BLOCK" +} + write_felis_toml() { - local target="$1" db_host="$2" + local target="$1" db_host="$2" smtp_block + smtp_block="$(persisted_smtp_block)" + if [ -n "$smtp_block" ]; then + log "carrying forward the configured [smtp] relay" + smtp_block="${smtp_block}"$'\n' # keep a blank line before the next section + fi cat > "$target" < + The configured SMTP relay refused the message (code mail_undeliverable), so no + code was delivered. Distinct from 500 because the fault is in the install's + [smtp] settings, not in the request or the platform — most often a From address + the relay will not let this account send as. The relay's own text is deliberately + withheld (it names the SMTP account) and written to the felis-api log instead, + keyed by the same request_id this response carries. Retrying the same address + changes nothing until an operator fixes the relay. + content: + application/json: + schema: { $ref: '#/components/schemas/Error' } AccessResult: description: The structured access mutation succeeded; the raw RCON reply is in output. content: @@ -2105,6 +2117,8 @@ paths: content: application/json: schema: { $ref: '#/components/schemas/Error' } + '502': + $ref: '#/components/responses/MailUndeliverable' /api/v1/auth/email/verify: post: @@ -2219,6 +2233,8 @@ paths: content: application/json: schema: { $ref: '#/components/schemas/Error' } + '502': + $ref: '#/components/responses/MailUndeliverable' /api/v1/auth/op-login/status/{id}: get: @@ -3531,6 +3547,8 @@ paths: schema: { $ref: '#/components/schemas/Error' } '401': $ref: '#/components/responses/Unauthorized' + '502': + $ref: '#/components/responses/MailUndeliverable' /api/v1/account/email/verify: post: @@ -3835,6 +3853,8 @@ paths: content: application/json: schema: { $ref: '#/components/schemas/Error' } + '502': + $ref: '#/components/responses/MailUndeliverable' /api/v1/account/migrate/confirm/otp/verify: post: diff --git a/internal/api/errors.go b/internal/api/errors.go index c5d60d1..1e2f38b 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "log" "net/http" ) @@ -103,9 +104,18 @@ func writeJSON(w http.ResponseWriter, status int, v any) { // writeError renders err as the standard error envelope. Non-apiError values // collapse to a 500 so driver/internal details never reach the client. +// +// That collapse is deliberately lossy on the wire and deliberately NOT lossy in +// the log. Everything the client is denied — the driver message, the wrapped +// chain, the handler that produced it — is written to stderr first, keyed by the +// same request_id the caller is shown. Without that line an operator holding a +// "internal error" has nothing to grep for, and diagnosis degrades into guessing +// against a live install; it cost a full debugging session to learn that once. func writeError(w http.ResponseWriter, r *http.Request, err error) { var ae *apiError if !errors.As(err, &ae) { + log.Printf("api: %s %s: unmapped error (request_id=%s): %v", + r.Method, r.URL.Path, requestIDFromContext(r.Context()), err) ae = newError(http.StatusInternalServerError, "internal", "internal error") } body := map[string]any{ diff --git a/internal/api/handlers_email_otp.go b/internal/api/handlers_email_otp.go index 2c37ffc..f1571c9 100644 --- a/internal/api/handlers_email_otp.go +++ b/internal/api/handlers_email_otp.go @@ -227,7 +227,21 @@ func (a *API) deliverOTP(ctx context.Context, email, code string) error { log.Printf("email-otp: no Mailer configured; code for %s is %s (KNOWN-LIMITATION: demo has no SMTP)", email, code) return nil } - return a.Mailer.SendOTP(ctx, email, code) + if err := a.Mailer.SendOTP(ctx, email, code); err != nil { + // Mapped here rather than at each of the four call sites, so every door that + // mails a code answers the same way. A relay refusal is neither the caller's + // fault nor a bug in Felis, and a bare 500 says neither — it reads as "the + // panel is broken" and sends the operator hunting through handler code + // instead of their [smtp] block. + // + // The relay's own text stays in the log: it can name the SMTP account and the + // sending identity ("smtp: auth as ops@example.net: 535 …"), and these routes + // are reachable by any signed-in player. + log.Printf("api: OTP delivery failed (request_id=%s): %v", requestIDFromContext(ctx), err) + return newError(http.StatusBadGateway, "mail_undeliverable", + "the mail relay refused this message; ask the server operator to check the SMTP settings") + } + return nil } // setEmailRequest is the record-email body: the address to bind to the caller's diff --git a/internal/api/handlers_email_otp_test.go b/internal/api/handlers_email_otp_test.go index af75113..0715719 100644 --- a/internal/api/handlers_email_otp_test.go +++ b/internal/api/handlers_email_otp_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "sync" "sync/atomic" "testing" @@ -458,6 +459,42 @@ func TestEmailOTPStartFailedDeliveryReleasesCooldown(t *testing.T) { } } +// leakyMailer fails with the shape a real relay error carries: the SMTP account +// and the relay's numeric refusal are both in the text. +type leakyMailer struct{} + +func (leakyMailer) SendOTP(_ context.Context, _, _ string) error { + return errors.New("smtp: auth as ops@example.net: 535 5.7.8 bad credentials") +} + +// TestEmailOTPStartRelayRefusalIsNotInternalError pins both halves of the +// delivery-failure contract. A relay that refuses the message is not a bug in +// Felis, so it must not answer "internal error" — that reads as a broken panel +// and sends the operator hunting through handler code instead of their [smtp] +// block. And the relay's own text must not ride along on the wire: it names the +// SMTP account, while this route is reachable by any signed-in player. +func TestEmailOTPStartRelayRefusalIsNotInternalError(t *testing.T) { + user := &Principal{UserID: "u1", Email: "u1@example.net", Role: "user"} + api := newTestAPI(newFakeRepo(), newFakeCluster()) + api.External = staticExternal{p: user} + api.Mailer = leakyMailer{} + eh := api.ExternalHandler() + + w := do(eh, "POST", "/api/v1/account/email/start", `{"email":"player@example.net"}`, nil) + if w.Code != http.StatusBadGateway { + t.Fatalf("code = %d, want 502 (a relay refusal is not an internal error) (%s)", w.Code, w.Body.String()) + } + body := w.Body.String() + if !strings.Contains(body, "mail_undeliverable") { + t.Errorf("want a distinct machine code the panel can explain, got: %s", body) + } + for _, leak := range []string{"ops@example.net", "535", "bad credentials"} { + if strings.Contains(body, leak) { + t.Errorf("relay text %q leaked to an unprivileged caller: %s", leak, body) + } + } +} + // gateMailer blocks every SendOTP until all concurrent callers have arrived, making // any check-then-act window in the throttle deterministically observable instead of // scheduler-dependent. It is the committed counterpart of the adversarial burst probe. diff --git a/internal/mail/mail.go b/internal/mail/mail.go index 1e51b0b..56d79b9 100644 --- a/internal/mail/mail.go +++ b/internal/mail/mail.go @@ -47,41 +47,69 @@ func (s *SMTP) SendOTP(ctx context.Context, email, code string) error { return err } defer c.Close() - if err := c.Mail(s.From); err != nil { - return fmt.Errorf("smtp: MAIL FROM %s: %w", s.From, err) - } - if err := c.Rcpt(email); err != nil { - return fmt.Errorf("smtp: RCPT TO: %w", err) - } - w, err := c.Data() - if err != nil { - return fmt.Errorf("smtp: DATA: %w", err) - } - if _, err := w.Write(message(s.From, email, code, time.Now())); err != nil { - return fmt.Errorf("smtp: write message: %w", err) - } - if err := w.Close(); err != nil { - return fmt.Errorf("smtp: deliver: %w", err) + if err := s.deliver(c, email, message(s.From, email, code, time.Now())); err != nil { + return err } return c.Quit() } -// Ping proves the configured relay is reachable and the credentials work -// WITHOUT sending any mail: connect, (STARTTLS,) AUTH, NOOP, QUIT. The setup -// wizard runs it before writing anything, so a typo fails at the keyboard -// instead of at the first code a player is waiting on. +// Ping proves the configured relay will actually ACCEPT mail from this sender, +// by running a complete transaction — connect, (STARTTLS,) AUTH, MAIL FROM, +// RCPT TO, DATA — and delivering a short self-test message to From itself. The +// setup wizard runs it before writing anything, so a bad relay fails at the +// keyboard instead of at the first code a player is waiting on. +// +// It really does send that one message, and it has to: a probe that stops at +// NOOP (or even at MAIL FROM) proves nothing about deliverability, because +// relays which validate sender identity answer MAIL FROM with an unconditional +// 250 and defer the verdict to end-of-DATA. Fastmail does exactly that, and a +// NOOP-only Ping green-lit a From on a domain the account could not send as — +// every OTP after it died at w.Close() with the wizard reporting success. +// +// Addressing the self-test to From cannot cause a false negative: this is an +// authenticated submission relay, whose job is to accept RCPT for any +// destination, so the recipient is never what a refusal is about — while the +// sender identity, which is, still gets checked. It also puts the proof +// somewhere the operator can go look at it. func (s *SMTP) Ping(ctx context.Context) error { c, err := s.connect(ctx) if err != nil { return err } defer c.Close() - if err := c.Noop(); err != nil { - return fmt.Errorf("smtp: noop: %w", err) + if err := s.deliver(c, s.From, selfTest(s.From, time.Now())); err != nil { + return err } return c.Quit() } +// deliver runs one MAIL FROM → RCPT TO → DATA transaction on an established +// client. SendOTP and Ping share it so the wizard's check exercises the exact +// path a player's code takes — a check that skips a step is a check that can +// pass while the real send fails. +func (s *SMTP) deliver(c *smtp.Client, to string, msg []byte) error { + if err := c.Mail(s.From); err != nil { + return fmt.Errorf("smtp: MAIL FROM %s: %w", s.From, err) + } + if err := c.Rcpt(to); err != nil { + return fmt.Errorf("smtp: RCPT TO: %w", err) + } + w, err := c.Data() + if err != nil { + return fmt.Errorf("smtp: DATA: %w", err) + } + if _, err := w.Write(msg); err != nil { + return fmt.Errorf("smtp: write message: %w", err) + } + // End-of-DATA is where a relay renders its real verdict on the sender, so + // this error names From: "550 …" here almost always means the account is + // not allowed to send as that address. + if err := w.Close(); err != nil { + return fmt.Errorf("smtp: relay refused mail from %s: %w", s.From, err) + } + return nil +} + // connect dials the relay, upgrades to TLS per the port's posture, and // authenticates when a username is configured. The whole conversation shares // one deadline (the context's, else sendTimeout from now). @@ -130,19 +158,27 @@ func (s *SMTP) connect(ctx context.Context) (*smtp.Client, error) { return c, nil } -// message renders the one mail shape Felis sends: RFC 5322 headers (CRLF, the -// subject Q-encoded for its non-ASCII half) over a short bilingual plain-text -// body carrying the code. Split out from SendOTP so the shape is testable -// without a relay. -func message(from, to, code string, now time.Time) []byte { +// headers renders the RFC 5322 header block every Felis message shares: CRLF +// throughout, the subject Q-encoded because both subjects carry non-ASCII, and +// the blank line that ends the block. +func headers(from, to, subject string, now time.Time) string { var b strings.Builder b.WriteString("From: " + from + "\r\n") b.WriteString("To: " + to + "\r\n") - b.WriteString("Subject: " + mime.QEncoding.Encode("utf-8", "Felis 验证码 · verification code") + "\r\n") + b.WriteString("Subject: " + mime.QEncoding.Encode("utf-8", subject) + "\r\n") b.WriteString("Date: " + now.Format(time.RFC1123Z) + "\r\n") b.WriteString("MIME-Version: 1.0\r\n") b.WriteString("Content-Type: text/plain; charset=utf-8\r\n") b.WriteString("\r\n") + return b.String() +} + +// message renders the one mail players receive: a short bilingual plain-text +// body carrying the code. Split out from SendOTP so the shape is testable +// without a relay. +func message(from, to, code string, now time.Time) []byte { + var b strings.Builder + b.WriteString(headers(from, to, "Felis 验证码 · verification code", now)) b.WriteString("Your Felis verification code / Felis 验证码:\r\n") b.WriteString("\r\n") b.WriteString(" " + code + "\r\n") @@ -150,3 +186,16 @@ func message(from, to, code string, now time.Time) []byte { b.WriteString("If you didn't request this, ignore this message. / 若非本人操作，请忽略此邮件。\r\n") return []byte(b.String()) } + +// selfTest renders the message Ping delivers to the sender itself. It carries +// no code and says why it arrived, so an operator who finds it in the inbox +// reads it as the wizard's proof of delivery rather than a stray OTP. +func selfTest(from string, now time.Time) []byte { + var b strings.Builder + b.WriteString(headers(from, from, "Felis SMTP 自检 · relay self-test", now)) + b.WriteString("Felis accepted this relay because it accepted this message.\r\n") + b.WriteString("Felis 已确认该邮件中继可用：本邮件即为投递证明。\r\n") + b.WriteString("\r\n") + b.WriteString("Sent by `felis setup` when the SMTP relay was configured. / 由 `felis setup` 配置 SMTP 时发出。\r\n") + return []byte(b.String()) +} diff --git a/internal/mail/mail_test.go b/internal/mail/mail_test.go index e3fb272..93f6635 100644 --- a/internal/mail/mail_test.go +++ b/internal/mail/mail_test.go @@ -1,6 +1,11 @@ package mail import ( + "bufio" + "context" + "io" + "net" + "strconv" "strings" "testing" "time" @@ -39,3 +44,131 @@ func TestMessageShape(t *testing.T) { t.Errorf("body missing the code:\n%s", body) } } + +// TestSelfTestCarriesNoCode keeps Ping's message distinguishable from a real +// OTP: an operator finding it in the inbox must read it as the wizard's proof +// of delivery, not as a code someone requested on their account. +func TestSelfTestCarriesNoCode(t *testing.T) { + msg := string(selfTest("felis@example.net", time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC))) + if strings.Contains(msg, "verification code") { + t.Errorf("self-test must not read as an OTP mail:\n%s", msg) + } + if !strings.Contains(msg, "To: felis@example.net") { + t.Errorf("self-test must be addressed to the sender itself:\n%s", msg) + } +} + +// fakeRelay speaks just enough SMTP for net/smtp, answering 250 to MAIL FROM +// and RCPT TO but dataVerdict at end-of-DATA. That split is the entire point: +// relays which validate sender identity (Fastmail among them) accept MAIL FROM +// unconditionally and only render their verdict after the message body, so a +// probe stopping short of end-of-DATA reports a green relay that cannot send. +func fakeRelay(t *testing.T, dataVerdict string) (string, int) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go serveFakeRelay(conn, dataVerdict) + } + }() + host, portStr, err := net.SplitHostPort(ln.Addr().String()) + if err != nil { + t.Fatal(err) + } + port, err := strconv.Atoi(portStr) + if err != nil { + t.Fatal(err) + } + return host, port +} + +func serveFakeRelay(conn net.Conn, dataVerdict string) { + defer conn.Close() + br := bufio.NewReader(conn) + io.WriteString(conn, "220 fake ESMTP\r\n") + for { + line, err := br.ReadString('\n') + if err != nil { + return + } + switch cmd := strings.ToUpper(strings.TrimSpace(line)); { + case strings.HasPrefix(cmd, "EHLO"), strings.HasPrefix(cmd, "HELO"): + io.WriteString(conn, "250 fake\r\n") + case strings.HasPrefix(cmd, "MAIL FROM"), strings.HasPrefix(cmd, "RCPT TO"): + io.WriteString(conn, "250 2.1.0 Ok\r\n") + case strings.HasPrefix(cmd, "DATA"): + io.WriteString(conn, "354 go ahead\r\n") + for { + l, err := br.ReadString('\n') + if err != nil { + return + } + if strings.TrimRight(l, "\r\n") == "." { + break + } + } + io.WriteString(conn, dataVerdict+"\r\n") + case strings.HasPrefix(cmd, "QUIT"): + io.WriteString(conn, "221 bye\r\n") + return + default: + io.WriteString(conn, "250 ok\r\n") + } + } +} + +// TestPingRejectsRelayThatRefusesAtEndOfData is the regression this whole +// change exists for. A live install passed the wizard with a From on a domain +// the relay would not send as, then failed every OTP: the old Ping stopped at +// NOOP, and even a MAIL FROM probe would have been waved through with a 250. +// Only a full transaction sees the refusal. +func TestPingRejectsRelayThatRefusesAtEndOfData(t *testing.T) { + host, port := fakeRelay(t, "550 5.7.1 sender identity not allowed") + s := &SMTP{Host: host, Port: port, From: "noreply@example.net"} + + err := s.Ping(context.Background()) + if err == nil { + t.Fatal("Ping accepted a relay that refuses this sender at end-of-DATA") + } + // The operator has to be able to act on this, which means seeing both the + // relay's own refusal and which address it refused. + if !strings.Contains(err.Error(), "550") { + t.Errorf("want the relay's refusal surfaced, got: %v", err) + } + if !strings.Contains(err.Error(), "noreply@example.net") { + t.Errorf("want the rejected sender named, got: %v", err) + } +} + +// The counterpart: a relay that accepts the message must leave the wizard happy. +func TestPingAcceptsDeliverableRelay(t *testing.T) { + host, port := fakeRelay(t, "250 2.0.0 Ok") + s := &SMTP{Host: host, Port: port, From: "noreply@example.net"} + + if err := s.Ping(context.Background()); err != nil { + t.Fatalf("Ping rejected a relay that accepted the message: %v", err) + } +} + +// SendOTP must fail on the same relay Ping rejects — they share deliver(), and +// that shared path is what makes the wizard's check meaningful. +func TestSendOTPSurfacesEndOfDataRefusal(t *testing.T) { + host, port := fakeRelay(t, "550 5.7.1 sender identity not allowed") + s := &SMTP{Host: host, Port: port, From: "noreply@example.net"} + + err := s.SendOTP(context.Background(), "player@example.org", "042137") + if err == nil { + t.Fatal("SendOTP reported success against a relay that refused the message") + } + if !strings.Contains(err.Error(), "550") { + t.Errorf("want the relay's refusal surfaced, got: %v", err) + } +}