When reporting URLs to Google using one of these reporting pages: browser.safebrowsing.reportMalwareMistakeURL browser.safebrowsing.reportPhishMistakeURL browser.safebrowsing.reportPhishURL We appear to be using the page URL: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/toolkit/components/url-classifier/SafeBrowsing.jsm#155 through this function: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/browser-safebrowsing.js#48-50 called in all of these places: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/browser.js#3008 https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/browser.js#3017 https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/report-phishing-overlay.xul#25 https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/report-phishing-overlay.xul#32 This means that our users are sending Google lots of unhelpful / bogus reports. Since they use these voluntary reports to make the Safe Browsing service better (by fixing false positives and adding missing phishing sites), we should ensure that we include the URL that matched the Safe Browsing list whenever we draft a report.

We already have a bug for false negatives (missing phishing sites) so let's address only the false positives in this bug.

When we find a good way to keep the matching URL around, we may be able to also keep track of the provider or the list name(s) that matched.

As I understand, what need to be handled here is the false positive case, which is "sites" in the phishing list are safe in fact. Firefox Users help to report these sites using the menu "This is not deceptive sites" in Help menu. (basically this menu only appears when user is in phishing warning page). If I understand correctly, I think we used URL to report mistakePhishing correctly. Indeed, channel will be cancelled immediately if it's Url matches Safe Browsing list. I am not quite familiar to DocShell, but still do dome code tracings in the case warning page is shown. The flow is OnStateChange [1] -> EndPageLoad() [2] -> DisplayLoadError() [3] -> LoadErrorPage() in [4] [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7309 [2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7552 [3] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7916 [4] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5244 The channel's uri (which matches phishing list) is passed to LoadErrorPage() then to mFailedURI then to mCurrentURI when OnNewURI() is called in [6] [5] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5269 [6] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9117 We use the currentURI in [7] to report mistakePhishing site [7] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-safebrowsing.js#49 Look into nsDoShell, this points to mCurrentURI in [8] [8] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5512 So, basically, we are using correct URL to report "mistake phishing site" to server.

That's a quite good idea to keep matching URL around to use later(or provider, list info). Imbo, there're 2 ways to do: - 1 - Create a new idl interface/service to store match URL if nsChannelClassifier catch it. Channel's URL is stored every time the URL breaks the Safe Browsing rule. But I am still confused when we remove the stored url - 2 - When we are going to report an URL which is "mistake phishing site", we will look into our hashstore, do lookup/scanning again and then receive whatever information we need. ( I don't know if it's good idea) A little bit tricky in this bug, perhaps we should do the above idea in another bug, such as bug 1181955. Do you have any thoughts?

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #3) > As I understand, what need to be handled here is the false positive case, > which is "sites" in the phishing list are safe in fact. Firefox Users help > to report these sites using the menu "This is not deceptive sites" in Help > menu. (basically this menu only appears when user is in phishing warning > page). That's one way. There are two other ways: 1. Go to http://itisatrap.org/firefox/its-a-trap.html 2. Click the "Ignore this warning" link. 3. Click the "This isn't a deceptive site.. " button in the banner at the top. or 1. Go to http://itisatrap.org/firefox/its-an-attack.html 2. Click the "Ignore this warning" link. 3. Click the "This isn't an attack site.. " button in the banner at the top. > So, basically, we are using correct URL to report "mistake phishing site" to > server. That's only in the first case. If you ignore a warning and report the mistake from the top bar, it will give Google the page URL. Here's how to reproduce that problem: 1. Run Apache on your machine at 127.0.0.1 2. Use the following Apache config: <VirtualHost localhost:80> ServerName testsafebrowsing.appspot.com DocumentRoot /var/www/html Redirect "/s/phishing.html" "http://example.com" </VirtualHost> 3. Redirect the testsafebrowsing.appspot.com test page to your machine by putting the following in /etc/hosts: 127.0.0.1 testsafebrowsing.appspot.com 4. Visit http://testsafebrowsing.appspot.com/s/phishing.html in a browser and click "Ignore this warning" then "This isn't a deceptive site...". 5. Notice that the URL that would be reported to Google is http://example.com. The other possibility would be if we are loading a page which has malware/phishing iframes (see bug 1195242). The page itself wouldn't be on the list but the iframes would and when we ignore the warning and report the page, we would be reporting the page URL not the iframe. Here's a test page for this: https://people.mozilla.org/~fmarier/test-safebrowsing-mistake-reporting.html but for it to work, you need to set: security.mixed_content.block_active_content = false

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #4) > That's a quite good idea to keep matching URL around to use later(or > provider, list info). Imbo, there're 2 ways to do: > - 1 - Create a new idl interface/service to store match URL if > nsChannelClassifier catch it. Channel's URL is stored every time the URL > breaks the Safe Browsing rule. But I am still confused when we remove the > stored url > - 2 - When we are going to report an URL which is "mistake phishing site", > we will look into our hashstore, do lookup/scanning again and then receive > whatever information we need. ( I don't know if it's good idea) > > A little bit tricky in this bug, perhaps we should do the above idea in > another bug, such as bug 1181955. > Do you have any thoughts? The method you suggested would work but it seems a little heavyweight. Is there a way we can attach the following information to the channel we just cancelled? - URL that matched on the Safe Browsing list - name of the Safe Browsing list that matched (e.g. goog-phish-shavar)

