Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784753003:
CSP: Enable whitelisting of external JavaScript via hashes (Closed)
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
Mike West CC:
blink-reviews, blink-reviews-frames_chromium.org, blink-worker-reviews_chromium.org, chromium-reviews, falken+watch_chromium.org, horo+watch_chromium.org, kinuko+worker_chromium.org, shimazu+worker_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSP: Enable whitelisting of external JavaScript via hashes
See https://w3c.github.io/webappsec-csp/#external-hash
Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEWCwAJ
BUG=706380
Review-Url: https://codereview.chromium.org/2784753003
Cr-Commit-Position: refs/heads/master@{#462883}
Committed: https://chromium.googlesource.com/chromium/src/+/95c2d0890691fd854623b81bdd12037895987d18
Patch Set 1 #Patch Set 2 : webkit_unit_tests builds #
Total comments: 8
Patch Set 3 : don't support sha1, add comment, rebase #Patch Set 4 : unit test #Patch Set 5 : rebase #
Total comments: 5
Patch Set 6 : review #Patch Set 7 : layout test #Patch Set 8 : unit_tests++ #
Total comments: 5
Patch Set 9 : review #
Total comments: 2
Patch Set 10 : add tests for whitelisted host #
Total comments: 2
Patch Set 11 : remove duplicate test #Dependent Patchsets: Messages
Total messages: 49 (25 generated)
treib@chromium.org changed reviewers: + mkwst@chromium.org
PTApreliminaryL! This is clearly not ready for submission, but I think it's good enough for a first sanity check. It does seem to do the job for my use case, but I've almost certainly missed some other cases. If you agree that this looks reasonable, then I'll see about some tests.
That was pretty damn fast! I think you can clean up a little duplication of code, but in general this looks like it's going in the right direction. As you note, you need some tests. :) I'll send out an "Intent to Ship" on your behalf. Thanks! https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:48: ContentSecurityPolicyHashAlgorithm convertHashAlgorithmToCSPHashAlgorithm( Hrm. Can we just make these the same thing? I don't know why we have two hash enums. That doesn't make much sense. https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:52: return ContentSecurityPolicyHashAlgorithmSha1; We shouldn't support SHA-1. https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:64: bool parseBase64Digest(String base64, DigestValue& hash) { Can you add some comments noting why this is necessary? (just to accept base64 and base64url? in which case just move the comment up) https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:211: return false; This appears to replicate much of the logic in `checkDigest()` from `ContentSecurityPolicy.cpp`. Can we extract the normalization steps somewhere that can be reused?
(Tie this to bug 706380, please.) (Also, if you want to chat about tests, I'm happy to!)
On 2017/03/29 at 13:09:27, Mike West wrote: > (Tie this to bug 706380, please.) (Also note the intent thread at https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEW...)
Description was changed from ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash BUG=??? ========== to ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash BUG=706380 ==========
On 2017/03/29 13:29:37, Mike West wrote: > On 2017/03/29 at 13:09:27, Mike West wrote: > > (Tie this to bug 706380, please.) Thanks, done! > (Also note the intent thread at > https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEW...) Yup, I saw it! It was definitely a good thing that you handled that ;)
https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:48: ContentSecurityPolicyHashAlgorithm convertHashAlgorithmToCSPHashAlgorithm( On 2017/03/29 13:03:54, Mike West wrote: > Hrm. Can we just make these the same thing? I don't know why we have two hash > enums. That doesn't make much sense. I was wondering that myself :) The CSP variant has a "None" entry while the other doesn't, and it's also set up as a bitmask, so it won't be completely trivial to merge them. If you want, I can give it a try in a separate CL. (Though TBH, I'm also kinda trying to minimize the amount of yak shaving here ;) ) https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:52: return ContentSecurityPolicyHashAlgorithmSha1; On 2017/03/29 13:03:54, Mike West wrote: > We shouldn't support SHA-1. Done. https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:64: bool parseBase64Digest(String base64, DigestValue& hash) { On 2017/03/29 13:03:54, Mike West wrote: > Can you add some comments noting why this is necessary? (just to accept base64 > and base64url? in which case just move the comment up) The DigestValue type has a binary digest value, while SRI has a base64-encoded string. Comment added. (I'm actually not sure if base64url is required, but I think it doesn't hurt to accept it too..?) https://codereview.chromium.org/2784753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:211: return false; On 2017/03/29 13:03:54, Mike West wrote: > This appears to replicate much of the logic in `checkDigest()` from > `ContentSecurityPolicy.cpp`. Can we extract the normalization steps somewhere > that can be reused? I don't think they're all that similar. checkDigest() actually computes the hashes, while this just converts the format of existing hashes. The mapping between the two HashAlgorithm enums is duplicated in a slightly different form, but checkDigest() allows sha-1, so I'm not sure it's worth trying to de-dupe that.
Description was changed from ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash BUG=706380 ========== to ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEW... BUG=706380 ==========
Got it. Ok, thanks! Let's get some tests in, and we're good to go!
On 2017/03/30 at 10:10:53, Mike West (OOO until 4th) wrote: > Got it. Ok, thanks! > > Let's get some tests in, and we're good to go! (Anything I can do to help you make progress here? :) )
The CQ bit was checked by treib@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...
On 2017/04/04 12:13:09, Mike West (OOO until 4th) wrote: > On 2017/03/30 at 10:10:53, Mike West (OOO until 4th) wrote: > > Got it. Ok, thanks! > > > > Let's get some tests in, and we're good to go! > > (Anything I can do to help you make progress here? :) ) Sorry, something more urgent came up... But as it happens, I *just* uploaded some unit tests. Feel free to take a look :) I'll look into layout tests now. I assume they'd go into third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/ ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/04 at 12:15:19, treib wrote: > On 2017/04/04 12:13:09, Mike West (OOO until 4th) wrote: > > On 2017/03/30 at 10:10:53, Mike West (OOO until 4th) wrote: > > > Got it. Ok, thanks! > > > > > > Let's get some tests in, and we're good to go! > > > > (Anything I can do to help you make progress here? :) ) > > Sorry, something more urgent came up... > But as it happens, I *just* uploaded some unit tests. Feel free to take a look :) > I'll look into layout tests now. I assume they'd go into third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/ ? Unit tests are great % one question! Thanks for adding them. https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:351: << "List: `" << test.list << "`, URL: `" << test.url << "`"); You'll probably want to add `test.integrity` in here as well. https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:363: EXPECT_EQ(test.expected, Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode?
https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:351: << "List: `" << test.list << "`, URL: `" << test.url << "`"); On 2017/04/04 13:59:46, Mike West (OOO until 4th) wrote: > You'll probably want to add `test.integrity` in here as well. Good catch, done. https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:363: EXPECT_EQ(test.expected, On 2017/04/04 13:59:46, Mike West (OOO until 4th) wrote: > Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode? Hm, good question. I just copied this from AllowFromSourceWithNonce above. In fact, it looks like all the Allow* tests here treat enfore and report-only exactly the same. I guess the report-only thing is just handled on another level? Or something is seriously broken here...
https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:363: EXPECT_EQ(test.expected, On 2017/04/04 14:17:21, Marc Treib wrote: > On 2017/04/04 13:59:46, Mike West (OOO until 4th) wrote: > > Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode? > > Hm, good question. I just copied this from AllowFromSourceWithNonce above. In > fact, it looks like all the Allow* tests here treat enfore and report-only > exactly the same. I guess the report-only thing is just handled on another > level? Or something is seriously broken here... I'm confused now. There's ContentSecurityPolicyHeaderType (with values Report or Enforce) which is a property of the CSPDirectiveList. This is what we're setting here. But then there's also SecurityViolationReportingPolicy (with values SuppressReporting or Report), which is a param to allowScriptFromSource, and we're always passing SuppressReporting in the tests. That has the effect that CSPDirectiveList::allowScriptFromSource calls CSPDirectiveList::checkSource rather than CSPDirectiveList::checkSourceAndReportViolation, and checkSource doesn't care about the HeaderType, only ..AndReportViolation does. I'm not sure how all of that fits together to make sense. What are those two enums?
On 2017/04/04 at 14:29:49, treib wrote: > https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): > > https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:363: EXPECT_EQ(test.expected, > On 2017/04/04 14:17:21, Marc Treib wrote: > > On 2017/04/04 13:59:46, Mike West (OOO until 4th) wrote: > > > Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode? > > > > Hm, good question. I just copied this from AllowFromSourceWithNonce above. In > > fact, it looks like all the Allow* tests here treat enfore and report-only > > exactly the same. I guess the report-only thing is just handled on another > > level? Or something is seriously broken here... > > I'm confused now. There's ContentSecurityPolicyHeaderType (with values Report or Enforce) which is a property of the CSPDirectiveList. This is what we're setting here. But then there's also SecurityViolationReportingPolicy (with values SuppressReporting or Report), which is a param to allowScriptFromSource, and we're always passing SuppressReporting in the tests. That has the effect that CSPDirectiveList::allowScriptFromSource calls CSPDirectiveList::checkSource rather than CSPDirectiveList::checkSourceAndReportViolation, and checkSource doesn't care about the HeaderType, only ..AndReportViolation does. > > I'm not sure how all of that fits together to make sense. What are those two enums? One defines whether the policy itself is report-only or enforce mode. The other defines how the caller works: sometimes we just want to check whether a URL passes the page's policy for things like UseCounters without actually triggering a DOM event or report POST. I think this is a bug in `checkSource`, but I don't think it's your bug. :( Would you mind filing a bug against me? :)
On 2017/04/04 14:43:35, Mike West (OOO until 4th) wrote: > On 2017/04/04 at 14:29:49, treib wrote: > > > https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp > (right): > > > > > https://codereview.chromium.org/2784753003/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:363: > EXPECT_EQ(test.expected, > > On 2017/04/04 14:17:21, Marc Treib wrote: > > > On 2017/04/04 13:59:46, Mike West (OOO until 4th) wrote: > > > > Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode? > > > > > > Hm, good question. I just copied this from AllowFromSourceWithNonce above. > In > > > fact, it looks like all the Allow* tests here treat enfore and report-only > > > exactly the same. I guess the report-only thing is just handled on another > > > level? Or something is seriously broken here... > > > > I'm confused now. There's ContentSecurityPolicyHeaderType (with values Report > or Enforce) which is a property of the CSPDirectiveList. This is what we're > setting here. But then there's also SecurityViolationReportingPolicy (with > values SuppressReporting or Report), which is a param to allowScriptFromSource, > and we're always passing SuppressReporting in the tests. That has the effect > that CSPDirectiveList::allowScriptFromSource calls CSPDirectiveList::checkSource > rather than CSPDirectiveList::checkSourceAndReportViolation, and checkSource > doesn't care about the HeaderType, only ..AndReportViolation does. > > > > I'm not sure how all of that fits together to make sense. What are those two > enums? > > One defines whether the policy itself is report-only or enforce mode. The other > defines how the caller works: sometimes we just want to check whether a URL > passes the page's policy for things like UseCounters without actually triggering > a DOM event or report POST. > > I think this is a bug in `checkSource`, but I don't think it's your bug. :( > > Would you mind filing a bug against me? :) Filed crbug.com/708167. I wasn't sure about labels/components etc, so you'll have to add those yourself.
I've added a first layout test, PTAL! How much coverage do we want here? There's essentially unlimited numbers of combinations that could be tested, potentially duplicating all the unit tests.
The CQ bit was checked by treib@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by treib@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is great, Marc! Thanks! I would like to see more of the unit tests replicated into layout tests. I know that seems repetitious, but layout tests have the huge advantage of being usable by other vendors. If/when Firefox wants to catch up with our implementation, they're not going to be able to reuse our unit tests, but they _will_ be able to reuse the layout tests, so a little more investment up front on our part makes it possible for them to follow more quickly. https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/externalScript.js (right): https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/externalScript.js:1: externalRan = true; Nit: Please put this in .../content-security-policy/script-src/support/ https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html (right): https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html:48: </script> While I think this test doesn't actually run into race conditions, it reads as pretty racy due to the way you're using the same variable for everything. Rather than setting a property on the global object, how about listening for a message? You could include `.../content-security-policy/script-src/simpleSourcedScript.js` (which should also be in `.../support/` :( ), add an `id` attribute on the script, and wait for a message. Something like: ``` var test_cases = [ [ 'matching integrity', 'sha256-abc', true ], [ 'multiple matching integrity', 'sha256-abc sha512-abcdef', true ], [ 'no integrity', '', false ], [ 'mismatched integrity', 'sha256-xyz', false ], [ 'multiple mismatched intgerity', 'sha256-xyz sha256-zyx', false ], [ 'partially matching integrity', 'sha256-abc sha256-xyz', false ], ]; test(_ => { for (item of test_cases) { async_test(t => { var s = document.createElement('script'); s.id = item[0].replace(' ', '-'); s.src = './support/simpleSourcedScript.js'; s.integrity = item[1]; if (item[2]) { s.onerror = t.unreached_func("Script should load!"); window.addEventListener('message', t.step_func(e => { if (e.data == s.id) t.done(); })); } else { s.onerror = t.step_func_done(); window.addEventListener('message', t.step_func(e => { if (e.data == s.id) assert_unreached("Script should not execute!"); })); } document.body.appendChild(s); }, item[0]); } }, "Load all the tests."); ``` We should also verify that a policy that contains an origin allows scripts from that origin to execute even if the integrity attribute doesn't match the policy, etc.
https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/externalScript.js (right): https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/externalScript.js:1: externalRan = true; On 2017/04/06 10:26:35, Mike West (OOO until 4th) wrote: > Nit: Please put this in .../content-security-policy/script-src/support/ Since there's a whole bunch of other scripts that should go there, which in turn would require updating lots of tests, I've left it where it is for now, so at least things are consistent. If you insist that these scripts must move into support/, then I'll do that in a separate CL and rebase this one on top. https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html (right): https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html:48: </script> On 2017/04/06 10:26:35, Mike West (OOO until 4th) wrote: > While I think this test doesn't actually run into race conditions, it reads as > pretty racy due to the way you're using the same variable for everything. Rather > than setting a property on the global object, how about listening for a message? > You could include > `.../content-security-policy/script-src/simpleSourcedScript.js` (which should > also be in `.../support/` :( ), add an `id` attribute on the script, and wait > for a message. Something like: > > ``` > var test_cases = [ > [ 'matching integrity', 'sha256-abc', true ], > [ 'multiple matching integrity', 'sha256-abc sha512-abcdef', true ], > [ 'no integrity', '', false ], > [ 'mismatched integrity', 'sha256-xyz', false ], > [ 'multiple mismatched intgerity', 'sha256-xyz sha256-zyx', false ], > [ 'partially matching integrity', 'sha256-abc sha256-xyz', false ], > ]; > > test(_ => { > for (item of test_cases) { > async_test(t => { > var s = document.createElement('script'); > s.id = item[0].replace(' ', '-'); > s.src = './support/simpleSourcedScript.js'; > s.integrity = item[1]; > > if (item[2]) { > s.onerror = t.unreached_func("Script should load!"); > window.addEventListener('message', t.step_func(e => { > if (e.data == s.id) > t.done(); > })); > } else { > s.onerror = t.step_func_done(); > window.addEventListener('message', t.step_func(e => { > if (e.data == s.id) > assert_unreached("Script should not execute!"); > })); > } > > document.body.appendChild(s); > }, item[0]); > } > }, "Load all the tests."); > ``` Thanks! With my (lack of) JS skills, it would've probably taken me hours to piece together something like this. I think it's not really possible to test non-dynamically-inserted scripts this way though, so I've left that part in. I tried rewriting it to the postMessage style too, but there I don't know how to verify that the script *didn't* run, so in the error case that test will time out rather than properly fail :( Any ideas? Or should I just stick with the global variable there? (Now there'd be only the one test that uses it.) > We should also verify that a policy that contains an origin allows scripts from > that origin to execute even if the integrity attribute doesn't match the policy, > etc. (Not done yet; I'm looking into how to do that exactly.) https://codereview.chromium.org/2784753003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html (right): https://codereview.chromium.org/2784753003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html:64: <script nonce='dummy'> This is the new attempt for the parser-inserted-script-tag test, with postMessage. I don't know how to properly detect failure here; if something's wrong, the test would just time out. https://codereview.chromium.org/2784753003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html:75: <script nonce='dummy'> This is the previous version of the parser-inserted-script-tag test, with the global variable. But it's now the only test that uses that variable.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html (right): https://codereview.chromium.org/2784753003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.html:48: </script> On 2017/04/06 12:26:30, Marc Treib wrote: > On 2017/04/06 10:26:35, Mike West (OOO until 4th) wrote: > > While I think this test doesn't actually run into race conditions, it reads as > > pretty racy due to the way you're using the same variable for everything. > Rather > > than setting a property on the global object, how about listening for a > message? > > You could include > > `.../content-security-policy/script-src/simpleSourcedScript.js` (which should > > also be in `.../support/` :( ), add an `id` attribute on the script, and wait > > for a message. Something like: > > > > ``` > > var test_cases = [ > > [ 'matching integrity', 'sha256-abc', true ], > > [ 'multiple matching integrity', 'sha256-abc sha512-abcdef', true ], > > [ 'no integrity', '', false ], > > [ 'mismatched integrity', 'sha256-xyz', false ], > > [ 'multiple mismatched intgerity', 'sha256-xyz sha256-zyx', false ], > > [ 'partially matching integrity', 'sha256-abc sha256-xyz', false ], > > ]; > > > > test(_ => { > > for (item of test_cases) { > > async_test(t => { > > var s = document.createElement('script'); > > s.id = item[0].replace(' ', '-'); > > s.src = './support/simpleSourcedScript.js'; > > s.integrity = item[1]; > > > > if (item[2]) { > > s.onerror = t.unreached_func("Script should load!"); > > window.addEventListener('message', t.step_func(e => { > > if (e.data == s.id) > > t.done(); > > })); > > } else { > > s.onerror = t.step_func_done(); > > window.addEventListener('message', t.step_func(e => { > > if (e.data == s.id) > > assert_unreached("Script should not execute!"); > > })); > > } > > > > document.body.appendChild(s); > > }, item[0]); > > } > > }, "Load all the tests."); > > ``` > > Thanks! With my (lack of) JS skills, it would've probably taken me hours to > piece together something like this. > > I think it's not really possible to test non-dynamically-inserted scripts this > way though, so I've left that part in. I tried rewriting it to the postMessage > style too, but there I don't know how to verify that the script *didn't* run, so > in the error case that test will time out rather than properly fail :( > Any ideas? Or should I just stick with the global variable there? (Now there'd > be only the one test that uses it.) > > > We should also verify that a policy that contains an origin allows scripts > from > > that origin to execute even if the integrity attribute doesn't match the > policy, > > etc. > > (Not done yet; I'm looking into how to do that exactly.) Done now, PTAL!
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
LGTM! Thank you!
Thanks Mike! Any advice on the parser-inserted-script test? Tagged with comments below. https://codereview.chromium.org/2784753003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.sub.html (right): https://codereview.chromium.org/2784753003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.sub.html:91: <script nonce='dummy'> Here: New version with postMessage, but will time out rather than fail if something goes wrong. https://codereview.chromium.org/2784753003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/content-security-policy/script-src/script-src-sri_hash.sub.html:102: <script nonce='dummy'> Here: Old version with global variable (but it's now the only test that uses it).
The CQ bit was checked by treib@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If there's only one test using the global, then that seems fine? I think the `postMessage` variant is better if we decide to add tests in the future, but I'll defer to your aesthetic sense of right and wrong. :) Still LGTM.
On 2017/04/07 16:03:42, Mike West (OOO until 4th) wrote: > If there's only one test using the global, then that seems fine? I think the > `postMessage` variant is better if we decide to add tests in the future, but > I'll defer to your aesthetic sense of right and wrong. :) Still LGTM. Alright, thanks, then I'll land now! The problem with the postMessage variant is that I don't know how to detect failure (other than waiting for timeout). If you have ideas on how to do that, I'll make a followup CL to change things. Though I expect that if we do add more tests that use the global var, they'll all be non-async, so there shouldn't be problems with race conditions anyway.
The CQ bit was checked by treib@chromium.org
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": 200001, "attempt_start_ts": 1491581541323400,
"parent_rev": "e83dfc3e1bae851d9701cba5e2399be919cf75df", "commit_rev":
"95c2d0890691fd854623b81bdd12037895987d18"}
Message was sent while issue was closed.
Description was changed from ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEW... BUG=706380 ========== to ========== CSP: Enable whitelisting of external JavaScript via hashes See https://w3c.github.io/webappsec-csp/#external-hash Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/t2ai4lsHhWI/MndrZyEW... BUG=706380 Review-Url: https://codereview.chromium.org/2784753003 Cr-Commit-Position: refs/heads/master@{#462883} Committed: https://chromium.googlesource.com/chromium/src/+/95c2d0890691fd854623b81bdd12... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/95c2d0890691fd854623b81bdd12... |
