286760 - email address with leading/trailing whitespace in Address book, Account manager, or Composition [[e-mail] ] displays wrongly with added quotes when composing ["foo"@bar.com], causing lost mails and malformed "duplicate" AB entry

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; es-AR; rv:1.7.6) Gecko/20050226 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.7.5) Gecko/20041219

When you finish a mail adress with a space in adress book, and later you try to
 send a mail to this adress, mozilla will try to send the mail to an incorrect
mail adress and the mail will be lost.

Example: correo electrónico:(Mail adress:) <[e-mail] >. Later I try
to send a mail to [e-mail] clicking in the adress book and I'll get: 
Para:(To:) <"example"@example.com> and, obviously, [e-mail] won't
receive the mail.

Reproducible: Always

Steps to Reproduce:
1.Go to "Libreta de direcciones/Nueva targeta" (Adress book/New card) and fill
it with a mail adress ending with a space.
2.Try to send a mail to this adress. 


Actual Results:  
The mail adress that will appear in the "To: " field will not match and the mail
will be lost.

Expected Results:  
The mail should be sent correctly.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050317] (nightly) (W98SE)

Confirmed.

Comment 2

[[e-mail]]

I created an addresss book card for <[e-mail] > and
sent it from my office PC and although when composing it displayed as
"neil.parkwaycc.co.uk"@myrealbox.com it arrived successfully.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> I created an addresss book card for <[e-mail] > and
> sent it from my office PC and although when composing it displayed as
> "neil.parkwaycc.co.uk"@myrealbox.com it arrived successfully.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050317] (nightly) (W98SE)

Same here: I had only tested the UI part :-|
Morphing: only the address bar gets it wrong ... the mail is received with
correct address (I checked ViewSource) !

Reporter,
Can you confirm if the mail does get lost in FireFox/1.0.1 (or Mozilla/1.7.5) ?

Comment 4

[simon annear]

confirmed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b) Gecko/20050302

This does not appear to be a dataloss issue (at least not with my mail server)
but a UI issue

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050317] (nightly) (W98SE)
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041217] (release) (W98SE)
[Mozilla Thunderbird, version 1.0+ (20050319)] (nightly) (W98SE)

MASv1.7.x branch and TBv1.0+ have the same issue.

No dataloss, but it could depend on the SMTP server !?

Anyway, the sent message is affected too:

Sent:
{{
To: SergeG <"gautheri"@noos.fr>
}}

Received:
{{
To: SergeG <[e-mail]>
}}

Suggestion:
Trim the ending space(s) when saving the card !!?

Comment 6

[Serge Gautherie (:sgautherie)]

[Netscape® Communicator 4.8 : en-20020722] (W98SE)

Nv4 keeps the ending space in the card,
but composes and sends the mail without any double quote.

Workaround:
obviously, remove the ending space.

Comment 7

[[e-mail]]

Reporter: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.7.5) Gecko/20041219.

I think the mail arrives or not depending on the destination mail server. I have
2 mail accounts: one hotmail and one gmail. I have tried to send a message to
this 2 accounts and while it has arrived to the hotmail account, it hasn't
arrived to the gmail account.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to comment #7)
> Reporter: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.7.5) Gecko/20041219.
> 
> I think the mail arrives or not depending on the destination mail server.

This could very well be the case.  Two other data points.
1. what does address looks like after saving the mail as a draft or template?
2. UI is definitely a factor - if I ctrl-M using the "spaced" address book
entry, TO: of the mail composition does NOT have a blank - either address book
or compose removed it. But if address book entry is typed into To: line, then
the blank does appear.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> *** Bug 342376 has been marked as a duplicate of this bug. ***

In that bug:
*"rejected as unknown by Exchange": confirming dataloss depending on SMTP server.
*"LDIF file": to remember to check that we fix both cases.

Comment 12

[Mark Banner (:standard8)]

I think I want this on at least the wanted radar for TB 3.

I think it is the http://mxr.mozilla.org/seamonkey/source/mailnews/mime/public/nsIMsgHeaderParser.idl that is really at fault here, and could probably be easily tested by extending the existing test http://mxr.mozilla.org/seamonkey/source/mailnews/mime/test/unit/test_nsIMsgHeaderParser1.js

