Closed Bug 1033423 Opened 5 years ago Closed 5 years ago

CSP reports do not correctly escape newline characters

Categories

(Core :: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: mgoodwin, Assigned: ckerschb)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 9 obsolete files)

1.72 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.92 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
9.18 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Issue:
CSP reports containing script samples retain newlines from the script without escaping newlines. The resulting JSON is, therefore, invalid.

STR:
1) Set up your browser to proxy through a mitm proxy (e.g. OWASP ZAP)
2) Visit http://people.mozilla.org/~mgoodwin/csp/test2.html
3) Observe one of the CSP reports is malformed

Remediation:
Data included in CSP reports should be adequately JSON encoded.
Duplicate of this bug: 1033425
Attached patch bug_1033423.patch (obsolete) — Splinter Review
Probably we have some escaping goodies available in our codebase. As of now, I haven't seen anything that would do. The patch provides it's own string escaping which is probably exactly what we want. If there is something better in the codebase, please let me know.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Created attachment 8450599 [details] [diff] [review]
> bug_1033423.patch
> 
> Probably we have some escaping goodies available in our codebase. As of now,
> I haven't seen anything that would do. The patch provides it's own string
> escaping which is probably exactly what we want. If there is something
> better in the codebase, please let me know.

RFC 4627 says that control characters should also be escaped:

> 2.5.  Strings
> 
>    The representation of strings is similar to conventions used in the C
>    family of programming languages.  A string begins and ends with
>    quotation marks.  All Unicode characters may be placed within the
>    quotation marks except for the characters that must be escaped:
>    quotation mark, reverse solidus, and the control characters (U+0000
>    through U+001F).

And do we handle Unicode correctly?
This is the specification of the quote operation for stringifying JSON: <http://es5.github.io/x15.12.html#Quote>

This is the implementation in the JS engine: <http://mxr.mozilla.org/mozilla-central/source/js/src/json.cpp#62>  I suggest filing a bug to ask the JS engine to export this as an API that you can call to avoid duplicating the logic.
Depends on: 925004
Attached patch bug_1033423.patch (obsolete) — Splinter Review
Sid, can you review this first? Once you r+, we can/should let bholley look it over to make sure we got the setup for the JS part correct.

Once done, we should also uplift the patch!

Also, since we are creating all the necessary foo values ourselves, I guess there is no need for "Bug 1035251 - Export JSONs escaping mechanism to avoid duplicating code" anymore. Sid, if you agree, we can mark 1035251 as a wontfix.
Attachment #8450599 - Attachment is obsolete: true
Attachment #8464177 - Flags: review?(sstamm)
Comment on attachment 8464177 [details] [diff] [review]
bug_1033423.patch

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

There looks to be a lot of common code for the various properties.  Can you find a clean way to factor out the boilerplate into a macro or inline function so it's easier to read?  For example:

> JS::Rooted<JSString*> jsDocumentURI(jsCX, JS_NewUCStringCopyN(jsCX, 
>                                                               NS_ConvertUTF8toUTF16(documentURI).get(),
>                                                               documentURI.Length()));
> NS_ENSURE_TRUE(jsDocumentURI, NS_ERROR_OUT_OF_MEMORY);
> JS_DefineProperty(jsCX, reportProperty, "document-uri", jsDocumentURI, JSPROP_ENUMERATE);

Could become something like:

> // jsonify document-uri
> CSP_DefineJSONObject(jsCX, "document-uri",
>                      NS_ConvertUTF8toUTF16(documentURI).get(),
>                      documentURI.Length(),
>                      reportProperty);

Goal should be to make this really trivial to fix errors or add fields to the report.  If you can do that you may be able to lift some of the report construction back into ::SendReports to keep the stack depth constant... or not, probably doesn't matter a whole lot at that point so long as the common stuff gets factored out.

In general, looks pretty good but I'd like to see another patch first with much of the JS stuff factored out.

