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.
This will be a good time to remove all the code associated with the phishing popup bubble, which is really buggy.
Blocks M9 if M9 is to be beta.
Flags: blocking-firefox3? → blocking-firefox3+
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?
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
If browser is contributing both the error page and the pref pointing there, the about: implementation should probably move there too.
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
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+
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.
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 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)
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+
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.
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
Whiteboard: [has patch][need review biesi] → [has patch][needs ui-review beltzner]
Attachment #286321 - Flags: ui-review?(beltzner) → ui-review+
Created attachment 286554 [details] [diff] [review] Change the AssignASCII call as mentioned in comment 16
Attachment #286315 - Attachment is obsolete: true
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: 11 years ago
Resolution: --- → FIXED
Summary: Phishing UI hidden by content, inconsistent with malware → replace safebrowsing (phishing) bubble with error page (Phishing UI hidden by content, inconsistent with malware)
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
Litmus triage team: tomcat will handle Litmus testcase.
the testcases for the new safebrowsing are now created in litmus.
Flags: in-litmus? → in-litmus+
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.