399233 - replace safebrowsing (phishing) bubble with error page (Phishing UI hidden by content, inconsistent with malware)

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Johnathan Nightingale [:johnath]]

The current safebrowsing bubble is hidden by content, as a symptom of bug 341950.  Additionally, it's now inconsistent with the malware UI introduced in bug 380932.

After conversations with mconnor and biesi, it feels like the right thing to do here is to move malware and phishing to a common error page implementation in the short term.  When bug 398776 lands, both can then be moved to the "doorhanger" implementation, but imho we shouldn't let our phishing protection go to beta broken, while blocking on doorhanger.

Bug 341950 was originally marked blocking because of the anti-phishing impact, as I understand it, so I am nominating this one for blocking, which may or may not mean clearing the flag on the other, depending on how the drivers assess the rest of that regression.

Comment 1

[Tony Chang (Google)]

This will be a good time to remove all the code associated with the phishing popup bubble, which is really buggy.

Comment 2

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

Blocks M9 if M9 is to be beta.

Comment 3

[Johnathan Nightingale [:johnath]]

Attachment: WIP patch

Attached patch in progress - this creates a new error page in browser/components/safebrowsing, since that's where the rest of the browser UI for anti-phishing hangs out, as mentioned.

TODO:

 - finish hooking in to the docshell and toolkit code that handles the display of this page - will get dave camp to help here

POSSIBLY TODO:

 - add service to get "last updated time" and add that information to the page
 - add support for "go to the site anyhow" which is functionality currently regressed out by this error page
 - remove existing safebrowsing UI.  The potential for regression might make this safer to do as a separate patch?

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: Working patch

This is a complete patch, though it may require clean-up.

- the new error page is added as about:blocked
- docshell checks the "urlclassifier.alternate_error_page" pref to find the location, and avoid a dependency on browser/.  If the pref is not present, it falls back to netError.
- the appropriate fall back strings have been added to netError
- the speech bubble UI is neutralized but not removed (see bug 400324)

Comment 5

[Dave Camp (:dcamp)]

If browser is contributing both the error page and the pref pointing there, the about: implementation should probably move there too.

Comment 6

[Johnathan Nightingale [:johnath]]

Attachment: Move about:blocked work out of toolkit

