Closed Bug 1022723 Opened 10 years ago Closed 10 years ago

Some headers are sent prefixed with "********:" when they were folded due to their length

Categories

(MailNews Core :: MIME, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(thunderbird31 unaffected, thunderbird32 fixed, seamonkey2.28 unaffected, seamonkey2.29 affected, seamonkey2.30 affected)

RESOLVED FIXED
Thunderbird 33.0
Tracking Status
thunderbird31 --- unaffected
thunderbird32 --- fixed
seamonkey2.28 --- unaffected
seamonkey2.29 --- affected
seamonkey2.30 --- affected

People

(Reporter: infofrommozilla, Assigned: jcranmer)

References

Details

(Keywords: dogfood, regression, Whiteboard: [regression:TB32])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140605202143

Steps to reproduce:

Write a message (news or mail) with a long mail address as From.


Actual results:

The From header is scrambled (Stars are added).

Example: news post in mozilla.test (news.mozilla.org)
Message-ID: <kdKdnbcrY548RwjOnZ2dnUVZ_sqdnZ2d@mozilla.org>
(three lines)

From: *****:
 Alfred Peters
 <miteinere-mail-adresseindereinleitungszeilewirddieseoftzulang@geekmail.de>
Depends on: 790855
Regressions should block the originating bug report rather than depend on it.
Blocks: 790855
Component: Message Compose Window → MIME
No longer depends on: 790855
Keywords: regression
Product: Thunderbird → MailNews Core
Version: Trunk → 32
Whiteboard: [regression:TB32]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: papercut
Adjusting summary since bug 1025879, which was made a dupe of this is about the subject (so this does happen to subjects) & adding some more keywords to make this easier to find :-)
Summary: Some headers (From:, To: but not Subject:) are sent broken when they were folded, because of their length. → Some headers are sent prefixed with "********:" when they were folded due to their length
https://hg.mozilla.org/comm-central/rev/8877b30a38c3 adds:

    9.86 +  encodeMimePartIIStr_UTF8: function (aHeader, aStructured, aCharset,
    9.87 +      aFieldNameLen, aLineLength) {
...
    9.95 +    // Compute the encoding options. The way our API is structured in this
    9.96 +    // method is really horrendous and does not align with the way that JSMime
    9.97 +    // handles it. Instead, we'll need to create a fake header to take into
    9.98 +    // account the aFieldNameLen parameter.
    9.99 +    let fakeHeader = '*'.repeat(aFieldNameLen);
...
   9.108 +    // Add the text to the be encoded.
   9.109 +    emitter.addHeaderName(fakeHeader);
...
   9.127 +    // Compute the output. We need to strip off the fake prefix added earlier
   9.128 +    // and the extra CRLF at the end.
   9.129 +    emitter.finish(true);
   9.130 +    let value = handler.value;
   9.131 +    value = value.substring(value.indexOf(': ') + 2);
   9.132 +    return value.substring(0, value.length - 2);
   9.133 +  },

We're presumably not stripping off this fake header properly.
Flags: needinfo?(Pidgeot18)
Keywords: papercutdogfood
Allowing for my poor programming talents:
  9.132 +    return value.substring(0, value.length - 2);

Shouldn't this just be:

