415846 - can't report a page as not actually being a web forgery

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

Since switching to the docshell error page, safebrowsing.setReportPhishingMenu (in browser/components/safebrowsing/content/sb-loader.js) doesn't properly detect if we're looking at a malware/phishing error page, so never properly updates the "Report Web Forgery" menu item.

This check needs to be updated to check for the new docshell error page.

Comment 1

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

This does not block the final release of Firefox 3.

Comment 2

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

Couple ways to solve this:

1. Insert the "This isn't a web forgery" string beside "Ignore This Warning" separated by a |

   This isn't a web forgery | Ignore This Warning

Johnath rightly points out that because we're now fully blocking the content, there's no way for users to know if the page that we're blocking is or isn't a web forgery. I mean, they can be pretty sure, but I bet they'll want to check.

So, his idea was ..:

2. Add a "This isn't a web forgery" button to the infobar we pop up if the warning is ignored.

# Reported Web Forgery!                               ( This isn't a web forgery )

And then he realized he could just do one of those, and also maybe fix the menu code.

Comment 3

[Johnathan Nightingale [:johnath]]

> And then he realized he could just do one of those, and also maybe fix the menu
> code.

Ah, I sort of miscommunicated in IRC, what I was saying was that another option, looking more closely, is just to do neither of those net-new things, and instead just revive sb-loader's code that managed the existing menu items, as seen in FF2.  That would reverse the regression and keep the existing facility available.  At that point, whether or not we do those other things is just gravy, imo.

Comment 4

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

Sure, but user-visible gravy, and Ian Fette mentioned that the data has proven quite useful in the past, so he's asked us to see if we can preserve it.

Comment 5

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

Just so we don't lose this:

http://lxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd#15

Comment 6

[Johnathan Nightingale [:johnath]]

