It seems the current CSP testsuite is not covering all the different corner cases. We should extend the testsuite and test in particular: * Report-only mode, and also several policies (some report only, some note) * Show that X-Content-Security policy is not supported any longer - check for console messages. * Show that only the first occurence of a directive is enforced and following are ignored (e.g. if script-src is occurs several times in a policy; check for warning) * show that 'none' is ignored in case the is a valid src for that directive * ... I guess we can always add more tests here as we go further along with the implementation.

Hey Garrett, I assembled a new testsuite; which we can use for other CSP tests as well from now on. I don't know whether this should be feedback or a review request. I flag it as a review for now. If something is missing, let me know, but I guess I should have covered all the different needs. Let me know what you think. Thanks!

Comment on attachment 8405520 [details] [diff] [review] bug_993477_testsuite_for_csp_in_cpp.patch Review of attachment 8405520 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming you intend to write more tests in test_csp_in_cpp_basics.html. ::: content/base/test/csp/file_csp_in_cpp_basics.js @@ +1,1 @@ > +document.getElementById("externalScriptDiv").innerHTML = "allowed"; Why do this instead of using the http-on-modify-request observer to detect if an external script loaded? Unlike this technique, that can be extended to other load types. ::: content/base/test/csp/file_csp_testharness.js @@ +5,5 @@ > + * Create an array of testObjects which specify the following properties: > + * > + * * description = a short description of the test > + * * cspHeader1 = the CSP-header to be used when serving the testfile > + * * policy1 = the CSP-policy to be used when serving the testfile What's the difference between header and policy? Is cspHeader {Content-Security-Policy,Content-Security-Policy-Report-Only}, and policy is the header? Why cspHeader1, cspHeader2, and policy1, policy2 instead of an Array of headers? (which can just be strings) @@ +10,5 @@ > + * * testfile = the testfile to be used > + * * loads = (optinal) JS-Object defining loads which should be allowed/denied; > + * to identify loads, the TestHarness extracts the "testid=" > + * from every URL reqeust, which needs to be defined. > + * * divs = (optional) JS-Object which allows to sepcify div containers with exptected Maybe clarify that these are different testing strategies for external vs. inline behavior? e.g. s/loads/external_loads and s/divs/inline_divs, or explain it in the comment. @@ +72,5 @@ > +} > + > +/* === Keep track of current test === */ > +var tests; > +// init the counter to -1 because runNextTest icrements before indexing s/icrements/increments @@ +73,5 @@ > + > +/* === Keep track of current test === */ > +var tests; > +// init the counter to -1 because runNextTest icrements before indexing > +var counter = -1; Why don't we just set this to 0, and increment after runNextTest? @@ +75,5 @@ > +var tests; > +// init the counter to -1 because runNextTest icrements before indexing > +var counter = -1; > +// we need to init the console Msg because the listener starts > +// listening before any tests are actually running */ Nit: don't mix line and block comment styles. @@ +121,5 @@ > + debug("checkReports, data", data); > + // Parse the report using JSON and verify that the properties have expected values. > + var reportText = "{}"; > + try { > + var uploadStream = SpecialPowers.wrap(SpecialPowers.do_QueryInterface(subject, "nsIUploadChannel")).uploadStream; FYI: this is not going to work with multiprocess. We'll need to figure out what to do here (maybe a cross-process proxy via SpecialPowers, similar to how we handle http-on-modify-request). @@ +217,5 @@ > + var asciiSpec = subject; > + try { > + asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec"); > + } > + catch (e) { Formatting is all messed up here. ::: content/base/test/csp/file_csp_testharness_server.sjs @@ +38,5 @@ > + var csp1 = unescape(query['csp1']); > + > + // get the second csp policy > + var cspheader2 = unescape(query['cspheader2']) > + var csp2 = unescape(query['csp2']); Nit: remove all trailing whitespace @@ +51,5 @@ > + response.setHeader(cspheader1, csp1, false); > + > + // if a second policy available, set it as a header > + if (cspheader2 != "") { > + response.setHeader(cspheader2, csp2, false); Are you sure you want the merge argument to be false? Won't this replace cspheader1 with cspheader2? See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel#setResponseHeader%28%29

This patch is intended to provide a general testing framework for CSP. We can use and activate it once we implement new CSP 1.1 features. Removing dependency so we can flip the pref to use the new backend.

We have a lot of CSP tests, and I'm going to close this because our coverage has grown immensely for CSP and this is a monumental task to rewrite all of them.