Limit requests for which link headers can install service workers to secure contexts.

Limit requests for which link headers can install service workers to secure contexts.

Main resource requests are only allowed if the context will become a secure
context (as decided by ServiceWorkerProviderHost::IsContextSecureForServiceWorker),
while for subresources what matters is if the request was made from a secure
context.

BUG=582310
Committed: https://crrev.com/db276dcdee7a1b993483f5fcc706e77e8e2542c4
Cr-Commit-Position: refs/heads/master@{#401902}

[Marijn Kruisselbrink, 2016-06-21 23:14:22]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2016-06-22 18:49:27]

Description was changed from

==========
WIP Limit requests for which link headers can install service workers to secure
contexts.

Still needs tests.

BUG=
==========

to

==========
Limit requests for which link headers can install service workers to secure
contexts.

Main resource requests are only allowed if the context will become a secure
context (as decided by
ServiceWorkerProviderHost::IsContextSecureForServiceWorker),
while for subresources what matters is if the request was made from a secure
context.

BUG=582310
==========

[Marijn Kruisselbrink, 2016-06-22 18:49:59]

mek@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-06-23 14:42:27]

mostly looking good, some nits

https://codereview.chromium.org/1914593002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1914593002/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2264: false);  //
initiated_in_secure_context
when we're enabling plznavigate how will we handle this? (it's ok not to have
answer for now, but if necessary we can note additional TODO here too)

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:77:
ServiceWorkerContextCore* context() const { return helper_->context(); }
nit: avoid returning non const ptr from const method

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:81:
ServiceWorkerProviderHost* provider_host() const {
ditto

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:88: ResourceType
resource_type = RESOURCE_TYPE_SCRIPT) {
Adding bool parameter makes the code harder to read, while adding /*
secure_context */ comment to all the callsites seems a bit too much hassle for
testing code...

maybe just keep this method as is but add another method that takes two more
params (without default param) like

ProcessLinkHeaderWithSecureContextInfo()

and call the new one from existing ProcessLinkHeader() one with 'true,
RESOURCE_TYPE_SCRIPT' parameters?  For the new method we could always add
comment like /* secure_context */

[Marijn Kruisselbrink, 2016-06-23 18:02:02]

https://codereview.chromium.org/1914593002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1914593002/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2264: false);  //
initiated_in_secure_context
On 2016/06/23 at 14:42:27, kinuko wrote:
> when we're enabling plznavigate how will we handle this? (it's ok not to have
answer for now, but if necessary we can note additional TODO here too)

Added a TODO. At least it doesn't matter for the link header case, as in the
case of navigations I'm relying on
ServiceWorkerProviderHost::IsContextSecureForServiceWorker rather than the
initiated_in_secure_context flag.

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:77:
ServiceWorkerContextCore* context() const { return helper_->context(); }
On 2016/06/23 at 14:42:27, kinuko wrote:
> nit: avoid returning non const ptr from const method

removed the const. (fwiw, this particular const method returning a non-const
ServiceWorkerContextCore* pointer exists in at least six other serviceworker
unit test classes).

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:81:
ServiceWorkerProviderHost* provider_host() const {
On 2016/06/23 at 14:42:27, kinuko wrote:
> ditto

Done

https://codereview.chromium.org/1914593002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:88: ResourceType
resource_type = RESOURCE_TYPE_SCRIPT) {
On 2016/06/23 at 14:42:27, kinuko wrote:
> Adding bool parameter makes the code harder to read, while adding /*
secure_context */ comment to all the callsites seems a bit too much hassle for
testing code...
> 
> maybe just keep this method as is but add another method that takes two more
params (without default param) like
> 
> ProcessLinkHeaderWithSecureContextInfo()
> 
> and call the new one from existing ProcessLinkHeader() one with 'true,
RESOURCE_TYPE_SCRIPT' parameters?  For the new method we could always add
comment like /* secure_context */

Yeah, having default parameters and bool parameters isn't very nice. Now instead
I changed this method to be just a CreateRequest method, and have the tests
explicitly call ProcessLinkHeaderForRequest. That way tests that need to can
manually set/clear the secure_context flag.

[kinuko, 2016-06-24 10:01:29]

lgtm.

[Marijn Kruisselbrink, 2016-06-24 16:24:06]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-06-24 16:24:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 17:54:37]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-24 17:57:22]

Description was changed from

==========
Limit requests for which link headers can install service workers to secure
contexts.

Main resource requests are only allowed if the context will become a secure
context (as decided by
ServiceWorkerProviderHost::IsContextSecureForServiceWorker),
while for subresources what matters is if the request was made from a secure
context.

BUG=582310
==========

to

==========
Limit requests for which link headers can install service workers to secure
contexts.

Main resource requests are only allowed if the context will become a secure
context (as decided by
ServiceWorkerProviderHost::IsContextSecureForServiceWorker),
while for subresources what matters is if the request was made from a secure
context.

BUG=582310

Committed: https://crrev.com/db276dcdee7a1b993483f5fcc706e77e8e2542c4
Cr-Commit-Position: refs/heads/master@{#401902}
==========

[commit-bot: I haz the power, 2016-06-24 17:57:23]

Patchset 5 (id:??) landed as
https://crrev.com/db276dcdee7a1b993483f5fcc706e77e8e2542c4
Cr-Commit-Position: refs/heads/master@{#401902}