support allowlisting sites from tracking protection

RESOLVED FIXED in mozilla34

Status

()

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

People

(Reporter: mmc, Assigned: mmc)

Tracking

unspecified
mozilla34
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

29.95 KB, patch
mmc
: review+
Details | Diff | Splinter Review
This should probably be per-site rather than per-url.
Blocks: 1029886
Created attachment 8462909 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Created attachment 8462921 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8462909 - Attachment is obsolete: true
Created attachment 8464297 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8462921 - Attachment is obsolete: true
Created attachment 8465672 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8464297 - Attachment is obsolete: true
Created attachment 8465764 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8465672 - Attachment is obsolete: true
Created attachment 8466285 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8465764 - Attachment is obsolete: true
Created attachment 8466290 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8466285 - Attachment is obsolete: true
Try: https://tbpl.mozilla.org/?tree=Try&rev=8ca0f03f614b

This patch implements the backend for the allowlisting feature from https://bug1029193.bugzilla.mozilla.org/attachment.cgi?id=8459849 ("Disable protection for this site" on the right-hand side of the mockup)

- Integrates nsIPermissionManager checks into nsChannelClassifier. nsIPermissionManager can only be used on the main thread, but nsChannelClassifier is already on it. The alternative is to do the permission manager checks in each of the loaders that sets NS_LOAD_CLASSIFY_URI as a loadflag in NS_NewChannel.
- Fixes existing test_classified_annotations
- Adds test_allowlist_annotations
Comment on attachment 8466290 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

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

jduell, we're trying to land this in 34 so if you could find someone to do the review, that would be great.

Thanks,
Monica
Attachment #8466290 - Flags: review?(jduell.mcbugs)

Updated

3 years ago
Blocks: 1043801

Updated

3 years ago
Blocks: 1047705
Comment on attachment 8466290 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

Or maybe Patrick can help? Greener try: https://tbpl.mozilla.org/?tree=Try&rev=3db35d72756f
Attachment #8466290 - Flags: superreview?(mcmanus)
Comment on attachment 8466290 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

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

I think jason is the right reviewer.. let me know if you need a backstop
Attachment #8466290 - Flags: superreview?(mcmanus)
Flags: needinfo?(jduell.mcbugs)

Updated

3 years ago
Blocks: 1050348
Hey Brian,

Do you have time for this review? Jason's backed up on some b2g stuff and said he would be happy for you to review this.

Thanks,
Monica
Flags: needinfo?(jduell.mcbugs) → needinfo?(brian)
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #12)
> Do you have time for this review? Jason's backed up on some b2g stuff and
> said he would be happy for you to review this.

