Limit foreign fetch to only intercept requests made from secure contexts.

Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-foreign-fetch-request-algorithm

BUG=540509
Committed: https://crrev.com/dd933bda0baa6a13ab0120f0056a2b783e459efb
Committed: https://crrev.com/c9ce61d2dda342354302a9337910094a835213d2
Cr-Original-Commit-Position: refs/heads/master@{#398752}
Cr-Commit-Position: refs/heads/master@{#399870}

[Marijn Kruisselbrink, 2016-06-02 17:44:26]

Description was changed from

==========
Limit foreign fetch to intercepting requests made from secure contexts.

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

to

==========
Limit foreign fetch to intercepting requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-06-02 17:45:01]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2016-06-02 17:47:40]

Description was changed from

==========
Limit foreign fetch to intercepting requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509
==========

to

==========
Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-06-02 18:35:24]

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

[kinuko, 2016-06-03 09:17:16]

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler.cc (right):

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler.cc:68: return;
This makes me feel if we should just skip calling InitializeHandler if this is
false in resource_dispatcher_host_impl... any reason we don't do that?

https://codereview.chromium.org/2032653003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
(right):

https://codereview.chromium.org/2032653003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:181:
remote_url))
nit: indent slightly off?

[Marijn Kruisselbrink, 2016-06-06 19:23:34]

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler.cc (right):

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler.cc:68: return;
On 2016/06/03 at 09:17:16, kinuko wrote:
> This makes me feel if we should just skip calling InitializeHandler if this is
false in resource_dispatcher_host_impl... any reason we don't do that?

No particular reason, other than keeping all the checks in one place. Similarly
ResourceDispatcherHost could skip calling InitializeHandler (and
ServiceWorkerRequestHandler::InitializeHandler) when context_wrapper is null,
and for a few other checks that InitializeHandler does. So it's mostly a
question of how much you want ResourceDispatcherHost to know about service
worker specific checks.
(also in a future (still very much WIP) patch I'm working on to limit which
requests we process Link rel=serviceworker HTTP response headers for the same
initiated_in_secure_context flag is used for that as well, so ResourceDH won't
be just passing the flag on to InitializeHandler anymore then:
https://codereview.chromium.org/1914593002)

https://codereview.chromium.org/2032653003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
(right):

https://codereview.chromium.org/2032653003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:181:
remote_url))
On 2016/06/03 at 09:17:16, kinuko wrote:
> nit: indent slightly off?

Fixed

[kinuko, 2016-06-07 06:55:57]

lgtm

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler.cc (right):

