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

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

Committed: https://crrev.com/59a2e54eeb0e61971a0c27c44c54dd0c30b5d06d
Cr-Commit-Position: refs/heads/master@{#398229}

[falken, 2016-05-24 09:18:36]

Description was changed from

==========
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
==========

to

==========
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
==========

[falken, 2016-05-24 09:31:02]

falken@chromium.org changed reviewers:
+ clamy@chromium.org, horo@chromium.org, jww@chromium.org, kinuko@chromium.org

[falken, 2016-05-24 09:31:03]

PTAL
horo: everything
kinuko: content/renderer
clamy: frame_host
jww: security review

[falken, 2016-05-24 11:29:58]

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

[falken, 2016-05-24 11:29:58]

+creis for OOPIF question

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_network_provider.cc:89: if (parent
&& !parent->document().isSecureContext(errorMessage))
Some tests fail. It looks like this can't be done because parent can be "remote"
(out of process iframe), in which case parent->document() fails.

Is there a way to check security context of a WebFrame including remote frames?

[clamy, 2016-05-24 11:54:14]

content/browser/frame_host lgtm.

[falken, 2016-05-24 13:16:38]

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_network_provider.cc:89: if (parent
&& !parent->document().isSecureContext(errorMessage))
On 2016/05/24 11:29:58, falken wrote:
> Some tests fail. It looks like this can't be done because parent can be
"remote"
> (out of process iframe), in which case parent->document() fails.
> 
> Is there a way to check security context of a WebFrame including remote
frames?

I see now, there is WebSecurityOrigin. Updated the patch.

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
This ancestor walk should probably be some utility function like
Document::isSecureContext(). One subtle difference is we shouldn't check for
scheme exceptions like Document does, since those shouldn't be applied to child
frames, which is what we care about here.

[Marijn Kruisselbrink, 2016-05-24 17:03:35]

drive-by comment

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
On 2016/05/24 at 13:16:37, falken wrote:
> This ancestor walk should probably be some utility function like
Document::isSecureContext(). One subtle difference is we shouldn't check for
scheme exceptions like Document does, since those shouldn't be applied to child
frames, which is what we care about here.

Also keep in mind that to truly match what is specified in
https://w3c.github.io/webappsec-secure-contexts/#settings-object you need to not
only exclude iframes inside insecure contexts, but also top-level windows opened
by insecure contexts without no-opener.

[Charlie Reis, 2016-05-24 17:21:59]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org
- creis@chromium.org

[Charlie Reis, 2016-05-24 17:22:00]

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_network_provider.cc:89: if (parent
&& !parent->document().isSecureContext(errorMessage))
On 2016/05/24 13:16:37, falken wrote:
> On 2016/05/24 11:29:58, falken wrote:
> > Some tests fail. It looks like this can't be done because parent can be
> "remote"
> > (out of process iframe), in which case parent->document() fails.
> > 
> > Is there a way to check security context of a WebFrame including remote
> frames?
> 
> I see now, there is WebSecurityOrigin. Updated the patch.

Yep, that's the right way.  +alexmos to verify it's used correctly here.  (See
also mek@'s comment.)

[Charlie Reis, 2016-05-24 17:22:19]

Description was changed from

==========
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
==========

to

==========
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
==========

[jww, 2016-05-24 18:06:04]

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
To ask a dumb question, why not add that check right now?

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
On 2016/05/24 17:03:35, Marijn Kruisselbrink wrote:
> On 2016/05/24 at 13:16:37, falken wrote:
> > This ancestor walk should probably be some utility function like
> Document::isSecureContext(). One subtle difference is we shouldn't check for
> scheme exceptions like Document does, since those shouldn't be applied to
child
> frames, which is what we care about here.
> 
> Also keep in mind that to truly match what is specified in
> https://w3c.github.io/webappsec-secure-contexts/#settings-object you need to
not
> only exclude iframes inside insecure contexts, but also top-level windows
opened
> by insecure contexts without no-opener.

I have a strong preference to factor this out into a shared utility function
that Document and this both use. It will be much easier to track changes, and
even to just track that this check exists at all. This would also probably give
you a helpful error message to use, no?

Also, why don't you check if the current origin is potentially trustworthy? That
is, you only start checking the child frames. I think this is related to mek's
comment.

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.h:21: }
nit: "  // namespace blink"

[alexmos, 2016-05-24 18:51:55]

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_network_provider.cc:89: if (parent
&& !parent->document().isSecureContext(errorMessage))
On 2016/05/24 17:22:00, Charlie Reis (slow to reply) wrote:
> On 2016/05/24 13:16:37, falken wrote:
> > On 2016/05/24 11:29:58, falken wrote:
> > > Some tests fail. It looks like this can't be done because parent can be
> > "remote"
> > > (out of process iframe), in which case parent->document() fails.
> > > 
> > > Is there a way to check security context of a WebFrame including remote
> > frames?
> > 
> > I see now, there is WebSecurityOrigin. Updated the patch.
> 
> Yep, that's the right way.  +alexmos to verify it's used correctly here.  (See
> also mek@'s comment.)

Yes, remote frames should have correct SecurityOrigins (which we replicate from
the corresponding local frame).  We've also recently refactored
SecurityOrigin::isPotentiallyTrustworthy and Document::isSecureContext to work
correctly with remote frames (see issue 571079).  Ideally you'd just call
WebDocument::isSecureContext on the |frame|'s document, so it's too bad that it
isn't created yet.

+1 to jww@'s suggestion to unify this walk with the one in
Document::isSecureContext if possible.

[falken, 2016-05-25 01:33:34]

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
On 2016/05/24 18:06:03, jww wrote:
> On 2016/05/24 17:03:35, Marijn Kruisselbrink wrote:
> > On 2016/05/24 at 13:16:37, falken wrote:
> > > This ancestor walk should probably be some utility function like
> > Document::isSecureContext(). One subtle difference is we shouldn't check for
> > scheme exceptions like Document does, since those shouldn't be applied to
> child
> > frames, which is what we care about here.
> > 
> > Also keep in mind that to truly match what is specified in
> > https://w3c.github.io/webappsec-secure-contexts/#settings-object you need to
> not
> > only exclude iframes inside insecure contexts, but also top-level windows
> opened
> > by insecure contexts without no-opener.
> 
> I have a strong preference to factor this out into a shared utility function
> that Document and this both use. It will be much easier to track changes, and
> even to just track that this check exists at all. This would also probably
give
> you a helpful error message to use, no?
> 
> Also, why don't you check if the current origin is potentially trustworthy?
That
> is, you only start checking the child frames. I think this is related to mek's
> comment.

I can't start with the current frame because document is not yet created.
getSecurityOrigin() calls frame()->securityContext()->getSecurityOrigin(), but
securityContext() is just document(), so it blows up.

I probably do need to check the current frame to address mek's point. Ideas?

[falken, 2016-05-25 04:09:45]

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
On 2016/05/25 01:33:34, falken wrote:
> On 2016/05/24 18:06:03, jww wrote:
> > On 2016/05/24 17:03:35, Marijn Kruisselbrink wrote:
> > > On 2016/05/24 at 13:16:37, falken wrote:
> > > > This ancestor walk should probably be some utility function like
> > > Document::isSecureContext(). One subtle difference is we shouldn't check
for
> > > scheme exceptions like Document does, since those shouldn't be applied to
> > child
> > > frames, which is what we care about here.
> > > 
> > > Also keep in mind that to truly match what is specified in
> > > https://w3c.github.io/webappsec-secure-contexts/#settings-object you need
to
> > not
> > > only exclude iframes inside insecure contexts, but also top-level windows
> > opened
> > > by insecure contexts without no-opener.
> > 
> > I have a strong preference to factor this out into a shared utility function
> > that Document and this both use. It will be much easier to track changes,
and
> > even to just track that this check exists at all. This would also probably
> give
> > you a helpful error message to use, no?
> > 
> > Also, why don't you check if the current origin is potentially trustworthy?
> That
> > is, you only start checking the child frames. I think this is related to
mek's
> > comment.
> 
> I can't start with the current frame because document is not yet created.
> getSecurityOrigin() calls frame()->securityContext()->getSecurityOrigin(), but
> securityContext() is just document(), so it blows up.
> 
> I probably do need to check the current frame to address mek's point. Ideas?

Does Chrome currently check for insecure opener when deciding whether a context
is secure? It looks like Document::isSecureContext() doesn't check it. If I do
window.open from an http page to an https page, I can register a service worker
from the opened page.

[falken, 2016-05-25 08:53:26]

Patchset #3 (id:40001) has been deleted

[falken, 2016-05-25 09:19:58]

I've refactored out the parent walk. Please take another look.

No check is added for opener since it looks like Chrome doesn't check that
currently for isSecureContext().

This is failing some tests with --site-per-process enabled. I can reproduce the
failed content_browsertest, but wasn't able to debug. It looks like it crashes.
The output is just:
[ RUN      ]
NavigationControllerBrowserTest.FrameNavigationEntry_SlowNestedAutoSubframe
[0525/181736:ERROR:kill_posix.cc(82)] Unable to terminate process group 11836:
No such process
[1/1]
NavigationControllerBrowserTest.FrameNavigationEntry_SlowNestedAutoSubframe
(1570 ms)
1 test failed:

No luck with running in gdb/no-sandbox. The test appears to be creating a nested
frame in an not-yet-loaded frame, so maybe it's incorrect to assume that
frame->parent() has a security context after all?

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/24 18:06:03, jww wrote:
> To ask a dumb question, why not add that check right now?

I didn't look too hard, but there's no WebFrame here. clamy, how hard would it
be to add a check here like the one in service_worker_navigation_provider?

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.h:21: }
On 2016/05/24 18:06:03, jww wrote:
> nit: "  // namespace blink"

Done.

[clamy, 2016-05-25 11:21:46]

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/25 09:19:58, falken wrote:
> On 2016/05/24 18:06:03, jww wrote:
> > To ask a dumb question, why not add that check right now?
> 
> I didn't look too hard, but there's no WebFrame here. clamy, how hard would it
> be to add a check here like the one in service_worker_navigation_provider?

If you're checking for the sandbox flags, you can get them from the
FrameTreeNode that NavigationRequest keeps a pointer to.

[falken, 2016-05-26 02:09:49]

I fixed the site isolation error.

alexmos, jww: Can you review this again?

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/25 11:21:46, clamy wrote:
> On 2016/05/25 09:19:58, falken wrote:
> > On 2016/05/24 18:06:03, jww wrote:
> > > To ask a dumb question, why not add that check right now?
> > 
> > I didn't look too hard, but there's no WebFrame here. clamy, how hard would
it
> > be to add a check here like the one in service_worker_navigation_provider?
> 
> If you're checking for the sandbox flags, you can get them from the
> FrameTreeNode that NavigationRequest keeps a pointer to.

I want to do an ancestor walk over the frame tree, checking their security (I
think FrameTreeNode doesn't have that info). But I think the latest patch
changed this so service_worker_network_provider can handle the PlzNavigate case
well.

[clamy, 2016-05-26 15:45:31]

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/26 02:09:49, falken wrote:
> On 2016/05/25 11:21:46, clamy wrote:
> > On 2016/05/25 09:19:58, falken wrote:
> > > On 2016/05/24 18:06:03, jww wrote:
> > > > To ask a dumb question, why not add that check right now?
> > > 
> > > I didn't look too hard, but there's no WebFrame here. clamy, how hard
would
> it
> > > be to add a check here like the one in service_worker_navigation_provider?
> > 
> > If you're checking for the sandbox flags, you can get them from the
> > FrameTreeNode that NavigationRequest keeps a pointer to.
> 
> I want to do an ancestor walk over the frame tree, checking their security (I
> think FrameTreeNode doesn't have that info). But I think the latest patch
> changed this so service_worker_network_provider can handle the PlzNavigate
case
> well.

FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a
FrameTreeNodeReplicationState that has some security info, including the origin
(and whether it's potentially trustworthy). The only thing missing to do a check
that matches the one in RenderFrame is the list of whitelisted urls. I'm not
sure where you'd find that in the renderer.

[Marijn Kruisselbrink, 2016-05-26 20:37:11]

drive-by nit

https://codereview.chromium.org/2009453002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2009453002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:219: if
(new_provider_id == kInvalidServiceWorkerVersionId)
Shouldn't this be kInvalidServiceWorkerProviderId rather than VersionId? I know
they're all -1, but the types are different.

[alexmos, 2016-05-26 22:13:35]

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/26 15:45:31, clamy wrote:
> On 2016/05/26 02:09:49, falken wrote:
> > On 2016/05/25 11:21:46, clamy wrote:
> > > On 2016/05/25 09:19:58, falken wrote:
> > > > On 2016/05/24 18:06:03, jww wrote:
> > > > > To ask a dumb question, why not add that check right now?
> > > > 
> > > > I didn't look too hard, but there's no WebFrame here. clamy, how hard
> would
> > it
> > > > be to add a check here like the one in
service_worker_navigation_provider?
> > > 
> > > If you're checking for the sandbox flags, you can get them from the
> > > FrameTreeNode that NavigationRequest keeps a pointer to.
> > 
> > I want to do an ancestor walk over the frame tree, checking their security
(I
> > think FrameTreeNode doesn't have that info). But I think the latest patch
> > changed this so service_worker_network_provider can handle the PlzNavigate
> case
> > well.
> 
> FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a
> FrameTreeNodeReplicationState that has some security info, including the
origin
> (and whether it's potentially trustworthy). The only thing missing to do a
check
> that matches the one in RenderFrame is the list of whitelisted urls. I'm not
> sure where you'd find that in the renderer.

Yeah, you should be able to follow the FrameTreeNode::parent() chain and do the
check on FrameTreeNode::current_origin().  You'd need to do some additional work
to get isSecureContext's scheme whitelist here, but seems like
secureContextBypassingSchemes() ultimately comes from 
GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common, so
available in the browser process.  So it's actually isn't that bad, but you'd
need some extra delegate plumbing to get to it from content/.

What seems harder is that the origin we have on FrameTreeNodes is a url::Origin,
which doesn't have all the information from a SecurityOrigin.  We do have a
trustworthiness bit in FrameTreeNode::current_replication_state(),
has_potentially_trustworthy_unique_origin, but it isn't actually equivalent to
SecurityOrigin::isPotentiallyTrustworthy.  It's only meaningful for unique
origins (i.e., it indicates potential trustworthiness for a sandboxed iframe),
and you'd still need to either do all the other work in
SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or replicate
the full value of isPotentiallyTrustworthy() from the renderer.  Overall, all
this seems non-trivial and I'd vote for doing it in a separate CL.

CC-ing estark@, who's worked on isSecureContext plumbing recently and may have
more thoughts on getting it to work from the browser process.

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool
canHaveSecureChild(WebString& errorMessage) const;
Do we need both this and WebDocument::isSecureContext in the public API?  Seems
WebDocument::isSecureContext is only used in plugins, where it's called on the
plugin container frame.  Could that just use canHaveSecureChild?

Also, since both uses don't care about the error message, maybe we don't need it
in the public API either?

[falken, 2016-05-27 05:47:03]

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

[falken, 2016-05-27 05:47:04]

Thanks! comment-only for now.

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/26 22:13:35, alexmos wrote:
> On 2016/05/26 15:45:31, clamy wrote:
> > On 2016/05/26 02:09:49, falken wrote:
> > > On 2016/05/25 11:21:46, clamy wrote:
> > > > On 2016/05/25 09:19:58, falken wrote:
> > > > > On 2016/05/24 18:06:03, jww wrote:
> > > > > > To ask a dumb question, why not add that check right now?
> > > > > 
> > > > > I didn't look too hard, but there's no WebFrame here. clamy, how hard
> > would
> > > it
> > > > > be to add a check here like the one in
> service_worker_navigation_provider?
> > > > 
> > > > If you're checking for the sandbox flags, you can get them from the
> > > > FrameTreeNode that NavigationRequest keeps a pointer to.
> > > 
> > > I want to do an ancestor walk over the frame tree, checking their security
> (I
> > > think FrameTreeNode doesn't have that info). But I think the latest patch
> > > changed this so service_worker_network_provider can handle the PlzNavigate
> > case
> > > well.
> > 
> > FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a
> > FrameTreeNodeReplicationState that has some security info, including the
> origin
> > (and whether it's potentially trustworthy). The only thing missing to do a
> check
> > that matches the one in RenderFrame is the list of whitelisted urls. I'm not
> > sure where you'd find that in the renderer.
> 
> Yeah, you should be able to follow the FrameTreeNode::parent() chain and do
the
> check on FrameTreeNode::current_origin().  You'd need to do some additional
work
> to get isSecureContext's scheme whitelist here, but seems like
> secureContextBypassingSchemes() ultimately comes from 
> GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common,
so
> available in the browser process.  So it's actually isn't that bad, but you'd
> need some extra delegate plumbing to get to it from content/.
> 
> What seems harder is that the origin we have on FrameTreeNodes is a
url::Origin,
> which doesn't have all the information from a SecurityOrigin.  We do have a
> trustworthiness bit in FrameTreeNode::current_replication_state(),
> has_potentially_trustworthy_unique_origin, but it isn't actually equivalent to
> SecurityOrigin::isPotentiallyTrustworthy.  It's only meaningful for unique
> origins (i.e., it indicates potential trustworthiness for a sandboxed iframe),
> and you'd still need to either do all the other work in
> SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or
replicate
> the full value of isPotentiallyTrustworthy() from the renderer.  Overall, all
> this seems non-trivial and I'd vote for doing it in a separate CL.
> 
> CC-ing estark@, who's worked on isSecureContext plumbing recently and may have
> more thoughts on getting it to work from the browser process.

Sorry yes I meant "FrameTreeNode doesn't have the security info". But I think
the latest patchset is OK doing the walk renderer-side, for the PlzNavigation
case?

https://codereview.chromium.org/2009453002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2009453002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:219: if
(new_provider_id == kInvalidServiceWorkerVersionId)
On 2016/05/26 20:37:11, Marijn Kruisselbrink wrote:
> Shouldn't this be kInvalidServiceWorkerProviderId rather than VersionId? I
know
> they're all -1, but the types are different.

Done. Bad autocomplete choice.

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool
canHaveSecureChild(WebString& errorMessage) const;
On 2016/05/26 22:13:35, alexmos wrote:
> Do we need both this and WebDocument::isSecureContext in the public API? 
Seems
> WebDocument::isSecureContext is only used in plugins, where it's called on the
> plugin container frame.  Could that just use canHaveSecureChild?

I think there's two problems there:
1) isSecureContext allows for scheme exceptions and canHaveSecureChild doesn't.
2) A Document can be frameless (apparently. the comments for WebDocument::frame
say so). So document.isSecureContext() can't be transformed to
document.frame()->canHaveSecureChild().

1) could probably be resolved by refactoring to a common method, but 2) looks
more difficult.

To be more specific, IIRC, the plugin situation looks like:
- Frame
  - Document
    ...
      - plugin container (Element)
        - plugin instance

It's calling isSecureContext on the document. So it can be whitelisted by
scheme. If we instead call Frame::canHaveSecureChild, the scheme whitelist
doesn't happen. And we might not have a frame.

Probably canHaveSecureChild should be renamed to something like
canHaveSecureChildFrameIgnoringSchemeExceptions.

Having said all that, the plugin check is just for metrics and isn't a security
check. So maybe it's ok being slightly "wrong" there? +raymes

> Also, since both uses don't care about the error message, maybe we don't need
it
> in the public API either?

I can certainly remove it.

[alexmos, 2016-05-27 22:44:30]

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool
canHaveSecureChild(WebString& errorMessage) const;
On 2016/05/27 05:47:04, falken wrote:
> On 2016/05/26 22:13:35, alexmos wrote:
> > Do we need both this and WebDocument::isSecureContext in the public API? 
> Seems
> > WebDocument::isSecureContext is only used in plugins, where it's called on
the
> > plugin container frame.  Could that just use canHaveSecureChild?
> 
> I think there's two problems there:
> 1) isSecureContext allows for scheme exceptions and canHaveSecureChild
doesn't.
> 2) A Document can be frameless (apparently. the comments for
WebDocument::frame
> say so). So document.isSecureContext() can't be transformed to
> document.frame()->canHaveSecureChild().
> 
> 1) could probably be resolved by refactoring to a common method, but 2) looks
> more difficult.
> 
> To be more specific, IIRC, the plugin situation looks like:
> - Frame
>   - Document
>     ...
>       - plugin container (Element)
>         - plugin instance
> 
> It's calling isSecureContext on the document. So it can be whitelisted by
> scheme. If we instead call Frame::canHaveSecureChild, the scheme whitelist
> doesn't happen. And we might not have a frame.
> 
> Probably canHaveSecureChild should be renamed to something like
> canHaveSecureChildFrameIgnoringSchemeExceptions.
> 
> Having said all that, the plugin check is just for metrics and isn't a
security
> check. So maybe it's ok being slightly "wrong" there? +raymes

