Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mailer][Mime] Support unicode email addresses #58361

Merged
merged 7 commits into from
Oct 6, 2024
Merged

[Mailer][Mime] Support unicode email addresses #58361

merged 7 commits into from
Oct 6, 2024

Conversation

arnt
Copy link
Contributor

@arnt arnt commented Sep 23, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
License MIT

This allows applications to send mail to all-Chinese email addresses, or like my test address grå@grå.org. Code that uses Symfony needs no change and should experience no difference, although if the upstream MTA doesn't support it (most do by now) then an exception is thrown slightly later than before this change.

Before this commit, Envelope would throw InvalidArgumentException when a
unicode sender address was used. Now, that error is thrown slightly later,
is thrown for recipient addresses as well, but is not thrown if the
next-hop server supports SMTPUTF8.

As a side effect, transports that use JSON APIs to ESPs can also use
unicode addresses if the ESP supports that (many do, many don't).
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title Support unicode email addresses [Mailer][Mime] Support unicode email addresses Sep 23, 2024
Also fix one mysteriously broken unit test.
@arnt
Copy link
Contributor Author

arnt commented Sep 23, 2024

I pushed a new commit resolving all received comments. Thanks!

@stof
Copy link
Member

stof commented Sep 24, 2024

Please fix the coding standards to follow the Symfony coding standards (see the fabbot.io check)

src/Symfony/Component/Mailer/Envelope.php Outdated Show resolved Hide resolved
* The SMTPUTF8 extension is strictly required if any address
* contains a non-ASCII character in its localpart. If non-ASCII
* is only used in domains (e.g. horst@freiherr-von-mühlhausen.de)
* then it is possible to to send the message using IDN encoding
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* then it is possible to to send the message using IDN encoding
* then it is possible to send the message using IDN encoding

@@ -44,10 +44,6 @@ public static function create(RawMessage $message): self

public function setSender(Address $sender): void
{
// to ensure deliverability of bounce emails independent of UTF-8 capabilities of SMTP servers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would bounces still work fine if your SMTP server supports SMTPUTF8 but the target server does not ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you get a 5xx error when the supporting SMTP client sends to the unsupporting SMTP server.

The Symfony code is written to minimize the chance of running into this at all, though. If you send to e.g. info@grå.org, Symfony will be able to send that even to an unsupporting client. That's why the code tests for non-ASCII in the localpart (not the entire address). This is the same approach as e.g. Exchange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should elaborate. The 5xx error means that the server that generates the DNS is one that supports SMTPUTF8. It will generate a bounce that does not require SMTPUTF8 in order to be delivered, and which contains UTF8 in the body text.

It works quite well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Sender will become the recipient of the bounce. That's why I'm wondering how this would behave.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sender uses an ASCII address (highly advisable at this time), then the bounce does not require SMTPUTF8. Delivering the DSN is simple.

If the sender uses a non-ASCII address, then Symfony's upstream MTA will generate a DSN that uses SMTPUTF8, but in this case we know that the sender has support for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bounces work reliably. My job (at ICANN) is one where I get to hear about this kind of problem ;)

If the recipient's server receives the message at all, then it supports SMTPUTF8. An extended server never forwards an SMTPUTF8 message to an unextended server. (This is a nontrivial design decision, and was made only after a large testbed experiment.)

This means that if any server along the path needs (or chooses) to bounce the message, then it has SMTPUTF8 support.

I wonder whether it makes sense to enforce ASCII in the sender's localpart… let me sleep on that, please. AIUI Symfony is used mostly to send mail from servers to users? Like a web server's noreply@example.com? Is Symfony also used by scripts that people like us run on the command line or from cron? There's a five-digit number of domains that actively use unicode email addresses, maybe even six-digit.

Copy link
Member

@stof stof Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what people build using symfony/mailer (nothing forbids you to write a PHP script meant to be run on the command line, and Symfony does not call home to report us that you did write such a script). However, my intuition is that at least 99% of usages (and probably much more than that) is about sending from a server

If the recipient's server receives the message at all, then it supports SMTPUTF8. An extended server never forwards an SMTPUTF8 message to an unextended server. (This is a nontrivial design decision, and was made only after a large testbed experiment.)

that's actually a good context to have to understand how this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about forbidding UTF8 sender addresses, and…

I think the key here is that when people make the mistake of sending "here's the link to change password" or "your order has shipped" from a unicode address, they understand it really quickly. When you do that, maybe 20% of your outgoing mail bounces, and you change your configuration later on the same day.

It's not the kind of mistake that causes slow trouble over a long time, it's the kind of mistake that causes a lot of trouble immediately.

If, on the other hand, you write a script to process, sort and forward inbound mail, then you may not receive any from a unicode address very soon, and a limitaiton on sender address is one that shows up slowly, after a while, and seldom.

For this reason, I think the argument to forbid UTF8 sender addresses is fairly weak. But you can judge it better than I — my expertise is in unicode email and domain names, you know Symfony users and traditions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If, on the other hand, you write a script to process, sort and forward inbound mail, then you may not receive any from a unicode address very soon, and a limitaiton on sender address is one that shows up slowly, after a while, and seldom.

wouldn't those fail DMARC checks if you forward them using the original sender ?

I would vote for keeping the restriction on UTF8 sender, which will give immediate feedback to devs instead of waiting for them to get trouble with their delivery once reaching production (if they attempt to use an Unicode sender, it is likely that their own email servers will support them and so they won't get delivery issues in dev/staging)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen any of those scripts do external forwarding, which would subject the message to DMARC tests. People write code to sort mail to info@… into different classes, route some to autoresponders and some to various local addresses. If the body text mentions product x, the message is forwarded to address y, etc. Some companies make sure a Key Account Representative get a copy of mail to/from key customers.

Some mail servers can do that with rules, but if a company employs developers, it's often done in the language those developers use.

I'll add another commit that reinstates the restriction on the localpart, with a unit test or two that make sure of compatibility with all receivers. You can include or exclude that commit when you merge the PR (I do hope you'll merge). IMO both approaches are good, the choice of best is a matter of Symfony's philosophy.

This commit also adds a test that Symfony chooses IDN encoding when
possible (to be compatible with all email receivers), and adjusts a couple
of tests to match the name used in the main source code.
@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
@fabpot
Copy link
Member

fabpot commented Oct 6, 2024

Thank you @arnt.

@fabpot fabpot merged commit 4a9c2e8 into symfony:7.2 Oct 6, 2024
10 checks passed
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Mailer Mime ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants