937265 - fake scam warning for auto-linked ip address

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Plain text emails with ip address links get flagged as scam. For instance,  http://130.128.4.1 - the address get auto-linked, but auto-linking adds a trailing slash.

Comment 1

[Jim Porter (:squib)]

Comment on attachment 830362 [details] [diff] [review]
proposed fix

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

rs=me with the following changes.

::: mail/base/content/phishingDetector.js
@@ +133,5 @@
>          aLinkText = aLinkText.replace(/^<(.+)>$|^"(.+)"$/, "$1$2");
>  
>        var failsStaticTests = false;
> +      if (aLinkText != aUrl &&
> +          aLinkText.replace(/\/+$/, "") != aUrl.replace(/\/+$/, ""))

I don't think we need the "aLinkText != aUrl" now (although I'd be willing to accept an argument that it makes things faster). We would need to make sure that aLinkText is non-null, though.

A brief comment here would also be nice.

@@ +210,5 @@
>     * Private helper method to determine if the link node contains a user visible
>     * url with a host name that differs from the actual href the user would get taken to.
>     * i.e. <a href="http://myevilsite.com">http://mozilla.org</a>
>     * 
>     * @return true if aHrefURL.host matches the host of the link node text. 

Isn't this exactly the opposite of what this function returns?

::: mail/test/mozmill/message-header/test-phishing-bar.js
@@ +33,5 @@
>      contentType: "text/html"
>    }}));
>    add_message_to_folder(folder, create_message());
> +  add_message_to_folder(folder, create_message({body: {
> +    body: 'check out http://130.128.4.1. and http://130.128.4.2/.', 

Nit: trailing whitespace.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 830362 [details] [diff] [review]
proposed fix

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

::: mail/base/content/phishingDetector.js
@@ +133,5 @@
>          aLinkText = aLinkText.replace(/^<(.+)>$|^"(.+)"$/, "$1$2");
>  
>        var failsStaticTests = false;
> +      if (aLinkText != aUrl &&
> +          aLinkText.replace(/\/+$/, "") != aUrl.replace(/\/+$/, ""))

I think we should keep the check. It avoids null and for the majority of cases it's all that's needed.

@@ +210,5 @@
>     * Private helper method to determine if the link node contains a user visible
>     * url with a host name that differs from the actual href the user would get taken to.
>     * i.e. <a href="http://myevilsite.com">http://mozilla.org</a>
>     * 
>     * @return true if aHrefURL.host matches the host of the link node text. 

Good catch!

Comment 3

[Magnus Melin [:mkmelin]]

That was meant as
(In reply to Jim Porter (:squib) from comment #1)
> >        var failsStaticTests = false;
> > +      if (aLinkText != aUrl &&
> > +          aLinkText.replace(/\/+$/, "") != aUrl.replace(/\/+$/, ""))
> 
> I don't think we need the "aLinkText != aUrl" now (although I'd be willing
> to accept an argument that it makes things faster). We would need to make
> sure that aLinkText is non-null, though.

I think we should keep the check. It avoids null and for the majority of cases it's all that's needed.

Comment 4

[Jim Porter (:squib)]

Alright, that's fine. Land this whenever you're ready! (And then I can finish up the patch for bug 296952. :))

Comment 5

[Magnus Melin [:mkmelin]]

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