fix(smtp): prevent header injection and honor use_tls

- Validate all recipients with net/mail.ParseAddress; reject CR/LF.
- RFC 2047 (Q-encoding) for Subject and FromName so CRLF and non-ASCII
  cannot break out of headers.
- Honor use_tls: implicit TLS on port 465 with certificate verification;
  otherwise document the opportunistic-STARTTLS path.
- Table-driven tests for validation, injection neutralization, and
  multipart building.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

internal/notifier/smtp.go

@@ -3,9 +3,12 @@ package notifier
 import (
 	"context"
 	"crypto/rand"
+	"crypto/tls"
 	"encoding/hex"
 	"fmt"
 	"html"
+	"mime"
+	"net/mail"
 	"net/smtp"
 	"regexp"
 	"strings"
@@ -75,15 +78,16 @@ func (s *SMTPNotifier) Send(ctx context.Context, notification *domain.Notificati
 	allRecipients = append(allRecipients, notification.CC...)
 	allRecipients = append(allRecipients, notification.BCC...)
 
-	// Validate email recipients
+	// Validate email recipients: reject header-injection attempts (CR/LF) outright and
+	// otherwise require a syntactically valid RFC 5322 address.
 	for _, recipient := range allRecipients {
-		if !strings.Contains(recipient, "@") {
+		if err := validateRecipient(recipient); err != nil {
 			return &domain.NotificationResult{
 				NotificationID: notification.ID,
 				Success:        false,
-				Error:          fmt.Sprintf("invalid email address: %s", recipient),
+				Error:          err.Error(),
 				SentAt:         time.Now(),
-			}, fmt.Errorf("invalid email address: %s", recipient)
+			}, fmt.Errorf("invalid recipient: %w", err)
 		}
 	}
 
@@ -95,7 +99,7 @@ func (s *SMTPNotifier) Send(ctx context.Context, notification *domain.Notificati
 	auth := smtp.PlainAuth("", s.config.Username, s.config.Password, s.config.Host)
 
 	// smtp.SendMail needs all recipients (To, CC, BCC) for actual delivery
-	err := smtp.SendMail(addr, auth, s.config.From, allRecipients, []byte(message))
+	err := s.sendMail(addr, auth, s.config.From, allRecipients, []byte(message))
 	if err != nil {
 		return &domain.NotificationResult{
 			NotificationID: notification.ID,
@@ -118,14 +122,106 @@ func (s *SMTPNotifier) Send(ctx context.Context, notification *domain.Notificati
 	}, nil
 }
 
+// sendMail dispatches the message using the transport appropriate for the configured port.
+// When UseTLS is enabled and the port is the implicit-TLS SMTPS port (465), the connection is
+// wrapped in TLS from the very first byte. Otherwise smtp.SendMail is used, which opportunistically
+// upgrades the plaintext connection to STARTTLS if the server advertises support for it, but
+// will silently fall back to a plaintext session if it does not.
+func (s *SMTPNotifier) sendMail(addr string, auth smtp.Auth, from string, recipients []string, msg []byte) error {
+	if s.config.UseTLS && s.config.Port == 465 {
+		return sendMailImplicitTLS(addr, s.config.Host, auth, from, recipients, msg)
+	}
+
+	return smtp.SendMail(addr, auth, from, recipients, msg)
+}
+
+// sendMailImplicitTLS sends an email over an implicit TLS (SMTPS) connection, verifying the
+// server certificate against serverName. The certificate is always verified (no
+// InsecureSkipVerify escape hatch is provided).
+func sendMailImplicitTLS(addr, serverName string, auth smtp.Auth, from string, recipients []string, msg []byte) error {
+	tlsConfig := &tls.Config{
+		ServerName: serverName,
+		MinVersion: tls.VersionTLS12,
+	}
+
+	conn, err := tls.Dial("tcp", addr, tlsConfig)
+	if err != nil {
+		return fmt.Errorf("failed to establish TLS connection to %s: %w", addr, err)
+	}
+	defer conn.Close()
+
+	client, err := smtp.NewClient(conn, serverName)
+	if err != nil {
+		return fmt.Errorf("failed to create SMTP client: %w", err)
+	}
+	defer client.Close()
+
+	if auth != nil {
+		if ok, _ := client.Extension("AUTH"); ok {
+			if err := client.Auth(auth); err != nil {
+				return fmt.Errorf("SMTP authentication failed: %w", err)
+			}
+		}
+	}
+
+	if err := client.Mail(from); err != nil {
+		return fmt.Errorf("failed to set sender %q: %w", from, err)
+	}
+
+	for _, recipient := range recipients {
+		if err := client.Rcpt(recipient); err != nil {
+			return fmt.Errorf("failed to add recipient %q: %w", recipient, err)
+		}
+	}
+
+	writer, err := client.Data()
+	if err != nil {
+		return fmt.Errorf("failed to open message data stream: %w", err)
+	}
+
+	if _, err := writer.Write(msg); err != nil {
+		return fmt.Errorf("failed to write message body: %w", err)
+	}
+
+	if err := writer.Close(); err != nil {
+		return fmt.Errorf("failed to finalize message: %w", err)
+	}
+
+	return client.Quit()
+}
+
+// validateRecipient ensures a recipient address cannot be used to inject additional SMTP
+// headers and is a syntactically valid RFC 5322 address. CR/LF are rejected outright (rather
+// than relying on mail.ParseAddress to catch them) so the failure reason is unambiguous.
+func validateRecipient(recipient string) error {
+	if strings.ContainsAny(recipient, "\r\n") {
+		return fmt.Errorf("recipient %q contains illegal CR/LF characters", recipient)
+	}
+
+	if _, err := mail.ParseAddress(recipient); err != nil {
+		return fmt.Errorf("recipient %q is not a valid email address: %w", recipient, err)
+	}
+
+	return nil
+}
+
+// encodeHeaderValue produces a header-safe representation of an untrusted value such as a
+// Subject or display name. mime.QEncoding.Encode leaves plain ASCII untouched but RFC
+// 2047-encodes anything containing control characters (including bare CR/LF) or non-ASCII
+// runes, so injected header/line breaks cannot survive into the raw message.
+func encodeHeaderValue(value string) string {
+	return mime.QEncoding.Encode("utf-8", value)
+}
+
 // buildMessage constructs the email message with headers
 func (s *SMTPNotifier) buildMessage(notification *domain.Notification) string {
 	var builder strings.Builder
 
-	// Format From header with optional display name
+	// Format From header with optional display name. The display name is untrusted
+	// configuration input, so it's run through the same header-encoding as Subject.
 	fromHeader := s.config.From
 	if s.config.FromName != "" {
-		fromHeader = fmt.Sprintf("%s <%s>", s.config.FromName, s.config.From)
+		fromHeader = fmt.Sprintf("%s <%s>", encodeHeaderValue(s.config.FromName), s.config.From)
 	}
 
 	builder.WriteString(fmt.Sprintf("From: %s\r\n", fromHeader))
@@ -142,7 +238,9 @@ func (s *SMTPNotifier) buildMessage(notification *domain.Notification) string {
 
 	// Note: BCC is intentionally NOT included in headers (that's the point of BCC!)
 
-	builder.WriteString(fmt.Sprintf("Subject: %s\r\n", notification.Subject))
+	// Subject is fully attacker-controlled, so it is always run through RFC 2047 encoding.
+	// This neutralizes embedded CR/LF (and non-ASCII) instead of interpolating it raw.
+	builder.WriteString(fmt.Sprintf("Subject: %s\r\n", encodeHeaderValue(notification.Subject)))
 	builder.WriteString("MIME-Version: 1.0\r\n")
 
 	switch {

internal/notifier/smtp_test.go

@@ -0,0 +1,292 @@
+package notifier
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/igodwin/notifier/internal/domain"
+)
+
+// TestValidateRecipient covers the recipient validation helper used by Send() to reject
+// header-injection attempts and syntactically invalid addresses before a message is built
+// or a network connection is opened.
+func TestValidateRecipient(t *testing.T) {
+	tests := []struct {
+		name      string
+		recipient string
+		wantErr   bool
+	}{
+		{
+			name:      "valid simple address",
+			recipient: "user@example.com",
+			wantErr:   false,
+		},
+		{
+			name:      "valid address with display name",
+			recipient: "Jane Doe <jane@example.com>",
+			wantErr:   false,
+		},
+		{
+			name:      "missing at sign",
+			recipient: "not-an-email",
+			wantErr:   true,
+		},
+		{
+			name:      "empty string",
+			recipient: "",
+			wantErr:   true,
+		},
+		{
+			name:      "CRLF header injection attempt",
+			recipient: "user@example.com\r\nBcc: attacker@evil.com",
+			wantErr:   true,
+		},
+		{
+			name:      "bare LF header injection attempt",
+			recipient: "user@example.com\nX-Injected: true",
+			wantErr:   true,
+		},
+		{
+			name:      "bare CR header injection attempt",
+			recipient: "user@example.com\rX-Injected: true",
+			wantErr:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateRecipient(tt.recipient)
+			if tt.wantErr && err == nil {
+				t.Fatalf("validateRecipient(%q) = nil, want error", tt.recipient)
+			}
+			if !tt.wantErr && err != nil {
+				t.Fatalf("validateRecipient(%q) = %v, want nil", tt.recipient, err)
+			}
+		})
+	}
+}
+
+// TestSendRejectsInvalidRecipientsWithoutNetworkAccess verifies that Send() rejects invalid
+// or CRLF-laden recipients during validation, before ever attempting to dial the SMTP server.
+// The configured host is deliberately non-routable so the test would hang or fail on a real
+// dial attempt if validation didn't short-circuit first.
+func TestSendRejectsInvalidRecipientsWithoutNetworkAccess(t *testing.T) {
+	tests := []struct {
+		name       string
+		recipients []string
+		cc         []string
+		bcc        []string
+	}{
+		{
+			name:       "invalid To address",
+			recipients: []string{"not-an-email"},
+		},
+		{
+			name:       "CRLF injection in To address",
+			recipients: []string{"user@example.com\r\nBcc: attacker@evil.com"},
+		},
+		{
+			name:       "CRLF injection in CC address",
+			recipients: []string{"user@example.com"},
+			cc:         []string{"cc@example.com\r\nX-Injected: true"},
+		},
+		{
+			name:       "CRLF injection in BCC address",
+			recipients: []string{"user@example.com"},
+			bcc:        []string{"bcc@example.com\r\nX-Injected: true"},
+		},
+	}
+
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host: "invalid.invalid", // non-routable placeholder; must never be dialed
+		Port: 587,
+		From: "sender@example.com",
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			notification := &domain.Notification{
+				ID:         "test-id",
+				Type:       domain.TypeEmail,
+				Subject:    "Test Subject",
+				Body:       "Test Body",
+				Recipients: tt.recipients,
+				CC:         tt.cc,
+				BCC:        tt.bcc,
+			}
+
+			result, err := notifier.Send(t.Context(), notification)
+			if err == nil {
+				t.Fatalf("Send() error = nil, want validation error")
+			}
+			if result == nil || result.Success {
+				t.Fatalf("Send() result = %+v, want Success=false", result)
+			}
+		})
+	}
+}
+
+// TestBuildMessageNeutralizesSubjectCRLF ensures a CRLF-laden Subject cannot smuggle a new
+// header into the raw message: the injected header line must not appear verbatim.
+func TestBuildMessageNeutralizesSubjectCRLF(t *testing.T) {
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host: "smtp.example.com",
+		Port: 587,
+		From: "sender@example.com",
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	notification := &domain.Notification{
+		ID:         "test-id",
+		Type:       domain.TypeEmail,
+		Subject:    "Hello\r\nX-Injected: evil",
+		Body:       "Test Body",
+		Recipients: []string{"user@example.com"},
+	}
+
+	message := notifier.buildMessage(notification)
+
+	if strings.Contains(message, "\r\nX-Injected:") {
+		t.Fatalf("built message contains injected header line:\n%s", message)
+	}
+	if strings.Contains(message, "X-Injected: evil") {
+		t.Fatalf("built message contains raw injected header value:\n%s", message)
+	}
+
+	// The Subject header line must still be present, just RFC 2047 encoded.
+	if !strings.Contains(message, "Subject: =?utf-8?q?") {
+		t.Fatalf("expected RFC 2047 encoded Subject header, got message:\n%s", message)
+	}
+}
+
+// TestBuildMessageNeutralizesFromNameCRLF ensures a CRLF-laden FromName config value cannot
+// inject an extra header into the From line.
+func TestBuildMessageNeutralizesFromNameCRLF(t *testing.T) {
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host:     "smtp.example.com",
+		Port:     587,
+		From:     "sender@example.com",
+		FromName: "Evil\r\nX-Injected: true",
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	notification := &domain.Notification{
+		ID:         "test-id",
+		Type:       domain.TypeEmail,
+		Subject:    "Hello",
+		Body:       "Test Body",
+		Recipients: []string{"user@example.com"},
+	}
+
+	message := notifier.buildMessage(notification)
+
+	if strings.Contains(message, "\r\nX-Injected:") {
+		t.Fatalf("built message contains injected header line from FromName:\n%s", message)
+	}
+	if strings.Contains(message, "X-Injected: true") {
+		t.Fatalf("built message contains raw injected FromName value:\n%s", message)
+	}
+
+	if !strings.Contains(message, "From: =?utf-8?q?") {
+		t.Fatalf("expected RFC 2047 encoded From display name, got message:\n%s", message)
+	}
+}
+
+// TestBuildMessagePlainSubjectUnchanged verifies that a benign ASCII subject is left
+// unencoded (mime.QEncoding.Encode is a no-op for plain ASCII), preserving existing behavior.
+func TestBuildMessagePlainSubjectUnchanged(t *testing.T) {
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host: "smtp.example.com",
+		Port: 587,
+		From: "sender@example.com",
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	notification := &domain.Notification{
+		ID:         "test-id",
+		Type:       domain.TypeEmail,
+		Subject:    "Plain Subject Line",
+		Body:       "Test Body",
+		Recipients: []string{"user@example.com"},
+	}
+
+	message := notifier.buildMessage(notification)
+
+	if !strings.Contains(message, "Subject: Plain Subject Line\r\n") {
+		t.Fatalf("expected plain ASCII subject to be left unencoded, got message:\n%s", message)
+	}
+}
+
+// TestBuildMessageMultipartWithHTMLBody verifies that supplying HTMLBody still produces a
+// correct multipart/alternative message with both text/plain and text/html parts.
+func TestBuildMessageMultipartWithHTMLBody(t *testing.T) {
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host: "smtp.example.com",
+		Port: 587,
+		From: "sender@example.com",
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	notification := &domain.Notification{
+		ID:         "test-id",
+		Type:       domain.TypeEmail,
+		Subject:    "Multipart Test",
+		Body:       "Plain text body",
+		HTMLBody:   "<p>HTML body</p>",
+		Recipients: []string{"user@example.com"},
+	}
+
+	message := notifier.buildMessage(notification)
+
+	if !strings.Contains(message, "Content-Type: multipart/alternative; boundary=") {
+		t.Fatalf("expected multipart/alternative content type, got message:\n%s", message)
+	}
+	if !strings.Contains(message, "Content-Type: text/plain; charset=UTF-8") {
+		t.Fatalf("expected text/plain part, got message:\n%s", message)
+	}
+	if !strings.Contains(message, "Content-Type: text/html; charset=UTF-8") {
+		t.Fatalf("expected text/html part, got message:\n%s", message)
+	}
+	if !strings.Contains(message, "Plain text body") {
+		t.Fatalf("expected plain text body verbatim, got message:\n%s", message)
+	}
+	if !strings.Contains(message, "<p>HTML body</p>") {
+		t.Fatalf("expected HTML body verbatim, got message:\n%s", message)
+	}
+
+	// Ensure the message ends with a proper closing boundary.
+	if !strings.Contains(message, "--\r\n") {
+		t.Fatalf("expected closing MIME boundary, got message:\n%s", message)
+	}
+}
+
+// TestNewSMTPNotifierDefaultsAndUseTLS is a sanity check that UseTLS is stored on the config
+// and that port defaulting still works as before, since Send() now branches on both.
+func TestNewSMTPNotifierDefaultsAndUseTLS(t *testing.T) {
+	notifier, err := NewSMTPNotifier(&SMTPConfig{
+		Host:   "smtp.example.com",
+		From:   "sender@example.com",
+		UseTLS: true,
+	})
+	if err != nil {
+		t.Fatalf("NewSMTPNotifier() error = %v", err)
+	}
+
+	if notifier.config.Port != 587 {
+		t.Fatalf("expected default port 587, got %d", notifier.config.Port)
+	}
+	if !notifier.config.UseTLS {
+		t.Fatalf("expected UseTLS to be preserved as true")
+	}
+}