1476428 - For "scam" warnings due to bad links, only warn on click.

Status: RESOLVED FIXED

(Thunderbird :: Security, enhancement)

Description

[Josiah Bruner [:jsbruner]]

The current scam detection algorithm has a fairly high false-positive rate (one high enough that some propose is cause to disable i.e. bug 623198).

One main cause of this is due to link tracking, short urls, etc. This is because our current algorithm warns when the URL that's presented to the user doesn't match the actual underlying URL.

It was therefore proposed in bug 623198 to mitigate this by only displaying the scam warning when a user clicks on the "bad" link.

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: bug1476428.patch

Uploading an initial patch which only warns the user about phishing URLs in links when they click a link.

This will break some of the phishing tests. Will fix those next...

Comment 2

[Josiah Bruner [:jsbruner]]

Attachment: Patch v2

This patch now corrects the test-phishing-bar.js tests so they pass. It does this two ways:

For tests which test the phishing-bar behavior, and not the underlying algorithm, I switched the "bad" emails to use bad forms rather than links, since the form warnings always appear. This keeps these tests as true to the original as possible.

For tests which were dependent on algorithm, I now cause the links to pressed (via click_link_if_available()) so we can assert whether or not the warning bar appears.

But, in situations where the warning doesn't appear (because the link is OK), the changes cause the corresponding pages to be opened in the browser. I doubt this is okay, but I don't know how to test this adequately without doing it. 

Magnus, do you have any idea?

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8995673 [details] [diff] [review]
Patch v2

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

As for the tests, I don't think you need to explicitly verify that the link opened in the browser. Since you don't plan for the dialog to pop up in those cases, in case it does appear the tests would fail - so it is actually tested already.

But the functionality of the patch is not as I had intended. Currently it shows a scam warning if you click *any* link in a message that's determined to be scam. 
Let me explain what I had in mind:

 - the link analysis may only have to be done on click, or at least status just checked at that point
 - on click, only if the link looks suspicious, pop up a dialog describing something like 

      The link you just clicked seems to lead to another site than the link text indicated.
      This is commonly used for tracking whether you followed the link or not, but may also be a scam.

      [Go ahead] [Skip tracker] [Cancel]

  * the go ahead button would open the link
  * the skip tracker button would open what the link text said it would

What do you think?

Comment 5

[Josiah Bruner [:jsbruner]]

Regarding the functionality:

I definitely agree that we should only do detection / warn on the specific link that is clicked. I will make that change.

I like the idea of having a "skip tracker" mechanism, though I would change it a bit. E.g. for something like:

<a href="https://example.com/bad/link/here"> https://google.com/search </a>

The popup would look like:

----------------------------------------------------------------------------------------------------------
      The link you just clicked seems to lead to another site than the link text indicated.
      This is sometimes used for tracking whether you clicked the link, but could also be a scam.

                             The site you are about to navigate to is:

                                https://example.com/bad/link/here

      [Go ahead] [Go directly to google.com]                                         [Get me out of here!]

----------------------------------------------------------------------------------------------------------

What do you think about this? I like it because it:

A) Tells you what the underlying link is, which is helpful for determining whether it's a scam or not.
B) Tells you where you'll go if you "skip tracker". For users who don't understand trackers, this seems clearer and will likely cause it to be used more often.

The biggest downside IMO is that "Go directly to ..." can't contain the entire (potentially very long) link. I feel it's an okay compromise though.

Comment 6

[Josiah Bruner [:jsbruner]]

Oh, I should note that my intention is for [Go directly to google.com] to still take you to the full URL: https://google.com/search

Comment 7

[Alexander B.]

I like this feature! Personally, I hover over links in mails in order to check out where it actually takes me. With this, I wouldn't have to do this anymore and some slight variations built in to the actual URL for scam (like add some strange tld) that might be overlooked become visible more easily.

Probably the message box might for the unexperienced, short-tempered user be faster to comprehend, if the "Go there" part is right at the URL. Why not even have the URL clickeable?

----------------------------------------------------------------------------------------------------------
      The link you just clicked seems to lead to another site than the link text indicated.
      This is sometimes used for tracking whether you clicked the link, but could also be a scam.

      The link you were about to navigate to is (click it to go there nevertheless):
            https://example.com/bad/link/here

      Alternatively, you can go the link indicated in the text (by clicking it):
            https://google.com/search

                                                                                       [I'm going nowhere]

----------------------------------------------------------------------------------------------------------

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Josiah Bruner [:jsbruner] [:📧🔒] from comment #5)
> What do you think about this? I like it because it:
> 
> A) Tells you what the underlying link is, which is helpful for determining
> whether it's a scam or not.

