Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792973004:
Moved all tests about bypassing CSP into csp-tests (content layer) (Closed)
DescriptionMoved all tests about bypassing CSP into ContentSecurityPolicyTest (mirror)
This is a mirror implementation of https://codereview.chromium.org/2714203002
Summary from there:
"Removed logic related to bypassing CSP from SourceListDirective
Ammended ContentSecurityPolicy to know about blob and filesystem bypasses"
BUG=692046
Review-Url: https://codereview.chromium.org/2792973004
Cr-Commit-Position: refs/heads/master@{#462058}
Committed: https://chromium.googlesource.com/chromium/src/+/e5cc01bf83814738ab18e0ef4c0404b5e2119b3e
Patch Set 1 #
Total comments: 7
Patch Set 2 : CR changes #
Total comments: 4
Patch Set 3 : CR changes Jochen #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== Moved all tests about bypassing CSP into ContentSecurityPolicyTest (mirror) This is a mirror implementation of "https://codereview.chromium.org/2714203002" Summary from there: "Removed logic related to bypassing CSP from SourceListDirective Ammended ContentSecurityPolicy to know about blob and filesystem bypasses" BUG=692046 ========== to ========== Moved all tests about bypassing CSP into ContentSecurityPolicyTest (mirror) This is a mirror implementation of https://codereview.chromium.org/2714203002 Summary from there: "Removed logic related to bypassing CSP from SourceListDirective Ammended ContentSecurityPolicy to know about blob and filesystem bypasses" BUG=692046 ==========
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
andypaicu@chromium.org changed reviewers: + arthursonzogni@chromium.org, mkwst@chromium.org
Hi Arthur, As discussed by e-mail this is for the third bug you mentioned. Please let me know your thoughts.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Andy! That looks great. A few comments: https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:97: const GURL ExtractInnerURL(const GURL& url) { Nit: Add a newline above. https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy_unittest.cc (right): https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy_unittest.cc:164: // Register 'https' as bypassing CSP, which should now bypass is entirely Nit: missing dot at the end of the comment. Same thing for the next two comments. https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy_unittest.cc:187: GURL("filesystem:https://example.com/file.txt/"), This line exceeds 80 character. Please run git cl format For the the URLs in this 3 tests, please declare it on its own line such that it could be reused. https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy_unittest.cc:187: GURL("filesystem:https://example.com/file.txt/"), Why is there a slash at the end of the URL? There is the same thing in a few other URLs in this CL.
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:97: const GURL ExtractInnerURL(const GURL& url) { On 2017/04/04 at 11:54:34, arthursonzogni wrote: > Nit: Add a newline above. Done https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy_unittest.cc (right): https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy_unittest.cc:164: // Register 'https' as bypassing CSP, which should now bypass is entirely On 2017/04/04 at 11:54:34, arthursonzogni wrote: > Nit: missing dot at the end of the comment. > Same thing for the next two comments. Done https://codereview.chromium.org/2792973004/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy_unittest.cc:187: GURL("filesystem:https://example.com/file.txt/"), On 2017/04/04 at 11:54:34, arthursonzogni wrote: > This line exceeds 80 character. > Please run git cl format > > For the the URLs in this 3 tests, please declare it on its own line such that it could be reused. Urgh... git cl format was failing locally but the presubmit checks where hiding the error so I thought everything is ok. The slash is there for copy paste reasons :D. Also the blob URLs don't make sense with a "file.txt" at the end of the URL. I'm not exactly sure what you mean by on it's own line. Do you mean have a variable for the URLs something like GURL exampleURL("https://example.com") and then use the variable below in the tests? If so I'm not exactly sure if that's a better approach because it makes the URL less obvious at a glance when looking at the tests. For me seeing the URL on every test makes it more readable than having variables and having to look at the variable definition to see what the actual URL is.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM % 2 nits. https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:140: if (ShouldBypassContentSecurityPolicy(context, url)) return true; Nit: please use two lines. https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... File content/common/content_security_policy/csp_source_list.cc (right): https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... content/common/content_security_policy/csp_source_list.cc:54: if (source_list.allow_self && context->AllowSelf(url)) return true; Nit: please use two lines.
On 2017/04/05 at 08:27:13, arthursonzogni wrote: > Thanks! LGTM % 2 nits. > > https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... > File content/common/content_security_policy/content_security_policy.cc (right): > > https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... > content/common/content_security_policy/content_security_policy.cc:140: if (ShouldBypassContentSecurityPolicy(context, url)) return true; > Nit: please use two lines. > > https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... > File content/common/content_security_policy/csp_source_list.cc (right): > > https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... > content/common/content_security_policy/csp_source_list.cc:54: if (source_list.allow_self && context->AllowSelf(url)) return true; > Nit: please use two lines. That's git cl format doing that so I don't think I should change that.
On 2017/04/05 09:16:29, andypaicu wrote: > That's git cl format doing that so I don't think I should change that. My git cl format was using two lines. Then I updated the depots_tools and it uses one line now. So I am okay with your change. > For me seeing the URL on every test makes it more readable than > having variables and having to look at the variable definition to see what the > actual URL is. Okay, that's a good point. I am okay with it.
The CQ bit was checked by andypaicu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
andypaicu@chromium.org changed reviewers: + jochen@chromium.org
andypaicu@chromium.org changed required reviewers: + jochen@chromium.org
On 2017/04/05 at 11:57:39, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Hi Jochen, It appears I need a content owner for this patch, could I bother you to have a look? Also the presubmit format warning is a red-herring as I've run it and it does not edit anything.
lgtm with nit https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy_unittest.cc (right): https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy_unittest.cc:33: }; please add DISALLOW_COPY_AND_ASSIGN(CSPContextTest);
https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy_unittest.cc (right): https://codereview.chromium.org/2792973004/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy_unittest.cc:33: }; On 2017/04/05 at 12:22:35, jochen wrote: > please add DISALLOW_COPY_AND_ASSIGN(CSPContextTest); Done
The CQ bit was checked by andypaicu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, arthursonzogni@chromium.org Link to the patchset: https://codereview.chromium.org/2792973004/#ps40001 (title: "CR changes Jochen")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491396919164640,
"parent_rev": "fb0d1678d04256ad6e41aaa88a7edd83ca8ca030", "commit_rev":
"e5cc01bf83814738ab18e0ef4c0404b5e2119b3e"}
Message was sent while issue was closed.
Description was changed from ========== Moved all tests about bypassing CSP into ContentSecurityPolicyTest (mirror) This is a mirror implementation of https://codereview.chromium.org/2714203002 Summary from there: "Removed logic related to bypassing CSP from SourceListDirective Ammended ContentSecurityPolicy to know about blob and filesystem bypasses" BUG=692046 ========== to ========== Moved all tests about bypassing CSP into ContentSecurityPolicyTest (mirror) This is a mirror implementation of https://codereview.chromium.org/2714203002 Summary from there: "Removed logic related to bypassing CSP from SourceListDirective Ammended ContentSecurityPolicy to know about blob and filesystem bypasses" BUG=692046 Review-Url: https://codereview.chromium.org/2792973004 Cr-Commit-Position: refs/heads/master@{#462058} Committed: https://chromium.googlesource.com/chromium/src/+/e5cc01bf83814738ab18e0ef4c04... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e5cc01bf83814738ab18e0ef4c04... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
