496440 - reply-button defaults to reply-all even when there are no header recipients

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Sometimes messages get sent to a group of people and the sender doesn't want to publicize the list of people they're sending to, so everyone ends up BCCed.

I got a message today with the following header:

To: undisclosed-recipients:;

Thunderbird got confused and offered "reply all" as the default menu choice rather than "reply".

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Yeah, I believe that my code is thinking that "undisclosed-recipients:" is a recipient, and so would try to reply-all to the sender and "undisclosed-recipients:".

Assuming there's a way to find out whether an address is real, this should be a fairly small fix.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Seems like we could tweak nsIMsgDbHdr and/or nsIMsgHeaderParser to understand that syntax, if they don't already.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

That seems reasonable, but it's not really a syntax, just a convention.  I've seen, in my inbox, the following values:
undisclosed-recipients:
undisclosed-recipients: ;
undisclosed-recipients:;

And from X-Mailer: Microsoft Outlook Express 6, I also saw:
To: "Undisclosed-Recipient:;"@outbox.allstream.net <mailto:>  
To: <"Undisclosed-Recipient:;"@latte.ca>
To: <"Undisclosed-Recipient:;"@pobox10.ipowerweb.com>

I think it would be reasonable, if not perfect, to punt on the last three.  For the first three, I'm calling nsIMsgHeaderParser.parseHeadersWithArray().  Would it be reasonable to return null or "" in the email address array for these cases, or would it be better to omit them entirely?

I've got a quick patch which I'll put up, which kind of hacks around the problem.  Let me know if you think it's okay for now, or if you'ld rather leave it the way it is until I (or someone else) can change the nsIMsgHeaderParser/nsIMsgDbHdr.  (I think the HeaderParser might be a better place to do it, just from the methods I'm calling.)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: Strip out undisclosed recipients, in three of their many forms.

It's a quick hack, but it works for most of the email I have access to, and might let us push a more involved fix off to a later beta.

Let me know what you think.

Thanks,
Blake.

Comment 5

[Magnus Melin [:mkmelin]]

If it's just one recipient, check that ':' and ';' aren't present in it?

It doesn't have to say "undisclosed-recipients", but can be any valid rfc2822 group syntax.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: Re-work the logic to include the sending address in our calculations.

(In reply to comment #5)
> If it's just one recipient, check that ':' and ';' aren't present in it?
> It doesn't have to say "undisclosed-recipients", but can be any valid rfc2822
> group syntax.

I've seen "undisclosed-recipients:" (without the ';') in the wild, so I'm going to just check for ':'.  This is reasonably safe, since from my tests, it looks like if there is an email address group, the ':' will become part of the first name, and not show up in the email address list.

After implementing that, I had something that seemed to work fairly well, and then I tried it on my sent messages, and it totally didn't do the right thing, so I've reworked the logic to include the sender's address when we're trying to figure out how many people we'll be replying to.

One of the things I'm not sure about is the indentation for:
+  let numAddresses = hdrParser.parseHeadersWithArray(uniqueAddresses,
+                                                     emailAddresses, {}, {});

I've seen a few different formats for lines like it, and picked the one I thought you would like the most.  ;)

The other thing I'm not sure about is:
+  if ("bcc" in currentHeaderData)
+    addresses += currentHeaderData.bcc.headerValue;
I sort of expected the data from the bcc header to show up in msgHdr.bccList, but it's not there, and is in currentHeaderData.bcc instead.  Since I can get to the data, I'm not sure whether it's worth filing a bug on.

Thanks,
Blake.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 382910 [details] [diff] [review]
Re-work the logic to include the sending address in our calculations.

Sorry about the delay, unfortunately this bitrotted :(

Did this work when people have e.g. "Example [:foo] <[e-mail]>" addresses?

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: The un-bit-rotted version of the previous patch.

(In reply to comment #7)
> (From update of attachment 382910 [details] [diff] [review])
> Sorry about the delay, unfortunately this bitrotted :(

No worries.  I like the change that bitrotted it.  :)

> Did this work when people have e.g. "Example [:foo] <[e-mail]>"
> addresses?

Yes, because in that case the ":" will appear in their name, not in their address, so the check on line 863 will never see it.

It will fail for people with a ":" in their email address, but from my reading of the spec, I believe that's illegal.  Also, the only address I've seen with a ":" in it was '"Undisclosed-Recipient:;"@latte.ca', which is fairly clearly not an actual address.

Thanks,
Blake.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 383298 [details] [diff] [review]
The un-bit-rotted version of the previous patch.


>+  for (i in emailAddresses.value)
>+    if (/:/.exec(emailAddresses.value[i]))
>+      numAddresses--;

Three nits:
 o var i
 o use test() instead of exec()
 o use {'s for the for loop (easier to read as its body is multiline

r=mkmelin with that

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: The patch with mkmelin's suggestions.

(In reply to comment #9)
> Three nits:
>  o var i

Fixed.

>  o use test() instead of exec()

Fixed.

>  o use {'s for the for loop (easier to read as its body is multiline

Fixed.

> r=mkmelin with that

Thanks.

Comment 11

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9f8df696c12b