515797 - Allow necko to create channels which are aware of Content Security Policy

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brandon Sterne (:bsterne)]

Since nsIContentPolicy is not called for channel redirects, we need a way to create channels which are annotated with Content Security Policy information, so that when redirects occur, the information needed to determine whether or not to allow the redirect is available within the channel.

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: Add necko API to create CSP-aware channels

Patch adds necko API NS_NewChannelWithPolicy which works just like NS_NewChannel but takes an additional nsChannelPolicy container (also new) which can be propagated from an old channel to a new channel when redirects occur.

Comment 2

[Brandon Sterne (:bsterne)]

I see Sid has included IContentSecurityPolicy.idl in the CSP Core Modules (bug 515433) so I'll remove it from this patch.  Updated patch forthcoming.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

What's a content security policy and why is it not enough to store it on channels via nsIWritablePropertyBag2?

Comment 4

[Brandon Sterne (:bsterne)]

We need the Content Security Policy _and_ the load type associated with the channel, so instead of putting both into the property bag separately, jst suggested creating a wrapper object to encapsulate these two items (and any future additional items) and just placing the wrapper item in the channel.

A content security policy, in brief, is a representation of a document's declared policy for how content should behave, where it should load from, etc.

Comment 5

[Brandon Sterne (:bsterne)]

