1476967 - Allow the adding of tracking-protection entries via prefs

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

Currently, there is not an easy way to add entries in the Tracking Protection lists. In this bug, we want to introduce a set of preferences which will be used as extra TP lists. This approach will be extremely powerful for debugging and testing purposes.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: aaa.patch

Comment 2

[François Marier [:francois]]

Comment on attachment 8994873 [details] [diff] [review]
aaa.patch

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

(Now that bug 1461515, we'll need to also handle urlclassifier.trackingAnnotationTable
and urlclassifier.trackingAnnotationWhitelistTable too.)

Good job finding all of the users and handling them all. I was under the
(misguided) impression that we might be able to handle all of these API
users with the same generic code:

  if (GetPref(basepref + ".testEntries")) {
    // add these domains to the list too
  }

but it looks like I was wrong and it's actually much more complicated than
that.

Given the amount of code involved, I think our best option is probably to
just handle tracking protection and tracking annotations. That's the only
use case that we have where we'd need to let web developers add entries and
so I'd rather have these extra entries just where we know for sure that they
will be useful. Otherwise we'll have a lot of seldom-used code that will
make the classifier even harder to understand.

Sorry for changing my mind, I was expecting this to be much simpler to
write. This should allow you to avoid changing LoginReputation and
greatly reduce the amount of changes in nsUrlClassifierDBService (probably
we only need to modify AsyncClassifyLocalWithTables).

We should document somewhere in a comment the exact format of the pref.
i.e. it's a comma-separated list of hostnames to match _exactly_ against
URLs.

::: netwerk/base/nsIURIClassifier.idl
@@ +83,5 @@
>     * Synchronously classify a URI with a comma-separated string
>     * containing the given tables. This does not make network requests.
>     * The result is an array of table names that match.
>     */
> +  [noscript] StringArrayRef classifyLocalWithTables(in nsIURI aURI,

I believe that the only user of this function is the flash blocking, which doesn't support adding extra entries, so we don't need to change this.

@@ +102,5 @@
>    /**
>     * Same as above, but returns a comma separated list of table names.
>     * This is an internal interface used only for testing purposes.
>     */
> +  ACString classifyLocal(in nsIURI aURI, in ACString aTables,

Since this is only used in tests, which already do their own thing, I don't think we need to change this.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1851,5 @@
> +      nsCOMPtr<nsIRunnable> cbRunnable = NS_NewRunnableFunction(
> +        "nsUrlClassifierDBService::AsyncClassifyLocalWithTables",
> +        [callback]() -> void {
> +          callback->OnClassifyComplete(NS_OK, // Not used.
> +                                       NS_LITERAL_CSTRING("prefs"),

This trick is not going to work anymore because TrackingURICallback::OnClassifyComplete() now looks at the list names that matched to determine whether the resource is to be blocked or annotated.

We probably need to pass strings like "urlclassifier.trackingTable.testEntries" or "urlclassifier.trackingAnnotationTable.testEntries" and then special-case these "list names" in the channel classifier.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: bbb.patch

Comment 4

[François Marier [:francois]]

Comment on attachment 8998192 [details] [diff] [review]
bbb.patch

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

::: netwerk/base/nsIURIClassifier.idl
@@ +82,5 @@
>    /**
>     * Synchronously classify a URI with a comma-separated string
>     * containing the given tables. This does not make network requests.
>     * The result is an array of table names that match.
>     */

I don't think we use this anywhere for tracking protection or annotations, so I don't think we need to add the extra parameters.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1748,3 @@
>    if (aTrackingProtectionEnabled) {
>      AppendTables(mTrackingProtectionTables, tables);
> +    extraTablesByPrefs.AppendElement("tracking-whitelist-pref");

Maybe we should have constants like `TABLE_TRACKING_BLACKLIST_PREF` like we have in `nsChannelClassifier.cpp`?

@@ +1924,5 @@
>    return gDbBackgroundThread->Dispatch(r, NS_DISPATCH_NORMAL);
>  }
>  
>  NS_IMETHODIMP
>  nsUrlClassifierDBService::ClassifyLocalWithTables(nsIURI *aURI,

Again, as far as I can see, we don't use this for trackers, so we don't need to honour the testEntries prefs here. We're also hoping to phase this one out at some point (i.e. when the flashblocking code goes away).

What we could do here instead is to look at the tables that are requested and `MOZ_ASSERT` that they're not tracking-related (`*-track-*` or `*-trackwhite-*`).

Comment 5

[Andrea Marchesini [:baku]]

Attachment: bbb.patch

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 9004076 [details] [diff] [review]
bbb.patch

Here a new version of the patch. I just wanted to keep in sync the 'sync' and async' version of classifyLocalWithTables.

Comment 7

[François Marier [:francois]]

(In reply to Andrea Marchesini [:baku] from comment #6)
> Here a new version of the patch. I just wanted to keep in sync the 'sync'
> and async' version of classifyLocalWithTables.

Yes, and that would make perfect sense if we weren't trying to deprecate and remove one of those functions :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5092029dac4e
Allow the adding of tracking-protection entries via prefs, r=francois

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Allow the adding of tracking-protection entries via prefs,

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 9004462 [details] [diff] [review]
Allow the adding of tracking-protection entries via prefs,

(Sorry, I made a mistake with bzexport.)

Comment 11

[Andrea Marchesini [:baku]]

My patch has been already reviewed and it's already in inbound.

njn, do you want to have a second review from glandium?
Do you mind to land the comments (if any) in a separate bug?

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5092029dac4e

Comment 13

[Nicholas Nethercote [inactive]]

No need for any extra reviews or work, my contribution to this bug was entirely accidental! (I errneously did `hg bzexport` when I didn't have any patches applied, and the patch from this bug was the tip, so it got re-uploaded to this bug.)