It seems to me that the test is wrong - XHR kicked off by https://web-platform.test:8444/service-workers/service-worker/resources/fetch-request-xhr-iframe.https.html should not be able to get cross-origin response from  https://www1.web-platform.test:8444.  Therefore, I plan to just add a timeout expectation to third_party/WebKit/LayoutTests/FlagExpectations/site-per-process in https://crrev.com/c/874752.

OTOH, maybe I am missing something here?  It seems weird that a test would be written with an expectation that cross-origin XHR returns response body.  Would such a test ever work on Chrome (even in absence of XSDB) or other browsers?

I double-checked that cross-origin XHR is blocked, by trying to XHR to http://tests.netsekure.org/ it out from a DevTools console of a http://netsekure.org/ frame.  But... maybe CORS handling is (should be?) different for service workers?  In the wpt/service-workers/service-worker/fetch-request-xhr.https.html test (cross_origin_get_header_test testcase), I see that blink::ResourceLoader::DetermineCORSStatus is called, but it returns early because of response.WasFetchedViaServiceWorker() - CORSStatus::kServiceWorkerSuccessful is returned, because response.ResponseTypeViaServiceWorker() == FetchResponseType::kDefault.

FWIW, I see that CORSStatus::kServiceWorkerSuccessful was introduced in r483013

Hmmm... I guess no security boundaries are really crossed in the test, because the cross-origin www1.web-platform.test resources is returned by a first-party (web-platform.test) service worker's fetch handler.  So - maybe XSDB shouldn't block such responses?  I am really confused now... :-(

Jake: Is this test correct?

I hope that Jake can confirm this, but I've been trying to read more on this topic and AFAICT returning cross-origin responses from a first-party service worker is WAI (e.g. see https://github.com/w3c/ServiceWorker/issues/130).  In theory, we could make XSDB ignore responses handled by service workers by having CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders return false if |response->head.was_fetched_via_service_worker|.

OTOH, I am not quite sure how third-party service workers (e.g. foreignfetch) should interact with XSDB.  I guess if response contains sensitive data (i.e. data that should not be exposed cross-origin), then it is a bug in the service worker (not in Chrome) if the service worker serves such data via foreignfetch.  Still, maybe XSDB should be active in this case, as a defence-in-depth (that would probably require plumbing the 1st-party-fetch-VS-3rd-party-foreign-fetch bit into network::ResourceResponseInfo, to supplement the |was_fetched_via_service_worker| field)?  WDYT?

Foreign fetch has been removed from the spec and impl, so there are only "first-party" service workers. 

I haven't looked into the test in detail, but yes a SW can return a cross-origin response (in a couple senses: the request doesn't need to have the same origin as the SW, and the response doesn't need to have the same origin as the request)

Yes, I believe the test is correct. The page is making a cross-origin request which the service worker intercepts. The service worker synthesizes a response and returns it, which is allowed. If the service worker instead actually tried to get the response from the origin and respond with it: respondWith(fetch(e.request.url)), the page should not be able to read the response.

What you probably want is to look at ResourceResponseInfo's |response_type_via_service_worker|. If the response is opaque, it should not be possible to read the response body.

By the way, Firefox seems to pass this test:
https://wpt.fyi/service-workers/service-worker/fetch-request-xhr.https.html

We are failing four of the test cases as seen here but it's not due to the cross-origin thing:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr.https-expected.txt?rcl=43b02f309ef49538984dbb2febeaa00206765e82

Comments 10-11: Thanks for the context.  I don't think we would be concerned about cases where the ServiceWorker synthesizes a response for a cross-origin request, because the cross-origin network response itself never needs to be given to the renderer process.

I'm not clear on how that relates to the test failure lukasza@ observed, though, which looks like it was affected when the renderer process received an empty response when XSDB (http://www.chromium.org/developers/design-documents/blocking-cross-site-documents) was in effect.

Can you clarify whether the ServiceWorker's renderer process needs to receive the body of a cross-origin network response?  (That would be a security concern if it's able to pull things like sensitive JSON files into the renderer process, so hopefully it's not needed.)

Marking restrict-view-security out of caution, but we might be able to undo that if we establish there's no issue here.

Yes I'm a little confused too. The test is doing this:

1) PAGE does XHR.
2) SERVICEWORKER intercepts the XHR and returns a synthesized response to PAGE.
3) PAGE reads the response.

I'm assuming that XSDB is interpreting the response from SERVICEWORKER as coming from cross-origin, since the request URL was cross-origin, and is clearing the response body.

Neither SERVICEWORKER nor PAGE should need to receive the body of a cross-origin network response (unless of course CORS is used to make it available).

RE: #c13

Correct - during the test XSDB is interpreting the response from SERVICEWORKER as coming from cross-origin and therefore XSDB is clearing the response body.


RE: #c11: hat you probably want is to look at ResourceResponseInfo's |response_type_via_service_worker|. If the response is opaque, it should not be possible to read the response body.

During the test I see that |response->head.response_type_via_service_worker| is |FetchResponseType::kDefault| (i.e. NOT FetchResponseType::kOpaque).

So, I am not sure if

1. XSDB should only clear the body of service-worker responses marked as FetchResponseType::kDefault?

VS

2. XSDB should clear the body of all service-worker responses?


