523239 - Have Content Security Policy get called for channel redirects

Status: RESOLVED FIXED

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

Description

[Brandon Sterne (:bsterne)]

We need to make CSPService implement nsIChannelEventSink so we can get called when channels redirect.

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: csp-channel-event-sink-v7.3

Make CSPService implement nsIChannelEventSink

Comment 2

[Brandon Sterne (:bsterne)]

Attachment: csp-channel-event-sink-v7.4

Fixed some small bit rot.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 421162 [details] [diff] [review]
csp-channel-event-sink-v7.4

- In CSPService::OnChannelRedirect():

+  props->GetPropertyAsInterface(NS_LITERAL_STRING("csp.channelPolicy"),
+                                NS_GET_IID(nsISupports),
+                                getter_AddRefs(policyContainer));

Seems like this should use NS_CHANNEL_PROP_CHANNEL_POLICY now that bug 515797 has landed.

+  if (!policyContainer)
+    return NS_OK;
+
+  // see if we have a valid nsIChannelPolicy containing CSP and load type
+  nsCOMPtr<nsIChannelPolicy> channelPolicy(do_QueryInterface(policyContainer));
+  if (!channelPolicy)
+    return NS_OK;

I wonder if we should disallow redirects if we for some reason find a non-null channel policy and it does *not* implement nsIChannelPolicy? Seems like that's either a bad bug in our code, or someone's monkeying around with channel policies that we probably don't want to support? If not, we can drop the first of those two if checks.

+  if (aDecision != 1) {
+    newChannel->Cancel(NS_BINDING_FAILED);
+  }
+  
+  // if the redirect is permitted, propagate the Content Security Policy
+  // and load type to the redirecting channel
+  else {

I'd recommend moving the above comment inside the else case to make it much harder for someone to later on inserting code between the if and the else here. If I was writing this I'd write it like this:

  if (...) {
    // comments...
    do stuff
  } else {
    // comments...
    do other stuff
  }

+      props->SetPropertyAsInterface(NS_LITERAL_STRING("csp.channelPolicy"),
+                                    channelPolicy);

Use NS_CHANNEL_PROP_CHANNEL_POLICY here too.

r=jst with that looked into.

Comment 4

[Brandon Sterne (:bsterne)]

Attachment: csp-channel-event-sink-final

Addresses jst's review comments.  Ready to be checked in.

Comment 5

[Brandon Sterne (:bsterne)]

I'll check this in when the tree is green.  FYI, the increased size of the final patch is only due to whitespace fixes.

Comment 6

[Brandon Sterne (:bsterne)]

Comment on attachment 440038 [details] [diff] [review]
csp-channel-event-sink-final

Carrying forward jst's r+.

Comment 7

[Brandon Sterne (:bsterne)]

http://hg.mozilla.org/mozilla-central/rev/a96445f7bcc3