Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1260003003:
Move Service Worker %2f validation logic from browser into Blink (2) (Closed)
|
Created:
5 years, 4 months ago by jeremyarcher Modified:
5 years, 4 months ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, falken, kinuko+serviceworker, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMove Service Worker %2f validation logic from browser into Blink (2)
Adding this feature allows the navigator.serviceWorker.register() function
immediately reject with a TypeError when the scope or scriptURL appears
to be invalid (see bug for spec details).
1. (Chromium) https://codereview.chromium.org/1259213002
2. (Blink) This CL.
3. (Chromium) https://codereview.chromium.org/1256833004/
BUG=513622
R=nhiroki,falken
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200231
Patch Set 1 #
Total comments: 3
Patch Set 2 : More validation back into Blink. #
Total comments: 2
Patch Set 3 : Fix compile error (!) in original patch. #
Total comments: 1
Patch Set 4 : Add { return false; } #Patch Set 5 : Add prefix error label before failure message. #
Total comments: 3
Messages
Total messages: 28 (6 generated)
https://codereview.chromium.org/1260003003/diff/1/LayoutTests/http/tests/serv... File LayoutTests/http/tests/serviceworker/registration.html (right): https://codereview.chromium.org/1260003003/diff/1/LayoutTests/http/tests/serv... LayoutTests/http/tests/serviceworker/registration.html:261: It'd be good to have tests for capital letter cases (ie. %5C and %2F) https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:252: if (path.findIgnoringCase("%2f") != WTF::kNotFound || path.findIgnoringCase("%5c") != WTF::kNotFound || scopePath.findIgnoringCase("%2f") != WTF::kNotFound || scopePath.findIgnoringCase("%5c") != WTF::kNotFound) { To dedup code, how about providing path check functionalities from chromium via WebServiceWorkerProvider? For example, 1) In blink, add WebSWProvider::containsDisallowedCharacter(const WebURL&) 2) In chromium, implement WebSWProviderImpl::containsDC(const WebURL&) which calls SWUtils::ContainsDC(). 3) In chromium, SWUtils is currently under content/browser/. We need to move it into content/common so that WebSWProviderImpl can use it from content/child/. https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:255: "Failed to register a ServiceWorker: The provided scope ('" + patternURL.string() + "') or scriptURL ('" + scriptURL.string() + "') includes a disallowed escape character.")); In blink, you don't have to wrap at 80-column.
On 2015/07/28 07:04:36, nhiroki wrote: > https://codereview.chromium.org/1260003003/diff/1/LayoutTests/http/tests/serv... > File LayoutTests/http/tests/serviceworker/registration.html (right): > > https://codereview.chromium.org/1260003003/diff/1/LayoutTests/http/tests/serv... > LayoutTests/http/tests/serviceworker/registration.html:261: > It'd be good to have tests for capital letter cases (ie. %5C and %2F) > > https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:252: if > (path.findIgnoringCase("%2f") != WTF::kNotFound || path.findIgnoringCase("%5c") > != WTF::kNotFound || scopePath.findIgnoringCase("%2f") != WTF::kNotFound || > scopePath.findIgnoringCase("%5c") != WTF::kNotFound) { > To dedup code, how about providing path check functionalities from chromium via > WebServiceWorkerProvider? For example, > > 1) In blink, add WebSWProvider::containsDisallowedCharacter(const WebURL&) > 2) In chromium, implement WebSWProviderImpl::containsDC(const WebURL&) which > calls SWUtils::ContainsDC(). > 3) In chromium, SWUtils is currently under content/browser/. We need to move it > into content/common so that WebSWProviderImpl can use it from content/child/. > > https://codereview.chromium.org/1260003003/diff/1/Source/modules/serviceworke... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:255: "Failed to > register a ServiceWorker: The provided scope ('" + patternURL.string() + "') or > scriptURL ('" + scriptURL.string() + "') includes a disallowed escape > character.")); > In blink, you don't have to wrap at 80-column. It has been refactored. How's it look?
Looks good. Nits only. https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/registration.html (right): https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration.html:231: new TypeError, Just passing a string 'TypeError' doesn't work? https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:252: resolver->reject(V8ThrowException::createTypeError(scriptState->isolate(), errorMessage)); '"Failed to register a ServiceWorker: " + errorMessage' seems more consistent with other error messages.
On 2015/07/31 07:43:31, nhiroki wrote: > Looks good. Nits only. > > https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... > File LayoutTests/http/tests/serviceworker/registration.html (right): > > https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/registration.html:231: new TypeError, > Just passing a string 'TypeError' doesn't work? I think in that case it looks for a type of DOMException called "TypeError:" https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:252: > resolver->reject(V8ThrowException::createTypeError(scriptState->isolate(), > errorMessage)); > '"Failed to register a ServiceWorker: " + errorMessage' seems more consistent > with other error messages. I've added that on the other side- that okay? The "Failed to register a ServiceWorker: " bit seems to exist in both places.
On 2015/07/31 08:10:13, jeremyarcher wrote: > On 2015/07/31 07:43:31, nhiroki wrote: > > Looks good. Nits only. > > > > > https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... > > File LayoutTests/http/tests/serviceworker/registration.html (right): > > > > > https://codereview.chromium.org/1260003003/diff/20001/LayoutTests/http/tests/... > > LayoutTests/http/tests/serviceworker/registration.html:231: new TypeError, > > Just passing a string 'TypeError' doesn't work? > > I think in that case it looks for a type of DOMException called "TypeError:" > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Ah, I didn't know it. Thank you for your explanation. > > > https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... > > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > > > > https://codereview.chromium.org/1260003003/diff/20001/Source/modules/servicew... > > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:252: > > resolver->reject(V8ThrowException::createTypeError(scriptState->isolate(), > > errorMessage)); > > '"Failed to register a ServiceWorker: " + errorMessage' seems more consistent > > with other error messages. > > I've added that on the other side- that okay? The "Failed to register a > ServiceWorker: " bit seems to exist in both places. Do you mean that validateScopeAndScriptURL() will return an error message prefixed with "Failed to..."? I think it'd be better not to include the prefix in that side because this utility function is independent of registration sequence.
https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... File public/platform/WebServiceWorkerProvider.h (right): https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... public/platform/WebServiceWorkerProvider.h:65: virtual bool validateScopeAndScriptURL(const WebURL& scope, const WebURL& scriptURL, WebString* errorMessage); Can you add "{ return false; }"?
On 2015/07/31 08:47:32, nhiroki wrote: > https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... > File public/platform/WebServiceWorkerProvider.h (right): > > https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... > public/platform/WebServiceWorkerProvider.h:65: virtual bool > validateScopeAndScriptURL(const WebURL& scope, const WebURL& scriptURL, > WebString* errorMessage); > Can you add "{ return false; }"? Sure thing! Added. (PTAL)
On 2015/08/03 03:18:54, jeremyarcher wrote: > On 2015/07/31 08:47:32, nhiroki wrote: > > > https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... > > File public/platform/WebServiceWorkerProvider.h (right): > > > > > https://codereview.chromium.org/1260003003/diff/40001/public/platform/WebServ... > > public/platform/WebServiceWorkerProvider.h:65: virtual bool > > validateScopeAndScriptURL(const WebURL& scope, const WebURL& scriptURL, > > WebString* errorMessage); > > Can you add "{ return false; }"? > > Sure thing! Added. > > (PTAL) Forgot to mention- the code also now adds the prefix on the Blink side.
lgtm
https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:251: if (!m_provider->validateScopeAndScriptURL(patternURL, scriptURL, &webErrorMessage)) { Ah it's kind of weird that validateScopeAndScript is a non-static function on m_provider, it's just a static check that has nothing to do with the m_provider instance. I guess we had to to this to allow blink code to call into chromium. Is there a better way? I can't think of one, so won't block this, but just wanted to raise the question.
nit on cl description: navigator.serviceWorker.register() isn't an IPC, it's just a javascript function.
https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:251: if (!m_provider->validateScopeAndScriptURL(patternURL, scriptURL, &webErrorMessage)) { On 2015/08/05 06:54:23, falken wrote: > Ah it's kind of weird that validateScopeAndScript is a non-static function on > m_provider, it's just a static check that has nothing to do with the m_provider > instance. I guess we had to to this to allow blink code to call into chromium. > Is there a better way? > > I can't think of one, so won't block this, but just wanted to raise the > question. Ah, good point. Moving validateSASURL into Platform.h could be an option?
https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/1260003003/diff/80001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:251: if (!m_provider->validateScopeAndScriptURL(patternURL, scriptURL, &webErrorMessage)) { On 2015/08/05 07:15:09, nhiroki wrote: > On 2015/08/05 06:54:23, falken wrote: > > Ah it's kind of weird that validateScopeAndScript is a non-static function on > > m_provider, it's just a static check that has nothing to do with the > m_provider > > instance. I guess we had to to this to allow blink code to call into chromium. > > Is there a better way? > > > > I can't think of one, so won't block this, but just wanted to raise the > > question. > > Ah, good point. Moving validateSASURL into Platform.h could be an option? I had an offline chat w/ jeremy and falken. We decided to keep this as is because either way we cannot make this static.
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/1260003003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260003003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
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/1260003003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260003003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
jeremyarcher@google.com changed reviewers: + tkent@chromium.org
Er, oops. +tkent@chromium.org
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/1260003003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260003003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200231 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
