Problem/Motivation

Mails sent using the new symfony-based mailer 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 part 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

Add an event subscriber for the MessageEvent which ensures that:

  1. A From header is present. Defaults to the system.site.mail with system.site.name as the display name.
  2. A Sender header is present if-and-only-if necessary.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3397418

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

znerol created an issue. See original summary.

znerol’s picture

znerol commented
Parent issue: » #1803948: [META] Adopt the symfony mailer component
Related issues: +#3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer)

Postponed on #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer)

znerol’s picture

znerol commented
Title: Ensure origin headers of mails sent using the mailer.transport service comply to RFC5322 » [PP-1] Ensure origin headers of mails sent using the mailer.transport service comply to RFC5322
adamps’s picture

adamps commented
Title: [PP-1] Ensure origin headers of mails sent using the mailer.transport service comply to RFC5322 » [PP-1] Ensure origin headers of mails sent using the symfony-based mailer service comply to RFC5322
Issue summary: View changes

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 #3530860: Create a new mailer service to wrap symfony mailer - 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?

znerol’s picture

znerol commented
Status: Postponed » Needs review

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.

znerol’s picture

znerol commented
Title: [PP-1] Ensure origin headers of mails sent using the symfony-based mailer service comply to RFC5322 » Ensure origin headers of mails sent using the symfony-based mailer service comply to RFC5322
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Didn't see any MR to review?

znerol opened merge request !13080

znerol’s picture

znerol commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Can we clean up the summary some please. Proposed solution is blank.

znerol’s picture

znerol commented
Status: Needs review » Needs work
znerol’s picture

znerol commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record

This looks really good. Can we get a CR for the new subscriber for others to use

znerol’s picture

znerol commented
Assigned: Unassigned » znerol

Note to self: add a section to the existing mailer module CR.

znerol’s picture

znerol commented
Assigned: znerol » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs change record

Updated the existing CR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

CR entry LGTM.

imclean’s picture

imclean commented

In the CR example "For authenticated SMTP", what is the default port? Should this be mentioned somewhere?

znerol’s picture

znerol commented

In the CR example "For authenticated SMTP", what is the default port? Should this be mentioned somewhere?

Interesting question. There is not one default port but two (25 for SMTP and 465 for SMTPS). This is analogous to HTTP (port 80) and HTTPS (port 443). My recommendation would be to just leave out the port unless its a custom one (e.g., most useful example is mailpit). Although, some mail providers require port 587 to send out authenticated mail and only allow port 25 for incoming messages only.

That said, I do not think that the CR should go into these kind details. Imagine we'd be replacing guzzle with some other HTTP client library. Would you expect the CR to discuss port numbers?

imclean’s picture

imclean commented 
        if (null === $tls) {
            if (465 === $port) {
                $tls = true;
            } else {
                $tls = \defined('OPENSSL_VERSION_NUMBER') && 0 === $port && 'localhost' !== $host;
            }
        }

In the first instance this sets TLS to true if the port is set to 465 (or port is 0 and openssl is present);

        if (0 === $port) {
            $port = $tls ? 465 : 25;
        }

It then sets the port to 465 if TLS is true and port is 0. I guess they cover different situations, where a port of 0 autoselects the port based on whether TLS is set or not.

Can TLS be specified in the transport configuration?

znerol’s picture

znerol commented

There is a config schema for mailer_dsn. The use of TLS depends on the options. The options correspond directly with the ones provided by the Symfony Mailer component.

I expect that we will add a proper admin UI for transport configuration before the new mailer in Drupal gets stable.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7b7712f0891 to main and bfc8c953690 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed bfc8c953 on 11.x 
    feat: #3397418 Ensure origin headers of mails sent using the symfony-...

  • longwave committed 7b7712f0 on main 
    feat: #3397418 Ensure origin headers of mails sent using the symfony-...

longwave closed merge request !13080

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented

Is there an issue for non-symfony mails that have an incorrect `Sender` header currently in Drupal 11.3.5?

znerol’s picture

znerol commented

Maybe this one: #3055296: Setting the "Return-Path" header doesn't follow RFC 5321