I'd avoid displaying the full URLs, as scammers like to have similar looking sub-domains and other junk, possibly very long urls. But maybe the dialog text can tell you the FQDN for both cases.

Another think to keep in mind though is that many big sites have sub-domains for trackers. So you might have tracker.example.com and example.com. Then the "go directly to example.com" case gets a bit confusing

> B) Tells you where you'll go if you "skip tracker". For users who don't
> understand trackers, this seems clearer and will likely cause it to be used
> more often.

Yeah maybe it's to techy wording...

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Alexander B. from comment #7)
> faster to comprehend, if the "Go there" part is right at the URL. Why not
> even have the URL clickeable?

The dialog would be modal, and opening urls by clicking from there would not be so nice.

I kind of agree it would be nice to see the full url somewhere, but for scam prevention with maybe a domain name off by a few chars that's probably not such a good idea. Regular users wouldn't understand the difference.

Comment 10

[Josiah Bruner [:jsbruner]]

Attachment: AnalyzePerLinkPatch WIP 1

I'm uploading my WIP patch that now analyzes link URLs only on click.

It works just fine, but I'm not requesting review yet because I haven't changed the front-end behavior yet.

Comment 11

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin from comment #8)
> (In reply to Josiah Bruner [:jsbruner] [:📧🔒] from comment #5)
> > What do you think about this? I like it because it:
> > 
> > A) Tells you what the underlying link is, which is helpful for determining
> > whether it's a scam or not.
> 
> I'd avoid displaying the full URLs, as scammers like to have similar looking
> sub-domains and other junk, possibly very long urls. But maybe the dialog
> text can tell you the FQDN for both cases.

Sounds good. I'll do just the FQDN for now.

> 
> Another think to keep in mind though is that many big sites have sub-domains
> for trackers. So you might have tracker.example.com and example.com. Then
> the "go directly to example.com" case gets a bit confusing

Good point. You're right too; using the FQDN thing would solve this issue.

Comment 12

[Magnus Melin [:mkmelin]]

I just recalled, that actually for just differing subdomain scam detection wouldn't trigger, as we're comparing the effective base domain: https://dxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#188

Comment 13

[Josiah Bruner [:jsbruner]]

Attachment: AnalyzePerLinkPatch WIP 2

This patch makes the changes to the front-end dialog and adds relevant localization strings.

The patch still needs:

1. Test improvements. Notably, unit tests for phishingDetector and more comprehensive integration (mozmill) tests.
2. Refactoring. Some of the code can be cleaned up, especially the warnOnSuspiciousLinkClick function.
3. Removal of console.log statements.

That said, I'm still requesting feedback from mkmelin at this point since those changes are all relatively small. The general design and implementation works.

I particularly dislike the appearance of the model. It doesn't appear nsIPromptService gives me the ability to center text, for example. In addition, I don't really have control of the location of the buttons, so their layout is sub par in my opinion. I'll attach a screenshot of what it looks like on macOS.

Comment 14

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of the suspicious URL dialog

Comment 15

[Josiah Bruner [:jsbruner]]

Comment on attachment 9004008 [details]
Screenshot of the suspicious URL dialog

Jesse,

Interested in your thoughts on this dialog. I'm not thrilled with the UX surrounding this feature, but we're limited in what we can do reasonably.

For instance, is the dialog easy to understand?
Would users be confused about what "Go Ahead" means?
Does only using the domain name, rather than the entire URL confuse things?
Etc.

Comment 16

[Jesse Bruner]

Comment on attachment 9004008 [details]
Screenshot of the suspicious URL dialog

Well, few thoughts on the design from an UX perspective which I would consider researching: 

1)First thing I am concerned about is the tone of the message. Having a question mark, plus using the blue color create a feeling that this is merely a help popup. Not what we would want if we would like our user to have a sense of urgency about this. To keep the user from clicking the message off and disregarding it, from the initial impression the user should have a sense of stress. I think Chrome does this fairly elegantly, with very few words, a red lock, and a the threat of stolen credit cards. (See https://www.technipages.com/wp-content/uploads/2015/04/Chrome-Advanced.png)

2) I personally for a while was confused as to what the "Go directly to google.com" button would do, and why it's even there. Upon further investigation the use is now made quite clear, however, I am worried that the usage would not be obvious as intended to the user.

