Ensure origin headers of mails sent using the mailer.transport service comply to RFC5322

Created on 28 October 2023, over 1 year ago

Problem/Motivation

Mails sent using the new mailer.transport service may not comply with RFC 5322 section 3.6.2. The Sender header is especially tricky, since whether or not this should be present depends on the content of the From header.

Additionally the Sender header must not be confused with the envelope sender. The former is port of the message while the latter is used in the SMTP protocol in the MAIL FROM command (see the Protocol Overview) in the Wikipedia SMTP for a good introduction).

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Feature request
Status

Postponed

Version

11.0 🔥

Component
Mail 

Last updated about 7 hours ago

No maintainer
Created by

🇨🇭Switzerland znerol

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @znerol
  • 🇬🇧United Kingdom adamps

    Great, this seems like a very thorough and precise treatment of these two headers - although I didn't check the standards myself to confirm correctness.

    Message::getPreparedHeaders() has code that overlaps/conflicts with this issue (I'm looking at v6.4.17). It will set from based on sender so potentially we don't need setDefaultFrom(). And it will set sender based on from, which will undo the work of removeRedundantSender().

    I'm adjusted the terminology in the title/IS slightly. Emails will pass through the transport layer, but they aren't sent directly to it as there are other layers on top (including delivery and likely another Drupal-specific one that we create).

    I see this as a part of Create a new mailer service to wrap symfony mailer Active - it's one of the things that should be done as part of building every email. There is similar, but simpler code in MailManager. We could agree to create some Drupal-specific email events, in which case this should be adjusted to use them. So I feel that "postponed" is the correct status for a couple of weeks more.

    @znerol How does that all sound to you?

  • 🇨🇭Switzerland znerol

    Message::getPreparedHeaders() has code that overlaps/conflicts with this issue (I'm looking at v6.4.17). It will set from based on sender so potentially we don't need setDefaultFrom().

    Interesting idea. I tested that. The very first test fails at line 55 (last assertion in the following snipped).

        $originalEmail = $sentMessage->getOriginalMessage();
        assert($originalEmail instanceof Email);
        $actualFrom = $originalEmail->getFrom();
        $this->assertEquals([$expectedAddress], $actualFrom);
    

    The test expects that $originalEmail->getFrom() contains the site address. This isn't the case, because Message::getPreparedHeaders() does only derive, but not save the from header.

    However, imagine a message subscriber which logs messages which failed. This might want to log the from header as well.

    And it will set sender based on from, which will undo the work of removeRedundantSender().

    That is not true. removeRedundantSender() only removes sender if there are multiple from addresses. Message::getPreparedHeaders() only adds sender when there is none and when there are multiple from addresses.

    Regarding the process, nothing blocks this. Even if the implementation should change at some point, its good to have the tests in place. And I guess it will take some time and energy to review them as well.

Production build 0.71.5 2024