Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196633002:
[ServiceWorker] Don't check the origin equality when disable-web-security flag is set. (Closed)
|
Created:
4 years, 4 months ago by horo Modified:
4 years, 4 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, 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. |
Description[ServiceWorker] Don't check the origin equality when disable-web-security flag is set.
When "disable-web-security" flag is set, we don't check the origin equality in Blink.
So we shouldn't check it in content side.
BUG=619294, 630495, 630496
Committed: https://crrev.com/c19aeb3cd6b368a2ba4e46bd76fefe42a7f11e44
Cr-Commit-Position: refs/heads/master@{#408948}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : add browser_tests #
Total comments: 20
Patch Set 4 : incorporated nhiroki and shimazu's comment #
Total comments: 2
Patch Set 5 : incorporated nhiroki's comment #Messages
Total messages: 37 (26 generated)
The CQ bit was checked by horo@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by horo@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.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
The CQ bit was checked by horo@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...
Description was changed from ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. BUG=619294,630495,630496 ========== to ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ==========
Description was changed from ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ========== to ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't check the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ==========
horo@chromium.org changed reviewers: + nhiroki@chromium.org
nhiroki@ Could you please review?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nhiroki@chromium.org changed reviewers: + shimazu@chromium.org
+shimazu@ to reviewers
https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1919: Can you add a comment about what this test confirms? https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1957: net::EmbeddedTestServer cross_origin_server_; Can you move this to the private section? https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:71: switches::kDisableWebSecurity)) && Checking the flag whenever matching origins could be error-prone. How about making AllOriginsMatch() to accept variable length arguments and also checking the flag in the function like this? /* In class ServiceWorkerUtils */ // Returns true if all given urls have the same origin or // '--disable-web-security' flag is specified. template <typename... Args> static bool AllOriginsMatch(const GURL& url, const Args&... urls) { if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kDisableWebSecurity)) return true; for (const GURL& u : { urls... }) { if (url.GetOrigin() != u.GetOrigin()) return false; } return true; } Then, /* In CanUnregisterServiceWorker() etc */ return ServiceWorkerUtils::AllOriginsMatch<GURL>(document_url, pattern) && OriginCanAccessServiceWorkers(document_url) && OriginCanAccessServiceWorkers(pattern); https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:83: switches::kDisableWebSecurity); Just to confirm: This flag has an effect only when --user-data-dir flag is also specified, right? https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_unregister.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:5: iframe.onload = (_ => { Can you verify iframe is on a different origin from parent frame's origin? For example, if (iframe.location.origin == document.location.origin) { document.title = "FAIL"; return; } https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_update.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:5: iframe.onload = (_ => { ditto (origin check)
https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_unregister.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:7: .then(reg => reg.unregister() ) Extra space before the last ')' https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:7: .then(reg => reg.unregister() ) +2 indent? # There seems to be two types of coding style in this directory... which is correct? = Type 1 = func() .then(function() { return; }) = Type 2 = func().then(function() { return; }) https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_update.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:6: iframe.contentWindow.navigator.serviceWorker.ready +2 indent? https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:7: .then(reg => reg.update() ) Extra space before the last ')'
The CQ bit was checked by horo@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...
https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1919: On 2016/08/01 04:32:45, nhiroki wrote: > Can you add a comment about what this test confirms? Done. https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1957: net::EmbeddedTestServer cross_origin_server_; On 2016/08/01 04:32:45, nhiroki wrote: > Can you move this to the private section? Done. https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:71: switches::kDisableWebSecurity)) && On 2016/08/01 04:32:45, nhiroki wrote: > Checking the flag whenever matching origins could be error-prone. How about > making AllOriginsMatch() to accept variable length arguments and also checking > the flag in the function like this? > > /* In class ServiceWorkerUtils */ > > // Returns true if all given urls have the same origin or > // '--disable-web-security' flag is specified. > template <typename... Args> > static bool AllOriginsMatch(const GURL& url, const Args&... urls) { > if (base::CommandLine::ForCurrentProcess()->HasSwitch( > switches::kDisableWebSecurity)) > return true; > for (const GURL& u : { urls... }) { > if (url.GetOrigin() != u.GetOrigin()) > return false; > } > return true; > } > > Then, > > /* In CanUnregisterServiceWorker() etc */ > return ServiceWorkerUtils::AllOriginsMatch<GURL>(document_url, pattern) && > OriginCanAccessServiceWorkers(document_url) && > OriginCanAccessServiceWorkers(pattern); Done. I introduced PassOriginEqualitySecurityCheck(). https://codereview.chromium.org/2196633002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:83: switches::kDisableWebSecurity); On 2016/08/01 04:32:45, nhiroki wrote: > Just to confirm: This flag has an effect only when --user-data-dir flag is also > specified, right? If "user-data-dir" flag is not set, "disable-web-security" flag doesn't disable the same-origin policy in Blink side. See ChromeContentBrowserClient::OverrideWebkitPrefs(). https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_unregister.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:5: iframe.onload = (_ => { On 2016/08/01 04:32:45, nhiroki wrote: > Can you verify iframe is on a different origin from parent frame's origin? For > example, > > if (iframe.location.origin == document.location.origin) { > document.title = "FAIL"; > return; > } Done. https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:7: .then(reg => reg.unregister() ) On 2016/08/01 05:06:20, shimazu wrote: > Extra space before the last ')' Done. https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_unregister.html:7: .then(reg => reg.unregister() ) On 2016/08/01 05:06:20, shimazu wrote: > +2 indent? > > # There seems to be two types of coding style in this directory... which is > correct? > > = Type 1 = > func() > .then(function() { > return; > }) > > = Type 2 = > func().then(function() { > return; > }) Done. We use type 2. See http://www.chromium.org/blink/serviceworker/testing https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... File content/test/data/service_worker/disable_web_security_update.html (right): https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:5: iframe.onload = (_ => { On 2016/08/01 04:32:45, nhiroki wrote: > ditto (origin check) Done. https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:6: iframe.contentWindow.navigator.serviceWorker.ready On 2016/08/01 05:06:20, shimazu wrote: > +2 indent? Done. https://codereview.chromium.org/2196633002/diff/40001/content/test/data/servi... content/test/data/service_worker/disable_web_security_update.html:7: .then(reg => reg.update() ) On 2016/08/01 05:06:20, shimazu wrote: > Extra space before the last ')' Done.
lgtm
lgtm with a nit https://codereview.chromium.org/2196633002/diff/60001/content/common/service_... File content/common/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/2196633002/diff/60001/content/common/service_... content/common/service_worker/service_worker_utils.cc:12: #include "content/public/common/content_switches.h" These inclusions are not necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you! https://codereview.chromium.org/2196633002/diff/60001/content/common/service_... File content/common/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/2196633002/diff/60001/content/common/service_... content/common/service_worker/service_worker_utils.cc:12: #include "content/public/common/content_switches.h" On 2016/08/01 08:21:25, nhiroki wrote: > These inclusions are not necessary. Done.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2196633002/#ps80001 (title: "incorporated nhiroki's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't check the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ========== to ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't check the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't check the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 ========== to ========== [ServiceWorker] Don't check the origin equality when disable-web-security flag is set. When "disable-web-security" flag is set, we don't check the origin equality in Blink. So we shouldn't check it in content side. BUG=619294,630495,630496 Committed: https://crrev.com/c19aeb3cd6b368a2ba4e46bd76fefe42a7f11e44 Cr-Commit-Position: refs/heads/master@{#408948} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c19aeb3cd6b368a2ba4e46bd76fefe42a7f11e44 Cr-Commit-Position: refs/heads/master@{#408948} |
