diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go index 066065ba68..5dfcc43851 100644 --- a/coderd/notifications/dispatch/smtp.go +++ b/coderd/notifications/dispatch/smtp.go @@ -156,11 +156,11 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery } // Sender identification. - from, err := s.validateFromAddr(s.cfg.From.String()) + envelopeFrom, headerFrom, err := s.validateFromAddr(s.cfg.From.String()) if err != nil { return false, xerrors.Errorf("'from' validation: %w", err) } - err = c.Mail(from, &smtp.MailOptions{}) + err = c.Mail(envelopeFrom, &smtp.MailOptions{}) if err != nil { // This is retryable because the server may be temporarily down. return true, xerrors.Errorf("sender identification: %w", err) @@ -200,7 +200,7 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery msg := &bytes.Buffer{} multipartBuffer := &bytes.Buffer{} multipartWriter := multipart.NewWriter(multipartBuffer) - _, _ = fmt.Fprintf(msg, "From: %s\r\n", from) + _, _ = fmt.Fprintf(msg, "From: %s\r\n", headerFrom) _, _ = fmt.Fprintf(msg, "To: %s\r\n", strings.Join(recipients, ", ")) _, _ = fmt.Fprintf(msg, "Subject: %s\r\n", subject) _, _ = fmt.Fprintf(msg, "Message-Id: %s@%s\r\n", msgID, s.hostname()) @@ -486,15 +486,25 @@ func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, erro return nil, errs } -func (*SMTPHandler) validateFromAddr(from string) (string, error) { +// validateFromAddr parses the "from" address and returns two values: +// 1. envelopeFrom: The bare email address for use in the SMTP MAIL FROM command. +// 2. headerFrom: The original address (possibly including display name) for use in the email header. +// +// This separation is necessary because SMTP envelope addresses (used in MAIL FROM +// and RCPT TO commands) must be bare email addresses, while email headers can +// include display names (e.g., "John Doe "). +func (*SMTPHandler) validateFromAddr(from string) (envelopeFrom, headerFrom string, err error) { addrs, err := mail.ParseAddressList(from) if err != nil { - return "", xerrors.Errorf("parse 'from' address: %w", err) + return "", "", xerrors.Errorf("parse 'from' address: %w", err) } if len(addrs) != 1 { - return "", ErrValidationNoFromAddress + return "", "", ErrValidationNoFromAddress } - return from, nil + // Use the parsed email address for the SMTP envelope (MAIL FROM command), + // but preserve the original string for the email header (which may include + // a display name). + return addrs[0].Address, from, nil } func (s *SMTPHandler) validateToAddrs(to string) ([]string, error) { diff --git a/coderd/notifications/dispatch/smtp_internal_test.go b/coderd/notifications/dispatch/smtp_internal_test.go new file mode 100644 index 0000000000..cc193673f0 --- /dev/null +++ b/coderd/notifications/dispatch/smtp_internal_test.go @@ -0,0 +1,81 @@ +package dispatch + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateFromAddr(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expectedEnvelope string + expectedHeader string + expectedErrContain string + }{ + { + name: "bare email address", + input: "system@coder.com", + expectedEnvelope: "system@coder.com", + expectedHeader: "system@coder.com", + }, + { + name: "email with display name", + input: "Coder System ", + expectedEnvelope: "system@coder.com", + expectedHeader: "Coder System ", + }, + { + name: "email with quoted display name", + input: `"Coder Notifications" `, + expectedEnvelope: "notifications@coder.com", + expectedHeader: `"Coder Notifications" `, + }, + { + name: "email with special characters in display name", + input: `"O'Brien, John" `, + expectedEnvelope: "john@example.com", + expectedHeader: `"O'Brien, John" `, + }, + { + name: "invalid email address", + input: "not-an-email", + expectedErrContain: "parse 'from' address", + }, + { + name: "empty string", + input: "", + expectedErrContain: "parse 'from' address", + }, + { + name: "multiple addresses", + input: "a@example.com, b@example.com", + expectedErrContain: "'from' address not defined", + }, + } + + handler := &SMTPHandler{} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + envelope, header, err := handler.validateFromAddr(tc.input) + + if tc.expectedErrContain != "" { + require.Error(t, err) + require.ErrorContains(t, err, tc.expectedErrContain) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expectedEnvelope, envelope, + "envelope address should be the bare email") + require.Equal(t, tc.expectedHeader, header, + "header address should preserve the original input") + }) + } +} diff --git a/coderd/notifications/dispatch/smtp_test.go b/coderd/notifications/dispatch/smtp_test.go index 7b6e5ebc2d..34aed0feed 100644 --- a/coderd/notifications/dispatch/smtp_test.go +++ b/coderd/notifications/dispatch/smtp_test.go @@ -515,3 +515,124 @@ func TestSMTP(t *testing.T) { }) } } + +// TestSMTPEnvelopeAndHeaders verifies that SMTP envelope addresses (used in +// MAIL FROM and RCPT TO commands) contain only bare email addresses, while +// email headers preserve the full address including display names. +// +// This is important because RFC 5321 requires envelope addresses to be bare +// emails, while RFC 5322 allows headers to include display names. +// +// See: https://github.com/coder/coder/issues/20727 +func TestSMTPEnvelopeAndHeaders(t *testing.T) { + t.Parallel() + + const ( + hello = "localhost" + to = "bob@bob.com" + + subject = "This is the subject" + body = "This is the body" + ) + + tests := []struct { + name string + fromConfig string // The configured From address (may include display name) + expectedEnvFrom string // Expected envelope MAIL FROM (bare email) + expectedHeaderFrom string // Expected From header (preserves display name) + }{ + { + name: "bare email address", + fromConfig: "system@coder.com", + expectedEnvFrom: "system@coder.com", + expectedHeaderFrom: "system@coder.com", + }, + { + name: "email with display name", + fromConfig: "Coder System ", + expectedEnvFrom: "system@coder.com", + expectedHeaderFrom: "Coder System ", + }, + { + name: "email with quoted display name", + fromConfig: `"Coder Notifications" `, + expectedEnvFrom: "notifications@coder.com", + expectedHeaderFrom: `"Coder Notifications" `, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + cfg := codersdk.NotificationsEmailConfig{ + Hello: serpent.String(hello), + From: serpent.String(tc.fromConfig), + } + + backend := smtptest.NewBackend(smtptest.Config{ + AuthMechanisms: []string{}, + }) + + srv, listen, err := smtptest.CreateMockSMTPServer(backend, false) + require.NoError(t, err) + t.Cleanup(func() { + assert.ErrorIs(t, srv.Shutdown(ctx), smtp.ErrServerClosed) + }) + + var hp serpent.HostPort + require.NoError(t, hp.Set(listen.Addr().String())) + cfg.Smarthost = serpent.String(hp.String()) + + handler := dispatch.NewSMTPHandler(cfg, logger.Named("smtp")) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + assert.NoError(t, srv.Serve(listen)) + }() + + require.Eventually(t, func() bool { + cl, err := smtptest.PingClient(listen, false, false) + if err != nil { + return false + } + _ = cl.Close() + return true + }, testutil.WaitShort, testutil.IntervalFast) + + payload := types.MessagePayload{ + Version: "1.0", + UserEmail: to, + Labels: make(map[string]string), + } + + dispatchFn, err := handler.Dispatcher(payload, subject, body, helpers()) + require.NoError(t, err) + + msgID := uuid.New() + retryable, err := dispatchFn(ctx, msgID) + + require.NoError(t, err) + require.False(t, retryable) + + msg := backend.LastMessage() + require.NotNil(t, msg) + + // Verify envelope address (MAIL FROM) contains only the bare email. + require.Equal(t, tc.expectedEnvFrom, msg.From, + "SMTP envelope MAIL FROM should contain only the bare email address") + + // Verify header From preserves the display name. + require.Contains(t, msg.Contents, fmt.Sprintf("From: %s\r\n", tc.expectedHeaderFrom), + "Email From header should preserve the display name if present") + + require.NoError(t, srv.Shutdown(ctx)) + wg.Wait() + }) + } +} diff --git a/docs/admin/monitoring/notifications/index.md b/docs/admin/monitoring/notifications/index.md index b1461cfec5..4abbe547aa 100644 --- a/docs/admin/monitoring/notifications/index.md +++ b/docs/admin/monitoring/notifications/index.md @@ -109,11 +109,11 @@ existing one. **Server Settings:** -| Required | CLI | Env | Type | Description | Default | -|:--------:|---------------------|-------------------------|----------|-----------------------------------------------------------|-----------| -| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use. | | -| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `string` | The SMTP relay to send messages (format: `hostname:port`) | | -| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost | +| Required | CLI | Env | Type | Description | Default | +|:--------:|---------------------|-------------------------|----------|-------------------------------------------------------------------|-----------| +| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use (e.g. `"Coder "`). | | +| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `string` | The SMTP relay to send messages (format: `hostname:port`) | | +| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost | **Authentication Settings:**