Closed Bug 1166206 Opened 4 years ago Closed 4 years ago

Display-name with comma in it does not get properly quoted in From: field in Tb38.0b5..

Categories

(Thunderbird :: Message Compose Window, defect)

38 Branch
defect
Not set

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed)

RESOLVED FIXED
Thunderbird 41.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird41 --- fixed

People

(Reporter: neomjp, Assigned: rkent)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

1. thunderbird -ProfileManager [-no-remote] # create a new profile.
2. A popup "Welcome to Thunderbird, Would you like a new email address?" appears. Select "Skip this and use my existing email".
3. A popup "Mail Account Setup" appears. Create a new account with "Your name" of the form Familyname, Personalname (Note a comma between them, but without quotes).
This should be recorded in prefs.js as
user_pref("mail.identity.id1.fullName", "Familyname, Personalname");
4. Compose a new E-mail and save it.
5. Go to Drafts folder, select the draft message, and View > Message Source (Ctrl+U).



Actual results:

In the header of the message source, the From: field is

From: Familyname , Personalname <email@example.com>

This is interpreted as two addresses: "Familyname" and "Personalname <email@example.com>", and probably fails.


Expected results:

The display-name in From: field should be quoted.

From: "Familyname, Personalname" <email@example.com>

According to RFC5322, display-name can be a quoted string with a comma in it.

3.6.2.  Originator Fields
3.4.  Address Specification
3.2.5.  Miscellaneous Tokens
3.2.4.  Quoted Strings

This bug occurred in 
thunderbird-38.0b5
thunderbird-38.0b2
thunderbird-38.0b1
but not in 
thunderbird-37.0b1
so this is an regression.
A simple workaround is to set
Your name: "Familyname, Personalname"
user_pref("mail.identity.id1.fullName", "\"Familyname, Personalname\"");

But any users of thunderbird < 38 who had a comma in mail.identity.id?.fullName may get affected.
Blocks: TB38found
I can confirm this bug.
TB31 stores |Form: "Surname, Firstname" <mail@mail.com>|
TB38 (beta 6) stores |From: Surname , Firstname <mail@mail.com>|

In the message pane message header this is displayed as:
Surname <>, 1 more, the "1 more" can be expanded.

When sent, the message is received with the From header as stated above.
When replied to, this creates a reply to two recipients.

Another observation: If you type |xx, yy <xx@yy.com>| into a To field and save the message, this is correctly stored as |To: "xx, yy <xx@yy.com>|. So this works.

Maybe this is the last JSMIME-related bug that Kent was predicting in last Tuesday's meeting.

I suggest to fix it before TB38 ships, since it's pretty bad to break people's existing configuration. It can't be hard to identify the place where the identity's full name is placed into the From field and add the missing quotes.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rkent)
Flags: needinfo?(Pidgeot18)
Keywords: regression
Sorry, missing quote:
Another observation: If you type |xx, yy <xx@yy.com>| into a To field and save the message, this is correctly stored as |To: "xx, yy" <xx@yy.com>|. So this works.

Sounds a little like bug 1130248 where the following processing added the missing quotes:
-  fields.to = addressNode.getAttribute("fullAddress");
+  let addresses = MailServices.headerParser.makeFromDisplayAddress(
+    addressNode.getAttribute("fullAddress"), {});
+  fields.to = MailServices.headerParser.makeMimeHeader(addresses, 1);
It's done here:

https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3228
let address = MailServices.headerParser.makeMailboxObject(identity.fullName, identity.email).toString();
let item = menulist.appendItem(identity.identityName, address, account.incomingServer.prettyName);
Flags: needinfo?(rkent)
Attachment #8609886 - Flags: review?(rkent)
I also tried with a rcf2047 encoded name. I tried "Kaß, Jörg".
Unfixed: From: =?UTF-8?Q?Ka=c3=9f?= , =?UTF-8?B?SsO2cmc=?= <xx@yy.com>
Fixed: From: =?UTF-8?B?S2HDnywgSsO2cmc=?= <xx@yy.com>
The fixed one is correct.
Don't add quotes if there are quotes already.
Maybe this is still too simplistic and there is a better solution.
Attachment #8609886 - Attachment is obsolete: true
Attachment #8609886 - Flags: review?(rkent)
Attachment #8609892 - Flags: review?(rkent)
(In reply to Jorg K from comment #7)
> Created attachment 8609892 [details] [diff] [review]
> This fixes the problem, but perhaps there is a more elegant solution
> (revised)
> 
> Don't add quotes if there are quotes already.
> Maybe this is still too simplistic and there is a better solution.

Jorg, I think you might have just advanced to "Blackbelt" hacking status, but are you taking the same track here as Magnus is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1039443 I didn't actually look at the code :)
Thanks for taking this on, I know it breaks many webmail clients.
Joe: Thanks for the hint.

In my patch I do:
+      let fullName = identity.fullName;
+      if (fullName.indexOf(",") >= 0 && !fullName.startsWith("\"") && !fullName.endsWith("\"")) {
+        fullName = "\"" + fullName + "\"";
+      }
       let address = MailServices.headerParser.makeMailboxObject(
-        identity.fullName, identity.email).toString();
+        fullName, identity.email).toString();  <=== *toString()* used here.