::: content/base/src/nsCSPContext.cpp
@@ +601,5 @@
> +{
> +  // first, let's generate a javascript context
> +  mozilla::dom::AutoJSAPI jsapi;
> +  jsapi.Init();
> +  JSContext *jsCX = jsapi.cx();

"cx" is most commonly used for the JSContext variable name.  (It's also shorter.)

@@ +626,5 @@
> +  else {
> +    // this can happen for frame-ancestors violation where the violating
> +    // ancestor is cross-origin.
> +    NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report.");
> +  }

Should this be a separate if check after the previous block?  What if blockedURI has no value after the previous few lines?  We should warn about that here too.
Attachment #8464177 - Flags: review?(sstamm) → review-
Attached patch bug_1033423_v1.patch (obsolete) — Splinter Review
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6)
> Comment on attachment 8464177 [details] [diff] [review]
> bug_1033423.patch
> 
> Review of attachment 8464177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There looks to be a lot of common code for the various properties.  Can you
> find a clean way to factor out the boilerplate into a macro or inline
> function so it's easier to read?

Yes, I added a method for defining properties on a JSObj into the nested struct ‘local’, which allows us to use the namespace ::local. We don’t want any of the two functions within that struct to be visible outside of the function that generates the JSON.


> ::: content/base/src/nsCSPContext.cpp
> @@ +601,5 @@
> > +{
> > +  // first, let's generate a javascript context
> > +  mozilla::dom::AutoJSAPI jsapi;
> > +  jsapi.Init();
> > +  JSContext *jsCX = jsapi.cx();
> 
> "cx" is most commonly used for the JSContext variable name.  (It's also
> shorter.)

Initially, I wanted to call it “jsCX”, because “cx” within contentPolicies is most often used for the requesting context, but now as I think about it, we should be consistent with what is used throughout the codebase.

> @@ +626,5 @@
> > +  else {
> > +    // this can happen for frame-ancestors violation where the violating
> > +    // ancestor is cross-origin.
> > +    NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report.");
> > +  }
> 
> Should this be a separate if check after the previous block?  What if
> blockedURI has no value after the previous few lines?  We should warn about
> that here too.

Actually, I haven’t touched that part of the code, but I agree, we should make that check. Good catch!


I also pushed this patch to try, using the ASAN, to make sure we do not have any ‘use after free’ issues for all those strings. Should be fine though. Here is the link:
https://tbpl.mozilla.org/?tree=Try&rev=c5f9f08821f9

Once you r+ the patch, I will flag bholley for review on the JS part.
Attachment #8464177 - Attachment is obsolete: true
Attachment #8464465 - Flags: review?(sstamm)
Comment on attachment 8464465 [details] [diff] [review]
bug_1033423_v1.patch

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

This all looks fine to me, need bholley to take a look too.

Can you write a test or update the test_cspreports.js xpcshell test to check for properly escaped newline chars?
Attachment #8464465 - Flags: review?(sstamm)
Attachment #8464465 - Flags: review?(bobbyholley)
Attachment #8464465 - Flags: review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> Comment on attachment 8464465 [details] [diff] [review]
> bug_1033423_v1.patch
> 
> Review of attachment 8464465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks fine to me, need bholley to take a look too.
> 
> Can you write a test or update the test_cspreports.js xpcshell test to check
> for properly escaped newline chars?

Sure can. Probably also worth adding a test for escaping quotes in the scriptsample correctly since bug 1033425 [1] was marked as a duplicate of this one.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1033425
Comment on attachment 8464465 [details] [diff] [review]
bug_1033423_v1.patch

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

I don't think this is the right way to go. I think we should define a webIDL Dictionary, and then invoke ToJS on it. That'll let the bindings machinery take care of all of the nasty JSAPI.

::: content/base/src/nsCSPContext.cpp
@@ +602,5 @@
> +  // generating namespace ::local, so that the following
> +  // two methods are only called from within that function.
> +  struct local {
> +    static inline nsresult
> +    DefinePropOnJSObj(JSContext *aCx, JS::Rooted<JSObject*>& aJSObj,

I'd call this DefineStringOnObject.

@@ +608,5 @@
> +      JS::Rooted<JSString*>
> +       propVal(aCx, JS_NewUCStringCopyN(aCx, aPropVal.BeginReading(),
> +                                        aPropVal.Length()));
> +      NS_ENSURE_TRUE(propVal, NS_ERROR_OUT_OF_MEMORY);
> +      JS_DefineProperty(aCx, aJSObj, aPropName, propVal, JSPROP_ENUMERATE);

You need to check for exceptions here.

@@ +624,5 @@
> +  };
> +
> +  // first, let's generate a javascript context
> +  mozilla::dom::AutoJSAPI jsapi;
> +  jsapi.Init();

you need to check for failure here.

@@ +629,5 @@
> +  JSContext *cx = jsapi.cx();
> +  JSAutoCompartment ac(cx, xpc::GetSafeJSContextGlobal()); // usage approved by bholley
> +
> +  JS::Rooted<JSObject*> reportObj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(),
> +                                                   JS::NullPtr()));

You need to check for OOM here.

@@ +699,5 @@
> +  }
> +
> +  // line-number
> +  if (aLineNum != 0) {
> +    JS_DefineProperty(cx, reportObj, "line-number", aLineNum, JSPROP_ENUMERATE);

And here.

@@ +704,5 @@
> +  }
> +
> +  // finally, let's Stringify the JSON
> +  JS::Rooted<JSObject*> cspObj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(),
> +                                                   JS::NullPtr()));

