Bug 41025 - Koha::Email->is_valid needs more attention
Summary: Koha::Email->is_valid needs more attention
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-10-15 13:23 UTC by Marcel de Rooy
Modified: 2025-10-16 22:40 UTC (History)
2 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41025: Check for a dot in domain name, allow localhost (2.66 KB, patch)
2025-10-15 15:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2025-10-15 13:23:49 UTC

    
Comment 1 Marcel de Rooy 2025-10-15 13:25:29 UTC
Related to change from Email::Valid to Email::Address
Comment 2 Marcel de Rooy 2025-10-15 14:15:39 UTC
Tomas added an issue on CPAN:
https://github.com/Perl-Email-Project/Email-Address/issues/24
Comment 3 Marcel de Rooy 2025-10-15 15:09:55 UTC
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>
Comment 4 David Cook 2025-10-15 22:26:04 UTC
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.
Comment 5 Tomás Cohen Arazi (tcohen) 2025-10-15 23:49:53 UTC
(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.
Comment 6 Tomás Cohen Arazi (tcohen) 2025-10-15 23:50:24 UTC
(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.
Comment 7 David Cook 2025-10-16 01:08:31 UTC
(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.
Comment 8 David Cook 2025-10-16 01:13:57 UTC
(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.
Comment 9 Marcel de Rooy 2025-10-16 10:11:08 UTC
(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.
Comment 10 Marcel de Rooy 2025-10-16 10:13:53 UTC
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.
Comment 11 Marcel de Rooy 2025-10-16 11:39:52 UTC
(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 12 Marcel de Rooy 2025-10-16 14:12:58 UTC
Comment on attachment 187917 [details] [review]
Bug 41025: Check for a dot in domain name, allow localhost

This is not enough and arguable
Comment 13 Marcel de Rooy 2025-10-16 14:19:24 UTC
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.
Comment 14 David Cook 2025-10-16 22:40:47 UTC
(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.