Closed Bug 1329288 Opened 7 years ago Closed 7 years ago

Content policy check in docshell was incorrectly removed

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 + fixed

People

(Reporter: bzbarsky, Assigned: ckerschb)

References

Details

Attachments

(3 files, 3 obsolete files)

bug_1329288_bring_back_contentpolicy_check_within_docshell.patch
2.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
bug_1329288_test_contentpolicy_block_new_window.patch
5.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
bug_1329288_webrequest.patch
4.95 KB, patch
bzbarsky
: review+
kmag
: review+
Details | Diff | Splinter Review 
[Tracking Requested - why for this release]: User-visible functionality regression.

Bug 1182569 removed the NS_CheckContentLoadPolicy call in nsDocShell::InternalLoad (but not any of the comments referring to it!).  

There's an explicit comment at http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/docshell/base/nsDocShell.cpp#9834-9835 explaining why the now-removed check was there (though it's a bit confused in that !targetDocShell might mean "about to create one" or might mean "we're not being targeted at all" we should consider fixing that part to be clearer and only doing the conpol check when aWindowTarget or something), and that reason is still relevant, in addition to the mailnews breakage this caused.  Why was this check removed?

In any case, the checks done by the content security manager are clearly not good enough for providing context for docshell loads per bug 1328847 comment 27; that may need to be fixed too, if we want to change the docshell code to be conditioned on aWindowTarget.  But the check in InternalLoad needs to be brought back anyway, for the cases when we _are_ about to open a new window.