Additionally, I'm fairly sure the appropriate RFC says that whitespace is allowed at the end of the address, so the parser should be able to cope with it properly (if that's what is at fault).

Comment 13

[Carlos]

Same error using TB 2.0.0.14 (20080421)

Sending SMTP mail with one space char under pop3 user config, receiving mails works ok. 

MAIL command looks like this

SMTP Send: MAIL FROM:<"foo"@foo.com>

Comment 15

[David Humphrey (:humph)]

Mikey is going to give this one a shot as a student project.  Any suggestions who he should talk to about getting started?

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> Mikey is going to give this one a shot as a student project.  Any suggestions
> who he should talk to about getting started?

I'm probably best, but comment 12 already points at some good places to look. You could just "fix" the email input on the cards, but there are existing cards and manual input when composing emails to think about as well.

Comment 17

[Mark Banner (:standard8)]

Expanded the title because this can also happen in compose window & account manager (hence comment 12 pointing to fixing the core issue should fix most cases).

Comment 19

[Wayne Mery (:wsmwk)]

non-perfect query for some related bugs https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwords&short_desc=&product=MailNews+Core&product=Thunderbird&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&resolution=---&emailreporter1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=short_desc&type0-0-0=anywordssubstr&value0-0-0=address+email+e-mail&field1-0-0=short_desc&type1-0-0=anywords&value1-0-0=space+spaces+&field1-0-1=short_desc&type1-0-1=substring&value1-0-1=pars

Comment 20

[mikey.osd600]

Attachment: A while() statement is added to remove trailing spaces.

I added some code at the beginning of the msg_quote_phrase_or_addr() function to remove the additional spaces given in the address. I extended the test_nsIHeaderParser1.js to show the successful results. However, from reading comment 12, I am not too sure if this is allowable due to the RFC?

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 413868 [details] [diff] [review]
A while() statement is added to remove trailing spaces.

>diff --git a/mailnews/mime/src/nsMsgHeaderParser.cpp b/mailnews/mime/src/nsMsgHeaderParser.cpp

>+  int temp_length = length - 1;
>+  
>   /* If the entire address is quoted, fall out now. */
>   if (address[0] == '\"' && address[length - 1] == '\"')
>      return length;

temp_length should be a PRInt32 (as per new_length & full_length). It can be created & initialised after this if statement as it isn't needed until afterwards.

nit: please use the // style of comments rather than /* ... */

>+  /* Remove trailing spaces. */
>+  while(addr_p && address[temp_length] == ' '){
>+      address[temp_length--] = '\0';
>+      full_length--;
>+  }

The addr_p check doesn't need to be part of the loop as the loop never changes it.

nit: please use 2-space indentation.

