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) + } +}