It's unfortunate that it's so hard to write a test for this.  :(
Flags: needinfo?(ckerschb)
Flags: needinfo?(bugs)
Blocks: 1328847
Note that this is breaking Thunderbird too, in addition to breaking UX around targeted links blocked by content policy.
Tracking 53+ for this user visible regression.
tracking-firefox53: ? → +
I do believe this is the most likely bugzilla entry related to a new issue filed for my extension uBlock Origin ("uBO")[1], which is that it no longer can block (filtered) popup tabs/windows with latest Nightly. All works fine for stable Firefox or older versions of Firefox.

To do its job of filtering popups, uBO implements nsIContentPolicy.shouldLoad[2] and act when shouldLoad is called for instances of TYPE_DOCUMENT. In such case, the aContext argument allows uBO to find out which window opened the new tab/window (aka potential popup) and to message that information to uBO's core process.

I have observed that with the latest Nightly, the aContext argument is always null for resource of type TYPE_DOCUMENT, whereas on stable Firefox it does properly reference the window of the new tab/window (from which uBO can get the opener property and a message manager). Without a valid aContext argument, it's no longer possible for uBO to have the proper information for its popup blocker to do its job. I do not see any possible workaround.

[1] https://github.com/gorhill/uBlock/issues/2290
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPolicy
Blocks: 1329494
> I do not see any possible workaround.

This is no longer true. So as far as my extension is concerned, this is no longer an issue. Sorry for the noise.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> There's an explicit comment at
> http://searchfox.org/mozilla-central/rev/
> 0f254a30d684796bcc8b6e2a102a0095d25842bb/docshell/base/nsDocShell.cpp#9834-
> 9835 explaining why the now-removed check was there (though it's a bit
> confused in that !targetDocShell might mean "about to create one" or might
> mean "we're not being targeted at all" we should consider fixing that part
> to be clearer and only doing the conpol check when aWindowTarget or
> something), and that reason is still relevant, in addition to the mailnews
> breakage this caused.  Why was this check removed?

My thinking was that we can't load any data if we don't call AsyncOpen2() on the docshell, which consults the securityManager which should actually perform the relevant ContentSecurity checks.

> In any case, the checks done by the content security manager are clearly not
> good enough for providing context for docshell loads per bug 1328847 comment
> 27; that may need to be fixed too, if we want to change the docshell code to
> be conditioned on aWindowTarget.  But the check in InternalLoad needs to be
> brought back anyway, for the cases when we _are_ about to open a new window.

The reason why we are not getting the right context from the LoadInfo is because we only pass a loadingNode (requestingContext) to the loadInfo for TYPE_SUBDOCUMENT loads. Load of TYPE_DOCUMENT get created with the loadingWindow. That behavior originates from Bug 1105556 [1].

I would imagine for all the other cases the right contentPolicy check is performed.

Olli, what do you think, as a first measure, should we just bring back the contentPolicy checks within nsDocShell with the downside that security checks are performed twice (once in nsDocShell and once within the contentSecurityManager) or should we immediately try to identify the relevant cases and either pass the right information to the loadInfo so the securityManager can perform the relevant checks or should we perform some special casing and perform some security checks within nsDocShell?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8741109&action=diff
Flags: needinfo?(ckerschb)
Attachment #8824918 - Flags: review?(bugs)
Please note that the patch within comment 5 just brings back the removed content policy check from Bug 1182569.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8806091&action=diff
Status: NEW → ASSIGNED
> My thinking was that we can't load any data if we don't call AsyncOpen2() on the docshell

Right, but this check is guarding against side-effects other than just loading of data!  Those side effects (opening a new window) need to not happen if content policy blocks the load.  And that's what the comments say.

> Load of TYPE_DOCUMENT get created with the loadingWindow

OK, then why do we not propagate that window through to the content policy checks as the context?

Have you manually checked that the patch fixes the "targeted load which is blocked by content policy opens a new window anyway" problem?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> > Load of TYPE_DOCUMENT get created with the loadingWindow
> 
> OK, then why do we not propagate that window through to the content policy
> checks as the context?

Well, at the moment we only store the windowId but not the actual window within the Loadinfo
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#253
Probably in a follow up bug we could store the actual window and pass that to contentPolicy checks within asyncOpen2() for TYPE_DOCUMENT loads.

> Have you manually checked that the patch fixes the "targeted load which is
> blocked by content policy opens a new window anyway" problem?

I am trying to write an automated test for it. Probably there is a better way to actually check that the right context is passed to contentPolicies (not just check for null). The test fails when applied on top of the current tree but passes with the bring-back for the contentPolicy checks within docshell within this bug. What do you think?
Attachment #8825158 - Flags: feedback?(bzbarsky)
Comment on attachment 8825158 [details] [diff] [review]
bug_1329288_test_contentpolicy_block_new_window.patch

Thank you for putting this together!

This is testing whether we ever make a content policy call with the right URL and context, not whether we open the window.  To test the latter you do not want to test window.open (which always opens).  You want something more like:

  <a href="http://test1.example.org/tests/docshell/test/navigation/file_contentpolicy_block_window.html"
     target="_blank"
     id="testlink">This is a link</a>

and then in script:

  document.getElementById("testlink").click()

to trigger the load.  Or click the link in a manual test.  No new window or tab should be opened, and I expect on current trunk one does get opened.

>+setTimeout( function() { window.open(TESTURL, "_blank", "width=10,height=10"); }, 0);

Why is this needed?  Is content policy registration async in some way?  Does it not notify when it completes?

Put another way, how do we know that waiting for 0ms is long enough?
Attachment #8825158 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8824918 [details] [diff] [review]
bug_1329288_bring_back_contentpolicy_check_within_docshell.patch

This is bringing back the old code, which is good for now.
And assuming we get a test for this, the current comment higher up in the file is probably good enough, even though it is quite far away from this particular code.
Flags: needinfo?(bugs)
Attachment #8824918 - Flags: review?(bugs) → review+
Hey Olli, as suggested by Boris within comment 9, we should rather use document.getElementById("testlink").click(). I did that within this updated patch.

If you have a suggestion on how to check for the right context other than verifying it's not null, I would like to know - thanks!
Attachment #8825158 - Attachment is obsolete: true
Attachment #8825333 - Flags: review?(bugs)
No longer blocks: 1329494
Comment on attachment 8825333 [details] [diff] [review]
bug_1329288_test_contentpolicy_block_new_window.patch

>+<!-- have a testlink which we can use for the test to open a new window -->
>+<a href="http://test1.example.org/tests/docshell/test/navigation/file_contentpolicy_block_window.html"
>+   target="_blank"
>+   id="testlink">This is a link</a>
>+
>+
>+<script class="testbody" type="text/javascript">
>+/*
>+ * Description of the test:
>+ * The test tries to open a new window and makes sure that a registered contentPolicy
>+ * gets called with the right (a non null) 'context' for the TYPE_DOCUMENT load.
>+ */
>+
>+const Cc = SpecialPowers.Cc;
>+const Ci = SpecialPowers.Ci;
>+const TESTURL =
>+  "http://test1.example.org/tests/docshell/test/navigation/file_contentpolicy_block_window.html";
Do you need this...

>+
>+// Content policy / factory implementation for the test
>+var policyID = SpecialPowers.wrap(SpecialPowers.Components).ID("{b80e19d0-878f-d41b-2654-194714a4115c}");
>+var policyName = "@mozilla.org/testpolicy;1";
>+var policy = {
>+  // nsISupports implementation
>+  QueryInterface: function(iid) {
>+    iid = SpecialPowers.wrap(iid);
>+    if (iid.equals(Ci.nsISupports) ||
>+        iid.equals(Ci.nsIFactory) ||
>+        iid.equals(Ci.nsIContentPolicy))
>+      return this;
>+    throw SpecialPowers.Cr.NS_ERROR_NO_INTERFACE;
>+  },
>+
>+  // nsIFactory implementation
>+  createInstance: function(outer, iid) {
>+    return this.QueryInterface(iid);
>+  },
>+
>+  // nsIContentPolicy implementation
>+  shouldLoad: function(contentType, contentLocation, requestOrigin, context, mimeTypeGuess, extra) {
>+
>+    if (SpecialPowers.wrap(contentLocation).spec !== TESTURL) {
... couldn't you just use document.getElementById("testlink").href here. Might be a bit easier to read and no two copies of the url string in the file.



>+    ok(context, "context is not allowed to be null");
so isn't context == this in e10s and in non-e10s xul:browser element.
e10s case would be easier to test, but doesn't SpecialPower also let one to access xul:browser
So, could the check be something like
if (context == this) {
  ok(true, "Expected context in e10s");
} else {
  // do some wrapping stuff
  ok(browserElement.localName, "browser", "Expected context in non-e10s")
}
Attachment #8825333 - Flags: review?(bugs) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3884fa2026
Bring back ContentPolicy check within nsDocShell. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7136147b3a26
Test ContentPolicy blocks opening a new window. r=smaug
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ba64015065
Update test_contentpolicytype_targeted_link_iframe to not call finish several times. r=me
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68009145&repo=mozilla-inbound
Flags: needinfo?(ckerschb)
(In reply to Carsten Book [:Tomcat] from comment #15)
> backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=68009145&repo=mozilla-
> inbound

Shane, Kris, probably you can provide a little guidance here. After some local debugging I found out that only test_webRequest_frames [1] causes troubles. The reason is (I think) because the test requires certain events to happen in order which is not the case anymore.

In the old world, we only had one contentPolicy check for iframes which happenend within nsDocShell.cpp and this test worked just fine. After Bug 1182569 landed, we removed the ContentPolicy check from nsDocShell.cpp because now we are going through nsContentSecurityManager.cpp which performs the ContentPolicy check and this test still worked fine.

Now, within this bug we are adding back the contentPolicy check to docShell but are also consulting the nsContentSecuritymanager which also performs ContentPolicy checks. In other words, contentPolicy checks are now performed twice for iframe loads which causes the test to fail.

I tried to update the test and shuffle events into optional_events but I simply don't fully understand when those events are fired. Please also have a look at the link on top of this comment to see why the patches were backed out.

Any help is highly appreciated - thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html#282
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(ckerschb)
I wasn't subscribed and missed all the action ;-(
Thanks for working on this. I can confirm that the patch fixes the failure in the TB Mozmill test suite.
Basically calling CSP twice causes onBeforeRequest and onCompleted to be called twice, which would be a bug, and its good the test caught it.  I'm going to look into why bug 1182569 didn't cause a failure, seems like it should have.

The webrequest events are fairly specific in order and when the occur in the cycle, you can see the docs on that here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

The issue is going to have to be solved either by not calling CSP twice (IMO the best solution) or figuring out in the webrequest csp listener a) when we're going to get two calls and b) which call is the correct call to use.  Code handling this is here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#242
Flags: needinfo?(mixedpuppy)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Now, within this bug we are adding back the contentPolicy check to docShell
> but are also consulting the nsContentSecuritymanager which also performs
> ContentPolicy checks. In other words, contentPolicy checks are now performed
> twice for iframe loads which causes the test to fail.

I haven't looked into these changes, so the answer is probably obvious, but,
is there a reason we need to run content policy checks twice? Do the
nsContentSecurityManager checks add something to the checks already done by
the docShell?

If we can possibly avoid running those checks twice, I think we probably
should. Leaving the slight inefficiency aside, I'd be extremely surprised if
WebRequest is the only code affected by this. At the least, it will almost
certainly affect some security/privacy/content blocking add-ons.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #20)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)

> I'd be extremely surprised if
> WebRequest is the only code affected by this.

This was also on my mind, solving it for webrequest may just be the start of whack-a-mole.
(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> Basically calling CSP twice causes onBeforeRequest and onCompleted to be
> called twice, which would be a bug, and its good the test caught it.  I'm
> going to look into why bug 1182569 didn't cause a failure, seems like it
> should have.

But 1182569 removed the contentPolicy check from within nsDocShell and added it to the contentSecuritymanager hence CSP was only consulted once. Hence no test failure.

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> If we can possibly avoid running those checks twice, I think we probably
> should. Leaving the slight inefficiency aside, I'd be extremely surprised if
> WebRequest is the only code affected by this. At the least, it will almost
> certainly affect some security/privacy/content blocking add-ons.

Obviously adding back the content policy check within nsDocShell is just a temporary measure to fix the problem described in this bug. The ultimate goal is to have only the contentSecurityManager call contentPolicies and not doing those checks twice.

I think we can bail out of contentPolicy checks for TYPE_DOCUMENT and TYPE_SUBDOCUMENT within the contentSecurityManager as a temporary measure. The only call I see that uses TYPE_SUBDOCUMENT and does not go through nsDocshell is within the importManager [1]. I don't know exactly what this code is doing - we would have to evaluate.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8643950&action=diff
Smaug, as described in comment 17, test_ext_webrequest_basic.html is failing because content policies are consulted twice. Once within nsDocShell (since we are adding back the check) and once within the contentSecurityManager. As previously discussed consulting contentPolicies twice is not that great, not only for performance reasons.

How would you feel if we temporarily bail out of contentPolicy checks for TYPE_DOCUMENT and TYPE_SUBDOCUMENT until we have found a better solution to fix this bug (probably after 53 ships). Only problem with that approach is TYPE_SUBDOCUMENT is not only used within nsDocSHell but also within ImportManager.cpp. Bailing for that load as well is not desired. We would have to add yet some special casing. E.g. by looking at LOAD_BACKGROUND (which the importManager uses) or also by looking at the securityFlag (SEC_REQUIRE_CORS_DATA_INHERITS used by importManager). Doesn't sound that great to me, but on the other hand probably our best choice. Any thoughts?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8643950&action=diff
Flags: needinfo?(bugs)
Before we get too far off into the weeds, some things need mentioning:

> Basically calling CSP twice causes onBeforeRequest and onCompleted to be called twice, which would be a bug

You had this problem before too.  Before bug 1182569, this markup:

  <a href="whatever" target="_blank">Click me</a>

followed by the user clicking on the link, would call CSP twice for that load.  The first time in the original context, before opening the new window, the second time in the new window.

Of course if we just restore the check in InternalLoad, this will now cause CSP to be called three times instead, I think.

> The webrequest events are fairly specific in order and when the occur in the cycle

That may or may not match Gecko's requirements in terms of the interaction of CSP checks and window-opening.

Specifically, in my example above, if webrequest cancels the load, should the new window still get opened?
Flags: needinfo?(mixedpuppy)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24)
> Before we get too far off into the weeds, some things need mentioning:
> 
> > Basically calling CSP twice causes onBeforeRequest and onCompleted to be called twice, which would be a bug
> 
> You had this problem before too.  Before bug 1182569, this markup:
> 
>   <a href="whatever" target="_blank">Click me</a>

We do have link/click test, but not one with a target.  Was the double call specific to using target?

> > The webrequest events are fairly specific in order and when the occur in the cycle
> 
> That may or may not match Gecko's requirements in terms of the interaction
> of CSP checks and window-opening.

I'm not against addressing this in webrequest, but we need a reliable way to identify situations where we need to ignore or coalesce the calls.  Is it best to only use the first call and ignore calls after, or to try and use only the last call?  I do think there is potential to cause problems in other areas with the additional csp calls, particularly in addons, but that problem kind of goes away in 57.

> Specifically, in my example above, if webrequest cancels the load, should
> the new window still get opened?

I'd have to test that situation and see what happens now.  What happens with any code using CSP to cancel the request in that situation?  I'd assume [perhaps incorrectly] that webrequest would have the same behavior.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24)
> > Before we get too far off into the weeds, some things need mentioning:
> > 
> > > Basically calling CSP twice causes onBeforeRequest and onCompleted to be called twice, which would be a bug
> > 
> > You had this problem before too.  Before bug 1182569, this markup:
> > 
> >   <a href="whatever" target="_blank">Click me</a>
> 
> We do have link/click test, but not one with a target.  Was the double call
> specific to using target?

Actually, am correcting myself.  It does use target=_blank in the click tests.
> Was the double call specific to using target?

Yes, and to the target not targeting an existing window.

> What happens with any code using CSP to cancel the request in that situation? 

The whole point of the current setup (before the recent change that broke it) is that if content policy (not the same thing as CSP) cancels the request the new window should not be opened.

Again, it's not clear to me how that fits in with webrequest and its even ordering guarantees...
We should be ignoring the calls for clicks. We currently only use the content policy to allow monitoring/blocking loads of non-HTTP URLs, so we'll never get that behavior for normal web links anyway (and we currently don't actually support blocking those requests, because we can't handle content policy checks asynchronously).

In the future, we should support blocking the window opening from the webNavigation API instead, but that doesn't really have any bearing on this.
Shane, Kris, thanks for the speedy responses. Given all that info - any chance we could disable the iframe subtest within test_ext_webrequest_basic.html (e.g. comment it out with a comment and a follow up bug number) so that we can land the patches within this bug? Or what is the proposed way forward here?
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8826286 [details] [diff] [review]
bug_1329288_bail_out_conpol_checks.patch

I'm not sure whether I should comment here. With this patch applied additionally, our TB Mozmill test still passes.
No, we need to make sure we're only dispatching one event per request. That probably just means ignoring the check generated for the link click and using the one generated for the load.
Flags: needinfo?(kmaglione+bmo)
The current link click test is passing just fine, since it it testing http requests.  It's a non-http request (in this case a data:url loaded in an iframe) test that is failing.  I'd presume a link click on a non-http url would have the same problem.

This is the second or maybe third failure in webrequest related to changes in document loading/csp/etc, so I'd say no, we cant disable this test.
Flags: needinfo?(mixedpuppy)
Comment on attachment 8826286 [details] [diff] [review]
bug_1329288_bail_out_conpol_checks.patch

(In reply to Jorg K (GMT+1) from comment #30)
> I'm not sure whether I should comment here. With this patch applied
> additionally, our TB Mozmill test still passes.

Given all the info that, I think we all agree that we definitely do not want that patch (or any variation of it) to land. The problem has to be fixed within webrequest code. Hence also no ni? from smaug needed anymore.
Attachment #8826286 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Hmm, is there a misunderstanding here? I applied attachment 8824918 [details] [diff] [review]. Result: Our test passes.
Then I applied attachment 8826286 [details] [diff] [review] additionally. Our test *still* passes. I'm a little confused how you derive from this, that the patch is no good.
(In reply to Jorg K (GMT+1) from comment #34)
> Hmm, is there a misunderstanding here? I applied attachment 8824918 [details] [diff] [review]
> [details] [diff] [review]. Result: Our test passes.
> Then I applied attachment 8826286 [details] [diff] [review] additionally.
> Our test *still* passes. I'm a little confused how you derive from this,
> that the patch is no good.

That most likely means that the nsIContentPolicy implementation within TB is correct and does not need the additional patch vs. the webrequest implementation of nsIContentPolicy has some issue which we have to overcome. Initially we wanted to fix that within then nsContentSecurityManager but the right solution is to fix the problem within webrequests shouldLoad implementation.
Comment on attachment 8824918 [details] [diff] [review]
bug_1329288_bring_back_contentpolicy_check_within_docshell.patch

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

::: docshell/base/nsDocShell.cpp
@@ +9871,5 @@
> +    if (!requestingPrincipal && aReferrer) {
> +      rv =
> +        CreatePrincipalFromReferrer(aReferrer, getter_AddRefs(requestingPrincipal));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

Tanvi just pointed out that we moved the triggeringPrincipal fallback within LoadURI, and have an assert on top of InternalLoad that we always have a valid triggeringPrincipal. That means that the fallback to create a principal from the referrer is in fact dead code.
(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> The current link click test is passing just fine, since it it testing http
> requests.  It's a non-http request (in this case a data:url loaded in an
> iframe) test that is failing.  I'd presume a link click on a non-http url
> would have the same problem.

It should, yes, and possibly even a worse problem, so we should probably add a link click test for a data: URL, too.

> This is the second or maybe third failure in webrequest related to changes
> in document loading/csp/etc, so I'd say no, we cant disable this test.

Just to be pedantic, these issues have been with content policy handling, not CSP. CSP is largely unrelated to content policies.

Christoph, do you have any suggestions on how we should decide which shouldLoad checks to ignore, and which to process?
Flags: needinfo?(ckerschb)
(In reply to Kris Maglione [:kmag] from comment #37)
> Christoph, do you have any suggestions on how we should decide which
> shouldLoad checks to ignore, and which to process?

So, this is a hard problem because most likely all of those calls to SHouldLoad will look more or less identical. I don't know how practical it is to actually maintain some kind of map that allows you to only handle the first call and ignore following shouldLoad calls for the same load. Boris, any suggestions?
Flags: needinfo?(ckerschb) → needinfo?(bzbarsky)
Ignoring the calls from the content security manager right now is not too hard, because they don't have the right context, right?  But we want to fix that anyway, so that won't last.

A reasonable long-term solution for this might be to:

1)  Fix the check done from the content security manager to include the right context.
2)  Change the docshell check to only be done when it would really affect the opening
    of a new window (i.e. not for untargeted loads).
3)  Make the docshell check use a different content policy type to indicate what it's _really_ asking.

and then filtering that new type in webrequest would be easy.

Note that #1 is a prerequisite for #2 happening to not break Thunderbird, unless we provide them with some other way to avoid using content policy for the thing they're doing.  Which, long-term, we should.

In the short term, we can probably do something like this:

A) Have docshell use a different type for the "have a target" vs "no target" cases.
   Note that the "have a target" case will end up doing a second check too,
   falling into the "no target" case that time, all from docshell code.
B) Have the webrequest code use the type to filter out the "is it ok to open this window?" checks
   and the missing/different context to filter out the content security manager checks.

Then at the point when we fix the content security manager's context we can switch to the first approach I described.  And the corresponding webrequest test failures will remind us that we have to do step 2 of that approach.

How does that sound?
Flags: needinfo?(bzbarsky)
Blocks: 1331740
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #39)
> Ignoring the calls from the content security manager right now is not too
> hard, because they don't have the right context, right?  But we want to fix
> that anyway, so that won't last.

I think that is only true for TYPE_DOCUMENT loads, for all other types the contentSecurityManager should pass the right context. Please note that for TYPE_DOCUMENT loads the context is null when called from within the contentSecurityManager but non null when called within docShell.

> A reasonable long-term solution for this might be to:
> 
> 1)  Fix the check done from the content security manager to include the
> right context.

Right, I filed Bug 1331740 so that the loadInfo actually includes the right context and we can remove the content security check from docshell again.

> 2)  Change the docshell check to only be done when it would really affect
> the opening
>     of a new window (i.e. not for untargeted loads).