"Get me out of here!" is perfectly clear, and creates that sense of urgency. This should be the default option, because if we want the user to trust Thunderbird, then Thunderbird should appear to be trying to always nudge the user towards the safest option, while still giving control over to the user. As much as "get me out of here" is clear, I am afraid the other options are not. I believe we should limit the options down to 2. Limiting the options to only 2 reduces cognitive strain on the user, who may be very confused as to why they are receiving this popup already. (In this situation I am applying Hick's Law to my logic.) 

I think the best option would be to cut out "Go ahead" entirely and just keep the "direct to google.com" button. The reason I mention this is because I struggle to think of a reason why a user would want to use the "Go ahead." 

-Most major sites that the majority of users use don't use shortened url's that may be mistaken as a scam. When I looked at a few I received from google and apple, they did not use short urls.
-Given that the "direct to google.com" option is the more secure option, and would work the majority of the time, it should be used over "go ahead." 

This is all in the interest of the user. This dialog is already unusual and could be disconcerning. When users feel unsure of what is going on, they tend to go into a fight or flight mode. In this case, we should keep the options limited to just be continue on (but with the added security), or "get me out of here!", which would be the option we nudge towards to minimize the negatives of rash decisions. 

Furthermore, if what we are interested in is the experience of the user with Thunderbird, any potential problem with using the "go directly to google.com" feature where it may break where the user thought the link would end up would most likely not effect their experience with Thunderbird. To the user, that would appear to be more of a broken link than a problem with Thunderbird, and all the user experienced was Thunderbird warning them of a potential threat. The vast majority of the time, Thunderbird is going to be protective, and that will benefit the users experience more than sending them directly to the address which may have trackers, or scam redirects. 

I also understand many of Thudnerbird users may be more advanced and want options. For these users, I would recommend having in settings an option to enable following links or not.


3) And in terms of the text, it is fine, and would work, however it also appears to be written like a help popup. Additionally, in contrast to the previously mentioned Chrome warning, if the user reads the text, I am not convinced they would feel any sense of urgency. There isn't any emotional element to the text. Chrome works because it hits the user where it hurts, i.e. passwords and messages, which are private information, and credit cards, which is directly tied to their finances. If possible, we should entertain the idea of rewording the sentences to be more along the lines of:

"What you just clicked may lead to another site than indicated. This could be a scam to retrieve personal information. The site you are about to navigate to is: example.com"

All in all, my biggest worry is that the user will disregard this and instinctively click off of the message. We need to make the message more intimidating, yet also more clear as to show goodwill to our user. If we want Thunderbird to be secure, yet offer a great experience in these situations, minimizing the options down to 2 would make these situations easier for the user. Keeping it to be "get me out of here" or "go directly to google.com" (which should be reworded) then for the majority of the possible scams/false positives, the user will be properly warned, or the link will still work properly.

I hope my points were clear enough, this is quite a complex issue as to the benefits and downsides of each option. The reasons I chose the options I did is because I feel it will respect the user the most, while also being as secure as possible. 

To illustrate my idea:
--------------------------------------------------------------------
!!!
What you just clicked may lead to another site than indicated. This could be a scam to retrieve personal information. 

The site you are about to navigate to is: 
example.com/realpage.html

[Continue Anyway]    ([Get me out of here!])       
--------------------------------------------------------------------

Jesse

Comment 17

[Josiah Bruner [:jsbruner]]

(In reply to Jesse Bruner from comment #16)
> Comment on attachment 9004008 [details]
> Screenshot of the suspicious URL dialog
> 
> Well, few thoughts on the design from an UX perspective which I would
> consider researching: 
> 
> 1)First thing I am concerned about is the tone of the message. Having a
> question mark, plus using the blue color create a feeling that this is
> merely a help popup...

I agree the dialog is too passive. For now I think it's reasonable to simply use a more severe icon (red or yellow !, for example).


> I think the best option would be to cut out "Go ahead" entirely and just
> keep the "direct to google.com" button. The reason I mention this is because
> I struggle to think of a reason why a user would want to use the "Go ahead." 
> 
> -Most major sites that the majority of users use don't use shortened url's
> that may be mistaken as a scam. When I looked at a few I received from
> google and apple, they did not use short urls.
> -Given that the "direct to google.com" option is the more secure option, and
> would work the majority of the time, it should be used over "go ahead." 
> 

Good point. I too can't think of a decent reason I user would *need* to follow the tracking link and not use the link that was displayed to them.

> I also understand many of Thudnerbird users may be more advanced and want
> options. For these users, I would recommend having in settings an option to
> enable following links or not.

Good idea. I'll add a new pref in the Settings -> Security -> Email Scams section to disable *this particular feature*.

