Bug 28017 - Allow non-FQDN and IP addresses in emails
Summary: Allow non-FQDN and IP addresses in emails
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 22343
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-22 17:27 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:25 UTC (History)
9 users (show)

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


Attachments
Bug 28017: Regression tests (1.44 KB, patch)
2021-03-22 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28017: Allow non-FQDN (@localhost) addresses (2.25 KB, patch)
2021-03-22 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28017: (follow-up) Make controller scripts use -fqdn as well (3.58 KB, patch)
2021-03-22 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28017: Regression tests (1.50 KB, patch)
2021-03-23 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28017: Allow non-FQDN (@localhost) addresses (2.31 KB, patch)
2021-03-23 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28017: (follow-up) Make controller scripts use -fqdn as well (3.64 KB, patch)
2021-03-23 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28017: Test IPs are supported (1.29 KB, patch)
2021-03-23 15:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28017: Regression tests (1.57 KB, patch)
2021-03-23 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28017: Allow non-FQDN (@localhost) addresses (2.38 KB, patch)
2021-03-23 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28017: Test IPs are supported (1.36 KB, patch)
2021-03-23 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28017: (follow-up) Fix Letters.t (1.95 KB, patch)
2021-04-06 11:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-03-22 17:27:17 UTC
Bug 22343 introduced a rewrite of Koha::Email. One of the changes it introduced was that it now validates all addresses, not just the 'To' header. This immediately affected anyone with default values like root@localhost.

An exchange with the library author [1] highlighted that the way we use Email::Valid is to blame for breaking on @localhost addresses. It also made it clear that this behavior can be overridden by using a trivial flag on the call.

While the RFC for email systems specify that FQDN needs to be used, debugging bug 26705 highlighted we need this to allow anyone to test email stuffs.

I propose to allow non-fqdn addresses.

[1] https://github.com/Perl-Email-Project/Email-Valid/issues/49
Comment 1 Tomás Cohen Arazi 2021-03-22 17:56:31 UTC
Created attachment 118620 [details] [review]
Bug 28017: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-03-22 17:56:36 UTC
Created attachment 118621 [details] [review]
Bug 28017: Allow non-FQDN (@localhost) addresses

This patch makes Koha::Email support using @localhost addresses.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Koha::Email doesn't support non-fqdn addresses
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-03-22 17:56:42 UTC
Created attachment 118622 [details] [review]
Bug 28017: (follow-up) Make controller scripts use -fqdn as well

This patch makes the *memberentry.pl scripts use the -fqdn flag as well.
This is to allow setting the @localhost addresses.

I'm not sure about the need for this patch, as we shouldn't probably
allow end-users to use such addresses, and for testing purposes we can
set things on the DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Martin Renvoize 2021-03-23 10:25:56 UTC
Created attachment 118644 [details] [review]
Bug 28017: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-03-23 10:25:59 UTC
Created attachment 118645 [details] [review]
Bug 28017: Allow non-FQDN (@localhost) addresses

This patch makes Koha::Email support using @localhost addresses.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Koha::Email doesn't support non-fqdn addresses
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-03-23 10:26:02 UTC
Created attachment 118646 [details] [review]
Bug 28017: (follow-up) Make controller scripts use -fqdn as well

This patch makes the *memberentry.pl scripts use the -fqdn flag as well.
This is to allow setting the @localhost addresses.

I'm not sure about the need for this patch, as we shouldn't probably
allow end-users to use such addresses, and for testing purposes we can
set things on the DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-03-23 10:27:28 UTC
Signing off because it works as described..

However, I'm not entirely sure about how far it goes.

1. I think we don't need/want the final patch
2. I'm not sure we want to disable the domain checking for all the address headers..  shouldn't we be a bit more selective?
Comment 8 Martin Renvoize 2021-03-23 10:32:01 UTC
Addresses fields currently affected:

