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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mmc, Assigned: mmc)

Tracking

Trunk
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

So we can use it to block tracking domains.
Created attachment 8439360 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Created attachment 8439506 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier
Attachment #8439360 - Attachment is obsolete: true
 https://tbpl.mozilla.org/?tree=Try&rev=0b72534bfceb
Depends on: 1021419
Created attachment 8445427 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier
Attachment #8439506 - Attachment is obsolete: true
Blocks: 1029886
Created attachment 8446740 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier
Attachment #8445427 - Attachment is obsolete: true
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
Flags: needinfo?(rtilder)
Created attachment 8448231 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (
Attachment #8446740 - Attachment is obsolete: true
Created attachment 8448235 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (
Attachment #8448231 - Attachment is obsolete: true
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.
Attachment #8448235 - Flags: review?(rtilder)
Attachment #8448235 - Flags: review?(gpascutto)
Flags: needinfo?(rtilder)
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
Attachment #8448235 - Flags: review?(gpascutto) → review-
(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
Created attachment 8448950 [details] [diff] [review]
Register tracking protection list and hook it up in nsChannelClassifier (
Attachment #8448235 - Attachment is obsolete: true
Attachment #8448235 - Flags: review?(rtilder)
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
Attachment #8448950 - Flags: review?(gpascutto)
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?
Attachment #8448950 - Flags: review?(gpascutto) → review+
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
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e35172b6476
https://hg.mozilla.org/mozilla-central/rev/4e35172b6476
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.