558431 - CSP policy-uri policies should be fetched asynchronously

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

For performance, when CSP fetches a policy from a policy-uri, this should be done asynchronously.  This involves blocking the page display until the policy is obtained, or at the very least, stopping any outgoing requests until after the policy is available.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I *think* it would work to call Suspend on the channel from inside OnStartRequest, and then call Resume once you've got the policy file.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just be careful that you don't forget to call Resume in various error conditions. You can probably call Cancel and then Resume if you need to cancel the channel.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

And we should use an HTTP channel instead of XHR to fetch the policy.  Example of how to convert an XHR to a channel is in bug 612391.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm going to nominate this for blocking. Spinning the event loop is really scary, and probably bad for perf too.

Comment 5

[Johnny Stenback (:jst)]

Agreed.

Comment 6

[Brandon Sterne (:bsterne)]

Attachment: fix

Adds the nsIRequest representing the parent document channel into refinePolicy so we can suspend that request while we fetch the policy.  Convert the policy-uri request from an XHR to a raw channel.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 494219 [details] [diff] [review]
fix

While I'm not excited about suspending the main document channel here I don't see a better way to do this at this point. I think we should file a followup bug to avoid this in the future, so that speculative parsing can happen while we're waiting for the policy to load etc. r=jst with that bug filed.

Comment 8

[Brandon Sterne (:bsterne)]

Filed bug 617400 to address jst's comment 7.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Sorry I'm coming fashionably late to the party.

I'm a bit concerned that the patch in attachment 494219 [details] [diff] [review] had to modify the api.  Before this patch, the flow in nsDocument was:

1.  create instance of nsIContentSecurityPolicy
2.  scan the HTTP request headers (for reporting)
3.  refine the policy with any CSP header data
4.  attach the CSP to the principal.

It seems to me a more attractive solution to update step 2 to grab a reference to the document channel than to modify the API in step 3.   In fact, the channel is already passed into the call to ScanRequestData() in step 2, so this should be a simpler approach.  (If there's a reason we need to put it in parameters to RefinePolicy(), then RefinePolicy should probably be renamed to reflect the fact that it may pause the channel, maybe "refinePolicyAndMaybePauseLoadingChannel()" or "PauseAndLoadPolicy()".

Instead of the current approach, I suggest grabbing the reference to the channel in ScanRequestData(), omitting the extra parameter to RefinePolicy(), and then keeping the rest of the logic as-is.

Also, it's probably a good idea to make sure that ScanRequestData() is always called before RefinePolicy() since if they're called in reverse order, channel pausing can't work.  I think we should add an assertion in RefinePolicy() that fires if ScanRequestData() hasn't been called yet.  (There must always be a channel passed to ScanRequestData, so it's straightforward to test if ScanRequestData has been called based on whether or not there's a reference to it in the CSP instance.)

Comment 10

[Brandon Sterne (:bsterne)]

Attachment: simpler fix

Addresses comment 9

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 505993 [details] [diff] [review]
simpler fix

>@@ -309,31 +371,29 @@ CSPRep.fromString = function(aStr, self)
         }
       }
 
-      var req = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
-                  .createInstance(Components.interfaces.nsIXMLHttpRequest);  
+      // suspend the parent document request while we fetch the policy-uri
+      try {
+        docRequest.suspend();
+        var chan = gIoService.newChannel(uri.asciiSpec, null, null);
+        if (!chan) {
+          CSPError("Error creating channel to fetch policy-uri.");
+          docRequest.resume();
+          return CSPRep.fromString("allow 'none'");
+        }

Per our discussion in person, we should change this code around a bit to always resume the docRequest, even in the case where newChannel throws (which I believe is the normal error case, returning a null channel may not be possible, even). That way in the case where any of this fails we'll still load the document, but we'll fall back on a CSP policy that allows nothing. And alternatively, you could also move the docRequest.suspend() call to the end of this method and then you wouldn't ever need to resume the document request.

And it seems this would be testable as well, so if possible, write a test that causes the above newChannel to throw and test that the document with the async CSP policy ends up being loaded, but with a "allow 'none'" policy. 

r=jst with that!

Comment 12

[Brandon Sterne (:bsterne)]

Attachment: final fix (with test)

Addresses comment 11 review feedback and adds a unit test.

Comment 13

[Brandon Sterne (:bsterne)]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9ce4d80efab6

Comment 14

[Brandon Sterne (:bsterne)]

Backed out due to orange:
http://hg.mozilla.org/mozilla-central/rev/9797baa9246d

Comment 15

[Brandon Sterne (:bsterne)]

Attachment: final fix (with passing tests)

Had to split out CSP xpcshell tests for policy-uri due to asynchronous fetching.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 511062 [details] [diff] [review]
final fix (with passing tests)

Patch looks good. If you didn't already, we should run this through try to make sure all tests look good.

Comment 17

[Brandon Sterne (:bsterne)]

(In reply to comment #16)
> Patch looks good. If you didn't already, we should run this through try to make
> sure all tests look good.

I did run this through try right after I posted the patch:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a7c20e7f85ef

Everything looks good to go and I'm going to attempt to land this tomorrow morning when I'll have time to watch the tree.

Comment 18

[Brandon Sterne (:bsterne)]

Comment on attachment 511062 [details] [diff] [review]
final fix (with passing tests)

I just overhead some talk that landings are soon going to be restricted to hardblockers and explicit approval so requesting approval.

Comment 19

[Brandon Sterne (:bsterne)]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/7b4760f315a7

Comment 20

[Ed Morley [:emorley]]

Comment on attachment 511062 [details] [diff] [review]
final fix (with passing tests)

Has landed already, so clearing redundant approval2.0?, since it's causing this fixed bug to erroneously appear under approval requests here:
http://office.smedbergs.us/blocker-report/