Possible bug for fix in 2.11.0: null sender in releasing email from sql quarantine

[Mark Martinec]

On 2016-04-05 12:45, Tobias wrote:
> I also ran into this issue while configuring amavis. I use 2.10.1, and
> this code is unchanged in 2.11.0.
> 
> If SQL is specified as the quarantine method then variable
> $mailfrom_to_quarantine has no impact on headers or envelope. And also
> if X-Envelope-From is not added before the mail is delivered to
> amavis, then it is never added.
> 
> My postfix MTA does not add this header, nor is any Return-Path header
> added until actual mailbox delivery.
> 
> I found no way to influence this behavior from the configuration file
> so I ended up commenting out the $orig_env_sender_retained = 1, to
> always add X-Envelope-From.
> 
> @@ -16998,7 +16998,7 @@
>        # locations which do not depend on envelope
>        $quar_msg->sender($msginfo->sender);  # original sender
>        $quar_msg->sender_smtp($msginfo->sender_smtp);
> -      $orig_env_sender_retained = 1;
> +      #$orig_env_sender_retained = 1;
>      } elsif (defined $mftq) {  # have a replacement and smtp, lmtp, 
> pipe, local
>        $quar_msg->sender($mftq);
>        $mftq = qquote_rfc2821_local($mftq);
> /Tobias

> On 2016-03-18 03:03, Tom Johnson wrote:
>>> We're using a sql quarantine.
>>> We'd like to release emails from the quarantine and have the SMTP 
>>> envelope MAIL FROM reflect the original sender.
>>> So we set  mailfrom_to_quarantine to undef.
>>> But, since the email is quarantined without a Return-Path or 
>>> X-Envelope-From header.
>>> Postfix doesn't add those - because it's handling the email only via 
>>> a relay to amavisd-new.
>>> Amavisd-new doesn't add them - the check for whether to add the 
>>> X-Envelope-From header looks like this:
>>> 
>>>     my $mftq = c('mailfrom_to_quarantine');
>>>     if (!defined $mftq || $quar_m_protocol =~ /^(?:bsmtp|sql)\z/) {
>>>       # we keep the original envelope sender address if replacement 
>>> sender
>>>       # is not provided, or with quarantine methods which store to 
>>> fixed
>>>       # locations which do not depend on envelope
>>>       $quar_msg->sender($msginfo->sender);  # original sender
>>>       $quar_msg->sender_smtp($msginfo->sender_smtp);
>>>       $orig_env_sender_retained = 1;
>>>     }.....
>>>     ...
>>>     if (!$orig_env_sender_retained) { # unless X-Envelope-* would be 
>>> redundant
>>>       $hdr_edits->prepend_header('X-Envelope-From', 
>>> $msginfo->sender_smtp);
>>>     }
>>> And when you use amavisd-release to release a message, it's checked 
>>> only for those two headers for the sender.
>>> 
>>> So it's always sent from a null sender.  The releasing code only 
>>> checks the headers
>>> of the message for an Return-Path or X-Envelope-From header.  It 
>>> doesn't even
>>> default to using mailfrom_to_quarantine, so setting that doesn't do 
>>> any good either.
>>> 
>>> This looks like a bug to me.  Or am I missing something?



Instead of keeping $orig_env_sender_retained at false, I think it would
be better to just add 'X-Envelope-From' unconditionally, can't hurt:

<  if (!$orig_env_sender_retained) { # unless X-Envelope-* would be 
redundant
<    $hdr_edits->prepend_header('X-Envelope-From', 
$msginfo->sender_smtp);
<  }

> $hdr_edits->prepend_header('X-Envelope-From', $msginfo->sender_smtp);


Will make it so for the release.

The original idea was that the X-Envelope-From with SQL is unnecessary,
since the envelope addresses are stored in SQL tables anyway.
It's just that the message release code never bothers to fetch these.
Instead of complicating the releasing code, it's probably the
simplest solution to just add the X-Envelope-From unconditionally
to all quarantined messages.

Thanks for the problem report and your patience.

   Mark