Closed Bug 988616 Opened 11 years ago Closed 10 years ago

Split CSP tests into separate files for prefixed and unprefixed header

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: grobinson, Assigned: grobinson)

References

Details

Attachments

(5 files, 13 obsolete files)

(deleted), patch
grobinson
: review+
Details | Diff | Splinter Review
(deleted), patch
grobinson
: review+
Details | Diff | Splinter Review
(deleted), patch
grobinson
: review+
Details | Diff | Splinter Review
(deleted), patch
geekboy
: review+
Details | Diff | Splinter Review
(deleted), patch
grobinson
: review+
Details | Diff | Splinter Review
In preparation for turning on the new CSP parser, which only supports CSP 1.0+ (the prefixed header), and removing the old one, we need to split many of the CSP tests into separate files, so each separate file tests either the prefixed header or the unprefixed header. This will maintain our test coverage and allow us to land and turn on the new parser carefully, and with the ability to easily revert to using the old parser in case of regressions.
Assignee: nobody → grobinson
This is a blocker for the new backend test suite. We also must complete the split before we remove x-csp support (so we can easily remove the x-csp tests easily).
Blocks: 993477, csp-legacy-removal
No longer blocks: 925004, 951457
Status: NEW → ASSIGNED
Depends on: 986077
When you split the tests, you can take advantage of the work in attachment 8421875 [details] [diff] [review] on bug 994320 (frame ancestors tests for new implementation). That patch includes some tweaks to the frame ancestors tests (to make them more accurate) and also removes x- header support.
Attached patch split_csp_tests (obsolete) (deleted) — Splinter Review
This patch splits the CSP mochitests into two directories: content/base/test/xcsp and content/base/test/csp (the current home of the mochitests). The goal is for all of the tests in test/csp to correctly test spec-compliant behavior, so when we are ready to remove support for X-CSP all we'll have to do is rm -rf test/xcsp and remove any references to it. All tests pass for me locally. Since I landed bug 986077 this morning, this patch probably doesn't apply cleanly and will need to be rebased. That should be easy, although care should be taken to migrate any shared platform changes from csp/mochitest.ini to xcsp/mochitest.ini. Most of the changes are straightforward. Tests that only tested CSP were left unchanged. Tests that only tested X-CSP were copied to test/xcsp unchanged, and efforts were made to adapt them to test CSP unless the features/bugs they were testing were specific to X-CSP (e.g. policy-uri). Tests that tested both X-CSP and CSP were split, with the X-CSP tests moved to test/xcsp and the CSP tests remaining in test/csp. A few things to note moving forward: * test_bug949549.html Tests the behavior of setRequestContext on a specific *interface*, Cc["@mozilla.org/contentsecuritypolicy;1"]. This is the same across X-CSP and CSP as implemented by the old parser, but the name of the interface is different for the new parser (it is Cc["@mozilla.org/cspcontext;1"]). We should probably change this in csprewrite's new_backend_mochitests. * test_policyuri_async_fetch.html This only tests the X-CSP header. policy-uri is supported by both X-CSP and CSP in the old parser, and is not supported by the new parser. It is not included in any of the 1.x specs. For this reason, I did not attempt to adapt the test for the CSP header. * test_policyuri_regression_from_multipolicy.html This only tests the CSP header. Unfortunately, it contradicts my previous decision for test_policyuri_async_fetch.html. I am leaving this unchanged for now, becuase it depends on whether we decide to implement policy-uri in the new parser or not. If we do, we should keep this (and adapt test_policyuri_async-fetch.html). If we do not, we should remove this test, probably as part of new_backend_mochitests.
Attachment #8423466 - Flags: feedback?(sstamm)
Attachment #8423466 - Flags: feedback?(mozilla)
test_bug949549.html: I don't quite follow what you're saying about test_bug949549.html. It doesn't test an interface. It tests an implementation of an interface. The "@mozilla..." strings are contract IDs that reference an interface/implementation pair. So both things referenced by the two contract IDs are different *implementations*, but both of the same interface: nsIContentSecurityPolicy. Are you suggesting we will need to update that file to the new contract ID when we install the new CSP backend? test_policyuri_async_fetch.html: hmmm... policy-uri is not supported as part of CSP 1.0. We could support it, though. Would it make sense to file a follow-up bug to update this test to use the new CSP header and backend? test_policyuri_regression_from_multipolicy.html: yeah, this file and the previously mentioned one should go together and we should port them over to use the standardized CSP header (with a non-standard directive) *only* if we decide to implement policy-uri. Maybe we should update this file to use the XCSP header?
> Are you suggesting we will need to update that file to the new contract ID when we install the new CSP backend? Yes, exactly. Sorry if that wasn't clear. > We could support it, though. I'm arguing that we need to discuss this. If no one is using it, and I don't think anyone is, it doesn't make sense to bear the maintenance burden. Filing the follow-up only makes sense it we do want to support policyuri going forward. > Maybe we should update this file to use the XCSP header? I considered doing that, but that contradicts the bug report that the test was created for. Probably isn't actually a problem, but it makes me uneasy.
Attached patch csp_tests.diff (obsolete) (deleted) — Splinter Review
I didn't use "hg mv" to prepare the split_csp_tests patch, and as a result it is very hard to read. I'll try to improve that situation tomorrow, but right now here's a file with all of the diffs between files copied from test/csp to test/xcsp, for easy comparison.
Attachment #8423466 - Flags: feedback?(sstamm)
Attachment #8423466 - Flags: feedback?(mozilla)
Blocks: 1011058
Attached patch xcsp_only_moves (obsolete) (deleted) — Splinter Review
Move files for tests that only test X-CSP from test/csp to test/xcsp.
Attachment #8423466 - Attachment is obsolete: true
Attachment #8423503 - Attachment is obsolete: true
Attachment #8431990 - Flags: review?(sstamm)
Attachment #8431990 - Flags: review?(mozilla)
Attached patch split_copies (obsolete) (deleted) — Splinter Review
"hg cp" files that that will be used in the "split" to test/xcsp. This makes the next patch readable.
Attachment #8431992 - Flags: review?(sstamm)
Attachment #8431992 - Flags: review?(mozilla)
Attached patch split (obsolete) (deleted) — Splinter Review
The actual tests, split. You will need to apply all 3 patches to build, since only this final patch contains the necessary changes to the build system's files (mochitest.ini, chrome.ini, and moz.build).
Attachment #8431994 - Flags: review?(sstamm)
Attachment #8431994 - Flags: review?(mozilla)
Garrett, it seems something is wrong with the first two of three attached patches: "No valid patch files were found in the attachment." Can you take a look and re-flag for review?
Comment on attachment 8431990 [details] [diff] [review] xcsp_only_moves Needs a commit message and updated user line.
Attachment #8431990 - Flags: review?(sstamm) → review-
Comment on attachment 8431992 [details] [diff] [review] split_copies Needs a commit message. I don't think this was properly exported either... not sure.
Attachment #8431992 - Flags: review?(sstamm) → review-
Comment on attachment 8431994 [details] [diff] [review] split Review of attachment 8431994 [details] [diff] [review]: ----------------------------------------------------------------- Most of this looks great. I'd like to see it again (in conjunction with an update on the other two patches). I anticipate it will be a quick second round. ::: content/base/test/csp/file_csp_redirects_main.html @@ +10,5 @@ > var thisSite = "http://mochi.test:8888"; > var otherSite = "http://example.com"; > var page = "/tests/content/base/test/csp/file_csp_redirects_page.sjs"; > > +var tests = { "font-src-spec-compliant": thisSite+page+"?testid=font-src-spec-compliant&csp=1&spec=1", Since this is changing the file in test/csp, "spec-compliant" doesn't make a whole lot of sense and will confuse readers after we remove x-csp support. Please s/-spec-compliant//g (probably in other files in this dir as well). ::: content/base/test/csp/test_CSP.html @@ +110,5 @@ > ["media.preload.default", 2]]}, > function() { > // save this for last so that our listeners are registered. > // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe').src = 'file_CSP_main_spec_compliant.html'; please rename this file to not have _spec_compliant. ::: content/base/test/csp/test_CSP_evalscript.html @@ +51,5 @@ > function() { > // save this for last so that our listeners are registered. > // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe').src = 'file_CSP_evalscript_main_spec_compliant.html'; > + document.getElementById('cspframe2').src = 'file_CSP_evalscript_main_spec_compliant_allowed.html'; Please rename these files to not be spec_compliant too (hg mv is probably fine, plus edits in this file and mochitest.ini) ::: content/base/test/csp/test_CSP_frameancestors.html @@ +112,5 @@ > {'set':[["security.csp.speccompliant", true]]}, > function() { > // save this for last so that our listeners are registered. > // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe').src = 'file_CSP_frameancestors_main_spec_compliant.html'; Here's another file to hg mv (to remove _spec_compliant) ::: content/base/test/csp/test_csp_bug773891.html @@ +1,4 @@ > <!DOCTYPE HTML> > <html> > <!-- > + https://bugzilla.mozilla.org/show_bug.cgi?id=773891 Good catch. Thanks! ::: content/base/test/xcsp/test_CSP_inlinescript.html @@ -76,5 @@ > if (inlineScriptsThatRan + inlineScriptsBlocked < inlineScriptsTotal) > return; > > - // The four scripts in the page with 'unsafe-inline' should run. > - is(inlineScriptsThatRan, 4, "there should be 4 inline scripts that ran"); Does it make sense to keep the "ran" tests?
Attachment #8431994 - Flags: review?(sstamm)
Attached patch 01-xcsp_only_moves (obsolete) (deleted) — Splinter Review
Fixed commit message and user. I think I forgot to check the "patch" box last time - let me know if you have difficult reviewing this time around.
Attachment #8431990 - Attachment is obsolete: true
Attachment #8431990 - Flags: review?(mozilla)
Attachment #8432782 - Flags: review?(sstamm)
Attachment #8432782 - Flags: review?(mozilla)
Attached patch 02-split_copies (obsolete) (deleted) — Splinter Review
Attachment #8431992 - Attachment is obsolete: true
Attachment #8431992 - Flags: review?(mozilla)
Attachment #8432784 - Flags: review?(sstamm)
Attachment #8432784 - Flags: review?(mozilla)
Attachment #8432784 - Attachment is patch: true
Attachment #8432784 - Attachment mime type: message/rfc822 → text/plain
Looks like Bugzilla/Splinter just can't display "hg mv" and "hg cp" nicely. The patches are valid and apply cleanly. Just look at the "Details" view to review them... :(
> Does it make sense to keep the "ran" tests? Nope, I'll remove that unused code in an updated patch.
Comment on attachment 8431994 [details] [diff] [review] split Review of attachment 8431994 [details] [diff] [review]: ----------------------------------------------------------------- Garrett, the general splitting looks good, as Sid already pointed out we should eliminate all "spec_compliant" postfixes. I marked some more. Other than that, looks good. Second round will be quick. ::: content/base/test/csp/test_CSP.html @@ +18,1 @@ > img_spec_compliant_good: -1, Sid was already nitpicking on the "spec_compliant" postfix in other files, please also remove the "spec_compliant" part from all of the test names here as well. ::: content/base/test/csp/test_CSP_evalscript_getCRMFRequest.html @@ +53,5 @@ > function() { > // save this for last so that our listeners are registered. > // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe').src = 'file_CSP_evalscript_main_spec_compliant_getCRMFRequest.html'; > + document.getElementById('cspframe2').src = 'file_CSP_evalscript_main_spec_compliant_allowed_getCRMFRequest.html'; Same nitpick: Remove spec_compliant from the testname. ::: content/base/test/csp/test_CSP_frameancestors.html @@ +18,2 @@ > aa_allow_spec_compliant: -1, /* innermost frame allows a * > //aa_block_spec_compliant: -1, /* innermost frame denies a */ Nothing to do with the split (besides the spec_compliant portion of the name), but do you have any idea why those are commented? If they are not used, should we remove them from the test? ::: content/base/test/csp/test_CSP_inlinescript.html @@ +105,5 @@ > {'set':[["security.csp.speccompliant", true]]}, > function() { > // save this for last so that our listeners are registered. > // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe1').src = 'file_CSP_inlinescript_main_spec_compliant.html'; Another spec_compliant. ::: content/base/test/csp/test_csp_redirects.html @@ +72,1 @@ > "font-src-redir-spec-compliant": false, As in other files, remove spec-compliant postfix.
Attachment #8431994 - Flags: review?(mozilla) → review-
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18) > Garrett, the general splitting looks good, as Sid already pointed out we > should eliminate all "spec_compliant" postfixes. I marked some more. Other > than that, looks good. Second round will be quick. Cool. I made a patch to remove all of the spec_compliant stuff yesterday, then accidentally ruined it trying to do some shenanigans with mq :( Will re-do it and post ASAP. > ::: content/base/test/csp/test_CSP_frameancestors.html > @@ +18,2 @@ > > aa_allow_spec_compliant: -1, /* innermost frame allows a * > > //aa_block_spec_compliant: -1, /* innermost frame denies a */ > > Nothing to do with the split (besides the spec_compliant portion of the > name), but do you have any idea why those are commented? If they are not > used, should we remove them from the test? I don't know why those are commented out, and am not familiar with that test generally. imelven (or sstamm), do you know? If you want to remove them from the test, we should do it in a follow-up. I don't want to get into the weeds of cleaning up all of the warts on these tests in this bug.
Flags: needinfo?(ian.melven)
Comment on attachment 8432784 [details] [diff] [review] 02-split_copies I applied your patches and looked through them one by one, the most important thing for me is that we are not going to delete any testcoverage. While all of what you did looks good to me, I was wondering if we want to also move/convert the following three tests and keep them in the /csp folder: *test_bothCSPheaders.html *test_dual_headers_warning *test_policyuri_async_fetch I know that we can't simply move the files over to /csp, but most certainly web developers will continue using XCSP headers. Kepping/having some tests that print output to the console alerting them that XCSP is deprecated is something we should keep in my opinion. What do you think?
Attachment #8432784 - Flags: review?(mozilla) → review+
Comment on attachment 8432782 [details] [diff] [review] 01-xcsp_only_moves As mentioned in the review above, I applied all your patches and looks good to me. Wuhu, time to flip the pref :-)
Attachment #8432782 - Flags: review?(mozilla) → review+
(In reply to Garrett Robinson [:grobinson] from comment #19) > I don't know why those are commented out, and am not familiar with that test > generally. imelven (or sstamm), do you know? They're listed in the structure because the tests run. They're commented out because when CSP blocks the thing, the csp-on-violate-policy observers don't get to know which test was violated. We could probably rewrite the test to be more precise, but this test just counts the number of violations and assumes the count will be equal to the number of things commented out in that structure. Maybe we fix in a follow-up, probably not appropriate here. Don't remove the commented lines, please, so we know what runs.
Flags: needinfo?(ian.melven)
Do we need to do anything with these files yet? I'm guessing no, but would like your thoughts Garrett. /browser/devtools/webconsole/test/test-bug-821877-csperrors.html /browser/devtools/webconsole/test/browser_webconsole_bug_821877_csp_errors.js /browser/devtools/webconsole/test/test-bug-821877-csperrors.html^headers^ /browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js /browser/devtools/webconsole/test/test_bug_1010953_cspro.html /browser/devtools/webconsole/test/test_bug_1010953_cspro.html^headers^ /dom/workers/test/csp_worker.js /dom/workers/test/test_csp.html /dom/workers/test/test_csp.html^headers^ /dom/workers/test/test_csp.js /content/base/test/unit/test_csputils.js /content/base/test/unit/test_cspreports.js /content/base/test/unit/test_csp_ignores_path.js
Flags: needinfo?(grobinson)
There's also: /content/base/test/unit/test_bug558431.js /browser/components/sessionstore/test/browser_911547.js /browser/components/sessionstore/test/browser_911547_sample.html /browser/components/sessionstore/test/browser_911547_sample.html^headers^ /browser/devtools/webconsole/test/test_bug_770099_violation.html /browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js /browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js /browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html /browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html^headers^ /browser/devtools/webconsole/test/test_bug_770099_violation.html^headers^ I don't think we need to do anything with these until we remove the x-header support (maybe convert them to use the spec compliant header in the remove-x-header bug), but want your opinion.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20) > I know that we can't simply move the files over to /csp, but most certainly > web developers will continue using XCSP headers. Kepping/having some tests > that print output to the console alerting them that XCSP is deprecated is > something we should keep in my opinion. What do you think? I don't think having mochitests/unittests that spit out errors about XCSP will be helpful moving forward. We *could* look at dumping warnings in the web console that XCSP support was removed, but I'm not even sure how useful that is given the deprecation warnings we've been spitting out for about 10 releases now.
(In reply to Garrett Robinson [:grobinson] from comment #17) > > Does it make sense to keep the "ran" tests? > > Nope, I'll remove that unused code in an updated patch. Just in the xcsp tests, right? 'cause we need to make sure that still works in the csp tests ('unsafe-inline' needs test coverage).
Attached patch 03-split (obsolete) (deleted) — Splinter Review
Removes unused code as described in Comment 17. Removing the "spec-compliant" wording is deferred to follow up patches for ease of review.
Attachment #8431994 - Attachment is obsolete: true
Attachment #8433527 - Flags: review?(sstamm)
Attachment #8433527 - Flags: review?(mozilla)
Attached patch 04-rename_spec_compliant_test_files (obsolete) (deleted) — Splinter Review
Rename files in test/csp that have "spec_compliant" in the filename.
Attachment #8433529 - Flags: review?(sstamm)
Attachment #8433529 - Flags: review?(mozilla)
Attached patch 05-remove_refs_to_spec_compliant (obsolete) (deleted) — Splinter Review
Remove references to "spec.*compliant" in variables, etc. in the test/csp files, except for setting the "security.csp.speccompliant" pref, which we cannot remove until we remove X-CSP support completely.
Attachment #8433532 - Flags: review?(sstamm)
Attachment #8433532 - Flags: review?(mozilla)
Attachment #8433529 - Attachment is patch: true
Attachment #8433529 - Attachment mime type: message/rfc822 → text/plain
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #26) > (In reply to Garrett Robinson [:grobinson] from comment #17) > > > Does it make sense to keep the "ran" tests? > > > > Nope, I'll remove that unused code in an updated patch. > > Just in the xcsp tests, right? Yes (see patch just posted).
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20) > I was wondering if we want to also move/convert the following three tests > and keep them in the /csp folder: > > *test_bothCSPheaders.html > *test_dual_headers_warning No. Once we remove X-CSP, we should remove all code related to it, including the behavior (in nsDocument::InitCSP) tested by these two tests. Web devs have had long enough to stop using X-CSP, it has been deprecated w/ a warning since Firefox 23. Note that once we land this, we will do a blog post on hacks.mozilla.org or similar to get the word out (that will be at the beginning of the release cycle, so developers will have plenty of time to test and update as it rides the trains). > *test_policyuri_async_fetch See above comments about policyuri. This feature is not part of CSP 1.0 or 1.1, and we are the only implementation that supports it. The new CSP backend does not support it. I think we should drop support completely and stick to the spec, save some maintenance burden for a feature that no one is using.
Comment on attachment 8432782 [details] [diff] [review] 01-xcsp_only_moves Commit message needs bug number and such, but looks good to me.
Attachment #8432782 - Flags: review?(sstamm) → review+
Comment on attachment 8432784 [details] [diff] [review] 02-split_copies Looks good. Needs bug number and such in the commit message.
Attachment #8432784 - Flags: review?(sstamm) → review+
Attached patch 04-rename_spec_compliant_test_files (obsolete) (deleted) — Splinter Review
Manually edited the patch to fix a merge issue: "cannot create content/base/test/csp/file_CSP_evalscript_main_getCRMFRequest.html: destination already exists". Looks like hg rm -> hg mv (to the rm'ed filename) might have issues either with hg or mq.
Attachment #8433529 - Attachment is obsolete: true
Attachment #8433529 - Flags: review?(sstamm)
Attachment #8433529 - Flags: review?(mozilla)
Attachment #8433657 - Flags: review?(sstamm)
Attachment #8433657 - Flags: review?(mozilla)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24) > I don't think we need to do anything with these until we remove the x-header > support (maybe convert them to use the spec compliant header in the > remove-x-header bug), but want your opinion. > browser/devtools/webconsole/test/test-bug-821877-csperrors.html > browser/devtools/webconsole/test/test-bug-821877-csperrors.html^headers^ > browser/devtools/webconsole/test/browser_webconsole_bug_821877_csp_errors.js These should be removed when we remove X-CSP, because we no longer want to display "X-CSP is deprecated" once it's been removed. > browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js > browser/devtools/webconsole/test/test_bug_1010953_cspro.html > browser/devtools/webconsole/test/test_bug_1010953_cspro.html^headers^ No changes needed here. > dom/workers/test/csp_worker.js > dom/workers/test/test_csp.html > dom/workers/test/test_csp.html^headers^ > dom/workers/test/test_csp.js This only tests X-CSP, and should be updated to test CSP. More importantly, this is a bad test and should be completely rewritten (see bug 929292, comment 7). > content/base/test/unit/test_csputils.js This is testing the implementation of CSPUtils.jsm. We cannot use it as-is with the new parser because the implementation is all C++. However, there is a lot of good test coverage here, and we should consider filing a bug to duplicate it for the new parser. > content/base/test/unit/test_cspreports.js This looks like it could actually be adapted fairly easily to test the new implementation. It only uses methods from the nsIContentSecurityPolicy interface, which is implemented identically by the new implementation. > content/base/test/unit/test_csp_ignores_path.js Same as test_csputils.js: we can't use this test as-is, but should endeavor to keep its coverage for the new parser. > content/base/test/unit/test_bug558431.js This tests policy-uri, which we are not going to support once we remove the old implementation(s). It can be safely removed. > browser/components/sessionstore/test/browser_911547.js > browser/components/sessionstore/test/browser_911547_sample.html > browser/components/sessionstore/test/browser_911547_sample.html^headers^ Definitely want to keep this coverage as the bug was a bypass. This looks like it depends on our successful implementation of nsISerializable, so I think it should work as-is for the new implementation. It tests behavior, not implementation specifics. > browser/devtools/webconsole/test/test_bug_770099_violation.html > browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js > browser/devtools/webconsole/test/test_bug_770099_violation.html^headers^ Tests X-CSP only. This bug tests that CSP violation messages appear in the Web Console, so we should keep it and just update the header to CSP. > browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js > browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html > browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html^headers^ Also tests policy-uri, so it can be safely removed. I think there might be more CSP tests in browser/devtools that we haven't considered yet.
Flags: needinfo?(grobinson)
Comment on attachment 8433527 [details] [diff] [review] 03-split Review of attachment 8433527 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this looks good. I ran tests locally and they seemed to pass with new and old backends with one exception: if you try to run the tests in content/base/test/csp with the new backend (by flipping the pref) you'll get failures in test_csp_regexp_parsing.html; chris is fixing those failures in bug 1019984, and they should not affect landing these test changes since the new backend is not enabled by default.
Attachment #8433527 - Flags: review?(sstamm) → review+
Attachment #8433532 - Flags: review?(sstamm) → review+
Attachment #8433657 - Flags: review?(sstamm) → review+
So this should be ready to go once: 1. r+ from ckerschb on everything 2. to be safe you should run it through try first to make sure there are no unexpected side-effects on B2G (maybe it can't find tests to disable or something). 3. we land all the patches at the same time
We should land 1019984 before we land the test splitting, so all the tests pass once the splitting lands.
Depends on: 1019984
(In reply to Garrett Robinson [:grobinson] from comment #35) > > content/base/test/unit/test_csputils.js > > This is testing the implementation of CSPUtils.jsm. We cannot use it as-is > with the new parser because the implementation is all C++. However, there is > a lot of good test coverage here, and we should consider filing a bug to > duplicate it for the new parser. I wrote a script that extracts all testcases from test_csputils.js and integrated them into the compiled code tests [1]. A 1:1 mapping of those tests isn't possible, but I was able to extract all the different policies which became part of the C++ parser testsuite. We can certainly extend if you think we need more test coverage. Keep in mind that we also got the fuzzy tests in TestCSPParser which also cover a lot of unexpected input. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestCSPParser.cpp#473
(In reply to Garrett Robinson [:grobinson] from comment #35) > (In reply to Sid Stamm [:geekboy or :sstamm] from comment #24) > > content/base/test/unit/test_csp_ignores_path.js > > Same as test_csputils.js: we can't use this test as-is, but should endeavor > to keep its coverage for the new parser. Indeed, we should keep those around, especially since we already invested the time in crafting those. I filed bug 1020477 to do so.
Comment on attachment 8433527 [details] [diff] [review] 03-split Review of attachment 8433527 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #8433527 - Flags: review?(mozilla) → review+
Comment on attachment 8433532 [details] [diff] [review] 05-remove_refs_to_spec_compliant Review of attachment 8433532 [details] [diff] [review]: ----------------------------------------------------------------- Let's rock this thing.
Attachment #8433532 - Flags: review?(mozilla) → review+
Comment on attachment 8433657 [details] [diff] [review] 04-rename_spec_compliant_test_files Review of attachment 8433657 [details] [diff] [review]: ----------------------------------------------------------------- Solid! Please make sure you update all your commit messages to include bug number, reviewer, etc once you export those patches for landing.
Attachment #8433657 - Flags: review?(mozilla) → review+
Attached patch 01-xcsp_only_moves (deleted) — Splinter Review
Updated patch with final commit message
Attachment #8432782 - Attachment is obsolete: true
Attachment #8434500 - Flags: review+
Attached patch 02-split_copies (deleted) — Splinter Review
Attachment #8432784 - Attachment is obsolete: true
Attachment #8434502 - Flags: review+
Attached patch 03-split (obsolete) (deleted) — Splinter Review
Attachment #8433527 - Attachment is obsolete: true
Attachment #8434503 - Flags: review+
Attached patch 04-rename_spec_compliant_test_files (obsolete) (deleted) — Splinter Review
Attachment #8433657 - Attachment is obsolete: true
Attachment #8434504 - Flags: review+
Attachment #8433532 - Attachment is obsolete: true
Attachment #8434505 - Flags: review+
This caused breakage on inbound: * test_30*_redirect.html are timing out: https://tbpl.mozilla.org/php/getParsedLog.php?id=41070153&tree=Mozilla-Inbound I think I have a fix for this. Unfortunately, we also appear to be causing * android's mochitest-8: https://tbpl.mozilla.org/php/getParsedLog.php?id=41070424&tree=Mozilla-Inbound So I am going to back out the patches.
It's entirely possible that these tests just depend on the state left behind from previous tests, and moving things around changed which chunks they're run in.
The redirect tests (at least) may be an intermittent orange (bug 1011211) that this change made permaorange. What's your fix, grobinson? It would really be unfortunate if KWierso is right about the android tests. :(
Fixed the redirect tests in bug 1011211, and the Android tests were fixed in bug 1021644. Let's *try* this again: https://tbpl.mozilla.org/?tree=Try&rev=264b4546eec5
Depends on: 1011211, 1021644
Comment on attachment 8434503 [details] [diff] [review] 03-split Review of attachment 8434503 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/csp/file_redirect_content.sjs @@ +24,5 @@ > } > > var csp = "default-src \'self\';report-uri http://mochi.test:8888/tests/content/base/test/csp/file_redirect_report.sjs?" + redirect; > > + response.setHeader("Content-Security-Policy", csp, false); Because of this change we'll have to make sure that all the tests that depend on this use the spec-compliant implementation. That means pushPrefEnv: +SpecialPowers.pushPrefEnv( + {'set':[["security.csp.speccompliant", true]]}, This is 'cause we're not using the speccompliant parser by default on b2g yet. Files to change: all content/base/test/csp/test_30*_redirect.html in this patch
Attachment #8434503 - Flags: review+
Comment on attachment 8434503 [details] [diff] [review] 03-split Review of attachment 8434503 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/csp/file_csp_report.sjs @@ +14,5 @@ > response.setHeader("Cache-Control", "no-cache", false); > > // set CSP header > + response.setHeader("Content-Security-Policy", > + "default-src 'self'; report-uri http://mochi.test:8888/csp-report.cgi", Anything that depends on this sjs file will need to pushprefenv security.csp.speccompliant to true so this passes on b2g as well. ::: content/base/test/csp/file_multi_policy_injection_bypass.html^headers^ @@ +1,1 @@ > +Content-Security-Policy: default-src 'self', default-src * Anything that depends on this sjs file will need to pushprefenv security.csp.speccompliant to true so this passes on b2g as well. ::: content/base/test/csp/file_multi_policy_injection_bypass_2.html^headers^ @@ +1,1 @@ > +X-Content-Security-Policy: default-src 'self' , default-src * Anything that depends on this sjs file will need to pushprefenv security.csp.speccompliant to true so this passes on b2g as well. This should also be Content-Security-Policy, not X-Content... ::: content/base/test/csp/file_subframe_run_js_if_allowed.html^headers^ @@ +1,1 @@ > +Content-Security-Policy: default-src *; script-src 'unsafe-inline' Anything that depends on this sjs file will need to pushprefenv security.csp.speccompliant to true so this passes on b2g as well.
and of course in the previous comment, some of the files I called "this sjs" were ^headers^ files not sjs... copy paste error, sorry. Files that probably need updating: test_csp_report.html test_multi_policy_injection_bypass.html test_subframe_run_js_if_allowed.html test_30*_redirect.html
Attached patch 03-split (deleted) — Splinter Review
Make sure every test in test/csp sets the "security.csp.speccompliant" flag for B2G, which doesn't have it set by default.
Attachment #8434503 - Attachment is obsolete: true
Attachment #8436135 - Flags: review?(sstamm)
Fix merge conflict due to changes to 03-split. Carrying over r+ since the changes are minimal.
Attachment #8434504 - Attachment is obsolete: true
Attachment #8436137 - Flags: review+
Attachment #8436135 - Attachment is patch: true
Attachment #8436135 - Attachment mime type: message/rfc822 → text/plain
Attachment #8436137 - Attachment is patch: true
Attachment #8436137 - Attachment mime type: message/rfc822 → text/plain
Comment on attachment 8436135 [details] [diff] [review] 03-split Review of attachment 8436135 [details] [diff] [review]: ----------------------------------------------------------------- I *think* you got them all. If try goes green, this looks good. Should be enabling the speccompliant parser on b2g shortly (bug 858787).
Attachment #8436135 - Flags: review?(sstamm) → review+
Depends on: 1023472
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: