Skip to content

[Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x #39685

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

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jan 2, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39656
License MIT
Doc PR N/A

@carsonbot carsonbot added this to the 4.4 milestone Jan 2, 2021
@carsonbot carsonbot changed the title Allow egulias/email-validator 3.x [Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x Jan 2, 2021
@derrabus
Copy link
Member Author

derrabus commented Jan 2, 2021

I need a little help with that test:

public function testIdRightCanBeDotAtom()
{
/* -- RFC 2822, 3.6.4.
id-right = dot-atom-text / no-fold-literal / obs-id-right
*/
$header = new IdentificationHeader('References', 'a@b.c+&%$.d');
$this->assertEquals('a@b.c+&%$.d', $header->getId());
$this->assertEquals('<a@b.c+&%$.d>', $header->getBodyAsString());
}

During the construction of IdentificationHeader the email validator is called. After upgrading the library to v3, all of the special characters used here are rejected by the validator: +&%$. I am not familiar with the spec, but my gut feeling is that neither of those characters should be part of a domain name.

@fabpot you are the author of that test: Do you recall what is tested here and have an idea how we can proceed with the changed behavior of egulias/email-validator?

composer.json Outdated
@@ -118,7 +118,7 @@
"predis/predis": "~1.1",
"psr/http-client": "^1.0",
"psr/simple-cache": "^1.0",
"egulias/email-validator": "~1.2,>=1.2.8|~2.0",
"egulias/email-validator": "^1.2.8|^2|^3",
Copy link
Member Author

Choose a reason for hiding this comment

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

Shall I change bump to ^2.1.10|^3 here to be consistent with the constraint in the components? I don't think that we're testing with v1 anywhere at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Let's drop 1.2, no need to support 3 major versions anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to drop support for it in a patch release?

Copy link
Member

Choose a reason for hiding this comment

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

We should drop it in 5.x only.

@fabpot
Copy link
Member

fabpot commented Jan 3, 2021

I need a little help with that test:

public function testIdRightCanBeDotAtom()
{
/* -- RFC 2822, 3.6.4.
id-right = dot-atom-text / no-fold-literal / obs-id-right
*/
$header = new IdentificationHeader('References', 'a@b.c+&%$.d');
$this->assertEquals('a@b.c+&%$.d', $header->getId());
$this->assertEquals('<a@b.c+&%$.d>', $header->getBodyAsString());
}

During the construction of IdentificationHeader the email validator is called. After upgrading the library to v3, all of the special characters used here are rejected by the validator: +&%$. I am not familiar with the spec, but my gut feeling is that neither of those characters should be part of a domain name.

@fabpot you are the author of that test: Do you recall what is tested here and have an idea how we can proceed with the changed behavior of egulias/email-validator?

But these characters are valid (see https://tools.ietf.org/html/rfc2822#section-3.2.4). If looks like a wrong change in email validator.

@derrabus
Copy link
Member Author

derrabus commented Jan 3, 2021

Indeed, when validating strictly according to the RFC, these characters should be allowed if I read the RFC correctly. Friendly ping to @egulias.

@egulias
Copy link
Contributor

egulias commented Jan 3, 2021

Hello there,
thanks for the ping, happy to help.
As you can read in the breacking changes for v3 is that domain will be enforced towards RFC1035 ("internet domains" RFC). The reasons for this change are that email RFCs are very broad in the requirement for the domain part (chich caused a series of "user" complications" and "suggest" to follow RFC1035 and the fact that in Real Life® SMTP follow RFC1035.

@fabpot
Copy link
Member

fabpot commented Jan 4, 2021

@egulias Thanks for the comment. The main "issue" here is that we are validating a message id/reference/reply to and for those, I'm not sure we should use RFC 1035. At least, that would be a BC break (which we can assume for good reasons). Is there a way to keep validating according to the original spec?

@egulias
Copy link
Contributor

egulias commented Jan 4, 2021

Hi @fabpot ,
what would solve the issue would be adding support for it, as suggested in egulias/EmailValidator#193 (comment) ; RFC2822 is deprecated in favour of RFC5322 with the described behaviour in the issue.

To solve this two options come to my mind:

a) is for Symfony to add a custom validator for it implementing the interface.

b) is to make it the next feature to be added. I can work on my own here but it will take time; some help would be very much appreciated to speed it up.

Happy to hear more possibilites.

@egulias
Copy link
Contributor

egulias commented Jan 17, 2021

Hi @fabpot @derrabus ,
To better address the validations of ID & mailbox (https://tools.ietf.org/html/rfc2822#section-3.4), can you point me to how this is used and were? I'm aware of Symfony's Email component and the validator, but it would be of great help to know where to look.
While mailbox has some patterns to distinguish from an email address, id has some challenges.
I'm pondering adding an specific method for validating them or having the capability of distinguishing from the pattern.
Which would better help you?

@egulias
Copy link
Contributor

egulias commented Jan 30, 2021

Ok. So did a research on how the validator is being used in Symfony & Swiftmailer.

My conclusion is as follows:
Symfony (and Swiftmailer for that matter) are using the EmailValidator for validating a Header field "message-id" (section 3.6.4) which has a different syntaxis for the right side after the @ sign than "addr-spec" (section 3.4.1) which is the commonly known as "email address".

This was possible in version <3 of the EmailValidator because of the interpretation done of the RFC.
Version 3 follows the suggestions of the RFC and user land feedback, by following RFC 1035 (see below for details), breaking that use case.

The solution for this, then, is to create a new validation for "message-id" and then addapt Address to it.
The change would be in Address#L54:

if (!self::$validator->isValid($this->address, new MessageIDValidation())) {

@fabpot @xabbuh & others, please let me know if this would work for you.

Details

The exception bubbles from Address in the Mime component, used from the IdentificationHeader when building from message IDs.

Now, here's the thing. message-id has a different syntaxis for id-right (not a "domain" as will see) than addr-spec has for it. In fact, despite previous use-case seeming to validate, it was actually allowing for non-compliant id-right elemtns, like [ or CFWS.

id-right

dot-atom-text / no-fold-literal / obs-id-right

To note that obs-id-right in 4.5.4 is defined as domain

domain syntaxis

domain = dot-atom / domain-literal / obs-domain

domain-literal = [CFWS] "[" *([FWS] dcontent) [FWS] "]" [CFWS]

dcontent = dtext / quoted-pair

dtext = NO-WS-CTL / ; Non white space controls
%d33-90 / ; The rest of the US-ASCII
%d94-126 ; characters not including "[",
; "]", or ""

And at the ends, specifies:

The domain portion identifies the point to which the mail is
delivered. In the dot-atom form, this is interpreted as an Internet
domain name (either a host name or a mail exchanger name) as
described in [STD3, STD13, STD14]. In the domain-literal form, the
domain is interpreted as the literal Internet address of the
particular host. In both cases, how addressing is used and how
messages are transported to a particular host is covered in the mail
transport document [RFC2821]. These mechanisms are outside of the
scope of this document.

STD13 is RFC1035

@egulias
Copy link
Contributor

egulias commented Feb 21, 2021

Hi there!
I've done some progress and I have a working solution. I should be able to release v3.1 in a couple of weeks.
It will require the change in the line mentioned in #39685 (comment)
The change would be in Address#L54:

if (!self::$validator->isValid($this->address, new MessageIDValidation())) {

@derrabus derrabus force-pushed the bugfix/email-validator-3 branch from f6f5404 to b37e598 Compare February 22, 2021 07:08
@derrabus
Copy link
Member Author

@egulias That's great news, thank you. Give me a ping as soon as we can test the new validator. I'll update this PR then.

@egulias
Copy link
Contributor

egulias commented Feb 28, 2021

PR ready @derrabus egulias/EmailValidator#290 , will give it a week and merge next one.

@derrabus derrabus force-pushed the bugfix/email-validator-3 branch from b37e598 to 3ec6a8b Compare February 28, 2021 22:45
@derrabus derrabus changed the title [Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x WIP: [Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x Feb 28, 2021
@derrabus
Copy link
Member Author

@egulias This PR now runs against your feature branch and the tests are green now. Apparently, your changes work for us. 🎉

@egulias
Copy link
Contributor

egulias commented Mar 7, 2021

@derrabus release done! You can continue now, let me know when it is done!
Thanks for your help!

@derrabus derrabus force-pushed the bugfix/email-validator-3 branch from 3ec6a8b to 3beeadb Compare March 7, 2021 15:15
@derrabus derrabus changed the title WIP: [Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x [Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x Mar 7, 2021
@derrabus
Copy link
Member Author

derrabus commented Mar 7, 2021

Thank you, the PR is ready now. 🚀

@fabpot
Copy link
Member

fabpot commented Mar 7, 2021

Thank you @egulias!

@fabpot
Copy link
Member

fabpot commented Mar 7, 2021

Thank you @derrabus.

@fabpot fabpot merged commit d8cfa3e into symfony:4.4 Mar 7, 2021
@derrabus derrabus deleted the bugfix/email-validator-3 branch March 7, 2021 16:01
@derrabus
Copy link
Member Author

derrabus commented Mar 7, 2021

Thanks! I've merged the changes up to 5.x and resolved all the composer.json conflicts.

@michaljusiega
Copy link
Contributor

Thank you @fabpot, @derrabus and @egulias ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants