Potential fix for code scanning alert no. 2: Email content injection

[ziembor]

Potential fix for https://github.com/ziembor/gomailtesttool/security/code-scanning/2

General fix: sanitize untrusted email body content before it is inserted into the RFC 5322 message, using a conservative normalization that removes dangerous control characters while preserving expected text/newlines.

Best minimal fix without changing architecture: in internal/protocols/smtp/sendmail.go, sanitize body inside buildEmailMessage before concatenating it into the message. Keep current header sanitization intact. Add a helper that:

• normalizes \r\n and lone \r to \n,
• removes NUL and other non-printable control chars except \n and \t,
• returns cleaned text used in message assembly.

This addresses both alert variants because both taint paths converge to the same sink (w.Write(data)) through buildEmailMessage.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

[claude]

Code Review

Overview

This PR adds a sanitizeEmailBody helper and calls it inside buildEmailMessage to strip unsafe control characters from the email body before message assembly. This addresses the code scanning alert about email content injection (tainted data reaching the w.Write sink via buildEmailMessage).

The approach is sound in intent, but there are several issues to address before merging.

---

Issues

1. Existing test will break (high severity)

The test case "Body preserves newlines (NOT sanitized)" in sendmail_test.go:136 passes body: "Line 1\nLine 2\r\nLine 3" with bodyPreserve: true, asserting strings.Contains(messageStr, tt.body) is true. After sanitizeEmailBody, the \r\n sequence is converted to \n, so the message will contain "Line 1\nLine 2\nLine 3" — the original \r\n is gone. The bodyPreserve check will fail.

The test name also needs updating: the comment "NOT sanitized" is now incorrect, and the expected body value should reflect the normalized output.

2. RFC 5322 line-ending compliance concern (medium severity)

sanitizeEmailBody normalizes all line endings to bare \n, but RFC 5322 §2.3 requires \r\n in email message bodies. buildEmailMessage already uses \r\n consistently in the headers. After this change, body line endings become bare LFs — a protocol violation.

Suggested fix: after stripping control characters, convert \n back to \r\n:

func sanitizeEmailBody(body string) string {
    body = strings.ReplaceAll(body, "\r\n", "\n")
    body = strings.ReplaceAll(body, "\r", "\n")
    var b strings.Builder
    b.Grow(len(body))
    for _, r := range body {
        if r == '\n' || r == '\t' || r >= 0x20 {
            b.WriteRune(r)
        }
    }
    // Re-normalize to CRLF for RFC 5322 compliance
    return strings.ReplaceAll(b.String(), "\n", "\r\n")
}

3. No unit tests for the new sanitizeEmailBody function (medium severity)

sanitizeEmailHeader has a dedicated TestSanitizeEmailHeader table-driven test. sanitizeEmailBody has no equivalent. Being security-sensitive, it should cover: normal text (no-op), CRLF normalization, lone CR normalization, NUL byte stripped, control chars 0x01–0x1F (except tab) stripped, tab preserved, Unicode printable preserved, and injection attempts (fake headers injected after a double CRLF in the body).

---

Minor Notes

• Unicode C1 controls (U+0080–U+009F): These satisfy r >= 0x20 and pass through the filter. Rarely seen in practice, but a minor gap if strict sanitization is ever needed.
• Comment accuracy: The updated doc comment on buildEmailMessage now correctly describes body sanitization — good change.

---

Summary

Issue	Severity	Action Required
Existing bodyPreserve test will fail	High	Update test expectation and name
Body bare-LF violates RFC 5322	Medium	Convert \n to \r\n after stripping
No direct tests for sanitizeEmailBody	Medium	Add TestSanitizeEmailBody
C1 Unicode controls pass through	Low	Acceptable for now

The core logic is correct and the function is clean. With the test fix, RFC 5322 line-ending normalization, and a dedicated unit test, this would be in good shape to merge.