570505 - CSP code makes URIs out of strings out of URIs

Status: RESOLVED FIXED

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

Description

[Sid Stamm [:geekboy or :sstamm]]

in content/base/src/contentSecurityPolicy.js:
256     var newpolicy = CSPRep.fromString(aPolicy,
257                                       selfURI.scheme + "://" + selfURI.hostPort);

in bug 552523#c2, it's pointed out that CSP sometimes creates strings out of URIs, then makes URIs out of those strings again.  We need to streamline this a bit to remove unnecessary work.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not only that, but the code above is just wrong unless you know for a fact something about selfURI.scheme...

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Well we do know that "selfURI" represents a document with CSP on it, and CSP only supports documents served over HTTP (or HTTPS) right now since the policy is delivered in an HTTP header.  So we can assume that the URI's scheme is http or https.

Granted, that's probably not a good assumption since we may expand CSP to protect other protocols in the future, so no time like the present to fix this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

You can have an nsIHttpChannel and HTTP headers even if the URI is not http:// (e.g. ftp over proxy works that way, as could various extension protocols).  But it sounds like we agree in general.  ;)

Comment 4

[Ian Melven :imelven]

Attachment: first cut at a patch

has passed the existing csp tests, needs a full test suite run, will make sure i do this after feedback

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 539346 [details] [diff] [review]
first cut at a patch

This looks good... I can't off the top of my head recall other places where URI->String->URI happens.

Comment 6

[Ian Melven :imelven]

i grepped through for fromString() and followed the changes i made through the code, i'll do a more thorough check through, i think

Comment 7

[Ian Melven :imelven]

did another pass through and that looks like the only place, should i r? to you, sid ?

Comment 8

[Ian Melven :imelven]

Attachment: updated patch

Comment 9

[Tanvi Vyas[:tanvi]]

Looks like this is just waiting for a review?  Sid, dveditz?

Comment 10

[Ian Melven :imelven]

