Related to change from Email::Valid to Email::Address
Tomas added an issue on CPAN: https://github.com/Perl-Email-Project/Email-Address/issues/24
Created attachment 187917 [details] [review] Bug 41025: Check for a dot in domain name, allow localhost If Email::Address has been patched, this can be removed. Test plan: Run t/Koha/Email.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hmm, technically I think that Tomas might be wrong to raise a bug for the email library. A valid email address is just a localpart@domain where domain can be a domain name or an IP address. As noted in this patch, the domain could be localhost. It could be a computer name, it could be an internal domain of some other type. I think there's a difference between a valid email address and an existing email address. That said, it could be worthwhile adding some extra validation at the Koha level. Rather than hard-coding localhost, we might want to have a syspref to allow admins to add additional local domains. While it seems unlikely for most real world scenarios I can think of off the top of my head, it is possible for special/edge cases.
(In reply to David Cook from comment #4) > Hmm, technically I think that Tomas might be wrong to raise a bug for the > email library. > > A valid email address is just a localpart@domain where domain can be a > domain name or an IP address. As noted in this patch, the domain could be > localhost. It could be a computer name, it could be an internal domain of > some other type. You're probably right. RFC 5322 says the domain-literal needs to make sense in the context being used: https://datatracker.ietf.org/doc/html/rfc5322#section-3.4.1 I was looking at the SMTP RFC: https://datatracker.ietf.org/doc/html/rfc5321#section-2.3.5 and thought in the context of SMTP it should really be a FQDN, an IP or some reserved words like localhost. Either way, it is not the email address parser responsibility, but the transport's.
(In reply to David Cook from comment #4) > Hmm, technically I think that Tomas might be wrong to raise a bug for the > email library. I'd like to hear from the author, though. I need to know if that project is alive at all.
(In reply to Tomás Cohen Arazi (tcohen) from comment #6) > (In reply to David Cook from comment #4) > > Hmm, technically I think that Tomas might be wrong to raise a bug for the > > email library. > > I'd like to hear from the author, though. I need to know if that project is > alive at all. Mmm good call. Fingers crossed they get back to you.
(In reply to Tomás Cohen Arazi (tcohen) from comment #5) > Either way, it is not the email address parser responsibility, but the > transport's. I figure the SMTP client will work it out in the end, as per its specs, but it could be interesting to have validation in Koha as well.
(In reply to David Cook from comment #8) > (In reply to Tomás Cohen Arazi (tcohen) from comment #5) > > Either way, it is not the email address parser responsibility, but the > > transport's. > > I figure the SMTP client will work it out in the end, as per its specs, but > it could be interesting to have validation in Koha as well. If you use a smarthost to push the mail further for later processing (in my case in another mail container), Koha will just mark these wrong mails as sent (like successful) while they will be failing later on.
I am wondering too if we should allow IP addresses and local host names (like localhost etc) by default? I guess not. See also bug 28017 which was (partially) undone when moving away from Email::Valid.
(In reply to Marcel de Rooy from comment #9) > (In reply to David Cook from comment #8) > > (In reply to Tomás Cohen Arazi (tcohen) from comment #5) > > > Either way, it is not the email address parser responsibility, but the > > > transport's. > > > > I figure the SMTP client will work it out in the end, as per its specs, but > > it could be interesting to have validation in Koha as well. > > If you use a smarthost to push the mail further for later processing (in my > case in another mail container), Koha will just mark these wrong mails as > sent (like successful) while they will be failing later on. Suppose that using the queue runner in exim would have the same result btw.
Comment on attachment 187917 [details] [review] Bug 41025: Check for a dot in domain name, allow localhost This is not enough and arguable
Changed the title to be a bit more generic. To include this observation. is_valid decides that an email address is valid when parse creates one or more Email::Address objects. That is imo a wrong algorithm. Do we really want to check multiple lines / addresses here in the first place? The line "Name" <some@domain.com> is parsed. The address is available via object->address. But is_valid just looks at the count and says the line is valid! So we are going to push the line to SMTP, but not the parsed address! Does not work. The confusion about localhost, non-FQDN etc. still lingers here too.
(In reply to Marcel de Rooy from comment #9) > If you use a smarthost to push the mail further for later processing (in my > case in another mail container), Koha will just mark these wrong mails as > sent (like successful) while they will be failing later on. That's true. That is often confusing for users.