Closed Bug 1033871 Opened 10 years ago Closed 10 years ago

support allowlisting sites from tracking protection

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 8 obsolete files)

Check nsIPermissionManager before enabling tracking protection in nsChannelClassifier (
29.95 KB, patch
mmc
: review+
Details | Diff | Splinter Review 
This should probably be per-site rather than per-url.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8462909 - Attachment is obsolete: true
Attachment #8462921 - Attachment is obsolete: true
Attachment #8464297 - Attachment is obsolete: true
Attachment #8465672 - Attachment is obsolete: true
Attachment #8465764 - Attachment is obsolete: true
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)
Blocks: 1043801
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)
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)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/cdf21c0388a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: