1024610 - register tracking protection list in Safebrowsing.jsm and hook it up to nsChannelClassifier

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

So we can use it to block tracking domains.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

 https://tbpl.mozilla.org/?tree=Try&rev=0b72534bfceb

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Ryan,

Can you specify which paths you want for the gethash and update URLs?

https://tracking.services.mozilla.com/{gethash,update,whatever you want}

The existing gethash and update urls are here: http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#927

The SAFEBROWSING_ID stuff is rewritten by the url formatter, which we can just skip if you don't think we'll need it: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/nsURLFormatter.js#126

Thanks,
Monica

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier (

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier (

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8448235 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (

Review of attachment 8448235 [details] [diff] [review]:
-----------------------------------------------------------------

Ryan, please review URLs in firefox.js.

gcp, to dogfood please change browser.tracking_protection.enabled = true and point updateUrl to "http://54.184.154.124/downloads?updateURL&client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%". 

I filed bug 1032414 to fix the stream updater from calling downloadsuccess for NXDOMAINs, and bug 1032393 to enable gethash requests for this list when there's a server for it.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8448235 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (

Review of attachment 8448235 [details] [diff] [review]:
-----------------------------------------------------------------

"There are only two hard things in Computer Science: cache invalidation and naming things."

Don't you need changes to the error pages to deal with the third category?

::: browser/app/profile/firefox.js
@@ +935,5 @@
>  
>  pref("browser.safebrowsing.malware.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
>  pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
>  
> +pref("browser.tracking_protection.updateURL", "https://tracking.services.mozilla.com/update?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%");

GOOGLE_API_KEY for our own service? Boo!

::: modules/libpref/src/init/all.js
@@ +819,5 @@
>  //   0 = tracking is acceptable
>  //   1 = tracking is unacceptable
>  pref("privacy.donottrackheader.value",      1);
> +// Enforce tracking protection
> +pref("privacy.trackingprotection.enabled",  false);

tracking_protection earlier vs trackingprotection here

@@ +4186,5 @@
>  pref("urlclassifier.malware_table", "goog-malware-shavar,test-malware-simple");
>  pref("urlclassifier.phish_table", "goog-phish-shavar,test-phish-simple");
>  pref("urlclassifier.downloadBlockTable", "");
>  pref("urlclassifier.downloadAllowTable", "");
> +pref("urlclassifier.tracking_list", "mozpub-track-digest256");

malware_table, BlockTable and now tracking_list. Consistent naming please.

::: toolkit/components/url-classifier/content/listmanager.js
@@ +379,5 @@
>      // As long as the delay is something sane (5 minutes or more), update
>      // our delay time for requesting updates. Setting the delay requires a
>      // repeating timer, so always use one.
>      if (delay >= (5 * 60) && this.updateCheckers_[updateUrl]) {
> +      log("Waiting for " + delay + " seconds");

nit: unrelated change

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> Comment on attachment 8448235 [details] [diff] [review]
> Register tracking protection list and hook it up in nsChannelClassifier (
> 
> Review of attachment 8448235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "There are only two hard things in Computer Science: cache invalidation and
> naming things."
> 
> Don't you need changes to the error pages to deal with the third category?

This list is not intended to include domains that people actually want to visit as a top-level URL. I filed bug 1032480 in case someone actually does want to visit a site on the list, but I don't think it should block this one, especially since it is prefed off and undiscoverable right now.

> GOOGLE_API_KEY for our own service? Boo!

OK :)
 
> @@ +4186,5 @@
> >  pref("urlclassifier.malware_table", "goog-malware-shavar,test-malware-simple");
> >  pref("urlclassifier.phish_table", "goog-phish-shavar,test-phish-simple");
> >  pref("urlclassifier.downloadBlockTable", "");
> >  pref("urlclassifier.downloadAllowTable", "");
> > +pref("urlclassifier.tracking_list", "mozpub-track-digest256");
> 
> malware_table, BlockTable and now tracking_list. Consistent naming please.

Yeah, this is due to bug 985720 :( I will rename everything to camel case.
 
> ::: toolkit/components/url-classifier/content/listmanager.js
> @@ +379,5 @@
> >      // As long as the delay is something sane (5 minutes or more), update
> >      // our delay time for requesting updates. Setting the delay requires a
> >      // repeating timer, so always use one.
> >      if (delay >= (5 * 60) && this.updateCheckers_[updateUrl]) {
> > +      log("Waiting for " + delay + " seconds");
> 
> nit: unrelated change

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Register tracking protection list and hook it up in nsChannelClassifier (

Comment 13

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8448950 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (

Review of attachment 8448950 [details] [diff] [review]:
-----------------------------------------------------------------

Green try (doesn't include observer changes): https://tbpl.mozilla.org/?tree=Try&rev=ad4d23be4bed

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8448950 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (

Review of attachment 8448950 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but no reason not to add the URLs for all platforms.

::: browser/app/profile/firefox.js
@@ +936,5 @@
>  pref("browser.safebrowsing.malware.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
>  pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
>  
> +pref("browser.trackingprotection.updateURL", "https://tracking.services.mozilla.com/update?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
> +pref("browser.trackingprotection.gethashURL", "https://tracking.services.mozilla.com/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");

Don't you need to update Android with the same links? Alternatively, why aren't these in the generic all.js?

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Hmm, interesting -- the regular safebrowsing update prefs are in firefox.js and mobile.js only, I missed the mobile.js one.

I'll try moving to all.js: https://tbpl.mozilla.org/?tree=Try&rev=270d8479f2f5

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e35172b6476

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e35172b6476