If I do #1, then the fetch-request-xhr-iframe.https.html test will be broken in presence of XSDB (because the test expects to see the response body, but the response is kDefault, not kOpaque).

AFAICT (looking at service-worker/resources/fetch-request-xhr-iframe.https.html) the test doesn't request 'no-cors' mode (the test uses XMLHttpRequest rather than the new 'fetch' API) and therefore I don't think there is an expectation of receiving an 'opaque' response.


FWIW, I try to do #2 in https://chromium-review.googlesource.com/c/chromium/src/+/879348

Right, I recommend the opposite. If the response is opaque, the receiver shouldn't have access to the body, so I think XSDB should clear it. That is:
- If the service worker response is FetchResponseType::kOpaque (or FetchResponseType::kOpaqueRedirect, but I don't think those can have bodies anyway), then XSDB should clear the body.
- If the service worker response is any other response type, then XSDB shouldn't clear the body.

Doh... it all makes sense now :-)  Not sure how I managed to misunderstand #c11 as opposite of what you actually meant.  I'll fix up the CL tomorrow.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0ed883d05962ca4e3696232360163ac6d416da1

commit f0ed883d05962ca4e3696232360163ac6d416da1
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Feb 05 19:31:16 2018

Exempt non-opaque service worker responses from XSDB.

The CL tweaks
CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders
so that:
1. XSDB doesn't touch most kinds of responses from service workers
   (e.g. responses created entirely within the service worker, without
   a network trip to a cross-origin site).
2. XSDB continues to strip the body of kOpaque responses from service
   workers (e.g. responses to 'no-cors' requests that have been cached
   in a service worker).

The CL also adds regression tests for the impacted scenarios above:
- CrossSiteDocumentBlockingServiceWorkerTest.NoNetwork covers scenario1
- CrossSiteDocumentBlockingServiceWorkerTest.NetworkAndOpaqueResponse
  covers scenario2 and also makes sure that XSDB is also applied to
  network requests originating from within a service worker.

The CL also moves test files used by XSDB tests into a XSDB-specific
directory - this makes it easier to align the scope of the service
worker and the page it works for.

Bug:  803672 ,  805503 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I60bdd958887f6e8ac49a79b2518610a2a03eea90
Reviewed-on: https://chromium-review.googlesource.com/879348
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534459}
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/browser/loader/cross_site_document_resource_handler.h
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/renderer/loader/site_isolation_stats_gatherer_browsertest.cc
[rename] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/test/data/cross_site_document_blocking/request.html
[rename] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/test/data/cross_site_document_blocking/request_target.html
[add] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/content/test/data/cross_site_document_blocking/service_worker.js
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
[modify] https://crrev.com/f0ed883d05962ca4e3696232360163ac6d416da1/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Before the fix, XSDB (active if users opted into Site Isolation) would break pages dependent on Service Workers that can return non-opaque cross-origin responses.  This compat risk is relatively small (many conditions needed to trigger the bug), but the product-code changes are small and feel safe for merging back to M65.  The fix has been baking on the Canary and Dev channels since 66.0.3341.0.

+ awhalley@ (Security TPM) for M65 merge review.

We can remove the security restrictions from the bug - there is no security bug in behavior of either Service Workers or XSDB.  The bug was that XSDB incorrectly blocked some SW responses (i.e. XSDB tried to be *too* secure - it blocked things which should have been allowed).

Approving merge to M65 branch 3325 based on comment #20. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/793e1b8886e87b99827d4e78985d2069506f1902

commit 793e1b8886e87b99827d4e78985d2069506f1902
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Feb 09 21:45:12 2018

Exempt non-opaque service worker responses from XSDB.

The CL tweaks
CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders
so that:
1. XSDB doesn't touch most kinds of responses from service workers
   (e.g. responses created entirely within the service worker, without
   a network trip to a cross-origin site).
2. XSDB continues to strip the body of kOpaque responses from service
   workers (e.g. responses to 'no-cors' requests that have been cached
   in a service worker).

The CL also adds regression tests for the impacted scenarios above:
- CrossSiteDocumentBlockingServiceWorkerTest.NoNetwork covers scenario1
- CrossSiteDocumentBlockingServiceWorkerTest.NetworkAndOpaqueResponse
  covers scenario2 and also makes sure that XSDB is also applied to
  network requests originating from within a service worker.

The CL also moves test files used by XSDB tests into a XSDB-specific
directory - this makes it easier to align the scope of the service
worker and the page it works for.

TBR=lukasza@chromium.org

(cherry picked from commit f0ed883d05962ca4e3696232360163ac6d416da1)

Bug:  803672 ,  805503 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I60bdd958887f6e8ac49a79b2518610a2a03eea90
Reviewed-on: https://chromium-review.googlesource.com/879348
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#534459}
Reviewed-on: https://chromium-review.googlesource.com/912402
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#413}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/browser/loader/cross_site_document_resource_handler.h
[modify] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/renderer/loader/site_isolation_stats_gatherer_browsertest.cc
[rename] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/test/data/cross_site_document_blocking/request.html
[rename] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/test/data/cross_site_document_blocking/request_target.html
[add] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/content/test/data/cross_site_document_blocking/service_worker.js
[modify] https://crrev.com/793e1b8886e87b99827d4e78985d2069506f1902/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter