diff --git a/internal/email/coverage_extra_test.go b/internal/email/coverage_extra_test.go index b2a04d0ea..4cdf19182 100644 --- a/internal/email/coverage_extra_test.go +++ b/internal/email/coverage_extra_test.go @@ -400,7 +400,7 @@ func TestSMTPSender_SendRIExchangePendingApproval_WithNotifyEmail(t *testing.T) port: 587, fromEmail: "from@example.com", notifyEmail: "notify@example.com", - // useTLS is false so sendMailTLS is not called; smtp.SendMail will be attempted + // useTLS is false so STARTTLS is not required; a plain send will be attempted // but fail — we just verify the method reaches the send path (not a no-op) useTLS: false, } @@ -714,7 +714,7 @@ var _ SNSPublisher = (*mockSNSPublisher)(nil) var _ SESEmailSender = (*mockSESEmailSender)(nil) func TestSMTPSender_SendToEmail_AuthPath(t *testing.T) { - // A sender with username/password set but useTLS=false will use smtp.SendMail. + // A sender with username/password set but useTLS=false takes the plain (non-STARTTLS) send path. // With a bogus host, it will fail at Dial — which still exercises the auth // construction branch (auth != nil when username/password are both non-empty). sender := &SMTPSender{ diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index ff9c19c9b..b53cb2583 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -6,10 +6,12 @@ import ( "crypto/rand" "crypto/tls" "encoding/hex" + "errors" "fmt" "net" "net/smtp" "strings" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -143,7 +145,7 @@ func (s *SMTPSender) SendToEmailWithCCMultipart(ctx context.Context, toEmail str msg := s.buildSMTPMessageMultipart(toEmail, sanitizedCC, subject, textBody, htmlBody) rcpts := append([]string{toEmail}, sanitizedCC...) - if err := s.dispatchSMTP(rcpts, msg); err != nil { + if err := s.dispatchSMTP(ctx, rcpts, msg); err != nil { return err } @@ -174,7 +176,7 @@ func (s *SMTPSender) SendToEmailWithCC(ctx context.Context, toEmail string, ccEm msg := s.buildSMTPMessage(toEmail, sanitizedCC, subject, body) rcpts := append([]string{toEmail}, sanitizedCC...) - if err := s.dispatchSMTP(rcpts, msg); err != nil { + if err := s.dispatchSMTP(ctx, rcpts, msg); err != nil { return err } @@ -308,9 +310,9 @@ func (s *SMTPSender) buildSMTPMessage(toEmail string, cc []string, subject, body return []byte(headers + body + "\r\n") } -// dispatchSMTP runs the actual SMTP SendMail call, routing through -// STARTTLS when s.useTLS is true. The rcpts list carries every address -// that should receive the message (To + Cc). +// dispatchSMTP runs the SMTP conversation, routing through STARTTLS when +// s.useTLS is true. The rcpts list carries every address that should receive +// the message (To + Cc). ctx bounds the whole conversation (see sendMail). // // Security (07-H2): if credentials are configured but TLS is disabled the // call is refused unless AllowInsecure was explicitly set. Sending SMTP AUTH @@ -318,7 +320,7 @@ func (s *SMTPSender) buildSMTPMessage(toEmail string, cc []string, subject, body // password) and token-bearing message bodies in cleartext. AllowInsecure must // never be set in production; it exists solely for integration tests against a // local plaintext stub server. -func (s *SMTPSender) dispatchSMTP(rcpts []string, msg []byte) error { +func (s *SMTPSender) dispatchSMTP(ctx context.Context, rcpts []string, msg []byte) error { addr := fmt.Sprintf("%s:%d", s.host, s.port) var auth smtp.Auth if s.username != "" && s.password != "" { @@ -327,13 +329,7 @@ func (s *SMTPSender) dispatchSMTP(rcpts []string, msg []byte) error { if auth != nil && !s.useTLS && !s.allowInsecure { return fmt.Errorf("SMTP auth over non-TLS connection is refused: set UseTLS=true or, for test-only stubs, AllowInsecure=true") } - if s.useTLS { - if err := s.sendMailTLS(addr, auth, s.fromEmail, rcpts, msg); err != nil { - return fmt.Errorf("failed to send email via SMTP: %w", err) - } - return nil - } - if err := smtp.SendMail(addr, auth, s.fromEmail, rcpts, msg); err != nil { + if err := s.sendMail(ctx, addr, auth, s.fromEmail, rcpts, msg); err != nil { return fmt.Errorf("failed to send email via SMTP: %w", err) } return nil @@ -344,6 +340,10 @@ func smtpAuthenticate(c *smtp.Client, auth smtp.Auth) error { if auth == nil { return nil } + // Same refusal smtp.SendMail made, so a server without AUTH fails fast. + if ok, _ := c.Extension("AUTH"); !ok { + return errors.New("smtp: server doesn't support AUTH") + } if err := c.Auth(auth); err != nil { if strings.Contains(err.Error(), "535") { return fmt.Errorf("SMTP authentication failed - check username/password") @@ -374,23 +374,60 @@ func smtpSendBody(c *smtp.Client, from string, to []string, msg []byte) error { return w.Close() } -// sendMailTLS sends email using STARTTLS (required for most modern SMTP servers). -func (s *SMTPSender) sendMailTLS(addr string, auth smtp.Auth, from string, to []string, msg []byte) error { - c, err := smtp.Dial(addr) +// sendMail sends msg over one SMTP connection that ctx bounds (issue #1433). +// net/smtp's Dial and SendMail take no context and set no deadline, so a mail +// server that accepted the TCP connection and then stopped answering held the +// caller until the OS gave up, well past the 30s failure-alert budget. Here +// the dial honors ctx, and once ctx is done (deadline or cancel) the +// connection's deadline is moved into the past, which fails any read or write +// in flight, TLS handshake included. The error then wraps ctx.Err(). +// +// With s.useTLS the server must offer STARTTLS. Without it (AllowInsecure test +// stubs, or no credentials), STARTTLS is used when the server offers it, as +// smtp.SendMail did. +func (s *SMTPSender) sendMail(ctx context.Context, addr string, auth smtp.Auth, from string, to []string, msg []byte) error { + host, _, err := net.SplitHostPort(addr) if err != nil { return err } - defer c.Close() - - host, _, err := net.SplitHostPort(addr) + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", addr) if err != nil { return err } + defer conn.Close() - // MinVersion guards against TLS 1.0/1.1 negotiation (issue #410). - if err := c.StartTLS(&tls.Config{ServerName: host, MinVersion: tls.VersionTLS12}); err != nil { + stop := context.AfterFunc(ctx, func() { + // SetDeadline only fails on a closed connection, which has nothing + // left to unblock. + if err := conn.SetDeadline(time.Unix(1, 0)); err != nil { + logging.Debugf("SMTP: expire connection deadline: %v", err) + } + }) + defer stop() + + if err := smtpConverse(conn, host, s.useTLS, auth, from, to, msg); err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return fmt.Errorf("%w: %w", ctxErr, err) + } return err } + return nil +} + +// smtpConverse runs greeting, STARTTLS, AUTH, MAIL/RCPT/DATA and QUIT on conn. +func smtpConverse(conn net.Conn, host string, requireTLS bool, auth smtp.Auth, from string, to []string, msg []byte) error { + c, err := smtp.NewClient(conn, host) + if err != nil { + return err + } + defer c.Close() + + if ok, _ := c.Extension("STARTTLS"); ok || requireTLS { + // MinVersion guards against TLS 1.0/1.1 negotiation (issue #410). + if err := c.StartTLS(&tls.Config{ServerName: host, MinVersion: tls.VersionTLS12}); err != nil { + return err + } + } if err := smtpAuthenticate(c, auth); err != nil { return err @@ -589,7 +626,7 @@ func (s *SMTPSender) sendMultipartWithUnsubscribe(ctx context.Context, toEmail s } rcpts := append([]string{toEmail}, sanitizedCC...) - if err := s.dispatchSMTP(rcpts, msg); err != nil { + if err := s.dispatchSMTP(ctx, rcpts, msg); err != nil { return err } diff --git a/internal/email/smtp_sender_test.go b/internal/email/smtp_sender_test.go index 511e76217..275ed307b 100644 --- a/internal/email/smtp_sender_test.go +++ b/internal/email/smtp_sender_test.go @@ -335,7 +335,7 @@ func TestSendRegistrationReceivedNotification_SubjectHeaderInjection(t *testing. // so that TLS 1.0/1.1 cannot be negotiated even if the server offers them. // // We cannot dial a live SMTP server in a unit test, so we verify the constant -// directly and confirm the sendMailTLS helper passes a config that enforces +// directly and confirm the sendMail STARTTLS path passes a config that enforces // the minimum version via a test-double net.Listener. func TestSMTPStartTLS_MinVersionTLS12(t *testing.T) { // Verify the constant value matches Go's crypto/tls expectation. @@ -344,7 +344,7 @@ func TestSMTPStartTLS_MinVersionTLS12(t *testing.T) { t.Fatal("tls.VersionTLS12 is zero; build environment issue") } - // Build the config the same way sendMailTLS does and confirm MinVersion. + // Build the config the same way smtpConverse does and confirm MinVersion. cfg := &tls.Config{ServerName: "smtp.example.com", MinVersion: tls.VersionTLS12} if cfg.MinVersion != tls.VersionTLS12 { t.Errorf("TLS config MinVersion is %d; want tls.VersionTLS12 (%d) (regression of #410)", diff --git a/internal/email/smtp_server_test.go b/internal/email/smtp_server_test.go index 196dba512..327fa5088 100644 --- a/internal/email/smtp_server_test.go +++ b/internal/email/smtp_server_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "net/smtp" + "strconv" "strings" "sync" "testing" @@ -198,7 +199,7 @@ func TestSMTPSender_DispatchSMTP_RefusesAuthOverCleartext(t *testing.T) { useTLS: false, allowInsecure: false, } - err := sender.dispatchSMTP([]string{"r@example.com"}, []byte("msg")) + err := sender.dispatchSMTP(context.Background(), []string{"r@example.com"}, []byte("msg")) require.Error(t, err) assert.Contains(t, err.Error(), "SMTP auth over non-TLS connection is refused") } @@ -216,7 +217,7 @@ func TestSMTPSender_DispatchSMTP_AllowsAuthWithAllowInsecure(t *testing.T) { useTLS: false, allowInsecure: true, } - err := sender.dispatchSMTP([]string{"r@example.com"}, []byte("msg")) + err := sender.dispatchSMTP(context.Background(), []string{"r@example.com"}, []byte("msg")) require.Error(t, err) // Guard error must NOT appear; a connection error is expected instead. assert.NotContains(t, err.Error(), "SMTP auth over non-TLS connection is refused") @@ -470,7 +471,7 @@ func TestSMTPSender_SendToEmail_WithMockServer_LongContent(t *testing.T) { require.NoError(t, err) } -// --- Direct sendMailTLS tests with flexible error behaviors --- +// --- Direct sendMail (STARTTLS) tests with flexible error behaviors --- // startFlexSMTPServer starts a mock SMTP server with configurable failure behavior. func startFlexSMTPServer(t *testing.T, behavior string) (string, func()) { @@ -596,7 +597,7 @@ func (a *testFlexAuth) Next(fromServer []byte, more bool) ([]byte, error) { func TestSendMailTLS_DialFail(t *testing.T) { sender := &SMTPSender{useTLS: true} - err := sender.sendMailTLS("127.0.0.1:1", nil, "from@test.com", []string{"to@test.com"}, []byte("test")) + err := sender.sendMail(context.Background(), "127.0.0.1:1", nil, "from@test.com", []string{"to@test.com"}, []byte("test")) require.Error(t, err) } @@ -606,19 +607,19 @@ func TestSendMailTLS_StartTLSFail(t *testing.T) { host, _, _ := net.SplitHostPort(addr) sender := &SMTPSender{host: host, useTLS: true} - err := sender.sendMailTLS(addr, nil, "from@test.com", []string{"to@test.com"}, []byte("test")) + err := sender.sendMail(context.Background(), addr, nil, "from@test.com", []string{"to@test.com"}, []byte("test")) require.Error(t, err) } func TestSendMailTLS_Auth535Error(t *testing.T) { - // sendMailTLS always attempts STARTTLS; mock server doesn't support it, + // sendMail with useTLS always attempts STARTTLS; mock server doesn't support it, // so the error will be a STARTTLS failure (not an auth error) addr, cleanup := startFlexSMTPServer(t, "auth_fail_535") defer cleanup() sender := &SMTPSender{useTLS: true} auth := &testFlexAuth{username: "user", password: "pass"} - err := sender.sendMailTLS(addr, auth, "from@test.com", []string{"to@test.com"}, []byte("test")) + err := sender.sendMail(context.Background(), addr, auth, "from@test.com", []string{"to@test.com"}, []byte("test")) require.Error(t, err) } @@ -627,7 +628,7 @@ func TestSendMailTLS_DataFail(t *testing.T) { defer cleanup() sender := &SMTPSender{useTLS: true} - err := sender.sendMailTLS(addr, nil, "from@test.com", []string{"to@test.com"}, []byte("test")) + err := sender.sendMail(context.Background(), addr, nil, "from@test.com", []string{"to@test.com"}, []byte("test")) require.Error(t, err) } @@ -661,3 +662,93 @@ func TestSMTPSender_SendPurchaseFailedNotification_Recipient(t *testing.T) { }) } } + +// --- issue #1433: the SMTP conversation honors the caller's context --- + +// startStallingSMTPServer accepts connections and then stops answering. With +// greet=false it never sends the 220 greeting; with greet=true it greets, +// answers EHLO (offering AUTH but not STARTTLS), then goes silent. Connections stay +// open until the test's cleanup, as a hung mail server's would. +func startStallingSMTPServer(t *testing.T, greet bool) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + release := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for { + conn, err := ln.Accept() + if err != nil { + return + } + wg.Add(1) + go func() { + defer wg.Done() + defer conn.Close() + if greet { + fmt.Fprintf(conn, "220 localhost stalling server\r\n") + if _, err := bufio.NewReader(conn).ReadString('\n'); err == nil { + fmt.Fprintf(conn, "250-localhost\r\n250 AUTH PLAIN\r\n") + } + } + <-release + }() + } + }() + t.Cleanup(func() { + close(release) + ln.Close() + wg.Wait() + }) + return ln.Addr().String() +} + +// TestSMTPSender_SendHonorsContext pins #1433: a send to a server that accepts +// the connection and then stalls returns soon after the context ends, with an +// error wrapping the context's error, on both the STARTTLS path and the plain +// AllowInsecure path. Before the fix the call blocked until the server let go. +func TestSMTPSender_SendHonorsContext(t *testing.T) { + for _, tc := range []struct { + name string + useTLS, greet bool + cancel bool + want error + }{ + {name: "starttls, no greeting", useTLS: true, want: context.DeadlineExceeded}, + {name: "starttls, stalls after EHLO", useTLS: true, greet: true, want: context.DeadlineExceeded}, + {name: "plain, no greeting", want: context.DeadlineExceeded}, + {name: "plain, stalls after EHLO", greet: true, want: context.DeadlineExceeded}, + {name: "plain, canceled", greet: true, cancel: true, want: context.Canceled}, + } { + t.Run(tc.name, func(t *testing.T) { + host, portStr, err := net.SplitHostPort(startStallingSMTPServer(t, tc.greet)) + require.NoError(t, err) + port, err := strconv.Atoi(portStr) + require.NoError(t, err) + sender := &SMTPSender{host: host, port: port, fromEmail: "sender@test.com", useTLS: tc.useTLS} + if !tc.useTLS { + // The plain path with credentials is the AllowInsecure test path. + sender.username, sender.password, sender.allowInsecure = "user", "pass", true + } + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + if tc.cancel { + time.AfterFunc(50*time.Millisecond, cancel) + } + start := time.Now() + errCh := make(chan error, 1) + go func() { errCh <- sender.SendToEmail(ctx, "recipient@test.com", "Subject", "Body") }() + + select { + case err := <-errCh: + require.ErrorIs(t, err, tc.want) + assert.Less(t, time.Since(start), 2*time.Second) + case <-time.After(5 * time.Second): + t.Fatal("SendToEmail ignored its context: still blocked 5s after a 200ms deadline") + } + }) + } +}