Closed
Bug 1033423
Opened 10 years ago
Closed 10 years ago
CSP reports do not correctly escape newline characters
Categories
(Core :: Security, defect)
Tracking
()
VERIFIED
FIXED
mozilla34
People
(Reporter: mgoodwin, Assigned: ckerschb)
References
(Blocks 1 open bug, )
Details
Attachments
(3 files, 9 obsolete files)
1.72 KB,
patch
|
ckerschb
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.92 KB,
patch
|
ckerschb
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
ckerschb
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
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.
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
(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?
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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-
Assignee | ||
Comment 7•10 years ago
|
||
(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 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
(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 10•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → mozilla
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
(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)
Comment 13•10 years ago
|
||
> 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)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
> 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...
Comment 17•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
> 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.
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Comment 21•10 years ago
|
||
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++.
Assignee | ||
Comment 22•10 years ago
|
||
(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!
Assignee | ||
Comment 23•10 years ago
|
||
I applied the patch provided by bz so that webidl names can make use of "-". Works just fine.
Attachment #8467258 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8464465 -
Attachment is obsolete: true
Attachment #8467259 -
Flags: review?(sstamm)
Attachment #8467259 -
Flags: review?(bobbyholley)
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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 27•10 years ago
|
||
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+
Comment 28•10 years ago
|
||
> 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".)
Comment 29•10 years ago
|
||
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).
Assignee | ||
Comment 30•10 years ago
|
||
Fixed nits and carrying over r+ from bholley.
Attachment #8467258 -
Attachment is obsolete: true
Attachment #8467259 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8467535 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
> + 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)
Assignee | ||
Comment 32•10 years ago
|
||
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)
Comment 33•10 years ago
|
||
>+ dom::CSPReportProperties reportProperties;
>+ dom::CSPReport report;
>+ report.mCsp_report = reportProperties;
This doesn't make any sense to me... reportProperties is unused after this, no?
Assignee | ||
Comment 34•10 years ago
|
||
(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?
Comment 35•10 years ago
|
||
> report.mCsp_report.mBlocked_uri
That's using report.mCsp_report, not reportProperties.
Assignee | ||
Comment 36•10 years ago
|
||
(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 37•10 years ago
|
||
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+
Comment 38•10 years ago
|
||
(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 39•10 years ago
|
||
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+
Assignee | ||
Comment 40•10 years ago
|
||
> 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+
Assignee | ||
Comment 41•10 years ago
|
||
(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+
Assignee | ||
Comment 42•10 years ago
|
||
(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]
Assignee | ||
Updated•10 years ago
|
Attachment #8468202 -
Flags: review+
Assignee | ||
Comment 43•10 years ago
|
||
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
Comment 44•10 years ago
|
||
> 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.
Assignee | ||
Comment 45•10 years ago
|
||
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
Flags: in-testsuite?
Target Milestone: --- → mozilla34
Assignee | ||
Comment 46•10 years ago
|
||
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 47•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7fbd6ef4f882 https://hg.mozilla.org/mozilla-central/rev/e68224d6163e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 48•10 years ago
|
||
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?
Assignee | ||
Comment 49•10 years ago
|
||
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?
Assignee | ||
Comment 50•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8468202 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 51•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #45) > https://hg.mozilla.org/integration/mozilla-inbound/rev/368852aa96c5 This cset landed with the wrong bug number in the commit message. Please double-check that before pushing in the future. https://hg.mozilla.org/releases/mozilla-aurora/rev/99b65bd83b4a https://hg.mozilla.org/releases/mozilla-aurora/rev/11df11373975 https://hg.mozilla.org/releases/mozilla-aurora/rev/1373e74fff1a
status-firefox33:
--- → fixed
status-firefox34:
--- → fixed
Updated•10 years ago
|
Attachment #8468201 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8468200 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
QA Contact: mwobensmith
Comment 52•10 years ago
|
||
Confirmed issue on Fx33, 2014-08-11. Verified fixed on Fx33 and Fx34, 2014-08-22.
You need to log in
before you can comment on or make changes to this bug.
Description
•