Reland: service worker: Don't control a subframe of an insecure context

Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002

Committed: https://crrev.com/ad1850962644e19cdb040d60eb236e0ebc23c243
Cr-Commit-Position: refs/heads/master@{#400093}

[falken, 2016-06-16 02:35:05]

Description was changed from

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2009453002
Cr-Commit-Position: refs/heads/master@{#398229}
==========

to

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Original review: https://codereview.chromium.org/2009453002
==========

[falken, 2016-06-16 02:38:42]

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

[falken, 2016-06-16 02:38:43]

kinuko: Would you be able to review this reland?

The diff from the previous patch is the delta from patchset 1.

The problem with the previous patch was that Clients.claim() could also cause an
insecure context to be controlled. The new patch checks for that and adds a test
case.

[kinuko, 2016-06-16 03:33:59]

lgtm.

[falken, 2016-06-16 04:25:16]

Description was changed from

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Original review: https://codereview.chromium.org/2009453002
==========

to

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002
==========

[falken, 2016-06-16 04:25:16]

falken@chromium.org changed reviewers:
+ alexmos@chromium.org, clamy@chromium.org, dcheng@chromium.org,
jww@chromium.org, rdevlin.cronin@chromium.org

[falken, 2016-06-16 04:25:44]

TBRing owners from previous review

[falken, 2016-06-16 04:26:39]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2016-06-16 04:26:50]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-06-16 04:27:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2071433003/20001

[commit-bot: I haz the power, 2016-06-16 06:10:18]

Description was changed from

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002
==========

to

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002
==========

[commit-bot: I haz the power, 2016-06-16 06:10:20]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002
==========

to

==========
Reland: service worker: Don't control a subframe of an insecure context

We must check isSecureContext when creating the network provider to
adhere to
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged.

We already did this for getRegistration(), register(), unregister() but must
also do this when deciding whether to control an in-scope document.

BUG=607543
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=reviewers from the original review

Original review: https://codereview.chromium.org/2009453002

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

[commit-bot: I haz the power, 2016-06-16 06:12:07]

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

[dcheng, 2016-06-16 08:59:44]

Sorry, I didn't see this CL till now.

I thought about it a bit more the last few days, and I don't think we should add
the Blink public API for this. It means we can't share the code, but I think
that's fine: this is a one-off thing for service workers, as far as I can tell.
If it becomes useful for other things, then we can revisit exposing it in the
public API.

[falken, 2016-06-17 00:52:34]

On 2016/06/16 08:59:44, dcheng wrote:
> Sorry, I didn't see this CL till now.
> 
> I thought about it a bit more the last few days, and I don't think we should
add
> the Blink public API for this. It means we can't share the code, but I think
> that's fine: this is a one-off thing for service workers, as far as I can
tell.
> If it becomes useful for other things, then we can revisit exposing it in the
> public API.

I don't have a strong opinion (the early patchsets on the previous review did
that) but jww@ and alexmos@ expressed a strong preference for sharing the code.

I'm loath to revert this now since I want it to pass through canary and get it
merged ASAP.

I'll fire up a patch to change the public API part and see how it looks.

[dcheng, 2016-06-17 06:49:24]

On 2016/06/17 00:52:34, falken wrote:
> On 2016/06/16 08:59:44, dcheng wrote:
> > Sorry, I didn't see this CL till now.
> > 
> > I thought about it a bit more the last few days, and I don't think we should
> add
> > the Blink public API for this. It means we can't share the code, but I think
> > that's fine: this is a one-off thing for service workers, as far as I can
> tell.
> > If it becomes useful for other things, then we can revisit exposing it in
the
> > public API.
> 
> I don't have a strong opinion (the early patchsets on the previous review did
> that) but jww@ and alexmos@ expressed a strong preference for sharing the
code.
> 
> I'm loath to revert this now since I want it to pass through canary and get it
> merged ASAP.
> 

Sorry, I was unclear: we definitely shouldn't revert this patch.

> I'll fire up a patch to change the public API part and see how it looks.

A followup CL is fine for this investigation.

[alexmos, 2016-06-17 06:52:19]

On 2016/06/17 00:52:34, falken wrote:
> On 2016/06/16 08:59:44, dcheng wrote:
> > Sorry, I didn't see this CL till now.
> > 
> > I thought about it a bit more the last few days, and I don't think we should
> add
> > the Blink public API for this. It means we can't share the code, but I think
> > that's fine: this is a one-off thing for service workers, as far as I can
> tell.
> > If it becomes useful for other things, then we can revisit exposing it in
the
> > public API.
> 
> I don't have a strong opinion (the early patchsets on the previous review did
> that) but jww@ and alexmos@ expressed a strong preference for sharing the
code.
> 
> I'm loath to revert this now since I want it to pass through canary and get it
> merged ASAP.
> 
> I'll fire up a patch to change the public API part and see how it looks.

FWIW, I supported sharing the code before we knew that we couldn't merge the
duplicate calls in public API, but had the same concern as Daniel for that
(https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...),
so I'm ok with following Daniel's suggestion in a followup CL.