And here.

@@ +705,5 @@
> +
> +  // finally, let's Stringify the JSON
> +  JS::Rooted<JSObject*> cspObj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(),
> +                                                   JS::NullPtr()));
> +  JS_DefineProperty(cx, cspObj, "csp-report", reportObj, JSPROP_ENUMERATE);

And here.
Attachment #8464465 - Flags: review?(bobbyholley) → review-
Assignee: nobody → mozilla
(In reply to Bobby Holley (:bholley) from comment #10)
> Comment on attachment 8464465 [details] [diff] [review]
> bug_1033423_v1.patch
> 
> Review of attachment 8464465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is the right way to go. I think we should define a webIDL
> Dictionary, and then invoke ToJS on it. That'll let the bindings machinery
> take care of all of the nasty JSAPI.

Can you be a little more explicit of what you have in mind in detail?

> @@ +608,5 @@
> > +      JS::Rooted<JSString*>
> > +       propVal(aCx, JS_NewUCStringCopyN(aCx, aPropVal.BeginReading(),
> > +                                        aPropVal.Length()));
> > +      NS_ENSURE_TRUE(propVal, NS_ERROR_OUT_OF_MEMORY);
> > +      JS_DefineProperty(aCx, aJSObj, aPropName, propVal, JSPROP_ENUMERATE);
> 
> You need to check for exceptions here.

It seems we have to do a lot of cleanup in our codebase, where we use JS_DefineProperty, e,g:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#991

But I agree, error checking is great. I will fix that.

> @@ +624,5 @@
> > +  };
> > +
> > +  // first, let's generate a javascript context
> > +  mozilla::dom::AutoJSAPI jsapi;
> > +  jsapi.Init();
> 
> you need to check for failure here.

How can I check for failure here?
Flags: needinfo?(bobbyholley)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > Comment on attachment 8464465 [details] [diff] [review]
> > bug_1033423_v1.patch
> > 
> > Review of attachment 8464465 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't think this is the right way to go. I think we should define a webIDL
> > Dictionary, and then invoke ToJS on it. That'll let the bindings machinery
> > take care of all of the nasty JSAPI.
> 
> Can you be a little more explicit of what you have in mind in detail?

Basically, if you define a dictionary type somewhere in dom/webidl, a helper struct will be automatically generated for you at runtime that takes care of all of this stuff. All you need to do is set the members of the struct to the appropriate XPCOM strings, and then invoke ToJS on it.

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types