> 
> 
> 3) And in terms of the text, it is fine, and would work, however it also
> appears to be written like a help popup. Additionally, in contrast to the
> previously mentioned Chrome warning, if the user reads the text, I am not
> convinced they would feel any sense of urgency. There isn't any emotional
> element to the text. Chrome works because it hits the user where it hurts,
> i.e. passwords and messages, which are private information, and credit
> cards, which is directly tied to their finances. If possible, we should
> entertain the idea of rewording the sentences to be more along the lines of:
> 
> "What you just clicked may lead to another site than indicated. This could
> be a scam to retrieve personal information. The site you are about to
> navigate to is: example.com"

I like this. It's simpler. The description of tracking emails would probably be confusing for most users anyway. I will simplify the text.

> 
> I hope my points were clear enough, this is quite a complex issue as to the
> benefits and downsides of each option. The reasons I chose the options I did
> is because I feel it will respect the user the most, while also being as
> secure as possible. 
> 
> To illustrate my idea:
> --------------------------------------------------------------------
> !!!
> What you just clicked may lead to another site than indicated. This could be
> a scam to retrieve personal information. 
> 
> The site you are about to navigate to is: 
> example.com/realpage.html
> 
> [Continue Anyway]    ([Get me out of here!])       
> --------------------------------------------------------------------
> 
> Jesse

Is the example.com domain the displayed link or the hidden one? If it's the hidden (actual) one, then "Continue Anyway" seems confusing. I thought you proposed using only the "Go directly to ...". We could do:

 --------------------------------------------------------------------
 !!!
 What you just clicked may lead to another site than indicated. This could be
 a scam to retrieve personal information. 
 
 The site the link tried to take you to is: 
 example.com/realpage.html

 Instead we'll take you to the displayed page:
 displayed.com/indicatedpage.html
 
 [Continue]    ([Get me out of here!])       
 --------------------------------------------------------------------

What do you think?

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Jesse Bruner from comment #16)
> -Most major sites that the majority of users use don't use shortened url's
> that may be mistaken as a scam. When I looked at a few I received from
> google and apple, they did not use short urls.

Well twitter is using t.co if you get emails from them, and that content contains links to external sites (or at least used to, I don't have a fresh email sample handy).

I agree more than 2 options is a bit of a cognitive stress.
But, there may be use cases for going through the tracker. E.g. a system internal agreements so that within a group everybody knows who read what. Or wanting to go there to keep stats clear (you may be testing your own tracer too.) I suggest to drop the [get me out of here], but allow for that fucntionalty by cancelling the prompt ([x] or Esc).

Though this feature is used for scam prevention also, the ratio of actual scams vs trackers I imagine is pretty small. So I would tone down the urgency of the mismatched link - "Email Scam Alert" in the title is a bit scary. Maybe "Link Mismatch Detected"?

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9004005 [details] [diff] [review]
AnalyzePerLinkPatch WIP 2

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

Regarding the button placement. Looks good on linux. That's the plus of not having to do the layout cross platform (but of course, if it doesn't look good on Mac, that would be nice to fix in core.)

