fix(email): bound SMTP sends by the caller's context

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{

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,17 +310,17 @@ 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
 // over a non-TLS connection exposes credentials (SendGrid API key, Azure ACS
 // 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
 	}
 

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)",

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