242693 - Smarter handling (even just a parsing error!) of semicolon-separated addresses in To: field

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[A S]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

One line summary: You should allow semi-colons as a valid address separator. 

Using Thunderbird 0.6, when attempting to use an Exchange 2000 Server as my SMTP
server, if I type multiple addresses separated by semi-colons, I get an error
that says: 

"An error occured while sending mail.  The mail server responded: Relaying not
allowed.  Please verify that your e-mail address is correct in your Mail
preferences."  

This would seem to be an Exchange configuration issue, but relaying IS allowed
from any IP on this network.   I dig further: if I remove the semi-colons and
replace them with commas, and it works fine.  

Reproducible: Always
Steps to Reproduce:
1. Configure Thunderbird to use Exchange 2000 as an outgoing SMTP server. 
2. Click "Write"
3. Type multiple addresses separated by semi-colons
4. Click send
Actual Results:  
mail will not send

Expected Results:  
realized the semi-colons were separators.  if not, warn me that semi-colons are
not allowed or could cause problems.  

I have only tested this with one Exchange server, as its all I have access to.

Comment 1

[A S]

Attachment: showing error

Comment 2

[Mike Cowperthwaite]

(In reply to comment #0)
> One line summary: You should allow semi-colons as a valid address separator. 

Semi-colons in fact are not legal separators.

The semicolon has a specific use, defined in RFC2822, as a group terminator.
This looks like;
   groupname: [e-mail], [e-mail] ;     <- semicolon ends group
You'll see this often in the form:
   Undisclosed-Recipients:;

Mozilla does not support group addr's well, if at all -- bug 83521, bug 110605.
See also bug 94676.


> Using Thunderbird 0.6, when attempting to use an Exchange 2000 Server as my
> SMTP server, if I type multiple addresses separated by semi-colons, I get an
> error that says: 
> 
> "An error occured while sending mail.  The mail server responded: Relaying not
> allowed.  Please verify that your e-mail address is correct in your Mail
> preferences."  

See bug 254000 for alternate errors reported from other servers.

Also see what happens if you simply Send Later, then look at how the message is 
addressed in the Unsent Messages folder.


> Expected Results:  
> warn me that semi-colons are not allowed or could cause problems.

This is a very reasonable expectation.  Mozilla should already know the 
addresses are a problem before the SMTP server is contacted, and the user should 
be informed.  Repurposing the bug for this request.

See also bug 258653, bug 210521.

Comment 3

[Mike Cowperthwaite]

Here are results of the various ways using of semi-colons as separators.  In 
each case, I enter a string in the address field, hit Send Later, then copy the 
To: header from the message source in Unsent Messages.  Same results in
TB 1.0+0222 and Moz 1.8b-0120 (Win2K)

To: [e-mail]; [e-mail]
 is parsed into
To:  "                 <- yes, just a sole double-quote
                          displays as  ""  in the envelope

To: [e-mail]; B <[e-mail]>
 is parsed into
To: "[e-mail]; B" <[e-mail]>

To: A <[e-mail]>; [e-mail]
 is parsed into
To: "A ; [e-mail]" <[e-mail]>

To: A <[e-mail]>; B <[e-mail]>
 is parsed into
To: "A <[e-mail]>; B" <[e-mail]>

The first case is the one that will result in actual errors when sent to the 
SMTP server; the other parse results are legal if unexpected.  Unfortunately, 
the first case is also the most likely to occur, e.g. in a bogus URL.

Comment 4

[Mike Cowperthwaite]

*** Bug 270615 has been marked as a duplicate of this bug. ***

Comment 5

[Chitu Okoli]

(In reply to comment #2)
> Semi-colons in fact are not legal separators.
> 

If semi-colons are not "legal" separators, then there should be an option to override the default. Numerous programs that generate e-mail lists use semi-colons to separate them--this is a constant frustration for me.

Interestingly, Outlook uses semi-colons as the default separator, and has an option to permit commas as well. I think Thunderbird/Mozilla should do the opposite: use commas as the default (the legal spec?) and give an option to also permit semi-colons (this option should default to "on"). This makes Thunderbird more interoperable with many resources that go by the Outlook default, whether we like it or not.

Comment 6

[Jan Pridal]

I've come across the semicolon vs. comma issue in the mailto: link while
implementing our product. I tried to find a format that would be parsed
successfully by widespread mail clients - Outlook, Outlook Express,
Thunderbird, etc.
As far as I am aware the Outlook does not (at least by default) support the comma
separator for multiple recipients and thus does not follow the RFC. But
when I replace the commas with semicolons it is Thunderbird that fails
(and probably other Mozilla based mail clients).

Comment 7

[Magnus Melin [:mkmelin]]

*** Bug 254000 has been marked as a duplicate of this bug. ***

Comment 9

[Viliam Bur]

Happened to me too. The worst thing about this bug is that there is no feedback (while sending) that anything went wrong! I write a long mail, copy a long prepared list of e-mail addresses from the other file, click "Send", message is sent without problems, I go happily to sleep. Only three days later people start complaining to me that they received no e-mail. When I look into "Sent items", I see that the list of recipients is "".

Why did I use ";" as a mail address separator? Well, on my computer this is a list separator as specified in regional settings. Many other programs (like Excel) require me to use it; so I used it without thinking in Thunderbird too. And the program behaved exactly as if I provided a valid list, so there was nothing to raise my suspicion.

I guess the quickest was to fix it would be to display a warning window if the e-mail address list contains semicolon. Though I am not sure why the program evaluated the list of recipients to be "", and then sent it anyway; perhaps this should cause an error message.

Please please please do something about it, because for user this is very unpleasant. A mistake easy to be made, and no feedback; the mails are not sent, but the user completely believes they were.

Comment 11

[Earl Wallace]

My biggest concern is that the average user will not know that their email was not sent successfully. They never got an error. The reason they use an email program is to send and receive mail.  If they believe Thunderbird can not be trusted to send mail they will stop using it.

In my case, the SMTP server that Thunderbird contacted, in which to send the message, did not flag the addresses as an error, gave no error message to Thunderbird (?), but dropped the message, but Thunderbird thought all was well and gave no error message itself.

The user saw just saw his message was sent, and he only needed to look in the Sent folder for confirmation.  If the user went to the Sent folder and looked at the message, he would have seen something like this:

   Subject: Enclosed is my latest article, hope it's under the deadline!
      From: [e-mail]
        To: ""
      Date: 3/18/2009 3:10 PM
  X-Mozilla-Status: 0001
 X-Mozilla-Status2: 00800000
    ...

Notice the empty To field?  How is the average user supposed to know this?  If Thunderbird is going to go after the average Joe or Jane, it has to report errors because your average user is not any good at reading RFCs.

Thunderbird can do a quick fix right now - after sending the message, go into the Sent folder and look at what you sent and if the To field is bogus, flag it as an error to the user so they can at least troubleshoot it.

Comment 12

[A S]

Seriously, how is this even an issue after five years? It is the epitome of open-source idiocy to force your users to alter totally expected and otherwise trained behavior so your *frontend* follows an RFC.  

Thunderbird users are making mistakes.  Nobody cares about the technical docs - that's for software to handle.  The front end, the UI, it should be able to spot user mistakes and just fix them.   

My recommendation, as the one who open this bug five years ago is to do one of the following: 

* warn the user via an alert dialog, when a semi-colon is spotted, that it is not a valid separator.  It could do this on blur or on send OR
* transparently change semi-colons to commas in the to: field on blur or on send OR
* just accept semi-colons as a valid separator in the software, and pass it to the SMTP server with commas 
* least acceptably, just do not send the message. 

Blanking the To: field seems to be the least acceptable outcome of all.  If I relied on Thunderbird and it did this to me, I would immediately ditch it with a grudge, thinking the developers clearly were not coding for me as a user.    

This is dead simple to fix and one of your team agreed in 2005, so the fact that it's open perplexes me.  Why anyone would use the RFC as an argument not to properly capture user behavior is beyond me.  C'mon -- just *fix* it.

Comment 13

[A S]

Sorry for the second email, but on re-read that sounds really harsh.  Obviously, I am a Thunderbird user for several years. It just seems like a simple fix for a potentially big problem.   

Thanks for your time and hard work.  But please, let's knock out simple issues like this one.

Comment 14

[Earl Wallace]

I know you have lots of bugs to fix, and that few engineers like working on bugs - they rather work on the new stuff instead.  You guys and gals are doing the world a big favor by helping us wean away from that Outlook-drug we have been taking all these years :-)  I love what you are doing!

As we go around and convince others to switch over, we have to be realistic and admit a lot of our "users" are not technically oriented.  They trust the software to do the thinking for them, like Adam said.  Whatever it takes to send and receive mail reliability has to take priority.  You can not control which SMTP or POP servers you talk to, and you can not assume those servers were written to an RFC standard.

If my users completely and totally believe that Thunderbird will always send and receive mail without fail, they'll stick with it.  The reason one of my users switched is because Outlook sometimes would fail to send some email they had (they managed to insert some weird characters into their mail for some reason). They are very sensitive about sending and receiving email.

Comment 15

[Wayne Mery (:wsmwk)]

cc: exchange dudes and bryan for opinions on changing behavior and/or erroring out (in part because the wanted? has not been addressed)

twto other semi-colon bugs
bug 210521 
bug 258653

Comment 16

[Jae-Seong Lee-Russo]

Attachment: Replace semicolons with commas

I appreciate any feedback on this patch and the following.  If the patch is fine, I am thinking of doing the same thing for Bug 210521 & Bug 258653.

SetCC & SetTo in nsMSgCompFields.cpp will pass the replaced string (semicolns are replaced with commas) to SetUnicodeHeader.

Comment 17

[Magnus Melin [:mkmelin]]

Thanks for working on this. 

From a quick look that doesn't look correct though. Things like
To: mygroup:;, [e-mail], othergroup:[e-mail], [e-mail]; 

are valid syntax. I assume that would break with that patch. 

So you can't blindly replace semicolons with commas. I think it would be ok if the address isn't in a group (starting with "<label>:" ).

Comment 18

[Mark Banner (:standard8)]

This patch doesn't seem to be in the right place. If the error is with  reformatUnquotedAddresses not following the spec, then it should be fixed. If, however, the error is when we're formulating the items to go into the address field then we should be correcting that formatting.

Comment 19

[Jae-Seong Lee-Russo]

Attachment: msg_parse_Header_addresses: add "Group Addresses" support, add ';' as a delimeter

Comment 20

[Jae-Seong Lee-Russo]

Attachment: test cases

$ make SOLO_FILE="test_splitRecipients.js" -C mailnews/compose/test/ check-one
make: Entering directory `/c/mozilla-build/comm-central-devel/obj-i686-pc-mingw3
2/mailnews/compose/test'
/c/mozilla-build/python25/python -u /c/mozilla-build/comm-central-devel/mozilla/
config/pythonpath.py \
          -I/c/mozilla-build/comm-central-devel/mozilla/build \
          /c/mozilla-build/comm-central-devel/mozilla/testing/xpcshell/runxpcshe
lltests.py \
          --symbols-path=../../../mozilla/dist/crashreporter-symbols \
          --test-path=test_splitRecipients.js \
          ../../../mozilla/dist/bin/xpcshell \
          ../../../mozilla/_tests/xpcshell/test_compose/unit
TEST-PASS | c:\mozilla-build\comm-central-devel\obj-i686-pc-mingw32\mozilla\_tes
ts\xpcshell\test_compose\unit\test_splitRecipients.js | test passed
INFO | Result summary:
INFO | Passed: 1
INFO | Failed: 0

Comment 21

[Magnus Melin [:mkmelin]]

Please request review for the patch and testcase. standard8 might be a suitable reviewer for it.

Comment 22

[Jae-Seong Lee-Russo]

Attachment: msg_parse_Header_addresses: add "Group Addresses" support, add ';' as a delimeter, 1

Comment 23

[Jae-Seong Lee-Russo]

Attachment: test cases

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 408986 [details] [diff] [review]
test cases

The tests are looking good. Although I think you need a couple of more cases:

Undisclosed recipients: [e-mail] ;;extra:;

Undisclosed recipients:;;extra:[e-mail];

The second one passes but the first one doesn't afaict.

Whilst these are slightly obscure, they are just a variant on what you are trying to support.

Could you also use foo.invalid for the cases you're adding (or something similar) rather than invalid.com as per bug 533314?

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 408985 [details] [diff] [review]
msg_parse_Header_addresses: add "Group Addresses" support, add ';' as a delimeter, 1

Sorry for the delay in getting to these.

>+    // Find the fist non-empty group address or a non-group address

nit: First

>+    const char *line_temp = line_end;
>+    while (*line_temp
>+           && !(*line_temp == ',' && paren_depth <= 0
>+                && (!mailbox_start || mailbox_end))
>+           && !(*line_temp == ';' && paren_depth <= 0
>+                && (!mailbox_start || mailbox_end)))

Can you put the operators at the end of lines please e.g.

while (*line_temp &&
       !(*line_temp == ',' ...

>+    /* Skip over extra whitespace or commas between addresses. */
>+    while (*line_end && (isspace(*line_end) || *line_end == ','))
>+      NEXT_CHAR(line_end);

Just like you did above, please use IS_SPACE here.

>+    if(!*line_end)
>+      break;

nit: space before (

I still need to think about the logic a bit more, but if you can do the update to this and the test case first, that would be good.

Comment 26

[Jae-Seong Lee-Russo]

Attachment: fix bug in comment 24, add change per comment 25

Comment 27

[Jae-Seong Lee-Russo]

Attachment: add test cases in comment 24 [Checkin: Comment 31]

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 419950 [details] [diff] [review]
fix bug in comment 24, add change per comment 25

>-  /* Skip over extra whitespace or commas before addresses.
>-   */
>-  while (*line_end && (IS_SPACE(*line_end) || *line_end == ','))
>+  /* Skip over extra whitespace, commas or semicolons before addresses. */
>+  while (*line_end && (isspace(*line_end) || *line_end == ',' ||
>+                       *line_end == ';'))
>     NEXT_CHAR(line_end);

This should definitely be IS_SPACE to account for non-ASCII characters.

r=Standard8 with that fixed.

Comment 29

[Jae-Seong Lee-Russo]

Attachment: comment 28 [Checkin: Comment 30]

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 424565 [details] [diff] [review]
comment 28
[Checkin: Comment 30]


http://hg.mozilla.org/comm-central/rev/81ba8ae8671d

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 419951 [details] [diff] [review]
add test cases in comment 24
[Checkin: Comment 31]


http://hg.mozilla.org/comm-central/rev/544cf79ddf8c

Comment 32

[Thomas D. (:thomas8)]

Can someone say what the outcome of this bug was?

I don't know anything about group addresses, but when I enter semicolon-separated single addresses into one recipient row, then press Enter, I think that should be parsed by TB into single addresses, or not?

[e-mail]; [e-mail]

current result: nothing
expected result: two single addresses in two recipient rows

Comment 33

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #32)
> [e-mail]; [e-mail]
> current result: nothing

the two addresses just stay in one row

> expected result: two single addresses in two recipient rows

TB 24.0a1 (2013-06-21) on WinXP

Comment 34

[Thomas D. (:thomas8)]

Oh, we expand semicolon-separated addresses only *when sending*...
So is that by design, to educate users that they should not use ; for separating single addresses because that's illegal?

Comment 35

[Mitar]

What? Why only when sending? How can one edit the list before sending?

Comment 36

[Steuard Jensen]

This seems to have regressed in Thunderbird 31, according to bug 1059988. (See that bug for details.)

Comment 37

[toni]

Hi dears, 
sorry for coming back. I noticed that for somehow reason Gmail accounts can't be added to Thunderbird.
Therefore it's compulsory to download and install the last version.
I noticed that the feature about the row separation has not been yet re-added. This is the reason I am still on 24.6.0 version.
Now Gmail will force me to upgrade to the version 31 or laterhttps://support.mozilla.org/en-US/kb/thunderbird-and-gmail

Using several times multiple, recipient I need the feature working back again like v. 24.

Can you please help me ??

Comment 38

[Thomas D. (:thomas8)]

[OT]

(In reply to toni from comment #37)
> Can you please help me ??

Toni, please note that bugzilla.mozilla.org is NOT a support forum. Google for "Thunderbird Support" will take you here:
https://support.mozilla.org/en-US/products/thunderbird
-> https://support.mozilla.org/en-US/kb/get-community-support
-> https://support.mozilla.org/en-US/questions/new

Please use the links above to ask for help with your personal problems when using Thunderbird.

Also please do not cross-post the same message in several bugs.

Anje and I have assisted Toni in bug 1059988. Apparently his problem is NOT about semicolons, but about copying blank lines between addresses from Word. This was handled gracefully by earlier versions of TB, but unfortunately fails now (which I consider a bug). So for now, until TB becomes smarter: All you need to do, Toni, is to just remove any empty lines from your list of recipients in Word *before* copying, as I explained to you in detail in Bug 1059988 comment 27.

But as I said, for any further support questions or complaints, please turn to the support forums (links above).

Comment 39

[toni]

Thank you Thomas.
Sorry about but this doesn't make sens to me.
What should I ask in the support forum IF the issue depends on a TB 26.6 late version bug ?

Comment 40

[Thomas D. (:thomas8)]

(In reply to toni from comment #39)
> Thank you Thomas.
> Sorry about but this doesn't make sens to me.
> What should I ask in the support forum IF the issue depends on a TB 26.6
> late version bug ?

You had mentioned a number of issues involving Gmail and Yahoo which appeared to be support issues rather than bugs.

It's certainly arguable whether not parsing blank lines within your Word recipient list (copied into TB) is a bug or a missing feature which we'd like to have - if you feed TB with a correct list of comma-separated recipients, you'll succeed. If you are failing to remove the blank lines from your word document as we've advised you, that's another support issue which doesn't belong here.

Not parsing semicolons as address separators is definitely NOT a bug (not an RFC separator, worse semicolon explicitly used for RFC group syntax), but we also agree we'd like to see that as a convenience feature in view of malformed Outlook tradition. Afasics, all bugs/RFE's are on record, so everyone is aware of that; we've also tried to get some traction here, and starting points for analysis have been provided by Jörg. We're busy preparing for the TB60 release, so atm there's nothing more we can do here unless if you can provide patches yourself. Any insights where to start in our 7 million lines of code or how to fix this are welcome. Further lamenting and advocacy comments on this rather minor issue will only distract or make angry the very developers whom we need to look into this and thus make the desired changes less likely to happen.