Yeah, I'm curious if the plugin case cares about scheme exceptions at all. 
Looks like it was added (by raymes@ in r329895) before scheme exceptions were
introduced (by jww@ in r354650), so I'll let jww@ and raymes@ comment. :)

[jww, 2016-05-28 01:35:14]

On 2016/05/27 22:44:30, alexmos wrote:
>
https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebFrame.h (right):
> 
>
https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool
> canHaveSecureChild(WebString& errorMessage) const;
> On 2016/05/27 05:47:04, falken wrote:
> > On 2016/05/26 22:13:35, alexmos wrote:
> > > Do we need both this and WebDocument::isSecureContext in the public API? 
> > Seems
> > > WebDocument::isSecureContext is only used in plugins, where it's called on
> the
> > > plugin container frame.  Could that just use canHaveSecureChild?
> > 
> > I think there's two problems there:
> > 1) isSecureContext allows for scheme exceptions and canHaveSecureChild
> doesn't.
> > 2) A Document can be frameless (apparently. the comments for
> WebDocument::frame
> > say so). So document.isSecureContext() can't be transformed to
> > document.frame()->canHaveSecureChild().
> > 
> > 1) could probably be resolved by refactoring to a common method, but 2)
looks
> > more difficult.
> > 
> > To be more specific, IIRC, the plugin situation looks like:
> > - Frame
> >   - Document
> >     ...
> >       - plugin container (Element)
> >         - plugin instance
> > 
> > It's calling isSecureContext on the document. So it can be whitelisted by
> > scheme. If we instead call Frame::canHaveSecureChild, the scheme whitelist
> > doesn't happen. And we might not have a frame.
> > 
> > Probably canHaveSecureChild should be renamed to something like
> > canHaveSecureChildFrameIgnoringSchemeExceptions.
> > 
> > Having said all that, the plugin check is just for metrics and isn't a
> security
> > check. So maybe it's ok being slightly "wrong" there? +raymes
> 
> Yeah, I'm curious if the plugin case cares about scheme exceptions at all. 
> Looks like it was added (by raymes@ in r329895) before scheme exceptions were
> introduced (by jww@ in r354650), so I'll let jww@ and raymes@ comment. :)

