After the upgrade from 18.11 to 20.11, users with non-ASCII characters (e.g. German umlauts) in their first name or surname can no longer send shopping carts because the newly introduced email address validation of the create function of Email.pm fails. A possible solution would be to omit the display name generated from first name and surname in opac-sendbasket.pl. diff --git a/opac/opac-sendbasket.pl b/opac/opac-sendbasket.pl index 55dc7546bc..c4dee93e3d 100755 --- a/opac/opac-sendbasket.pl +++ b/opac/opac-sendbasket.pl @@ -59,7 +59,7 @@ if ( $email_add ) { my $user_email = $patron->first_valid_email_address || C4::Context->preference('KohaAdminEmailAddress'); - my $email_replyto = $patron->firstname . " " . $patron->surname . " <$user_email>"; + my $email_replyto = $user_email; my $comment = $query->param('comment'); # Since we are already logged in, no need to check credentials again
Email::Valid is no longer maintained and there are several known bugs (including this one). We should reconsider this module. https://github.com/Perl-Email-Project/Email-Valid/issues https://github.com/Perl-Email-Project/Email-Valid/issues/47
I don't know what to do. I have the feeling that what Ulrich is suggesting is not the right thing as we are using the "Full name <email@domain>" format for a while (bug 3280) now. On the other hand removing the validation seems bad as well. Adding a split in Koha::Email is not optimal (we could search for "<>" and pass that to Email::Valid instead of the whole replyto string). Any good other suggestions, Tomas?
Trying something for this.
Ok, I've been playing with this. My conclusion is that Jonathan is right: Email::Valid is buggy. I've learnt that regexps for validating RFC2822 addresses are not trivial [1] The author/maintainer of the libraries we use, also maintains the Email::Address library, that provides such regular expressions. My tests (on Debian Buster) verify that Email::Address is able to handle UTF-8 characters correctly. I can submit a patch that replaces one library with the other (will probably do it anyway) but I have doubts about the importance of validating email addresses here. It feels like overkill. [1] https://metacpan.org/pod/Email::Address#Package-Variables The script I used for testing: #!/usr/bin/perl use Modern::Perl; use utf8; use Email::Address; use Email::Valid; binmode STDOUT, ':encoding(UTF-8)'; binmode STDERR, ':encoding(UTF-8)'; my $string = 'Tomás Cohen Arazi <tomascohen@theke.io>'; my ($email) = Email::Address->parse($string); ## This construct could be used instead of manually crafting ## the address in the $string format above. Same results # my $email = Email::Address->new( # 'Tomás Cohen Arazi' => 'tomacohe@theke.io' # ); print "Testing address: $email\n"; say "Email::Address tests =>"; if ( "$email" =~ m/$Email::Address::mailbox/ ) { say "Yay!"; } else { say "Boo!"; } say "Email::Valid tests =>"; if (Email::Valid->address( -address => "$email", -fqdn => 0 )) { say "Yay!"; } else { say "Boo!"; } 1;
Created attachment 124002 [details] [review] Bug 28870: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124003 [details] [review] Bug 28870: Use Email::Address to validate email addresses This patch adds a new dependency, Email::Address. It is used in Koha::Email to replace the current use of Email::Valid, which proved to be problematic when it comes to UTF-8 characters. Email::Address provides suitable regexes that -when used- keep our tests passing, but also deal better with UTF-8 data. To test: 1. Apply the regression tests patch 2. Notice the only change is that it tweaks a couple addresses so they contain umlauts and also have the "Description <address>" format that is used when sending carts. 3. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Tests fail! Things die because Email::Valid doesn't like the from we passed. 4. Run: $ sudo apt install libemail-address-perl 5. Apply this patch 6. Repeat 3 => SUCCESS: Tests pass! 7. Try what is described in comment 1 => SUCCESS: Things are back to normal 8. Sign off :-D 9. Send cookies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124004 [details] [review] Bug 28870: Remove traces of Email::Valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #2) > I don't know what to do. I have the feeling that what Ulrich is suggesting > is not the right thing as we are using the "Full name <email@domain>" format > for a while (bug 3280) now. > > On the other hand removing the validation seems bad as well. > > Adding a split in Koha::Email is not optimal (we could search for "<>" and > pass that to Email::Valid instead of the whole replyto string). > > Any good other suggestions, Tomas? I would like to add that if this solution based on Email::Address moves forward, I would like to make sure we don't build addresses manually, but pass Email::Address objects around instead. Or Koha::Email::Address objects if we wanted to do something Koha-ish on the addresses. I really like the library and the stringification it does.
I attempted to test, but always seem to have a problem with sending carts/list where I have an error "Problem sending the cart..." (before patches are applied, using koha-testing-docker). All the tests now ass. Also, does this address the issue of being able to send emails with accents and umlauts in them, or is that a separate bug? For example, I couldn't add an email address for a patron or system preferences where it has an accent or umlaut in either the address or the domain - get "Please enter a valid email address.".
Oops... tat should have said "All tests pass"!
(In reply to David Nind from comment #9) > I attempted to test, but always seem to have a problem with sending > carts/list where I have an error "Problem sending the cart..." (before > patches are applied, using koha-testing-docker). Yes, that's because KTD doesn't have an SMTP server running. But you should look at the error logs before and after the patches. The errors should go from: [2021/08/23 12:45:40] [WARN] Error sending mail: Invalid 'to' parameter: tomascohen+á@gmail.com at /kohadevbox/koha/opac/opac-sendbasket.pl line 178. into [2021/08/23 13:11:22] [WARN] Error sending mail: unable to establish SMTP connection to localhost port 25 > All the tests now ass. :-D > Also, does this address the issue of being able to send emails with accents > and umlauts in them, or is that a separate bug? For example, I couldn't add > an email address for a patron or system preferences where it has an accent > or umlaut in either the address or the domain - get "Please enter a valid > email address.". It really depends on the context. But generally I'd say it is a different bug, as validation on (say) memberentry is done a JavaScript level.
Created attachment 124117 [details] [review] Bug 28870: Move email address validation to a specific class method To ease testing and future changes if needed.
Created attachment 124118 [details] [review] Bug 28870: Use Email::Address->parse
Tomas, see the last 2 patches. It seems that matching the regex is not enough (last patch fixes a test). However there is still a test failing, root@localhost is considered valid (FQDN no longer required).
(In reply to Jonathan Druart from comment #14) > Tomas, see the last 2 patches. > > It seems that matching the regex is not enough (last patch fixes a test). > > However there is still a test failing, root@localhost is considered valid > (FQDN no longer required). Shoudl it not be valid since bug 28017?
Created attachment 124119 [details] [review] Bug 28870: non-FQDN addresses are valid
(In reply to Katrin Fischer from comment #15) > (In reply to Jonathan Druart from comment #14) > > Tomas, see the last 2 patches. > > > > It seems that matching the regex is not enough (last patch fixes a test). > > > > However there is still a test failing, root@localhost is considered valid > > (FQDN no longer required). > > Shoudl it not be valid since bug 28017? I forgot that one!
(In reply to Jonathan Druart from comment #14) > Tomas, see the last 2 patches. > > It seems that matching the regex is not enough (last patch fixes a test). > > However there is still a test failing, root@localhost is considered valid > (FQDN no longer required). On it.
Created attachment 124178 [details] [review] Bug 28870: Move email address validation to a specific class method To ease testing and future changes if needed. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124179 [details] [review] Bug 28870: Use Email::Address->parse Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124180 [details] [review] Bug 28870: non-FQDN addresses are valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ulrich, can we get your signoff on those patches please?
Created attachment 124183 [details] [review] Bug 28870: Move email address validation to a specific class method To ease testing and future changes if needed. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124184 [details] [review] Bug 28870: Use Email::Address->parse Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124185 [details] [review] Bug 28870: non-FQDN addresses are valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #22) > Ulrich, can we get your signoff on those patches please? We don't have a master environment we can test email functionality with right now. We could test with latest 20.11. I tried to rebase, resolved some 'easy' conflicts in the use statements on "Remove traces of Email::Valid", but then got stuck with below on the 4th patch: error: sha1 information is lacking or useless (about.pl). error: could not build fake ancestor Patch failed at 0001 Bug 28870: Move email address validation to a specific class method The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-28870-Move-email-address-validation-to-a-speci-qJAYKn.patch
Created attachment 124752 [details] [review] Bug 28870: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124753 [details] [review] Bug 28870: Use Email::Address to validate email addresses This patch adds a new dependency, Email::Address. It is used in Koha::Email to replace the current use of Email::Valid, which proved to be problematic when it comes to UTF-8 characters. Email::Address provides suitable regexes that -when used- keep our tests passing, but also deal better with UTF-8 data. To test: 1. Apply the regression tests patch 2. Notice the only change is that it tweaks a couple addresses so they contain umlauts and also have the "Description <address>" format that is used when sending carts. 3. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Tests fail! Things die because Email::Valid doesn't like the from we passed. 4. Run: $ sudo apt install libemail-address-perl 5. Apply this patch 6. Repeat 3 => SUCCESS: Tests pass! 7. Try what is described in comment 1 => SUCCESS: Things are back to normal 8. Sign off :-D 9. Send cookies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124754 [details] [review] Bug 28870: Remove traces of Email::Valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124755 [details] [review] Bug 28870: Move email address validation to a specific class method To ease testing and future changes if needed. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124756 [details] [review] Bug 28870: Use Email::Address->parse Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124757 [details] [review] Bug 28870: non-FQDN addresses are valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Letting pragmatism win here. I feel the is_valid method is in the wrong context here, perhaps a Koha::Email::Address object we pass around should be created.. or the class method here could be renamed validate_address() or something. But, the code works, the tests pass. Signing off.
Created attachment 124762 [details] [review] Bug 28870: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124763 [details] [review] Bug 28870: Use Email::Address to validate email addresses This patch adds a new dependency, Email::Address. It is used in Koha::Email to replace the current use of Email::Valid, which proved to be problematic when it comes to UTF-8 characters. Email::Address provides suitable regexes that -when used- keep our tests passing, but also deal better with UTF-8 data. To test: 1. Apply the regression tests patch 2. Notice the only change is that it tweaks a couple addresses so they contain umlauts and also have the "Description <address>" format that is used when sending carts. 3. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Tests fail! Things die because Email::Valid doesn't like the from we passed. 4. Run: $ sudo apt install libemail-address-perl 5. Apply this patch 6. Repeat 3 => SUCCESS: Tests pass! 7. Try what is described in comment 1 => SUCCESS: Things are back to normal 8. Sign off :-D 9. Send cookies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124764 [details] [review] Bug 28870: Remove traces of Email::Valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124765 [details] [review] Bug 28870: Move email address validation to a specific class method To ease testing and future changes if needed. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124766 [details] [review] Bug 28870: Use Email::Address->parse Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124767 [details] [review] Bug 28870: non-FQDN addresses are valid Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Katrin Fischer from comment #26) > (In reply to Jonathan Druart from comment #22) > > Ulrich, can we get your signoff on those patches please? > > We don't have a master environment we can test email functionality with > right now. Just FYI - if your email account provides SMTP settings you can enter those in the SMTP server configuration and use your email to test :-)
Pushed to master for 21.11, thanks to everybody involved!
Please add patches for 21.05.x if needed.
Created attachment 127521 [details] [review] 21.05.x: Bug 28870: Regression tests Hope this is ok.
Created attachment 127522 [details] [review] 21.05.x: Bug 28870: Use Email::Address to validate email addresses
Created attachment 127523 [details] [review] 21.05.x: Bug 28870: Remove traces of Email::Valid
Created attachment 127524 [details] [review] 21.05.x: Bug 28870: Move email address validation to a specific class method
Created attachment 127525 [details] [review] 21.05.x: Bug 28870: Use Email::Address->parse
Created attachment 127526 [details] [review] 21.05.x: Bug 28870: non-FQDN addresses are valid
*** Bug 28019 has been marked as a duplicate of this bug. ***
Pushed to 21.05.x for 21.05.05
(In reply to Kyle M Hall from comment #50) > Pushed to 21.05.x for 21.05.05 Thanks!
This was originally found in 20.11 - problem exists there for sure. Is it possible to backport?
Created attachment 127745 [details] [review] 20.11.x: Bug 28870: Regression tests cherry-pick applied clean for all patches. So here's a backport.
Created attachment 127746 [details] [review] 20.11.x: Bug 28870: Use Email::Address to validate email addresses
Created attachment 127747 [details] [review] 20.11.x: Bug 28870: Remove traces of Email::Valid
Created attachment 127748 [details] [review] 20.11.x: Bug 28870: Move email address validation to a specific class method
Created attachment 127749 [details] [review] 20.11.x: Bug 28870: Use Email::Address->parse
Created attachment 127750 [details] [review] 20.11.x: Bug 28870: non-FQDN addresses are valid
Thx, Michael!
Applying on 20.11.x I tried to use patches in this bug report but it failed on : error: sha1 information is lacking or useless (Koha/Email.pm). error: could not build fake ancestor Patch failed at 0001 Bug 28870: Move email address validation to a specific class method So I tried to cherry-pick from 21.05.x and it works. Unit test is OK so should be OK.
Pushed to 20.11.x for 20.11.12
Hi, I'm trying to see if 20.05.x is affected. How can I test this? I tried setting a server in koha-conf.xml like in bug 29330 comment 40 but it still tries to connect to localhost when sending an OPAC cart. I think it because I would need bug 22343 right? I can't find how it was done before bug 22343. Any hints?
(In reply to Victor Grousset/tuxayo from comment #62) > Hi, I'm trying to see if 20.05.x is affected. How can I test this? > > I tried setting a server in koha-conf.xml like in bug 29330 comment 40 but > it still tries to connect to localhost when sending an OPAC cart. I think it > because I would need bug 22343 right? > > I can't find how it was done before bug 22343. Any hints? Prior to bug 22343, Koha expected an MTA in localhost. What I did for testing in those days was installing postfix, and mutt. You can assign localhost as the resolution for any find and have the MTA deliver locally. It was usually enough to use mutt to read the root user emails for bounces and such.
> What I did for testing in those days was installing postfix, and mutt. You can assign localhost as the resolution for any find and have the MTA deliver locally. That might be a little adventure ^^" In case 20.05.x turns out to be affected - it should be, since it uses Email::Valid - the patch conflicts quite a lot due to bug 22343 missing. Unless there is a way that doesn't mess with too much code. I don't think it would be wise to backport (recode more likely) this fix in the less than 48h remaining before the last minor release of 20.05.x
*** Bug 28275 has been marked as a duplicate of this bug. ***
(In reply to Jonathan Druart from comment #41) > Pushed to master for 21.11, thanks to everybody involved! I don't think the libemail-address-perl got added to debian/control. 21.11 worked fine in koha-testing-docker (https://gitlab.com/koha-community/koha-testing-docker/-/issues/274), but looks like it fails to startup in a normal from-scratch installation.
(In reply to David Cook from comment #66) > (In reply to Jonathan Druart from comment #41) > > Pushed to master for 21.11, thanks to everybody involved! > > I don't think the libemail-address-perl got added to debian/control. > > 21.11 worked fine in koha-testing-docker > (https://gitlab.com/koha-community/koha-testing-docker/-/issues/274), but > looks like it fails to startup in a normal from-scratch installation. I'm not sure why we didn't use Email::Address::XS since Email::MIME has it as a dependency already...