Boris, do you have a preference for where people put non-web dictionaries? Should we make a new .webidl file for each, or should we have some sort of GeckoDictionaries.webidl?

> It seems we have to do a lot of cleanup in our codebase, where we use
> JS_DefineProperty, e,g:

That's exactly why we're trying to minimize the usage of manual JSAPI in Gecko. ;-)

> How can I check for failure here?

Oh, you're right. The no-argument Init() is infallible.
Flags: needinfo?(bobbyholley) → needinfo?(bzbarsky)
> Should we make a new .webidl file for each

Yes, please.  Dictionary idl headers have to pull in lots of other headers (for the dictionary members), so generally we want one dictionary per header.

Note that we were talking about just adding a ToJSON on WebIDL dictionaries in bug 1025230.  I'm happy to just do that, for this bug to use.
Flags: needinfo?(bzbarsky)
Bobby, I am following your suggestions to create a webidl for the csp-report and then stringify it by calling StringifyToJSON() on it. I got 2 questions however:

1) Why is StringifyToJSON 'protected' [1]? Wouldn't it be better to use the 'public' qualifier here? Don't we just want to create an webidl-object and then call StringifyToJSON() on that object directly?

2) CSPReports use dashes internally for their names, see the following property list:

csp-report:
  * blocked-uri
  * document-uri
  * original-policy
  * referrer
  * violated-directive
  * source-file
  * script-sample
  * line-number

When compiling, WebIDL quits with the following error because it doesn't allow dashes:

> WebIDL.WebIDLError: error: invalid syntax, mc/dom/webidl/CSPReport.webidl line 19:11
> long line-number = 0;

What would be the best way forward here?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingDeclarations.h#35
Flags: needinfo?(bobbyholley)
> 1) Why is StringifyToJSON 'protected'

Because the public API is called ToJSON.

> CSPReports use dashes internally for their names

That's a pretty nasty API for JS consumers.  :(  Web IDL can't handle that right now, though we could add some sort of escaping mechanism to allow it...
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #16)
> > CSPReports use dashes internally for their names
> 
> That's a pretty nasty API for JS consumers.  :(  Web IDL can't handle that
> right now, though we could add some sort of escaping mechanism to allow it...

Yeah, can we just change that? How widespread is the usage here? Cursory MXR-ing indicates "not so much".
Flags: needinfo?(bobbyholley)
> How widespread is the usage here?

The JSON needs to have those dash bits per the spec.  See https://w3c.github.io/webappsec/specs/content-security-policy/#example-violation-report for an example of what the JSON for a violation report is supposed to look like.
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #18)
> > How widespread is the usage here?
> 
> The JSON needs to have those dash bits per the spec.  See
> https://w3c.github.io/webappsec/specs/content-security-policy/#example-
> violation-report for an example of what the JSON for a violation report is
> supposed to look like.

Yuck.
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #16)
> > 1) Why is StringifyToJSON 'protected'
> 
> Because the public API is called ToJSON.

Facepalm - makes sense now - thanks.

