Closed Bug 1476967 Opened Last year Closed Last year

Allow the adding of tracking-protection entries via prefs

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch aaa.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8994873 - Flags: review?(francois)
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Whiteboard: [domsecurity-active]
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.
Attachment #8994873 - Flags: review?(francois) → review-
Attached patch bbb.patch (obsolete) — Splinter Review
Attachment #8994873 - Attachment is obsolete: true
Attachment #8998192 - Flags: review?(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-*`).
Attachment #8998192 - Flags: review?(francois) → review-
Attached patch bbb.patchSplinter Review
Attachment #8998192 - Attachment is obsolete: true
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.
Attachment #9004076 - Flags: review?(francois)
Attachment #9004076 - Flags: review?(francois) → review+
(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 :)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5092029dac4e
Allow the adding of tracking-protection entries via prefs, r=francois
Assignee: amarchesini → n.nethercote
Assignee: n.nethercote → amarchesini
https://hg.mozilla.org/mozilla-central/rev/5092029dac4e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #9004462 - Attachment is obsolete: true
Flags: needinfo?(n.nethercote)
Attachment #9004462 - Flags: review?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.