Trim surrounding single quotes in emails to support Outlook-style single-quotes around To: header weirdness

[CapnFelix]

Outlook does add single quotes to emails sometimes which is not valid

Q	A
Documentation	no
Bugfix	yes
BC Break	no
New Feature	no
RFC	no
QA	no

Description

We're seeing emails generated by outlook which do have surrounding single quote like this:
To: <'user@example.com'>
The fix does just trim those.

Issue on microsoft.com: https://answers.microsoft.com/en-us/outlook_com/forum/all/single-quote-marks-around-email-addresses-in/37122680-0ea2-46ed-bfeb-8f67d3441e5f

This is my first PR, so please let me know if did something wrong :)

[Ocramius]

Tests strictly required to validate such a change: IMO not valid to accept a string within quotes as a valid Address anyway, as such kind of cleanup should occur wherever you input data into the system that uses Address::fromString()

[CapnFelix]

Thanks for the quick feedback.
I agree that stripping it in ::fromString is not perfect, even though when looking at the caller stack, I'm not quite sure where to put this instead.

Message is loaded from IMAP in
Laminas\Mail\Storage\Imap->getMessage()
which is then calling \Laminas\Mail\Storage\Message
and then finally using \Laminas\Mail\Header\AbstractAddressList::fromString to generate the AddressList.
I think it would be possible to do the trimming here, but I would need to use the same reg ex to unpack the email address.
What do you think?

[Ocramius]

Aha, the fact that it is loaded via IMAP is new info.

I wonder if it is possible to get one of the loaded raw messages as part of data provider entries for our test suite? That would make it clear that we're talking about a real-world IMAP use-case, and prevent future regressions.

[CapnFelix]

I am sorry, I should have provided this info in the initial description.
I've added a regression test to verify the change + added an example mail to the files folder.
Does this make sense?

[Ocramius]

Tests make sense to me, although the type errors reported by psalm should really be checked (see them on the diff).

As for the trim() call, I'm thinking that it should be moved into Message, rather than in Address. Rationale: Address::fromString() is used outside the context of Message too, while it looks like this Outlook IMAP oddity is only applicable when parsing messages in Outlook.

[CapnFelix]

Maybe there is a small misunderstanding: We're looking at a message sent from an outlook client to a Gmail inbox. We're actually downloading this message via IMAP from Gmail in this case.
So I guess this is more a general problem not related to the storage since it is the email that is "wrong" no matter which protocol I'm using to download it.
But I can look into a way to do that fix in Message...

[CapnFelix]

Some thoughts looking at it a little more in depths:
what the Address::fromString really does is extracting name and email from an address header value string. So I think it would make sense to enhance the extracting to be able to deal with the single quotes either by adjusting the preg_match or by doing the trimming.
When looking at the Message class I don't really have a good idea how to fix it there.
Let me know what you think!
Thanks!

[Ocramius]

Reasoning seems fine: are you able to bring CI to green, so that somebody else can throw in a more informed review on whether putting this responsibility in Address is acceptable?

[Slamdunk]

Issue on microsoft.com: https://answers.microsoft.com/en-us/outlook_com/forum/all/single-quote-marks-around-email-addresses-in/37122680-0ea2-46ed-bfeb-8f67d3441e5f

I've read https://datatracker.ietf.org/doc/html/rfc5322 twice, and nowhere it allows <'foo@example.com'> as valid syntax 🤦

But as maintainer of ddeboer/imap it's no surprise to me

[Ocramius]

Ok, so let's get it green, then merged

[CapnFelix]

Awesome!
I am not familiar with psalm yet, but I think I got it to work :D

[Ocramius]

Awesome! 🚢

Thanks @CapnFelix! Releasing nao.

[CapnFelix]

Thank you so much! 🌊