Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256833004:
Move Service Worker %2f validation logic from browser into Blink (3) (Closed)
|
Created:
5 years, 4 months ago by jeremyarcher Modified:
5 years, 4 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove Service Worker %2f validation logic from browser into Blink (3)
This patch ensures that a compromised renderer is still unable to
register Service Workers with an invalid scope or scriptURL (see
bug for spec details).
1. (Chromium) https://codereview.chromium.org/1259213002
2. (Blink) https://codereview.chromium.org/1260003003/
3. (Chromium) This CL.
BUG=513622
R=nhiroki, falken
Committed: https://crrev.com/950451079244693e16f3326eb8bee9263d8276d1
Cr-Commit-Position: refs/heads/master@{#342601}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Switch back to previous validation routine for %2f in scopes. #
Total comments: 1
Patch Set 3 : Fix merge conflicts with earlier patch. #Patch Set 4 : Add tests for dangerous %5c and %2f in SW URLs. #
Total comments: 3
Patch Set 5 : Fix copypasta in testing identifier. #
Total comments: 2
Patch Set 6 : Clean up test for style. #
Messages
Total messages: 23 (6 generated)
This might be silly, but for some reason the validation (..., std::string *error) functions feel qualitatively different from the security check functions, so I've separated the logic out, even though they're performing the same computation. Thoughts?
https://codereview.chromium.org/1256833004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1256833004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_dispatcher_host.cc:355: ContainsDisallowedCharacter(script_url)) { I'd prefer to reuse ServiceWorkerUtils::ContainsDisallowedCharacter because it's generally difficult to keep duplicate code consistent. How about making the function to accept null |error_message|?
On 2015/07/28 05:32:13, nhiroki wrote: > https://codereview.chromium.org/1256833004/diff/1/content/browser/service_wor... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1256833004/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_dispatcher_host.cc:355: > ContainsDisallowedCharacter(script_url)) { > I'd prefer to reuse ServiceWorkerUtils::ContainsDisallowedCharacter because it's > generally difficult to keep duplicate code consistent. How about making the > function to accept null |error_message|? Works for me! PTAL
lgtm
Let me add one more comment... https://codereview.chromium.org/1256833004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1256833004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:337: bad_message::ReceivedBadMessage(this, bad_message::SWDH_REGISTER_CANNOT); Can you add tests for this case in service_worker_dispatcher_host_unittest.cc?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2015/07/31 07:39:36, nhiroki wrote: > Let me add one more comment... > > https://codereview.chromium.org/1256833004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1256833004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_dispatcher_host.cc:337: > bad_message::ReceivedBadMessage(this, bad_message::SWDH_REGISTER_CANNOT); > Can you add tests for this case in service_worker_dispatcher_host_unittest.cc? Added. Er, they might not pass until https://codereview.chromium.org/1259213002 is landed, but I've tested it with a local rebase and everything appears fine.
https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:370: TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { These test names look reversed.
https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:370: TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { On 2015/08/05 06:55:35, falken wrote: > These test names look reversed. Whoops! Had to stare at it for a second; PTAL.
The CQ bit was checked by jeremyarcher@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1256833004/#ps100001 (title: "Add tests for dangerous %5c and %2f in SW URLs.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256833004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256833004/100001
The CQ bit was unchecked by jeremyarcher@google.com
https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:370: TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { On 2015/08/10 03:29:40, jeremyarcher wrote: > On 2015/08/05 06:55:35, falken wrote: > > These test names look reversed. > > Whoops! Had to stare at it for a second; PTAL. I cannot see the updated patchset. Maybe you failed to upload it?
On 2015/08/10 08:03:17, nhiroki wrote: > https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... > File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc > (right): > > https://codereview.chromium.org/1256833004/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:370: > TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { > On 2015/08/10 03:29:40, jeremyarcher wrote: > > On 2015/08/05 06:55:35, falken wrote: > > > These test names look reversed. > > > > Whoops! Had to stare at it for a second; PTAL. > > I cannot see the updated patchset. Maybe you failed to upload it? Huh, must've. I've re-git cl upload'd -- is it showing up now?
Now I can see the patchset. Thanks :) https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:337: TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { "Register_BadCharactersShouldFail" seems more consistent with other tests. https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:337: TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { Can you move this test to line 313 because this is placed between tests for filesystem URLs (Register_FileSystemDocumentShouldFail and Register_FileSystemScriptOrScopeShouldFail). It'd be preferable to group similar tests.
On 2015/08/10 08:15:51, nhiroki wrote: > Now I can see the patchset. Thanks :) > > https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... > File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc > (right): > > https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... > content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:337: > TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { > "Register_BadCharactersShouldFail" seems more consistent with other tests. Ah, right; changed. > https://codereview.chromium.org/1256833004/diff/120001/content/browser/servic... > content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:337: > TEST_F(ServiceWorkerDispatcherHostTest, RegisterWithBadCharactersShouldFail) { > Can you move this test to line 313 because this is placed between tests for > filesystem URLs (Register_FileSystemDocumentShouldFail and > Register_FileSystemScriptOrScopeShouldFail). It'd be preferable to group similar > tests. Done.
lgtm
The CQ bit was checked by jeremyarcher@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256833004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256833004/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/950451079244693e16f3326eb8bee9263d8276d1 Cr-Commit-Position: refs/heads/master@{#342601} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