That sounds reasonable assuming that doesn't cause TB to break again.

> 3)  Make the docshell check use a different content policy type to indicate
> what it's _really_ asking.

I don't think we want that. As of now we always pass the External contentPolicy type to nsIContentPolicy implementations because of addon compat. If we would introduce a different contentPolicyType we would face addon issues again, no?

> and then filtering that new type in webrequest would be easy.
> 
> Note that #1 is a prerequisite for #2 happening to not break Thunderbird,
> unless we provide them with some other way to avoid using content policy for
> the thing they're doing.  Which, long-term, we should.
> 
> In the short term, we can probably do something like this:
> 
> A) Have docshell use a different type for the "have a target" vs "no target"
> cases.
>    Note that the "have a target" case will end up doing a second check too,
>    falling into the "no target" case that time, all from docshell code.
> B) Have the webrequest code use the type to filter out the "is it ok to open
> this window?" checks
>    and the missing/different context to filter out the content security
> manager checks.

If I am understanding that correctly you are suggesting something like:
nsIContentPolicyType TYPE_DOC_TARGET vs TYPE_DOC_NORMAL?
If it's not a problem for addons, then I am all fine for that approach.
> I think that is only true for TYPE_DOCUMENT loads

Ah, for TYPE_SUBDOCUMENT the contexts the two loads passed are the same?

