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

VERIFIED FIXED in Firefox 3 beta1

Status

()

defect
VERIFIED FIXED
12 years ago
5 years ago

People

(Reporter: johnath, Assigned: johnath)

Tracking

(Blocks 1 bug, {relnote})

Trunk
Firefox 3 beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

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.
Flags: blocking-firefox3?
This will be a good time to remove all the code associated with the phishing popup bubble, which is really buggy.
Flags: in-litmus?
Blocks M9 if M9 is to be beta.
Flags: blocking-firefox3? → blocking-firefox3+
Posted patch WIP patch (obsolete) — Splinter Review
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?
Status: NEW → ASSIGNED
Blocks: 349054
Blocks: 400324
Posted patch Working patch (obsolete) — Splinter Review
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)
Attachment #284961 - Attachment is obsolete: true
If browser is contributing both the error page and the pref pointing there, the about: implementation should probably move there too.
(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.
Attachment #285406 - Attachment is obsolete: true
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.
Attachment #285477 - Flags: review?(tony)
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?
Attachment #285477 - Flags: review?(tony) → review+
Posted patch (Most) Nits addressed (obsolete) — Splinter Review
(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.
Attachment #285477 - Attachment is obsolete: true
Attachment #285755 - Flags: review?(bzbarsky)
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".
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.
Blocks: 400728
Blocks: 400731
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.)
Attachment #285755 - Flags: review?(bzbarsky) → review?(cbiesinger)
Whiteboard: [has patch][need review biesi]
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
Attachment #285755 - Flags: review?(cbiesinger) → review+
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.
Attachment #285755 - Attachment is obsolete: true
Attachment #286315 - Flags: ui-review?
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.
Attachment #286321 - Flags: ui-review?(beltzner)
+            errorPage.AssignASCII(alternateErrorPage);

just make that Assign to avoid the implicit conversion to const char* you otherwise get
Blocks: 387524
Whiteboard: [has patch][need review biesi] → [has patch][needs ui-review beltzner]
Attachment #286321 - Flags: ui-review?(beltzner) → ui-review+
Keywords: checkin-needed
Whiteboard: [has patch][needs ui-review beltzner]
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 401642
Depends on: 401645
Blocks: 402370
Keywords: relnote
Summary: Phishing UI hidden by content, inconsistent with malware → replace safebrowsing (phishing) bubble with error page (Phishing UI hidden by content, inconsistent with malware)
Depends on: 402435
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.
v. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110610 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Depends on: 403055
Depends on: 405685
Litmus triage team: tomcat will handle Litmus testcase.
the testcases for the new safebrowsing are now created in litmus.
Flags: in-litmus? → in-litmus+
Depends on: 409856
Depends on: 417687
Depends on: 426067
Depends on: 427938
Depends on: 428948
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.