Bug 28729 - Return-path header not set in emails
Summary: Return-path header not set in emails
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords: regression
Depends on: 22343
Blocks: 29365
  Show dependency treegraph
 
Reported: 2021-07-21 13:48 UTC by Katrin Fischer
Modified: 2024-02-16 15:00 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
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 28729: Reverse 'sender' and 'from' (2.08 KB, patch)
2021-10-18 13:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28729: Unit tests (1.59 KB, patch)
2021-10-19 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28729: Make Koha::Email->send_or_die handle returnpath (2.12 KB, patch)
2021-10-19 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28729: Unit tests (1.64 KB, patch)
2021-10-27 15:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 28729: Make Koha::Email->send_or_die handle returnpath (2.16 KB, patch)
2021-10-27 15:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 28729: Make Koha::Email->send_or_die handle returnpath (2.72 KB, patch)
2021-10-29 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28729: Unit tests (1.73 KB, patch)
2021-10-29 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28729: Additions to POD (1.08 KB, patch)
2021-10-29 12:23 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-07-21 13:48:51 UTC
We have been testing the email setup in Koha comparing our 18.11 installations with a 20.11 one. In those tests we see an issue with the Return-Path header.

The Return-Path header should be using (if set) the
* ReturnpathDefault system preference
* Return-Path from library configuration

But in 20.11 we see that it's set using
* KohaAdminEmailAddress
* branch email

This is quite an issue. Wer are using specific and different email addresses for each of those settings:

* KohaAdminEmailAddress / branch email
* ReturnPathDefault / return-path for library
* ReplytoDefault / reply to for library

Something between versions has changed to cause the change in the Return-Path header. Maybe another side effect of the SMTP library change?
Comment 1 Katrin Fischer 2021-07-21 13:51:43 UTC
Tests were done using the cart email (directly sent) and the checkout notice (goes through message_queue).
Comment 2 Jonathan Druart 2021-07-28 08:25:43 UTC
What's the expected behaviour? For each of the headers! :)
Because it's confusing.

I can confirm the bug in master:
KohaAdminEmailAddress => root@example.org
ReturnPathDefault => ReturnPathDefault@default.org
No emails defined at library level

"""
From: root@example.org
Subject: Checkouts
To: edna@example.org
Sender: ReturnPathDefault@default.org
"""


