Closed Bug 1175839 Opened 9 years ago Closed 8 years ago

Reply does not correctly display senderaddress if senderaddress contains an open square bracket

Categories

(MailNews Core :: MIME, defect)

Product:
MailNews Core
Component:
MIME
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird45+ fixed, thunderbird46 fixed, thunderbird_esr3844+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 46.0
Tracking Flags:
Tracking Status
thunderbird45 + fixed
thunderbird46 --- fixed
thunderbird_esr38 44+ fixed

People

(Reporter: stefan.meissner, Assigned: jorgk-bmo)

References

Details

(Keywords: regression, Whiteboard: [regression:TB38.0])

Attachments

(1 file, 2 obsolete files)

Proposed solution (v1).
6.91 KB, patch
rkent
: review+
jorgk-bmo
: approval-comm-aurora+
rkent
: approval-comm-esr38+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

The from header contains no valid parenthesis with square brackets, e.g.:
From: "]User[ domain" <user@domain.tld>


Actual results:

If I reply to this there is an additional empty recipients in the To Field:
]User [ domain <user@domain.tld> <>

Furthermore there is an additional space directly ahead of the [.


Expected results:

The Response Address should be
]User[ domain <user@domain.tld>
Component: Untriaged → Message Compose Window
Confirmed. js-mime regression no doubt. (Works in 31).

I'll note that also, if you set your name in thunderbird to "]User[ domain" the From address will be 
"]User[ domain <you@example.com" 

yes, like that, the whole address and all inside quotes.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Component: Message Compose Window → MIME
Product: Thunderbird → MailNews Core
The problem is here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#489
} else if (opts.dliteral && ch == '[') {
  // Domain literals end the last token and start a new one.
  tokenIsEnding = true;
  tokenIsStarting = true;
  endQuote = ']';
(If you take this out, the problem goes away, also the problem mentioned in comment #1, but of course you can't just take it out, since it's here for a reason).

Basically the logic waits for a closing "]", but that never arrives.
Attached patch Is this the right approach? (obsolete) — — Splinter Review 
I wonder whether we should do the same for '('. And for '"'?
If this is the correct approach, I'll of course add a test.
Attachment #8632487 - Flags: feedback?(Pidgeot18)
(In reply to Jorg K from comment #3)
> I wonder whether we should do the same for '('.

It is already possible to reply to a message from |")User( domain" <sender@example.com>| and to send a message when your name is configured to |)User( domain|.
Waiting for feedback since July. Perhaps we can fix this for 45.
Flags: needinfo?(Pidgeot18)
How do you feel about looking at yet another JSmime regression? This has been sitting here since July.
Flags: needinfo?(rkent)
regression of which bug#?
Whiteboard: [regression:TB??]
You know, JSMime, that was landed in bug 959309.
thanks. let's make it so
Blocks: 959309
Whiteboard: [regression:TB??] → [regression:TB31.0]
Adjusting regression per comment 1
Flags: needinfo?(rkent)
Whiteboard: [regression:TB31.0] → [regression:TB38.0]
OK I spent a couple of hours looking at this, and did not get very far. But here are my comments.

First, your "If this is the correct approach, I'll of course add a test." is the wrong order. The first thing that I do, and that I tried to do here, is to create a test that fails. In this case, using your recommendation of the point of failure, I failed to create such a test.

That is, an address that looks like comment 0:

"]User[ domain" <user@domain.tld>

is correctly parsed by the current code. I added a test in test_header.js under 'parseAddressingHeader' like this:

      ["\"]User[ domain\" <user@domain.tld>",
        [{name: "]User[ domain", email: "user@domain.tld"}]],

Looking at the code itself, you are recommending that we modify line 489 in jsmime.js, which is under a section with this note: "If we reach this point, we're not inside of quoted strings ..."

But the original IS inside of a quoted string. I believe that you are reaching this point (but did not confirm) because at some point the address string is being converted into some other format, probably

]User[ domain <user@domain.tld>

That is not a valid string to parse, because the [] characters are special, and are required to be quoted.

For the alternate example of comment 1, which is easier to reproduce, I can confirm that the value presented to getTokenHeaders is of the format:

"]User Name[ domain <you@example.com"

That is, getTokenHeader is already seeing the entire thing quoted.

It is hard to say much more without a failing test that I can use to easily understand what is happening, which is why that is the place to start. But the evidence that I see is that the root cause of this is that the full addressing header value is at some point being converted into an unquoted format that is no longer in a form that is a valid RFC5322 header field, and then jsmime is not reasonably interpreting the invalid data. It's been awhile, but IIRC we encountered this same general issue in working on the more severe regressions from jsmime, and decided that the required changes were probably too extensive to risk adding to TB 38.

As for your specific change, given the above I would consider it likely to be a kludge for a specific issue that is actually caused elsewhere. As a kludge, it risks having unforeseen side effects. But given that, is hard to see how it is reasonable to get into a state where we see a "[", and start looking for a "]" that we can easily determine a priori is not there.

It would be great though, given that we have plenty of time before TB 45, to try to better understand the root issue.
Kent, many thanks for looking that this thoroughly.

(In reply to Kent James (:rkent) from comment #11)
> ]User[ domain <user@domain.tld>
> That is not a valid string to parse, because the [] characters are special,
> and are required to be quoted.
OK, then my patch is not a good idea ;-)

> It would be great though, given that we have plenty of time before TB 45, to
> try to better understand the root issue.
OK.

Let's just summarise the problem here:
In TB 31, configure an account with "Your Name" set to ]User[ domain (no quotes in the field).
Send an e-mail from that account. Resulting From header:
From: "]User[ domain" <xx@yy.com>

Now repeat with TB 38 or 45:
You get:
From: "]User [ domain <xx@yy.com>"
And that's invalid.

Now repeat quoted with "]User[ domain".
Result TB 31 is the same:
From: "]User[ domain" <xx@yy.com>
Result TB 38 or 45:
From: From: ]User[ domain <xx@yy.com>
Again, invalid.

So as you pointed out, the root issue is not in the parsing, but in the way the From field is constructed from the account information. TB 31 got it right, but TB 38-45 get it wrong.

I'll look into it.
Flags: needinfo?(Pidgeot18)
Attachment #8632487 - Attachment is obsolete: true
Attachment #8632487 - Flags: feedback?(Pidgeot18)
There are more things fishy here:

Type this into a from field:
]user[ domain <xx@yy.com>
E-mail gets send with
To: "]user [ domain <xx@yy.com>"

Now type "]user[ domain" <xx.yy.com> into the from field. You get an error:
"]user [ domain <xx.yy.com>" is not a valid e-mail address because it is not of the form user@host ...

So there is something wrong about how the name and e-mail are encoded into a header.
Damn, forgot the @ and didn't see it. OK, typing "]user[ domain" <xx@yy.com> also results in
To: "]user [ domain <xx@yy.com>"
Kent, not only you have looked into this for hours, so have I.

Don't read the previous comments, just read this one.

The problem is in MailServices.headerParser.makeMimeHeader.

If you want
  ]user[ domain
to get quoted, then we need to look at HeaderEmitter.prototype.addAddress and HeaderEmitter.prototype.addQuotable.

That figures out whether the text needs to be enclosed in quotes. The characters triggering the quoting are passed in, they are here:
  this.addPhrase(addr.name, ",()<>[]:;.\"", true);
The [] was *not* included, I added it. I did so in two places, the rest of the patch is debug in case you want to play with it.

The test cases are:
For the From address: Configure the account so "Your Name" is
 ]User[ Domain
(no quotes).

Send an e-mail to
 ]Kent[ James <kent@cc.com>
(no quotes).

With the patch applied, you'll get correct From and To headers.

To look at the patch, ignore all the debug, just look at the last two changes:
-    this.addPhrase(addr.name, ",()<>:;.\"", true);
+    this.addPhrase(addr.name, ",()<>[]:;.\"", true);

-      this.addPhrase(addr.name, ",()<>:;.\"", false);
+      this.addPhrase(addr.name, ",()<>[]:;.\"", false);

BTW, tests (mozilla/mach xpcshell-test mailnews/mime) still pass. I'd have to see where to add a test for this.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8697884 - Flags: feedback?(rkent)
OK, here is a patch you can review.
It has the change I talked about:
-    this.addPhrase(addr.name, ",()<>:;.\"", true);
+    this.addPhrase(addr.name, ",()<>[]:;.\"", true);

-      this.addPhrase(addr.name, ",()<>:;.\"", false);
+      this.addPhrase(addr.name, ",()<>[]:;.\"", false);

It adds tests to test_header_emitter.js:
+      [[{name: "]user[ domain", email: "user@d.com"}],
+        "\"]user[ domain\" <user@d.com>"],
+      [[{name: "Group", group: [{name: "]u[ d", email: "a@a.c"},
+                                {name: "]u[ c", email: "b@b.c"}]}],
+        "Group: \"]u[ d\" <a@a.c>,\r\n \"]u[ c\" <b@b.c>;"],

Also, it fixes a typo in jsmime.js and white-space problems in test_header.js. This fixes the sins I've committed before. I hope you approve.

I won't obsolete the other patch, in case you want to play around looking at debug.
Attachment #8697895 - Flags: review?(rkent)
Comment on attachment 8697895 [details] [diff] [review]
Proposed solution (v1).

Because this is a change in jsmime, we should really give jcranmer first crack at the review. If he is not able to get to this in a timely manner, then I can try to look at it.
Attachment #8697895 - Flags: review?(rkent) → review?(Pidgeot18)
Comment on attachment 8697895 [details] [diff] [review]
Proposed solution (v1).

Apart from a spelling mistake correction, correcting some white-space and adding a test, this is a *two-character-change*, adding "[]" to the string of special characters that will cause quoting to take place.
Attachment #8697895 - Flags: review?(rkent)
Comment on attachment 8697895 [details] [diff] [review]
Proposed solution (v1).

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

OK this makes sense to me, and seems to work fine. But this area is prone to regressions, so we'll be cautions. r+

Notes: the "phrase" in RFC 5322 is either an atom or a quoted-string, or an obs-phrase (will permits periods). So it seems to me that where a phrase is being added, we should also quote the full list of specials from 3.2.3. That would include the new [] as well as @ and \, and perhaps allow . for the obs-phrase.  I tested @ and \ in the username, and although I got some strange results, they were not as bad as the current bug, so it does not make sense to just add them here. But I do wonder why jsmime does not keep a formal list of specials, and use those as a constant throughout.

test_header.js has format corrections unrelated to this bug. Please do not add such changes in the future, if you want to do a bunch of formatting changes, do a separate patch. Otherwise I have to look carefully at each changed formatting to see if you intended that change to also have a real change in it.
Attachment #8697895 - Flags: review?(rkent)
Attachment #8697895 - Flags: review?(Pidgeot18)
Attachment #8697895 - Flags: review+
Attachment #8697884 - Flags: feedback?(rkent)
Comment on attachment 8697884 [details] [diff] [review]
Here is the proposed solution (last hunk) with lots of debug

This was for debugging and is not needed any more.
Attachment #8697884 - Attachment is obsolete: true
Thanks for the review.

(In reply to Kent James (:rkent) from comment #21)
> OK this makes sense to me, and seems to work fine. But this area is prone to
> regressions, so we'll be cautions. r+
What do you mean by "we'll be cautions"?

> test_header.js has format corrections unrelated to this bug. Please do not
> add such changes in the future, if you want to do a bunch of formatting
> changes, do a separate patch. Otherwise I have to look carefully at each
> changed formatting to see if you intended that change to also have a real
> change in it.
I mentioned that a few times. Anyway, I will do as you said next time.
Keywords: checkin-needed
Strange results indeed:
xx @ yy <xx@yy.com> gets changed to xx@yy <xx@yy.com> and
xx \ yy <xx@yy.com> gets changed to xx yy <xx@yy.com>.
Perhaps one day Joshua can enlighten us all on the issue.
Comment on attachment 8697895 [details] [diff] [review]
Proposed solution (v1).

[Approval Request Comment]
Regression caused by (bug #): JSMime, bug 959309.
User impact if declined: Users can't enter display name they might want.
Testing completed (on c-c, etc.): Unit test with test_header.js
Risk to taking this patch (and alternatives if risky):
Not risky, only added [] to the set of characters causing quoting.
Attachment #8697895 - Flags: approval-comm-aurora?
Attachment #8697895 - Flags: approval-comm-aurora? → approval-comm-aurora+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: