Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085923002:
service worker: When claiming, don't assume document_url is valid (Closed)
|
Created:
4 years, 6 months ago by falken Modified:
4 years, 6 months ago Reviewers:
horo CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_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. |
Descriptionservice worker: When claiming, don't assume document_url is valid
Many provider hosts have an empty document_url, for example those
that haven't yet been loaded or those created for special URLs like
chrome-search://. So that claim can use IsContextSecureForServiceWorker,
return false when the URL is invalid instead of doing a
DCHECK that it's valid.
BUG=621762, 607543
Committed: https://crrev.com/dd5dd98f5f59f4f53274308ec0a7dca74aba6525
Cr-Commit-Position: refs/heads/master@{#401216}
Patch Set 1 #Patch Set 2 : horo comment #
Total comments: 1
Patch Set 3 : comment #Patch Set 4 : revise comment #Messages
Total messages: 22 (13 generated)
falken@chromium.org changed reviewers: + horo@chromium.org
horo: Can you take a look? As mentioned in the description, even calling IsContextSecureForServiceWorker should be redundant, but I don't want to rely on MatchRegistration for the security check.
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. We can't call IsContextSecureForServiceWorker unless the URL is valid. This patch moves the IsContextSecureForServiceWorker to after the URL has matched the registration. Actually MatchRegistration() should not return true unless the context is secure, but call it directly anyway instead of relying on MatchRegistration to enforce the security check. BUG=621762 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. We can't call IsContextSecureForServiceWorker unless the URL is valid. This patch moves the IsContextSecureForServiceWorker to after the URL has matched the registration. Actually MatchRegistration() should not return true unless the context is secure, but call it directly anyway instead of relying on MatchRegistration to enforce the security check. BUG=621762,607543 ==========
On 2016/06/21 09:33:48, falken wrote: > horo: Can you take a look? > > As mentioned in the description, even calling IsContextSecureForServiceWorker > should be redundant, but I don't want to rely on MatchRegistration for the > security check. I think that changing ServiceWorkerProviderHost::IsContextSecureForServiceWorker() to return false when !document_url_.is_valid() is better.
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. We can't call IsContextSecureForServiceWorker unless the URL is valid. This patch moves the IsContextSecureForServiceWorker to after the URL has matched the registration. Actually MatchRegistration() should not return true unless the context is secure, but call it directly anyway instead of relying on MatchRegistration to enforce the security check. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid. BUG=621762,607543 ==========
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of DCHECK that it's valid. BUG=621762,607543 ==========
Ah yea that's better. Done.
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an invalid document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of DCHECK that it's valid. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of DCHECK that it's valid. BUG=621762,607543 ==========
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of DCHECK that it's valid. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of doing a DCHECK that it's valid. BUG=621762,607543 ==========
LGTM with nit https://codereview.chromium.org/2085923002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2085923002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:122: if (!document_url_.is_valid()) nit: Please add comments about when it happens.
Done.
The CQ bit was checked by falken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org Link to the patchset: https://codereview.chromium.org/2085923002/#ps40001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2085923002/40001
The CQ bit was unchecked by falken@chromium.org
The CQ bit was checked by falken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org Link to the patchset: https://codereview.chromium.org/2085923002/#ps60001 (title: "revise comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2085923002/60001
Message was sent while issue was closed.
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of doing a DCHECK that it's valid. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of doing a DCHECK that it's valid. BUG=621762,607543 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of doing a DCHECK that it's valid. BUG=621762,607543 ========== to ========== service worker: When claiming, don't assume document_url is valid Many provider hosts have an empty document_url, for example those that haven't yet been loaded or those created for special URLs like chrome-search://. So that claim can use IsContextSecureForServiceWorker, return false when the URL is invalid instead of doing a DCHECK that it's valid. BUG=621762,607543 Committed: https://crrev.com/dd5dd98f5f59f4f53274308ec0a7dca74aba6525 Cr-Commit-Position: refs/heads/master@{#401216} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dd5dd98f5f59f4f53274308ec0a7dca74aba6525 Cr-Commit-Position: refs/heads/master@{#401216} |