(In reply to comment #4)
> and just placing the wrapper item in the channel.

That should have been "in the channel's property bag".

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Sorry, I thought you were doing more changes to necko than you're actually doing. But why not keep this entirely in content?

Comment 7

[Brandon Sterne (:bsterne)]

There are going to be callers of this API which are not content-aware or which are before content in the build order, e.g. libpr0n.

Comment 8

[Brandon Sterne (:bsterne)]

Attachment: Minimal necko patch

This patch now depends on bug 515433 for IContentSecurityPolicy.idl

Comment 9

[Jason Duell]

Comment on attachment 402211 [details] [diff] [review]
Minimal necko patch

>+++ b/netwerk/base/public/nsNetUtil.h
>+NS_NewChannelWithPolicy(nsIChannel           **result,
>+                        nsIURI                *uri,
>+                        nsIIOService          *ioService = nsnull,
>+                        nsILoadGroup          *loadGroup = nsnull,
>+                        nsIInterfaceRequestor *callbacks = nsnull,
>+                        PRUint32               loadFlags = nsIRequest::LOAD_NORMAL,
>+                        nsISupports           *channelPolicy = nsnull)
>+{

So all you're doing here is duplicating the entire NS_NewChannel function, in
order to add another default argument to the end.  Why don't we avoid the code
duplication by just changing NS_NewChannel to take the channelPolicy = nsnull
argument?   Biesi, do you have any opinion?  

Everything else in the patch looks good.

Comment 10

[Brandon Sterne (:bsterne)]

Attachment: CSP necko changes

Carrying forward jduell's r+.  biesi, please provide superreview at your earliest convenience so we can move forward with landing.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

So I still don't think that this function belongs in necko, since necko doesn't do anything with this. indeed you're not even using a necko prefix for your channel property.

Since you're adding nsIChannelContentPolicy to necko in this patch, why does your function take an nsISupports argument instead?

Please don't add completely undocumented interfaces. For example I have no idea what the loadType attribute would specify. I'm also not happy with a necko interface depending on an interface in content (i.e. nsIContentSecurityPolicy)

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 421161 [details] [diff] [review]
CSP necko changes

Also:
+++ b/content/base/src/nsChannelPolicy.h
+class NS_COM nsChannelPolicy : public nsIChannelPolicy

I don't think you want NS_COM here. This isn't in xpcom, and if it were you wouldn't want to export it.


+++ 

b/netwerk/base/public/nsIChannelPolicy.idl
+/* A container for policy information to be used during channel creation */

This line should be right in front of the [scriptable...] line, so that doxygen can tell that the comment is about the interface.

Comment 13

[Brandon Sterne (:bsterne)]

Attachment: necko patch v3

Addresses superreview comments: moves content-dependent interface out of necko, added documentation, got rid of NS_COM in implementation, rebased to trunk.

Comment 14

[Brandon Sterne (:bsterne)]

Ah... now I remember why we were putting this interface in necko.  See comment 7.  There are non-content users who need to use it.  Because of build ordering, there doesn't seem to be a better place to put it.

Biesi, if we leave nsIChannelPolicy in necko would you be more comfortable
if I changed the type of the nsContentSecurityPolicy attribute to nsISupports?  This would prevent any build-time dependencies.  We'd have to QueryInterface to nsIChannelPolicy at the various call sites, but that's a small cost.

Comment 15

[Brandon Sterne (:bsterne)]

Attachment: csp-necko-patch-v4

Updated patch per comment 14.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 424022 [details] [diff] [review]
csp-necko-patch-v4

+++ b/netwerk/base/public/nsIChannelPolicy.idl
+*/
+
+[scriptable, uuid(18045e96-1afe-4162-837a-04691267158c)]
+interface nsIChannelPolicy : nsISupports

I'd remove the empty line, and the */ line needs another space of indentation

+++ b/netwerk/base/public/nsNetUtil.h
-              nsIIOService          *ioService = nsnull,    // pass in nsIIOService to optimize callers
+              nsIIOService          *ioService = nsnull,     // pass in nsIIOService to optimize callers

any particular reason for the change in indentation?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 424022 [details] [diff] [review]
csp-necko-patch-v4

Though you've ignored the part in comment 11 about not using nsIChannelPolicy instead of nsISupports here. Since this is in necko, you should probably also add the define for this property to nsChannelProperties.h.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #17)
> (From update of attachment 424022 [details] [diff] [review])
> Though you've ignored the part in comment 11 about not using nsIChannelPolicy

s/not//

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It looks like comments 16 and 17 are unaddressed in the latest patch so I'm clearing checkin-needed.  If I'm wrong please reset the keyword.

Comment 20

[Brandon Sterne (:bsterne)]

Attachment: csp-necko-patch-v5

(In reply to comment #16)
> I'd remove the empty line, and the */ line needs another space of indentation

Fixed.

> any particular reason for the change in indentation?

Nope, it was accidental.  Reverted.

(In reply to comment #17)
> Though you've ignored the part in comment 11 about not using nsIChannelPolicy
> instead of nsISupports here. Since this is in necko, you should probably also
> add the define for this property to nsChannelProperties.h.

I apologize, but I don't understand this comment (especially after the patch has received superreview+).  Doesn't my comment 14 address why this interface needs to be inside necko?  And since nsIChannelPolicy is in netwerk, why do I need to define the property inside nsChannelProperties.h?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #20)
> (In reply to comment #17)
> > Though you've ignored the part in comment 11 about not using nsIChannelPolicy
> > instead of nsISupports here. Since this is in necko, you should probably also
> > add the define for this property to nsChannelProperties.h.
> 
> I apologize, but I don't understand this comment

Sorry, that "not" shouldn't have been there. What I meant was:
+              nsISupports           *channelPolicy = nsnull)

That should be an nsIChannelPolicy instead of an nsISupports.

> (especially after the patch has received superreview+).

The changes seemed straightforward enough that I just marked sr+, I didn't think I needed to take a look at the version with the changes again.

>  And since nsIChannelPolicy is in netwerk, why do I
> need to define the property inside nsChannelProperties.h?

The reason to put it in nsChannelProperties.h is so that people can refer to the property with a symbolic name, without having to remember the exact literal string.

Comment 22

[Brandon Sterne (:bsterne)]

Attachment: csp-necko-patch-v6

Addresses comments 16 and 17.  Carrying forward review and superreview.  Ready to land.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/40df35d082a7