> and we can remove the content security check from docshell again

Then what would prevent the unwanted new window from being opened?

And you're right... my suggestion totally breaks for this too, because the new type would not get blocked, while the real load would be.  :(

I'm really not sure how to solve this sanely.  Maybe the answer is that we should in fact change the docshell and content security manager code in some way to ensure that we only do one check, and do that check in docshell before opening the new window or something.  Or move webrequest off of relying on content policy for the thing it really wants.
Let me summarize my findings:

First, as far as I can tell we are only facing that problem for webrequests for TYPE_SUBDOCUMENT, right?
When running test_ext_webrequest_basic.html I get basically identical calls to shouldLoad [1] see details underneath for the two shouldLoad calls. The only difference I see is the mimeTypeGuess, which is empty when called from docshell (see first patch in this bug). When called from the contentSecurityManager however  it's 'text/html' [2]. The reason it's 'text/html' originates from converting the importManager [3]. Whether it might cause problems that we are passing different mimeType guesses or not, I can't really tell - I would imagine not.

Anyway, we could use the mimeTypeGuess to differentiate between the calls. I know this is a dirty hack, but it works. Within ShouldLoad of WebRequestContent [1] we then do:

+    if (policyType == 7 && mimeTypeGuess == "text/html") {
+      return Ci.nsIContentPolicy.ACCEPT;
+    }