::: mail/base/content/contentAreaClick.js
@@ +170,5 @@
>    // Let the phishing detector check the link.
> +  urlPhishCheckResult = gPhishingDetector.warnOnSuspiciousLinkClick(href, linkText)
> +  if (urlPhishCheckResult === 1) {
> +    return false; // Block request
> +  } else if (urlPhishCheckResult === 2) {

no else after return please

::: mail/base/content/phishingDetector.js
@@ +133,5 @@
>        // We don't use dynamic checks anymore. The old implementation was removed
>        // in bug bug 1085382. Using the toolkit safebrowsing is bug 778611.
> +      //
> +      // Because these static link checks tend to cause false positives
> +      // we delay showing the warning until a user tries to click the link.

It's a bit confusing to show the scam bar at all, initiating it once the click happens. At that point the user is alreayd aware and makes a choice.

@@ +247,3 @@
>  
> +        var bundle = document.getElementById("bundle_messenger");
> +        var titleMsg = bundle.getString("confirmPhishingTitle");

please make these |let| while you're thouching this

@@ +253,5 @@
> +        var aButtonFlags = (nsIPS.BUTTON_POS_0) * (nsIPS.BUTTON_TITLE_IS_STRING) + (nsIPS.BUTTON_POS_1) * (nsIPS.BUTTON_TITLE_IS_STRING) + (nsIPS.BUTTON_POS_2) * (nsIPS.BUTTON_TITLE_IS_STRING) + nsIPS.BUTTON_POS_2_DEFAULT;
> +        var goAheadText = bundle.getString("confirmPhishingGoAhead");
> +        var getOutText = bundle.getString("confirmPhishingGetOut");
> +        var goDirectText = bundle.getFormattedString("confirmPhishingGoDirect",
> +                          [displayedURI.host], 1);

align with (" on the prev line

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +535,5 @@
>  confirmPhishingTitle=Email Scam Alert
>  #LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
>  confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
> +#LOCALIZATION NOTE %1$S is the host name of the host name of the url being visited
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.\n\nThe site you are about to navigate to is:\n\n%1$S

I think it would look better to just put it on the same line, i.e. drop the \n\ŋ and just have a space

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9004005 [details] [diff] [review]
AnalyzePerLinkPatch WIP 2

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +535,5 @@
>  confirmPhishingTitle=Email Scam Alert
>  #LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
>  confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
> +#LOCALIZATION NOTE %1$S is the host name of the host name of the url being visited
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.\n\nThe site you are about to navigate to is:\n\n%1$S

I meant for the "the site you are about to navigate to is:\n\n"

But now, I realize this wording is not very correct at least if we change the buttons. Where you navigate would change based on that.

So perhaps

+---------------------------------------------------------------------------------------------
|                                 Link Mismatch Detected
+---------------------------------------------------------------------------------------------
|
|   The link you just clicked seems to lead to another site than the link text indicated.
|
|   The link text indicated that the link would lead to <site>, but it will go to <tracker>.
|   This is sometimes used for tracking whether you clicked the link, but could also be a scam.
|   
|                                                 [ Go to <tracker> anyway ] [ Go to <site> ]
|
+----------------------------------------------------------------------------------------------

Comment 21

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin from comment #19)

> ::: mail/base/content/phishingDetector.js
> @@ +133,5 @@
> >        // We don't use dynamic checks anymore. The old implementation was removed
> >        // in bug bug 1085382. Using the toolkit safebrowsing is bug 778611.
> > +      //
> > +      // Because these static link checks tend to cause false positives
> > +      // we delay showing the warning until a user tries to click the link.
> 
> It's a bit confusing to show the scam bar at all, initiating it once the
> click happens. At that point the user is alreayd aware and makes a choice.

I suppose that's true. The rationale was that it bumped up the severity of the warning. However, if I change the icon to an '!', we would get that urgency anyway. So ok, I'll make this change.


(In reply to Magnus Melin from comment #20)
> So perhaps
> 
> +----------------------------------------------------------------------------
> -----------------
> |                                 Link Mismatch Detected
> +----------------------------------------------------------------------------
> -----------------
> |
> |   The link you just clicked seems to lead to another site than the link
> text indicated.
> |
> |   The link text indicated that the link would lead to <site>, but it will
> go to <tracker>.
> |   This is sometimes used for tracking whether you clicked the link, but
> could also be a scam.
> |   
> |                                                 [ Go to <tracker> anyway ]
> [ Go to <site> ]
> |
> +----------------------------------------------------------------------------
> ------------------

Fine with me. I'll do that.

I'll also address your other comments in the next patch. Thanks!

Comment 22

[Josiah Bruner [:jsbruner]]

So after trying to make these changes, I've hit a few roadblocks. One is a purely technical one, the other UX.

From the technical side, it looks like using nsIPromptService just isn't going to be appropriate here. There are a few reasons:

1. You can't change the icon.
  1.1 You can use prompt (and get a blue question mark) or alert and get a yellow exclamation mark.
    1.1.1 However, if you use alert, you can only have one button and optionally a check box, which isn't appropriate here.
2. You can't use two buttons when neither of them act as "cancel".
  2.1 If I try to use two buttons per Magnus' recommendation in comment 20, "Go to <site>" doesn't work, because the button in position "1" always returns the value 1, which is the same value the "Esc" key always returns: bug 345067

To fix this, we would need to create our own modal dialog and display that instead.


However, this brings me to the UX problem; I wonder if modal dialogs are the correct approach at all? They are generally frowned upon [1].

I think a better approach might be to just *automatically* replace the underlying URL with the displayed one, and provide some way for a user to undo this if they don't like it. It would be really nice to get a mock-up for such behavior. I will try to create one. Or maybe jessebruner can?


[1] https://softwareengineering.stackexchange.com/questions/106031/javascripts-prompt-confirm-and-alert-considered-old-fashioned

Comment 23

[Josiah Bruner [:jsbruner]]

Attachment: AnalyzePerLinkPatch WIP 3

Check-pointing work. This addresses many of Magnus' comments and also switches to the (broken) 2-button layout.

Comment 24

[Jesse Bruner]

Before I create an animated mockup of the behaviour, what do you think of this static idea?


1) User either clicks on link and is automatically taken to the actual url OR the user will hover over the link:

Falselink.com


2) User hovers over link for a pop-up underneath to appear. Featuring an "original link" button and a help button. The original link allows the user to follow the link as intended by the sender. When the user mouses off the pop-up or the falselink then the popup will disappear:

falselink.com
|---------------------------------|
|            |Original Link|  |?| | 
|---------------------------------|

3) User is confused so hovers over/clicks the help (?) button, and another popup occurs explaining to the user why this is up.