MozReview-Commit-ID: 8SlpXinhXQx

Thanks Francois for your clear steps. Could you please take a look at the patches? The patches add support of classified channel interface to HttpBaseChannel, then later use it to query matched URL information. Hi Patrick, could you please take a look at necko part? Thanks you

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review71880 ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:12 (Diff revision 1) > const TRACKING_TABLE_NAME = "mochitest-track-simple"; > const TRACKING_TABLE_PREF = "urlclassifier.trackingTable"; > const WHITELIST_TABLE_NAME = "mochitest-trackwhite-simple"; > const WHITELIST_TABLE_PREF = "urlclassifier.trackingWhitelistTable"; > > +const PHISHING_TABLE_NAME = "t-phish-simple"; nit: maybe we should name this list `mochitest-phish-simple` to match the convention used in the other ones ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:61 (Diff revision 1) > + addTestPhishingSite(url) { > + // Add URL to the phishing databases > + > + let phishingUpdate = > + "n:1\ni:" + PHISHING_TABLE_NAME + "\nad:3\n" + > + "a:1:32:" + url.length + "\n" + url + "\n"; This actually gives the same ID to all of the URLs that you add to the database. Maybe it would be simpler to hard-code URLs like `phishing.example.com` and `phishing2.example.com` here, just like we do for tracking URLs. ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:94 (Diff revision 1) > * @return {Promise} > */ > useTestDatabase(tables) { > for (var table of tables) { > Services.prefs.setCharPref(table.pref, table.name); > + if (table.provider) Services.prefs.setCharPref(table.provider, table.name); That code might fail in the future if we set the provider in more than one table. How about doing a `getCharPref()` first and then adding `table.name` to the existing list? ::: toolkit/components/url-classifier/tests/mochitest/report.sjs:1 (Diff revision 1) > +const SJS = "report.sjs?"; Missing copyright header: /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ ::: toolkit/components/url-classifier/tests/mochitest/report.sjs:59 (Diff revision 1) > + } > + > + // Redirect as default > + params.delete("a"); > + params.append("a", "create-blocked-page-redirect"); > + // 302 found, 301 Moved Permanently, 303 See Other, 307 Temporary Redirect nit: that comment seems unnecessary ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:29 (Diff revision 1) > + .QueryInterface(Ci.nsIDocShellTreeItem) > + .rootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > +let SJS = "example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs"; > +let BASED_URL = "https://" + SJS + "?"; typo: `BASED_URL` should be just `BASE_URL` ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:33 (Diff revision 1) > +let SJS = "example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs"; > +let BASED_URL = "https://" + SJS + "?"; > + > +let PHISHING_URL = "itisaphishingsite.org/phishing.html"; > +let PHISHING_URL2 = "phishing.example.com/phishing.html"; > +let PHISHING_LIST = "t-phish-simple"; Should be updated to match the other one. ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:130 (Diff revision 1) > + let expectedReportUrl = BASED_URL + "a=reporturl&reporturl=" + encodeURIComponent(aUrl); > + is(aReportBrowser.contentDocument.location.href, expectedReportUrl, "Correct report URL"); > +} > + > +SpecialPowers.pushPrefEnv( > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], I'm not sure why we need to change this tracking protection pref. ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:134 (Diff revision 1) > +SpecialPowers.pushPrefEnv( > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], > + ["browser.safebrowsing.reportPhishMistakeURL", BASED_URL + "a=reporturl&reporturl="], > + ["security.mixed_content.block_active_content", false], > + ["browser.safebrowsing.phishing.enabled", true], > + ["browser.safebrowsing.malware.enabled", true], Do we need to enable malware checking? ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:135 (Diff revision 1) > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], > + ["browser.safebrowsing.reportPhishMistakeURL", BASED_URL + "a=reporturl&reporturl="], > + ["security.mixed_content.block_active_content", false], > + ["browser.safebrowsing.phishing.enabled", true], > + ["browser.safebrowsing.malware.enabled", true], > + ["browser.safebrowsing.enabled", true]]}, This should be removed. That pref doesn't exist anymore.

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review71880 > nit: maybe we should name this list `mochitest-phish-simple` to match the convention used in the other ones Changing the name makes path length be over COMPLETE_SIZE [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/ProtocolParser.cpp#377 Changed and added getHash to the test > This actually gives the same ID to all of the URLs that you add to the database. > > Maybe it would be simpler to hard-code URLs like `phishing.example.com` and `phishing2.example.com` here, just like we do for tracking URLs. Removed and used classifierHelper and added getHash support > That code might fail in the future if we set the provider in more than one table. > > How about doing a `getCharPref()` first and then adding `table.name` to the existing list? Removed

Sorry for a delay. I am trying to understand this. There can be more than one iframe to a phishing site? How do you deal with this case? I am wondering if there is a easier way to do this? In docShell error page you have this info, is it possible to get it from there somehow? I am not sure if it is possible, I will need some time to think about this, before I can review it.

(In reply to Dragana Damjanovic [:dragana] from comment #16) > Sorry for a delay. > > I am trying to understand this. > > There can be more than one iframe to a phishing site? How do you deal with > this case? > > I am wondering if there is a easier way to do this? In docShell error page > you have this info, is it possible to get it from there somehow? I am not > sure if it is possible, I will need some time to think about this, before I > can review it. Thanks for looking at this Dragana :) We are doing similar ([1]) [1] https://reviewboard.mozilla.org/r/73358/diff/2#3 The failed information is loaded from failed channel of the owner doc (docshell). From that, we can get blocked information of each frame for later report.

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review76916 ::: browser/base/content/content.js:546 (Diff revision 2) > + let classifiedChannel = docShell.failedChannel. > + QueryInterface(Ci.nsIClassifiedChannel); > + if (classifiedChannel) { > + blockedInfo = { list: classifiedChannel.matchedList, > + provider: classifiedChannel.matchedProvider, > + uri: classifiedChannel.matchedUri.asciiSpec }; You can get channel uri here, it must be the same as the uri that is blocked. And you do not need matchedUri. from a quick look list and provider are not used?

(In reply to Dragana Damjanovic [:dragana] from comment #18) > > from a quick look list and provider are not used? Yep, at the time being, they are not used but might be used later (like comment 6). Or, like bug 1181955, "Chinese data providers generally want to let the user know they're the ones protecting his/her from bad guys." I marked the bug 1181955 as duplicated and give provider info in this bug. Thanks

Also, testcase included checking the correct value of list and provider. I think it would be better to comment them out but leave some comments if someone would like to use them. Thanks for looking at this

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review79972 Remove matchedUri, if it is the same as the channel uri(without query part). ::: browser/base/content/content.js:546 (Diff revision 2) > + let classifiedChannel = docShell.failedChannel. > + QueryInterface(Ci.nsIClassifiedChannel); > + if (classifiedChannel) { > + blockedInfo = { list: classifiedChannel.matchedList, > + provider: classifiedChannel.matchedProvider, > + uri: classifiedChannel.matchedUri.asciiSpec }; You can remove matchedUri, since it is the channel uri without query part. I have not noticed any other uri transformations. But this is only a pointer in most of the connection so it does not have a huge impact. ::: netwerk/base/nsIParentChannel.idl:24 (Diff revision 2) > > /** > * Implemented by chrome side of IPC protocols. > */ > > -[scriptable, uuid(e0fc4801-6030-4653-a59f-1fb282bd1a04)] > +[scriptable, uuid(d270bb28-a591-44df-9671-e11f746be298)] You do not need to change uuids any more. ::: netwerk/base/nsIURIClassifier.idl:14 (Diff revision 2) > interface nsIURI; > > /** > * Callback function for nsIURIClassifier lookups. > */ > -[scriptable, function, uuid(8face46e-0c96-470f-af40-0037dcd797bd)] > +[scriptable, function, uuid(d9dfadf5-203f-4f91-a174-a665af68b1ae)] same here.

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review87450 Thanks for the new revision Thomas and sorry for the delay in reviewing again. One problem I see, assuming I understand the test correctly, is that in the redirect case we're adding this URL to the blacklist: http://example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-redirect which will get normalized to: example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-redirect and then we are redirecting to: https://test1.example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-redirect which will also match the entry we added to the blacklist (as per the [the simplified regex lookup](https://web.archive.org/web/20160422212049/https://developers.google.com/safe-browsing/developers_guide_v2#RegexLookup)). Can we redirect to a URL that will not be blacklisted to make sure that the test case is testing exactly what we want (and is easier to reason about)? ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:204 (Diff revision 2) > + let url = "http://" + PHISHING_URL; > + yield testOnWindow(url, url, function(browser) { > + checkBlockedURL(browser, url); > + }, createBlockedPage); > + > + // Iframe Could you please expand this comment to include the URLs: // Iframe case: // A top level page at http://mochi.test:8888/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-iframe // contains an iframe to http://phishing.example.com/test.html (blocked). ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:212 (Diff revision 2) > + url = "http://" + PHISHING_URL2; > + yield testOnWindow(BASE_URL + "action=create-blocked-iframe", url, function(browser) { > + checkBlockedURL(browser, url); > + }, createBlockedIframe); > + > + // Redirect Could you please expand this comment to include the URLs: // Redirect case: // A top level page at // http://example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-redirect (blocked) // will get redirected to // https://test1.example.com/chrome/toolkit/components/url-classifier/tests/mochitest/report.sjs?action=create-blocked-redirect. Please double-check that I've got the URLs right though, it's a little hard to decipher.

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review87462 This is a very good start Thomas. I like the approach and it looks like you've figured out the hard parts. I'm suggesting a lot of changes here because the work you are doing as part of fixing this bug will enable us to resolve a number of other long-standing issues. It will also be useful when we want to tackle reporting in V4. ::: browser/base/content/browser-safebrowsing.js:49 (Diff revision 2) > * Used to report a phishing page or a false positive > * @param name String One of "Phish", "Error", "Malware" or "MalwareError" > + * @param uri URL to be reported > * @return String the report phishing URL. > */ > - getReportURL: function(name) { > + getReportURL: function(name, uri) { Now that we have the provider, we should pass it to this function too. Then within `SafeBrowsing.getReportURL()`, we can lookup the correct reporting URL for that provider. We don't currently have more than one reporting URL, we only have `browser.safebrowsing.reportPhishMistakeURL` and `browser.safebrowsing.reportMalwareMistakeURL`, but we can check that the provider is "google" and return an empty URL if it's not. We can add the "google4" URLs and refactor this later. ::: browser/base/content/browser.js:3015 (Diff revision 2) > title = gNavigatorBundle.getString("safebrowsing.reportedAttackSite"); > buttons[1] = { > label: gNavigatorBundle.getString("safebrowsing.notAnAttackButton.label"), > accessKey: gNavigatorBundle.getString("safebrowsing.notAnAttackButton.accessKey"), > callback: function() { > - openUILinkIn(gSafeBrowsing.getReportURL('MalwareMistake'), 'tab'); > + openUILinkIn(gSafeBrowsing.getReportURL('MalwareMistake', We could pass in the blockedInfo structure directly instead of just the URI. ::: browser/base/content/browser.js:3026 (Diff revision 2) > title = gNavigatorBundle.getString("safebrowsing.deceptiveSite"); > buttons[1] = { > label: gNavigatorBundle.getString("safebrowsing.notADeceptiveSiteButton.label"), > accessKey: gNavigatorBundle.getString("safebrowsing.notADeceptiveSiteButton.accessKey"), > callback: function() { > - openUILinkIn(gSafeBrowsing.getReportURL('PhishMistake'), 'tab'); > + openUILinkIn(gSafeBrowsing.getReportURL('PhishMistake', ditto ::: netwerk/base/nsChannelClassifier.h:62 (Diff revision 2) > + nsIURI *aUri); > > public: > // If we are blocking tracking content, update the corresponding flag in > // the respective docshell and call nsISecurityEventSink::onSecurityChange. > static nsresult SetBlockedTrackingContent(nsIChannel *channel); It might actually be possible to refactor `SetBlockedTrackingContent()` and `SetBlockedUnsafeBrowsingContent()` into a single `SetBlockedContent()`. That means we would keep the blocking info on the channel in both cases (unsafe browsing and tracking) and we would also: 1. create a new security state that UI can listen to (similar to [nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT](http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/netwerk/base/nsChannelClassifier.cpp#563)), and 2. emit a console message to indicate that something has been blocked (similar to ["TrackingUriBlocked"](http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/netwerk/base/nsChannelClassifier.cpp#571-576)) This is a little bit of work, but it would enable follow-up work to degrade the security indicator when unsafe content is blocked on a page (Chrome is either doing this or thinking about it, IE has that), would fix bug 538204 and help with bug 1195242. ::: netwerk/base/nsIClassifiedChannel.idl:30 (Diff revision 2) > + * @param aProvider > + * Name of the Safe Browsing provider that matched (e.g. google) > + * @param aUri > + * Url that matched Safe Browsing list. > + */ > + void setMatchedInfo(in ACString aList, Another piece of information that would be useful is the hash prefix that the URL matched. Google would like to receive this information to more easily debug errors with their list. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1149 (Diff revision 2) > + mCallback->OnClassifyComplete(response, tables, provider, uri); > return NS_OK; > } > > +nsresult > +nsUrlClassifierClassifyCallback::FindProvider(const nsACString& tables, It would be nice to have a unit test just for this function. In particular, I'm interested in making sure that this function still works with: 1. lists without a provider (e.g. `test-track-simple`) 2. urls which matched two tables (e.g. `googpub-phish-shavar,goog-phish-shavar`) 3. urls which matched two tables from two different providers (e.g. `test-malware-simple,goog-malware-shavar`) In case #1, we should return an empty provider and hide the "this isn't a phishing/attack site" button. In case #2, we should return one of the lists (it doesn't matter since they have the same provider). In case #3, we should return the list with an associated provider (i.e. `goog-malware-shavar`) since it's more likely to be useful than test lists without a provider. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1165 (Diff revision 2) > + &prefCount, &prefNames); > + > + if (NS_SUCCEEDED(rv) && prefCount > 0) { > + for (uint32_t i = 0; i < prefCount; ++i) { > + nsAdoptingCString value = Preferences::GetCString(prefNames[i]); > + if (value && FindInReadable(tables, value)) { This is going through all of the prefs looking for a match. It's unnecessary and could lead to problems in the future. We only need to look at `browser.safebrowsing.provider.<provider name>.lists` for each provider.

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #25) > Comment on attachment 8783611 [details] > Bug 1288633 - Add SafeBrowsing report false positive URL test. > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/73360/diff/2-3/ Update rebased

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review87462 > It might actually be possible to refactor `SetBlockedTrackingContent()` and `SetBlockedUnsafeBrowsingContent()` into a single `SetBlockedContent()`. > > That means we would keep the blocking info on the channel in both cases (unsafe browsing and tracking) and we would also: > > 1. create a new security state that UI can listen to (similar to [nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT](http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/netwerk/base/nsChannelClassifier.cpp#563)), and > 2. emit a console message to indicate that something has been blocked (similar to ["TrackingUriBlocked"](http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/netwerk/base/nsChannelClassifier.cpp#571-576)) > > This is a little bit of work, but it would enable follow-up work to degrade the security indicator when unsafe content is blocked on a page (Chrome is either doing this or thinking about it, IE has that), would fix bug 538204 and help with bug 1195242. Sure, I will take a closer look into this part. > Another piece of information that would be useful is the hash prefix that the URL matched. Google would like to receive this information to more easily debug errors with their list. The HandleResult currently only returns a string which represents the list of table that matched. I am still figuring out to provide hash in the result array returned from our classifier. > It would be nice to have a unit test just for this function. > > In particular, I'm interested in making sure that this function still works with: > > 1. lists without a provider (e.g. `test-track-simple`) > 2. urls which matched two tables (e.g. `googpub-phish-shavar,goog-phish-shavar`) > 3. urls which matched two tables from two different providers (e.g. `test-malware-simple,goog-malware-shavar`) > > In case #1, we should return an empty provider and hide the "this isn't a phishing/attack site" button. > > In case #2, we should return one of the lists (it doesn't matter since they have the same provider). > > In case #3, we should return the list with an associated provider (i.e. `goog-malware-shavar`) since it's more likely to be useful than test lists without a provider. Thanks, I am rewriting the function based on DeriveProviderFromPref which has been landed recently.

Something in getting providers should be changed in Bug 1315097

This is rebased commit, sorry for tagging Dragana for review, that is mozreview's bug

Update the patch based on reviewer's comment. Thanks you for looking at that. Also, I have changed a bit in IPC message to make getting matched info could work in content process (as i know, Clasify() works in content process now).

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review105398 This is great Thomas! It will allow us to fix a number of issues with Safe Browsing. I've got a few more tweaks, but nothing major in here. I like your approach. ::: browser/base/content/browser-safebrowsing.js:44 (Diff revision 5) > > /** > * Used to report a phishing page or a false positive > - * @param name String One of "Phish", "Error", "Malware" or "MalwareError" > + * > + * @param name > + * String One of "Phish", "Error", "Malware" or "MalwareError" The original comment is actually wrong. The values are "PhishMistake" and "MalwareMistake" as far as I can tell. ::: browser/base/content/browser-safebrowsing.js:46 (Diff revision 5) > * Used to report a phishing page or a false positive > - * @param name String One of "Phish", "Error", "Malware" or "MalwareError" > + * > + * @param name > + * String One of "Phish", "Error", "Malware" or "MalwareError" > + * @param info > + * Report information. In the case false positive, it may contain I would describe this as "Information about the reasons for blocking the resource." ::: netwerk/base/nsChannelClassifier.cpp:599 (Diff revision 5) > const char16_t* params[] = { spec.get() }; > + const char* message = (aErrorCode == NS_ERROR_TRACKING_URI) ? > + "TrackingUriBlocked" : "UnsafeBrowsingUriBlocked"; > + nsCString category = (aErrorCode == NS_ERROR_TRACKING_URI) ? > + NS_LITERAL_CSTRING("Tracking Protection") : > + NS_LITERAL_CSTRING("Safe Browsing Protection"); Let's use just "Safe Browsing". ::: netwerk/base/nsChannelClassifier.cpp:709 (Diff revision 5) > LOG(("nsChannelClassifier[%p]: cancelling channel %p for %s " > "with error code %s", this, mChannel.get(), > uri->GetSpecOrDefault().get(), errorName.get())); > } > > - // Channel will be cancelled (page element blocked) due to tracking. > + // Channel will be cancelled (page element blocked) due to tracking I would phrase this as: // Channel will be cancelled (page element blocked) due to tracking protection or Safe Browsing. ::: netwerk/locales/en-US/necko.properties:42 (Diff revision 5) > PhishingAuthAccept=I understand and will be very careful > SuperfluousAuth=You are about to log in to the site “%1$S” with the username “%2$S”, but the website does not require authentication. This may be an attempt to trick you.\n\nIs “%1$S” the site you want to visit? > AutomaticAuth=You are about to log in to the site “%1$S” with the username “%2$S”. > > TrackingUriBlocked=The resource at “%1$S” was blocked because tracking protection is enabled. > +UnsafeBrowsingUriBlocked=The resource at “%1$S” was blocked because Safe Browsing Protection is enabled. Simpler phrasing: The resource at “%1$S” was blocked by Safe Browsing. Also, we could name this just `UnsafeUriBlocked`. ::: netwerk/protocol/ftp/FTPChannelParent.cpp:571 (Diff revision 5) > NS_IMETHODIMP > +FTPChannelParent::SetClassifierMatchedInfo(const nsACString& aList, > + const nsACString& aProvider, > + const nsACString& aPrefix) > +{ > + // nothing to do I would suggestion the same comment as the previous function: // One day, this should probably be filled in. ::: toolkit/components/url-classifier/Entries.h:91 (Diff revision 5) > -#ifdef DEBUG > void ToString(nsACString& aStr) const { > uint32_t len = ((sHashSize + 2) / 3) * 4; > + > + // Capacity should be one greater than length, because PL_Base64Encode > + // will not be null-terminated, while nsCString requires missing word: "while nsCString requires it." ::: toolkit/components/url-classifier/SafeBrowsing.jsm:137 (Diff revision 5) > updateURL: null, > gethashURL: null, > > reportURL: null, > > - getReportURL: function(kind, URI) { > + getReportURL: function(kind, info) { Let's change this function a little more in order to make it easier to support multiple providers. Let's move `browser.safebrowsing.report*URL` into `browser.safebrowsing.provider.google.report*URL`. That way, we can do something like: case "Phish": pref = "browser.safebrowsing." + info.provider + ".reportPhishURL"; break; And then later we can check whether or not the URL is empty: let reportUrl = Services.urlFormatter.formatURLPref(pref); if (reportUrl) { reportUrl += encodeURIComponent(info.uri); } and that would replace the provider check so we don't have to hard-code `google` and `google4` here. ::: toolkit/components/url-classifier/SafeBrowsing.jsm:161 (Diff revision 5) > - // Remove the query to avoid including potentially sensitive data > - if (pageUri instanceof Ci.nsIURL) > - pageUri.query = ''; > + // For now we only support reporting false positive if provider is Google > + if (kind == "PhishMistake" || kind == "MalwareMistake") { > + if (!info.list || !info.uri) { > + return null; > + } > + if ((info.list.indexOf("test-") != -1) && I think this check is wrong. I believe it should be (in pseudo-code): if list.contains("test-") OR (provider != "google" AND provider != "google4") But for clarity, we may as well break it down into several `if` statements: if !list OR !uri return null if list.contains("test-") return null if provider != "google" AND provider != "google4" return null ::: toolkit/components/url-classifier/nsIUrlClassifierDBService.idl:243 (Diff revision 5) > + */ > +[uuid(091adf98-28a5-473d-8dec-5b34b4e62496)] > +interface nsIUrlClassifierClassifyCallback : nsISupports > +{ > + /** > + * The function is called for each time the URL matches a Safe Browsing list "The function is called each time..." ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1158 (Diff revision 5) > + nsCString name; > + uint8_t priority; > +}; > + > +// Order matters > +static const Provider kBuildInProviders[] = { `kBuiltInProviders` ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1161 (Diff revision 5) > + > +// Order matters > +static const Provider kBuildInProviders[] = { > + { NS_LITERAL_CSTRING("google"), 0 }, > + { NS_LITERAL_CSTRING("google4"), 1 }, > + { NS_LITERAL_CSTRING("mozilla"), 2 }, I'm thinking we should make `mozilla` the lowest priority here because we don't have a reporting mechanism while Google does have one. ::: uriloader/base/nsIWebProgressListener.idl:206 (Diff revision 5) > > /** > - * Tracking content flags > + * Safe Browsing blocking content flags > * > * May be set in addition to the State security Flags, to indicate that > - * tracking content has been encountered. > + * tracking or unsafe browsing content has been encountered. I would simply say "tracking or unsafe content..." ::: uriloader/base/nsIWebProgressListener.idl:219 (Diff revision 5) > + * STATE_BLOCKED_UNSAFE_BROWSING_CONTENT > + * Content which againts SafeBrowsing list has been blocked from loading. > */ > - const unsigned long STATE_BLOCKED_TRACKING_CONTENT = 0x00001000; > - const unsigned long STATE_LOADED_TRACKING_CONTENT = 0x00002000; > + const unsigned long STATE_BLOCKED_TRACKING_CONTENT = 0x00001000; > + const unsigned long STATE_LOADED_TRACKING_CONTENT = 0x00002000; > + const unsigned long STATE_BLOCKED_UNSAFE_BROWSING_CONTENT = 0x00004000; `STATE_BLOCKED_UNSAFE_CONTENT` would be shorter and just as descriptive.

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review105398 > Let's change this function a little more in order to make it easier to support multiple providers. > > Let's move `browser.safebrowsing.report*URL` into `browser.safebrowsing.provider.google.report*URL`. > > That way, we can do something like: > > case "Phish": > pref = "browser.safebrowsing." + info.provider + ".reportPhishURL"; > break; > > And then later we can check whether or not the URL is empty: > > let reportUrl = Services.urlFormatter.formatURLPref(pref); > if (reportUrl) { > reportUrl += encodeURIComponent(info.uri); > } > > and that would replace the provider check so we don't have to hard-code `google` and `google4` here. That's great idea. Just changed as you suggested. I have no idea about google4's reporting mechanism, I assume that google4 uses the same as google (use https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url= then redirect to google site). > I'm thinking we should make `mozilla` the lowest priority here because we don't have a reporting mechanism while Google does have one. I used the order : google -> google 4 > mozilla -> other providers, but the lowest priority is 2 and highest is 0, the code looks hard to review. I've just changed to Others: 0 (lowest), mozilla 1, google4 2, google: 3 (highest)

it seems mozreview changed and I have no idea why it did not set request review again after getting a r-

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review106500 That looks great. One thing I'm not sure about though and that I'd like you to double-check before you land this is the `browser.safebrowsing.reportPhishURL` pref. I think it's only used for the "Help | Report deceptive site..." menu option on OSX and Linux. If that's the case, then we should leave that pref as it is (`browser.safebrowsing.reportPhishURL`) and not move it to the `google` and `google4` providers because we don't have any way to choose which provider to direct users to when they click that menu item.

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review106500 Thanks you for your review. Yep, this pref is only used for "Help | Report deceptive site..." menu option In the case google wants to change the way clients report (for example, and we may have to use the new report URL) from V2 to V4, then we may need providers in the pref. I think we discussed in the last meeting, assuming google and google4 use the same way to report URL. So we could revert back to use browser.safebrowsing.reportPhishURL because using only one browser.safebrowsing.reportPhishURL is enough. But we may leave some hard codes "google" and "google4" in our code :)

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #45) > Yep, this pref is only used for "Help | Report deceptive site..." menu option > In the case google wants to change the way clients report (for example, and > we may have to use the new report URL) from V2 to V4, then we may need > providers in the pref. If you have more than one provider active, we won't know which one to report to. We'd have to pick one as the winner or something like that, but there will never be more than one menu item. So I would suggest we keep that one as it is. It will be more obvious that this menu option doesn't support multiple providers. If Google changes the URL, then we'll need to update the redirects we have in place on our servers.

(In reply to François Marier [:francois] from comment #46) > (In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #45) > > Yep, this pref is only used for "Help | Report deceptive site..." menu option > > In the case google wants to change the way clients report (for example, and > > we may have to use the new report URL) from V2 to V4, then we may need > > providers in the pref. > > If you have more than one provider active, we won't know which one to report > to. We'd have to pick one as the winner or something like that, but there > will never be more than one menu item. > > So I would suggest we keep that one as it is. It will be more obvious that > this menu option doesn't support multiple providers. > > If Google changes the URL, then we'll need to update the redirects we have > in place on our servers. Agree, changed back to use browser.safebrowsing.reportPhishURL, and I added a test case in which non-google provider matched. There should be no report button is showed

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review106512 This test looks really good. The only reason I'm keeping my r- is that I want to understand the weird `expectedReportUri` values, as commented below. ::: toolkit/components/url-classifier/tests/mochitest/test_classifier_match.html:19 (Diff revision 8) > +<pre id="test"> > + > +<script class="testbody" type="application/javascript"> > +var Cc = SpecialPowers.Cc; > +var Ci = SpecialPowers.Ci; > +var Cu = SpecialPowers.Cu; Unused in this file. ::: toolkit/components/url-classifier/tests/mochitest/test_classifier_match.html:52 (Diff revision 8) > + provider: "mozilla" > + }, > + > +]; > + > +function hash(str) { nit: for clarity, I'd suggest calling this function `hashprefix()` ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:20 (Diff revision 8) > +</div> > +<pre id="test"> > + > +<script class="testbody" type="text/javascript"> > + > +const Cc = Components.classes; Unused in this file. ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:151 (Diff revision 8) > +var testDatas = [ > + { topUrl: "http://itisaphishingsite.org/phishing.html", > + testUrl: "itisaphishingsite.org/phishing.html", > + list: "mochi1-phish-simple", > + blockCreater : createBlockedPage, > + expectedReportUri: "http://itisaphishingsite.org/phishing.html" I don't understand why this is the `expectedReportUri`. Do you mean that when there is no report URL for a given provider, clicking the "This isn't a deceptive site..." button doesn't do anything? ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:181 (Diff revision 8) > + }, > + > +]; > + > +SpecialPowers.pushPrefEnv( > + {"set" : [["security.mixed_content.block_active_content", false], Is this required or could we just have the test pages over HTTPS?

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review106512 > I don't understand why this is the `expectedReportUri`. > > Do you mean that when there is no report URL for a given provider, clicking the "This isn't a deceptive site..." button doesn't do anything? If there's no report URL, the report button should not be shown. Could you plz check the lastest diff, I added a case that uses a "fake" provider. > Is this required or could we just have the test pages over HTTPS? You are right, I forgot removing this from prevous diff. Thanks

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review106556 ::: modules/libpref/init/all.js (Diff revision 8) > pref("browser.safebrowsing.provider.google4.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site="); > +pref("browser.safebrowsing.provider.google4.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url="); > +pref("browser.safebrowsing.provider.google4.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url="); > +pref("browser.safebrowsing.provider.google4.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url="); > > -pref("browser.safebrowsing.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url="); Sorry, what I meant is that we should move `reportPhishMistakeURL` and `reportMalwareMistakeURL` to the provider (as in your previous revision), but keep `reportPhishURL` as `browser.safebrowsing.reportPhishURL`.

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review106560 ::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:199 (Diff revision 9) > + }, > + > +]; > + > +SpecialPowers.pushPrefEnv( > + {"set" : [["browser.safebrowsing.reportPhishMistakeURL", BASE_URL + "action=reporturl&reporturl="], This should be `browser.safebrowsing.provider.google.reportPhishMistakeURL`. Sorry for the confusion.

oops, seems that I misunderstood your idea in comment 44, sorry about that. The bellow prefs should be used, tell me if I am wrong browser.safebrowsing.reportPhishURL browser.safebrowsing.provider.google.reportPhishMistakeURL browser.safebrowsing.provider.google4.reportPhishMistakeURL browser.safebrowsing.provider.google.reportMalwareMistakeURL browser.safebrowsing.provider.google4.reportMalwareMistakeURL I will update the diff based on that

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #54) > The bellow prefs should be used, tell me if I am wrong > browser.safebrowsing.reportPhishURL > browser.safebrowsing.provider.google.reportPhishMistakeURL > browser.safebrowsing.provider.google4.reportPhishMistakeURL > browser.safebrowsing.provider.google.reportMalwareMistakeURL > browser.safebrowsing.provider.google4.reportMalwareMistakeURL That's right.

Comment on attachment 8783610 [details] Bug 1288633 - Add more information when an URL matches Safe Browsing list. https://reviewboard.mozilla.org/r/73358/#review106898

Comment on attachment 8783611 [details] Bug 1288633 - Add SafeBrowsing report false positive URL test. https://reviewboard.mozilla.org/r/73360/#review106900

Since thomas is PTO until 2/4, i will help on checking the try server error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2882644391bd&selectedJob=70558737

Sorry about pending this for a while. I found the try error[1] can be reproduced locally by setting parentRunner.showTestReport[2] to false. And it is related to when we call SpecialPowers.pushPrefEnv more than once in a test, runNextTest callback will lost in specialpower.flushPrefEnv. But i still need some time to find out the reason that cause runNextTest lost. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa3a8f9d4c8f4b482eb3b9b3e69c29bf66b6fed4&selectedJob=71847336 [2] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1136

Thanks Dimi for the great finding

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f686d1f1c90edec79bd4ac4e128afa6e4b8a77 Try

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/077f42a9964c Add more information when an URL matches Safe Browsing list. r=dragana,francois https://hg.mozilla.org/integration/autoland/rev/c4eeea895a9c Add SafeBrowsing report false positive URL test.r=francois

https://hg.mozilla.org/mozilla-central/rev/077f42a9964c https://hg.mozilla.org/mozilla-central/rev/c4eeea895a9c

https://hg.mozilla.org/mozilla-central/rev/c4eeea895a9c#l5.12 https://hg.mozilla.org/mozilla-central/rev/c4eeea895a9c#l6.182 Please do not add any new enablePrivilege calls.

(In reply to Masatoshi Kimura [:emk] from comment #68) > https://hg.mozilla.org/mozilla-central/rev/c4eeea895a9c#l5.12 > https://hg.mozilla.org/mozilla-central/rev/c4eeea895a9c#l6.182 > Please do not add any new enablePrivilege calls. I have just read the story in bug 1365199, filed bug 1365466 and will run try again to see.