160631 - bug_email.pl is broken

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

Last week the fixes for bug_email got merged, but it appears that noone actually
tested it afterwards...

We now chdir('..') at teh top of the script, _AND_ do so before calling processmail.

This is broken on trunk, 2.16, and 2.14.3. 'oops'

Patch coming.

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: fix

Comment 2

[Gervase Markham [:gerv]]

This is what we get for waiting until the last minute to do stuff like this. Who
actually did that merge?

The fix looks fine, but it would be good to get it reviewed by someone who
actually _uses_ this stuff.

Gerv

Comment 3

[Bradley Baetz (:bbaetz)]

justdave, IIRC. I was on holiday at the time, and did my patch on teh assuption
that the other one wasn't going it.

As to who uses it, b.gnome.org is it, I believe. Noone has complained about this
yet, so....

Comment 4

[Erik Anderson]

The script was broken before, and the bug report was opened very late in the 
development cycle.  The security patch got in, which was the important part; I 
was scared that anyone who managed to get the script working wouldn't have seen 
it (I didn't) and started running with such a large hole.

In the meantime, let's get everything together and make this thing really rock 
(if that's possible)

Comment 5

[Bradley Baetz (:bbaetz)]

Erik - if you use this, then you may as well review it. I'll then check it in
onto all the branches, although theres no guarentee that there will ever be a
2.14.4 release.

Comment 6

[Erik Anderson]

I'll take a look at it later.  I don't have too many opportunities to work on 
it, and my local copy of the script is fairly munged from exploration on the 
last bug.

Comment 7

[Jussi Sirpoma]

I'm using an almost identical fix myself and it does work. The only difference
is that I changed the bugzilla_email_append.pl also. There is the line

system("cd .. ; ./processmail $found_id '$SenderShort'");

and it needs to be change to

system("./processmail $found_id '$SenderShort'");

Comment 8

[Bradley Baetz (:bbaetz)]

jussi: Oh. I filed a separate bug on that, and ccd you

Comment 9

[Erik Anderson]

Please don't send interpolated strings to system().  Could someone create a 
patch to send both scripts into taint mode?

Comment 10

[Bradley Baetz (:bbaetz)]

Erik: I've got a separate patch to fix that now, in a different bug.

The plan is to rewrite the email stuff for 2.18 - currently, its really ugly,
and broken. When the rewrite happens, it will be done in taint mode.

Note that stuff in contrib isn't supported, and may have bugs, security or
otherwise.

Comment 11

[Joel Peshkin]

Comment on attachment 93677 [details] [diff] [review]
fix

This now is restored to working order, so it certainly is a change that should
go into CVS for contrib, but the bug_email still needs a lot of work.

Specifically...
1) The shebang points to the wrong perl.

2) Instructions should cover non-procmail users...
/etc/mail/aliases:
bug-tip: "| bug_email"
/etc/smrsh/bug_email:
cd BUGZILLADIR
sudo -u bugzilla BUGZILLADIR/bug_email.pl   
/etc/sudoers:
mailnull ALL=(bugzilla) NOPASSWD: BUGZILLADIR/contrib/bug_email.pl

Also, bug_email doesn't handle email sent in plan+html format well and should
really handle attachments.

None of this takes away from the fact that this patch needs to go in, though.

When the code is rewritten, it should probably also use FindBin to locate the
correct directory.  Then, it will not be necessary to cd before invoking.

Comment 12

[Bradley Baetz (:bbaetz)]

Yeah - see the various meta bugs on this + related issues

checked in on trunk, 2.16, and 2.14 branches

Not that there will probably ever be a 2.14.4, but hey... (the 2.14 branch
needed to be patched with -l (ignore whitespace), because of tabs)