falselink.com
|---------------------------------|----------------------------------------------------------|
|            |Original Link|  |?| |The link displayed does not match the site where you      |
|---------------------------------|will be taken. This may be a scam to retrieve personal    |
                                  |information, or for tracking purposes. The original link  |
                                  |button will take you to the address the sender intended.  |
                                  |                                                          |
                                  |Original Link:                                            |
                                  |superscam.io/sucker                                       |
                                  ------------------------------------------------------------

The user can then decide to follow the original link or click the link with the thunderbird feature enabled. Regardless, this solves many issues:
1) The majority of users will just click the Thunderbird-corrected link, and in the majority of cases that will be fine. 
2) The users who care can easily and elegantly follow the original link by just hovering over the link listed.
3) The help notification is also available but hidden and un-intrusive. 

Obviously the wording needs to be changed. Thoughts?

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Josiah Bruner [:jsbruner] [:📧🔒] from comment #22)
I see your point. Changing the actual link will be somewhat controversial though. And a lot harder to implement than you'd think, unfortunately.

Maybe we can just go with having the nsIPromptService and a [Cancel] button. (In the WIP 3 patch the button positions are the wrong way around.) Channing the icon is not that important - since most times it's not a scam, just a tracker. Having the prompt instead of silently switching the link has the positive side that it notices people on whether they are being tracked.

Comment 26

[Magnus Melin [:mkmelin]]