Boris, what do you think, might that be an acceptable approach for a temporary fix? If you agree, I would prepare a patch and would flag Kris or Shane for review.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequestContent.js#81
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#237
[3] https://hg.mozilla.org/mozilla-central/rev/12742043d2a7#l1.21

ShouldLoad:
  policyType: 7
  contentLocation: data:text/plain,webRequestTest
  requestOrigin: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html
  node: [object HTMLIFrameElement]
  mimeTypeGuess:
  extra: null
 
ShouldLoad:
  policyType: 7
  contentLocation: data:text/plain,webRequestTest
  requestOrigin: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html
  node: [object HTMLIFrameElement]
  mimeTypeGuess: text/html
  extra: null
Flags: needinfo?(bzbarsky)
I dislike the hacky fix, its a fix the test fix.  I'm pretty sure that it will cover an edge case or two, but not confident it will cover others.   Will a real fix make it for uplift (next week), or be uplift-able?
(In reply to Shane Caraveo (:mixedpuppy) from comment #43)
> I dislike the hacky fix, its a fix the test fix.  I'm pretty sure that it
> will cover an edge case or two, but not confident it will cover others.  
> Will a real fix make it for uplift (next week), or be uplift-able?

In fact I am confident that this fix is reliable. I am worried about uplifting a more complex patch and would rather land a fix right now. Anyway, let's wait for Boris' thoughts on this.
I'd be OK with that as a workaround until we have a better solution.
> Boris, what do you think, might that be an acceptable approach for a temporary fix?

If we don't think it will interfere with handling of the calls made by the importmanager, yes.

> Will a real fix make it for uplift (next week), or be uplift-able?

Both seem fairly unlikely to me.
Flags: needinfo?(bzbarsky)
Attached patch bug_1329288_webrequest.patch (obsolete) — — Splinter Review 
Boris, Kris. I think filling the 'aExtra' argument within Content Policy is the more reliable option and we can precisely skip content policy checks originated from docshell. This approach will also allow webrequests to work once we remove the contentPolicy check from within docshell.
Attachment #8828365 - Flags: review?(kmaglione+bmo)
Attachment #8828365 - Flags: review?(bzbarsky)
bug_1329288_webrequest.patch doesn't apply. Please refresh.
Comment on attachment 8828365 [details] [diff] [review]
bug_1329288_webrequest.patch

So nominally aExtra is something for non-Gecko code to use and Gecko more or less promises to always pass null.

But I can live with this as a temporary hack, I guess.  With any luck this won't break any extensions.

>+    nsCOMPtr<nsIStringInputStream> sis(do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID));

