Closed Bug 401014 Opened 14 years ago Closed 7 years ago

Configure Text Format Behavior, Send Options, HTML Domain and Plain Text Domains match too broadly. Domain is truncated when using format autodetect

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: donb, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322)
Build Identifier: version 2.0.0.6 (20070728)

In "Tools | Options | Composition | Send Options | Plain Text Domains",
I entered lists.berkeley.edu, which is only one server in our domain - the listserv machine.

Then I try to send an html formatted email to myself, donb@berkeley.edu. It is received as text only. When I remove the lists.berkeley.edu spec, and send it again, it is received with proper formatting.

I uninstalled all add-ons except the default Talkback, and the problem persisted.

Reproducible: Always

Steps to Reproduce:
1.In "Tools | Options | Composition | Send Options | Plain Text Domains", add xxxx.YourMailServerDomain
 
2.Send yourself some red or blue text.
3.Receive it looking black.
Actual Results:  
Email is received without html formatting.

Expected Results:  
Email should be received with html formatting.

My listserv server does not handle html coded email.
Version: unspecified → 2.0
Confirmed in 2.0.0.16 on a Mac.

More specifically, it appears to be accepting a partial match instead of an exact match.  If either "test.example.com" or "example.com.test" are set as plain text domains and you write to someone@example.com, it will get sent in plain text.

Actually, it's a little worse than that -- a message to someone@ample.com will also get sent in plain text, which is really confusing and unexpected.

It should require an exact match (possibly with wildcards).  If not, the screen should get updated to reflect the fact that it isn't, because there's nothing there right now to indicate that partial matches are happening.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Summary: The HTML blocker is too broad → HTML and Plain Text Domains match too broadly
Duplicate of this bug: 649853
Duplicate of this bug: 647522
Duplicate of this bug: 300705
this has probably existed forever
Component: General → Message Compose Window
QA Contact: general → message-compose
Summary: HTML and Plain Text Domains match too broadly → Configure Text Format Behavior, Send Options, HTML Domain and Plain Text Domains match too broadly. Domain is truncated when using format autodetect
Version: 2.0 → unspecified
Duplicate of this bug: 287854
No longer blocks: 698262
Duplicate of this bug: 698262
(A) There are mailnews.plaintext_domains and mailnews.html_domains.
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#4887
> 4887     NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.plaintext_domains", EmptyString(),
> 4888                                                 plaintextDomains);
> 4889     NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.html_domains", EmptyString(),
> 4890                                                 htmlDomains);

(B) mailnews.plaintext_domains and mailnews.html_domains looks used when recipient's text or html preference is not defined in address book(Unknown). 
> 4908       // if we don't have a prefer format for a recipient, check the domain in
> 4909       // case we have a format defined for it

(C) If domainpart of mail address(string after @) is substring of mailnews.plaintext_domains or mailnews.html_domains, mail&news looks to consider that the domainpart is registered as plaintext_domain or html_domain.   
> 4913         PRInt32 atPos = recipient.mEmail.FindChar('@');
> 4914         if (atPos >= 0)
> 4915         {
> 4916           domain = Substring(recipient.mEmail, atPos + 1);
> 4917           if (CaseInsensitiveFindInReadable(domain, plaintextDomains))
> 4918             recipient.mPreferFormat = nsIAbPreferMailFormat::plaintext;
> 4919           else
> 4920             if (CaseInsensitiveFindInReadable(domain, htmlDomains))
> 4921               recipient.mPreferFormat = nsIAbPreferMailFormat::html;
> 4922         }

Above code doesn't look to care for case of "@xxx.yyy.zzz is used and xxx.yyy.zzz is placed in mailnews.<plaintext or html>_domains, and @yyy.zzz is used too." It was very rare case, I think.

