94293 - Sendmail "from" header can not be configured in one config file, From header broken in SMTP

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Gerhard Wiesinger]

When you want to set the "from" item for the automatically sent emails one has 
to change in different perl scripts:

e.g. in CGI.pl:
Change
 open SENDMAIL, "|/usr/lib/sendmail -t";
to
 open SENDMAIL, "|/usr/lib/sendmail -t -f [e-mail]";

It would be nice to configure this globally.

Also the sendmail path (/usr/lib/sendmail) should be configured globally.

Comment 1

[Dave Miller [:justdave]]

You don't have to change any perl code to set the from header.  This is all in 
the preferences in editparams.cgi from the web.  Look for "passwordmail" and 
"newchangedmail".  Edit the headers to suit.

As for the sendmail location, that's bug 57798

Comment 2

[Gerhard Wiesinger]

Yes, thats is valid for the email itself (e.g. the headers) but not for the SMTP 
envelope:
Example:
Webserver runs under "[e-mail]", configured is as "from": 
[e-mail]. The receipient is [e-mail]

Then the following happens:
The envelope contains:
From: [e-mail]
To: [e-mail]

So the following SMTP session is established:
HELO ...
MAIL FROM: <[e-mail]>

When [e-mail] is in an intranet and mydomain.com is not valid outside  
the intranet mail sending does not work. (The other e.g. sendmail says: sender 
domain must exist).

So it must be possible to set the envelope with the -f option of sendmail.

The Email itself is correct:
From: [e-mail]
To: [e-mail]

Hope everything is clear => If not just mail me.

Comment 3

[Dave Miller [:justdave]]

In that case, I think this will be fixed by the patch on bug 84876.  Do you 
agree?

Comment 4

[Gerhard Wiesinger]

When ALL!! sendmail related things are abstracted into another API this is OK.

It must also be verified that the mail address from field (for the envelope) can 
be changed.

BTW: When will the patch be included in the next official version?

Comment 5

[Dave Miller [:justdave]]

That's a good question.  Right now it's targetted at 2.16, which is the next 
release after the one we're working on currently.  2.14 should be out any time 
now, and once it's out, we have over 90 patches waiting for review and checkin 
for 2.16 (that one among them).  It'd probably be safe to say within a few 
months.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 7

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 8

[Ben Bucksch (:BenB)]

Compare bug 126151, which is an alternative suggestion about the From header.

Comment 9

[Dave Miller [:justdave]]

Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed ([e-mail]).  Jake will be offline for a few months.

Comment 10

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 11

[Dave Miller [:justdave]]

The patch on bug 49893 fixes this.

Comment 12

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 13

[Jochen Wiedmann]

Attachment: Patch for setting the "from" address in the envelope

The attached patch searches for a "from" header in the mail being sent. If such a header is present, it adds an option "-f<fromaddr>" (sendmail delivery) or sets the environment address MAILADDRESS (smtp delivery). I haven't checked other delivery methods, because I have no possibility to verify them.

Comment 14

[Marc Schumann [:Wurblzap]]

There is bug 135812, too. Are the two bugs as identical as they seem to me?

Comment 15

[Frédéric Buclin]

this won't go into 2.22

Comment 16

[Jochen Wiedmann]