Definitely a question for raymes@. I'm very wary of adding a separate security
check that mimics but is slightly different from isSecureContext. Also, why does
canHaveSecureChildFrame ignore scheme exceptions? Namely, this ignores
extensions, I think, and we generally do want to treat them as secure.

[jww, 2016-05-28 01:35:25]

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/child/service_w...
content/child/service_worker/service_worker_network_provider.cc:86: while
(parent && should_create_provider_for_window) {
On 2016/05/25 04:09:45, falken wrote:
> On 2016/05/25 01:33:34, falken wrote:
> > On 2016/05/24 18:06:03, jww wrote:
> > > On 2016/05/24 17:03:35, Marijn Kruisselbrink wrote:
> > > > On 2016/05/24 at 13:16:37, falken wrote:
> > > > > This ancestor walk should probably be some utility function like
> > > > Document::isSecureContext(). One subtle difference is we shouldn't check
> for
> > > > scheme exceptions like Document does, since those shouldn't be applied
to
> > > child
> > > > frames, which is what we care about here.
> > > > 
> > > > Also keep in mind that to truly match what is specified in
> > > > https://w3c.github.io/webappsec-secure-contexts/#settings-object you
need
> to
> > > not
> > > > only exclude iframes inside insecure contexts, but also top-level
windows
> > > opened
> > > > by insecure contexts without no-opener.
> > > 
> > > I have a strong preference to factor this out into a shared utility
function
> > > that Document and this both use. It will be much easier to track changes,
> and
> > > even to just track that this check exists at all. This would also probably
> > give
> > > you a helpful error message to use, no?
> > > 
> > > Also, why don't you check if the current origin is potentially
trustworthy?
> > That
> > > is, you only start checking the child frames. I think this is related to
> mek's
> > > comment.
> > 
> > I can't start with the current frame because document is not yet created.
> > getSecurityOrigin() calls frame()->securityContext()->getSecurityOrigin(),
but
> > securityContext() is just document(), so it blows up.
> > 
> > I probably do need to check the current frame to address mek's point. Ideas?
> 
> Does Chrome currently check for insecure opener when deciding whether a
context
> is secure? It looks like Document::isSecureContext() doesn't check it. If I do
> window.open from an http page to an https page, I can register a service
worker
> from the opened page.

That's a great question. I believe the answer is "no", we don't do that yet.

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:276: bool
WebFrame::canHaveSecureChild(WebString& errorMessage) const
Can you add some unit tests to this, similar to what we have for
SecurityOrigin::isPotentiallyTrustworthy()?

[raymes, 2016-05-30 01:08:08]

+cc timwillis

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:555: BLINK_EXPORT bool
canHaveSecureChild(WebString& errorMessage) const;
The plugin case is just for metrics - we were just testing how frequently
plugins were loaded in secure contexts (that is, it's src is https and it's
parents are https). If we get something which approximates that it should be
fine especially if it simplifies code.

jww: Do we even still need those metrics? timwillis may know.

The metrics are: Pepper.PluginContextSecurity.TCPConnect and
Pepper.PluginContextSecurity.UDPBind

[falken, 2016-05-30 10:20:53]

I think jww is right: it's incorrect to ignore scheme exceptions here, since
extension service workers are expected to function inside insecure contexts.

That means the current document url must be taken into account. I’m wary,
though, of attempting to do so in SWNetworkProvider before we have a document.
One problem is redirects. If a chrome://extension could redirect to an https
context (it probably can’t), something could sneak past the check with:
1. An http frame embeds a frame with a scheme exception 
2. SWNetworkProvider treats the frame as secure, and sends the IPC to create a
SWProviderHost
3. SWControlleeRequestHandler intercepts the request
4. The request redirects to an https resource
SWControlleeRequestHandler continues handling the request. It has no idea that
it’s no longer secure.

It’s also weird that empirically the datasource URL is often empty when
SWNetworkProvider is created. So seems too early to do the complete check ala
Document->isSecureContext().

I propose:
SWNetworkProvider does the ancestor walk and sends is_parent_frame_secure in the
IPC, where it’s stored by SWProviderHost
SWControlleeRequestHandler combines that bit and the document url to check
whether the context is secure.

The latest patchset is a WIPish patch that does that. It adds a test that
extensions work too. WDYT of this approach?

I understand the concerns about adding a separate security check that mimics but
is slightly different from isSecureContext. At least for the web layer, I’m
happy removing WebDocument::isSecureContext() if that’s OK for the plugin
usecase.

I’m also wondering about frame reparenting. Is it possible to go from a secure
context to an insecure one, without triggering a reload? I did some
investigation and it looks like moving an iframe will trigger a reload (I
believe due to https://bugs.webkit.org/show_bug.cgi?id=81590 following
https://bugs.webkit.org/show_bug.cgi?id=32848)

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:276: bool
WebFrame::canHaveSecureChild(WebString& errorMessage) const
On 2016/05/28 01:35:25, jww wrote:
> Can you add some unit tests to this, similar to what we have for
> SecurityOrigin::isPotentiallyTrustworthy()?

Didn't get to this in the latest patchset, but will do this.

[clamy, 2016-05-30 16:22:42]

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
should do an isSecureContext check too.
On 2016/05/27 05:47:03, falken wrote:
> On 2016/05/26 22:13:35, alexmos wrote:
> > On 2016/05/26 15:45:31, clamy wrote:
> > > On 2016/05/26 02:09:49, falken wrote:
> > > > On 2016/05/25 11:21:46, clamy wrote:
> > > > > On 2016/05/25 09:19:58, falken wrote:
> > > > > > On 2016/05/24 18:06:03, jww wrote:
> > > > > > > To ask a dumb question, why not add that check right now?
> > > > > > 
> > > > > > I didn't look too hard, but there's no WebFrame here. clamy, how
hard
> > > would
> > > > it
> > > > > > be to add a check here like the one in
> > service_worker_navigation_provider?
> > > > > 
> > > > > If you're checking for the sandbox flags, you can get them from the
> > > > > FrameTreeNode that NavigationRequest keeps a pointer to.
> > > > 
> > > > I want to do an ancestor walk over the frame tree, checking their
security
> > (I
> > > > think FrameTreeNode doesn't have that info). But I think the latest
patch
> > > > changed this so service_worker_network_provider can handle the
PlzNavigate
> > > case
> > > > well.
> > > 
> > > FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a
> > > FrameTreeNodeReplicationState that has some security info, including the
> > origin
> > > (and whether it's potentially trustworthy). The only thing missing to do a
> > check
> > > that matches the one in RenderFrame is the list of whitelisted urls. I'm
not
> > > sure where you'd find that in the renderer.
> > 
> > Yeah, you should be able to follow the FrameTreeNode::parent() chain and do
> the
> > check on FrameTreeNode::current_origin().  You'd need to do some additional
> work
> > to get isSecureContext's scheme whitelist here, but seems like
> > secureContextBypassingSchemes() ultimately comes from 
> > GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common,
> so
> > available in the browser process.  So it's actually isn't that bad, but
you'd
> > need some extra delegate plumbing to get to it from content/.
> > 
> > What seems harder is that the origin we have on FrameTreeNodes is a
> url::Origin,
> > which doesn't have all the information from a SecurityOrigin.  We do have a
> > trustworthiness bit in FrameTreeNode::current_replication_state(),
> > has_potentially_trustworthy_unique_origin, but it isn't actually equivalent
to
> > SecurityOrigin::isPotentiallyTrustworthy.  It's only meaningful for unique
> > origins (i.e., it indicates potential trustworthiness for a sandboxed
iframe),
> > and you'd still need to either do all the other work in
> > SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or
> replicate
> > the full value of isPotentiallyTrustworthy() from the renderer.  Overall,
all
> > this seems non-trivial and I'd vote for doing it in a separate CL.
> > 
> > CC-ing estark@, who's worked on isSecureContext plumbing recently and may
have
> > more thoughts on getting it to work from the browser process.
> 
> Sorry yes I meant "FrameTreeNode doesn't have the security info". But I think
> the latest patchset is OK doing the walk renderer-side, for the PlzNavigation
> case?

Probably. In any case, we'll have to make significant changes to how PlzNavigate
works with ServiceWorker, so it's quite possible that this code will go away
anyway.

[jww, 2016-05-31 19:26:41]

On 2016/05/30 16:22:42, clamy wrote:
>
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/navigation_request.cc (right):
> 
>
https://codereview.chromium.org/2009453002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigation_request.cc:304: // TODO(falken): This
> should do an isSecureContext check too.
> On 2016/05/27 05:47:03, falken wrote:
> > On 2016/05/26 22:13:35, alexmos wrote:
> > > On 2016/05/26 15:45:31, clamy wrote:
> > > > On 2016/05/26 02:09:49, falken wrote:
> > > > > On 2016/05/25 11:21:46, clamy wrote:
> > > > > > On 2016/05/25 09:19:58, falken wrote:
> > > > > > > On 2016/05/24 18:06:03, jww wrote:
> > > > > > > > To ask a dumb question, why not add that check right now?
> > > > > > > 
> > > > > > > I didn't look too hard, but there's no WebFrame here. clamy, how
> hard
> > > > would
> > > > > it
> > > > > > > be to add a check here like the one in
> > > service_worker_navigation_provider?
> > > > > > 
> > > > > > If you're checking for the sandbox flags, you can get them from the
> > > > > > FrameTreeNode that NavigationRequest keeps a pointer to.
> > > > > 
> > > > > I want to do an ancestor walk over the frame tree, checking their
> security
> > > (I
> > > > > think FrameTreeNode doesn't have that info). But I think the latest
> patch
> > > > > changed this so service_worker_network_provider can handle the
> PlzNavigate
> > > > case
> > > > > well.
> > > > 
> > > > FrameTreeNodes do allow you to walk over the FrameTree. Each of them has
a
> > > > FrameTreeNodeReplicationState that has some security info, including the
> > > origin
> > > > (and whether it's potentially trustworthy). The only thing missing to do
a
> > > check
> > > > that matches the one in RenderFrame is the list of whitelisted urls. I'm
> not
> > > > sure where you'd find that in the renderer.
> > > 
> > > Yeah, you should be able to follow the FrameTreeNode::parent() chain and
do
> > the
> > > check on FrameTreeNode::current_origin().  You'd need to do some
additional
> > work
> > > to get isSecureContext's scheme whitelist here, but seems like
> > > secureContextBypassingSchemes() ultimately comes from 
> > > GetSchemesBypassingSecureContextCheckWhitelist(), which is in
chrome/common,
> > so
> > > available in the browser process.  So it's actually isn't that bad, but
> you'd
> > > need some extra delegate plumbing to get to it from content/.
> > > 
> > > What seems harder is that the origin we have on FrameTreeNodes is a
> > url::Origin,
> > > which doesn't have all the information from a SecurityOrigin.  We do have
a
> > > trustworthiness bit in FrameTreeNode::current_replication_state(),
> > > has_potentially_trustworthy_unique_origin, but it isn't actually
equivalent
> to
> > > SecurityOrigin::isPotentiallyTrustworthy.  It's only meaningful for unique
> > > origins (i.e., it indicates potential trustworthiness for a sandboxed
> iframe),
> > > and you'd still need to either do all the other work in
> > > SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or
> > replicate
> > > the full value of isPotentiallyTrustworthy() from the renderer.  Overall,
> all
> > > this seems non-trivial and I'd vote for doing it in a separate CL.
> > > 
> > > CC-ing estark@, who's worked on isSecureContext plumbing recently and may
> have
> > > more thoughts on getting it to work from the browser process.
> > 
> > Sorry yes I meant "FrameTreeNode doesn't have the security info". But I
think
> > the latest patchset is OK doing the walk renderer-side, for the
PlzNavigation
> > case?
> 
> Probably. In any case, we'll have to make significant changes to how
PlzNavigate
> works with ServiceWorker, so it's quite possible that this code will go away
> anyway.

raymes@: Yes, I think we still care about those metrics since they affect the
future of HTTPS restrictions on Pepper plugins, which we could tighten
eventually, depending on these types of metrics.

falken: That sounds good to me, although I think alexmos@ really needs to give
confirmation.

[alexmos, 2016-06-01 01:30:10]

> falken: That sounds good to me, although I think alexmos@ really needs to give
> confirmation.

Yes, that plan sounds ok.  Since you're already doing part of the check on the
browser process side, I do wonder if it's just easier to also move the ancestor
walk there now (and use origins on FrameTreeNodes).  You'd need to replicate
isPotentiallyTrustworthy values for FrameTreeNode origins, but that might
actually be easier than what you had to add for is_parent_frame_secure.  Or does
ServiceWorkerControlleeRequestHandler::PrepareForMainResource run on the IO
thread, in which case that's not possible?  (Sorry, I'm out of my depth for 
service worker machinery.)

[alexmos, 2016-06-01 01:34:20]

alexmos@chromium.org changed reviewers:
+ dcheng@chromium.org

[alexmos, 2016-06-01 01:34:21]

> I’m also wondering about frame reparenting. Is it possible to go from a secure
> context to an insecure one, without triggering a reload? I did some
> investigation and it looks like moving an iframe will trigger a reload (I
> believe due to https://bugs.webkit.org/show_bug.cgi?id=81590 following
> https://bugs.webkit.org/show_bug.cgi?id=32848)

Right, I don't think that should be possible without a reload, as otherwise all
sorts of things would break in the OOPIF world, but +dcheng@ to verify (I wasn't
aware we even supported this at some point).

[Marijn Kruisselbrink, 2016-06-01 01:47:55]

On 2016/06/01 at 01:34:21, alexmos wrote:
> > I’m also wondering about frame reparenting. Is it possible to go from a
secure
> > context to an insecure one, without triggering a reload? I did some
> > investigation and it looks like moving an iframe will trigger a reload (I
> > believe due to https://bugs.webkit.org/show_bug.cgi?id=81590 following
> > https://bugs.webkit.org/show_bug.cgi?id=32848)
> 
> Right, I don't think that should be possible without a reload, as otherwise
all sorts of things would break in the OOPIF world, but +dcheng@ to verify (I
wasn't aware we even supported this at some point).

On the subject of going from a secure to an insecure comment, at some point it
was discussed that maybe setting window.domain could be made mutually exclusive
with secure contexts (setting document.domain would make a secure context
insecure, and accessing any API that would require a secure context would make
setting window.domain impossible). But I don't believe that anybody is actively
pursuing that idea. And it would probably also be really hard to actually
implement, so probably not worth worrying about.

[falken, 2016-06-01 03:20:11]

On 2016/06/01 01:30:10, alexmos wrote:
> > falken: That sounds good to me, although I think alexmos@ really needs to
give
> > confirmation.
> 
> Yes, that plan sounds ok.  Since you're already doing part of the check on the
> browser process side, I do wonder if it's just easier to also move the
ancestor
> walk there now (and use origins on FrameTreeNodes).  You'd need to replicate
> isPotentiallyTrustworthy values for FrameTreeNode origins, but that might
> actually be easier than what you had to add for is_parent_frame_secure.  Or
does
> ServiceWorkerControlleeRequestHandler::PrepareForMainResource run on the IO
> thread, in which case that's not possible?  (Sorry, I'm out of my depth for 
> service worker machinery.)

Yep, all of ServiceWorkerControlleeRequestHandler is on the IO thread. Are
FrameTreeNodes used on the UI thread only?

I'll push forward with the is_parent_frame_secure approach.

[falken, 2016-06-01 07:14:44]

alexmos, jww: please review again

I decided not to change the plugin callsite since I think people shouldn't try
to think of WebFrame::canChildBeSecure as a check for whether a context is
secure. I put some explanatory comments above canChildBeSecure.

[falken, 2016-06-02 01:13:06]

alexmos, jww: ping

[Marijn Kruisselbrink, 2016-06-02 22:44:44]

drive-by suggestion, as I'm trying to build on top of this change

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:220:
if (schemes.find(provider_host_->document_url().scheme()) ==
Would it make sense to combine both the is_parent_frame_secure and the checking
for whitelisted schemes into one helper method somewhere in SWProviderHost? I
did something like that in https://codereview.chromium.org/1914593002 where I'm
(still very much WIP) trying to apply the same secure context limitations to
when Link headers are allowed to install service workers for navigations.

[jww, 2016-06-02 22:56:45]

On 2016/06/02 22:44:44, Marijn Kruisselbrink wrote:
> drive-by suggestion, as I'm trying to build on top of this change
> 
>
https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
> File
content/browser/service_worker/service_worker_controllee_request_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
>
content/browser/service_worker/service_worker_controllee_request_handler.cc:220:
> if (schemes.find(provider_host_->document_url().scheme()) ==
> Would it make sense to combine both the is_parent_frame_secure and the
checking
> for whitelisted schemes into one helper method somewhere in SWProviderHost? I
> did something like that in https://codereview.chromium.org/1914593002 where
I'm
> (still very much WIP) trying to apply the same secure context limitations to
> when Link headers are allowed to install service workers for navigations.

lgtm, but please do what for alexmos's review as well.

[jww, 2016-06-02 22:57:07]

On 2016/06/02 22:56:45, jww wrote:
> On 2016/06/02 22:44:44, Marijn Kruisselbrink wrote:
> > drive-by suggestion, as I'm trying to build on top of this change
> > 
> >
>
https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
> > File
> content/browser/service_worker/service_worker_controllee_request_handler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
> >
>
content/browser/service_worker/service_worker_controllee_request_handler.cc:220:
> > if (schemes.find(provider_host_->document_url().scheme()) ==
> > Would it make sense to combine both the is_parent_frame_secure and the
> checking
> > for whitelisted schemes into one helper method somewhere in SWProviderHost?
I
> > did something like that in https://codereview.chromium.org/1914593002 where
> I'm
> > (still very much WIP) trying to apply the same secure context limitations to
> > when Link headers are allowed to install service workers for navigations.
> 
> lgtm, but please do what for alexmos's review as well.

"what" -> "wait"

[alexmos, 2016-06-02 23:54:47]

> Yep, all of ServiceWorkerControlleeRequestHandler is on the IO thread. Are
> FrameTreeNodes used on the UI thread only?

Yes.  We're planning to make some of this information available on the IO thread
(via FrameIOData, see some related discussion at
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/L1nkukFURro...),
and I hope this will allow us to clean this up and move that check entirely to
the browser process.  That's a longer-term effort though, so I think it's ok to
use this approach in the meantime.  LGTM with a few nits below.

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:681:
WebAccessibleResourcesInsecureIframe) {
Can you run this through the linux_site_isolation trybot, just to make sure this
is working correctly with --site-per-process?

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:695: page_url =
page_url.ReplaceComponents(replace_host_and_scheme);
Why not just use embedded_test_server()->GetURL("a.com", "/rest/of/path")?

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:219: if
(new_provider_id == kInvalidServiceWorkerVersionId)
Why is this needed?

https://codereview.chromium.org/2009453002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2009453002/diff/220001/content/common/service...
content/common/service_worker/service_worker_messages.h:205: // (Service
Workers).
Update this comment to describe |is_parent_frame_secure|.

[falken, 2016-06-03 08:22:03]

falken@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, tsepez@chromium.org

[falken, 2016-06-03 08:22:05]

Thank you all. Pulling in more owners:

horo: Can you review everything?
devlin: chrome/browser/extensions/service_worker_apitest.cc
tsepez: platform/weborigin/
dcheng: messages and web/
(as for message security: a compromised renderer could lie about
|is_parent_frame_secure|, but we normally rely on the renderer to do these
security checks like in Document::isSecureContext anyway)

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:681:
WebAccessibleResourcesInsecureIframe) {
On 2016/06/02 23:54:47, alexmos wrote:
> Can you run this through the linux_site_isolation trybot, just to make sure
this
> is working correctly with --site-per-process?

That trybot is in the CQ_INCLUDE_TRYBOT so the CQ will run it (unfortunately,
git-cl try doesn't seem to know about it).

BTW can I do this locally with:
$ browser_tests --site-per-process
--gtest_filter="ServiceWorkerTest.WebAccessibleResourcesInsecureIframe"
?

https://codereview.chromium.org/2009453002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:695: page_url =
page_url.ReplaceComponents(replace_host_and_scheme);
On 2016/06/02 23:54:47, alexmos wrote:
> Why not just use embedded_test_server()->GetURL("a.com", "/rest/of/path")?

Done. This is because I just copied jww's test in
https://codereview.chromium.org/1383483007

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:220:
if (schemes.find(provider_host_->document_url().scheme()) ==
On 2016/06/02 22:44:43, Marijn Kruisselbrink wrote:
> Would it make sense to combine both the is_parent_frame_secure and the
checking
> for whitelisted schemes into one helper method somewhere in SWProviderHost? I
> did something like that in https://codereview.chromium.org/1914593002 where
I'm
> (still very much WIP) trying to apply the same secure context limitations to
> when Link headers are allowed to install service workers for navigations.

I like that and ended up adopting your patch, thanks. I think we must use
OriginCanAccessServiceWorkers, which is slightly different from IsOriginSecure
(SW is HTTP/HTTPS only and has its own set of exceptions, but in practice that
is just chrome://extensions).

I think the SWProviderHost function needs to be called something like
IsSecureEnoughForServiceWorkers.

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2009453002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:219: if
(new_provider_id == kInvalidServiceWorkerVersionId)
On 2016/06/02 23:54:47, alexmos wrote:
> Why is this needed?

Ah, this was needed in the earlier patch that used
kInvalidServiceWorkerProviderId instead of passing is_parent_frame_secure. That
patch caused site isolation bot to fail without this code.

I still think there's a bug here because we can use
kInvalidServiceWorkerProviderId due to sandboxed iframes, in which case
TransferProviderHostIn will crash. But I'll fix that in a separate patch.

Reverted changes to this file.

https://codereview.chromium.org/2009453002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2009453002/diff/220001/content/common/service...
content/common/service_worker/service_worker_messages.h:205: // (Service
Workers).
On 2016/06/02 23:54:47, alexmos wrote:
> Update this comment to describe |is_parent_frame_secure|.

Done.

[horo, 2016-06-03 09:54:45]

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:650: return false;
Do we need the returned value?

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:88:
is_parent_frame_secure_ = is_parent_frame_secure;
Please add DCHECK() to ensure that set_parent_frame_secure() can't be called
after IsContextSecureForServiceWorker() is called.

https://codereview.chromium.org/2009453002/diff/280001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2009453002/diff/280001/content/public/browser...
content/public/browser/content_browser_client.h:599: virtual void
GetSchemesBypassingSecureContextCheckWhitelist(
We need comment.

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:295: return result;
nit: we don't need `result` variable.

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8730: // A document in an
insecure context can be considered
nit: strange line break.

[falken, 2016-06-03 12:10:45]

Thank you.

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:650: return false;
On 2016/06/03 09:54:44, horo wrote:
> Do we need the returned value?

Not anymore. Failed partial revert. Done.

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2009453002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:88:
is_parent_frame_secure_ = is_parent_frame_secure;
On 2016/06/03 09:54:45, horo wrote:
> Please add DCHECK() to ensure that set_parent_frame_secure() can't be called
> after IsContextSecureForServiceWorker() is called.

Done. I ended up making the bool an enum, which is cleaner anyway.

https://codereview.chromium.org/2009453002/diff/280001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2009453002/diff/280001/content/public/browser...
content/public/browser/content_browser_client.h:599: virtual void
GetSchemesBypassingSecureContextCheckWhitelist(
On 2016/06/03 09:54:45, horo wrote:
> We need comment.

Done.

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:295: return result;
On 2016/06/03 09:54:45, horo wrote:
> nit: we don't need `result` variable.

Done.

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2009453002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8730: // A document in an
insecure context can be considered
On 2016/06/03 09:54:45, horo wrote:
> nit: strange line break.

My vim sometimes gets in a state where it can't gq well. Done.

[Devlin, 2016-06-03 15:06:44]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2016-06-03 15:06:45]

service_worker_apitest.cc lgtm, but +lazyboy@ as FYI and in case he sees
something I missed.

[Tom Sepez, 2016-06-03 17:29:58]

https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:362: bool
SecurityOrigin::isPotentiallyTrustworthy(String* errorMessage) const
Why do we need to add this?  Can't the caller use the existing version and then
call the public isPotentiallyTrustworthyErrorMessage() as needed?

[horo, 2016-06-06 01:54:33]

lgtm

[falken, 2016-06-06 04:53:59]

Patchset #17 (id:340001) has been deleted

[falken, 2016-06-06 06:38:13]

Thanks, updated. Please take another look.

devlin, lazyboy: I made some changes in /service_worker_apitest.cc. I had
originally meant to refactor the two tests into a common function but didn't see
a way to do that without using a new test class, which seemed like overkill.
Since the tests are the exact same except for the secure vs insecure URL, I
decided to just change the existing test to use the insecure URL, since secure
URLs are exercise in other tests. Let me know if you'd rather have two separate
tests, or have another idea.

https://codereview.chromium.org/2009453002/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:696: page_url =
page_url.ReplaceComponents(replace_host_and_scheme);
Oops, I meant to delete the use of GURL::Replacements earlier. Done now.

https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2009453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:362: bool
SecurityOrigin::isPotentiallyTrustworthy(String* errorMessage) const
On 2016/06/03 17:29:58, Tom Sepez wrote:
> Why do we need to add this?  Can't the caller use the existing version and
then
> call the public isPotentiallyTrustworthyErrorMessage() as needed?

Done, removed. Yeah, we don't need it. I also removed the param from
Frame::canChildBeSecure so now only Document::isSecureContext is responsible for
populating the error message. I think the original idea was for the message to
be more detailed about the exact error, but it's currently always just
SecurityOrigin::isPotentiallyTrustworthyErrorMessage.

[kinuko, 2016-06-06 06:43:16]

nits only

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:96: }
nit: why do we always translate enum to bool / bool to enum here?

https://codereview.chromium.org/2009453002/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:560: // check whether a context is
secure.
nit: the term 'context' is pretty vague in general, "check whether the document
is a secure context" ?

[falken, 2016-06-06 06:56:55]

Updated.

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:96: }
On 2016/06/06 06:43:16, kinuko wrote:
> nit: why do we always translate enum to bool / bool to enum here?

I'm still a bit undecided about this. On the renderer-side, UNINITIALIZED is
never used.  The renderer knows if the frame is secure or insecure, and sends
that bool in the message.

On the browser-side...
in non-plz-navigate case: we create the provider host once we get the IPC
message.
in the plz-navigate case: we create the provider before we get the IPC message.
So we need UNINITIALIZED on the browser-side.

Having the IPC message use a bool restricts it to the allowed values, which is
maybe a nicer than the enum. It also is simpler to work with: you can use an
if/else instead of switch.

https://codereview.chromium.org/2009453002/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2009453002/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:560: // check whether a context is
secure.
On 2016/06/06 06:43:16, kinuko wrote:
> nit: the term 'context' is pretty vague in general, "check whether the
document
> is a secure context" ?

Yes I think there's some inconsistency here. Did some wordsmithing.

[kinuko, 2016-06-06 09:28:04]

(rs-ish) lgtm

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2009453002/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:96: }
On 2016/06/06 06:56:55, falken wrote:
> On 2016/06/06 06:43:16, kinuko wrote:
> > nit: why do we always translate enum to bool / bool to enum here?
> 
> I'm still a bit undecided about this. On the renderer-side, UNINITIALIZED is
> never used.  The renderer knows if the frame is secure or insecure, and sends
> that bool in the message.
> 
> On the browser-side...
> in non-plz-navigate case: we create the provider host once we get the IPC
> message.
> in the plz-navigate case: we create the provider before we get the IPC
message.
> So we need UNINITIALIZED on the browser-side.
> 
> Having the IPC message use a bool restricts it to the allowed values, which is
> maybe a nicer than the enum. It also is simpler to work with: you can use an
> if/else instead of switch.

I think we have mild preference for enum over bool, while I see that having
UNINITIALIZED only in limited cases is a bit unfortunate. I'll let it go now,
but reg: if/else condition basically always checking security_level == SECURE
seems to be equivalent too.

[falken, 2016-06-06 13:32:35]

falken@chromium.org changed reviewers:
- raymes@chromium.org, tsepez@chromium.org

[falken, 2016-06-06 13:32:37]

Thanks. Refreshing reviewer roster:
+dcheng: web/ and messages
+devlin/lazyboy: changes to service_worker_apitest.cc since last review

(-tsepez, -raymes as the relevant files are no longer changed. Thanks for
looking.)

[Devlin, 2016-06-06 15:20:57]

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service
workers must be able to control insecure contexts. So set
I don't quite understand this comment.  I'm assuming "insecure context" refers
to a.com?  The extension isn't actually controlling the insecure context at all
(we don't allow extensions to serve up other origins) - it just controls and
extension page (secure context) iframed within an insecure context.  Right?  Or
am I missing something?

[Marijn Kruisselbrink, 2016-06-06 17:31:20]

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service
workers must be able to control insecure contexts. So set
On 2016/06/06 at 15:20:57, Devlin wrote:
> I don't quite understand this comment.  I'm assuming "insecure context" refers
to a.com?  The extension isn't actually controlling the insecure context at all
(we don't allow extensions to serve up other origins) - it just controls and
extension page (secure context) iframed within an insecure context.  Right?  Or
am I missing something?

An iframe within an insecure context is itself an insecure context, even if it
is served from a secure scheme like https (see example 7 in
https://w3c.github.io/webappsec-secure-contexts/#examples-framed). But yes, it
does seem like the comment could do a better job explaining what this code is
doing (probably by mentioning iframes inside insecure contexts rather than just
insecure contexts).

[falken, 2016-06-07 01:15:30]

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2009453002/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:638: // Extension service
workers must be able to control insecure contexts. So set
On 2016/06/06 17:31:20, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 15:20:57, Devlin wrote:
> > I don't quite understand this comment.  I'm assuming "insecure context"
refers
> to a.com?  The extension isn't actually controlling the insecure context at
all
> (we don't allow extensions to serve up other origins) - it just controls and
> extension page (secure context) iframed within an insecure context.  Right? 
Or
> am I missing something?
> 
> An iframe within an insecure context is itself an insecure context, even if it
> is served from a secure scheme like https (see example 7 in
> https://w3c.github.io/webappsec-secure-contexts/#examples-framed). But yes, it
> does seem like the comment could do a better job explaining what this code is
> doing (probably by mentioning iframes inside insecure contexts rather than
just
> insecure contexts).

Thanks, I revised the comment. What do you think?

[Devlin, 2016-06-07 01:17:19]

On 2016/06/07 01:15:30, falken wrote:
> Thanks, I revised the comment. What do you think?

Much clearer, thanks. :)

lgtm

[dcheng, 2016-06-07 02:21:20]

blink/ipc lgtm but two concerns that I'd like to see addressed in followups

1) I hope that we can (eventually?) enforce this check in the browser process
rather than relying on the renderer, since it can lie about the new bool.

2) I'm not a fan of the new Blink API.
- It's confusing to have canHaveSecureChild() and isSecureContext() exposed in
the public API
- It's also confusing that one is on Frame and the other is on Document.
canHaveSecureChild() is a property of a Document / RemoteSecurityContext, not
the Frame.
- It should be unnecessary to walk the ancestors each time this is called. The
parents of a frame cannot change (if they do, the Frame/Document is no longer
attached), nor can the security origin of a Document change. Instead, these bits
ought to only be determined once, when the context is initialized.

(1) is hard to address in the near-term (something something FrameIOData?). (2)
is something I'm hoping we can address sooner though.

https://codereview.chromium.org/2009453002/diff/420001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2009453002/diff/420001/content/public/browser...
content/public/browser/content_browser_client.h:603: std::set<std::string>*
schemes) {}
I guess this is just following existing convention, but note that it's fine to
return this by value: copy elision will generally prevent us from copying
anyway. If I saw something like this (without looking at the declaration), I
might think that it could return a bool or error or something else like that.

[falken, 2016-06-07 03:14:29]

Thanks. I made one minor change to Document to simplify the errorMessage
population. Will put through CQ.

https://codereview.chromium.org/2009453002/diff/420001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2009453002/diff/420001/content/public/browser...
content/public/browser/content_browser_client.h:603: std::set<std::string>*
schemes) {}
On 2016/06/07 02:21:20, dcheng wrote:
> I guess this is just following existing convention, but note that it's fine to
> return this by value: copy elision will generally prevent us from copying
> anyway. If I saw something like this (without looking at the declaration), I
> might think that it could return a bool or error or something else like that.

Thanks I see. I decided to keep this for consistency with the existing
GetSchemesBypassingSecureContextCheckWhitelist.

[falken, 2016-06-07 03:15:21]

The CQ bit was checked by falken@chromium.org

[falken, 2016-06-07 03:15:25]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
alexmos@chromium.org, jww@chromium.org, horo@chromium.org, kinuko@chromium.org,
rdevlin.cronin@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2009453002/#ps440001
(title: "refactor errorMessage")

[commit-bot: I haz the power, 2016-06-07 03:15:47]

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

[commit-bot: I haz the power, 2016-06-07 05:06:26]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-06-07 05:06:29]

Committed patchset #21 (id:440001)

[commit-bot: I haz the power, 2016-06-07 05:08:41]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/59a2e54eeb0e61971a0c27c44c54dd0c30b5d06d
Cr-Commit-Position: refs/heads/master@{#398229}
==========

[commit-bot: I haz the power, 2016-06-07 05:08:43]

Patchset 21 (id:??) landed as
https://crrev.com/59a2e54eeb0e61971a0c27c44c54dd0c30b5d06d
Cr-Commit-Position: refs/heads/master@{#398229}

[dcheng, 2016-06-08 18:03:53]

A revert of this CL (patchset #21 id:440001) has been created in
https://codereview.chromium.org/2055433002/ by dcheng@chromium.org.

The reason for reverting is:
ServiceWorkerProviderHost::SetControllerVersionAttribute CHECK is firing in
release builds: https://crbug.com/618365.