fix email injection issue

Author: Aditya-Mane1

internal/email/service.go

@@ -9,26 +9,42 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io"
+	"net/mail"
 	"net/smtp"
 	"regexp"
 	"strings"
 	"sync"
 	"time"
 )
 
-// headerInjectionPattern matches CRLF sequences that could be used for header injection
-var headerInjectionPattern = regexp.MustCompile(`[\r\n]`)
+// headerInjectionPattern matches control characters that could be used for header injection
+var headerInjectionPattern = regexp.MustCompile(`[\x00-\x1F\x7F]`)
 
-// bodyControlCharsPattern matches control characters (CR, LF, null) that enable content smuggling
-var bodyControlCharsPattern = regexp.MustCompile(`[\r\n\x00]`)
+// bodyControlCharsPattern matches null bytes and other unsafe control characters
+// while allowing tab, regular newlines, and carriage returns
+var bodyControlCharsPattern = regexp.MustCompile(`[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]`)
 
-// sanitizeHeader removes CRLF characters to prevent email header injection
+// sanitizeHeader removes control characters to prevent email header injection
 func sanitizeHeader(input string) string {
 	return headerInjectionPattern.ReplaceAllString(input, "")
 }
 
-// sanitizeBody removes control characters (CR/LF/null) that could enable header/content smuggling
-// while preserving normal message content
+// sanitizeEmailAddress uses net/mail to ensure the address is safe and correctly formatted
+func sanitizeEmailAddress(input string) string {
+	addr, err := mail.ParseAddress(input)
+	if err != nil {
+		// If it's not a valid RFC 5322 address, fallback to a strict header sanitization
+		// but ideally this should have been caught by IsValidEmail
+		return sanitizeHeader(input)
+	}
+	if addr.Name == "" {
+		return addr.Address
+	}
+	return addr.String()
+}
+
+// sanitizeBody removes null bytes and other control characters that could enable smuggling
+// while preserving normal message content including newlines
 func sanitizeBody(input string) string {
 	return bodyControlCharsPattern.ReplaceAllString(input, "")
 }
@@ -88,10 +104,10 @@ func (es *EmailService) SendEmail(to []string, subject, body string, isHTML bool
 	sanitizedBody := sanitizeBody(body)
 	sanitizedTo := make([]string, len(to))
 	for i, addr := range to {
-		sanitizedTo[i] = sanitizeHeader(addr)
+		sanitizedTo[i] = sanitizeEmailAddress(addr)
 	}
 	// Sanitize FromEmail before SMTP envelope usage
-	sanitizedFrom := sanitizeHeader(es.FromEmail)
+	sanitizedFrom := sanitizeEmailAddress(es.FromEmail)
 	if sanitizedFrom == "" {
 		return fmt.Errorf("invalid or empty sender email after sanitization")
 	}

tests/security_test.go

@@ -114,9 +114,14 @@ func TestEmailInjection_HeaderInjectionPrevention(t *testing.T) {
 	if err != nil {
 		t.Fatalf("SendEmail failed: %v", err)
 	}
+	// After sanitization, net/mail will typically remove the invalid parts or quote them
+	// In our case, it should at least remove the CRLF
 	if strings.Contains(capturedMessage, "\r") || strings.Contains(capturedMessage, "\n") {
 		t.Errorf("CRLF characters not sanitized from recipient: %s", capturedMessage)
 	}
+	// Note: net/mail.ParseAddress might return "<victim@example.comBcc:attacker@evil.com>"
+	// because it strips control characters via sanitizeHeader fallback if parsing fails,
+	// or it might just error. Our implementation currently falls back to sanitizeHeader if ParseAddress fails.
 }
 
 // TestEmailInjection_BodyControlCharactersPrevention tests that control characters in body are removed
@@ -136,9 +141,9 @@ func TestEmailInjection_BodyControlCharactersPrevention(t *testing.T) {
 		t.Fatalf("SendEmail failed: %v", err)
 	}
 
-	// Body should have control characters removed
-	if strings.Contains(capturedBody, "\r") || strings.Contains(capturedBody, "\n") {
-		t.Errorf("CRLF characters not sanitized from body: %q", capturedBody)
+	// Body should have null bytes removed, but CRLF should REMAIN
+	if !strings.Contains(capturedBody, "\r\n") {
+		t.Errorf("CRLF characters SHOULD remain in body for multi-line support: %q", capturedBody)
 	}
 	if strings.Contains(capturedBody, "\x00") {
 		t.Errorf("Null bytes not sanitized from body: %q", capturedBody)
@@ -255,8 +260,11 @@ func TestEmailInjection_TransactionalEmailSanitization(t *testing.T) {
 	if strings.Contains(capturedSubject, "\r") || strings.Contains(capturedSubject, "\n") {
 		t.Errorf("Subject CRLF not sanitized: %q", capturedSubject)
 	}
-	if strings.Contains(capturedBody, "\r") || strings.Contains(capturedBody, "\n") || strings.Contains(capturedBody, "\x00") {
-		t.Errorf("Body control characters not sanitized: %q", capturedBody)
+	if !strings.Contains(capturedBody, "\r\n") {
+		t.Errorf("Body CRLF SHOULD remain for multi-line support: %q", capturedBody)
+	}
+	if strings.Contains(capturedBody, "\x00") {
+		t.Errorf("Body null bytes not sanitized: %q", capturedBody)
 	}
 }