(In reply to comment #14)

> There is bug 135812, too. Are the two bugs as identical as they seem to me?

IMO, the two bugs are not identical, but related.

This bug adresses the fact, that setting the "from" address is not sufficient. Bug 135812 adresses the problem, that the "from" address should be configurable, preferrably at a central location.

Comment 17

[Jochen Wiedmann]

(In reply to comment #15)

> this won't go into 2.22

LpSolit: Would you please be so kind and explain the reasons? The suggested patch is relatively small and without side effects. (The latter point is, of course, subject to a review, but you didn't seem to review the patch.)

If my impression is right, then I can't think of a reason to defer landing this change.

Comment 18

[Frédéric Buclin]

(In reply to comment #17)
> LpSolit: Would you please be so kind and explain the reasons?

Because the trunk is frozen for two weeks already and no new enhancement bug will be accepted for 2.22 (we are only accepting bug fixes to stabilize the code). This bug is retargetted to 2.24 as it has a patch ready for review. Else this bug would have been retargetted to ---.

You know, I have some patches for enhancement bugs which have already been reviewed (r+) but which have now to wait till we unfreeze before landing. So I don't do this because it's you, I'm in the same situation than you! ;)

I also reassign the bug to you as you are actively working on it.

Comment 19

[Jochen Wiedmann]

(In reply to comment #18)

> I also reassign the bug to you as you are actively working on it.

Thanks, accepting.

Comment 20

[Teemu Mannermaa (:wicked)]

Comment on attachment 201118 [details] [diff] [review]
Patch for setting the "from" address in the envelope

I'll review this as soon as  2.22 freeze is over on the trunk.

Comment 21

[Marc Schumann [:Wurblzap]]

It seems to me that this might fix bug 318731. If so, then we should land this for 2.22. I'll take a closer look as soon as possible.

Comment 22

[Jochen Wiedmann]

Marc, I do indeed believe, that Bug 318731 is a duplicate. My patch is, IMO, a little bit more complete than yours, because it does also handle the "sendmail" method. On the other hand, you are carefully avoiding to override an existing MAILADDRESS environment variable. Whether that is good, seems to me to be a discussable subject. Personally, I think it is better to ignore the existing variable. If people have specified a "From" header, it is what they want to see.

Comment 23

[:glob ✱]

*** Bug 318731 has been marked as a duplicate of this bug. ***

Comment 24

[:glob ✱]

Comment on attachment 201118 [details] [diff] [review]
Patch for setting the "from" address in the envelope

while the patch has bitrot, manually applying (most of) the changes looks good for me, except there's now a different call for sendmail if running on windows, which should also pick up the -f flag.

Comment 25

[Jochen Wiedmann]

Attachment: Updated version of the patch

Updated patch, this time including the -f flag even for Windows.

Comment 26

[:glob ✱]

Comment on attachment 206570 [details] [diff] [review]
Updated version of the patch

r-

>+        if ($] >= 5.008) {
>+            # Use modern version to invoke sendmail. This avoids
>+            # command injection via $from.

we know it's windows, just quote it.

>+            my @args = (SENDMAIL_EXE, '-t', '-i');
>+            if ($from) {
>+                push(@args, '-f$from');
>+            }
>+            open(SENDMAIL, '|' . SENDMAIL_EXE . ' -t -i') ||
>+                die 'Failed to execute ' . SENDMAIL_EXE . ": $!\n";

you're not actually adding -f here ;)

open(SENDMAIL, '|' . SENDMAIL_EXE . qq# -t -i "-f$from"#) ||

>+            # If we do not know, we'd better assume it doesn't.
>+            # Perhaps, the best option would be to use Mail::Mailer
>+            # anyways, even on Unix/Linux? Saves about 20 lines of code.

mail::mailer's sendmail method won't work on windows, which is why it's treated differently.

Comment 27

[Jochen Wiedmann]

Attachment: Updated version of the updated version of the patch (this is getting SCO like renewed version of ... :-)

> we know it's windows, just quote it.

Understood. Changed.

> you're not actually adding -f here  ;) 

Ye gods ...

> mail::mailer's sendmail method won't work on windows, which is
> why it's treated differently.

Understood.

Comment 28

[:glob ✱]

Comment on attachment 206575 [details] [diff] [review]
Updated version of the updated version of the patch (this is getting SCO like renewed version of ... :-)

> this is getting SCO like renewed version of ...  

hehe :)

sorry to do this, but..

>+        if ($from) {
>+            # We're on Windows, thus no danger of command injection
>+            # via $from. In other words, it is safe to embed $from.
>+            $cmd .= " -f$from";
>+        }

i should have been clearer when i said "just quote it".

you still need to put quotes around $from as it may contain spaces.

  $cmd .= qq# -f"$from"#;

Comment 29

[Jochen Wiedmann]

Attachment: (Updated version of the)^3 patch

If you can, please feel free to apply required changes and pull the patch in. The thing that matters is to get it in.

Comment 30

[:glob ✱]

Comment on attachment 206576 [details] [diff] [review]
(Updated version of the)^3 patch

r=glob

:)

Comment 31

[Dave Miller [:justdave]]

er, I was basing that on the summary and severity of the bug.  Then someone pointed out the dupe, which has a much more severe issue, which isn't an enhancement, but a bugfix which takes the enhancement this bug was originally for along as a side-effect.  Yeehah.

Comment 32

[:glob ✱]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.60; previous revision: 1.59
done

Comment 33

[Ilya Slobodin]

Attachment: Patch for From address with trailing newline

The attached patch fixes Jochen Wiedmann's patch "Patch for setting From address in the envelope" in attachment 201118 [details] [diff] [review]. 

On my Windows system with default BUGZILLA-2_22rc1 installation $from variable contains "bugzilla-daemon\n" that leads to following SMTP conversation when $ENV{'MAILADDRESS'} is set to $from. 

The proposed patch removes traling newlines.

Tue 2006-04-18 15:43:13: <-- EHLO localhost.localdomain
...
Tue 2006-04-18 15:43:17: <-- MAIL FROM:<bugzilla-daemon @MYCOMP.MYDOMAIN>
Tue 2006-04-18 15:43:17: --> 553 This server does not accept routed mail
Tue 2006-04-18 15:43:17: <-- RCPT TO:<[e-mail]>
Tue 2006-04-18 15:43:17: --> 503 Unexpected command or sequence of commands
Tue 2006-04-18 15:43:17: <-- DATA
Tue 2006-04-18 15:43:17: --> 503 Unexpected command or sequence of commands
...

Comment 34

[Jochen Wiedmann]

I have no idea where the \n comes from? It would be interesting to investigate the source.

Besides, I have no problem with the patch, only that I would change the regexp to

    s/\s+//s

This should be both faster and safer.

Comment 35

[Teemu Mannermaa (:wicked)]

Comment on attachment 218821 [details] [diff] [review]
Patch for From address with trailing newline

Thanks for the patch! As we usually only deal with one patch per bug, I've moved this patch to bug 331365, which was opened to track this regression.