FIX: Unauthenticated email header injection via the Send Email action…

[yurkodmtr]

… https://github.com/Crocoblock/issues-tracker/issues/20362

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

• This PR addresses a real security issue (CRLF/email header injection) in the Send Email action by adding a header validation step before assembling the headers.
• The approach is generally correct: checking for "\r"/"\n" sequences and throwing an Action_Exception prevents injection.

What I reviewed

• File: modules/actions-v2/send-email/send-email-action.php
• New/changed methods: get_default_headers(), validate_header_values()

What I like

• Good to see an explicit validation step added before building headers.
• The change targets the relevant header sources: From name/address, Reply-To, Content-Type, Cc, Bcc.

Issues / Improvements / Risks

1. Robustness of array_merge inputs

   • The code calls array_merge(array(...), $cc_emails, $bcc_emails). If get_cc() or get_bcc() ever return null or a string (rather than an array), array_merge will raise warnings or behave unexpectedly. Cast CC/BCC to array first: (array) $this->get_cc(), (array) $this->get_bcc(). This will prevent warnings and make the validation reliable.

2. validate_header_values should be defensive about types

   • The new validate_header_values iterates $values and calls strpos on each value. If any element is not a string (e.g. an array, object, null) strpos will emit warnings. Add an is_string() guard and skip or cast non-strings (prefer skipping and logging) so no warnings are triggered.

3. Validate and sanitize email addresses and names separately

   • Currently the validation only checks for CR/LF. That's good, but stronger validation for email-specific fields is recommended:
     • Use sanitize_email() and is_email() for From/Reply-To/Cc/Bcc addresses.
     • Use sanitize_text_field() for display name (From name). This both prevents CRLF and removes unwanted characters.
     • Content-Type should be validated against a small whitelist (e.g. 'text/plain', 'text/html') rather than only checking for newlines.
   • Rationale: rejecting only CR/LF is preventive, but you still want to ensure only valid emails and allowed content types are used.

4. Error handling & backwards compatibility

   • The method throws an Action_Exception('invalid_email') when an invalid header value is detected. That is appropriate for security, but may cause previously-working forms (if misconfigured) to fail. Recommend making the exception message more descriptive (e.g. include which header type triggered it) or log the raw problematic value in debug only. Ensure the thrown exception is handled where get_default_headers() is called and results in a safe failure for users.

5. Coding style / minor issues

   • Method signature/formatting: private function validate_header_values(array $values) { should follow WPCS spacing.
   • The method does not declare a return type (void) — consider : void.
   • Docblock: fine to keep @throws, but consider specifying the exception message or code.

6. Tests

   • There are no tests included. This is a security-sensitive change; please add unit tests validating:
     • CRLF injection attempts in From name/address, reply-to, content-type, cc, bcc are rejected.
     • Valid headers (including multiple CC/BCC) are allowed.
     • Non-array CC/BCC values are handled safely (cast/ignored or produce the same exception behavior).

7. Suggest small API hardening in get_default_headers()

   • Cast CC/BCC results to arrays: $cc_emails = (array) $this->get_cc(); $bcc_emails = (array) $this->get_bcc();
   • Sanitize before calling validate_header_values: apply sanitize_email()/sanitize_text_field() where appropriate then still validate for CR/LF.

Recommended code-level changes (example guidance)

• In get_default_headers():
  • $cc_emails = (array) $this->get_cc(); $bcc_emails = (array) $this->get_bcc();
  • $from_name = sanitize_text_field( $this->get_from_name() );
  • $from_address = sanitize_email( $this->get_from_address() ); if ( ! is_email( $from_address ) ) throw new Action_Exception( 'invalid_email' );
  • $reply_to = sanitize_email( $this->get_reply_to() ); etc.
• In validate_header_values(array $values):
  • foreach ($values as $value) {
    if ( ! is_string( $value ) || $value === '' ) { continue; }
    if ( false !== strpos( $value, "\r" ) || false !== strpos( $value, "\n" ) ) {
    throw new Action_Exception( 'invalid_email' );
    }
    }

Summary / Recommendation

• Merge after addressing the robustness items above (cast cc/bcc to array, guard against non-strings in validate_header_values, and prefer sanitizing/validating emails and content-type more strictly).
• Add unit tests that cover CRLF injection vectors and valid cases.
• Consider slightly clearer exception messages or safe logging for easier debugging.

Files touched

• modules/actions-v2/send-email/send-email-action.php (get_default_headers(), new validate_header_values())

Suggested changelog entry

- FIX: Prevent CRLF email header injection in the Send Email action by validating header values (From, Reply-To, Content-Type, Cc, Bcc).