Should XSDB apply to error responses? |
||||
Issue descriptionCurrently XSDB ignores the http status code when blocking responses (and consequently also blocks/clears body of 4xx or 5xx responses). Additionally, in https://chromium-review.googlesource.com/c/chromium/src/+/879348 we need to decide whether to block error responses coming from service workers (I note that ResourceLoader::DetermineCORSStatus treats network::mojom::FetchResponseType::kError the same as CORSStatus::kServiceWorkerSuccessful rather than as CORSStatus::kServiceWorkerOpaque. Some web servers communicate error details in the body of a 4xx or 5xx response. - If the body of such a response is an text/html, then it won't be rendered in XSDB-covered contexts (XSDB doesn't cover navigation requests). In such a case blocking/clearing/dropping the response body is invisible to end users and web developers. - If the body of such a response is an image/png, then the error details might be rendered inside an <img> element. In such a case blocking/clearing/dropping the response might be undesirable. OTOH, maybe returning image/png in a 404 is a purely theoretical concern. At any rate, XSDB should probably consistently treat errors 1) from http servers and 2) from the service workers. We should decide whether to block/clear/drop the body of error responses or not.
,
Jan 25 2018
I think kError is a bit different from HTTP status code. It's a network error. To be safe, I'd be inclined to drop the body. Based on the Fetch spec it should OK to drop the body, as it should be considered non-existent in the first place: === A response whose type is "error" is known as a network error. A network error is a response whose status is always 0, status message is always the empty byte sequence, header list is always empty, body is always null, and trailer is always empty. ===
,
Jan 25 2018
,
Feb 5 2018
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
,
Feb 9 2018
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
,
May 22 2018
I think there is nothing more to be done here... |
||||
►
Sign in to add a comment |
||||
Comment 1 by lukasza@chromium.org
, Jan 24 2018