Why a string input stream?  nsISupportsString seems like it would be more appropriate here.

>+++ b/dom/security/nsCSPContext.cpp

The comments here no longer match reality.  Please fix them.

Also, it's really disturbing that we have this code, because, again, aExtra is meant to be an extension point for extensions, which are free to pass whatever they want here.  So this CSP code is just buggy.  Please file a followup bug to fix it by calling a non-public-API method that takes the extra information!
Attachment #8828365 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #50)
> Why a string input stream?  nsISupportsString seems like it would be more
> appropriate here.

Agreed and updated.
 
> >+++ b/dom/security/nsCSPContext.cpp
> Also, it's really disturbing that we have this code, because, again, aExtra
> is meant to be an extension point for extensions, which are free to pass
> whatever they want here.  So this CSP code is just buggy.  Please file a
> followup bug to fix it by calling a non-public-API method that takes the
> extra information!

I filed [Bug 1332422 - CSP should not use 'aExtra' to indicate redirects within ContentPolicy] to get that fixed.
Attachment #8828365 - Attachment is obsolete: true
Attachment #8828365 - Flags: review?(kmaglione+bmo)
Attachment #8828489 - Flags: review?(kmaglione+bmo)
Attachment #8828489 - Flags: review?(bzbarsky)
Comment on attachment 8828489 [details] [diff] [review]
bug_1329288_webrequest.patch