(In reply to Jesse Bruner from comment #24)
> Obviously the wording needs to be changed. Thoughts?

The most obvious problem is that if it's a scam link the link might differ only by one char perhaps, and it's not obvious what the difference is unless you read real closely. 

Well, besides that technically not too easy to implement. You'd have to worry about which properties of the documents were set by us vs an attacker. There are also some ways that the changed content would end up in the reply, which is not nice.

Comment 27

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin [:mkmelin] from comment #25)
> (In reply to Josiah Bruner [:jsbruner] [:📧🔒] from comment #22)
> I see your point. Changing the actual link will be somewhat controversial
> though. And a lot harder to implement than you'd think, unfortunately.
> 
> Maybe we can just go with having the nsIPromptService and a [Cancel] button.
> (In the WIP 3 patch the button positions are the wrong way around.) Channing
> the icon is not that important - since most times it's not a scam, just a
> tracker. Having the prompt instead of silently switching the link has the
> positive side that it notices people on whether they are being tracked.

That's reasonable. I still like the idea of Jesse started describing, but we can investigate such a feature and its behavior in a different bug.

Also, are you suggesting we stick with two buttons, where one of them is [Cancel] or go back to three buttons?

I will happily implement either one.

Comment 28

[Magnus Melin [:mkmelin]]

I'd go with 3 buttons.

Comment 29

[Josiah Bruner [:jsbruner]]

Attachment: AnalyzePerLinkPatch WIP 4

Checkpoint. This patch appears to completely work. Just needs some cleanup and verification of the test cases.

Comment 30

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot - AnalyzePerLinkPatch WIP 4.png

This is a screenshot of what the dialog looks like now.

I'm not sure why there's a ton of padding on the right. Maybe a limitation of the toolkit code or something?

Comment 31

[Josiah Bruner [:jsbruner]]

Attachment: Patch

This patch:

  1. Corrects a failing phishing-bar test case.
  2. Refactors some of the code.
  3. Corrects some of the localization strings.

It is now ready for review.

I will attach a screenshot of the current dialog UI as well.

Comment 32

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of new modal dialog

Comment 33

[Josiah Bruner [:jsbruner]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a04937f059b2866ae574982506532054048c4128

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9011248 [details] [diff] [review]
Patch

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

Almost there, but still some things to fix up

::: mail/base/content/contentAreaClick.js
@@ +30,4 @@
>      {
>        if (target.hasAttribute("href"))
>          href = target.href;
> +      linkText = gatherTextUnder(target)

this should be inside the if-clause, no?

@@ +119,4 @@
>    if (isLinkToAnchorOnPage(target))
>      return true;
>  
> +  let hrefAndLinkText = hRefForClickEvent(aEvent)

nit: missing ;

but, this is better written

let [href, linkText] = hRefForClickEvent(aEvent);

@@ +165,4 @@
>    aEvent.preventDefault();
>  
>    // Let the phishing detector check the link.
> +  urlPhishCheckResult = gPhishingDetector.warnOnSuspiciousLinkClick(href, linkText)

undeclared? just ad let
also, missing ;

::: mail/base/content/phishingDetector.js
@@ +220,5 @@
> +    const nsIPS = Ci.nsIPromptService;
> +
> +    // If the message was detected as a scam in the general sense, display a
> +    // generic warning...
> +    if (prevDetectedAsScam) {

I hadn't expected this. 
Why special case it? If we based the detection on the link mismatch, I'd like the new linktext choice, but if it's for other reasons (like including a form), displaying the message here doesn't make sense to me.

@@ +225,5 @@
> +      var hrefURL;
> +      // make sure relative link urls don't make us bail out
> +      try {
> +        hrefURL = Services.io.newURI(aUrl);
> +      } catch(ex) { return false; }

return 0;

@@ +232,5 @@
> +      if (hrefURL.schemeIs('http') || hrefURL.schemeIs('https'))
> +      {
> +        // unobscure the host name in case it's an encoded ip address..
> +        let unobscuredHostNameValue = this.isLegalIPAddress(hrefURL.host, true)
> +          || hrefURL.host;

|| on the previous line please (yes, old code had it too)

@@ +259,5 @@
> +                          [displayedURI.host, actualURI.host], 2);
> +        
> +        warningButtons = (nsIPS.BUTTON_POS_0) * (nsIPS.BUTTON_TITLE_IS_STRING) + (nsIPS.BUTTON_POS_1) * (nsIPS.BUTTON_TITLE_IS_STRING) + (nsIPS.BUTTON_POS_2) * (nsIPS.BUTTON_TITLE_IS_STRING) + nsIPS.BUTTON_POS_2_DEFAULT;
> +        button0Text = bundle.getFormattedString("confirmPhishingGoAhead",
> +                                                    [actualURI.host], 1);

The button positions are wrong atm.
Usually on linux (and osx I see) you'd have

 [cancel]    [ B ] [ A ]

Where A is the default.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +537,3 @@
>  #LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
>  confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
> +#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host. 

trailing space, here and a few other places

@@ +537,4 @@
>  #LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
>  confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
> +#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host. 
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.

I would remove this newline

Also, should that be another site than *what*...

@@ +539,5 @@
> +#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host. 
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.
> +#LOCALIZATION NOTE $1$S is the host name of the indicated host.
> +confirmPhishingGoAhead=Go to %1$S anyway
> +confirmPhishingGetOut=Get me out of here!

Why no just Cancel? There's a standard button for it

Comment 35

[Magnus Melin [:mkmelin]]

Josiah, any plans to finish this? I'd like us to get this in for 68.

Comment 36

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin [:mkmelin] from comment #35)

Josiah, any plans to finish this? I'd like us to get this in for 68.

Oh, sorry. I had an increase in commitments and then forgot about this. I do plan to finish it. I'll get a new patch to you by Sunday evening.

Comment 37

[Josiah Bruner [:jsbruner]]

Attachment: Patch

Rebased on top of trunk

Comment 38

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin [:mkmelin] from comment #34)

::: mail/base/content/contentAreaClick.js
@@ +30,4 @@

 {
   if (target.hasAttribute("href"))
     href = target.href;

•  linkText = gatherTextUnder(target)
  

this should be inside the if-clause, no?

Well, it doesn't have to be. The point here was that in some cases the href attribute may not exist in target, so to prevent runtime errors we avoid accessing href in such a case. gatherTextUnder(), on the other hand, won't fail, so whatever it returns is fine (empty string or the actual contents).

If you have better suggestion for formatting I'm totally open to it!

::: mail/base/content/phishingDetector.js
@@ +220,5 @@

• const nsIPS = Ci.nsIPromptService;
• // If the message was detected as a scam in the general sense, display a
• // generic warning...
• if (prevDetectedAsScam) {

I hadn't expected this.
Why special case it? If we based the detection on the link mismatch, I'd
like the new linktext choice, but if it's for other reasons (like including
a form), displaying the message here doesn't make sense to me.

Primarily this was for backwards feature compatibility. If (for example) suspicious forms exists, the user will immediately see a red phishing bar. However, the user is still allowed to click on links.

The existing behavior is to, on these link clicks, warn the user that the current message was detected as a scam before they navigate away. I think this is still useful because malicious emails may make form links invisible (for example). We still don't want users to accidentally navigate to a third-party site.

@@ +539,5 @@

+#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host.
+confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.
+#LOCALIZATION NOTE $1$S is the host name of the indicated host.
+confirmPhishingGoAhead=Go to %1$S anyway
+confirmPhishingGetOut=Get me out of here!

Why no just Cancel? There's a standard button for it

The rationale was that the warning may "scare" users and may wonder what "Cancel" actually means. They may think it means ignoring the warning, which is not right. The goal of using "Get me out of here!" is to emphasize that it is a safe option (Firefox used similar text for TLS errors IIRC).

Upon further reflection though, I'm not sure "Get me out of here!" is as clear as I would like. Maybe "Stay Here" or "Don't leave" is clearer?

Comment 39

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin [:mkmelin] from comment #34)

Comment on attachment 9011248 [details] [diff] [review]
Patch

Review of attachment 9011248 [details] [diff] [review]:

@@ +537,4 @@

#LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
+#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host.
+confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.

I would remove this newline

Which newline?

Comment 40

[Josiah Bruner [:jsbruner]]

Attachment: Patch v3

Here's a new patch with most of :mkmelin's comments resolved.

Comment 41

[Josiah Bruner [:jsbruner]]

I just realized that a bug exists in Patch v3 since I moved the buttons around.

However, while resolving this, I figured out why I had the button positions they way I did originally. nsIPromptService says (on Linux) the cancel button is button 1 (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService#Button_default_flags).

Given this, I still think the correct approach (On Linux and Mac) is:

[Go to a.com anyway] [Cancel/Get Me out of Here] [Go to google.com]

New patching incoming...

Comment 42

[Josiah Bruner [:jsbruner]]

Attachment: Patch v4

This resets the button layout to match https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService#Button_default_flags

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 9053183 [details] [diff] [review]
Patch v4

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

Just some small changes. I'll just make them now so we can get landed

::: mail/base/content/phishingDetector.js
@@ +204,5 @@
> +    let button1Text = "";
> +    let button2Text = "";
> +    let titleMsg = "";
> +    let dialogMsg = ""
> +    var hrefURL;

let

@@ +207,5 @@
> +    let dialogMsg = ""
> +    var hrefURL;
> +
> +    let bundle = document.getElementById("bundle_messenger");
> +    const nsIPS = Ci.nsIPromptService;

I think short enough not to need the nsIPS temp

@@ +212,5 @@
> +
> +    // If the message was detected as a scam in the general sense, display a
> +    // generic warning...
> +    if (prevDetectedAsScam) {
> +      var hrefURL;

again?

@@ +221,3 @@
>  
> +      // only prompt for http and https urls
> +      if (hrefURL.schemeIs('http') || hrefURL.schemeIs('https'))

nit: double quotes please

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +546,4 @@
>  #LOCALIZATION NOTE %1$S is the brand name, %2$S is the host name of the url being visited
>  confirmPhishingUrl=%1$S thinks this message is a scam. The links in the message may be trying to impersonate web pages you want to visit. Are you sure you want to visit %2$S?
> +#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host.
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than what the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.

maybe we should put the "This is sometimes ..." into the first part.

@@ +548,5 @@
> +#LOCALIZATION NOTE %1$S is the host name of indicated host, %2$S is the host name of the actual host.
> +confirmPhishingUrlAlternate=The link you just clicked seems to lead to another site than what the link text indicated.\n\nThe link text indicated that the link would lead to %1$S, but it will go to %2$S.\nThis is sometimes used for tracking whether you clicked the link, but could also be a scam.
> +#LOCALIZATION NOTE $1$S is the host name of the indicated host.
> +confirmPhishingGoAhead=Go to %1$S anyway
> +confirmPhishingGetOut=Get me out of here!

I'd still just use Cancel for this. It doesn't really get you anywhere.

Comment 44

[Magnus Melin [:mkmelin]]

Attachment: almost_complete_2.patch

Sent off to try https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bca899727398710c824a94557ae2419d3705ca09

Comment 45

[Magnus Melin [:mkmelin]]

hRefForClickEvent needed some changes. New try

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d5bcae2abc6cd25e2896cc8b301574ca07035e6b

Comment 46

[Magnus Melin [:mkmelin]]

Attachment: almost_complete_2.patch

Good to go. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fdd37a4cf9f2c0fd26c851b3d5cd98b67a64b5c6

Comment 47

[Josiah Bruner [:jsbruner]]

Thanks Magnus!

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9ec42b993238
Only warn the user about possible phishing URLs when they click the link. r=mkmelin