https://codereview.chromium.org/2032653003/diff/20001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler.cc:68: return;
On 2016/06/06 19:23:34, Marijn Kruisselbrink wrote:
> On 2016/06/03 at 09:17:16, kinuko wrote:
> > This makes me feel if we should just skip calling InitializeHandler if this
is
> false in resource_dispatcher_host_impl... any reason we don't do that?
> 
> No particular reason, other than keeping all the checks in one place.
Similarly
> ResourceDispatcherHost could skip calling InitializeHandler (and
> ServiceWorkerRequestHandler::InitializeHandler) when context_wrapper is null,
> and for a few other checks that InitializeHandler does. So it's mostly a
> question of how much you want ResourceDispatcherHost to know about service
> worker specific checks.
> (also in a future (still very much WIP) patch I'm working on to limit which
> requests we process Link rel=serviceworker HTTP response headers for the same
> initiated_in_secure_context flag is used for that as well, so ResourceDH won't
> be just passing the flag on to InitializeHandler anymore then:
> https://codereview.chromium.org/1914593002)

Ah ok, makes sense.  Yeah having decision related logic here would be probably
better.

[Marijn Kruisselbrink, 2016-06-07 17:51:20]

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

[Marijn Kruisselbrink, 2016-06-07 17:51:20]

+dcheng for IPC OWNERS

[Marijn Kruisselbrink, 2016-06-08 16:07:29]

On 2016/06/07 at 17:51:20, Marijn Kruisselbrink wrote:
> +dcheng for IPC OWNERS

dcheng: ping?

[dcheng, 2016-06-08 18:08:01]

The CL looks fine. But I've noticed there's been a lot of SW CLs that depend on
plumbing a 'is secure' bit from the renderer to the browser. Do we have a
long-term plan for addressing this? FrameIOData, perhaps?

[Marijn Kruisselbrink, 2016-06-08 18:17:47]

On 2016/06/08 at 18:08:01, dcheng wrote:
> The CL looks fine. But I've noticed there's been a lot of SW CLs that depend
on plumbing a 'is secure' bit from the renderer to the browser. Do we have a
long-term plan for addressing this? FrameIOData, perhaps?

I don't know what FrameIOData is? A codesearch doesn't reveal anything other
than a comment referring to it... When I asked about checking for secure
contexts in the browser process  back in February
(https://groups.google.com/a/chromium.org/d/msg/net-dev/rLmvPUmVmVA/KX4GLAHnHgAJ)
the answer seemed to be "No, not in a trustworthy fashion". Work is being done
to enable this, but it's not done, and I'm not sure the status (this is a
superset of the Site Isolation work + PlzNavigate work)". I don't know what if
anything changed since then.

But pretty much all "is secure" checks are done purely in the renderer anyway,
so at least for now it doesn't seem so bad to "trust" the renderer to send this
bit to the browser.

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

On 2016/06/08 at 18:17:47, mek wrote:
> On 2016/06/08 at 18:08:01, dcheng wrote:
> > The CL looks fine. But I've noticed there's been a lot of SW CLs that depend
on plumbing a 'is secure' bit from the renderer to the browser. Do we have a
long-term plan for addressing this? FrameIOData, perhaps?
> 
> I don't know what FrameIOData is? A codesearch doesn't reveal anything other
than a comment referring to it... When I asked about checking for secure
contexts in the browser process  back in February
(https://groups.google.com/a/chromium.org/d/msg/net-dev/rLmvPUmVmVA/KX4GLAHnHgAJ)
the answer seemed to be "No, not in a trustworthy fashion". Work is being done
to enable this, but it's not done, and I'm not sure the status (this is a
superset of the Site Isolation work + PlzNavigate work)". I don't know what if
anything changed since then.
> 
> But pretty much all "is secure" checks are done purely in the renderer anyway,
so at least for now it doesn't seem so bad to "trust" the renderer to send this
bit to the browser.

FrameIOData doesn't exist yet, but it's the theoretical mechanism by which we'd
move these checks into the browser. We already have the replicated origin on
FrameTreeNodes, but it's only usable on the UI thread.

In any case, this CL LGTM. My main concern is if we have some way of tracking
all this stuff for long-term cleanup, but maybe that will happen as a natural
consequence of removing the "is secure" functions from the Blink public APi when
the time comes (assuming this is possible =).

[Marijn Kruisselbrink, 2016-06-08 18:36:53]

On 2016/06/08 at 18:22:03, dcheng wrote:
> On 2016/06/08 at 18:17:47, mek wrote:
> > On 2016/06/08 at 18:08:01, dcheng wrote:
> > > The CL looks fine. But I've noticed there's been a lot of SW CLs that
depend on plumbing a 'is secure' bit from the renderer to the browser. Do we
have a long-term plan for addressing this? FrameIOData, perhaps?
> > 
> > I don't know what FrameIOData is? A codesearch doesn't reveal anything other
than a comment referring to it... When I asked about checking for secure
contexts in the browser process  back in February
(https://groups.google.com/a/chromium.org/d/msg/net-dev/rLmvPUmVmVA/KX4GLAHnHgAJ)
the answer seemed to be "No, not in a trustworthy fashion". Work is being done
to enable this, but it's not done, and I'm not sure the status (this is a
superset of the Site Isolation work + PlzNavigate work)". I don't know what if
anything changed since then.
> > 
> > But pretty much all "is secure" checks are done purely in the renderer
anyway, so at least for now it doesn't seem so bad to "trust" the renderer to
send this bit to the browser.
> 
> FrameIOData doesn't exist yet, but it's the theoretical mechanism by which
we'd move these checks into the browser. We already have the replicated origin
on FrameTreeNodes, but it's only usable on the UI thread.
> 
> In any case, this CL LGTM. My main concern is if we have some way of tracking
all this stuff for long-term cleanup, but maybe that will happen as a natural
consequence of removing the "is secure" functions from the Blink public APi when
the time comes (assuming this is possible =).

Ah, I see. That all sounds interesting and useful. Yeah, those concerns make
sense, it would be unfortunate if all these things remain when they are no
longer needed. And even if somehow it doesn't become possible to remove the "is
secure" functions from the Blink public API, hopefully somebody would still go
through all the callsites to see if they are still needed...

[Marijn Kruisselbrink, 2016-06-08 18:36:56]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-06-08 18:37:27]

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

[Marijn Kruisselbrink, 2016-06-08 23:06:37]

The CQ bit was unchecked by mek@chromium.org

[Marijn Kruisselbrink, 2016-06-08 23:32:59]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-06-08 23:33:53]

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

[commit-bot: I haz the power, 2016-06-09 00:52:16]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-06-09 00:53:22]

Description was changed from

==========
Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509
==========

to

==========
Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509

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

[commit-bot: I haz the power, 2016-06-09 00:53:24]

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

[Marijn Kruisselbrink, 2016-06-09 03:41:20]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/2049723004/ by mek@chromium.org.

The reason for reverting is: ResourceRequest::initiated_in_secure_context should
have a default value, now it's causing failures on some MSAN bots .

[Marijn Kruisselbrink, 2016-06-15 08:45:50]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-06-15 08:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2032653003/#ps60001
(title: "properly initialize new ResourceRequest member")

[commit-bot: I haz the power, 2016-06-15 08:45:58]

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

[commit-bot: I haz the power, 2016-06-15 09:45:46]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-15 09:45:52]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 09:49:22]

Description was changed from

==========
Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509

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

to

==========
Limit foreign fetch to only intercept requests made from secure contexts.

Implements step 5 of "Handle Foreign Fetch" in [1]

[1]:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509

Committed: https://crrev.com/dd933bda0baa6a13ab0120f0056a2b783e459efb
Committed: https://crrev.com/c9ce61d2dda342354302a9337910094a835213d2
Cr-Original-Commit-Position: refs/heads/master@{#398752}
Cr-Commit-Position: refs/heads/master@{#399870}
==========

[commit-bot: I haz the power, 2016-06-15 09:49:24]

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