* 'To' - Shouldn't we require fully compliant non-local emails here?
* 'From' - This one I can understand the need for localhost
* 'Reply-to' - Again, this one feels like we ought to require a fully compliant non-local email for
* 'Return-path' - This one is a more difficult call.. I could imagine an IP based address being used here maybe
* 'Cc' - Again, shouldn't this be a fully compliant email
* 'Bcc' - As above.

Just some food for thought...
Comment 9 Tomás Cohen Arazi 2021-03-23 10:34:22 UTC
(In reply to Martin Renvoize from comment #8)
> Addresses fields currently affected:
> 
> * 'To' - Shouldn't we require fully compliant non-local emails here?
> * 'From' - This one I can understand the need for localhost
> * 'Reply-to' - Again, this one feels like we ought to require a fully
> compliant non-local email for
> * 'Return-path' - This one is a more difficult call.. I could imagine an IP
> based address being used here maybe
> * 'Cc' - Again, shouldn't this be a fully compliant email
> * 'Bcc' - As above.
> 
> Just some food for thought...

Maybe we need some syspref for enabling this. Like EmailDebug?
Comment 10 Tomás Cohen Arazi 2021-03-23 10:35:47 UTC
(In reply to Martin Renvoize from comment #8)
> * 'Return-path' - This one is a more difficult call.. I could imagine an IP
> based address being used here maybe

Using IPs requires other option switch (-allow_ip), actually.
Comment 11 Martin Renvoize 2021-03-23 14:55:29 UTC
I spoke to our sysadmin about this.. his suggestion was to allow IP and non-fqdn addresses at this low level.. which would allow people to 'get clever' with the admin settings.  But to drop the last patch here, as the more user-facing stuff is best left validated at a fairly strict level :)
Comment 12 Tomás Cohen Arazi 2021-03-23 15:20:43 UTC
Created attachment 118668 [details] [review]
Bug 28017: Test IPs are supported

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Kyle M Hall 2021-03-23 16:59:38 UTC
Created attachment 118696 [details] [review]
Bug 28017: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2021-03-23 16:59:50 UTC
Created attachment 118697 [details] [review]
Bug 28017: Allow non-FQDN (@localhost) addresses

This patch makes Koha::Email support using @localhost addresses.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Koha::Email doesn't support non-fqdn addresses
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2021-03-23 16:59:53 UTC
Created attachment 118698 [details] [review]
Bug 28017: Test IPs are supported

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Jonathan Druart 2021-04-01 16:55:23 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 17 Jonathan Druart 2021-04-06 08:54:44 UTC
Some tests failing, please fix ASAP

t/db_dependent/Letters.t .. 83/84 Fake send_or_die at t/db_dependent/Letters.t line 41.

    #   Failed test 'Exception thrown if invalid email is passed'
    #   at t/db_dependent/Letters.t line 995.
    # expecting: Koha::Exceptions::BadParameter
    # found: normal exit

    #   Failed test 'Invalid KohaAdminEmailAddress => status pending'
    #   at t/db_dependent/Letters.t line 999.
    #          got: 'sent'
    #     expected: 'pending'
Fake send_or_die at t/db_dependent/Letters.t line 41.

    #   Failed test 'Message 1 status is unchanged'
    #   at t/db_dependent/Letters.t line 1006.
    #          got: 'sent'
    #     expected: 'pending'
    # Looks like you planned 5 tests but ran 6.
    # Looks like you failed 3 tests of 6 run.
Comment 18 Tomás Cohen Arazi 2021-04-06 11:18:17 UTC
Created attachment 119208 [details] [review]
Bug 28017: (follow-up) Fix Letters.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Jonathan Druart 2021-04-06 13:59:38 UTC
Follow-up pushed to master.
Comment 20 Fridolin Somers 2021-04-13 15:05:20 UTC
Pushed to 20.11.x for 20.11.05
Comment 21 Andrew Fuerste-Henry 2021-04-24 15:41:29 UTC
Doesn't apply cleanly to 20.05. I'm guessing from comments this depends on 22343 (which is a 20.11 enhancement)? Please rebase if not dependent on 22343 and needed in 20.05.