r=me
Attachment #8828489 - Flags: review?(bzbarsky) → review+
Attachment #8828489 - Flags: review?(kmaglione+bmo) → review+
I hope someone can get this landed before the branch date ;-)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de60e0a191c7
Bring back ContentPolicy check within nsDocShell. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/828efd8ce683
Test ContentPolicy blocks opening a new window. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/69fb2fc61535
Allow content policy consumers to identify contentPolicy checks from docshell. r=bz,kmaglione
Backed out for failing test_bug375314.html on Android 4.3 opt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8408eee51c22a170aab6d88d7635da4ed07ba05b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a334bcbea07e899e172e10c34878fc0b42a9dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10f80aa43f2eea77e3e0698535141ce54db309a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=69fb2fc615351bca97b998c9d70cb7e79edffd66
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=71001906&repo=mozilla-inbound

[task 2017-01-22T07:53:07.219191Z] 07:53:07     INFO -  421 INFO TEST-START | dom/base/test/test_bug375314.html
[task 2017-01-22T07:53:07.220213Z] 07:53:07     INFO -  422 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_bug375314.html | uncaught exception - NS_ERROR_FACTORY_EXISTS: Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory] at doApply@chrome://specialpowers/content/specialpowersAPI.js:146:10
[task 2017-01-22T07:53:07.220376Z] 07:53:07     INFO -  apply@chrome://specialpowers/content/specialpowersAPI.js:211:30
[task 2017-01-22T07:53:07.221110Z] 07:53:07     INFO -  @http://mochi.test:8888/tests/dom/base/test/test_bug375314.html:71:1
[task 2017-01-22T07:53:07.221176Z] 07:53:07     INFO -      simpletestOnerror@SimpleTest/SimpleTest.js:1583:11
Flags: needinfo?(ckerschb)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #55)
> exception - NS_ERROR_FACTORY_EXISTS: Component returned failure code:
> 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory]

Oh, I forgot to unregister the factory within the test.
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/448f9880af62
Bring back ContentPolicy check within nsDocShell. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/991609b97f58
Test ContentPolicy blocks opening a new window. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9835516c8c5
Allow content policy consumers to identify contentPolicy checks from docshell. r=bz,kmaglione
Depends on: 1357369
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: