Don't apply the second patch. Comments:

In message <20130617183108.GC7600@runtux.com>,
Ralf Schlatterbeck writes:
>On Mon, Jun 17, 2013 at 02:01:05PM -0400, John P. Rouillard wrote:
>> >
>> >Anything that should be in the current release?
>>
>> Can somebody comment on whether the new patch I added to:
>>
>>    http://issues.roundup-tracker.org/issue2550747
>>
>> looks reasonable? I used the original requestors patch as a base and
>> updated it with a bit of error detection etc. If it looks ok, I'll
>> commit it.
>>
>> It would be nice if we could get through the backlog of tickets with
>> patches since people contributed them and this is a good way to get
>> additional contributers.
>
>Did you test this?

Yeah minimally. I had a mock and tested the output of the mock with a
real sendmail (not postgres/qmail in sendmail emulation
mode). However,[q I just checked my test logs and realized that the
test used the -t flag for sendmail which is not what my patch
implements 8-(. Which explains how my tests passed.

>To me it looks like the sending only works for a single recipient,
>sendmail (according to my manpage for the sendmail command coming with
>postfix) wants each recipient as a separate command-line parameter. Not
>a comma separated list. Note that postfix' sendmail doesn't have that
>weird exitcode.

Exit code 75 may exit only in a true sendmail(tm).

Does the -t flag work with postfix's sendmail interface?

   -t     Read  message  for recipients.  To:, Cc:, and Bcc: lines will be
          scanned for recipient addresses.  The Bcc: line will be  deleted
          before transmission.

>The stderr= setting of Popen also looks wrong to me (we don't read back
>from a pipe to sendmail).

Well my patch certainly does if there is an error exit.

+            print >>mailer.stdin, message
+            mailer.stdin.close()
+            if mailer.wait():
+                if mailer.returncode != 75:
+                    mailer_err = mailer.stderr.read(2048)
+                    raise MessageSendError("Error: couldn't send email: "
+                          "%s returned %s - %s"%(sendmail,
+                                    mailer.returncode, mailer_err))

>So the correct call would probably be
>
>args = [sendmail] + to
>mailer = subprocess.Popen(args, stdin=subprocess.PIPE)

At least it should be a sanitized set of space separated addresses (or
more properly an array of addresses since the shell would have split
the space separated addresses into separate elements of argv[]).

Also despite using popen, those addresses should never be exposed to
an external process (and possible shell) without sanitizing them. At
the very least shell metachars |, `, & should be considered to make
sure they won't trick popen into starting a shell.

>maybe we should add a test before committing this?

Sigh yeah. Well there's another timesink.

>Also note that one reason people wanted that feature is that sendmail
>doesn't require authorisation -- but roundup *can* be configured with a
>user and password for smtp authentication, so a normal authenticated
>service of the provider *can* be used.

>What do you think?

I still think it's a useful patch. Especially when you don't want to
embed passwords for your upstream email provider in roundup. Roundup
has a larger attack surface than a binary dedicated to schlepping
email around (or it should).

Using sendmail locally like this means that you get messages queued
and retried automatically. AFAIK roundup will never try to resend an
email that fails because the smtp server is down, inaccessible etc.

But yes this patch is crap, good thing I don't work as a developer.
I won't be committing it.