Service worker bypass for resource requests from suborigins

Service worker bypass for resource requests from suborigins

This implements a bypass of service workers for resource requests that
originate in a service worker. They should not be intercepted by service
workers that have been installed by the physical origin, and they cannot
install their own service workers, so instead they should bypass the
origin's service worker if it exists.

BUG=336894
Committed: https://crrev.com/765fbff4e0bbe8be4c7c2c7393aa8e2b674ecc3e
Cr-Commit-Position: refs/heads/master@{#420143}

[jww, 2016-09-16 00:43:17]

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

[jww, 2016-09-16 00:43:18]

falken@, can you take a look at this to see if I'm on the right track here? I
have a feeling I might need to also add a check to
ResourceFetcher::RevalidationPolicy to re-fetch the resource if it was fetched
via an SW the first time.

[jww, 2016-09-17 00:00:10]

On 2016/09/16 00:43:18, jww wrote:
> falken@, can you take a look at this to see if I'm on the right track here? I
> have a feeling I might need to also add a check to
> ResourceFetcher::RevalidationPolicy to re-fetch the resource if it was fetched
> via an SW the first time.

I was sort of right in my original message :-) I definitely needed to bypass the
SW-specific mem cache, but it had nothing to do with RevalidationPolicy. I've
added a MemCache test and addressed that part of the bypass as well.

[jww, 2016-09-19 18:06:17]

jww@chromium.org changed reviewers:
+ yoav@yoav.ws

[jww, 2016-09-19 18:06:18]