My proposed fix would be:

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 177a4f51f67..ca02293c492 100644
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -1372,6 +1372,7 @@ sub _send_message_by_email {
     my $from_address =
          $message->{'from_address'}
       || $branch_email
+      || $branch_returnpath
       || C4::Context->preference('KohaAdminEmailAddress');
     if( !$from_address ) {
         _set_message_status({

But I am not sure what should be used for "From" and "Sender".
Comment 3 Jonathan Druart 2021-07-28 08:30:57 UTC
However I don't find which commit could have caused a change on the "From" (is that what we are talking about)?
Comment 4 Katrin Fischer 2021-07-28 09:27:39 UTC
Hi Jonathan,

thx for checking.

The change is not in the From header, the change is the Return-Path header.

As far as I understand the issue: We have never set the Return-Path as such, but we have used "Sender".  
See: https://stackoverflow.com/questions/19334223/how-can-we-set-return-path-in-a-mail-when-we-use-mailsendmail-module-to-se)

In the emails sent, this resulted in Sender and Return-Path headers being set to the same email address - the one configured in our Return-Path configuration settings.

As we are on 18.11 at the moment, there is a bit of a version gap in my testing. 
With the changes to STMP configuration in mind, we have tested 20.11 more closely.

There the change appears:

Now Sender is still correct, but Return-Path shows the information from From (branchemail, KohaAdminEmailAddress).

The Return-Path header appears on the first line from the other headers in my testing, so can be missed easily when looking at the other headers that appear further below.

Sender and Return-Path should always match. They did, but not anymore.
Comment 5 Katrin Fischer 2021-07-28 09:32:17 UTC
> My proposed fix would be:
> 
> diff --git a/C4/Letters.pm b/C4/Letters.pm
> index 177a4f51f67..ca02293c492 100644
> --- a/C4/Letters.pm
> +++ b/C4/Letters.pm
> @@ -1372,6 +1372,7 @@ sub _send_message_by_email {
>      my $from_address =
>           $message->{'from_address'}
>        || $branch_email
> +      || $branch_returnpath
>        || C4::Context->preference('KohaAdminEmailAddress');
>      if( !$from_address ) {
>          _set_message_status({
> 
> But I am not sure what should be used for "From" and "Sender".

This change would be wrong - the From address correct with branchemail or KohaAdminEmailAddress.
Comment 6 Katrin Fischer 2021-07-28 09:40:31 UTC
I was going to add: I suspect the change is more likely to be caused by the change of Perl module than a Koha code change, but I don't have prove.
Comment 7 Tomás Cohen Arazi 2021-07-28 10:33:56 UTC
(In reply to Katrin Fischer from comment #6)
> I was going to add: I suspect the change is more likely to be caused by the
> change of Perl module than a Koha code change, but I don't have prove.

I didn't find traces of Return-path being set in Mail::Sendmail. That's why I'm a bit confused about what changed.

On the other hand, it is correct to set the return path correctly :-). If you want me to do that, I can just do it.
Comment 8 Katrin Fischer 2021-07-28 11:01:58 UTC
Could it be that it was not set, so the MTA did it - and it is set with the new library now, but with From if not set explicitly?
Comment 9 Katrin Fischer 2021-07-28 11:04:48 UTC
In some documentation I read that the MTA copy Sender if no Return-Path is set, which. That could be a possible explanation.
Comment 10 Katrin Fischer 2021-07-28 11:13:02 UTC
> On the other hand, it is correct to set the return path correctly :-). If
> you want me to do that, I can just do it.

Please do!
Comment 11 Tomás Cohen Arazi 2021-07-28 12:02:16 UTC
Sender and Return-path are envelope headers. So the email has a From, but the envelope also says 'This is sent by Sender on behalf of From, and in the event of a delivery failure, please send the failure to Return-path'.

My perception is that ReturnpathDefault/$library->returnpath should be used to set the Return-path, and the Sender. Unless we want to be incredibly flexible and split them. But that'd be out of the scope of the bug, and probably that should be solved by sysadmins with some rules on the MTA.
Comment 12 Katrin Fischer 2021-07-28 12:06:35 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> Sender and Return-path are envelope headers. So the email has a From, but
> the envelope also says 'This is sent by Sender on behalf of From, and in the
> event of a delivery failure, please send the failure to Return-path'.
> 
> My perception is that ReturnpathDefault/$library->returnpath should be used
> to set the Return-path, and the Sender. 

I agree - Sender and Return-path should be set to the same address. 

> Unless we want to be incredibly
> flexible and split them. But that'd be out of the scope of the bug, and
> probably that should be solved by sysadmins with some rules on the MTA.

This would be a change of behaviour and I see no use case where this would be necessary.
Comment 13 Jonathan Druart 2021-07-28 12:32:36 UTC
So if I understand correctly this is a new enhancement request, not a regression?
Comment 14 Katrin Fischer 2021-07-28 12:43:21 UTC
(In reply to Jonathan Druart from comment #13)
> So if I understand correctly this is a new enhancement request, not a
> regression?

No, it's a regression and a bug.

We have configuration options for the Return-Path in Koha that now are no longer used/functional.
Comment 15 Katrin Fischer 2021-07-28 12:44:16 UTC
Any library relying on the return-path paramters might be likely to no longer receive emails bouncing back.
Comment 16 Jonathan Druart 2021-07-28 12:51:13 UTC
Looks like we never set the "Return-Path" from Koha and haven't found anything related in the Mail::Sendmail library.
Comment 17 Katrin Fischer 2021-07-28 12:58:49 UTC
(In reply to Jonathan Druart from comment #16)
> Looks like we never set the "Return-Path" from Koha and haven't found
> anything related in the Mail::Sendmail library.

But it worked. Just because we haven't found out why it changed yet, doesn't mean this isn't a regression. Because the behaviour change can be proved.

The Return-path functionality was developed, tested and agreed on in bug 9350 in 2014. I can promise it worked when added to Koha and has worked since then at least up to 18.11 but probably in later versions as well.
Comment 18 Jonathan Druart 2021-07-28 13:00:26 UTC
Tried on 18.11.x

---------- MESSAGE FOLLOWS ----------
Sender: ReturnPathDefault@default.org
Mime-Version: 1.0
Charset: utf8
Content-Transfer-Encoding: quoted-printable
Subject: =?UTF-8?B?Q2hlY2tvdXRz?=
Content-Type: text/plain; charset="UTF-8"
Date: Wed, 28 Jul 2021 12:59:39 +0000
To: edna@example.org
From: root@example.org
X-Peer: 127.0.0.1

The following items have been checked out:
E Street shuffle=20
Thank you for visiting Centerville.=

------------ END MESSAGE ------------
Comment 19 Jonathan Druart 2021-07-28 13:10:24 UTC
Did you check the MTA config?
Comment 20 Ulrich Kleiber 2021-07-28 14:16:49 UTC
Hi Jonathan, the configuration of the MTA was not changed when upgrading to 20.11. It is the same on all our Koha Servers.
Comment 21 David Cook 2021-07-29 04:40:22 UTC
I wouldn't be surprised if this were related to Bug 26705.
Comment 22 David Cook 2021-07-29 04:43:54 UTC
(In reply to David Cook from comment #21)
> I wouldn't be surprised if this were related to Bug 26705.

I mean... not the same problem/fix, but that the new library might be messing with things.

I could be totally wrong though.
Comment 23 Marcel de Rooy 2021-07-29 09:03:19 UTC
Koha::Email (19.11)

    if ( C4::Context->preference('ReturnpathDefault') ) {
        $params->{sender} ||= C4::Context->preference('ReturnpathDefault');
    }

Mail::Sendmail
If you wish to use an envelope sender address different than the From: address, set $mail{Sender} in your %mail hash

MTAs will copy the Sender to Return-path

But in the meantime master has been changed quite a bit.
So the solution of this problem must be found in these changes?
Comment 24 Marcel de Rooy 2021-07-29 09:04:41 UTC
Bug 22343 ?
Comment 25 Marcel de Rooy 2021-07-29 09:06:12 UTC
(In reply to Marcel de Rooy from comment #24)
> Bug 22343 ?

This one was pushed to 20.11 not backported.
I think that the problem is there.
Comment 26 Katrin Fischer 2021-07-29 09:33:32 UTC
> Something between versions has changed to cause the change in the
> Return-Path header. Maybe another side effect of the SMTP library change?

That's what I was suspecting all along :)
Comment 27 Jonathan Druart 2021-07-29 09:38:32 UTC
Comment 18 shows that Koha didn't set Return-path in previous versions.
Comment 28 Ulrich Kleiber 2021-07-29 09:54:15 UTC
Maybe in Email.pm the envelope-from should be set to the return-path.

Koha 20.11

--- Email.pm    2021-07-29 11:07:24.560924469 +0200
+++ Email.pm.patched    2021-07-29 11:49:01.108795689 +0200
@@ -76,9 +76,6 @@
     my ( $self, $params ) = @_;

     my $args = {};
-    $args->{from} = $params->{from} || C4::Context->preference('KohaAdminEmailAddress');
-    Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from})
-        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0 ); # from is mandatory

     $args->{subject} = $params->{subject} // '';

@@ -117,6 +114,9 @@
           );
     }

+    $args->{from} = $addresses->{sender} || C4::Context->preference('ReturnpathDefault') || $params->{from} || C4::Context->preference('KohaAdminEmailAddress');
+    Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from})
+        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0 ); # from is mandatory
     $args->{cc} = $addresses->{cc}
         if $addresses->{cc};
     $args->{bcc} = $addresses->{bcc}
Comment 29 Ulrich Kleiber 2021-07-29 10:12:27 UTC
The previous patch does not work, because it unfortunately also changes the header-from.
Comment 30 Katrin Fischer 2021-07-29 10:17:12 UTC
(In reply to Jonathan Druart from comment #27)
> Comment 18 shows that Koha didn't set Return-path in previous versions.

Sorry, but I have to disagree.

We tested the initial development in 2014 and we have just tested with 18.11 and the Return-Path was definitely set correctly. 

I think it might depend on MTA and how you test the email. Local testing might be limited here. How did you test?

The issue we see now is also not that it's not set, but it's set to the wrong address, which is even worse.
Comment 31 Katrin Fischer 2021-07-29 10:20:08 UTC
(In reply to Marcel de Rooy from comment #23)
> Koha::Email (19.11)
> 
>     if ( C4::Context->preference('ReturnpathDefault') ) {
>         $params->{sender} ||= C4::Context->preference('ReturnpathDefault');
>     }
> 
> Mail::Sendmail
> If you wish to use an envelope sender address different than the From:
> address, set $mail{Sender} in your %mail hash
> 
> MTAs will copy the Sender to Return-path
> 
> But in the meantime master has been changed quite a bit.
> So the solution of this problem must be found in these changes?

Thx for digging this up. It's also the explanation I found so far. 

The strange thing now is that we see the wrong setting in Return-Path. Instead of using the information from Sender it appears that From is used for setting the Return-Path.

I would suspect that this is not done by the MTA that hasn't changed then, but there shoudl be a visible change in the Koha email format too.
Comment 32 Marcel de Rooy 2021-07-29 11:30:52 UTC
(In reply to Jonathan Druart from comment #18)
> Tried on 18.11.x
> 
> ---------- MESSAGE FOLLOWS ----------
> Sender: ReturnPathDefault@default.org
> Mime-Version: 1.0
> Charset: utf8
> Content-Transfer-Encoding: quoted-printable
> Subject: =?UTF-8?B?Q2hlY2tvdXRz?=
> Content-Type: text/plain; charset="UTF-8"
> Date: Wed, 28 Jul 2021 12:59:39 +0000
> To: edna@example.org
> From: root@example.org
> X-Peer: 127.0.0.1
> 
> The following items have been checked out:
> E Street shuffle=20
> Thank you for visiting Centerville.=
> 
> ------------ END MESSAGE ------------

So this proves that if the MTA copies Sender to Return-path, Koha indirectly sets Return-path :)
It is not enough to just look at headers leaving Koha; also look at what happens between sender and receiving side.
Comment 33 Katrin Fischer 2021-07-29 12:00:05 UTC
> So this proves that if the MTA copies Sender to Return-path, Koha indirectly
> sets Return-path :)
> It is not enough to just look at headers leaving Koha; also look at what
> happens between sender and receiving side.

Thx for the test - could you maybe repeat on master/20.11 for checking on differences? If the MTA hasn't changed like in our case... Koha might send something slightly different?
Comment 34 Ulrich Kleiber 2021-07-29 15:12:26 UTC
With this patch only the envelope-from is changed. After that, emailing under 20.11 behaves much like emailing under 18.11. At least that's true for our installations.

It's not pretty and I don't know if it covers everything or has any negative side effects. I hope this helps with further troubleshooting.

--- Email.pm	2021-07-29 11:07:24.560924469 +0200
+++ Email.pm.patched	2021-07-29 16:33:26.636468041 +0200
@@ -173,6 +173,9 @@
         $args->{to} = \@recipients;
     }
 
+    $args->{from} = $self->email->header("Sender") || C4::Context->preference('KohaAdminEmailAddress');
+    Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from})
+        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0 ); # from is mandatory
     $self->SUPER::send_or_die($args);
 }