(In reply to comment #5)
> If browser is contributing both the error page and the pref pointing there, the
> about: implementation should probably move there too.

You're right, of course, and that also helpfully means the patch barely touches toolkit at all.  about: Implementation is now entirely in safebrowsing code.

Comment 7

[Johnathan Nightingale [:johnath]]

Comment on attachment 285477 [details] [diff] [review]
Move about:blocked work out of toolkit

Tony - are you able to review this?  If need be, feel free to review only the safebrowsing changes, and I'll find a docshell peer for that portion.

Comment 8

[Tony Chang (Google)]

Comment on attachment 285477 [details] [diff] [review]
Move about:blocked work out of toolkit

I would be in favor of putting safeb.palm.warning.title in netError.dtd or blockedSite.properties instead of changing the string in phishing-afterload-warning-message.dtd. I think it'll be safe to remove phishing-afterload-warning-message.dtd after this change, but understand doing that in a follow up CL.

Nit: Please add a space between if and open paren in nsDocShell.cpp.

Nit: Do we need errorPage and error strings?  Can't we just use one string?

Comment 9

[Johnathan Nightingale [:johnath]]

Attachment: (Most) Nits addressed

(In reply to comment #8)
> (From update of attachment 285477 [details] [diff] [review])
> I would be in favor of putting safeb.palm.warning.title in netError.dtd or
> blockedSite.properties instead of changing the string in
> phishing-afterload-warning-message.dtd. I think it'll be safe to remove
> phishing-afterload-warning-message.dtd after this change, but understand doing
> that in a follow up CL.

Fixed.  Malware/Phishing each re-use the in-page title text as the document.title.

> Nit: Please add a space between if and open paren in nsDocShell.cpp.

Fixed in two places

> Nit: Do we need errorPage and error strings?  Can't we just use one string?

I'm easy either way - this way makes the "custom" string shorter, the other way saves us a variable - I'm gonna tag bz to review the non-browser components, we'll go with whichever he prefers.

Comment 10

[Reed Loden [:reed]]

Comment on attachment 285755 [details] [diff] [review]
(Most) Nits addressed

>+    <script type="application/x-javascript" src="chrome://global/content/strres.js"/>
>+    <script type="application/x-javascript"><![CDATA[
...
>+    <script type="application/x-javascript">initPage();</script>

We're trying to get rid of "application/x-javascript" in the codebase (bug 381467), so please use "application/javascript".

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this, but I have a lot of other things on my plate right now...  You might want to try biesi for the review.

Comment 12

[Johnathan Nightingale [:johnath]]

Comment on attachment 285755 [details] [diff] [review]
(Most) Nits addressed

swapping over to biesi per bz's request.  (And actually, I've spoken to biesi about some of this stuff in the past anyhow.)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 285755 [details] [diff] [review]
(Most) Nits addressed

+        char *alternateErrorPage = nsnull;

use an nsXPIDLCString instead, and getter_Copies

that has the advantage that you don't leak the string :)

actually maybe just getter_Copies(alternateErrorPage) works

Comment 14

[Johnathan Nightingale [:johnath]]

Attachment: Use nsXPIDLCString and getter_Copies

Figure I might as well do both, rather than leaving it as a char *. This should be ready for checkin, but I see that there isn't an explicit ui-r anywhere, so I'll run it by beltzner first.

Comment 15

[Johnathan Nightingale [:johnath]]

Attachment: Screencaps of before and after

The malware change is pretty minor (just the addition of a "Get me out of here" button as a result of picking up the same page as phishing) and the phishing text is largely similar, though not identical.

Two other things to note:
 - Unlike Firefox 2, the new phishing error does not allow clickthrough.  This is not a deliberate design decision, it's a limitation we pick up as a result of doing the stronger, docshell-level, no-redirects, no page-load blocking.  There isn't currently a way to tell docshell "ignore the url-classifier for this one page load," so I've opened bug 400731.
 - The new page is intended to have a "Last updated" line, to tell users how stale their blacklist data is.  This requires an interface that doesn't currently exist, so I've opened bug 400728.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

+            errorPage.AssignASCII(alternateErrorPage);

just make that Assign to avoid the implicit conversion to const char* you otherwise get

Comment 17

[Johnathan Nightingale [:johnath]]

Attachment: Change the AssignASCII call as mentioned in comment 16

Comment 18

[Reed Loden [:reed]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.215; previous revision: 1.214
done
Checking in browser/components/safebrowsing/jar.mn;
/cvsroot/mozilla/browser/components/safebrowsing/jar.mn,v  <--  jar.mn
new revision: 1.6; previous revision: 1.5
done
Checking in browser/components/safebrowsing/content/application.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/application.js,v  <--  application.js
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/browser/components/safebrowsing/content/blockedSite.xhtml,v
done
Checking in browser/components/safebrowsing/content/blockedSite.xhtml;
/cvsroot/mozilla/browser/components/safebrowsing/content/blockedSite.xhtml,v  <--  blockedSite.xhtml
initial revision: 1.1
done
Checking in browser/components/safebrowsing/content/malware-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/malware-warden.js,v  <--  malware-warden.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/safebrowsing/content/phishing-afterload-displayer.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-afterload-displayer.js,v  <--  phishing-afterload-displayer.js
new revision: 1.28; previous revision: 1.27
done
Checking in browser/components/safebrowsing/src/nsSafebrowsingApplication.js;
/cvsroot/mozilla/browser/components/safebrowsing/src/nsSafebrowsingApplication.js,v  <--  nsSafebrowsingApplication.js
new revision: 1.10; previous revision: 1.9
done
Checking in browser/locales/jar.mn;
/cvsroot/mozilla/browser/locales/jar.mn,v  <--  jar.mn
new revision: 1.73; previous revision: 1.72
done
RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/safebrowsing/blockedSite.properties,v
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
initial revision: 1.1
done
Checking in browser/locales/en-US/chrome/overrides/appstrings.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/overrides/appstrings.properties,v  <--  appstrings.properties
new revision: 1.9; previous revision: 1.8
done
Checking in browser/locales/en-US/chrome/overrides/netError.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/overrides/netError.dtd,v  <--  netError.dtd
new revision: 1.10; previous revision: 1.9
done
Checking in camino/embed-replacements/locale/en-US/global/netError.dtd;
/cvsroot/mozilla/camino/embed-replacements/locale/en-US/global/netError.dtd,v  <--  netError.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.863; previous revision: 1.862
done
Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.216; previous revision: 1.215
done
Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.697; previous revision: 1.696
done
Checking in dom/locales/en-US/chrome/appstrings.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/appstrings.properties,v  <--  appstrings.properties
new revision: 1.6; previous revision: 1.5
done
Checking in dom/locales/en-US/chrome/netError.dtd;
/cvsroot/mozilla/dom/locales/en-US/chrome/netError.dtd,v  <--  netError.dtd
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in uriloader/base/nsURILoader.h;
/cvsroot/mozilla/uriloader/base/nsURILoader.h,v  <--  nsURILoader.h
new revision: 1.28; previous revision: 1.27
done

Comment 19

[Steffen Wilberg]

Hardcoded test page: http://mozilla.com/firefox/its-an-attack.html

Comment 20

[Bill Gianopoulos [:WG9s]]

With this checkin, phishing sites identified in the GOOGLE data base are no longer being detected.  Sample failure url:

http://rngrmyk.com/images/yahoo_login/YAHOOPHO.HTM

see bug 402435.

Comment 21

[Samuel Sidler (old account; do not CC)]

v. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110610 Minefield/3.0a9pre

Comment 22

[Marcia Knous [:marcia]]

Litmus triage team: tomcat will handle Litmus testcase.

Comment 23

[Carsten Book [:Tomcat]]

the testcases for the new safebrowsing are now created in litmus.