Since we already have written all those tests in test/unit/test_csp_ignores_path.js which verify correct handling of paths, we should copy them into TestCSPParser.cpp. Only this morning, we already encountered a problem with path handling, so I think, the more tests the better, especially because we already invested the time in writing those tests.

Converted the tests from test_csp_ignores_path.js and added them to the compiled code tests. Probably the patch gets bitroted, so waiting with setting review flags.

Comment on attachment 8434407 [details] [diff] [review] bug_1020477_wip.patch Don't get confused because it's a WIP patch - I will make sure it applies cleanly once 1019984 landed and you are done with your review.

Comment on attachment 8434407 [details] [diff] [review] bug_1020477_wip.patch Review of attachment 8434407 [details] [diff] [review]: ----------------------------------------------------------------- Overall, lgtm. just a few nits. ::: content/base/test/TestCSPParser.cpp @@ +692,5 @@ > } > > +// ============ TestGoodGeneratedPoliciesForPathHandling ============ > + > +nsresult TestGoodGeneratedPoliciesForPathHandling() { Maybe add a comment that says we need to update these when we implement paths for csp 1.1 (bug 808292). @@ +820,5 @@ > + // The following tests were converted from test_csp_ignores_path.js > + { "img-src test1.example.com:88path-1/", "" }, > + { "img-src test1.example.com:80.js", "" }, > + { "img-src test1.example.com:*.js", "" }, > + { "img-src test1.example.com:*." "" }, Please add a few more with schemes. And since you have TestGoodGeneratedPoliciesForPathHandling for the valid tests, why not add another function for invalid paths?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3) > Comment on attachment 8434407 [details] [diff] [review] > ::: content/base/test/TestCSPParser.cpp > @@ +692,5 @@ > > } > > > > +// ============ TestGoodGeneratedPoliciesForPathHandling ============ > > + > > +nsresult TestGoodGeneratedPoliciesForPathHandling() { > > Maybe add a comment that says we need to update these when we implement > paths for csp 1.1 (bug 808292). In fact those tests do not need to be changed, because compiled code tests in TestCSPParser can only test that the parser correctly parses the policy including eventual paths after host[port]. For CSP 1.1 we need to implement an enforcement of paths which we will add to cspUtils.cpp which then need mochitests to verify correct blocking. > > @@ +820,5 @@ > > + // The following tests were converted from test_csp_ignores_path.js > > + { "img-src test1.example.com:88path-1/", "" }, > > + { "img-src test1.example.com:80.js", "" }, > > + { "img-src test1.example.com:*.js", "" }, > > + { "img-src test1.example.com:*." "" }, > > Please add a few more with schemes. > > And since you have TestGoodGeneratedPoliciesForPathHandling for the valid > tests, why not add another function for invalid paths? I manually added all the testcases that I can think of (including schemes and ports) that test that the parser can handle all of the different "path-scenarios". And since you requested it, I put them all in their own function, which actually makes sense. Thanks!

As discussed in person, we have to update the expected output for policies to include a path. Wasn't clear from the review feedback - I added a comment on top of the function including the bug number.

Comment on attachment 8434555 [details] [diff] [review] bug_1020477_v3.patch Review of attachment 8434555 [details] [diff] [review]: ----------------------------------------------------------------- Figure out what to do with // at the end of the paths (file new bug to fix it or show me wrong) and r=me. ::: content/base/test/TestCSPParser.cpp @@ +841,5 @@ > + { "img-src https://test1.example.com/abc/def/ghi//", "" }, > + { "img-src http://test1.example.com:80//", "" }, > + { "img-src http://test1.example.com:80/abc//", "" }, > + { "img-src https://test1.example.com:80/abc/def//", "" }, > + { "img-src https://test1.example.com:80/abc/def/ghi//", "" }, I think the paths ending with // are still valid productions from RFC3986 3.3 and don't think the parser should reject them. http://tools.ietf.org/html/rfc3986#section-3.3 That RFC says an absolute path can't *start* with //, but doesn't rule out // in the path after the first segment (only the first segment is "segment-nz", subsequent segments can have zero length). If the parser fails on these, we probably aren't parsing paths properly and should fix it (another bug).

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6) > Comment on attachment 8434555 [details] [diff] [review] > bug_1020477_v3.patch > > Review of attachment 8434555 [details] [diff] [review]: > ----------------------------------------------------------------- > > Figure out what to do with // at the end of the paths (file new bug to fix > it or show me wrong) and r=me. > > ::: content/base/test/TestCSPParser.cpp > @@ +841,5 @@ > > + { "img-src https://test1.example.com/abc/def/ghi//", "" }, > > + { "img-src http://test1.example.com:80//", "" }, > > + { "img-src http://test1.example.com:80/abc//", "" }, > > + { "img-src https://test1.example.com:80/abc/def//", "" }, > > + { "img-src https://test1.example.com:80/abc/def/ghi//", "" }, > > I think the paths ending with // are still valid productions from RFC3986 > 3.3 and don't think the parser should reject them. > http://tools.ietf.org/html/rfc3986#section-3.3 > > That RFC says an absolute path can't *start* with //, but doesn't rule out > // in the path after the first segment (only the first segment is > "segment-nz", subsequent segments can have zero length). > Thanks Sid for bringing this up again. As discussed in person yesterday our csp-parser should accept // in the middle of a path and also at the end of a path to be compliant with the general URI parser we are using. I think it’s acceptable to make that change within that patch and bug. Two tests had to be moved from failing tests to passing tests due to the change in the parser. I also crafted additional testcases to make sure we are handling paths including /, // and n*/ correctly now. The patch now not only affects the tests but also the parser. Even though it’s a very minimal change I would like you to review it again.

Comment on attachment 8435232 [details] [diff] [review] bug_1020477_including_parser_update.patch Review of attachment 8435232 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Parser change makes sense: subpath segments can be empty. r=me assuming you've verified all of TestCSPParser.cpp tests pass with the parser change.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8) > Comment on attachment 8435232 [details] [diff] [review] > bug_1020477_including_parser_update.patch > > Review of attachment 8435232 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Parser change makes sense: subpath segments can be empty. r=me > assuming you've verified all of TestCSPParser.cpp tests pass with the parser > change. Verified TestCSPParser.cpp tests are passing and also csp_regexp_parsing is passing. The patch is ready to land. Do you wanna land it?

The code in this patch is going to live behind csp newbackend pref which is not enabled until bug 925004 lands. Can someone of the sheriffs check it in please - thanks!