|
|
DescriptionEmbedding-CSP: Refactoring directive strings into enum.
This is an effort as part of the experimental feature. Causing
patch: https://codereview.chromium.org/2474903002
BUG=647588
Committed: https://crrev.com/a5e9944b7169dd3a3c9f2cb352aff3551b32cd12
Cr-Commit-Position: refs/heads/master@{#434474}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Changing reportViolation*() to accepting DirectiveType + adding a unit tests #
Total comments: 9
Patch Set 3 : Addressing comments #
Total comments: 4
Patch Set 4 : Second inversion/fixing default for a switch #
Total comments: 3
Patch Set 5 : Rebasing #Patch Set 6 : syncing #
Messages
Total messages: 38 (23 generated)
Description was changed from ========== For convenience, this patch refactors all directive strings to enum. This is an effort as part of the experimental feature EmbeddingCSP. BUG=647588 ========== to ========== Embedding-CSP: Refactoring directive strings into enum. This is an effort as part of the experimental feature. Causing patch: https://codereview.chromium.org/2474903002 BUG=647588 ==========
amalika@google.com changed reviewers: + mkwst@chromium.org
As per suggestion in Part 3.1 (https://codereview.chromium.org/2474903002), refactoring CSP to use enum instead of static const char* of directive names.
Looking good, thanks! https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:195: ContentSecurityPolicy::BlockAllMixedContent), As noted above, I think I'd prefer that you simply pass the enum into `reportViolation`, and calculate the name when you need it. https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:445: ContentSecurityPolicy::getDirectiveName(ContentSecurityPolicy::ChildSrc), Same thing here as in the other comment: convert |effectiveDirective| once, then do straight equality comparisons. https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:76: return bool(getDirectiveType(name)); Can this be `return getDirectiveType(name) != Unknown`? Also, while we're here, do we need this method at all if we have the one-liner above? It looks like we only use it twice... perhaps we can simply drop it? https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1003: ContentSecurityPolicy::getDirectiveName( I'd suggest converting |effectiveDirective| to an enum at the top of this function (passing the enum through instead of the string would be even better) to avoid all the case folding comparisons here. `effectiveDirectiveType == FrameSrc` is simpler and faster and shorter. :) Ditto for the remaining comparisons below. https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1530: return ""; Can we be stricter here? `NOTREACHED()` for instance? https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right): https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:87: enum DirectiveType { Please make this an `enum class`. https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:107: // CSP Level 3 Directives Nit: While you're here, I'd just drop these comments entirely, and sort alphabetically. https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:119: DirectiveTypeEnd Why do we need this? https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:366: static DirectiveType getDirectiveType(const String& name); Please add unit tests for these. I know they're trivial, but tests document their behavior (especially things like case-folding and "What happens if I stuff random data in?")? A comment to that effect would be helpful as well. "Returns the DirectiveType associated with |name|, or 'Unknown' if no such directive exists.", etc.
amalika@google.com changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1539: default: Note that `Undefined` type will go to default. https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:917: ContentSecurityPolicy::DirectiveType::BlockAllMixedContent, I tried iterating over possible DirectiveType values like for enum (using its values as ints), but that does not seem possible for enum class? https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:942: // ContentSecurityPolicy::DirectiveType::Undefined)); I was getting an error: ` error: cannot use 'try' with exceptions disabled EXPECT_ANY_THROW(ContentSecurityPolicy::getDirectiveName(ContentSecurityPolicy::DirectiveType::Undefined)); ` Not sure how to test the NOTREACHED() part?
Thanks! https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1539: default: On 2016/11/23 at 11:25:33, amalika wrote: > Note that `Undefined` type will go to default. Sure. But we should never get to a point where we're using `Undefined` internally, right? Actually, now that I'm looking at this again, please don't use `default:`. Instead, have a `case` for each item in the enum. Then if we add items to the enum in the future, the compiler will force us to come back and update this method (and we'll hopefully remember to to the same for `getDirectiveType` (crashing if we get an undefined item helps with the latter :) )). https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:917: ContentSecurityPolicy::DirectiveType::BlockAllMixedContent, On 2016/11/23 at 11:25:33, amalika wrote: > I tried iterating over possible DirectiveType values like for enum (using its values as ints), but that does not seem possible for enum class? No, you can't iterate over an enum's values. You shouldn't really iterate over a non-class enum's values (as there's no guarantee that they actually increment themselves by 1. They might be flags, for instance (`1 << 0`, `1 << 1`, etc is pretty common in Blink). Doing an array like this or a map is a better long-term option. https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:942: // ContentSecurityPolicy::DirectiveType::Undefined)); On 2016/11/23 at 11:25:33, amalika wrote: > I was getting an error: > ` > error: cannot use 'try' with exceptions disabled > EXPECT_ANY_THROW(ContentSecurityPolicy::getDirectiveName(ContentSecurityPolicy::DirectiveType::Undefined)); > ` > Not sure how to test the NOTREACHED() part? ASSERT_DEATH should work. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid.... https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:946: ContentSecurityPolicy::getDirectiveName(type))); Could you explicitly test both directions? You could add the expected value to the test case, for instance.
In part 3.1 https://codereview.chromium.org/2474903002, we need to iterate over all directives (line 1203 in CSPDirectiveList.cpp) Right now we explicitly whitelist those, and iterate over them. The danger is that if other directives are added, that `subsumes` will have to be modified as well. Should we add a method on CSP level that would return all possible directive types from the enum? https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:917: ContentSecurityPolicy::DirectiveType::BlockAllMixedContent, On 2016/11/23 at 11:38:34, Mike West (slow) wrote: > On 2016/11/23 at 11:25:33, amalika wrote: > > I tried iterating over possible DirectiveType values like for enum (using its values as ints), but that does not seem possible for enum class? > > No, you can't iterate over an enum's values. You shouldn't really iterate over a non-class enum's values (as there's no guarantee that they actually increment themselves by 1. They might be flags, for instance (`1 << 0`, `1 << 1`, etc is pretty common in Blink). Doing an array like this or a map is a better long-term option. Ah, I see! Though I guess I was concerned that if new directives are added, they will never be tested or they will have to be manually added here for tests. https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:942: // ContentSecurityPolicy::DirectiveType::Undefined)); On 2016/11/23 at 11:38:34, Mike West (slow) wrote: > On 2016/11/23 at 11:25:33, amalika wrote: > > I was getting an error: > > ` > > error: cannot use 'try' with exceptions disabled > > EXPECT_ANY_THROW(ContentSecurityPolicy::getDirectiveName(ContentSecurityPolicy::DirectiveType::Undefined)); > > ` > > Not sure how to test the NOTREACHED() part? > > ASSERT_DEATH should work. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid.... Perfect! Thank you :)
The CQ bit was checked by mkwst@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...
LGTM % some nits, thanks! On 2016/11/23 at 12:42:15, amalika wrote: > In part 3.1 https://codereview.chromium.org/2474903002, > we need to iterate over all directives (line 1203 in > CSPDirectiveList.cpp) > > Right now we explicitly whitelist those, and iterate over them. > The danger is that if other directives are added, that `subsumes` > will have to be modified as well. > Should we add a method on CSP level that would return all possible > directive types from the enum? If that seems helpful to you, add it in that patch and let's see how it looks. :) You'll still need to categorize the directives, right? Like, pick out the ones that are sourcelists and etc? It's not clear to me how you want that to look. https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1000: effectiveType != ContentSecurityPolicy::DirectiveType::FrameSrc && You're inside `ContentSecurityPolicy`. I think you can safely drop `ContentSecurityPolicy::` from these. https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp (right): https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:957: ContentSecurityPolicy::getDirectiveName(test.type))); Since you've already done everything else, can you invert this one too? :)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by amalika@google.com 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...
A few changes! https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1000: effectiveType != ContentSecurityPolicy::DirectiveType::FrameSrc && On 2016/11/23 at 12:58:08, Mike West (slow) wrote: > You're inside `ContentSecurityPolicy`. I think you can safely drop `ContentSecurityPolicy::` from these. Some of the methods are static here so if I remove `ContentSecurityPolicy::`, I get an error: use of undeclared identifier 'DirectiveType'. https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp (right): https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:957: ContentSecurityPolicy::getDirectiveName(test.type))); On 2016/11/23 at 12:58:08, Mike West (slow) wrote: > Since you've already done everything else, can you invert this one too? :) Just to make sure you meant this inversion, I added this: ` EXPECT_EQ(test.name, ContentSecurityPolicy::getDirectiveName( ContentSecurityPolicy::getDirectiveType(test.name))); ` https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1545: return ""; Since we did not add `default` case, I had to add this as otherwise the build was failing: ` error: control reaches end of non-void function [-Werror=return-type] `
Thanks! Still LGTM % comments. https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1545: return ""; On 2016/11/23 at 13:38:39, amalika wrote: > Since we did not add `default` case, I had to add this as otherwise the build was failing: > ` > error: control reaches end of non-void function [-Werror=return-type] > ` Ah. Windows. :/ https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right): https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:108: Undefined, Nit: Move this to the top, please. It's different enough to make it reasonable to break out of the alphabetical order.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2516383002/#ps120001 (title: "Rebasing")
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by amalika@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2516383002/#ps140001 (title: "syncing")
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": 140001, "attempt_start_ts": 1480071232716520, "parent_rev": "36f77430ea20b2cea81e87562945afc75ed8989d", "commit_rev": "3c431c9caef02edb333edb7ebd2683fdb598ca2e"}
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Embedding-CSP: Refactoring directive strings into enum. This is an effort as part of the experimental feature. Causing patch: https://codereview.chromium.org/2474903002 BUG=647588 ========== to ========== Embedding-CSP: Refactoring directive strings into enum. This is an effort as part of the experimental feature. Causing patch: https://codereview.chromium.org/2474903002 BUG=647588 Committed: https://crrev.com/a5e9944b7169dd3a3c9f2cb352aff3551b32cd12 Cr-Commit-Position: refs/heads/master@{#434474} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a5e9944b7169dd3a3c9f2cb352aff3551b32cd12 Cr-Commit-Position: refs/heads/master@{#434474}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2528133002/ by magjed@chromium.org. The reason for reverting is: Causing WebKit Win x64 Builder to fail on webkit_unit_tests on Windows-7-SP1 ContentSecurityPolicyTest.DirectiveType: ContentSecurityPolicyTest.DirectiveType (run #1): [ RUN ] ContentSecurityPolicyTest.DirectiveType c:\c\win_layout\src hird_party\webkit\source\corerame\csp\contentsecuritypolicytest.cpp(950): error: Death test: ContentSecurityPolicy::getDirectiveName( ContentSecurityPolicy::DirectiveType::Undefined) Result: failed to die. Error msg: [ DEATH ] [3244:5672:1125/030930:3521831:WARNING:test_suite.cc(236)] Test launcher output path C:\Users\CHROME~2\AppData\Local\Temp!6_27934 est_results.xml exists. Not adding test launcher result printer. [ DEATH ] [ FAILED ] ContentSecurityPolicyTest.DirectiveType (67 ms) https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win%....
Message was sent while issue was closed.
Patchset #7 (id:160001) has been deleted
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2533683003/ by magjed@chromium.org. The reason for reverting is: Causes ContentSecurityPolicyTest.DirectiveType to fail in webkit_unit_tests: C 143.019s Main [FAIL] ContentSecurityPolicyTest.DirectiveType: C 143.019s Main [ RUN ] ContentSecurityPolicyTest.DirectiveType C 143.019s Main C 143.019s Main [WARNING] ../../testing/gtest/src/gtest-death-test.cc:834:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 11 threads. C 143.019s Main ../../third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:950: Failure C 143.020s Main Death test: ContentSecurityPolicy::getDirectiveName( ContentSecurityPolicy::DirectiveType::Undefined) C 143.020s Main Result: failed to die. C 143.020s Main Error msg: C 143.020s Main [ DEATH ] C 143.020s Main [ FAILED ] ContentSecurityPolicyTest.DirectiveType (24 ms) https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr.... |