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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bsterne, Assigned: bsterne)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Attachment #399859 - Flags: review?
Attachment #399859 - Flags: review? → review?(jduell.mcbugs)
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.
Posted patch Minimal necko patch (obsolete) — Splinter Review
This patch now depends on bug 515433 for IContentSecurityPolicy.idl
Attachment #399859 - Attachment is obsolete: true
Attachment #402211 - Flags: review?(jduell.mcbugs)
Attachment #399859 - Flags: review?(jduell.mcbugs)
Depends on: 515433
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.
Attachment #402211 - Flags: superreview?(cbiesinger)
Attachment #402211 - Flags: review?(jduell.mcbugs)
Attachment #402211 - Flags: review+
Posted patch CSP necko changes (obsolete) — Splinter Review
Carrying forward jduell's r+.  biesi, please provide superreview at your earliest convenience so we can move forward with landing.
Attachment #402211 - Attachment is obsolete: true
Attachment #421161 - Flags: superreview?(cbiesinger)
Attachment #421161 - Flags: review+
Attachment #402211 - Flags: superreview?(cbiesinger)
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.
Posted patch necko patch v3 (obsolete) — Splinter Review
Addresses superreview comments: moves content-dependent interface out of necko, added documentation, got rid of NS_COM in implementation, rebased to trunk.
Attachment #421161 - Attachment is obsolete: true
Attachment #423856 - Flags: superreview?(cbiesinger)
Attachment #423856 - Flags: review+
Attachment #421161 - Flags: superreview?(cbiesinger)
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.
Posted patch csp-necko-patch-v4 (obsolete) — Splinter Review
Updated patch per comment 14.
Attachment #423856 - Attachment is obsolete: true
Attachment #424022 - Flags: superreview?(cbiesinger)
Attachment #424022 - Flags: review+
Attachment #423856 - Flags: superreview?(cbiesinger)
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?
Attachment #424022 - Flags: superreview?(cbiesinger) → superreview+
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//
Keywords: checkin-needed
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.
Keywords: checkin-needed
Posted patch csp-necko-patch-v5 (obsolete) — Splinter Review
(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?
Attachment #424022 - Attachment is obsolete: true
Attachment #436597 - Flags: superreview?(cbiesinger)
Attachment #436597 - Flags: review+
(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.
Attachment #436597 - Flags: superreview?(cbiesinger)
Addresses comments 16 and 17.  Carrying forward review and superreview.  Ready to land.
Attachment #436597 - Attachment is obsolete: true
Attachment #437106 - Flags: superreview+
Attachment #437106 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/40df35d082a7
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Depends on: 565617
You need to log in before you can comment on or make changes to this bug.