(In reply to comment #4)
> Sure, but user-visible gravy, and Ian Fette mentioned that the data has proven
> quite useful in the past, so he's asked us to see if we can preserve it.

Oh yeah, I definitely agree that we should make it available, and even that it might be useful to surface it more strongly, I'm just saying that at the very least, we should get ourselves back to where we were in FF2.

Comment 7

[Johnathan Nightingale [:johnath]]

Attachment: Restore the correct menu behaviour

First things first - here's the basic regression fix.

Still to-do:

 - Add browser-chrome tests so this doesn't happen again
 - Add the button to the notify bar

Dave, not tagging you for review until there's a complete patch, but if you want to offer any drive by, feel free.  :)

Comment 8

[Brian Ryner]

This seems like a pretty big problem.  Currently there is no easy way for FF3 users to report false positives in the phishing list, so as more users move from FF2 to 3 we'll probably see a lot fewer of these reports.  As Ian mentioned, these are very useful in keeping our feed quality high.

When do you guys think is the soonest that a fix could get in for this?

Comment 9

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

Johnath, is your patch ready for review?

Comment 10

[Ian Fette (Google)]

This bug is really important from Our (Google's) perspective. For one thing, it helps us with data quality if people are able to report false positives. It also makes us (where us == Google and Mozilla both) a lot less likely to face the wrath of angry webmasters if they have some easy, clear way of reporting an error. I don't know if I can get into numbers on this bug, but I can say that a very large percentage of all false positives that we become aware of and remove from the list come through Firefox users reporting a false positive.

Comment 11

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

Comment on attachment 314970 [details] [diff] [review]
Restore the correct menu behaviour

(In reply to comment #9)
> Johnath, is your patch ready for review?

Poke/prod - please get someone to review this ASAP.

Axel: I'm pretty sure that this is kosher from an l10n perspective, but need your sayso.

Ian: I agree that this is important, but I don't think I'd hold the first branch release on it. I'll commit to continuing to push, and I'll take a reviewed patch!

Comment 12

[Johnathan Nightingale [:johnath]]

Comment on attachment 314970 [details] [diff] [review]
Restore the correct menu behaviour

This patch is ready to land except for tests.  I have confirmed that it behaves as expected, but writing an automated test for it is complicated by the fact that the Mac help menu is a decidedly strange beast.

Writing a litmus test would be dirt simple - if drivers are happy with that, then I can mark it in-litmus? and proceed with reviews/landing.  If drivers would prefer an automated test, I can try to write something that at least tests it on windows/linux, where the help menu is a normal menu-popup, and not a platform-specific, automation-unfriendly thing.

This patch should have zero l10n impact unless there are localizations that aren't translating safeb.palm.notforgery.label2.

Comment 13

[Johnathan Nightingale [:johnath]]

To be clear (after conversations in email indicated confusion) this patch only re-enables the menu item when viewing the blocked page.  The discussion in earlier comments is done most cleanly by adding a string to a properties file, so I've opened bug 441624 as a follow-up and nominated it to block firefox-3.1.

In theory it could be done on branch, if we do some magic with the existing string in the dtd, but it's not how we would choose to do it except for the sake of working around a string-freeze, which feels like the wrong reason to commit code.

Comment 14

[Ian Fette (Google)]

Johnathan,

Not having a discoverable way to do this is a big problem for us. Is there any way to get bug 441624 for 3.0.1?

Comment 15

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

So, I'm gonna slice this fine:

This doesn't *block* 3.0.1, but we would really, really like to ship with it, so please expedite reviews and nominate for approval ASAP.

Comment 16

[Ian Fette (Google)]

Given that 3.0.1 seems to be out without this patch, can someone give a prediction of whether this will make 3.0.2? This is important for us for data quality, and searching (for this bug) I found a number of other bugs that boiled down to this...

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 314970 [details] [diff] [review]
Restore the correct menu behaviour

second r=me, I can't sr. Which is one of the reasons this slipped through my review queue over and over, I just don't have the habit to look at sr requests on me. Sorry for that.

Comment 18

[Johnathan Nightingale [:johnath]]

Comment on attachment 314970 [details] [diff] [review]
Restore the correct menu behaviour

I thought someone had landed this, but since they clearly haven't - marking for immediate approval and landing.

Comment 19

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

Comment on attachment 314970 [details] [diff] [review]
Restore the correct menu behaviour

Approved for 1.9.0.2. Please land in CVS. a=ss

To answer comment 12: A Litmus test is fine, for now, but I definitely want to see an automated test as well, even if we have to exclude Mac. Preferably, create one before landing.

Comment 20

[Johnathan Nightingale [:johnath]]

Attachment: With browser-chrome test

The patch code is unaltered, this just adds a browser-chrome test which runs on non-mac platforms, to ensure we don't regress this ever again.  Gonna tag gavin for review, but I want this to make 1.9.0.2 so I'm going to aim to land it by Friday - with the test if gavin can get it reviewed, or without if not, leaving the bug open until I can land the whole thing on branch and trunk.

Comment 21

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

Comment on attachment 333636 [details] [diff] [review]
With browser-chrome test

>diff --git a/browser/components/safebrowsing/content/test/browser_bug415846.js b/browser/components/safebrowsing/content/test/browser_bug415846.js

>+  newBrowser.contentWindow.location = 'http://example.com/';

This maps to the local webserver in the test harness right? (I always forget what the magic hosts are.)

>+  // Now launch the phishing test.  Can't use onload here because error pages don't
>+  // fire normal load events.
>+  newBrowser.contentWindow.location = 'http://www.mozilla.com/firefox/its-a-trap.html';
>+  window.setTimeout(testPhishing, 2000);

Do the other existing tests use 2000? I know we've talked about this but I still kinda hate the timeouts being added in new tests - want to make sure this doesn't introduce any "random" failures. Have we considered adding a magic test harness hostname to the hardcoded phishing blacklist to make this easier? (does the hardcoded phishing blacklist still exist?)

Comment 22

[Johnathan Nightingale [:johnath]]

(In reply to comment #21)
> (From update of attachment 333636 [details] [diff] [review])
> >diff --git a/browser/components/safebrowsing/content/test/browser_bug415846.js b/browser/components/safebrowsing/content/test/browser_bug415846.js
> 
> >+  newBrowser.contentWindow.location = 'http://example.com/';
> 
> This maps to the local webserver in the test harness right? (I always forget
> what the magic hosts are.)

Yeah.  www.example.com doesn't though - imagine my surprise.

> >+  window.setTimeout(testPhishing, 2000);
> 
> Do the other existing tests use 2000? I know we've talked about this but I
> still kinda hate the timeouts being added in new tests - want to make sure this
> doesn't introduce any "random" failures. Have we considered adding a magic test
> harness hostname to the hardcoded phishing blacklist to make this easier? (does
> the hardcoded phishing blacklist still exist?)

We have talked about it, and I think when I get time to fix bug 428948 this should be much easier (since that will almost certainly involve tricking about:blocked into firing some kind of chrome-catchable event.)  I'll happily take a follow-up to replace this timer and the one in browser_bug400731.js once that lands.  :)

How would adding one of those to the hardcoded blacklist make it easier, though?  There would still be non-zero time for "pageload", right?  And still no event fired?


Marking this checkin-needed because the branch tree has been closed and/or orange a lot lately, continues to be presently, and I'm about to be without internet for a few weeks.  I'm going to try to land it this afternoon, but anyone who sees it unlanded after 5PM PDT today should feel invited to land it, and should feel appreciated for doing so.

Comment 23

[Johnathan Nightingale [:johnath]]

Landed on branch - still need to land on trunk

Checking in browser/components/safebrowsing/content/report-phishing-overlay.xul;
/cvsroot/mozilla/browser/components/safebrowsing/content/report-phishing-overlay.xul,v  <--  report-phishing-overlay.xul
new revision: 1.13; previous revision: 1.12
done
Checking in browser/components/safebrowsing/content/sb-loader.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/sb-loader.js,v  <--  sb-loader.js
new revision: 1.29; previous revision: 1.28
done
Checking in browser/components/safebrowsing/content/test/Makefile.in;
/cvsroot/mozilla/browser/components/safebrowsing/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/components/safebrowsing/content/test/browser_bug415846.js,v
done
Checking in browser/components/safebrowsing/content/test/browser_bug415846.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/test/browser_bug415846.js,v  <--  browser_bug415846.js
initial revision: 1.1
done

Comment 24

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

(In reply to comment #22)
> How would adding one of those to the hardcoded blacklist make it easier,
> though?  There would still be non-zero time for "pageload", right?  And still
> no event fired?

Right - all it would do is remove network slowness/latency as a possible cause of timing related failures. You're right that we might as well just fix the core problem instead of investing time in partial workarounds.

Comment 25

[Reed Loden [:reed]]

Pushed as 17105:aa9d9fe4d8b7.

Comment 26

[Marcia Knous [:marcia]]

verified fixed on the 1.9.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008082004 GranParadiso/3.0.2pre. Adding verified keyword.

Also verified fixed on the trunk using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080820020636 Minefield/3.1a2pre.

Comment 27

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5894 added to Litmus.

Comment 28

[Boris Zbarsky [:bzbarsky]]

The test for this bug seems to hit the network... and in particular has been random-orange recently, possibly due to that.