Comment 35 Marcel de Rooy 2021-07-30 06:18:42 UTC
(In reply to Ulrich Kleiber from comment #34)
> With this patch only the envelope-from is changed. After that, emailing
> under 20.11 behaves much like emailing under 18.11. At least that's true for
> our installations.
> 
> It's not pretty and I don't know if it covers everything or has any negative
> side effects. I hope this helps with further troubleshooting.
> 
> --- Email.pm	2021-07-29 11:07:24.560924469 +0200
> +++ Email.pm.patched	2021-07-29 16:33:26.636468041 +0200
> @@ -173,6 +173,9 @@
>          $args->{to} = \@recipients;
>      }
>  
> +    $args->{from} = $self->email->header("Sender") ||
> C4::Context->preference('KohaAdminEmailAddress');
> +    Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter:
> ".$args->{from})
> +        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0
> ); # from is mandatory
>      $self->SUPER::send_or_die($args);
>  }

The default for Return-Path should be preference ReturnpathDefault.
Is that still used then?
Comment 36 Ulrich Kleiber 2021-07-30 07:35:31 UTC
If no branchreturnpath exists, sender and thus envelope-from will be set to ReturnpathDefault.

Maybe we should use

$args->{from} ||= $self->email->header("Sender");

instead of

$args->{from} = $self->email->header("Sender") || C4::Context->preference('KohaAdminEmailAddress');