nit: space after while i.e. while (

nit: please put the { on a new line as per the rest of the file.

Comment 22

[mikey.osd600]

Attachment: Updated patch with the corrections

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 416382 [details] [diff] [review]
Updated patch with the corrections

Sorry for the delay in getting back to reviewing this.

>+  // Remove trailing spaces.
>+  while (address[temp_length] == ' ')

I've just realised that this should be while (IS_SPACE(address[temp_length]) - the IS_SPACE function will correctly take care of any non-ascii values if we happen to have any.

r/sr=Standard8 with that fixed.

Comment 25

[Wayne Mery (:wsmwk)]

mikey, are you up for finishing the patch and setting it for checkin?

Comment 27

[Thomas D. (:thomas8)]

Will the pending patch for this take care of the following problems from duplicate bugs?

bug 689495 - quotes added when email address contains a point (.)
bug 780411 - brackets <[e-mail]> should be removed from email address field inputs (just like whitespace, this bug)

Comment 28

[Thomas D. (:thomas8)]

Similar: Bug 208018

Comment 29

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #25)
> mikey, are you up for finishing the patch and setting it for checkin?

It looks like Mikey is gone.
What's wrong with our procedures that a finished patch with review+ and superreview+ which just needs a single word added (see comment 23) is left to rot along for months and years?

Comment 30

[Mark Banner (:standard8)]

Unfortunately, we can't always track everything, and sometimes we do miss things as in this case. The good news is that the patch is simple and there is a unit test, so we should be able to land this reasonably easily.

Comment 31

[:aceman]

Probably also needs refresh of the PRInt32/PRBool now.

Comment 33

[Thomas D. (:thomas8)]

(In reply to Mark Banner (:standard8) from comment #30)
> The good news is that the patch is simple and there
> is a unit test, so we should be able to land this reasonably easily.

That's great news indeed! :) Thanks.

Current patch only fixes trailing spaces. Summary says "leading OR trailing spaces" - perhaps it's simple to add fix for leading spaces, too?

(Bug 208018 mentions that spaces *inside* the "real address" part <foo@bar .com> are also causing trouble...)

Comment 35

[Thomas D. (:thomas8)]

fyi

Code lives here:
http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMsgHeaderParser.cpp

Link of interest:
http://en.wikipedia.org/wiki/E-mail_address#Valid_email_addresses

<"much.more unusual"@example.com> is actually valid;
so this should be valid, too:
<" much.more unusual"@example.com>

But I think our case here is different because unless user actually entered quoted address into address book or whereever, we can safely assume that leading and trailing spaces (probably inner spaces, too) are just typos and should be removed. In fact, we should validate stricter and remove unprotected spaces earlier when entering addresses in AB, to avoid invalid addresses in AB to begin with.

Comment 36

[Thomas D. (:thomas8)]

Does the patch also handle this case?

Display: Firstname Lastname
email: [[e-mail] ] <- brackets added for clarity only

-> wrongly resurfaces in compose window as:
Firstname Lastname <"trailspace"@duellmann24.net >

Note the added quotes AND trailing space in recipient address, as opposed to the scenario without display name

Comment 37

[Mark Banner (:standard8)]

(In reply to Thomas D. from comment #36)
> Does the patch also handle this case?
> 
> Display: Firstname Lastname
> email: [[e-mail] ] <- brackets added for clarity only

Yes, you can see that from the additional tests in the test_nsIMsgHeaderParser1.js file.

Comment 38

[Mark Banner (:standard8)]

Attachment: Updated patch to fix bitrot

I've updated the patch to the latest code base. I was trying to fix the leading case as well, but I guess I got stuck on that a while ago, and then forgot about this again. So lets land this trailing space fix and we'll split out the leading space to another bug.

Comment 39

[Mark Banner (:standard8)]

Leading space case is now bug 853437.

Comment 40

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/comm-central/rev/9db520b03784

Thanks to mikey.osd600 for the patch.

Comment 41

[Mark Banner (:standard8)]

Unfortunately I had to back this out, as it seemed to have introduced a new crash (from the unit test results):

https://hg.mozilla.org/comm-central/rev/debe6d38b0a2

Crash reason:  SIGSEGV
Crash address: 0xb4c00000

Thread 0 (crashed)
 0  libxul.so!msg_format_Header_addresses [nsMsgHeaderParser.cpp:9db520b03784 : 1329 + 0x8]
    eip = 0x01eabe01   esp = 0xbfe75cc0   ebp = 0xbfe75d08   ebx = 0x02d15794
    esi = 0xb4bb0f04   edi = 0xb4c00000   eax = 0xb4b4d33c   ecx = 0xfff4d33a
    edx = 0x0000000f   efl = 0x00210282
    Found by: given as instruction pointer in context
 1  libxul.so!nsMsgHeaderParser::RemoveDuplicateAddresses(nsACString_internal const&, nsACString_internal const&, nsACString_internal&) [nsMsgHeaderParser.cpp:9db520b03784 : 1509 + 0x11]
    eip = 0x01eac4d0   esp = 0xbfe75d10   ebp = 0xbfe75da8
    Found by: previous frame's frame pointer
 2  libxul.so + 0x12d2097
    eip = 0x0218c098   esp = 0xbfe75db0   ebp = 0xbfe75dd8
    Found by: previous frame's frame pointer
 3  libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [XPCWrappedNative.cpp:a73a2b5c423b : 3086 + 0x15]
    eip = 0x01a41c75   esp = 0xbfe75de0   ebp = 0xbfe76058
    Found by: previous frame's frame pointer
 4  libxul.so!XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [XPCWrappedNativeJSOps.cpp:a73a2b5c423b : 1417 + 0x9]
    eip = 0x01a45169   esp = 0xbfe76060   ebp = 0xbfe76118
    Found by: previous frame's frame pointer
 5  libxul.so!js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) [jscntxtinlines.h:a73a2b5c423b : 338 + 0x15]
    eip = 0x0254ccc6   esp = 0xbfe76120   ebp = 0xbfe761e8
    Found by: previous frame's frame pointer
 6  libxul.so!js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [jsinterp.cpp:a73a2b5c423b : 2396 + 0x23]
    eip = 0x0254012f   esp = 0xbfe761f0   ebp = 0xbfe76638
    Found by: previous frame's frame pointer
 7  libxul.so!js::RunScript(JSContext*, js::StackFrame*) [jsinterp.cpp:a73a2b5c423b : 341 + 0x13]
    eip = 0x0254c4a2   esp = 0xbfe76640   ebp = 0xbfe767d8
    Found by: previous frame's frame pointer
 8  libxul.so!js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) [jsinterp.cpp:a73a2b5c423b : 531 + 0xb]
    eip = 0x0254daae   esp = 0xbfe767e0   ebp = 0xbfe76898

Comment 42

[Thomas D. (:thomas8)]

I'm also not sure how if patch of attachment 727672 [details] [diff] [review] does the right thing to fix this problem.

Can somebody say when the patch code gets called?
- immediately after entering an address into composition?
- or only "after-the-fact" when sending?

To fix this properly, I think the following is required:
- After users enters email address with (leading) or trailing whitespace into any of Address book, Account manager, or Composition, when that input box loses focus or the address is otherwise confirmed, (leading) and trailing whitespace should be immediately removed
- So that to me looks like something which probably should be handled by the UI using Javascript code to trim that data immediately, or not?

The point is that invalid addresses should never get stored into AB or account manager in the first place, and they shouldn't survive in composition either. Invalid spaces should be removed by the UI as soon as possible.

Comment 43

[Mark Banner (:standard8)]

(In reply to Thomas D. from comment #42)
> I'm also not sure how if patch of attachment 727672 [details] [diff] [review]
> [review] does the right thing to fix this problem.

The issue is the code handling the processing and formatting of email addresses which tries to do it according to the relevant RFCs but doesn't succeed in all cases.

> Can somebody say when the patch code gets called?
> - immediately after entering an address into composition?
> - or only "after-the-fact" when sending?

You should easily be able to trace that through via mxr and other tools, it gets called at various points depending on what you're actually doing.

> The point is that invalid addresses should never get stored into AB or
> account manager in the first place, and they shouldn't survive in
> composition either. Invalid spaces should be removed by the UI as soon as
> possible.

That's a secondary/different bug. Whilst that would help the issue, it wouldn't help those with already broken address books, or those with address books we can't alter (e.g. OSX or LDAP address book), nor does it stop people accidentally typing a space at the end of the compose code (and if they put multiple addresses on one line, we'd still need to be able to rely on this code).

We can do what you suggest as additional tidy up, but we're simply not going to be able to catch every case, so fixing the code that actually handles the email addresses and parses them and puts them back together in pretty format is the right place to really fix this issue.

Comment 44

[Thomas D. (:thomas8)]

^^ Completing the trio:

Bug 286760 (this bug) - trailing spaces in email address
Bug 853437 - leading spaces in email address
Bug 208018 - spaces in the middle of email address

Comment 45

[donald]

> > The point is that invalid addresses should never get stored into AB or
> > account manager in the first place, and they shouldn't survive in
> > composition either. Invalid spaces should be removed by the UI as soon as
> > possible.
> 
> That's a secondary/different bug. Whilst that would help the issue, it wouldn't
> help those with already broken address books, or those with address books we 
> can't alter (e.g. OSX or LDAP address book), nor does it stop people
> accidentally typing a space at the end of the compose code (and if they put
> multiple addresses on one line, we'd still need to be able to rely on this
> code).

I think that fixing this when creating contacts in thunderbird address books would solve the issue for many of us.  I create and manage all our company emails through Thunderbird address books and extensions.

Extra leading /trailing spaces are always an issue.  Self created due to copy/paste but something TB should easily be able to strip when saving contacts.

Comment 47

[Mark Banner (:standard8)]

Adding bug 858337 to the deps - that is working on a javascript implementation, which should make fixing this much easier (if it is still an issue).

Comment 48

[:Hb]

Allowing leading or trailing spaces in Address Books fields is bad design.

Comment 49

[Thomas D. (:thomas8)]

More than 8 years since this bug was filed and TB still cannot handle a little bit of trailing whitespace in an email address, while it's very easy for users to add such because we don't sufficiently validate email addresses in our own UI and we can't always validate from other sources like external ABs.

The result is dataloss for the user (per comment 7, you might be lucky with some servers, but that doesn't help the unlucky). More annoyingly, TB will add an extra new contact with wrong email address "foo"@bar.com into its AB, per current bug summary (set in comment 35). 

Ridiculous, bad and long-standing bugs like this undermine UX-trust into TB as an application.
Imo, they also violate Mozilla's promise of ensuring "on-going security and stability" for TB [1].
This should be fixed by a Mozilla developer asap.

[1] https://wiki.mozilla.org/Thunderbird/New_Release_and_Governance_Model

Comment 50

[Wayne Mery (:wsmwk)]

While I don't dispute the dataloss (at the receiver), many dataloss bugs by definition break trust and "should be fixed asap", but we all know it's not possible. And methinks it is possible to overstate the case 
- mitigating factors of comment 12, plus the sender has an outgoing copy of the mail should it need to be resent. 
- we have many dataloss bugs which fit your description but whose effects are far more reaching or devastating, and also many that are older [1]. 

Your description gives me the impression you believe the effects are terrible in the extereme. If that is true then the bug merits some tracking status, If not,  should be sufficient to just markup the bug with the facts and keywords and let the developer ecosystem take over.

[1] https://bugzilla.mozilla.org/buglist.cgi?f1=OP&order=Bug%20Number&o3=anywordssubstr&list_id=8816310&bug_severity=blocker&bug_severity=critical&bug_severity=major&v3=dataloss&resolution=---&classification=Client%20Software&classification=Components&o2=anywordssubstr&f4=CP&query_format=advanced&j1=OR&f3=keywords&f2=status_whiteboard&v2=dataloss&product=MailNews%20Core&product=Thunderbird

Comment 51

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #50)
> While I don't dispute the dataloss (at the receiver), many dataloss bugs by
> definition break trust and "should be fixed asap", but we all know it's not
> possible. And methinks it is possible to overstate the case 
> - mitigating factors of comment 12, plus the sender has an outgoing copy of
> the mail should it need to be resent.

I don't see much mitigation in comment 12, because even if trailing spaces were allowed for email addresses per RFCs for sending, that does not help this bug because the problem is TB converting the trailing spaces into a fragile rubbish address with quotes in the local part which were not there before.

> - we have many dataloss bugs which fit your description but whose effects
> are far more reaching or devastating, and also many that are older [1].

+1
 
> Your description gives me the impression you believe the effects are
> terrible in the extereme.

Sending mail to wrangled recipients is pretty bad for a mailer, and we have to start somewhere and dry up the cesspool of bad bugs. Imo this is a good start because it shouldn't be too hard (albeit with TB's code, you never know). If we ask developers to fix the really hard bugs, they will say it's too much work or they don't know where to start.

> If that is true then the bug merits some tracking
> status, If not,  should be sufficient to just markup the bug with the facts
> and keywords and let the developer ecosystem take over.

I have long stopped believing into the developer ecosystem of Thunderbird, especially because that ecosystem has never been sufficient in numbers/working hours to cope with the massive number and severity of TB/mailnews bugs and RFE's (10000+).

> [1]
> https://bugzilla.mozilla.org/buglist.
> cgi?f1=OP&order=Bug%20Number&o3=anywordssubstr&list_id=8816310&bug_severity=b
> locker&bug_severity=critical&bug_severity=major&v3=dataloss&resolution=---
> &classification=Client%20Software&classification=Components&o2=anywordssubstr
> &f4=CP&query_format=advanced&j1=OR&f3=keywords&f2=status_whiteboard&v2=datalo
> ss&product=MailNews%20Core&product=Thunderbird

What I find particularly annoying is that this bug had already landed and instead of investigating Mark's crash report of Comment 41 and finding out how this little patch could possibly cause the crashes described there, Comment 47 puts this bug on the backburner again to wait until all of TB's header wheels have been re-invented in bug 858337, which in turn waits for other bugs and so on. It's just too frustrating after all the wait-time since 2005 that this bug has already gone through.
And yeah, we have to start somewhere, and if Mozilla are really interested in "ongoing security and stability" for TB, imho they should do more.

Comment 52

[Thomas D. (:thomas8)]

And if Thunderbird or its users haven't died, they shall live unhappily everafter waiting for all those bugs to be fixed...

Comment 53

[Tony Mechelynck [:tonymec]]

(In reply to Thomas D. from comment #52)
> And if Thunderbird or its users haven't died, they shall live unhappily
> everafter waiting for all those bugs to be fixed...

Please!

See https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and in particular §§ 1.1 and 1.2.

If you think this bug takes too long to be fixed, then you can help: first fix bug 858337 (which blocks this bug) yourself, then continue with this bug. FWIW, fixing a bug means proposing a patch, having it reviewed by an owner or peer of the appropriate module, modifying the patch until it gets r+, then incorporating it into the code (by setting the checkin-needed keyword if you don't have push privileges to the Mozilla repositories).

If you don't have the necessary expertise, then please consider that the people who do are probably overworked (e.g. with other bugs). Many of the Mozilla developers (including some of the best) do it in their free time (e.g. after they come back home from college), which doesn't help much qua manpower.

Comment 55

[Magnus Melin [:mkmelin]]

Attachment: bug286760_addr_trailing_space.patch

Indeed, this is a tiny fix with jsmime.

Comment 56

[Magnus Melin [:mkmelin]]

Hm, I wonder if instead of trim(), we should remove all spaces in the address, to cater for cases like bug 208018. But that's more of an invalid input case.

Comment 57

[Magnus Melin [:mkmelin]]

Having thought about it some more, I think trim() is good at this part of the code. (Especially as lists can contain spaces in the description)

Comment 58

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #56)
> Hm, I wonder if instead of trim(), we should remove all spaces in the
> address, to cater for cases like bug 208018. But that's more of an invalid
> input case.

We need to be very careful about removing inner spaces from recipient addresses, they are valid even in the local part of an email address if quoted ("foo bar"@asdf.com is valid per RFCs, see bug 941861).
And I see no reason why we should remove extra inner spaces in display name, so the only part where we might safely remove them is the domain name if there are no RFCs which allow them.

Comment 59

[Thomas D. (:thomas8)]

Maybe the old design tried to accept user inputs in AB of email addresses with leading spaces in local part by auto-quoting them and thus converting them into technically valid email addresses (" foo"@bar.com).
So this bug also changes the UI/UX so that deliberately entering ´ [e-mail]` (leading space without quotes) will no longer work to save an email address *with* leading spaces in local part because we trim it after the fact. I think that's fine, as long as we allow user to enter " foo"@bar.com (leading space in quotes).
We should also clarify this new behaviour of our UI by doing immediate validation of new addresses (related bug 631206).
For trailing spaces after domain part, I think we always got it wrong before this bug, and it should be fine after this bug.
For inner spaces, probably bug 941861.

Comment 60

[Joshua Cranmer [:jcranmer]]

The way we handle email lists right now in the compose window makes me sad. Once I land bug 998191, we should be able to swap out our code to using groups instead of the hackish pseudo-email bullshit we pull now.

(In reply to Thomas D. from comment #58)
> (In reply to Magnus Melin from comment #56)
> > Hm, I wonder if instead of trim(), we should remove all spaces in the
> > address, to cater for cases like bug 208018. But that's more of an invalid
> > input case.
> 
> We need to be very careful about removing inner spaces from recipient
> addresses, they are valid even in the local part of an email address if
> quoted ("foo bar"@asdf.com is valid per RFCs, see bug 941861).

My basic running rule has been to quote the localpart if it needs to be quoted and keep it unquoted if it can be as a definition of "valid" email address--e.g., »foo [e-mail]« would be invalid but »"foo bar"@asdf.com« would be valid. I started writing up <https://github.com/jcranmer/jsmime/blob/emailutils/emailutils.js> to generate correct utilities, but ran into issues with checking validity of domain names (Firefox's new URL() doesn't validate the domain name, and ... polyfilling IDN has proved to be damn near impossible because I've found contradictions in the spec and testsuite I can't resolve).

Comment 61

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8530562 [details] [diff] [review]
bug286760_addr_trailing_space.patch

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

I guess a better solution would be to use canonicalize(email) from my jsmime work-in-progress. But it's a work-in-progress with no known ETA, so this is good enough for now.

Comment 62

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/3e883ab44c92 -> FIXED