yoav@yoav.ws: Would you mind taking a look at these changes with respect to
suborigins (https://w3c.github.io/webappsec-suborigins/)? I still want falken@
to take a look, since I talked to him initially, but he's quite busy, and I'd
love to get a review to make sure I'm not crazy here. Thanks!

[falken, 2016-09-20 03:48:57]

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js
(right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:6:
event.respondWith(fetch.event.request);
BTW event.respondWith(request) and not calling respondWith at all are supposed
to have the same behavior but go through much different code paths. To test
thoroughly you might want to test both.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:7:
event.respondWith(Promise.reject(Error("Rejecting all requests")));
I must be missing something but why are these rejects needed? All the URLs i see
in the test have "bypass" in the URL.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html
(right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html:12:
var t_nonsuborigin = async_test("Service worker can access fetch events from a
non-suborigin on the origin");
nit: (throughout this file) single-quote JS strings for consistency

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1037:
request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::All);
I know this is Blink but can we comment here why sw are being skipped?

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:599: return false;
Why is this needed?

I'm a little concerned that the document will have a
navigator.serviceWorker.controller, but all requests must skip the service
worker. I'm hoping the skip_service_worker flag is robust enough to handle this
state (we've previously had issues around CORS logic when the renderer's
expectations of whether SW touches a request are violated). It might be cleaner
to just not have a navigator.serviceWorker.controller but doing that is also
really tricky because we set it when processing the navigation request in the
browser process, before the response comes in. And we'd also have to ensure SW
can't later claim() the document.

Not considering implementation difficulty, is the desired behavior to have a
.controller when on a suborigin?

[jww, 2016-09-20 04:27:47]

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js
(right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:6:
event.respondWith(fetch.event.request);
On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> BTW event.respondWith(request) and not calling respondWith at all are supposed
> to have the same behavior but go through much different code paths. To test
> thoroughly you might want to test both.

Acknowledged.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:7:
event.respondWith(Promise.reject(Error("Rejecting all requests")));
On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> I must be missing something but why are these rejects needed? All the URLs i
see
> in the test have "bypass" in the URL.

It's a little tricky with what's going on here, but you'll note that in
suborigin-service-worker-fetch-event.html, the iframes are loaded with:
   iframe2.src =
"resources/fetch-resource.php?resource=/resources/dummy.txt&id=nonsuborigin&bypass";
This loads an iframe and, indeed, bypass is one of the URL parameters, so it
should bypass the error. However, the iframe itself takes the 'resource'
parameter (in this case, /resources/dummy.txt), and loads it. It's *that*
resource that does not have 'bypass' in the URL, and it's that resource we are
checking to see if it was intercepted.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html
(right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html:12:
var t_nonsuborigin = async_test("Service worker can access fetch events from a
non-suborigin on the origin");
On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> nit: (throughout this file) single-quote JS strings for consistency

Done.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1037:
request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::All);
On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> I know this is Blink but can we comment here why sw are being skipped?

Hahaha, I like you're style :-) I always get ticked off with the lack of
comments. Done.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:599: return false;
On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> Why is this needed?
> 
> I'm a little concerned that the document will have a
> navigator.serviceWorker.controller, but all requests must skip the service
> worker. I'm hoping the skip_service_worker flag is robust enough to handle
this
> state (we've previously had issues around CORS logic when the renderer's
> expectations of whether SW touches a request are violated). It might be
cleaner
> to just not have a navigator.serviceWorker.controller but doing that is also
> really tricky because we set it when processing the navigation request in the
> browser process, before the response comes in. And we'd also have to ensure SW
> can't later claim() the document.
> 
> Not considering implementation difficulty, is the desired behavior to have a
> .controller when on a suborigin?
No, and in fact in a previous CL, we disabled the JS API to
navigator.serviceWorker. See
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

From my perspective, I'd rather stick with easier implementation perspective for
now, and deal with any bugs that come up (which, I think, you're saying is to
use skip_service_worker), and then as suborigins become more robust, we go one
of two routes:
   * Completely remove the controller.
   * If a future version of the spec allows for service workers on suborigins,
implement a correct controller behavior.

Does that make sense as an approach?

[falken, 2016-09-20 04:52:52]

lgtm with nits

Regarding CL description: "resource requests that
originate in a service worker" should be something like "resource requests from
a document in a suborigin"? "originate in a service worker" usually means (to me
at least) fetch() called inside the SW.

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2343053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:599: return false;
On 2016/09/20 04:27:47, jww wrote:
> On 2016/09/20 03:48:57, falken (behind on reviews) wrote:
> > Why is this needed?
> > 
> > I'm a little concerned that the document will have a
> > navigator.serviceWorker.controller, but all requests must skip the service
> > worker. I'm hoping the skip_service_worker flag is robust enough to handle
> this
> > state (we've previously had issues around CORS logic when the renderer's
> > expectations of whether SW touches a request are violated). It might be
> cleaner
> > to just not have a navigator.serviceWorker.controller but doing that is also
> > really tricky because we set it when processing the navigation request in
the
> > browser process, before the response comes in. And we'd also have to ensure
SW
> > can't later claim() the document.
> > 
> > Not considering implementation difficulty, is the desired behavior to have a
> > .controller when on a suborigin?
> No, and in fact in a previous CL, we disabled the JS API to
> navigator.serviceWorker. See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> 
> From my perspective, I'd rather stick with easier implementation perspective
for
> now, and deal with any bugs that come up (which, I think, you're saying is to
> use skip_service_worker), and then as suborigins become more robust, we go one
> of two routes:
>    * Completely remove the controller.
>    * If a future version of the spec allows for service workers on suborigins,
> implement a correct controller behavior.
> 
> Does that make sense as an approach?

Makes sense to me. Let's go with this and see.

BTW disabling the JS API doesn't totally prevent registering a SW. Do you also
stop Link rel=serviceworker
https://www.chromestatus.com/feature/5682681044008960?

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:7:
event.respondWith(Promise.reject(Error("Rejecting all requests")));
single quote JS

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html:47:
}
nit: add semicolon

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-no-xorigin-caching.html
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-no-xorigin-caching.html:11:
var t_is_mem_cached = async_test("Service worker responses aren't cached for
suborigins");
nit (sorry): three double quoted JS strings in this file

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:599: return false;
OK the rationale for allowing .controller makes sense. But it's not obvious to
me why we need this code, I'd naively expect the setSkipServiceWorker() call to
do everything. Was this needed for the mem cache thing? Can you add a brief
comment about that?

[jww, 2016-09-20 17:52:27]

Thanks, Matt!

Yoav, would you mind OWNER reviewing core/*? Thanks!

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/resources/sw-reject-all-with-error.js:7:
event.respondWith(Promise.reject(Error("Rejecting all requests")));
On 2016/09/20 04:52:52, falken wrote:
> single quote JS

Done.

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-fetch-event.html:47:
}
On 2016/09/20 04:52:52, falken wrote:
> nit: add semicolon

Done.

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-no-xorigin-caching.html
(right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-no-xorigin-caching.html:11:
var t_is_mem_cached = async_test("Service worker responses aren't cached for
suborigins");
On 2016/09/20 04:52:52, falken wrote:
> nit (sorry): three double quoted JS strings in this file

Done.

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2343053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:599: return false;
On 2016/09/20 04:52:52, falken wrote:
> OK the rationale for allowing .controller makes sense. But it's not obvious to
> me why we need this code, I'd naively expect the setSkipServiceWorker() call
to
> do everything. Was this needed for the mem cache thing? Can you add a brief
> comment about that?

Yes, it was needed for the mem cache bit. Otherwise, when the mem cache ID is
reached for, if this returns true, it will set the mem cache id to be that of
the service worker controller (and thus return any resources cached by the
service worker, which is bad since we want to bypass the service worker for
suborigins). See ResourceFetcher::getCacheIdentifier() to see where this occurs.

Adding a comment to clarify all of this.

[Yoav Weiss, 2016-09-21 15:13:09]

core/ (RS)LGTM. I don't feel that I understand the SW mechanics enough, but
trust that falken@ does

>BTW disabling the JS API doesn't totally prevent registering a SW. Do you also
stop Link rel=serviceworker
https://www.chromestatus.com/feature/5682681044008960?

I didn't see you address that comment from falken@, which seems rather
important. Do you intend to address it in a followup CL?

https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (left):

https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:596:
ASSERT(m_documentLoader || frame()->loader().documentLoader());
I think you'd need to rebase as this is already DCHECK in my checkout

[Yoav Weiss, 2016-09-21 15:14:35]

Also, apologies for the late review. TPAC week, so meetings and beers got in the
way...

[jww, 2016-09-21 16:23:41]

On 2016/09/21 15:13:09, Yoav Weiss wrote:
> core/ (RS)LGTM. I don't feel that I understand the SW mechanics enough, but
> trust that falken@ does
> 
> >BTW disabling the JS API doesn't totally prevent registering a SW. Do you
also
> stop Link rel=serviceworker
> https://www.chromestatus.com/feature/5682681044008960
> 
> I didn't see you address that comment from falken@, which seems rather
> important. Do you intend to address it in a followup CL?
Yikes, I missed that comment! Yes, I'll address that in a followup.
> 
>
https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (left):
> 
>
https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:596:
> ASSERT(m_documentLoader || frame()->loader().documentLoader());
> I think you'd need to rebase as this is already DCHECK in my checkout

[jww, 2016-09-21 16:37:53]

The CQ bit was checked by jww@chromium.org

[jww, 2016-09-21 16:37:54]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2343053002/#ps80001
(title: "Rebase on ToT")

[jww, 2016-09-21 16:38:12]

Thanks for the review, Yoav, and no worries on the  timing; I figured you were
at TPAC.

https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (left):

https://codereview.chromium.org/2343053002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:596:
ASSERT(m_documentLoader || frame()->loader().documentLoader());
On 2016/09/21 15:13:09, Yoav Weiss wrote:
> I think you'd need to rebase as this is already DCHECK in my checkout

Done.

[commit-bot: I haz the power, 2016-09-21 16:38:19]

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

[commit-bot: I haz the power, 2016-09-21 18:14:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:14:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jww, 2016-09-21 18:45:05]

The CQ bit was checked by jww@chromium.org

[jww, 2016-09-21 18:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2343053002/#ps100001
(title: "Fix broken test")

[commit-bot: I haz the power, 2016-09-21 18:45:36]

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

[commit-bot: I haz the power, 2016-09-21 20:11:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-21 20:15:46]

Description was changed from

==========
Service worker bypass for resource requests from suborigins

This implements a bypass of service workers for resource requests that
originate in a service worker. They should not be intercepted by service
workers that have been installed by the physical origin, and they cannot
install their own service workers, so instead they should bypass the
origin's service worker if it exists.

BUG=336894
==========

to

==========
Service worker bypass for resource requests from suborigins

This implements a bypass of service workers for resource requests that
originate in a service worker. They should not be intercepted by service
workers that have been installed by the physical origin, and they cannot
install their own service workers, so instead they should bypass the
origin's service worker if it exists.

BUG=336894

Committed: https://crrev.com/765fbff4e0bbe8be4c7c2c7393aa8e2b674ecc3e
Cr-Commit-Position: refs/heads/master@{#420143}
==========

[commit-bot: I haz the power, 2016-09-21 20:15:47]

Patchset 6 (id:??) landed as
https://crrev.com/765fbff4e0bbe8be4c7c2c7393aa8e2b674ecc3e
Cr-Commit-Position: refs/heads/master@{#420143}