1245992 - Update the Safe Browsing phishing interstitial page

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

Attachment: fx-phishing-interstitial.png

The Safe Browsing list has moved beyond just phishing sites (or "web forgeries").

https://googleonlinesecurity.blogspot.ca/2015/11/safe-browsing-protection-from-even-more.html
https://googleonlinesecurity.blogspot.ca/2016/02/no-more-deceptive-download-buttons.html

We should update our interstitial page to reflect the expanded coverage of that list.

Comment 1

[François Marier [:francois]]

Attachment: cr-phishing-interstitial.png

This is the new phishing interstitial in Chrome.

Comment 2

[François Marier [:francois]]

Here's the current interstitial on Desktop:

> Reported Web Forgery!
> 
> This web page at itisatrap.org has been reported as a web forgery and has
> been blocked based on your security preferences.
> 
> Web forgeries are designed to trick you into revealing personal or financial
> information by imitating sources you may trust.
> 
> Entering any information on this web page may result in identity theft or
> other fraud.

and here's my proposed version:

> Deceptive Site!
> 
> This web page at itisatrap.org has been reported as a deceptive site and has
> been blocked based on your security preferences.
> 
> Deceptive sites are designed to trick you into doing something dangerous
> like installing software or revealing your personal information (for
> example, passwords, phone numbers or credit cards).
> 
> Entering any information on this web page may result in identity theft or
> other fraud.

I would also like to replace the current Fennec wording with the above so
that we end up with only one version of this copy as opposed to the current
situation where the copy between Fennec and Desktop only vary in style but
actually say the same thing.

Any comments/suggestions Matej?

Comment 3

[Matej Novak [:matej]]

Copy looks good, I'd just make a small change to the second paragraph:

Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.

Comment 4

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
The long pole on this change is the localization impact: once we settle on the English text we have to let it ride the full train. The sooner we get it in the sooner we can ship, and even if we changed it today we'll be 3 or 4 months behind the actual behavior change in the list. If we land before the March 7 merge day it will ship June 7; if we miss that the earliest is August 2 (8 week cycle).

Comment 5

[François Marier [:francois]]

Attachment: MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

Review commit: https://reviewboard.mozilla.org/r/35179/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35179/

Comment 6

[François Marier [:francois]]

Attachment: MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

Review commit: https://reviewboard.mozilla.org/r/35181/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35181/

Comment 7

[:Paolo Amadini]

I'll not be able to get to this review this week, you may want to redirect it if you need it to be ready soon.

Comment 8

[François Marier [:francois]]