(In reply to Tanvi Vyas from comment #9)
> Looks like this is just waiting for a review?  Sid, dveditz?

the patch may have bitrotted as well but should be very quick to fix that up. i'm happy to do that if someone reviews it.

Comment 11

[Tanvi Vyas[:tanvi]]

I looked over the code and Ian's patch.  My diff is a little bit different.  Specifically, for some of the functions we cannot assume that self is a URI or CSPSource, because it is called with an undefined parameter.

Also, I'm not sure if these 2 changes are needed in CSPUtils.jsm:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#857
Line 857:
-  if (self && !(self instanceof CSPSource)) {
+  if (self && (!(self instanceof CSPSource) || !(self instanceof Components.interfaces.nsIURI)) {
     self = CSPSource.create(self, undefined, false);
   }
Line 948:
-  if (self && !(self instanceof CSPSource)) {
+   if (self && (!(self instanceof CSPSource) || !(self instanceof Components.interfaces.nsIURI)) {
     self = CSPSource.create(self, undefined, false);

Comment 12

[Tanvi Vyas[:tanvi]]

My diff:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm
217c217
<   *        string or CSPSource representing the "self" source
---
>   *        URI or CSPSource representing the "self" source
608c608
<  *        string or CSPSource representing the "self" source
---
>  *        URI or CSPSource representing the "self" source


http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js
242c242,243
<                                       selfURI.scheme + "://" + selfURI.hostPort,
---
>                                       //selfURI.scheme + "://" + selfURI.hostPort,
> 				      selfURI,

Will work on testing next week.

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: New Updated Patch

All the CSP mochitests run with this patch.  jst/Sid/dveditz - can one of you review this?

I will push it to try tomorrow to make sure it passes all other mochitests too.

Comment 14

[Tanvi Vyas[:tanvi]]

Adding jst and Sid.  Please see below...

(In reply to Tanvi Vyas from comment #13)
> Created attachment 596867 [details] [diff] [review]
> New Updated Patch
> 
> All the CSP mochitests run with this patch.  jst/Sid/dveditz - can one of
> you review this?
> 
> I will push it to try tomorrow to make sure it passes all other mochitests
> too.

Comment 15

[Tanvi Vyas[:tanvi]]

Note that this patch includes minor logical changes in the code, and does not require any mochitests.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 596867 [details] [diff] [review]
New Updated Patch

Review of attachment 596867 [details] [diff] [review]:
-----------------------------------------------------------------

I know I probably told you something contradictory in person than I'm giving now, so I apologize in advance if I go back on my suggestions.  :)  And really, this bug is about just the one line in contentSecurityPolicy.js, but the rest of this clean up is really really helpful for code hygene, so we should do it all now.

I think the pattern we should make sure the parser follows is that self' parameters always get converted in this direction: (string -> nsIURI -> CSPSource), and when passed from one bit of the parser to another, it should always be a CSPSource.

For consistency we can probably just use this for parsing the self parameter in all the factory methods:

if (!(self instanceof CSPSource)) {
  self = CSPSource.create(self);
}

...and then let CSPSource.create do the type checking and conversion in every factory method, *except* in CSPRep.fromString.  We need to keep selfUri around (as a nsIURI) since the etld service requires a nsIURI.  Everything else can get by with a CSPSource, as far as I can tell so we should pass it around as a CSPSource and use it as that type everywhere else.  We still have to do the CSPSource.create(self) at the top of each factory method, since the tests are going to pass in strings and not always start from CSPRep.fromString(), but I don't think we need to keep any nsIURI versions of self around except the one mentioned above.

I'm clearing the review flags until for now, since I'd like you to change a few things and I'd like to see the next revision.

::: content/base/src/CSPUtils.jsm
@@ +87,5 @@
>    var textMessage = 'CSP WARN:  ' + aMsg + "\n";
>  
>    var consoleMsg = Components.classes["@mozilla.org/scripterror;1"]
>                      .createInstance(Components.interfaces.nsIScriptError);
> +  consoleMsg.init(textMessage, aSource, aScriptSample, aLineNum, 0,

good catch... not part of this bug, but a nice clean up.

@@ +213,5 @@
>    *
>    * @param aStr
>    *        string rep of a CSP
>    * @param self (optional)
> +  *        URI or CSPSource representing the "self" source

This doesn't follow the code.  It looks like in line 235 'self' is expected to be either a URI or a string (since if it's not a URI the code uses the io service to create a new one).  

I think expecting a string or URI makes sense since this is usually how the parser is initially invoked (CSPRep.fromString(entirePolicyString, URI or string).  The other bits of the parser may also see self as a CSPSource, but I don't think this one does.  If we want to be sure, we might want to allow all three (CSPSource, nsIURI, string) as types for all of the 'self' parameters in all of the factory methods.

@@ +274,5 @@
>      // SOURCE DIRECTIVES ////////////////////////////////////////////////
>      for each(var sdi in SD) {
>        if (dirname === sdi) {
>          // process dirs, and enforce that 'self' is defined.
> +        var dv = CSPSourceList.fromString(dirvalue, self, true);  

Please remove trailing spaces (everywhere).

@@ +604,5 @@
>   *
>   * @param aStr
>   *        string rep of a CSP Source List
>   * @param self (optional)
> + *        URI or CSPSource representing the "self" source

I think we also want to allow a string here, just in case.  (May require changes to the function body, see the 'self' type rant above).

::: content/base/src/contentSecurityPolicy.js
@@ +238,5 @@
>  
>      // If there is a policy-uri, fetch the policy, then re-call this function.
>      // (1) parse and create a CSPRep object
>      var newpolicy = CSPRep.fromString(aPolicy,
> +				      selfURI,

Please put a comment here that says something along the lines of: "we can pass the whole URI because when it's parsed as 'self', only the scheme host and port are kept." Just as a friendly reminder.

Comment 17

[Tanvi Vyas[:tanvi]]

Thanks Sid for all the advice!  I am revising the patch.  There is one thing that I didn't catch.  Can you explain what you meant by the following:

"We still have to do the CSPSource.create(self) at the top of each factory method, since the tests are going to pass in strings and not always start from CSPRep.fromString(),"

Are you referring to the mochitests?  Will they be calling the factory methods directly?  I will take a look at the CSP mochitests.

Thanks!

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas from comment #17)
> "We still have to do the CSPSource.create(self) at the top of each factory
> method, since the tests are going to pass in strings and not always start
> from CSPRep.fromString(),"
> 
> Are you referring to the mochitests?

Kind of, but mostly XPCShell tests.
  
> Will they be calling the factory methods directly?

No, but the XPCShell tests do. (see /content/base/test/unit/test_csputils.js) They test each bit of the parser unit-by-unit. to ensure proper operation of each self-parsing class.

Comment 19

[Tanvi Vyas[:tanvi]]

I made some changes and will attach a second update to the patch, although very rough and not ready to land.  But I just want to get some feedback before I proceed.

In summary, I'm not sure that I agree that we should allow strings in some of the factory methods, just so test cases pass.  Should we instead change the test cases to pass nsURIs (in most cases) or CSPSources instead of strings?  This would require more work, but I want to make sure we go about this the right way.  Another option (which you mentioned) is just allowing self to be string, URI, or CSPSource in all factory methods, just for consistency.  This would make things easier and cleaner, but isn't really necessary.

I've made some comments inline below.

Thanks!

(In reply to Sid Stamm [:geekboy] from comment #16)

Point 1:

> I think the pattern we should make sure the parser follows is that self'
> parameters always get converted in this direction: (string -> nsIURI ->
> CSPSource), and when passed from one bit of the parser to another, it should
> always be a CSPSource.
> 
> For consistency we can probably just use this for parsing the self parameter
> in all the factory methods:
> 
> if (!(self instanceof CSPSource)) {
>   self = CSPSource.create(self);
> }
> 
> ...and then let CSPSource.create do the type checking and conversion in
> every factory method, *except* in CSPRep.fromString.  We need to keep
> selfUri around (as a nsIURI) since the etld service requires a nsIURI. 
> Everything else can get by with a CSPSource, as far as I can tell so we
> should pass it around as a CSPSource and use it as that type everywhere
> else.  We still have to do the CSPSource.create(self) at the top of each
> factory method, since the tests are going to pass in strings and not always
> start from CSPRep.fromString(), but I don't think we need to keep any nsIURI
> versions of self around except the one mentioned above.
> 

Going through the functions, the only factory method this is a candidate for is CSPSourceList.fromString.(CSPRep.fromString is an exception as noted; CSPSource.create is the factory method we are calling, CSPSource.validSchemeName and CSPHost.fromString doesn't take a self parameter; CSPSource.fromURI and CSPSource.fromString are called from CSPSource.create)

I have added the call to CSPSource.create in CSPSourceList.fromString.


Point 2:

> @@ +213,5 @@
> >    *
> >    * @param aStr
> >    *        string rep of a CSP
> >    * @param self (optional)
> > +  *        URI or CSPSource representing the "self" source
> 
> This doesn't follow the code.  It looks like in line 235 'self' is expected
> to be either a URI or a string (since if it's not a URI the code uses the io
> service to create a new one).  
> 
> I think expecting a string or URI makes sense since this is usually how the
> parser is initially invoked (CSPRep.fromString(entirePolicyString, URI or
> string).  The other bits of the parser may also see self as a CSPSource, but
> I don't think this one does.  If we want to be sure, we might want to allow
> all three (CSPSource, nsIURI, string) as types for all of the 'self'
> parameters in all of the factory methods.
> 

Once we apply the main one line change to contentSecurityPolicy.js (line 242), CSPRep.fromString is called only once in the actual code with the second parameter set to a URI (http://mxr.mozilla.org/mozilla-central/search?string=CSPRep.fromString&case=on&find=&findi=&filter=[Cc]SPRep.fromString&hitlimit=&tree=mozilla-central).  I don't see strings or CSPSources, except for the strings from the test cases.  Hence, maybe we need to remove the else if case on lines 234-234 of CSPUtils.jsm (which will break the test cases).  Unless we want to allow all three types for all factory methods, as you've mentioned, for consistency.

Since in content/base/test/unit/csputils.js and test_bug558431.js, we do use strings in the second parameter of CSPRep.fromString, should we change the test cases, instead of changing the code to match the test cases.  What are your thoughts?


Point 3:

> @@ +604,5 @@
> >   *
> >   * @param aStr
> >   *        string rep of a CSP Source List
> >   * @param self (optional)
> > + *        URI or CSPSource representing the "self" source
> 
> I think we also want to allow a string here, just in case.  (May require
> changes to the function body, see the 'self' type rant above).
> 

CSPSourceList.fromString is called with a self parameter 3 times (http://mxr.mozilla.org/mozilla-central/search?string=CSPSourceList.fromString&find=&findi=&filter=&hitlimit=&tree=mozilla-central).  In the actual code, it is only called twice where both instances of self are taken from CSPSource.fromString which can only have URI self (as mentioned above).  Hence, CSPSourceList.fromString should also only be a URI.  However, in the test cases it is called with a string as the self parameter.  Again, here we ask whether we should change the code or the test cases.


Point 4:

> ::: content/base/src/contentSecurityPolicy.js
> @@ +238,5 @@
> >  
> >      // If there is a policy-uri, fetch the policy, then re-call this function.
> >      // (1) parse and create a CSPRep object
> >      var newpolicy = CSPRep.fromString(aPolicy,
> > +				      selfURI,
> 
> Please put a comment here that says something along the lines of: "we can
> pass the whole URI because when it's parsed as 'self', only the scheme host
> and port are kept." Just as a friendly reminder.

I haven't done this one yet ;)

Comment 20

[Tanvi Vyas[:tanvi]]

Attachment: Second Attempt at Patch

Comment 21

[Tanvi Vyas[:tanvi]]

Comment on attachment 599850 [details] [diff] [review]
Second Attempt at Patch

Fixing the feedback flag.

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Can you flag the attachment as a patch?  Splinter review isn't available for the attachment.  :-/

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Tanvi, you mentioned I should hold off on feedback for the patch since we talked last week.  Is that still the case?  (If so, please clear the flag).

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 539346 [details] [diff] [review]
first cut at a patch

Clearing feedback flag.  There is one additional change to the code (removing unnecessary code that handles strings in factory methods that no longer accept strings as input).  And changes to the test cases (converting strings to uri's before passing them into the factory methods).

Comment 25

[Tanvi Vyas[:tanvi]]

Completed the patch and will flag for feedback.

> Point 2:
> 
> Once we apply the main one line change to contentSecurityPolicy.js (line
> 242), CSPRep.fromString is called only once in the actual code with the
> second parameter set to a URI
> (http://mxr.mozilla.org/mozilla-central/search?string=CSPRep.
> fromString&case=on&find=&findi=&filter=[Cc]SPRep.
> fromString&hitlimit=&tree=mozilla-central).  I don't see strings or
> CSPSources, except for the strings from the test cases.  Hence, maybe we
> need to remove the else if case on lines 234-234 of CSPUtils.jsm (which will
> break the test cases).  

Removed lines 234-235 in CSPutils.jsm

 
> Since in content/base/test/unit/csputils.js and test_bug558431.js, we do use
> strings in the second parameter of CSPRep.fromString, should we change the
> test cases, instead of changing the code to match the test cases.  What are
> your thoughts?

Talked to Sid and agreed we should change the test cases to convert the strings in CSPRep.fromString to URIs before calling the factory method.  Second parameter to CSPRep.fromString should always be a URI.

Fixed test cases content/base/test/unit/test_csputils and content/base/test/unit/test_bug558431.js to convert string to URIs before calling CSPRep.fromString.  Note that since test_bug558431.js set DOCUMENT_URI to localhost, I had to switch this to 127.0.0.1 in order to create a nsIURI.
 
> Point 3:
> 
> > @@ +604,5 @@
> > >   *
> > >   * @param aStr
> > >   *        string rep of a CSP Source List
> > >   * @param self (optional)
> > > + *        URI or CSPSource representing the "self" source
> > 
> > I think we also want to allow a string here, just in case.  (May require
> > changes to the function body, see the 'self' type rant above).
> > 
> 
> CSPSourceList.fromString is called with a self parameter 3 times
> (http://mxr.mozilla.org/mozilla-central/search?string=CSPSourceList.
> fromString&find=&findi=&filter=&hitlimit=&tree=mozilla-central).  In the
> actual code, it is only called twice where both instances of self are taken
> from CSPSource.fromString which can only have URI self (as mentioned above).
> Hence, CSPSourceList.fromString should also only be a URI.  However, in the
> test cases it is called with a string as the self parameter.  Again, here we
> ask whether we should change the code or the test cases.

Changed the comment to allow only URI's in CSPSourceList.fromString.  And changed the test case content/base/test/unit/test_csputils  to convert the string to URI before calling CSPSourceList.fromString()


> Point 4:
> 
> > ::: content/base/src/contentSecurityPolicy.js
> > @@ +238,5 @@
> > >  
> > >      // If there is a policy-uri, fetch the policy, then re-call this function.
> > >      // (1) parse and create a CSPRep object
> > >      var newpolicy = CSPRep.fromString(aPolicy,
> > > +				      selfURI,
> > 
> > Please put a comment here that says something along the lines of: "we can
> > pass the whole URI because when it's parsed as 'self', only the scheme host
> > and port are kept." Just as a friendly reminder.
> 

Added a comment

Comment 26

[Tanvi Vyas[:tanvi]]

Haven't posted the patch yet, because I found a bug with my changes in test case test_bug558431.js.  Stay tuned :)

Comment 27

[Tanvi Vyas[:tanvi]]

Turns out my changes caused test_bug558431.js to fail, and I've been investigating why.  In the process, I have (so far) found 2 potential bugs in CSP.

Issue 1:
In CSPUtils.jsm, line 1088.  When parsing a url, CSPSource.fromString(aStr, self, enforceSelfChecks) does not account for a port followed by a path when parsing self.  Where self is:    
 * @param self (optional)
 *        string, URI, or CSPSource representing the "self" source

.
1089   if (!(chunks[2] === "*" || chunks[2].match(/^\d+$/))) {
1090     dump("Thinks it coudlnt' parse the port.  port is: "+chunks[2]+".\n\n");
1091     CSPError("Couldn't parse port in " + aStr);
1092     return null;
1093   }

In this case, the string is http://localhost:9000/document


The flow for test_bug558431.js is as follows- 
Call CSPRep.fromString, and goes into Directive loop.
First directive is allow directive.  Allow directive calls CSPSourceList.fromString (which shouldn't take a string but only a uri as its second parameter), which calls CSPSource.create(self).  CSPSource.create calls CSPSource.fromString(self).  CSPSource.fromString then parses self (aStr).  There is a comment in the code explaining why its not transformed to an nsURI here:

  // We could just create a URI and then send this off to fromURI, but
  // there's no way to leave out the scheme or wildcard the port in an nsURI.
  // That has to be supported here.

It goes on to parse the scheme, host and port.  And returns the CSPSource containing the scheme/host/port to CSPSource.fromString, which returns to CSPSource.create, which returns to CSPSourceList.fromString.  The CSPSource is added to a CSPSourceList and returned to CSPRep.fromString (back to the code for the allow directive).  Then the CSPSourceList is added to the CSPRep.

Does the CSPRep need the path?  Is there a path attribute in CSPSource so that we can set the path when parsing the string uri in CSPSource.fromString?  There is no path attribute in CSPSource, so do we ignore the path?

To solve this problem for now, I am removing the $ after \d+ in the regex for ports.  Since I believe there are other issues in the CSPUtils.jsm code that I want to debug (without the distraction of this one).



Issue 2:
Policy directive
CSP ERROR:  The policy-uri cannot be fetched without a parent request and a CSP.

CSPRep.fromString
 aStr: default-src 'none'.  self: undefined.  docRequest: undefined.  csp: undefined.

In test_bug558431.js we call  cspr = CSPRep.fromString("policy-uri " + POLICY_URI, DOCUMENT_URI, docChan, csp) and assume that docChan and csp are defined.  If they are not defined, we end up switching the policy to "default-src 'none'" and hence never use the POLICY_URL.

I added a check to see if csp and docChan existed.  docChan exists, but csp does not.  For some reason, this must be failing:
  var csp = Components.classes["@mozilla.org/contentsecuritypolicy;1"]
    .createInstance[Components.interfaces.nsIContentSecurityPolicy];

How do I create an instance of csp so that this test can perform properly.  I'm not really sure why the test is currently passing (without my code changes).  Still working on that.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Um... Why is CSP doing manual parsing of URI strings?  If it's trying to implement a specific algorithm from the spec, then it should be doing that (and I really doubt that the spec is defined in terms of regexps).  If it just wants generic URI parsing and canonicalization, shouldn't it just be using our existing facilities for that?

Comment 29

[Tanvi Vyas[:tanvi]]

> Issue 1:
...
> 
> To solve this problem for now, I am removing the $ after \d+ in the regex
> for ports.  Since I believe there are other issues in the CSPUtils.jsm code
> that I want to debug (without the distraction of this one).
> 
This of course breaks other things, so I'm going to have to try to properly patch this.

Comment 30

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky (:bz) from comment #28)
> Um... Why is CSP doing manual parsing of URI strings?  If it's trying to
> implement a specific algorithm from the spec, then it should be doing that
> (and I really doubt that the spec is defined in terms of regexps).  If it
> just wants generic URI parsing and canonicalization, shouldn't it just be
> using our existing facilities for that?

Yes, I'm not sure why it's doing manual parsing.  It starts around here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#956.

If someone can make sense of the comment line in 956 or provide some context on it, that would be great :)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Sounds like the string here is allowed to have non-URI syntax like http://example.com:*/ or whatnot, based on that comment....  That's pretty unfortunate.  Does the spec actually define the parsing here?

Comment 32

[Sid Stamm [:geekboy or :sstamm]]

Yes, the reason CSP is not using nsIURI there is because CSP sources are not URIs.  A source expression is not required to have a scheme or a port, and the host is optional as well... this makes it a little funny.  Some use cases are:

1. I want to allow all HTTPS traffic, regardless of the port or host
2. I want to allow all traffic on port 888, regardless of the scheme or host
3. I want to allow all traffic from host foo.com, regardless of the scheme or port

When this code was written, the spec was so in flux that all that had been laid out was the syntax, not the parsing model, and I believe this is the third incarnation of this source-parsing code.  It could probably use another once-over, but I was waiting for the W3C to recommend a spec first.

Anyway, the CSP draft is still in flux, but more stable and a current revision of a parsing sequence is here:
http://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#source-list

So we should re-write the parser, but I think we should just fix the bug in the parsing code now if we  can, and then when the W3C publishes a spec, adopt the parsing steps written in that.

Comment 33

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas from comment #27)
> Does the CSPRep need the path?  Is there a path attribute in CSPSource so
> that we can set the path when parsing the string uri in
> CSPSource.fromString?  There is no path attribute in CSPSource, so do we
> ignore the path?

Path doesn't make sense for a CSPSource; a source is basically an origin (scheme:host:port) with allowed wildcards.  The old (deprecated) spec gives some examples:
https://wiki.mozilla.org/Security/CSP/Specification#Source_Expression_List

> To solve this problem for now, I am removing the $ after \d+ in the regex
> for ports.  Since I believe there are other issues in the CSPUtils.jsm code
> that I want to debug (without the distraction of this one).

This is dangerous because the regex is used for detection of an attribute.

What you're encountering here is a call to CSPSource.fromString() with a string that is not a valid CSPSource.  Either the parser should be updated to reject that input or the caller of that factory method should pass in something that's a valid CSPSource.

In the case of this specific test, since DOCUMENT_URI is not really a URI but a string rep of a URI, we should make it a URI.  I'm willing to bet it'll work just fine if we turn the string that is currently DOCUMENT_URI into a nsIURI.

Can we augment CSPSource.fromString() to detect the path and complain via CSPError that it's not a valid source?  I think we can just reject anything with a slash in it (if the slashes are not between scheme and host) since a slash is not valid for the scheme name and also not valid for the host name.

> Issue 2:
> Policy directive
> CSP ERROR:  The policy-uri cannot be fetched without a parent request and a
> CSP.

I think probably this is happening because of the issue above (failure to parse self).  Try modifying the test to use an nsIURI instead of string for DOCUMENT_URI and POLICY_URI, and see if that helps.

But in case it's not...

> I added a check to see if csp and docChan existed.  docChan exists, but csp
> does not.  For some reason, this must be failing:
>   var csp = Components.classes["@mozilla.org/contentsecuritypolicy;1"]
>     .createInstance[Components.interfaces.nsIContentSecurityPolicy];
> 
> How do I create an instance of csp so that this test can perform properly. 
> I'm not really sure why the test is currently passing (without my code
> changes).  Still working on that.

Here's where nsDocument.cpp does it:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2439

The two lines of code you write above should create an instance of a CSP, then usually you have to call scanRequestData() to grab the channel info and then refinePolicy() to set up the document's policy.  I think the test is written correctly.  I'm not sure why it's failing.  What do you get back from .createInstance()?  is it null?

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 599850 [details] [diff] [review]
Second Attempt at Patch

removing feedback? flag at Tanvi's request.

Comment 35

[Sid Stamm [:geekboy or :sstamm]]

I filed Bug 737064 to address parsing of the source expressions.  While the spec doesn't directly define the parsing, we can at least do proper matching and throw errors when the string to be parsed is invalid.  That'll make it easier to detect the "extra stuff in the string" problem encountered by Tanvi.

Comment 36

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fixing test_bug558431.js

Square brackets.

test_bug558431.js was totally hosed.  There were square brackets used where parentheses should have been (after createInstance), so  I don't know why it was passing before.  I had my hand at re-spinning it.  Attached patch:

1. inserts a space between "options" directive tokens in "toString()" policy reconstruction (unrelated, but trivial ride-along).
2. rewrote test_bug558431.js to run tests in serial
3. .. and use a separate load listener for each test
4. .. and de-globalize the csp objects being passed around
5. .. and serialize/reparse each policy to remove any 'self' tokens (for proper comparison)
6. .. and use parentheses with .createInstance instead of square brackets

Tanvi, take a look and see if you can use this stuff with the rest of your patch, or just apply it somewhere in your patch queue.  I did *not* fold in any of the work you did so far that is in attachment 599850 [details] [diff] [review], so we shouldn't collide if you revert all your changes to test_bug558431.js.

Comment 37

[Tanvi Vyas[:tanvi]]

Attachment: Third Patch

Attaching my patch. I applied Sid's patch to the busted test_bug558431.js on top of the my patch, and all csp tests now pass!

I will fold these patches together, attach one consolidated patch, and push to try.  Not setting any feedback flags until then.

Comment 38

[Tanvi Vyas[:tanvi]]

Attachment: Updated Patch with Fix to test_bug558431 folded in

Folded Sid's patch to test_bug558431.js into mine.  CSP tests passed, and I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ef56f326d23b.

Adding feedback flag for Sid.

Comment 39

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 611181 [details] [diff] [review]
Updated Patch with Fix to test_bug558431 folded in

Review of attachment 611181 [details] [diff] [review]:
-----------------------------------------------------------------

Looks Good!

::: content/base/src/CSPUtils.jsm
@@ +602,5 @@
>   *
>   * @param aStr
>   *        string rep of a CSP Source List
>   * @param self (optional)
> + *        URI representing the "self" source

What if self is already a CSPSource?  We should allow that here since if we don't, code that wants to use it in this way will turn the self CSPSource into a URI before passing it in.  You can support it like CSPSource.create() does.  Comment 19 suggests you don't want to do this, but I think it wouldn't hurt to support one more type (quick if block).

@@ +623,5 @@
> +  if(self) {
> +     self = CSPSource.create(self);
> +  }
> +
> +

nit: extra blank line here.

::: content/base/test/unit/test_csputils.js
@@ -4,5 @@
>   * The contents of this file are subject to the Mozilla Public License Version
>   * 1.1 (the "License"); you may not use this file except in compliance with
>   * the License. You may obtain a copy of the License at
> - * http://www.mozilla.org/MPL/
> - *

Please don't remove these two lines from the license block.

@@ +46,5 @@
>  const POLICY_URI_RELATIVE = "/policy";
>  
> +//converts string to nsIURI
> +function URI(uriString) {
> +  var iOService = Components.classes["@mozilla.org/network/io-service;1"]

Optional Nit: iOService is an awkward capitalization.  Consider using a variable like "ios" or ioService.

@@ +49,5 @@
> +function URI(uriString) {
> +  var iOService = Components.classes["@mozilla.org/network/io-service;1"]
> +                .getService(Components.interfaces.nsIIOService);
> +  var uri = iOService.newURI(uriString, null, null);
> +  return uri;

Optional: you can combine these two lines into:
 return iOService.newURI(uriString, null, null);

Comment 40

[Tanvi Vyas[:tanvi]]

Thanks Sid for the feedback.  I've made all the changes you've requested (with a note on one below).  I will submit a new patch to try and to this bug tomorrow and request review from jst and/or dveditz.

> ::: content/base/src/CSPUtils.jsm
> @@ +602,5 @@
> >   *
> >   * @param aStr
> >   *        string rep of a CSP Source List
> >   * @param self (optional)
> > + *        URI representing the "self" source
> 
> What if self is already a CSPSource?  We should allow that here since if we
> don't, code that wants to use it in this way will turn the self CSPSource
> into a URI before passing it in.  You can support it like CSPSource.create()
> does.  Comment 19 suggests you don't want to do this, but I think it
> wouldn't hurt to support one more type (quick if block).
> 


I understand what you are saying here; even though we don't call CSPSourceList.fromString with a CSPSource in the second parameter today, we might want to tomorrow.  And we don't want a developer to convert CSPSource->nsIURI in order to call CSPSourceList.fromString (which will then convert it back to a CSPSource).  I've made the following change:


  /* If self parameter is passed, convert to CSPSource
     unless it is already a CSPSource. */
  if(self && !(self instanceof CSPSource)) {
     self = CSPSource.create(self);
  }

Comment 41

[Tanvi Vyas[:tanvi]]

Attachment: Patch with Fix to test_bug558431

Final patch (hopefully) to this bug.  Incorporates all Sid's feedback.  Passes all CSP tests.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d625872de2b3

Marking this patch for Review by jst or Dan Veditz.  If possible, I'd like to land this somewhat before 4-24 so it can make it into Firefox 14.

Thanks!

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 613697 [details] [diff] [review]
Patch with Fix to test_bug558431

Review of attachment 613697 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

::: content/base/src/CSPUtils.jsm
@@ +455,1 @@
>                             + (this._allowInlineScripts ? "inline-script" : ""));

Another way would be to move the space from the end of "options " to the start of each item if present -- then you don't need the added line and extra test.

Comment 43

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Daniel Veditz [:dveditz] from comment #42)
> Another way would be to move the space from the end of "options " to the
> start of each item if present -- then you don't need the added line and
> extra test.

Yeah!  I fully take the blame for that crap code, Dan, thanks for the optimization.  Tanvi, maybe you can work that in and make sure the CSP tests still pass before we land it?

Comment 44

[Tanvi Vyas[:tanvi]]

Attachment: Final Patch

I made the optimization that dveditz suggested in comment 42.  This is the diff:

-      dirs.push("options " + (this._allowEval ? "eval-script" : "")
-                           + (this._allowInlineScripts ? "inline-script" : ""));
+      dirs.push("options" + (this._allowEval ? " eval-script" : "")
+                           + (this._allowInlineScripts ? " inline-script" : ""));

The updated patch is attached.  All CSP tests pass and I've pushed to try: 
https://tbpl.mozilla.org/?tree=Try&rev=0254e37d0476

Since we got an r+ from dveditz, this is ready to land.  Sid, can you land this, or should I see if someone else with level 3 can before FF14 goes to aurora.

Comment 45

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 615618 [details] [diff] [review]
Final Patch

carrying over r=dveditz

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 615618 [details] [diff] [review]
Final Patch

content/base/src/contentSecurityPolicy.js
>@@ -234,18 +234,20 @@ ContentSecurityPolicy.prototype = {
>       selfURI = selfURI.innermostURI;
>     }
> 
>     // stay uninitialized until policy merging is done
>     this._isInitialized = false;
> 
>     // If there is a policy-uri, fetch the policy, then re-call this function.
>     // (1) parse and create a CSPRep object
>+    // Note that we pass the full URI since when it's parsed as 'self' to construct a 
>+    // CSPSource only the scheme, host, and port are kept. 
>     var newpolicy = CSPRep.fromString(aPolicy,
>-                                      selfURI.scheme + "://" + selfURI.hostPort,
>+				      selfURI,

Fix the indentation here. There's tab characters there, which are generally not accepted for indentation, replace those with spaces.

r=jst with that.

Comment 47

[Sid Stamm [:geekboy or :sstamm]]

Argh... bugzilla didn't take my comment submission earlier, probably due to the weak network I found myself using.

Landed before the approval-needed cut-off:
http://hg.mozilla.org/integration/mozilla-inbound/rev/67818a1a19c3

jst: we'll have to fix those tabs and land it with the next CSP follow-up.

Comment 48

[Tanvi Vyas[:tanvi]]

Thanks for pointing out the tabs jst.  I will fix this with a new patch (since mine has already landed on inbound), attach it here, and see if we can land and get approval for FF14.

Alternatively, I heard rumors about a simpler process for white space changes.  If anyone has info on that, that would be great!

Comment 49

[Tanvi Vyas[:tanvi]]

Attachment: One line whitespace change

Second patch to land to fix tabs to spaces.  Alternatively, we could add this to another csp bug that lands in the future.

Comment 50

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/67818a1a19c3

Comment 51

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas from comment #49)
> Second patch to land to fix tabs to spaces.  Alternatively, we could add
> this to another csp bug that lands in the future.

Lets land this one liner with a future bug.