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

RESOLVED FIXED in Thunderbird 3.1b1

Status

MailNews Core
Composition
RESOLVED FIXED
14 years ago
2 years ago

People

(Reporter: A S, Assigned: Jae-Seong Lee-Russo)

Tracking

(Depends on: 1 bug)

Trunk
Thunderbird 3.1b1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 147728 [details]
showing error

Comment 2

13 years ago
(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: addr@xx.tld, addr@yy.tld ;     <- 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.
Assignee: mscott → sspitzer
Status: UNCONFIRMED → NEW
Component: Preferences → MailNews: Composition
Ever confirmed: true
OS: Windows XP → All
Product: Thunderbird → Core
Hardware: PC → All
Summary: Relay not allowed with semi-colons → Smarter handling (even just a parsing error!) of semicolon-separated addresses in To: field
Version: unspecified → Trunk

Comment 3

13 years ago
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: a@xxx.tld; b@xxx.tld
 is parsed into
To:  "                 <- yes, just a sole double-quote
                          displays as  ""  in the envelope

To: a@xxx.tld; B <b@xxx.tld>
 is parsed into
To: "a@xxx.tld; B" <b@xxx.tld>

To: A <a@xxx.tld>; b@xxx.tld
 is parsed into
To: "A ; b@xxx.tld" <a@xxx.tld>

To: A <a@tld.xx>; B <b@tld.xxx>
 is parsed into
To: "A <a@tld.xx>; B" <b@tld.xxx>

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

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

Comment 5

12 years ago
(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.
Assignee: sspitzer → nobody

Comment 6

12 years ago
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

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

Updated

10 years ago
Flags: wanted-thunderbird3?

Updated

10 years ago
QA Contact: composition
Product: Core → MailNews Core

Updated

9 years ago
Duplicate of this bug: 457873

Comment 9

9 years ago
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.
Duplicate of this bug: 484109

Comment 11

9 years ago
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: Me@whyIwriteIdontknow.com
        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.
(Reporter)

Comment 12

9 years ago
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.
(Reporter)

Comment 13

9 years ago
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

9 years ago
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

9 years ago
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
(Assignee)

Comment 16

8 years ago
Created attachment 408336 [details] [diff] [review]
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

8 years ago
Thanks for working on this. 

From a quick look that doesn't look correct though. Things like
To: mygroup:;, foo@example.com, othergroup:bar@example.com, bar2@example.com; 

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>:" ).
Assignee: nobody → lusian
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.
(Assignee)

Comment 19

8 years ago
Created attachment 408596 [details] [diff] [review]
msg_parse_Header_addresses: add "Group Addresses" support, add ';' as a delimeter
Attachment #408336 - Attachment is obsolete: true
(Assignee)

Comment 20

8 years ago
Created attachment 408597 [details] [diff] [review]
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

8 years ago
Please request review for the patch and testcase. standard8 might be a suitable reviewer for it.
Status: NEW → ASSIGNED
(Assignee)

Comment 22

8 years ago
Created attachment 408985 [details] [diff] [review]
msg_parse_Header_addresses: add "Group Addresses" support, add ';' as a delimeter, 1
Attachment #408596 - Attachment is obsolete: true
Attachment #408985 - Flags: review?(bugzilla)
(Assignee)

Comment 23

8 years ago
Created attachment 408986 [details] [diff] [review]
test cases
Attachment #408597 - Attachment is obsolete: true
Attachment #408986 - Flags: review?(bugzilla)
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: a@foo.invalid ;;extra:;

Undisclosed recipients:;;extra:a@foo.invalid;

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?
Attachment #408986 - Flags: review?(bugzilla) → review-
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.
Attachment #408985 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 26

8 years ago
Created attachment 419950 [details] [diff] [review]
fix bug in comment 24, add change per comment 25
Attachment #408985 - Attachment is obsolete: true
Attachment #419950 - Flags: review?(bugzilla)
(Assignee)

Comment 27

8 years ago
Created attachment 419951 [details] [diff] [review]
add test cases in comment 24
[Checkin: Comment 31]
Attachment #408986 - Attachment is obsolete: true
Attachment #419951 - Flags: review?(bugzilla)
Attachment #419951 - Flags: review?(bugzilla) → review+
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.
Attachment #419950 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 29

8 years ago
Created attachment 424565 [details] [diff] [review]
comment 28
[Checkin: Comment 30]
Attachment #419950 - Attachment is obsolete: true
Attachment #424565 - Flags: superreview?(bienvenu)

Updated

8 years ago
Attachment #424565 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Comment on attachment 424565 [details] [diff] [review]
comment 28
[Checkin: Comment 30]


http://hg.mozilla.org/comm-central/rev/81ba8ae8671d
Attachment #424565 - Attachment description: comment 28 → comment 28 [Checkin: Comment 30]
Comment on attachment 419951 [details] [diff] [review]
add test cases in comment 24
[Checkin: Comment 31]


http://hg.mozilla.org/comm-central/rev/544cf79ddf8c
Attachment #419951 - Attachment description: add test cases in comment 24 → add test cases in comment 24 [Checkin: Comment 31]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b1
Depends on: 549931
Flags: wanted-thunderbird3?

Updated

8 years ago
Depends on: 564698

Comment 32

5 years ago
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?

a@b.com; b@c.com

current result: nothing
expected result: two single addresses in two recipient rows
Flags: needinfo?

Comment 33

5 years ago
(In reply to Thomas D. from comment #32)
> a@b.com; b@c.com
> 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
Flags: needinfo?

Comment 34

5 years ago
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?
Flags: needinfo?

Comment 35

5 years ago
What? Why only when sending? How can one edit the list before sending?
Flags: needinfo?

Updated

4 years ago
Depends on: 919953

Comment 36

2 years ago
This seems to have regressed in Thunderbird 31, according to bug 1059988. (See that bug for details.)
You need to log in before you can comment on or make changes to this bug.