In the proposed patch in the other bug 1039443, attachment 8456802 [details] [diff] [review], Magnus does something similar, but in the toString function:
-  toString: function () {
-    return this.name ? this.name + " <" + this.email + ">" : this.email;
+  toString: function (aQuote) {
+    if (!this.name)
+      return this.email;
+    // Quote name if requested, display name contains comma, and it's not
+    // already quoted.
+    let name = this.name;
+    if (aQuote && name.contains(",") &&
+        !(name[0] == '"' && name[name.length - 1] == '"'))
+      name = "\"" + name + "\"";
+    return name + " <" + this.email + ">";

I don't care either way, but we really should get this fixed.

We could use Magnus' approach and call toString(true) here.
The modified toString() could then be useful in other contexts as well.
Flags: needinfo?(mkmelin+mozilla)
I've come to the conclusion that adding the quotes in here is the wrong approach.
The quotes should be added where the From header is constructed, most likely in JSMIME.

I did this as an example:
User display name: |"xx, yy" zz|

Without any patch, that gets processed semi-correctly to a From header |From: "xx, yy zz" <whatever@whatever.com>|
So the string passed in is already processed. This processing should be fixed.

If we add quotes to |"xx, yy" zz|, so we pass |""xx, yy" zz"|, we confuse the existing logic and the resulting From header becomes |From:  xx , yy zz <whatever@whatever.com>|.
Flags: needinfo?(mkmelin+mozilla) → needinfo?(rkent)
Attachment #8609892 - Attachment is obsolete: true
Attachment #8609892 - Flags: review?(rkent)
I typed the following into the To fields:
K, J <xx@yy.com>
Kaß, Jörg <xx@yy.com>
"xx, yy" zz <xx@yy.com>
"K, J" <xx@yy.com>
"Kaß, Jörg" <xx@yy.com>

The resulting To headers:
To: "K, J" <xx@yy.com>, =?UTF-8?B?S2HDnywgSsO2cmc=?= <xx@yy.com>,
 "\"xx, yy\" zz" <xx@yy.com>, "K, J" <xx@yy.com>, =?UTF-8?B?S2HDnywgSsO2cmc=?=
 <xx@yy.com>

So as far as I can see, the MIME-encoding works as it should for the To header.
The question is: Why is it wrong for the From header? Why is processing for From and To different?
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1039443
Flags: needinfo?(rkent)
Here's my patch, using the same approach as in bug 1130248

What do you think, Jorg?
Flags: needinfo?(mozilla)
Perfect! I knew you would find the spot in five minutes. Thank you.

I noticed that Joshua added the call to makeFromDisplayAddress in addressingWidgetOverlay.js:
https://hg.mozilla.org/comm-central/annotate/97bc9fb598e1/mail/components/compose/content/addressingWidgetOverlay.js

He overlooked this spot here and the other one already fixed in bug 1130248.

Are you expecting another jsmime-related problem or are we done now? ;-)
Assignee: nobody → rkent
Flags: needinfo?(mozilla)
Flags: needinfo?(Pidgeot18)
Attachment #8609981 - Attachment is obsolete: true
Comment on attachment 8610347 [details] [diff] [review]
use makeFromDisplayAddress when setting From

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

Great that there exist the needed functions, we just need to use them up at the right places.
Attachment #8610347 - Flags: review?(mkmelin+mozilla)
Attachment #8610347 - Flags: review?(Pidgeot18)
Seamonkey compose code has the same "msgCompFields.from = GetMsgIdentityElement().value;" construct so this change will also be needed there.
Status: NEW → ASSIGNED
Attachment #8610347 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8610347 [details] [diff] [review]
use makeFromDisplayAddress when setting From

Checked in https://hg.mozilla.org/comm-central/rev/bae6809204cc

[Approval Request Comment]

Bad regression from Thunderbird 31
Attachment #8610347 - Flags: review?(Pidgeot18)
Attachment #8610347 - Flags: approval-comm-beta?
Attachment #8610347 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Blocks: 1170002
Comment on attachment 8610347 [details] [diff] [review]
use makeFromDisplayAddress when setting From

[Triage Comment]

https://hg.mozilla.org/releases/comm-esr38/rev/53f5c79a92f3
https://hg.mozilla.org/releases/comm-beta/rev/7a23f1e4d093
http://hg.mozilla.org/releases/comm-aurora/rev/6a665353e26f
Attachment #8610347 - Flags: approval-comm-esr38+
Attachment #8610347 - Flags: approval-comm-beta?
Attachment #8610347 - Flags: approval-comm-beta+
Attachment #8610347 - Flags: approval-comm-aurora?
Attachment #8610347 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.