in the upper patch.
Comment 37 Ulrich Kleiber 2021-07-30 07:57:56 UTC
The described change of the patch in my last comment is incorrect. Please ignore.
Comment 38 Katrin Fischer 2021-07-30 09:22:34 UTC
> The default for Return-Path should be preference ReturnpathDefault.
> Is that still used then?

As we copy the Sender information that behaves correctly in my testing, I think that is the case. You can find this line further above in Email.pm create()

$addresses->{sender} ||= C4::Context->preference('ReturnpathDefault')
        if C4::Context->preference('ReturnpathDefault');
Comment 39 Martin Renvoize 2021-10-18 13:37:10 UTC
OK.. I've dug through this a bit.. man the Email::Stuffer and friends and Mail::Sendmail docs are hard to understand.


In short, I think 'Sender' and 'From' are basically reversed between the two sets of modules.. I would say we need to set 'From' to 'returnpath' info (Thus setting the envelope MAIL FROM' used in the mail server connection and then if we have a more specific 'From' we want in the actual mail we set that in the 'Sender' slot.
Comment 40 Martin Renvoize 2021-10-18 13:48:25 UTC
Created attachment 126436 [details] [review]
Bug 28729: Reverse 'sender' and 'from'

Having read through the docs for both Email::Stuffer (Email::Sender) and
Mail::Sendmail, I believe the meaning of 'from' and 'Sender' has
reversed between the two modules.

Email::Sendmail interpreted 'from' as the from address of the mail and
used Sender as an override for the envelope when it was passed. However,
in Email::Stuffer the 'from' is set at the envelope level and thus sets
the connection 'MAIL FROM' which is subsequently used as the return-path
by the mail server. It then uses 'Sender' as the message from address if
one is passed in as a header.

This patch reverses our handling of those attribute in Koha::Email.
Comment 41 Katrin Fischer 2021-10-19 15:19:00 UTC
Hi Martin,

thanks a lot for looking into this! We'll try to test your fix, this is the patch we have been running with meanwhile:

Set the envelope-from and therefore the return-path to sender.
---
 Koha/Email.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Koha/Email.pm b/Koha/Email.pm
index 9c077a6b84..068f8d9004 100644
--- a/Koha/Email.pm
+++ b/Koha/Email.pm
@@ -173,6 +173,9 @@ sub send_or_die {
         $args->{to} = \@recipients;
     }
 
+    $args->{from} = $self->email->header("Sender") || C4::Context->preference('KohaAdminEmailAddress');
+    Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from})
+        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0 ); # from is mandatory
     $self->SUPER::send_or_die($args);
 }
Comment 42 Martin Renvoize 2021-10-19 16:12:46 UTC
I changed my mind.. patches inbound
Comment 43 Martin Renvoize 2021-10-19 16:22:57 UTC
Created attachment 126526 [details] [review]
Bug 28729: Unit tests
Comment 44 Martin Renvoize 2021-10-19 16:23:02 UTC
Created attachment 126527 [details] [review]
Bug 28729: Make Koha::Email->send_or_die handle returnpath

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library would extract the from address from the Sender
header to pass along in the MAIL FROM: field of the SMTP protocol [1].

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior as it expects the envelope to be passed and
falls back to extracting the 'From' header when said envelope is not
found [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {from} parameter is passed, it extracts the MAIL FROM
envelope from the Sender header, as Mail::Sendmail did, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the unit tests
2. Run:
   $ kshell
   k$ prove t/Koha/Email.t
   => FAIL: Sender is not handled correctly!
3. Apply this patch
4. Repeat 2
   => SUCCESS: Tests pass!
   The from parameter is correct!
   No Sender header sent!
5. Sign off :-D

[1] https://metacpan.org/dist/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L284
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#envelope-information
Comment 45 Martin Renvoize 2021-10-19 16:24:18 UTC
Wish you'd submitted that before I dug through it and came up with basically the same.
Comment 46 Katrin Fischer 2021-10-19 20:38:05 UTC
Actually, we did - comment#34. I missed it earlier too.
Comment 47 David Nind 2021-10-27 15:54:34 UTC
Created attachment 127016 [details] [review]
Bug 28729: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 David Nind 2021-10-27 15:54:39 UTC
Created attachment 127017 [details] [review]
Bug 28729: Make Koha::Email->send_or_die handle returnpath

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library would extract the from address from the Sender
header to pass along in the MAIL FROM: field of the SMTP protocol [1].

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior as it expects the envelope to be passed and
falls back to extracting the 'From' header when said envelope is not
found [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {from} parameter is passed, it extracts the MAIL FROM
envelope from the Sender header, as Mail::Sendmail did, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the unit tests
2. Run:
   $ kshell
   k$ prove t/Koha/Email.t
   => FAIL: Sender is not handled correctly!
3. Apply this patch
4. Repeat 2
   => SUCCESS: Tests pass!
   The from parameter is correct!
   No Sender header sent!
5. Sign off :-D

[1] https://metacpan.org/dist/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L284
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#envelope-information

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 Marcel de Rooy 2021-10-28 12:39:47 UTC
QA: Looking here
Comment 50 Marcel de Rooy 2021-10-28 14:23:46 UTC
Resuming tomorrow, dont worry
Comment 51 Marcel de Rooy 2021-10-29 09:52:27 UTC
Created attachment 127085 [details] [review]
Bug 28729: Make Koha::Email->send_or_die handle returnpath

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library would extract the from address from the Sender
header to pass along in the MAIL FROM: field of the SMTP protocol [1].

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior as it expects the envelope to be passed and
falls back to extracting the 'From' header when said envelope is not
found [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {from} parameter is passed, it extracts the MAIL FROM
envelope from the Sender header, as Mail::Sendmail did, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the unit tests
2. Run:
   $ kshell
   k$ prove t/Koha/Email.t
   => FAIL: Sender is not handled correctly!
3. Apply this patch
4. Repeat 2
   => SUCCESS: Tests pass!
   The from parameter is correct!
   No Sender header sent!
5. Sign off :-D

[1] https://metacpan.org/dist/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L284
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#envelope-information

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

AMENDED (SHORTENED)
-        my @headers = $self->email->header_str_pairs;
-        foreach my $pair ( pairs @headers ) {
-            my ( $header, $value ) = @$pair;
-            $args->{from} = $value if $header eq 'Sender';
-        }
-
-        # Remove the Sender header
-        $self->email->header_str_set('Sender');
+        $args->{from} = $self->email->header_str('Sender');
+        $self->email->header_str_set('Sender'); # remove Sender header

Tested with same results (scrambled domains):
From: noreply@mydevserver.com
Cc: marcel@email.nl
To: test@somewhere.nl
Reply-To: bieb@mydevserver.com
Return-Path: postmaster@mydevserver.com
Comment 52 Marcel de Rooy 2021-10-29 09:52:31 UTC
Created attachment 127086 [details] [review]
Bug 28729: Unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2021-10-29 09:54:47 UTC
The fix works fine on itself, but it feels like a fix on the wrong place. And a fix that more or less assumes that we are not calling send_or_die with the from parameter; this seems to the case in the current codebase where we only pass transport.

We should be fixing create too (origin of this problem). I will postpone these things to 29365. The test passes, could be improved too.

This serves as a minimal fix, allowing backports.
Comment 54 Marcel de Rooy 2021-10-29 10:03:36 UTC
We cover ReturnPathDefault but do not fallback to Admin like here:

+    $args->{from} = $self->email->header("Sender") || C4::Context->preference('KohaAdminEmailAddress');

I am keeping it in blocked over the weekend..
Comment 55 Martin Renvoize 2021-10-29 12:14:58 UTC
(In reply to Marcel de Rooy from comment #53)
> The fix works fine on itself, but it feels like a fix on the wrong place.
> And a fix that more or less assumes that we are not calling send_or_die with
> the from parameter; this seems to the case in the current codebase where we
> only pass transport.

Not sure what you mean here.. we account for 'from' being passed... the key challenge is understanding that the 'from' you might pass send_or_die is the envelope address and not the email from.

> We should be fixing create too (origin of this problem). I will postpone
> these things to 29365. The test passes, could be improved too.
> 
> This serves as a minimal fix, allowing backports.

I'm not sure what you mean about fixing create?  Sender is a bad name for this parameter and it was used due to a weird internal workaround provided by Mail::Sendmail..  I don't know that you could easily move the fix into create.. short of create producing the wrapper and returning a structure that's all nested one level deeper.
Comment 56 Martin Renvoize 2021-10-29 12:23:27 UTC
Created attachment 127094 [details] [review]
Bug 28729: Additions to POD
Comment 57 Marcel de Rooy 2021-11-01 08:26:42 UTC
(In reply to Martin Renvoize from comment #55)
> (In reply to Marcel de Rooy from comment #53)
> > The fix works fine on itself, but it feels like a fix on the wrong place.
> > And a fix that more or less assumes that we are not calling send_or_die with
> > the from parameter; this seems to the case in the current codebase where we
> > only pass transport.
> 
> Not sure what you mean here.. we account for 'from' being passed... the key
> challenge is understanding that the 'from' you might pass send_or_die is the
> envelope address and not the email from.
> 
> > We should be fixing create too (origin of this problem). I will postpone
> > these things to 29365. The test passes, could be improved too.
> > 
> > This serves as a minimal fix, allowing backports.
> 
> I'm not sure what you mean about fixing create?  Sender is a bad name for
> this parameter and it was used due to a weird internal workaround provided
> by Mail::Sendmail..  I don't know that you could easily move the fix into
> create.. short of create producing the wrapper and returning a structure
> that's all nested one level deeper.

Lets see on the new report.
Comment 58 Marcel de Rooy 2021-11-01 08:47:22 UTC
(In reply to Marcel de Rooy from comment #54)
> We cover ReturnPathDefault but do not fallback to Admin like here:
> 
> +    $args->{from} = $self->email->header("Sender") ||
> C4::Context->preference('KohaAdminEmailAddress');
> 
> I am keeping it in blocked over the weekend..

Dont think we should do that here.
If you keep the pref ReturnpathDefault empty, return path falls back correctly to sender. And we already use AdminEmail as a fallback.
The fix is fine as-is.
Comment 59 Jonathan Druart 2021-11-02 15:53:18 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 60 Kyle M Hall 2021-11-05 16:07:51 UTC
Pushed to 21.05.x for 21.05.05
Comment 61 Fridolin Somers 2021-11-11 01:43:23 UTC
Pushed to 20.11.x for 20.11.12
Comment 62 Victor Grousset/tuxayo 2021-11-16 08:12:33 UTC
Missing dependencies for 20.05.x, it shouldn't be affected, no backport.