Ben Bucksch, is my guess right?
If so, bug 647522 is perhaps "mail address in address book with text/html preference=HTML" case.
(In reply to WADA from comment #8)
> (C) If domainpart of mail address(string after @) is substring of
> mailnews.plaintext_domains or mailnews.html_domains, mail&news looks to
> consider that the domainpart is registered as plaintext_domain or
> html_domain.   
> > 4913         PRInt32 atPos = recipient.mEmail.FindChar('@');
> > 4914         if (atPos >= 0)
> > 4915         {
> > 4916           domain = Substring(recipient.mEmail, atPos + 1);
> > 4917           if (CaseInsensitiveFindInReadable(domain, plaintextDomains))
> > 4918             recipient.mPreferFormat = nsIAbPreferMailFormat::plaintext;
> > 4919           else
> > 4920             if (CaseInsensitiveFindInReadable(domain, htmlDomains))
> > 4921               recipient.mPreferFormat = nsIAbPreferMailFormat::html;
> > 4922         }
> 
> Ben Bucksch, is my guess right?

Indeed, that's precisely the bug, nail on head. This comparison is just wrong.

We need to split the string at ",", then compare the right-most part, and only get a hit, if the email address domain string is equal or longer than the pref domain string, not the other way around as now.

Ben
(and before string comparison, add a dot before both domains, to avoid matching "@nopaypal.com" when "paypal.com" is configured.)
See Also: → 889152
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
Attached patch proposed fix (obsolete) — Splinter Review
This fixes it so that the domain must match exactly, or at least end with dot+the domain.

So if the recipient is foo@cc.example.com it would match a domain of cc.example.com or example.com, but not ample.com
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #823030 - Flags: review?(mnyromyr)
Attachment #823030 - Flags: review?(irving)
Comment on attachment 823030 [details] [diff] [review]
proposed fix

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

Thanks for picking this up.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4912,5 @@
> +
> +        nsDependentSubstring emailDomain = Substring(recipient.mEmail, atPos + 1);
> +        nsAutoString domain;
> +
> +        // Check plain text domains.

Refactor out an 'isInDomainList' function so that we don't need to duplicate this code for each of the plaintext and html lists.

@@ +4922,5 @@
> +          if (right == kNotFound)
> +            right = plaintextDomains.Length();
> +          domain = Substring(plaintextDomains, left, right);
> +          nsAutoString dotDomain(domain);
> +          dotDomain.AppendLiteral(".");

We want the dot on the *beginning* of the domain string, don't we?
Attachment #823030 - Flags: review?(irving) → review-
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Thx for the review! Updated, and added a test.
Attachment #823030 - Attachment is obsolete: true
Attachment #823030 - Flags: review?(mnyromyr)
Attachment #8358839 - Flags: review?(irving)
Comment on attachment 8358839 [details] [diff] [review]
proposed fix, v2

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

Thanks for the tests. Almost there ;->

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +3028,5 @@
> + * Subdomains are also considered to match.
> + * @param aDomain - the domain name to check
> + * @param aDomainList - a comman separated string of domain names
> + */
> +bool IsInDomainList(const nsString &aDomain, const nsString &aDomainList)

The parameters should be nsAString&, see https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#As_function_parameters

@@ +3042,5 @@
> +    right = aDomainList.FindChar(',', left);
> +    if (right == kNotFound)
> +      right = aDomainList.Length();
> +    nsAutoString domain;
> +    domain = Substring(aDomainList, left, right);

nsDependentSubstring domain = Substring(aDomainList, left, right); should avoid allocating heap memory and copying the substring.

@@ +3044,5 @@
> +      right = aDomainList.Length();
> +    nsAutoString domain;
> +    domain = Substring(aDomainList, left, right);
> +    nsAutoString dotDomain = NS_LITERAL_STRING(".");
> +    dotDomain.Append(domain);

nsString dotDomain = NS_LITERAL_STRING(".") + domain; should similarly avoid copying, see https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#String_Concatenation

@@ +3045,5 @@
> +    nsAutoString domain;
> +    domain = Substring(aDomainList, left, right);
> +    nsAutoString dotDomain = NS_LITERAL_STRING(".");
> +    dotDomain.Append(domain);
> +    if (aDomain.Equals(domain, nsCaseInsensitiveStringComparator()) ||

Do this test (and return true) before you construct dotDomain, to avoid the extra work (micro-optimizations 'R' us ;-) e.g.

if (aDomain.Equals(...))
  return true;
nsString dotDomain = ....
if (stringEndsWith(aDomain, dotDomain...))
  return true;

@@ +4889,5 @@
> +        if (atPos < 0)
> +          continue;
> +
> +        nsAutoString emailDomain;
> +        emailDomain = Substring(recipient.mEmail, atPos + 1);

Use 'nsDependentSubstring emailDomain = Substring(...)' here; that lets the Substring object pass through without copying the data.
Attachment #8358839 - Flags: review?(irving) → review-
Attached patch proposed fix, v3Splinter Review
Adressing review comments, except for the one about plussing the strings.
That's not allowed for the external string api.
Attachment #8358839 - Attachment is obsolete: true
Attachment #8365709 - Flags: review?(irving)
Comment on attachment 8365709 [details] [diff] [review]
proposed fix, v3

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

This looks good.

Unfortunately I just thought of a case where both the old and new code will give the wrong answer, but I think it's an obscure enough issue that I'd be OK with filing a new bug and fixing it later:

If the user configures domain.example => plaintext, sub.domain.example => HTML, and the user sends mail to user@sub.domain.example, we'll match ".domain.example" in the plaintext check and not bother doing the HTML check, so we'll send user@sub.domain.example the wrong format.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5168,5 @@
>          recipientsStr.Append(recipient.mAddress);
>        }
>  
> +      // Add recipients to the nonHtmlRecipient list if they haven't
> +      // explicitely allowed it.

nittiest of comment nits: s/explicitely/explicitly/
Attachment #8365709 - Flags: review?(irving) → review+
https://hg.mozilla.org/comm-central/rev/268ad6c0803b -> FIXED
Flags: in-testsuite+
Target Milestone: --- → Thunderbird 29.0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
External api trap :/

Apparently this has to be
+    nsAutoString dotDomain;
+    dotDomain.Assign(NS_LITERAL_STRING("."));
+    dotDomain.Append(domain);

Relanded as https://hg.mozilla.org/comm-central/rev/9668ba473b98
.AssignLiteral ?
That should work too. The think I didn't expect was that nsAutoString dotDomain = NS_LITERAL_STRING("."); works find under external api but doesn't compile w/ internal.
You need to log in before you can comment on or make changes to this bug.