(In reply to :Paolo Amadini from comment #7)
> I'll not be able to get to this review this week, you may want to redirect
> it if you need it to be ready soon.

Next week would be fine too. I'm aiming to land it before the next uplift to aurora.

If you have someone else in mind though, I'm happy to redirect it.

Comment 9

[François Marier [:francois]]

Panos, is this something you can review? I'd like to make sure it gets into 47.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

https://reviewboard.mozilla.org/r/35179/#review32831

::: browser/base/content/blockedSite.xhtml:203
(Diff revision 1)
> -        <h1 id="errorTitleText_phishing">&safeb.blocked.phishingPage.title;</h1>
> +        <h1 id="errorTitleText_phishing">&safeb.blocked.phishingPage.title2;</h1>

AFAIK common practice is to change the label like this: safeb.blocked.phishingPage2.title

I see that there are other cases in this file where the style you chose is used. I'd like to get flod's opinion on this.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:7
(Diff revision 1)
> -<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgery…">
> +<!-- Label of the Help menu item -->

I believe using the same format as other localization notes will let l10n tools display the comment properly to localizers:

<!-- Localization note (safeb.palm.notdeceptive.label) - Label of the Help menu item -->

::: browser/locales/en-US/chrome/overrides/appstrings.properties:34
(Diff revision 1)
> -phishingBlocked=The website at %S has been reported as a web forgery designed to trick users into sharing personal or financial information.
> +phishingBlocked=This web page at %S has been reported as a deceptive site and has been blocked based on your security preferences.

This string also needs a new label (phishingBlocked2). There are a few instances of this in other files as well.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

https://reviewboard.mozilla.org/r/35181/#review32841

This looks fine to me for Firefox, but I wonder if these strings are still used in Thunderbird. Perhaps Fallen can confirm.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

(In reply to Panos Astithas [:past] from comment #10)
> ::: browser/base/content/blockedSite.xhtml:203
> (Diff revision 1)
> > -        <h1 id="errorTitleText_phishing">&safeb.blocked.phishingPage.title;</h1>
> > +        <h1 id="errorTitleText_phishing">&safeb.blocked.phishingPage.title2;</h1>
> 
> AFAIK common practice is to change the label like this:
> safeb.blocked.phishingPage2.title
> 
> I see that there are other cases in this file where the style you chose is
> used. I'd like to get flod's opinion on this.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 8719978 [details]
> MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r?paolo
> 
> https://reviewboard.mozilla.org/r/35181/#review32841
> 
> This looks fine to me for Firefox, but I wonder if these strings are still
> used in Thunderbird. Perhaps Fallen can confirm.

Comment 14

[Francesco Lodolo [:flod]]

https://reviewboard.mozilla.org/r/35179/#review32843

There's no practical different between teststring2.shortDesc and teststring.shortDesc2, the only case where I would ask you to not put the number at the end is for accesskeys/shortcuts (e.g. teststring2.accesskey is a better than teststring.accesskey2).

Both Panos' comments are correct, especially the one about renaming phishingBlocked

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:7
(Diff revision 1)
> -<!ENTITY safeb.palm.notforgery.label2 "This isn't a web forgery…">
> +<!-- Label of the Help menu item -->

Yes please, use the format used in the following comments.

<!-- Localization note (stringID) - ... -->

::: browser/locales/en-US/chrome/overrides/appstrings.properties:34
(Diff revision 1)
> -phishingBlocked=The website at %S has been reported as a web forgery designed to trick users into sharing personal or financial information.
> +phishingBlocked=This web page at %S has been reported as a deceptive site and has been blocked based on your security preferences.

This definitely needs a new ID.

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

Mostly minor issues, but the string in appstrings.properties needs a new id.

Comment 16

[François Marier [:francois]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35179/diff/1-2/

Comment 17

[François Marier [:francois]]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35181/diff/1-2/

Comment 18

[François Marier [:francois]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

I've pushed a new revision which should address all of your review comments. It's different enough that I'd prefer if you took another quick look to make sure I didn't do anything stupid. Thanks!

Comment 19

[Francesco Lodolo [:flod]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

https://reviewboard.mozilla.org/r/35179/#review33077

Strings looks good, thanks.

(newbie in action using "ship it" on MozReview, let's hope it doesn't steal Panos' review flag...)

Comment 20

[Francesco Lodolo [:flod]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

Urgh, fixing flags.

Side question: is there an equivalent bug for Fennec? Can't spot one in the dependency tree of bug 1149867.

Comment 21

[François Marier [:francois]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> Side question: is there an equivalent bug for Fennec? Can't spot one in the
> dependency tree of bug 1149867.

This bug actually covers both Desktop and Fennec. I tested both yesterday.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6eea401e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9614f87daf

Comment 23

[Phil Ringnalda (:philor)]

Backed both out in https://hg.mozilla.org/integration/mozilla-inbound/rev/36f8f2d29af6 - there's several browser-chrome tests which apparently depend on that page, https://treeherder.mozilla.org/logviewer.html#?job_id=22445051&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=22445055&repo=mozilla-inbound.

Comment 24

[Phil Ringnalda (:philor)]

And https://treeherder.mozilla.org/logviewer.html#?job_id=22456607&repo=mozilla-inbound, I thought there were (at least) three.

Comment 25

[François Marier [:francois]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35179/diff/2-3/

Comment 26

[François Marier [:francois]]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35181/diff/2-3/

Comment 27

[François Marier [:francois]]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

Removing these unused entities fixed the (unrelated) tests that broke and caused the backout. The other patch didn't change.

Comment 28

[Francesco Lodolo [:flod]]

Comment on attachment 8719977 [details]
MozReview Request: Bug 1245992 - Update the Safe Browsing phishing interstitial page. r=flod,past

https://reviewboard.mozilla.org/r/35179/#review33343

Thanks, strings looks good, and now the mobile part makes sense.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8719978 [details]
MozReview Request: Bug 1245992 - Remove unused Safe Browsing strings. r=past

https://reviewboard.mozilla.org/r/35181/#review33363

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e157cde861c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b81e1b1dcb3

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e157cde861c
https://hg.mozilla.org/mozilla-central/rev/2b81e1b1dcb3

Comment 32

[Ton [:Tonnes]]

(In reply to Carsten Book [:Tomcat] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/4e157cde861c
> +deceptiveBlocked=This web page at %S has been reported as a deceptive site and has been blocked based on your security preferences.

Sorry for mentioning this here, spotted during l10n. This line seems to be wrong for all 4 identical and their related occurences.

A page and a site are 2 totally different things, so the above is simply incorrect.

* A website is a collection of web pages.
* A browser is used for viewing web pages.

These are definitions all devs should know and keep in mind (and I haven’t made them up). For that reason, viewing an entire website would take weeks if all pages were to be viewed.
Furthermore, there were 3 identical strings starting with "The site at %S" in these files.

So either I would use
> deceptiveBlocked=The site at %S has been reported as a deceptive site and has been blocked based on your security preferences.
or
> deceptiveBlocked=The web page at %S has been reported as a deceptive page and has been blocked based on your security preferences. (And similar instances of "page" elsewhere.)

https://transvision.mozfr.org/?recherche=deceptiveblocked&repo=aurora&sourcelocale=en-US&locale=en-US&search_type=entities

Could anyone fix this (maybe even for 47) and keep the definition in mind?

Also note the present inconsistent use of "page" and "site" for such error messages:
https://transvision.mozfr.org/?recherche=reported+as&repo=aurora&sourcelocale=en-US&locale=en-US&search_type=strings

In most cases, these are about pages (hosted on sites allright) that are blocked, not entire sites. Or to put it simple: a page hosting unwanted software or attacking users doesn’t make an entire site bad.

Maybe it’s good to review all of them while at it?