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.

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.

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.

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

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.

(In reply to comment #4) > and just placing the wrapper item in the channel. That should have been "in the channel's property bag".

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

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.

This patch now depends on bug 515433 for IContentSecurityPolicy.idl

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.

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

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 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.

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

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.

Updated patch per comment 14.

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 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.

(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//

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.

(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?

(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.

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