195965 - custom headers omit \r

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[James Nord]

the implementation I did for multiple custom headers introduced a bug as the
headers are terminated with \n and not \r\n

Sendmail & postfix seem to handle this gracefully (even though its wrong) but
qmail aparantly refuse to accept the mail (see URL)

Comment 1

[James Nord]

Attachment: Patch to add \r\n to custom headers

Comment 2

[James Nord]

Need r  s/r Please

Nominating for 1.3 as is such a trivial fix and some ppl are unable to send mail
with custom headers without this

Comment 3

[James Nord]

mail headers should be separated by CRLF but in creating custom headers they are
separated by only LF

Test case: from news

setup custom headers in prefs.js
  user_pref("mail.compose.other.header", "X-James-Was-Here");
Compose a message and set the custom header
Send the mail though a QMail MTA

Actual results

"An error occured while sending mail. The mail server responded: See
http://pobox.com/~djb/docs/smtplf.html.

. Please check the message and try again."

This is a typical qmail complaint about a bare LF in the message.

(Works ok against postfix/EXIM/Sendmail)

Comment 4

[Sean Su]

Comment on attachment 116256 [details] [diff] [review]
Patch to add \r\n to custom headers

r=ssu for the trivial fix.

Comment 5

[James Nord]

Risks: (for blocking)

Only tested on WinXP against a EXIM server (no access to anything else)
Worst case could break custom headers (see bug 16925) for mail/news.

Code is not entered unless custom headers are used.
Risks of not patching Mail with custom headers is in violation of RFCs :-(

Comment 6

[Maurice Galland]

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

Comment 7

[(not reading, please use [e-mail] instead)]

actually, I don't think \r\n or \r should be in the js here, it should be a fix
to  nsMsgCompUtils.cpp, to call PUSH_NEWLINE().

Comment 8

[(not reading, please use [e-mail] instead)]

hmm, maybe not.  as that would not allow for multiple "custom header" lines.

I'll test and land the fix.

Comment 9

[(not reading, please use [e-mail] instead)]

fix checked in, re-assign to [e-mail] for credit.

Comment 10

[(not reading, please use [e-mail] instead)]

fixed during 1.4 alpha.

thanks for the patch, James.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 116256 [details] [diff] [review]
Patch to add \r\n to custom headers

sr noted.

when I checked in, I also added comments to the .js and C++ for why CRLF is
necessary.

nice work, James.

Comment 12

[James Nord]

Thanks

/James

Comment 13

[James Nord]

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