420751 - Update blockedSite text, use better stopbadware link

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Johnathan Nightingale [:johnath]]

After a conversation with the stopbadware.org folks, a couple of changes have been suggested to our blocked-page strings:

1) Call out more explicitly the possibility of a hacked site, to avoid the "I know this site is trustworthy" response.  Perhaps something like:

Some badware web sites intentionally distribute harmful software, while many others have been compromised without the knowledge or permission of their owners.

2) Use stopbadware's report page, for more detailed information about a blocked site, and to more easily request review.

http://www.stopbadware.org/reports/container?source=firefox3&reportname=example.com

Comment 1

[Johnathan Nightingale [:johnath]]

Attachment: first run

Screenshots and ui-r request to follow.

Comment 2

[Johnathan Nightingale [:johnath]]

Attachment: updated look and strings

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307316 [details]
updated look and strings

nits:

Malware:
 - none!

Phishing:
 - mirror the shortDescription text: "The web site at %s has been reported as a web forgery and has been blocked based on your security preferences."
 - make the first paragraph of longDescription about the consequences: "Web forgeries are designed to trick you into revealing personal or financial information by imitating sources you may trust."
 - make the second paragraph of longDescription: "Entering any information on this web page may result in identity theft or other fraud."

(it should be noted that Johnath did most of this ui-rework himself!)

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: With UI-nits

String changes (and entity revs) in.  Over to gavin for code review.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 307804 [details] [diff] [review]
With UI-nits

>Index: browser/app/profile/firefox.js

>+// URL for checking the reason for a malware warning.
>+pref("browser.safebrowsing.malware.reportURL", "http://www.stopbadware.org/reports/container?source=firefox3&reportname=");

It always worries me a bit when we hardcode version numbers (e.g. "source=firefox3"), because we might not remember to update it before Firefox 4. Do we want to make this a redirect through mozilla.com, in case we need to change the URL for some reason? Perhaps I'm overthinking this, but it might be nice to eventually offer a way to get localized information here.

Should there be some kind of indication in the text that this will send the URL you tried to access to a third party, for the privacy conscious?

>Index: browser/base/content/browser.js

>+    else if (/^about:blocked/.test(event.originalTarget.ownerDocument.documentURI)) {

Seems like you should stick event.originalTarget.ownerDocument.documentURI into a temporary given how many times it's used.

>+      // The event came from a button on a malware/phishing block page
>+      var ot = event.originalTarget;
>+      var errorDoc = ot.ownerDocument;

Could factor this out to avoid repeating it, too.

>+function getMeOutOfHere() {

>+  content.location = url;

Should this set ownerDocument.location, so that clicking the button in blocked pages in frames only loads the new URL in the frame? It doesn't appear to be possible to get a cert error to appear in a frame, but the malware/phishing pages can, and this would introduce a change in behavior since the previous window.home() method resulted in only the frame changing, instead of the entire page. Not sure whether it makes sense to maintain the outer frame in these cases... what are the odds that the outer site is also "bad"? This issue also applies to the existing code in BrowserOnCommand, but it currently changes content.location, so this patch isn't changing that (and we didn't change it when fixing bug 407369).

>Index: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd

>+<!ENTITY safeb.palm.report "Why was this site blocked?">

report.label, to match the others?

Comment 6

[Johnathan Nightingale [:johnath]]

Attachment: Address review comments, change reporting url

(In reply to comment #5)
> (From update of attachment 307804 [details] [diff] [review])
> >Index: browser/app/profile/firefox.js
> 
> >+// URL for checking the reason for a malware warning.
> >+pref("browser.safebrowsing.malware.reportURL", "http://www.stopbadware.org/reports/container?source=firefox3&reportname=");
> 
> It always worries me a bit when we hardcode version numbers (e.g.
> "source=firefox3"), because we might not remember to update it before Firefox
> 4. Do we want to make this a redirect through mozilla.com, in case we need to
> change the URL for some reason? Perhaps I'm overthinking this, but it might be
> nice to eventually offer a way to get localized information here.

I took this question to the stopbadware folks, and after a couple rounds we came up with the URL in the current patch, which pulls app name and version strings similarly to what we do with user agents here:

http://mxr.mozilla.org/mozilla/source/browser/app/profile/firefox.js#176

> Should there be some kind of indication in the text that this will send the URL
> you tried to access to a third party, for the privacy conscious?

I don't really think we need one - this is a voluntary action for extra information, and someone concerned for their privacy will see very quickly what's going on.  The only lingering concern I have is that it might be more obvious if it were a link, vs. a button, but yeah, I don't see it as a problem.

> >Index: browser/base/content/browser.js
> 
> >+    else if (/^about:blocked/.test(event.originalTarget.ownerDocument.documentURI)) {
> 
> Seems like you should stick event.originalTarget.ownerDocument.documentURI into
> a temporary given how many times it's used.
> 
> >+      // The event came from a button on a malware/phishing block page
> >+      var ot = event.originalTarget;
> >+      var errorDoc = ot.ownerDocument;
> 
> Could factor this out to avoid repeating it, too.

Yeah, and once I did that, I realised that event.originalTarget.ownerDocument.documentURI is really just errorDoc.documentURI.

> >+function getMeOutOfHere() {
> 
> >+  content.location = url;
> 
> Should this set ownerDocument.location, so that clicking the button in blocked
> pages in frames only loads the new URL in the frame? It doesn't appear to be
> possible to get a cert error to appear in a frame, but the malware/phishing
> pages can, and this would introduce a change in behavior since the previous
> window.home() method resulted in only the frame changing, instead of the entire
> page. Not sure whether it makes sense to maintain the outer frame in these
> cases... what are the odds that the outer site is also "bad"? This issue also
> applies to the existing code in BrowserOnCommand, but it currently changes
> content.location, so this patch isn't changing that (and we didn't change it
> when fixing bug 407369).

Yeah, this is a deliberate change.  If a user asks for elaboration on a framed blacklisted page, I think we should take the whole content area - wherever they were was framing bad content, so they certainly SEEM unfriendly, but it's not as though they lose access to it - back button still works. 

> >Index: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd
> 
> >+<!ENTITY safeb.palm.report "Why was this site blocked?">
> 
> report.label, to match the others?

Good call, done.

Comment 7

[Johnathan Nightingale [:johnath]]

Comment on attachment 307966 [details] [diff] [review]
Address review comments, change reporting url

requesting a+ for late string change

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307966 [details] [diff] [review]
Address review comments, change reporting url

a1.9=beltzner

Comment 9

[Johnathan Nightingale [:johnath]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.304; previous revision: 1.303
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.994; previous revision: 1.993
done
Checking in browser/locales/en-US/chrome/browser/safebrowsing/blockedSite.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/safebrowsing/blockedSite.properties,v  <--  blockedSite.properties
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd,v  <--  phishing-afterload-warning-message.dtd
new revision: 1.13; previous revision: 1.12
done
Checking in browser/components/safebrowsing/content/blockedSite.xhtml;
/cvsroot/mozilla/browser/components/safebrowsing/content/blockedSite.xhtml,v  <--  blockedSite.xhtml
new revision: 1.2; previous revision: 1.1
done

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Verified updated (and it has been updated again with the Larry graphic as well) in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre.