CORB should only retain Access-Control-* response headers.

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I8e39681139d273c16bc93cd58506bae2314fadd6
Bug: 846839
Reviewed-on: https://chromium-review.googlesource.com/1072645
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581035}

content/browser/loader/cross_site_document_blocking_browsertest.cc

@@ -89,7 +89,7 @@ void InspectHistograms(const base::HistogramTester& histograms,
                        const std::string& resource_name,
                        ResourceType resource_type) {
   // //services/network doesn't have access to content::ResourceType and
-  // therefore cannot log some XSDB UMAs.
+  // therefore cannot log some CORB UMAs.
   bool is_restricted_uma_expected = false;
   if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
     is_restricted_uma_expected = true;
@@ -613,38 +613,21 @@ IN_PROC_BROWSER_TEST_F(CrossSiteDocumentBlockingTest, BlockHeaders) {
   ASSERT_EQ(net::OK, interceptor.completion_status().error_code);
   ASSERT_TRUE(interceptor.completion_status().should_report_corb_blocking);
 
-  // Verify that safelisted headers have not been removed by XSDB.
-  // See https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name.
+  // Verify that most response headers have been removed by CORB.
   const std::string& headers =
       interceptor.response_head().headers->raw_headers();
-  EXPECT_THAT(headers,
-              HasSubstr("Cache-Control: no-cache, no-store, must-revalidate"));
-  EXPECT_THAT(headers, HasSubstr("Content-Language: TestLanguage"));
-  EXPECT_THAT(headers,
-              HasSubstr("Content-Type: application/json; charset=utf-8"));
-  EXPECT_THAT(headers, HasSubstr("Expires: Wed, 21 Oct 2199 07:28:00 GMT"));
-  EXPECT_THAT(headers,
-              HasSubstr("Last-Modified: Wed, 07 Feb 2018 13:55:00 PST"));
-  EXPECT_THAT(headers, HasSubstr("Pragma: TestPragma"));
-
-  // Make sure the test covers all the safelisted headers known to the product
-  // code.
-  for (const std::string& safelisted_header :
-       network::CrossOriginReadBlocking::GetCorsSafelistedHeadersForTesting()) {
-    EXPECT_TRUE(
-        interceptor.response_head().headers->HasHeader(safelisted_header));
-
-    std::string value;
-    interceptor.response_head().headers->EnumerateHeader(
-        nullptr, safelisted_header, &value);
-    EXPECT_FALSE(value.empty());
-  }
-
-  // Verify that other response headers have been removed by XSDB.
+  EXPECT_THAT(headers, HasSubstr("Access-Control-Allow-Origin: https://other"));
+  EXPECT_THAT(headers, Not(HasSubstr("Cache-Control")));
+  EXPECT_THAT(headers, Not(HasSubstr("Content-Language")));
   EXPECT_THAT(headers, Not(HasSubstr("Content-Length")));
-  EXPECT_THAT(headers, Not(HasSubstr("X-My-Secret-Header")));
+  EXPECT_THAT(headers, Not(HasSubstr("Content-Type")));
+  EXPECT_THAT(headers, Not(HasSubstr("Expires")));
+  EXPECT_THAT(headers, Not(HasSubstr("Last-Modified")));
   EXPECT_THAT(headers, Not(HasSubstr("MySecretCookieKey")));
   EXPECT_THAT(headers, Not(HasSubstr("MySecretCookieValue")));
+  EXPECT_THAT(headers, Not(HasSubstr("Pragma")));
+  EXPECT_THAT(headers, Not(HasSubstr("X-Content-Type-Options")));
+  EXPECT_THAT(headers, Not(HasSubstr("X-My-Secret-Header")));
 
   // Verify that the body is empty.
   EXPECT_EQ("", interceptor.response_body());
@@ -899,7 +882,7 @@ IN_PROC_BROWSER_TEST_F(CrossSiteDocumentBlockingServiceWorkerTest, NoNetwork) {
           }); )";
   EXPECT_TRUE(ExecuteScriptAndExtractString(shell(), script, &response));
 
-  // Verify that XSDB didn't block the response (since it was "faked" within the
+  // Verify that CORB didn't block the response (since it was "faked" within the
   // service worker and didn't cross any security boundaries).
   EXPECT_EQ("Response created by service worker", response);
   InspectHistograms(histograms, kShouldBeAllowedWithoutSniffing, "blah.html",
@@ -938,13 +921,13 @@ IN_PROC_BROWSER_TEST_F(CrossSiteDocumentBlockingServiceWorkerTest,
   std::string response;
   EXPECT_TRUE(ExecuteScriptAndExtractString(shell(), script, &response));
 
-  // Verify that XSDB blocked the response from the network (from
+  // Verify that CORB blocked the response from the network (from
   // |cross_origin_https_server_|) to the service worker.
   InspectHistograms(histograms, kShouldBeBlockedWithoutSniffing, "network.txt",
                     RESOURCE_TYPE_XHR);
 
   // Verify that the service worker replied with an expected error.
-  // Replying with an error means that XSDB is only active once (for the
+  // Replying with an error means that CORB is only active once (for the
   // initial, real network request) and therefore the test doesn't get
   // confused (second successful response would have added noise to the
   // histograms captured by the test).

content/browser/loader/cross_site_document_resource_handler.cc

@@ -532,7 +532,7 @@ void CrossSiteDocumentResourceHandler::OnResponseCompleted(
     next_handler_->OnResponseCompleted(net::URLRequestStatus(),
                                        std::move(controller));
   } else {
-    // Only report XSDB status for successful (i.e. non-aborted,
+    // Only report CORB status for successful (i.e. non-aborted,
     // non-errored-out) requests.
     if (status.is_success())
       analyzer_->LogAllowedResponse();

content/test/data/cross_site_document_blocking/headers-test.json.mock-http-headers

 HTTP/1.1 200 OK
+Access-Control-Allow-Origin: https://other.unrelated.origin.com
 Cache-Control: no-cache, no-store, must-revalidate
 Content-Length: 19
 Content-Language: TestLanguage

services/network/cross_origin_read_blocking.cc

@@ -153,19 +153,17 @@ SniffingResult MaybeSkipHtmlComment(StringPiece* data) {
   return CrossOriginReadBlocking::kYes;
 }
 
-// Headers from
-// https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name.
+// Removes headers that should be blocked in cross-origin case.
+//
+// Note that corbSanitizedResponse in https://fetch.spec.whatwg.org/#main-fetch
+// has an empty list of headers, but the code below doesn't remove all the
+// headers for improved user experience - for better error messages for CORS.
+// See also https://github.com/whatwg/fetch/pull/686#issuecomment-383711732 and
+// the http/tests/xmlhttprequest/origin-exact-matching/07.html layout test.
 //
-// Note that XSDB doesn't block responses allowed through CORS - this means
+// Note that CORB doesn't block responses allowed through CORS - this means
 // that the list of allowed headers below doesn't have to consider header
 // names listed in the Access-Control-Expose-Headers header.
-const char* const kCorsSafelistedHeaders[] = {
-    "cache-control", "content-language", "content-type",
-    "expires",       "last-modified",    "pragma",
-};
-
-// Removes headers that should be blocked in cross-origin case.
-// See https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name.
 void BlockResponseHeaders(
     const scoped_refptr<net::HttpResponseHeaders>& headers) {
   DCHECK(headers);
@@ -186,15 +184,10 @@ void BlockResponseHeaders(
       continue;
     }
 
-    // Remove all other headers (but note the final exclusion below).
+    // Remove all other headers.
     names_of_headers_to_remove.insert(base::ToLowerASCII(name));
   }
 
-  // Exclude from removals headers from
-  // https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name.
-  for (const char* header : kCorsSafelistedHeaders)
-    names_of_headers_to_remove.erase(header);
-
   headers->RemoveHeaders(names_of_headers_to_remove);
 }
 
@@ -444,14 +437,6 @@ void CrossOriginReadBlocking::SanitizeBlockedResponse(
     BlockResponseHeaders(response->head.headers);
 }
 
-// static
-std::vector<std::string>
-CrossOriginReadBlocking::GetCorsSafelistedHeadersForTesting() {
-  return std::vector<std::string>(
-      kCorsSafelistedHeaders,
-      kCorsSafelistedHeaders + arraysize(kCorsSafelistedHeaders));
-}
-
 // static
 void CrossOriginReadBlocking::LogAction(Action action) {
   UMA_HISTOGRAM_ENUMERATION("SiteIsolation.XSD.Browser.Action", action);

services/network/cross_origin_read_blocking.h

@@ -161,14 +161,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CrossOriginReadBlocking {
   static void SanitizeBlockedResponse(
       const scoped_refptr<network::ResourceResponse>& response);
 
-  // Returns explicitly named headers from
-  // https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name.
-  //
-  // Note that CORB doesn't block responses allowed through CORS - this means
-  // that the list of allowed headers below doesn't have to consider header
-  // names listed in the Access-Control-Expose-Headers header.
-  static std::vector<std::string> GetCorsSafelistedHeadersForTesting();
-
   // This enum backs a histogram, so do not change the order of entries or
   // remove entries. When adding new entries update |kMaxValue| and enums.xml
   // (see the SiteIsolationResponseAction enum).

services/network/cross_origin_read_blocking_explainer.md

@@ -77,10 +77,10 @@ CORB mitigates the following attack vectors:
 When CORB decides that a response needs to be CORB-protected, the response is
 modified as follows:
 * The response body is replaced with an empty body.
-* The response headers are filtered down to the following ones
-  (i.e. all other response headers are removed from the response):
-  - [CORS safelisted response headers](https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name)
-  - [CORS response headers](https://fetch.spec.whatwg.org/#http-responses)
+* The response headers are removed.
+
+> [lukasza@chromium.org] Chromium currently retains Access-Control-\* headers
+> (this helps generate better error messages for CORS).
 
 To be effective against speculative side-channel attacks, CORB blocking must
 take place before the response reaches the process hosting the cross-origin

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/multiple-headers.js

@@ -9,7 +9,7 @@
 
   session.evaluate(`fetch("${url}?fetch=1")`);
   const fetchResponse = (await dp.Network.onceResponseReceived()).params.response;
-  testRunner.log(`Pragma header of fetch of ${fetchResponse.url}: ${fetchResponse.headers['Pragma']}`);
+  testRunner.log(`Pragma header of fetch of ${fetchResponse.url}: ${fetchResponse.headers['Access-Control-Pragma']}`);
   await dp.Network.onceLoadingFinished();
 
   session.evaluate(`
@@ -18,6 +18,6 @@
     document.body.appendChild(f);
   `);
   const navigationResponse = (await dp.Network.onceResponseReceived()).params.response;
-  testRunner.log(`Pragma header of navigation to ${navigationResponse.url}: ${navigationResponse.headers['Pragma']}`);
+  testRunner.log(`Pragma header of navigation to ${navigationResponse.url}: ${navigationResponse.headers['Access-Control-Pragma']}`);
   testRunner.completeTest();
 })

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/multiple-headers.php

 <?php
     // We need live headers, so make sure it's not cached.
     header('Cache-Control: no-cache, no-store, must-revalidate, max-age=0');
-    header('Pragma: value1');
-    header('Pragma: value2', false /* = $replace */);
+    header('Access-Control-Pragma: value1');
+    header('Access-Control-Pragma: value2', false /* = $replace */);
 ?>
 <html>
 <body>