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

VERIFIED FIXED in Firefox 3 beta1

Status

()

Toolkit
Safe Browsing
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: johnath, Assigned: johnath)

Tracking

(Blocks: 1 bug, {relnote})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
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?

Comment 1

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

Updated

10 years ago
Flags: in-litmus?
Blocks M9 if M9 is to be beta.
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Comment 3

10 years ago
Created attachment 284961 [details] [diff] [review]
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?
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED

Updated

10 years ago
Blocks: 349054
(Assignee)

Updated

10 years ago
Blocks: 400324
(Assignee)

Comment 4

10 years ago
Created attachment 285406 [details] [diff] [review]
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)
Attachment #284961 - Attachment is obsolete: true

Comment 5

10 years ago
If browser is contributing both the error page and the pref pointing there, the about: implementation should probably move there too.
(Assignee)

Comment 6

10 years ago
Created attachment 285477 [details] [diff] [review]
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.
Attachment #285406 - Attachment is obsolete: true
(Assignee)

Comment 7

10 years ago
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 8

10 years ago
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+
(Assignee)

Comment 9

10 years ago
Created attachment 285755 [details] [diff] [review]
(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.
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.
(Assignee)

Updated

10 years ago
Blocks: 400728
(Assignee)

Updated

10 years ago
Blocks: 400731
(Assignee)

Comment 12

10 years ago
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)

Updated

10 years ago
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+
(Assignee)

Comment 14

10 years ago
Created attachment 286315 [details] [diff] [review]
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.
Attachment #285755 - Attachment is obsolete: true
Attachment #286315 - Flags: ui-review?
(Assignee)

Comment 15

10 years ago
Created attachment 286321 [details]
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.
Attachment #286321 - Flags: ui-review?(beltzner)
+            errorPage.AssignASCII(alternateErrorPage);

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

Updated

10 years ago
Blocks: 387524
Whiteboard: [has patch][need review biesi] → [has patch][needs ui-review beltzner]
Attachment #286321 - Flags: ui-review?(beltzner) → ui-review+
Attachment #286315 - Flags: ui-review?
(Assignee)

Comment 17

10 years ago
Created attachment 286554 [details] [diff] [review]
Change the AssignASCII call as mentioned in comment 16
Attachment #286315 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
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
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 401642
Depends on: 401645
(Assignee)

Updated

10 years ago
Blocks: 402370

Updated

10 years ago
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

Comment 19

10 years ago
Hardcoded test page: http://mozilla.com/firefox/its-an-attack.html
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

Updated

10 years ago
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+

Updated

10 years ago
Depends on: 409856

Updated

9 years ago
Depends on: 417687

Updated

9 years ago
Depends on: 426067

Updated

9 years ago
Depends on: 427938

Updated

9 years ago
Depends on: 428948
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.