746978 - sync CSP directive parsing and directive names with w3c CSP 1.0 spec

Status: RESOLVED FIXED

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

Description

[Wladimir Palant]

Somebody edited the MDN page on CSP to match the current version of the spec (https://developer.mozilla.org/index.php?title=en/Security/CSP/CSP_policy_directives&action=diff&revision=22&diff=23). However, as far as I can see only default-src has been actually implemented which makes the page highly confusing. Firefox expects "options inline-script eval-script" instead of "script-src 'unsafe-inline' 'unsafe-eval'" which it should be per spec. Also, Firefox expects "xhr-src" instead of "connect-src". While I could revert the change to the documentation, I think that the better solution would be supporting the new syntax.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

devdatta: I think this was you who made the edits.  If it was, is there a reason you modified the MDN page to conflict with the currently-shipping CSP implementation?

Comment 2

[Devdatta Akhawe [:devd]]

I could have sworn default-src worked on Firefox. I was working with multiple browsers, and I think I made a mistake. Should I just go ahead and revert?

Chrome only works with default-src. Given that MDN doesn't want to be Mozilla specific documentation, we should still talk about default-src somewhere.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

dev: default-source works, but the other changes you made don't.  Can you roll back the other changes you made to the MDN page?

Wladimir: what you are asking for is bug 737064.  I'll update this bug to reflect that MDN is out of sync with our shipping product.

In the interest of changing our implementation syntax as infrequently as possible, I'd like to wait until the W3C spec is more stable -- it's very close.  When it's time to change our implementation, we should update the code and then follow that with changes to MDN, not the other way around, because of the need for a little time between landing the code and when the change is released.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy] from comment #3)
> Wladimir: what you are asking for is bug 737064.  I'll update this bug to
> reflect that MDN is out of sync with our shipping product.

Actually, disregard that.  That bug is just focused on CSP Sources.  We should sync up directive names too.  Making this bug that.

Comment 5

[Devdatta Akhawe [:devd]]

Aah.. I think I saw default-src works and concluded that Mozilla had implemented the spec. My apologies.

Comment 6

[Devdatta Akhawe [:devd]]

reverted

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Thanks, dev. The action left for this bug is now to update the CSP directives and parser to reflect the W3C spec.

Comment 8

[Wladimir Palant]

(In reply to Sid Stamm [:geekboy] from comment #3)
Yes, I looked at that bug but it does seem to complain merely about the BNF not followed exactly.

I guessed that you might be waiting for the spec to become more stable. Still, I found no bug on changing inline-script and xhr-src so with the MDN documentation being confusing I decided to file one.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Wladimir Palant from comment #8)
> (In reply to Sid Stamm [:geekboy] from comment #3)
> Yes, I looked at that bug but it does seem to complain merely about the BNF
> not followed exactly.

You're right... that was my bad (I remembered the bug wrong).

> Still, I found no bug on changing inline-script and xhr-src so with the MDN
> documentation being confusing I decided to file one.

Yes, thank you for doing this!

Comment 10

[Mikko Rantalainen]

The page https://developer.mozilla.org/en/Security/CSP/CSP_policy_directives does not match the reality with the currently shipping browser (Firefox 13.0), again. Is this by design? I had to check the source code to figure out the currently shipping behavior!

Comment 11

[Devdatta Akhawe [:devd]]

@mikko: What was the error? Can you go ahead and fix the wiki page?

Unfortunately, the CSP specification had been in a flux for a while (ergo the confusion), but it seems it has finally been nailed down. We have someone working on making the Firefox implementation follow the spec, and it should be live.

Comment 12

[Mikko Rantalainen]

(In reply to Devdatta Akhawe from comment #11)
> @mikko: What was the error? Can you go ahead and fix the wiki page?

Haven't yet figured out how to fix the wiki myself because I don't have BrowserID or MDN profile.

The error in the page is as follows:

The MDN page describes the current state of the W3C CSP specification as far as I can see. However, the currently shipping behavior is to still use the 'options' parameter that is not specified by W3C at all.

W3C style:            Content-Security-Policy: script-src 'unsafe-eval';
Firefox 13 style:     Content-Security-Policy: options eval-script;

In short, if I have understood the situation correctly, Fx13 does not support 'unsafe-eval' or 'unsafe-inline' for 'script-src' (maybe not for default-src either). Instead it requires use of proprietary 'options' with proprietary values and quoting of those values is different from standard (that is, nothing instead of single quotes). The MDN documentation should reflect this instead of blidnly describing the W3C variant. IMHO, MDN documentation should be strictly about shipping behavior, for standard stuff it can just link to W3C site.

Comment 13

[Ian Melven :imelven]

A note that we plan to still support policy-uri and frame-ancestors directives when specified in a CSP 1.0 compliant Content-Security-Policy header.

Comment 14

[Ian Melven :imelven]

Attachment: Part 1: WIP - beginnings of 1.0 spec compliant support

Very much a Work In Progress.

There are quite a few TODOs in this patch and maybe an open question or two. 

This fails the xhr_bad test in content/base/test/test_CSP.html which I am working on fixing. 

It also fails the xhr-src-redir test in content/base/test/test_csp_redirects.html

It passes the CSP xpcshell tests and the other CSP mochitests. 

There's a bunch of whitespace and debugging code in this patch which will be cleaned up later.

This patch depends on having the patches in bug 783049 applied.

We also are going to need tests that use the 1.0 spec compliant header and need to add a test for the connect-src directive that EventSource is correctly blocked/allowed.

Comment 15

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #14)
> Created attachment 657489 [details] [diff] [review]
> Part 1: WIP - beginnings of 1.0 spec compliant support
>
> This fails the xhr_bad test in content/base/test/test_CSP.html which I am
> working on fixing. 
> 
> It also fails the xhr-src-redir test in
> content/base/test/test_csp_redirects.html

I need to handle xhr-src in makeExplicit and intersectWith.

Comment 16

[Ian Melven :imelven]

Attachment: Part 1 v2: WIP - CSP 1.0 spec compliant support

(In reply to Ian Melven :imelven from comment #15)

> > This fails the xhr_bad test in content/base/test/test_CSP.html which I am
> > working on fixing. 
> > 
> > It also fails the xhr-src-redir test in
> > content/base/test/test_csp_redirects.html

This fixed both of these tests
 
> I need to handle xhr-src in makeExplicit and intersectWith.

by doing this

There's still open issues, but I cleaned up the debugging code and whitespace, this now passes all CSP tests. Next up: adding tests for the 1.0 spec compliant header/parsing.

Comment 17

[Ian Melven :imelven]

Note to self : one way to handle the issue of intersectWith being called to intersect a non compliant and compliant CSP is to allow this for when   
ContentSecurityPolicy's ._isInitialized = false, which is what happens when the CSP is first being set up and the value from the header is being intersected with the initial policy from the constructor and to prohibit it otherwise - and then see if that breaks any of the tests..

Comment 19

[Ian Melven :imelven]

This patch doesn't address using unsafe-inline yet, see bug 763879 comment c16

Comment 20

[Ian Melven :imelven]

Comment on attachment 657511 [details] [diff] [review]
Part 1 v2: WIP - CSP 1.0 spec compliant support

Oops.

Comment 21

[Ian Melven :imelven]

We should document whatever deprecation plan for the old header we end up following.

Comment 22

[Ian Melven :imelven]

Attachment: Part 1 v3: WIP - CSP 1.0 spec compliant support

Fix some bugs I found while working on the tests.

Comment 23

[Ian Melven :imelven]

Attachment: Part 2: WIP - tests

This adds tests for the spec compliant header to test_CSP.html

Next up I'll do test_CSP_frameancestors.html and the xpcshell tests for the spec compliant header/functions.

Then after that, we attack unsafe-inline for styles and scripts and write tests for it.

Comment 24

[Ian Melven :imelven]

$ hg qseries
bug783049-1
bug783049-2
bug783049-3
bug746978-1
bug746978-2
csp1.0xpcshelltests
bug763879-csp-styles
bug763879-csp-styles-2
bug746978-3
bug746978-4
bug746978-5
try

https://tbpl.mozilla.org/?tree=Try&rev=e8adf59cd2ea

Comment 25

[Ian Melven :imelven]

Attachment: Part 1 v4: WIP - CSP 1.0 spec compliant support

Comment 26

[Ian Melven :imelven]

Attachment: Part 2 v2: WIP - main and frameancestors tests

Comment 27

[Ian Melven :imelven]

Attachment: Part 3 v1 - xpcshell tests for new parser

Thank you to Sid for writing these !

Sid, who should we ask to review these ? I took a quick look at them and they made sense to me..

Comment 28

[Ian Melven :imelven]

Attachment: Part 4v1 - unsafe-inline support and its tests and updated redirect tests

Comment 29

[Ian Melven :imelven]

Attachment: part 5 v1 - unsafe-eval support and tests

Comment 30

[Ian Melven :imelven]

Attachment: part 6 v1 - fix up toString

Comment 31

[Ian Melven :imelven]

Attachment: tweaks to patch in bug 763879 for inline styles

I'm just going to leave this here for now. It's some tweaks to Sid's patch for bug 763879 to support inline styles. Going to add bug 763879 as blocking this bug as well.

Comment 32

[Erlend Oftedal]

I just updated the CS testing page. YOu can now have it just use the unprefixed header:
http://csptesting.herokuapp.com/?disable_old_headers=true

Comment 33

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661404 [details] [diff] [review]
Part 1 v4: WIP - CSP 1.0 spec compliant support

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

::: content/base/src/CSPUtils.jsm
@@ +170,5 @@
>    function(request, context, status) {
>      if (Components.isSuccessCode(status)) {
>        // send the policy we received back to the parent document's CSP
>        // for parsing
> +      this._csp.refinePolicy(this._policy, this._docURI, this._csp._specCompliant);

Since the constructor learns whether this policy is spec compliant or not, can we just use the value inside the CSPRep instead of passing another argument to refinePolicy?

@@ -209,5 @@
>    OBJECT_SRC:       "object-src",
>    FRAME_SRC:        "frame-src",
>    FRAME_ANCESTORS:  "frame-ancestors",
>    FONT_SRC:         "font-src",
> -  XHR_SRC:          "xhr-src"

Why did this go away?  Does SRC_DIRECTIVES only represent the 1.0 compliant directives now?

If that's the case, should we have two of these structures, one for the old policies and one for the new one and then have the same parsing routine that just switches between the two lists of directives?  The current approach is fine, I'm just thinking this might be a little less code and it would be easier to find and fix bugs that affect both old and new policies.

@@ +730,5 @@
> +    if (this._specCompliant && CSPRep.CONNECT_SRC_DIRECTIVE === aContext) {
> +      return this._directives[CSPRep.CONNECT_SRC_DIRECTIVE].permits(aURI);
> +    } else if (!this._specCompliant && CSPRep.XHR_SRC_DIRECTIVE === aContext) {
> +      return this._directives[CSPRep.XHR_SRC_DIRECTIVE].permits(aURI);
> +    }

if we had two lists of SRC_DIRECTIVES, then this wouldn't be necessary... instead we could just have:

let DIRS = this._specCompliant ? CSPRep.SRC_DIR_NEW : CSPRep.SRC_DIR_OLD;
for (var i in DIRS) {
  if (DIRS[i] === aContext) {
     return this._directives[aContext].permits(aURI);
  }
}

Instead of the loop (above) and the if block you added here.

@@ +764,5 @@
> +    if (aCSPRep._specCompliant) {
> +      var dirv = CSPRep.CONNECT_SRC_DIRECTIVE;
> +      if (this._directives.hasOwnProperty(dirv)) {
> +        newRep._directives[dirv] = this._directives[dirv].intersectWith(aCSPRep._directives[dirv]);
> +      } 

Nit: trailing whitespace.

@@ +774,5 @@
> +      if (this._directives.hasOwnProperty(dirv))
> +        newRep._directives[dirv] = this._directives[dirv].intersectWith(aCSPRep._directives[dirv]);
> +      else
> +        newRep._directives[dirv] = aCSPRep._directives[dirv];
> +    }

same as previous comment: we could special-case this if there were two lists of SRC_DIRECTIVEs.  There are probably more places that this will simplify the multiplexing code too.

@@ +812,5 @@
>      var SD = CSPRep.SRC_DIRECTIVES;
>      var defaultSrcDir = this._directives[SD.DEFAULT_SRC];
>      if (!defaultSrcDir) {
> +      // TODO : this string will be wrong for a spec compliant policy
> +      // based on its id (1.0 spec has no 'allow' directive'

Ah.  Maybe add a non-localized warning here?
CSPWarning("foo");

::: content/base/src/contentSecurityPolicy.js
@@ +272,5 @@
>      this._policy = intersect;
> +
> +    if (aSpecCompliant) {
> +      this._policy._specCompliant = true;
> +    }

Could just do this._policy._specCompliant = !!aSpecCompliant (right?)

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661407 [details] [diff] [review]
Part 3 v1 - xpcshell tests for new parser

jst: would you take a look at these tests?  I wrote them, so can't r+ them, and I think you're pretty familiar with CSP.  imelven can help if you have questions.  :)

Comment 35

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661405 [details] [diff] [review]
Part 2 v2: WIP - main and frameancestors tests

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

Looks fine, these are a duplicate of the existing tests.  We could streamline these a bit by threading a "1.0 compliant flag" throughout the tests and duplicating the tests in each file, but in principle this should work too.

::: content/base/test/file_CSP_main_spec_compliant.html
@@ +9,5 @@
> +    <style>
> +      /* CSS font embedding tests */
> +      @font-face {
> +        font-family: "arbitrary_good";
> +        src: url('file_CSP.sjs?testid=font_spec_compliant_good&type=application/octet-stream');

Just making sure, but file_CSP.sjs is just serving the content here, and doesn't do anything with the testid, right?  (The test main .html file reads that.)

Comment 36

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661408 [details] [diff] [review]
Part 4v1 - unsafe-inline support and its tests and updated redirect tests

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

::: content/base/src/CSPUtils.jsm
@@ +904,5 @@
>  
>    // Set to true when this list is created using "makeExplicit()"
>    // It's useful to know this when reporting the directive that was violated.
>    this._isImplicit = false;
> +  

nit: trailing whitespace.

@@ +925,5 @@
>   */
>  CSPSourceList.fromString = function(aStr, self, enforceSelfChecks) {
>    // source-list = *WSP [ source-expression *( 1*WSP source-expression ) *WSP ]
>    //             / *WSP "'none'" *WSP
> +  

nit: more trailing whitespace

@@ +956,5 @@
>      }
> +    // if a source allows unsafe-inline, set our flag to indicate this.
> +    if (src._allowUnsafeInline)
> +      slObj._allowUnsafeInline = true;
> +    

nit: here too, please check the file for whitespace.

What if we catch the token 'unsafe-inline' here, instead of passing it through to CSPSource?  Should be a quick oneliner and would remove some of the logic from the source parsing.

In fact, the CSPSourceList probably doesn't need to know anything about whether 'unsafe-inline' means eval or inline.  It can just parse out 'unsafe-inline' and then in CSPRep, after we parse the source list, we can ask if it contained unsafeInline; if it did, based on the directive name we can choose what that means (e.g., set "allows inline script" or "allows eval").

I can see how parsing this deep in a source makes sense for when if we call toString() on the sourcelist... but either way is probably fine.

@@ +1362,5 @@
> +    return sObj;
> +  }
> +
> +  // TODO : unsafe-eval
> +  

Yeah, this could probably be uplifted into the source list.

::: content/base/src/contentSecurityPolicy.js
@@ +488,5 @@
>  
>      // don't filter chrome stuff
>      if (aContentLocation.scheme === 'chrome' ||
> +        aContentLocation.scheme === 'resource' ||
> +        aContentLocation.scheme === 'javascript') {

This fixes bug 548984, right?

::: content/base/test/file_csp_redirects_resource.sjs
@@ +48,5 @@
> +    response.setHeader("Content-Type", "text/plain", false);
> +    response.write("font data...");
> +    return;
> +  }
> +  

nit: trailing whitespace.

::: content/base/test/test_CSP_inlinescript.html
@@ +85,5 @@
> +  is(inlineScriptsThatRan, 4, "there should be 4 inline scripts that ran");
> +  
> +  // The other eight scripts in the other two pages should be blocked.
> +  is(inlineScriptsBlocked, 8, "there should be 8 inline scripts that were blocked"); 
> +    

please check all the files for trailing whitespace.

Comment 37

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661410 [details] [diff] [review]
part 6 v1 - fix up toString

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

Looks good.

::: content/base/src/CSPUtils.jsm
@@ +708,5 @@
>    toString:
>    function csp_toString() {
>      var dirs = [];
>  
> +    if (!this._specCompliant && (this._allowEval || this._allowInlineScripts || this._allowInlineStyles)) {

Nit: this line is kinda long, can you wrap it?

Comment 38

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy] from comment #36)
> What if we catch the token 'unsafe-inline' here, instead of passing it
> through to CSPSource?  Should be a quick oneliner and would remove some of
> the logic from the source parsing.

You know what, nevermind.  Keep doing what you're doing here.

Comment 39

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #35)
> Comment on attachment 661405 [details] [diff] [review]
> Part 2 v2: WIP - main and frameancestors tests
> 
> Review of attachment 661405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine, these are a duplicate of the existing tests.  We could
> streamline these a bit by threading a "1.0 compliant flag" throughout the
> tests and duplicating the tests in each file, but in principle this should
> work too.
> 
> ::: content/base/test/file_CSP_main_spec_compliant.html
> @@ +9,5 @@
> > +    <style>
> > +      /* CSS font embedding tests */
> > +      @font-face {
> > +        font-family: "arbitrary_good";
> > +        src: url('file_CSP.sjs?testid=font_spec_compliant_good&type=application/octet-stream');
> 
> Just making sure, but file_CSP.sjs is just serving the content here, and
> doesn't do anything with the testid, right?  (The test main .html file reads
> that.)

Right, testid isn't used by file_CSP.sjs at all.

I'll try to get to the refactoring/sjs-ification of this stuff soon and then will send it to you for review again.

Comment 40

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #36)
> Comment on attachment 661408 [details] [diff] [review]
> 
> What if we catch the token 'unsafe-inline' here, instead of passing it
> through to CSPSource?  Should be a quick oneliner and would remove some of
> the logic from the source parsing.
> 
> In fact, the CSPSourceList probably doesn't need to know anything about
> whether 'unsafe-inline' means eval or inline.  It can just parse out
> 'unsafe-inline' and then in CSPRep, after we parse the source list, we can
> ask if it contained unsafeInline; if it did, based on the directive name we
> can choose what that means (e.g., set "allows inline script" or "allows
> eval").
> 
> I can see how parsing this deep in a source makes sense for when if we call
> toString() on the sourcelist... but either way is probably fine.
> 
> Yeah, this could probably be uplifted into the source list.

I'm going to leave unsafe-inline and unsafe-eval as they are, it sounds like you're ok with that approach per comment 38 ? 

> ::: content/base/src/contentSecurityPolicy.js
> @@ +488,5 @@
> >  
> >      // don't filter chrome stuff
> >      if (aContentLocation.scheme === 'chrome' ||
> > +        aContentLocation.scheme === 'resource' ||
> > +        aContentLocation.scheme === 'javascript') {
> 
> This fixes bug 548984, right?

Yes, it does.

> please check all the files for trailing whitespace.

Sorry about that !

Comment 41

[Ian Melven :imelven]

Attachment: Part 4 v2 - unsafe-inline support and its tests and updated redirect tests

I want to sjs-ify these tests as well to avoid cluttering up the tree.

Comment 42

[Ian Melven :imelven]

Attachment: part 5 v2 - unsafe-eval support and tests

Unbitrot, carrying over Sid's r+ 

I would like to also sjs-ify the tests in this patch to cut down on tree clutter.

Comment 43

[Ian Melven :imelven]

Attachment: part 6 v2 - fix up toString

Wrap the long line Sid pointed out, carrying over his r+

Comment 44

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #33)
> Comment on attachment 661404 [details] [diff] [review]
> Part 1 v4: WIP - CSP 1.0 spec compliant support
> 
> Review of attachment 661404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/CSPUtils.jsm
> @@ +170,5 @@
> >    function(request, context, status) {
> >      if (Components.isSuccessCode(status)) {
> >        // send the policy we received back to the parent document's CSP
> >        // for parsing
> > +      this._csp.refinePolicy(this._policy, this._docURI, this._csp._specCompliant);
> 
> Since the constructor learns whether this policy is spec compliant or not,
> can we just use the value inside the CSPRep instead of passing another
> argument to refinePolicy?

that means making the 3rd argument to refinePolicy optional and then have the CSPRep's value used if it's not passed in - in some ways i like having it be explicit as it is here, but i'm fine with changing it as well.

> @@ -209,5 @@
> >    OBJECT_SRC:       "object-src",
> >    FRAME_SRC:        "frame-src",
> >    FRAME_ANCESTORS:  "frame-ancestors",
> >    FONT_SRC:         "font-src",
> > -  XHR_SRC:          "xhr-src"
> 
> same as previous comment: we could special-case this if there were two lists
> of SRC_DIRECTIVEs.  There are probably more places that this will simplify
> the multiplexing code too.

i was trying to avoid duplicating the whole directive list, but I think at this point i agree that doing so would lead to cleaner implementation, i'll change this.

> @@ +812,5 @@
> >      var SD = CSPRep.SRC_DIRECTIVES;
> >      var defaultSrcDir = this._directives[SD.DEFAULT_SRC];
> >      if (!defaultSrcDir) {
> > +      // TODO : this string will be wrong for a spec compliant policy
> > +      // based on its id (1.0 spec has no 'allow' directive'
> 
> Ah.  Maybe add a non-localized warning here?
> CSPWarning("foo");

maybe we don't need to log a warning at all here if we're dealing with a spec compliant policy ? 

it seems valid per the spec to have a policy without a default-src (which defaults to default-src *). how about we only log this warning on a non-spec-compliant policy ?

> ::: content/base/src/contentSecurityPolicy.js
> @@ +272,5 @@
> >      this._policy = intersect;
> > +
> > +    if (aSpecCompliant) {
> > +      this._policy._specCompliant = true;
> > +    }
> 
> Could just do this._policy._specCompliant = !!aSpecCompliant (right?)

indeed, i'll do that. 

thanks for the quick reviews !

Comment 45

[Ian Melven :imelven]

Attachment: Part 3 v2 - xpcshell tests for new parser

I had to tweak these a little to accommodate the change to the source directive lists Sid suggested in comment 33

Comment 46

[Ian Melven :imelven]

Attachment: Part 1 v5: CSP 1.0 spec compliant support

Make the changes outlined in comment 44, I left refinePolicy in the policy-uri case the way it was but wouldn't object to changing it as suggested.

I also thought about it again, and I don't think it's necessarily worth the time right now to refactor the tests to use an .sjs but would be open to filing as a followup.

Comment 47

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 665198 [details] [diff] [review]
Part 1 v5: CSP 1.0 spec compliant support

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

This is good.  I'd like to hear your thoughts on some lingering questions.

::: content/base/src/CSPUtils.jsm
@@ +170,5 @@
>    function(request, context, status) {
>      if (Components.isSuccessCode(status)) {
>        // send the policy we received back to the parent document's CSP
>        // for parsing
> +      this._csp.refinePolicy(this._policy, this._docURI, this._csp._specCompliant);

Nit: please wrap and indent the last argument here.

@@ +186,5 @@
>  
>  /**
>   * Class that represents a parsed policy structure.
>   */
> +function CSPRep(aSpecCompliant) {

Please document aSpecCompliant in the comment.

@@ +237,5 @@
>    POLICY_URI:       "policy-uri"  /* single URI */
>  };
>  
>  CSPRep.OPTIONS_DIRECTIVE = "options";
>  CSPRep.ALLOW_DIRECTIVE   = "allow";

Maybe add a comment here, too, that explains "options" and "allow" are pre-1.0 and will eventually get removed.

@@ +517,5 @@
> +          aCSPR._directives[sdi] = dv;
> +          continue directive;
> +        }
> +      }
> +    }

We should probably make this more efficient by just checking if(dirname in SD) -- or similar -- instead of running the loop even though SD is tiny.

@@ +536,5 @@
> +
> +          // if there's no host, don't do the ETLD+ check.  This will throw
> +          // NS_ERROR_FAILURE if the URI doesn't have a host, causing a parse
> +          // failure.
> +           uri.host;

nit: a little too much indentation on this line.

@@ +608,5 @@
> +      // Verify that policy URI comes from the same origin
> +      if (selfUri) {
> +        if (selfUri.host !== uri.host){
> +          CSPError(CSPLocalizer.getFormatStr("nonMatchingHost", [uri.host]));
> +           return CSPRep.fromStringSpecCompliant("default-src 'none'");

Nit: indentation

@@ +629,5 @@
> +        // policy-uri can't be abused for CSRF
> +        chan.loadFlags |= Components.interfaces.nsIChannel.LOAD_ANONYMOUS;
> +        chan.asyncOpen(new CSPPolicyURIListener(uri, docRequest, csp), null);
> +      }
> +     catch (e) {

Nit: indentation.

@@ +638,5 @@
> +      }
> +
> +      // return a fully-open policy to be intersected with the contents of the
> +      // policy-uri when it returns
> +      // policy-uri when it returns

Please remove duplicated line.

@@ +743,5 @@
>    intersectWith:
>    function cspsd_intersectWith(aCSPRep) {
>      var newRep = new CSPRep();
>  
> +    let DIRS = aCSPRep._specCompliant ? CSPRep.SRC_DIRECTIVES_NEW : CSPRep.SRC_DIRECTIVES_OLD;

Nit: please wrap and indent long line.

@@ +792,5 @@
> +
> +    // It's ok for a 1.0 spec compliant policy to not have a default source,
> +    // in this case it should use default-src *, see
> +    // bug 764937
> +    if (!defaultSrcDir && !this._specCompliant) {

So this fixes that bug, right?  Maybe we don't need to mention it here, but it should be clear that non-spec-compliant policies require a default-src or allow directive.

::: content/base/src/contentSecurityPolicy.js
@@ +22,5 @@
>  
> +// Needed to support CSP 1.0 spec and our original CSP implementation - should
> +// be removed when our original implementation is deprecated.
> +const CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT = "csp_type_xmlhttprequest_spec_compliant";
> +const CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT = "csp_type_websocket_spec_compliant";

It's not completely clear from the comment what these are for.  Please add something like "the content policy needs to know if the CSP is spec compliant or not, and so these are special contexts to identify this."

@@ +82,4 @@
>  
> +  /* Our original CSP implementation's mappings for XHR and websocket
> +   * These should be changed to be = cspr_sd.CONNECT_SRC when we remove
> +   * the original implementation - NOTE: order in this array is important !!!

Why does the order matter?

@@ +94,5 @@
> +
> +  /* CSP 1.0 spec compliant mappings for XHR and websocket */
> +  csp._MAPPINGS[CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT]    = cspr_sd_new.CONNECT_SRC;
> +  csp._MAPPINGS[CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT]         = cspr_sd_new.CONNECT_SRC;
> +  // TODO : EventSource should be here and also use connect-src

Why isn't it here?  Is there a bug filed for this?

@@ +445,5 @@
>  
>      // scan the discovered ancestors
> +    // frame-ancestors is the same in both old and new source directives,
> +    // so don't need to differentiate here.
> +    let cspContext = CSPRep.SRC_DIRECTIVES_OLD.FRAME_ANCESTORS;

Should we use SRC_DIRECTIVES instead of SRC_DIRECTIVES_OLD?  This way when we deprecate all the old stuff we don't have to edit this.

@@ +493,5 @@
> +    var cspContext;
> +
> +    let cp = Ci.nsIContentPolicy;
> +
> +    CSPdebug("policy is specCompliant : " + this._policy._specCompliant);

This is a little confusing, perhaps use the value of _specCompliant to change the message.  E.g:
CSPdebug("policy is " + (this._policy.specCompliant ? 
                         "1.0 compliant" : "pre-1.0"));

Comment 48

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 665197 [details] [diff] [review]
Part 3 v2 - xpcshell tests for new parser

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

::: content/base/test/unit/test_csputils.js
@@ +21,5 @@
>  const POLICY_URI = "http://localhost:" + POLICY_PORT + "/policy";
>  const POLICY_URI_RELATIVE = "/policy";
>  
> +// list of all the source directives
> +const SD = CSPRep.SRC_DIRECTIVES;

SRC_DIRECTIVES_OLD or SRC_DIRECTIVES_NEW?  After the other patch in this bug, CSPRep no longer has this symbol.  I think you want _NEW here, and then to change all the old tests to locally use _OLD.

Comment 49

[Ian Melven :imelven]

After talking to Sid, I'm rebasing these patches to apply direct to m-c (instead of on top of 783049) and also removing the inline style code, marking this as blocking the inline style work in bug 763879 instead of the other way around as well - the rationale here is that we want to get the CSP 1.0 spec compliant syntax landed asap and the inline style stuff is a little stalled out currently.

Comment 50

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #48)
> Comment on attachment 665197 [details] [diff] [review]
> Part 3 v2 - xpcshell tests for new parser
> 
> Review of attachment 665197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/test/unit/test_csputils.js
> @@ +21,5 @@
> >  const POLICY_URI = "http://localhost:" + POLICY_PORT + "/policy";
> >  const POLICY_URI_RELATIVE = "/policy";
> >  
> > +// list of all the source directives
> > +const SD = CSPRep.SRC_DIRECTIVES;
> 
> SRC_DIRECTIVES_OLD or SRC_DIRECTIVES_NEW?  After the other patch in this
> bug, CSPRep no longer has this symbol.  I think you want _NEW here, and then
> to change all the old tests to locally use _OLD.

SD wasn't actually used anywhere - the separate tests use 'var sd' which is set to SRC_DIRECTIVES_OLD or SRC_DIRECTIVES_NEW as appopriate for that test. I'll remove the const SD to make sure this is true.

Comment 51

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #47)
> Comment on attachment 665198 [details] [diff] [review]
> Part 1 v5: CSP 1.0 spec compliant support
> 
> Review of attachment 665198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is good.  I'd like to hear your thoughts on some lingering questions.

I made all the changes you suggested :)

> @@ +82,4 @@
> >  
> > +  /* Our original CSP implementation's mappings for XHR and websocket
> > +   * These should be changed to be = cspr_sd.CONNECT_SRC when we remove
> > +   * the original implementation - NOTE: order in this array is important !!!
> 
> Why does the order matter?

because these are really enums defined in nsIContentPolicy.idl 

> > +  // TODO : EventSource should be here and also use connect-src
> 
> Why isn't it here?  Is there a bug filed for this?

CSP support for EventSource doesn't seem to have been implemented. i filed bug 802872 for this and referenced it in the comment, as well as referencing Bug 667490 - EventSource should use the same nsIContentPolicy type as XHR which smaug fixed as part of the EventSource security review long ago. 
 
> @@ +445,5 @@
> >  
> >      // scan the discovered ancestors
> > +    // frame-ancestors is the same in both old and new source directives,
> > +    // so don't need to differentiate here.
> > +    let cspContext = CSPRep.SRC_DIRECTIVES_OLD.FRAME_ANCESTORS;
> 
> Should we use SRC_DIRECTIVES instead of SRC_DIRECTIVES_OLD?  This way when
> we deprecate all the old stuff we don't have to edit this.

I changed it to SRC_DIRECTIVES_NEW - there isn't plain SRC_DIRECTIVES any more. but using _NEW should make it clear how to change this when we deprecate the old implementation.

I also realized the approach I outlined in comment 49 won't work - in order for the tests in this bug to pass, as it stands the patches for 783049 are needed so the new header used in the new tests will be correctly processed. Sid and I discussed maybe using a hidden pref to be able to selectively turn on the new header parsing in 783049 for the tests. Then we can remove the pref check when we're ready to turn on CSP 1.0 for real.

Comment 52

[Ian Melven :imelven]

Attachment: Part 1 v6: CSP 1.0 spec compliant support

I made the changes suggested by Sid in the previous review and all the CSP tests are now passing again.

The patch queue for this looks like :
bug783049-1
bug783049-2
bug783049-3
bug746978-1
bug746978-2
csp1.0xpcshelltests
bug746978-3
bug746978-4
bug746978-5

the inline styles stuff is still there but the code that uses it is not (ie the patches around bug 763879)

https://hg.mozilla.org/users/imelven_mozilla.com/csp-patches/summary contains this patch queue and patches

Comment 53

[Ian Melven :imelven]

Attachment: Part 2 v3: WIP - main and frameancestors tests

Updated patch, needs review

Comment 54

[Ian Melven :imelven]

Attachment: Part 3 v3 - xpcshell tests for new parser

Updated patch, carrying over r+ from jst

Comment 55

[Ian Melven :imelven]

Attachment: Part 4 v3 - unsafe-inline support and its tests and updated redirect tests

Updated patch, Sid - I think you looked at this before but it's unclear whether it got an r+ so i r? to you

Comment 56

[Ian Melven :imelven]

Attachment: part 5 v3 - unsafe-eval support and tests

Updated patch, carrying over Sid's r+

Comment 57

[Ian Melven :imelven]

Attachment: part 6 v3 - fix up toString

Updated patch, carrying over Sid's r+

Comment 58

[Ian Melven :imelven]

Attachment: part 7 v1 - turn on prefs to opt in to new parser for tests

i added a pref check to the patches for bug 783049 where if the unprefixed header is seen, it will only be processed and the new parser used if this pref (security.csp.speccompliant) is set to true. I'll upload that patch to that bug once i finish up the web console logging stuff. 

*this* patch just sets that pref to true at the start of every CSP test and also resets it to its original value at the end of every CSP test.

Comment 59

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 676801 [details] [diff] [review]
Part 1 v6: CSP 1.0 spec compliant support

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

looks good.  Address the two minor comments and r=me.

::: content/base/src/CSPUtils.jsm
@@ +143,5 @@
>      if (Components.isSuccessCode(status)) {
>        // send the policy we received back to the parent document's CSP
>        // for parsing
> +      this._csp.refinePolicy(this._policy, this._docURI,
> +                             this._csp._specCompliant);

To be clear, this change removes an unused third (erroneous) argument passed to refinePolicy, and adds a new one (_specCompliant).   This last parameter will be added by fixing bug 783049.

@@ +148,4 @@
>      }
>      else {
>        // problem fetching policy so fail closed
> +      this._csp.refinePolicy("default-src 'none'", this._docURI, true);

Should the last argument here be "true" or this._csp._specCompliant?

::: content/base/src/contentSecurityPolicy.js
@@ +98,5 @@
> +  // address this. These won't be needed when we deprecate our original
> +  // implementation.
> +  csp._MAPPINGS[CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT]    = cspr_sd_new.CONNECT_SRC;
> +  csp._MAPPINGS[CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT]         = cspr_sd_new.CONNECT_SRC;
> +  // TODO : EventSource should be here and also use connect-src

Should be or "will be here when fixing bug 802872"?

Comment 60

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 676803 [details] [diff] [review]
Part 2 v3: WIP - main and frameancestors tests

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

These look fine, though if we're going to land these tests without the patch to flip on the pref, we either need to tweak the file to avoid running the new tests or use SpecialPowers.pushPrefEnv() to set the pref for the tests.

Comment 61

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 676806 [details] [diff] [review]
Part 4 v3 - unsafe-inline support and its tests and updated redirect tests

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

Since "file_CSP_inlinestyle_main_spec_compliant2.html" is checking that inline scripts *can* run, you might want to s/2/_allowed/ in the filename or put a comment in the file about what it's testing.

Also, see note about using pushPrefEnv to set the pref for if we land this before turning the pref on by default.  We either wanna set the pref or disable the tests.

With these fixes, r=me.

::: content/base/src/CSPUtils.jsm
@@ +525,5 @@
> +            // Check for unsafe-inline and unsafe-eval in script-src
> +            if (dv._allowUnsafeInline) {
> +              aCSPR._allowInlineScripts = true;
> +            } else if (dv._allowUnsafeEval) {
> +              // TODO: eval

I assume this TODO will be done in this bug.

::: content/base/src/contentSecurityPolicy.js
@@ +458,5 @@
>  
>      // don't filter chrome stuff
>      if (aContentLocation.scheme === 'chrome' ||
> +        aContentLocation.scheme === 'resource' ||
> +        aContentLocation.scheme === 'javascript') {

Note to self: when landing this, update bug 548984.

::: content/base/test/file_CSP_inlinescript_main_spec_compliant.html
@@ +7,5 @@
> +    <script type="text/javascript">
> +      window.parent.scriptRan(false, "textnode", "text node in a script tag executed.");
> +    </script>
> +
> +    <iframe src='javascript:window.parent..parent.scriptRan(false, "jsuri", "javascript: uri in image tag")' ></iframe>

You probably want parent.parent and not parent..parent here.

::: content/base/test/file_CSP_inlinescript_main_spec_compliant2.html
@@ +4,5 @@
> +  </head>
> +  <body onload="window.parent.scriptRan(true, 'eventattr', 'event attribute in body tag fired')">
> +
> +    <script type="text/javascript">
> +      //alert(window.parent.scriptRan);

nit: remove this commented-out alert.

Comment 62

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 677192 [details] [diff] [review]
part 7 v1 - turn on prefs to opt in to new parser for tests

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

So I've been thinking about this, and we should use pushPrefEnv instead for the tests (and probably just put it right into the patches that create these tests).  That's a bit easier than recording the old value and setting the new one.

The pattern is something like:

// ... do everything else in the test_CSP*.html file except trigger the test frame loads
SpecialPowers.pushPrefEnv(
  {'set':[["security.csp.speccompliant", true]]},
  function() {
    // ... trigger the test frame loads
  });

You'll wanna call "new examiner()" before the call to pushPrefEnv, and also SimpleTest.waitForExplicitFinish().  You'll want to verify that any callbacks scope global vars properly (using window. to locate them) or the tests might barf.

I'm gonna remove the r? on this patch not because it needs work, but you should drop this patch and fold these edits right into the other test patches.  (It's trivial, so no need to r? me again as long as you test and verify the changes).

Comment 63

[Ian Melven :imelven]

Comment on attachment 677192 [details] [diff] [review]
part 7 v1 - turn on prefs to opt in to new parser for tests

This got folded into the other patches in this bug, thanks Sid !

Comment 64

[Ian Melven :imelven]

Comment on attachment 661411 [details] [diff] [review]
tweaks to patch in bug 763879 for inline styles

This will get handled in bug 763879

Comment 65

[Ian Melven :imelven]

I removed the inline styles tests from these patches, I think it makes more sense to work on everything to do with inline styles in bug 763879

Comment 66

[Ian Melven :imelven]

Attachment: Part 1 v7 - CSP 1.0 spec compliant support

(In reply to Sid Stamm [:geekboy] from comment #59)
> Comment on attachment 676801 [details] [diff] [review]
> Part 1 v6: CSP 1.0 spec compliant support
> 
> Review of attachment 676801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good.  Address the two minor comments and r=me.
> 
> ::: content/base/src/CSPUtils.jsm
>
> @@ +148,4 @@
> >      }
> >      else {
> >        // problem fetching policy so fail closed
> > +      this._csp.refinePolicy("default-src 'none'", this._docURI, true);
> 
> Should the last argument here be "true" or this._csp._specCompliant?

default-src 'none' looks to be valid in both kinds of CSP, so I changed this to this._csp._specCompliant

> ::: content/base/src/contentSecurityPolicy.js
> @@ +98,5 @@
> > +  // address this. These won't be needed when we deprecate our original
> > +  // implementation.
> > +  csp._MAPPINGS[CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT]    = cspr_sd_new.CONNECT_SRC;
> > +  csp._MAPPINGS[CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT]         = cspr_sd_new.CONNECT_SRC;
> > +  // TODO : EventSource should be here and also use connect-src
> 
> Should be or "will be here when fixing bug 802872"?

I made this comment clearer along these lines.

Comment 67

[Ian Melven :imelven]

Attachment: Part 2 v4 - CSP main and frameancestor tests

Add the pushPrefEnv, unbitrot, carrying over r+

Comment 68

[Ian Melven :imelven]

Attachment: Part 3 v4 - xpcshell tests for new parser

Unbitrot, carrying over r+

Comment 69

[Ian Melven :imelven]

Attachment: Part 4 v4 - unsafe-inline support and its tests and updated redirect tests

(In reply to Sid Stamm [:geekboy] from comment #61)
> Comment on attachment 676806 [details] [diff] [review]
> Part 4 v3 - unsafe-inline support and its tests and updated redirect tests
> 
> Review of attachment 676806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since "file_CSP_inlinestyle_main_spec_compliant2.html" is checking that
> inline scripts *can* run, you might want to s/2/_allowed/ in the filename or
> put a comment in the file about what it's testing.

I changed the file to file_CSP_inlinestyle_main_spec_compliant_allowed.html

In part 5 I made this change for the file_CSP_evalscript_main_spec_compliant2.html as well (it's now file_CSP_evalscript_main_spec_compliant_allowed.html)

> Also, see note about using pushPrefEnv to set the pref for if we land this
> before turning the pref on by default.  We either wanna set the pref or
> disable the tests.

I added the pushPrefEnv.

> With these fixes, r=me.
> 
> ::: content/base/src/CSPUtils.jsm
> @@ +525,5 @@
> > +            // Check for unsafe-inline and unsafe-eval in script-src
> > +            if (dv._allowUnsafeInline) {
> > +              aCSPR._allowInlineScripts = true;
> > +            } else if (dv._allowUnsafeEval) {
> > +              // TODO: eval
> 
> I assume this TODO will be done in this bug.

yeah, eval is handled in part 5

> ::: content/base/src/contentSecurityPolicy.js
> @@ +458,5 @@
> >  
> >      // don't filter chrome stuff
> >      if (aContentLocation.scheme === 'chrome' ||
> > +        aContentLocation.scheme === 'resource' ||
> > +        aContentLocation.scheme === 'javascript') {
> 
> Note to self: when landing this, update bug 548984.

i made bug 548984 depend on this bug so we will remember to mark it fixed when this lands :)

> ::: content/base/test/file_CSP_inlinescript_main_spec_compliant.html
> @@ +7,5 @@
> > +    <script type="text/javascript">
> > +      window.parent.scriptRan(false, "textnode", "text node in a script tag executed.");
> > +    </script>
> > +
> > +    <iframe src='javascript:window.parent..parent.scriptRan(false, "jsuri", "javascript: uri in image tag")' ></iframe>
> 
> You probably want parent.parent and not parent..parent here.

you already fixed this in the CSP patch repo, thank you !

> ::: content/base/test/file_CSP_inlinescript_main_spec_compliant2.html
> @@ +4,5 @@
> > +  </head>
> > +  <body onload="window.parent.scriptRan(true, 'eventattr', 'event attribute in body tag fired')">
> > +
> > +    <script type="text/javascript">
> > +      //alert(window.parent.scriptRan);
> 
> nit: remove this commented-out alert.

done !

Comment 70

[Ian Melven :imelven]

Attachment: Part 5 v4 - eval, unsafe-inline for it, tests

added pushPrefEnv, carrying over the r+ from Sid

Comment 71

[Ian Melven :imelven]

These patches should be good to go modulo bitrot once bug 783049 is done, which I am working on next.

Comment 72

[Ian Melven :imelven]

Attachment: Part 1 v8 - CSP 1.0 spec compliant support

Some minor tweaks to fix the logging in CSPRep.fromStringSpecCompliant that I found while writing a test for bug 783049, carrying over the r+

Comment 73

[Ian Melven :imelven]

Attachment: Part 1 v9 - CSP 1.0 spec compliant support

Tiny tweak needed due to review feedback on bug 783049, carrying over r+

Comment 74

[Ian Melven :imelven]

Going to try to land this bug and bug 783049 - the CSP 1.0 parser will land pref'd off.

https://tbpl.mozilla.org/?tree=Try&rev=39176223dd44

Comment 75

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a16f8c77ab42
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a5547ae073
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b10284c48c
https://hg.mozilla.org/integration/mozilla-inbound/rev/11732b70b74d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff903c405aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb9e62dc084b

Comment 76

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a16f8c77ab42
https://hg.mozilla.org/mozilla-central/rev/e6a5547ae073
https://hg.mozilla.org/mozilla-central/rev/34b10284c48c
https://hg.mozilla.org/mozilla-central/rev/11732b70b74d
https://hg.mozilla.org/mozilla-central/rev/3ff903c405aa
https://hg.mozilla.org/mozilla-central/rev/eb9e62dc084b

Comment 77

[Ian Melven :imelven]

The pref to enable this won't be turned on in Fx21 so I suspect this doesn't need a relnote yet ?

Comment 78

[Jean-Yves Perrier [:teoli]]

I missed the pref ;-) You're right.

Comment 79

[Kohei Yoshino]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 80

[Ian Melven :imelven]

(In reply to Kohei Yoshino from comment #79)
> I've added this bug to the compatibility doc. Please correct the info if I'm
> wrong.
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Although this code landed in Fx21 it won't be turned on until bug 842657 to turn on the pref to enable lands - I hope to do this for Fx22 so I'd suggest that we add it to Fx22's compatibility doc if/when that happens. The info in the doc looks good though, thank you !

Comment 81

[Kohei Yoshino]

(In reply to Ian Melven :imelven from comment #80)
> Although this code landed in Fx21 it won't be turned on until bug 842657 to
> turn on the pref to enable lands - I hope to do this for Fx22 so I'd suggest
> that we add it to Fx22's compatibility doc if/when that happens. The info in
> the doc looks good though, thank you !

I was missing your comment... will move the info to the Firefox 23 compat doc. Thanks for your feedback!

Comment 82

[Ian Melven :imelven]

(In reply to Kohei Yoshino [:kohei] from comment #81)
> (In reply to Ian Melven :imelven from comment #80)
> > Although this code landed in Fx21 it won't be turned on until bug 842657 to
> > turn on the pref to enable lands - I hope to do this for Fx22 so I'd suggest
> > that we add it to Fx22's compatibility doc if/when that happens. The info in
> > the doc looks good though, thank you !
> 
> I was missing your comment... will move the info to the Firefox 23 compat
> doc. Thanks for your feedback!

Thanks ! Please see https://blog.mozilla.org/security/2013/06/11/content-security-policy-1-0-lands-in-firefox/ if you need more info on the changes with CSP 1.0.