Bug 28870 - Cart shipping fails because of Non-ASCII characters in display-name of reply-to address
Summary: Cart shipping fails because of Non-ASCII characters in display-name of reply-...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords: regression
: 28019 28275 (view as bug list)
Depends on: 8000
Blocks: 28803
  Show dependency treegraph
 
Reported: 2021-08-17 12:23 UTC by Ulrich Kleiber
Modified: 2022-12-12 21:23 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.12


Attachments
Bug 28870: Regression tests (1.56 KB, patch)
2021-08-20 21:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: Use Email::Address to validate email addresses (3.26 KB, patch)
2021-08-20 21:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: Remove traces of Email::Valid (5.59 KB, patch)
2021-08-20 21:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: Move email address validation to a specific class method (8.09 KB, patch)
2021-08-26 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: Use Email::Address->parse (733 bytes, patch)
2021-08-26 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: non-FQDN addresses are valid (1.08 KB, patch)
2021-08-26 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: Move email address validation to a specific class method (8.14 KB, patch)
2021-08-27 13:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: Use Email::Address->parse (786 bytes, patch)
2021-08-27 13:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: non-FQDN addresses are valid (1.82 KB, patch)
2021-08-27 13:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28870: Move email address validation to a specific class method (8.15 KB, patch)
2021-08-27 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: Use Email::Address->parse (788 bytes, patch)
2021-08-27 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: non-FQDN addresses are valid (1.14 KB, patch)
2021-08-27 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28870: Regression tests (1.62 KB, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: Use Email::Address to validate email addresses (3.32 KB, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: Remove traces of Email::Valid (5.64 KB, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: Move email address validation to a specific class method (8.20 KB, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: Use Email::Address->parse (851 bytes, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: non-FQDN addresses are valid (1.20 KB, patch)
2021-09-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28870: Regression tests (1.68 KB, patch)
2021-09-10 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28870: Use Email::Address to validate email addresses (3.37 KB, patch)
2021-09-10 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28870: Remove traces of Email::Valid (5.70 KB, patch)
2021-09-10 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28870: Move email address validation to a specific class method (8.26 KB, patch)
2021-09-10 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28870: Use Email::Address->parse (908 bytes, patch)
2021-09-10 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28870: non-FQDN addresses are valid (1.25 KB, patch)
2021-09-10 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
21.05.x: Bug 28870: Regression tests (1.73 KB, patch)
2021-11-10 23:31 UTC, Michael Hafen
Details | Diff | Splinter Review
21.05.x: Bug 28870: Use Email::Address to validate email addresses (2.99 KB, patch)
2021-11-10 23:32 UTC, Michael Hafen
Details | Diff | Splinter Review
21.05.x: Bug 28870: Remove traces of Email::Valid (5.83 KB, patch)
2021-11-10 23:32 UTC, Michael Hafen
Details | Diff | Splinter Review
21.05.x: Bug 28870: Move email address validation to a specific class method (8.21 KB, patch)
2021-11-10 23:33 UTC, Michael Hafen
Details | Diff | Splinter Review
21.05.x: Bug 28870: Use Email::Address->parse (956 bytes, patch)
2021-11-10 23:33 UTC, Michael Hafen
Details | Diff | Splinter Review
21.05.x: Bug 28870: non-FQDN addresses are valid (1.30 KB, patch)
2021-11-10 23:34 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: Regression tests (1.73 KB, patch)
2021-11-17 17:52 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: Use Email::Address to validate email addresses (2.99 KB, patch)
2021-11-17 17:52 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: Remove traces of Email::Valid (5.83 KB, patch)
2021-11-17 17:53 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: Move email address validation to a specific class method (8.10 KB, patch)
2021-11-17 17:53 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: Use Email::Address->parse (956 bytes, patch)
2021-11-17 17:54 UTC, Michael Hafen
Details | Diff | Splinter Review
20.11.x: Bug 28870: non-FQDN addresses are valid (1.30 KB, patch)
2021-11-17 17:54 UTC, Michael Hafen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ulrich Kleiber 2021-08-17 12:23:28 UTC
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
Comment 1 Jonathan Druart 2021-08-19 09:35:41 UTC
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
Comment 2 Jonathan Druart 2021-08-19 10:16:27 UTC
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?
Comment 3 Tomás Cohen Arazi 2021-08-20 17:51:28 UTC
Trying something for this.
Comment 4 Tomás Cohen Arazi 2021-08-20 21:06:18 UTC
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;
Comment 5 Tomás Cohen Arazi 2021-08-20 21:41:02 UTC
Created attachment 124002 [details] [review]
Bug 28870: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-08-20 21:41:09 UTC
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>
Comment 7 Tomás Cohen Arazi 2021-08-20 21:41:21 UTC
Created attachment 124004 [details] [review]
Bug 28870: Remove traces of Email::Valid

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2021-08-20 21:45:45 UTC
(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.
Comment 9 David Nind 2021-08-21 21:58:00 UTC
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.".
Comment 10 David Nind 2021-08-22 20:18:17 UTC
Oops... tat should have said "All tests pass"!
Comment 11 Tomás Cohen Arazi 2021-08-23 13:24:09 UTC
(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.
Comment 12 Jonathan Druart 2021-08-26 15:14:06 UTC
Created attachment 124117 [details] [review]
Bug 28870: Move email address validation to a specific class method

To ease testing and future changes if needed.
Comment 13 Jonathan Druart 2021-08-26 15:14:11 UTC
Created attachment 124118 [details] [review]
Bug 28870: Use Email::Address->parse
Comment 14 Jonathan Druart 2021-08-26 15:15:45 UTC
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).
Comment 15 Katrin Fischer 2021-08-26 15:19:09 UTC
(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?
Comment 16 Jonathan Druart 2021-08-26 15:21:53 UTC
Created attachment 124119 [details] [review]
Bug 28870: non-FQDN addresses are valid
Comment 17 Jonathan Druart 2021-08-26 15:22:26 UTC
(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!
Comment 18 Tomás Cohen Arazi 2021-08-26 17:45:42 UTC
(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.
Comment 19 Tomás Cohen Arazi 2021-08-27 13:04:33 UTC
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>
Comment 20 Tomás Cohen Arazi 2021-08-27 13:04:39 UTC
Created attachment 124179 [details] [review]
Bug 28870: Use Email::Address->parse

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2021-08-27 13:04:45 UTC
Created attachment 124180 [details] [review]
Bug 28870: non-FQDN addresses are valid

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Jonathan Druart 2021-08-27 13:30:45 UTC
Ulrich, can we get your signoff on those patches please?
Comment 23 Jonathan Druart 2021-08-27 13:42:33 UTC
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>
Comment 24 Jonathan Druart 2021-08-27 13:42:37 UTC
Created attachment 124184 [details] [review]
Bug 28870: Use Email::Address->parse

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Jonathan Druart 2021-08-27 13:42:42 UTC
Created attachment 124185 [details] [review]
Bug 28870: non-FQDN addresses are valid

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Katrin Fischer 2021-08-30 14:10:26 UTC
(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
Comment 27 Martin Renvoize 2021-09-10 12:06:09 UTC
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>
Comment 28 Martin Renvoize 2021-09-10 12:06:13 UTC
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>
Comment 29 Martin Renvoize 2021-09-10 12:06:17 UTC
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>
Comment 30 Martin Renvoize 2021-09-10 12:06:21 UTC
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>
Comment 31 Martin Renvoize 2021-09-10 12:06:26 UTC
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>
Comment 32 Martin Renvoize 2021-09-10 12:06:31 UTC
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>
Comment 33 Martin Renvoize 2021-09-10 12:08:33 UTC
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.
Comment 34 Nick Clemens 2021-09-10 13:45:42 UTC
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>
Comment 35 Nick Clemens 2021-09-10 13:45:46 UTC
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>
Comment 36 Nick Clemens 2021-09-10 13:45:50 UTC
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>
Comment 37 Nick Clemens 2021-09-10 13:45:53 UTC
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>
Comment 38 Nick Clemens 2021-09-10 13:45:59 UTC
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>
Comment 39 Nick Clemens 2021-09-10 13:46:04 UTC
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>
Comment 40 Nick Clemens 2021-09-10 13:48:13 UTC
(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 :-)
Comment 41 Jonathan Druart 2021-09-28 13:38:35 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 42 Kyle M Hall 2021-10-01 18:33:34 UTC
Please add patches for 21.05.x if needed.
Comment 43 Michael Hafen 2021-11-10 23:31:30 UTC
Created attachment 127521 [details] [review]
21.05.x: Bug 28870: Regression tests

Hope this is ok.
Comment 44 Michael Hafen 2021-11-10 23:32:06 UTC
Created attachment 127522 [details] [review]
21.05.x: Bug 28870: Use Email::Address to validate email addresses
Comment 45 Michael Hafen 2021-11-10 23:32:46 UTC
Created attachment 127523 [details] [review]
21.05.x: Bug 28870: Remove traces of Email::Valid
Comment 46 Michael Hafen 2021-11-10 23:33:16 UTC
Created attachment 127524 [details] [review]
21.05.x: Bug 28870: Move email address validation to a specific class method
Comment 47 Michael Hafen 2021-11-10 23:33:46 UTC
Created attachment 127525 [details] [review]
21.05.x: Bug 28870: Use Email::Address->parse
Comment 48 Michael Hafen 2021-11-10 23:34:09 UTC
Created attachment 127526 [details] [review]
21.05.x: Bug 28870: non-FQDN addresses are valid
Comment 49 Tomás Cohen Arazi 2021-11-12 18:16:15 UTC
*** Bug 28019 has been marked as a duplicate of this bug. ***
Comment 50 Kyle M Hall 2021-11-15 12:33:33 UTC
Pushed to 21.05.x for 21.05.05
Comment 51 Tomás Cohen Arazi 2021-11-15 13:05:03 UTC
(In reply to Kyle M Hall from comment #50)
> Pushed to 21.05.x for 21.05.05

Thanks!
Comment 52 Katrin Fischer 2021-11-17 14:21:42 UTC
This was originally found in 20.11 - problem exists there for sure. Is it possible to backport?
Comment 53 Michael Hafen 2021-11-17 17:52:09 UTC
Created attachment 127745 [details] [review]
20.11.x: Bug 28870: Regression tests

cherry-pick applied clean for all patches.  So here's a backport.
Comment 54 Michael Hafen 2021-11-17 17:52:56 UTC
Created attachment 127746 [details] [review]
20.11.x: Bug 28870: Use Email::Address to validate email addresses
Comment 55 Michael Hafen 2021-11-17 17:53:22 UTC
Created attachment 127747 [details] [review]
20.11.x: Bug 28870: Remove traces of Email::Valid
Comment 56 Michael Hafen 2021-11-17 17:53:52 UTC
Created attachment 127748 [details] [review]
20.11.x: Bug 28870: Move email address validation to a specific class method
Comment 57 Michael Hafen 2021-11-17 17:54:23 UTC
Created attachment 127749 [details] [review]
20.11.x: Bug 28870: Use Email::Address->parse
Comment 58 Michael Hafen 2021-11-17 17:54:51 UTC
Created attachment 127750 [details] [review]
20.11.x: Bug 28870: non-FQDN addresses are valid
Comment 59 Katrin Fischer 2021-11-18 11:14:22 UTC
Thx, Michael!
Comment 60 Fridolin Somers 2021-11-19 20:46:09 UTC
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.
Comment 61 Fridolin Somers 2021-11-19 20:46:43 UTC
Pushed to 20.11.x for 20.11.12
Comment 62 Victor Grousset/tuxayo 2021-11-21 02:59:41 UTC
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?
Comment 63 Tomás Cohen Arazi 2021-11-21 04:00:44 UTC
(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.
Comment 64 Victor Grousset/tuxayo 2021-11-21 05:02:35 UTC
> 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
Comment 65 Katrin Fischer 2022-02-19 22:31:53 UTC
*** Bug 28275 has been marked as a duplicate of this bug. ***
Comment 66 David Cook 2022-03-22 22:50:36 UTC
(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.
Comment 67 David Cook 2022-03-22 23:03:28 UTC
(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...