return value.substring(value, value.length);
Attached patch Fix the issueSplinter Review
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #8442068 - Flags: review?(irving)
Flags: needinfo?(Pidgeot18)
(In reply to Ed Morley [:edmorley UTC+0] from comment #4)
> Adjusting summary since bug 1025879, which was made a dupe of this is about
> the subject (so this does happen to subjects)

I had tested the subject header, but could not reproduce the error.

But actually, if the *first* *word* is more than 61 characters long, the error occurs there as well.
(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> Created attachment 8442068 [details] [diff] [review]
> Fix the issue

Yes, it works for me.
(In reply to Alfred Peters from comment #8)
> But actually, if the *first* *word* is more than 61 characters long, the
> error occurs there as well.

I guess "Donaudampfschifffahrtskaptiänsmützenbandstofffasermaterialeigenschaften neu geregelt" ist an orthodox and unlikely but still valid German email subject... ;-)
(In reply to Alfred Peters from comment #8)
> But actually, if the *first* *word* is more than 61 characters long, the
> error occurs there as well.

For me, it doesn't need to just be the first word - but either way, glad this is almost fixed :-)
Please can we either back out bug 790855 or get the fix landed today? Would you mind chasing the review up for this? This has been broken for 19 days now :-(
Flags: needinfo?(Pidgeot18)
Comment on attachment 8442068 [details] [diff] [review]
Fix the issue

Review of attachment 8442068 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. Since people have run with this, and I mostly understand what's going on here (modulo one of the tests), r+.

::: mailnews/compose/test/unit/test_attachment.js
@@ +73,5 @@
>    RFC2047: 'Content-Type: text/plain; charset=US-ASCII;\r\n'+
>             ' name="!\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~.txt"\r\n',
>  
>    RFC2047WithCRLF: 'Content-Type: text/plain; charset=US-ASCII;\r\n'+
> +                   ' name="!\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~.txt"\r\n',

What made this change? Not that it really matters...

::: mailnews/mime/src/mimeJSComponents.js
@@ +311,5 @@
>      // Compute the output. We need to strip off the fake prefix added earlier
>      // and the extra CRLF at the end.
>      emitter.finish(true);
>      let value = handler.value;
> +    value = value.replace(new RegExp(fakeHeader + ":\\s*"), "");

I'd probably have written this as value.replace(/^-*:\s*/, ""), but your way is fine (and arguably more readable).
Attachment #8442068 - Flags: review?(irving) → review+
(In reply to Ed Morley [:edmorley UTC+0] from comment #12)
> Please can we either back out bug 790855 or get the fix landed today?

Trunk is still closed due to bug 1027241 (bustage from m-c change).
(In reply to Jim Porter (:squib) from comment #13)
> ::: mailnews/compose/test/unit/test_attachment.js
> @@ +73,5 @@
> >    RFC2047: 'Content-Type: text/plain; charset=US-ASCII;\r\n'+
> >             ' name="!\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~.txt"\r\n',
> >  
> >    RFC2047WithCRLF: 'Content-Type: text/plain; charset=US-ASCII;\r\n'+
> > +                   ' name="!\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~.txt"\r\n',
> 
> What made this change? Not that it really matters...

I'm not totally sure. To be honest, we really shouldn't be doing anything other than RFC 2231 encoding, which means half of test_attachment.js is a messy test that needs to be culled.

(In reply to Ed Morley [:edmorley UTC+0] from comment #12)
> Please can we either back out bug 790855 or get the fix landed today? Would
> you mind chasing the review up for this? This has been broken for 19 days
> now :-(

I could land it today, but it wouldn't due any good thanks to a Mr. Brian Smith.
Flags: needinfo?(Pidgeot18)
Please can this land, iven that it's a regression and now that the builds are no longer burning? (albeit still not entirely green...!)
https://hg.mozilla.org/comm-central/rev/dd53e637e5e4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Comment on attachment 8442068 [details] [diff] [review]
Fix the issue

[Approval Request Comment]
Regression caused by (bug #): bug 790855
User impact if declined: Annoying *****s in subjects
Testing completed (on c-c, etc.): The test code checks an actual malformed header in automated tests. Currently baking on nightlies.
Risk to taking this patch (and alternatives if risky): Well, at worst, this patch ends up doing nothing and replaces the *****s with -----s so it doesn't look like we got censored :-)
Attachment #8442068 - Flags: approval-comm-aurora?
Comment on attachment 8442068 [details] [diff] [review]
Fix the issue

This now needs to be uplifted to beta as well.
Attachment #8442068 - Flags: approval-comm-beta?
Comment on attachment 8442068 [details] [diff] [review]
Fix the issue

a=Standard8
Attachment #8442068 - Flags: approval-comm-beta?
Attachment #8442068 - Flags: approval-comm-beta+
Attachment #8442068 - Flags: approval-comm-aurora?
You need to log in before you can comment on or make changes to this bug.