My r+ wouldn't do much good because I'm no longer a Necko peer, and also I'm very behind on what's happening in Necko (which is why I'm not a Necko peer). Sorry.
Flags: needinfo?(brian)
Created attachment 8469458 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier
Attachment #8466290 - Attachment is obsolete: true
Attachment #8466290 - Flags: review?(jduell.mcbugs)
Comment on attachment 8469458 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

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

Per our irc conversation.
Attachment #8469458 - Flags: review?(mcmanus)
Comment on attachment 8469458 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

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

::: content/base/src/ThirdPartyUtil.cpp
@@ +53,2 @@
>    nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(aWin);
> +  NS_ENSURE_ARG(scriptObjPrin);

NS_ENSURE_ARG is for arguments not local variables.

if (!var) {
return NS_ERROR_FOO;
}

@@ +55,3 @@
>  
>    nsIPrincipal* prin = scriptObjPrin->GetPrincipal();
> +  NS_ENSURE_ARG(prin);

dont use arg

@@ +251,5 @@
>  }
>  
> +NS_IMETHODIMP
> +ThirdPartyUtil::GetTopWindowForChannel(nsIChannel* aChannel, nsIDOMWindow** aWin)
> +{

NS_ENSURE_ARG(aWin)

@@ +266,5 @@
> +  if (!window) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsCOMPtr<nsIDOMWindow> top;

unused?

::: netwerk/base/public/mozIThirdPartyUtil.idl
@@ +149,5 @@
> +   */
> +  nsIURI getURIFromWindow(in nsIDOMWindow aWindow);
> +
> +  /**
> +   * getURIFromWindow

wrong comment

::: netwerk/base/src/nsChannelClassifier.cpp
@@ +77,5 @@
> +        nsString docURI;
> +        rv = doc->GetDocumentURI(docURI);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (StringBeginsWith(docURI,
> +            NS_LITERAL_STRING("chrome://mochikit/"))) {

I'm obligated to say yuck. Is this done in other parts of the code base I'm not aware of?

I think even a pref that says "this test needs a synthetic channel classifier uri would be much better. could that work?

@@ +85,5 @@
> +            NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +    }
> +    // Take the host/port portion so we can allowlist by site. Also ignore the
> +    // scheme, since users who put sites on the allowlist probably don't expect

I'm confused by this comment. Why aren't we using web origin here to be consistent with most things in this space. Scheme is part of what identifies the content - as much as port.

@@ +90,5 @@
> +    // allowlisting to depend on scheme.
> +    nsCOMPtr<nsIURL> url = do_QueryInterface(uri, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCString escaped = NS_LITERAL_CSTRING("https://");

nsCString escaped(NS_LITERAL_CSTRING("https://"));

@@ +91,5 @@
> +    nsCOMPtr<nsIURL> url = do_QueryInterface(uri, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCString escaped = NS_LITERAL_CSTRING("https://");
> +    nsCString temp;

nsAutoCString

::: netwerk/base/src/nsChannelClassifier.h
@@ +28,5 @@
>      ~nsChannelClassifier() {}
>      void MarkEntryClassified(nsresult status);
>      bool HasBeenClassified(nsIChannel *aChannel);
>      // Whether or not tracking protection should be enabled on this channel.
> +    nsresult ShouldEnableTrackingProtection(nsIChannel* aChannel, bool* result);

as in most of necko, its "type *var" not "type* var".

variations are tolerated just to locally match historical variations..

::: toolkit/components/url-classifier/tests/mochitest/mochitest.ini
@@ +4,5 @@
>    classifierFrame.html
>    cleanWorker.js
>    evil.css
>    evil.js
> +  evil.js

evil.js is listed twice.. necessary?
Attachment #8469458 - Flags: review?(mcmanus) → review+
Comment on attachment 8469458 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier

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

::: netwerk/base/src/nsChannelClassifier.cpp
@@ +77,5 @@
> +        nsString docURI;
> +        rv = doc->GetDocumentURI(docURI);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (StringBeginsWith(docURI,
> +            NS_LITERAL_STRING("chrome://mochikit/"))) {

D'oh! Of course, a pref. Will fix.

@@ +85,5 @@
> +            NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +    }
> +    // Take the host/port portion so we can allowlist by site. Also ignore the
> +    // scheme, since users who put sites on the allowlist probably don't expect

Let me ask our UX person about this. You are right about origin checks but I don't know if typical users would expect allowlisting a site with a grey globe in the urlbar (no scheme is shown, even!) would not also allowlist the same site with a grey or green lock icon.
Philipp, what's the user expectation if they "Disable protection for this site" on the non-SSL version of a site then later visit the SSL version, or vice versa? Would they expect the site to be allowlisted regardless of SSL, or to have to allowlisting apply only to the version they were on at the time?
Flags: needinfo?(philipp)
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #18)
> Philipp, what's the user expectation if they "Disable protection for this
> site" on the non-SSL version of a site then later visit the SSL version, or
> vice versa? Would they expect the site to be allowlisted regardless of SSL,
> or to have to allowlisting apply only to the version they were on at the
> time?

For tracking protection, SSL/non-SSL shouldn't make a difference. I.e. if I disable protection on the non-SSL site, it should also be disabled for the SSL version for that session).

If your question is about mixed content, I think that is an impossible use case, since mixed content blocking AFAIK only happens on SSL sites.

I hope I understood your question correctly :)
Flags: needinfo?(philipp)
Created attachment 8470262 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier (
Attachment #8469458 - Attachment is obsolete: true
Comment on attachment 8470262 [details] [diff] [review]
Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier (

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

Addressed Pat's review comments except for the one that phlsa resolved, carrying over r+.

Try: https://tbpl.mozilla.org/?tree=Try&rev=963f22c5fef6
Previous green try: https://tbpl.mozilla.org/?tree=Try&rev=3db35d72756f
Attachment #8470262 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf21c0388a3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cdf21c0388a3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1230351
You need to log in before you can comment on or make changes to this bug.