Closed
Bug 1020477
Opened 11 years ago
Closed 11 years ago
CSP in C++: Convert test/unit/test_csp_ignores_path.js to compiled code tests
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
Attachments
(1 file, 3 obsolete files)
13.12 KB,
patch
|
geekboy
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Summary: Bug 1020407 - CSP in C++: Convert test/unit/test_csp_ignores_path.js to compiled code tests → CSP in C++: Convert test/unit/test_csp_ignores_path.js to compiled code tests
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mozilla
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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.
Attachment #8434407 -
Flags: review?(sstamm)
Comment 3•11 years ago
|
||
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?
Attachment #8434407 -
Flags: review?(sstamm)
Assignee | ||
Comment 4•11 years ago
|
||
(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!
Attachment #8434407 -
Attachment is obsolete: true
Attachment #8434544 -
Flags: review?(sstamm)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Attachment #8434544 -
Attachment is obsolete: true
Attachment #8434544 -
Flags: review?(sstamm)
Attachment #8434555 -
Flags: review?(sstamm)
Comment 6•11 years ago
|
||
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).
Attachment #8434555 -
Flags: review?(sstamm) → review+
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Attachment #8434555 -
Attachment is obsolete: true
Attachment #8435232 -
Flags: review?(sstamm)
Comment 8•11 years ago
|
||
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.
Attachment #8435232 -
Flags: review?(sstamm) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(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?
Assignee | ||
Comment 10•11 years ago
|
||
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!
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•