Closed Bug 523239 Opened 10 years ago Closed 10 years ago

Have Content Security Policy get called for channel redirects

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bsterne, Assigned: bsterne)

References

Details

Attachments

(1 file, 2 obsolete files)

We need to make CSPService implement nsIChannelEventSink so we can get called when channels redirect.
Attached patch csp-channel-event-sink-v7.3 (obsolete) — Splinter Review
Make CSPService implement nsIChannelEventSink
Attached patch csp-channel-event-sink-v7.4 (obsolete) — Splinter Review
Fixed some small bit rot.
Attachment #412112 - Attachment is obsolete: true
Attachment #421162 - Flags: review?(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.
Attachment #421162 - Flags: review?(jst) → review+
Addresses jst's review comments.  Ready to be checked in.
Attachment #421162 - Attachment is obsolete: true
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 on attachment 440038 [details] [diff] [review]
csp-channel-event-sink-final

Carrying forward jst's r+.
Attachment #440038 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/a96445f7bcc3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.