Closed
Bug 1166206
Opened 9 years ago
Closed 9 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)
Tracking
(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed)
RESOLVED
FIXED
Thunderbird 41.0
People
(Reporter: neomjp, Assigned: rkent)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
rkent
:
approval-comm-aurora+
rkent
:
approval-comm-beta+
rkent
:
approval-comm-esr38+
|
Details | Diff | Splinter Review |
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.
Comment 2•9 years ago
|
||
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
Updated•9 years ago
|
status-thunderbird38:
--- → affected
status-thunderbird39:
--- → affected
status-thunderbird40:
--- → affected
status-thunderbird41:
--- → affected
tracking-thunderbird38:
--- → ?
Comment 3•9 years ago
|
||
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);
Comment 4•9 years ago
|
||
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);
Comment 5•9 years ago
|
||
Flags: needinfo?(rkent)
Attachment #8609886 -
Flags: review?(rkent)
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
(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.
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8609892 -
Attachment is obsolete: true
Attachment #8609892 -
Flags: review?(rkent)
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(rkent)
Assignee | ||
Comment 12•9 years ago
|
||
Here's my patch, using the same approach as in bug 1130248
What do you think, Jorg?
Flags: needinfo?(mozilla)
Comment 13•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8609981 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
Seamonkey compose code has the same "msgCompFields.from = GetMsgIdentityElement().value;" construct so this change will also be needed there.
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8610347 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 16•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Assignee | ||
Comment 17•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•