(In reply to On vacation Aug 5-18.  Please do not request review. from comment #18)
> > How widespread is the usage here?
> 
> The JSON needs to have those dash bits per the spec.  See
> https://w3c.github.io/webappsec/specs/content-security-policy/#example-
> violation-report for an example of what the JSON for a violation report is
> supposed to look like.

Yeah, as Boris pointed out, it's part of the spec.
I filed bug 1048437 on allowing dictionary members with dashed names.  In the C++ the '-' would be converted to '_' per my patch in that bug, but I'm open to suggestions for other options for how to map those names to C++.
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #21)
> I filed bug 1048437 on allowing dictionary members with dashed names.  In
> the C++ the '-' would be converted to '_' per my patch in that bug, but I'm
> open to suggestions for other options for how to map those names to C++.

Sounds great to me - thanks a lot Boris!
Attached patch bug_1033424_webidl.patch (obsolete) — Splinter Review
I applied the patch provided by bz so that webidl names can make use of "-". Works just fine.
Attachment #8467258 - Flags: review?(bobbyholley)
Attached patch bug_1033424_csp_json.patch (obsolete) — Splinter Review
Attachment #8464465 - Attachment is obsolete: true
Attachment #8467259 - Flags: review?(sstamm)
Attachment #8467259 - Flags: review?(bobbyholley)
Comment on attachment 8467258 [details] [diff] [review]
bug_1033424_webidl.patch

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

::: dom/webidl/CSPReport.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +  * This dictionnary holds the parameters used to send

nit - dictionary

@@ +7,5 @@
> +  * CSP reports in JSON format.
> +  */
> +
> +dictionary CSPReportProperties
> +{

Nit - { goes at the end of the previous line.

@@ +19,5 @@
> +  long line-number = 0;
> +};
> +
> +dictionary CSPReport
> +{

Same here.
Attachment #8467258 - Flags: review?(bobbyholley) → review+
Comment on attachment 8467259 [details] [diff] [review]
bug_1033424_csp_json.patch

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

Looks so much better!

r=bholley with that fix.

::: content/base/src/nsCSPContext.cpp
@@ +655,4 @@
>    }
>  
> +  mozilla::dom::CSPReport report;
> +  report.mCsp_report = reportProperties;

This is going to cause a lot of unnecessary copying. Can you just declare |report| at the top and do all of the assignments to report.mCsp_report.mFoo?
Attachment #8467259 - Flags: review?(bobbyholley) → review+
Comment on attachment 8467259 [details] [diff] [review]
bug_1033424_csp_json.patch

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

Yeah, this improves readability immensely.  +1 to bholley's suggestion using report.mCsp_report.mFoo.
Attachment #8467259 - Flags: review?(sstamm) → review+
> and do all of the assignments to report.mCsp_report.mFoo?

You can even do:

  dom::CSPReport report;
  dom::CSPReportProperties& reportProperties = report.mCsp_report;

and then leave the rest of the code as-is.  (I do think you should drop the "mozilla::" from these, because the whole file is "using namespace mozilla".)
And one more thing.  This block (post patch):

   if (!aSourceFile.IsEmpty()) {
     reportProperties.mSource_file = aSourceFile;
   }

can simply become:

   reportProperties.mSource_file = aSourceFile;

since you don't care if it's empty, right?  That's already the default value.

Or if you _do_ care, in that you don't want to output |"source-file": ""| in the JSON, then you need to remove the default value from the IDL and only construct the resulting Optional in the cases when you actually want to define that property of the report.

The same applies o the other things that are getting appended conditionally (script-sample, line-number).
Attached patch bug_1033424_webidl.patch (obsolete) — Splinter Review
Fixed nits and carrying over r+ from bholley.
Attachment #8467258 - Attachment is obsolete: true
Attachment #8467259 - Attachment is obsolete: true
Attached patch bug_1033424_csp_json.patch (obsolete) — Splinter Review
> +  mozilla::dom::CSPReport report;
> +  report.mCsp_report = reportProperties;

That is a great idea. Incorporated that suggestion of course.

>  dom::CSPReport report;
>  dom::CSPReportProperties& reportProperties = report.mCsp_report;
>
> and then leave the rest of the code as-is.  (I do think you should drop the "mozilla::" from these, because the whole file is "using namespace mozilla".)

I eliminated the lading mozilla:: namespace. Good catch!

> And one more thing.  This block (post patch):
>
>   if (!aSourceFile.IsEmpty()) {
>     reportProperties.mSource_file = aSourceFile;
>   }
>
> can simply become:
>
>   reportProperties.mSource_file = aSourceFile;

The initial version of CSP does only append values for source-file, script-sample, and line-number if the are non-null / != 0. It's fine with me that we always append those three values to the JSON, even if they are empty. (That happens for all of the inline stuff that calls the ::allows function family). But I let Sid decide on this one.

[Carrying over r+ from bholley] but giving Sid the chance to veto. Sid?
Attachment #8467537 - Flags: review?(sstamm)
Attached patch bug_1033424_tests.patch (obsolete) — Splinter Review
I rewrote test_csp_report to not only compare three properites of the csp-report (document-uri, blocked-uri, and violated-directive) but to compare all of the properties of a csp-report. The new tests also does not need to wait for a timeout in case something goes wrong within the try-catch of the "http-on-opening-request" observer. Further, it re-uses the more generic csp_test_server.sjs to serve the file which lets us remove file_csp_report.sjs. In addition, the JSON.stringify already throws an exception if something is not properly formatted in JSON. Looks like a win-win to me.

Sid, what do you think?
Attachment #8467539 - Flags: review?(sstamm)
>+  dom::CSPReportProperties reportProperties;
>+  dom::CSPReport report;
>+  report.mCsp_report = reportProperties;

This doesn't make any sense to me...  reportProperties is unused after this, no?
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #33)
> >+  dom::CSPReportProperties reportProperties;
> >+  dom::CSPReport report;
> >+  report.mCsp_report = reportProperties;
> 
> This doesn't make any sense to me...  reportProperties is unused after this,
> no?

Filling the values using for example
> report.mCsp_report.mBlocked_uri
where mBlocked_uri is a member of reportProperties and so forth. Seems to work. Would you rather see a different solution for that part of the code?
> report.mCsp_report.mBlocked_uri

That's using report.mCsp_report, not reportProperties.
Attached patch bug_1033424_csp_json.patch (obsolete) — Splinter Review
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #35)
> > report.mCsp_report.mBlocked_uri
> 
> That's using report.mCsp_report, not reportProperties.

Agreed, agreed - seems like I am too tired already :-( Time to go to bed.
I updated that real quick - here is the new patch - thanks for pointing that out Boris.
Attachment #8467537 - Attachment is obsolete: true
Attachment #8467537 - Flags: review?(sstamm)
Attachment #8467558 - Flags: review?(sstamm)
Comment on attachment 8467539 [details] [diff] [review]
bug_1033424_tests.patch

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

Please update the comment in the test file and put the JSON.parse call in its own try/catch block.  r=me with that.  

(Other comments in this file are optional, but if you think they're good ideas, tackle those too and flag me again.)

::: content/base/test/csp/test_csp_report.html
@@ +18,5 @@
> +
> +/*
> + * Description of the test:
> + * We try to load an inline-src using a policy that constrains
> + * all scripts from running (inline-src 'none'). We verify that

inline-src is not a directive, should be "without unsafe-inline in script-src".

@@ +21,5 @@
> + * We try to load an inline-src using a policy that constrains
> + * all scripts from running (inline-src 'none'). We verify that
> + * the generated csp-report contains the expceted values. If any
> + * of the JSON is not formatted properly (e.g. not properly escaped)
> + * then JSON.stringify will fail, which allows to pinpoit such errors

JSON.stringify isn't used, it's JSON.parse that will fail if the report is malformed.

@@ +60,5 @@
> +  is(cspReport["source-file"],
> +     "http://mochi.test:8888/tests/content/base/test/csp/file_csp_testserver.sjs" +
> +     "?file=tests/content/base/test/csp/file_csp_report.html" +
> +     "&csp=script-src%20%27none%27%3B%20report-uri%20http%3A//mochi.test%3A8888/foo.sjs",
> +     "Incorrect source-file");

Can we build this "source-file" path from constants/variables elsewhere in the file?  That way if we change the test this will get updated too.

@@ +69,5 @@
> +
> +  is(cspReport["line-number"],
> +    "7",
> +    "Incorrect line-number");
> +}

What if, instead of building all these manual checks, you just build a JS object and compare each JSON field?  e.g.:

var expectedReport = { 
  'csp-report': {
     'blocked-uri': "self",
      ... }};

for (x in ["param-a", "param-b" ...]) {
  is(reportObj['csp-report'][x], 
     expectedReport['csp-report'][x], 
     msg);
}

This would make the "expected values" easier to pick out by quick scan when reading this file.

@@ +116,4 @@
>        }
> +      catch(e) {
> +        ok(false, "Could not query report (exception: " + e + ")");
> +      }

Since you are updating this test to explicitly seek out malformed reports, please separate the JSON.parse() into its own try block and create a better test failure message to note this specific case.

@@ +139,5 @@
> +src += "?file=" + escape(testfile);
> +// append the CSP that should be used to serve the file
> +src += "&csp=" + escape(policy);
> +document.getElementById("cspframe").src = src;
> +

Why not just serve the file normally and use a file_csp_report.html^headers^ file?  Are we loading the file multiple times with different Content-Security-Policy headers?  Is it likely that we'll want to change the CSP applied to the test?

This .sjs technique is probably fine, but it will be harder to debug if something breaks this test.
Attachment #8467539 - Flags: review?(sstamm) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> The initial version of CSP does only append values for source-file,
> script-sample, and line-number if the are non-null / != 0. It's fine with me
> that we always append those three values to the JSON, even if they are
> empty. (That happens for all of the inline stuff that calls the ::allows
> function family). But I let Sid decide on this one.
> 
> [Carrying over r+ from bholley] but giving Sid the chance to veto. Sid?

Yes, given these fields are not mentioned in the spec and they're not always applicable (e.g., line-number doesn't make sense for frame-ancestors) I think we should omit them from the JSON blob when empty.  The document-uri, referrer, blocked-uri, violated-directive, and original-policy fields should always be present (even if empty).

http://www.w3.org/TR/CSP/#report-uri

It also probably makes sense to re-order the elements to line up with the spec's order so they're easy to find.
Comment on attachment 8467558 [details] [diff] [review]
bug_1033424_csp_json.patch

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

r=me with updates from my "veto" (see comment 38).

::: content/base/src/nsCSPContext.cpp
@@ +617,1 @@
>    }

Please update this if block so there's always a blocked-uri key in the JSON (even if empty).  The spec seems to want this.
http://www.w3.org/TR/CSP/#report-uri

@@ +644,3 @@
>  
>    // script-sample
> +  report.mCsp_report.mScript_sample = aScriptSample;

As I mentioned in comment 38, the "unspecified" fields should only be present if they're non-empty.  Please put the if statements back for those.
Attachment #8467558 - Flags: review?(sstamm) → review+
> It also probably makes sense to re-order the elements to line up with the
> spec's order so they're easy to find.

Reordered the elements so the line up with the spec and also removed the default value for:
+  DOMString source-file;
+  DOMString script-sample;
+  long line-number;
so they are not sent in the csp-report if not set explicitly.


[Carrying over r+ from bholley]
Attachment #8467535 - Attachment is obsolete: true
Attachment #8467539 - Attachment is obsolete: true
Attachment #8467558 - Attachment is obsolete: true
Attachment #8468200 - Flags: review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #39)
> Please update this if block so there's always a blocked-uri key in the JSON
> (even if empty).  The spec seems to want this.
> http://www.w3.org/TR/CSP/#report-uri

Since the blocked-uri has a default value in the *.webidl file, it will always be stringified and send with the JSON even if we do not set it ecplicitly to the empty-string. Hence, I think it's better to leave it where it is and only update mBlocked_Uri if not empty.

> @@ +644,3 @@
> >  
> >    // script-sample
> > +  report.mCsp_report.mScript_sample = aScriptSample;
> 
> As I mentioned in comment 38, the "unspecified" fields should only be
> present if they're non-empty.  Please put the if statements back for those.

I put the if-clauses for:
+  DOMString source-file;
+  DOMString script-sample;
+  long line-number;
back in the code, so those three fields are only present in the JSON if not empty.

[Carrying over r+ from bholley, sstamm]
Attachment #8468201 - Flags: review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #37)
--------------------------------------------------------------
> Please update the comment in the test file and put the JSON.parse call in
> its own try/catch block.  r=me with that.  

I updated the comment and also surrounded the JSON.parse with it's own try catch block.
Actually a great idea so that pinpoints a malformed JSON right away.


> Can we build this "source-file" path from constants/variables elsewhere in
> the file?  That way if we change the test this will get updated too.

Yes, cleaned that part up a bit.

> @@ +69,5 @@
> > +
> > +  is(cspReport["line-number"],
> > +    "7",
> > +    "Incorrect line-number");
> > +}
> 
> What if, instead of building all these manual checks, you just build a JS
> object and compare each JSON field? 
> ...
> This would make the "expected values" easier to pick out by quick scan when
> reading this file.

I actually feel the other way round - I think it's easier to read the way it is right now.

> Why not just serve the file normally and use a file_csp_report.html^headers^
> file?

No need to define an additonal ^header^ file.

> This .sjs technique is probably fine, but it will be harder to debug if
> something breaks this test.

Not sure why this would be harder to debug. In general, I think we should make use of testserver.sjs more often.
Easy to update and we need to define less ^header^ files.


[Carrying over r+ from sstamm]
The patches here can/should land together with the patch from bug 1048437. To make sure everything is fine, I pushed all of the patches together to TRY:

https://tbpl.mozilla.org/?tree=Try&rev=b3d2070a6c84
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3d2070a6c84
> It also probably makes sense to re-order the elements to line up with the spec's order 

Note that dictionaries always order their fields in alphabetical order.
Should have looked at this first:
  https://wiki.mozilla.org/Bugzilla:MozillaFlagsAndSuch

Adding a testcase which was reviewed by sstamm, so in-testsuite:+ is more appropriate.
Flags: in-testsuite? → in-testsuite+
Comment on attachment 8468200 [details] [diff] [review]
bug_1033424_webidl_v2.patch

Approval Request Comment
[Feature/regressing bug #]:

[User impact if declined]:
CSP reports are not encoded in proper JSON.

[Describe test coverage new/current, TBPL]:
Landed on inbound on 2014-08-06 09:44:43 PDT:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbd6ef4f882
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68224d6163e
https://hg.mozilla.org/integration/mozilla-inbound/rev/368852aa96c5

[Risks and why]:
Low risk because it only affects CSP reports.

[String/UUID change made/needed]: none
Attachment #8468200 - Flags: approval-mozilla-aurora?
Comment on attachment 8468201 [details] [diff] [review]
bug_1033424_csp_json_v2.patch

Approval Request Comment
[Feature/regressing bug #]:

[User impact if declined]:
CSP reports are not encoded in proper JSON.

[Describe test coverage new/current, TBPL]:
Landed on inbound on 2014-08-06 09:44:43 PDT:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbd6ef4f882
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68224d6163e
https://hg.mozilla.org/integration/mozilla-inbound/rev/368852aa96c5

[Risks and why]:
Low risk because it only affects CSP reports.

[String/UUID change made/needed]: none
Attachment #8468201 - Flags: approval-mozilla-aurora?
Comment on attachment 8468202 [details] [diff] [review]
bug_1033424_tests_v2.patch

Approval Request Comment
[Feature/regressing bug #]:

[User impact if declined]:
CSP reports are not encoded in proper JSON.

[Describe test coverage new/current, TBPL]:
Landed on inbound on 2014-08-06 09:44:43 PDT:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbd6ef4f882
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68224d6163e
https://hg.mozilla.org/integration/mozilla-inbound/rev/368852aa96c5

[Risks and why]:
Low risk because it only affects CSP reports.

[String/UUID change made/needed]: none
Attachment #8468202 - Flags: approval-mozilla-aurora?
Attachment #8468202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8468201 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8468200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: mwobensmith
Confirmed issue on Fx33, 2014-08-11.
Verified fixed on Fx33 and Fx34, 2014-08-22.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.