Stop relying on disabled web security in CORB tests.

This CL:

1. Transitions core CORB test coverage away from the fetch API (where
   blocking by OOR-CORS can interfere with test results) to triggering
   fetches via <img> tags (where CORB stays applicable even after
   OOR-CORS ships).

   Tests covering CORS/CORB intersection continue to be done via fetch
   API (in CrossSiteDocumentBlockingTest.BlockFetches test).

   Verification of response body no longer depends on disabled web
   security (and inspecting fetch API results), but is instead done
   via RequestInterceptor helper that lives next to the tests.
   The CL also uncovered and fixes a threading issue in the
   implementation of RequestInterceptor.

2. Stops testing range requests in content_browsertests and starts to
   rely on existing and new test coverage in content_unittests (fetch
   API seems to be the only directly-test-controllable way of making
   range requests).

I've tested this CL by manually running CrossSiteDocumentBlockingTest
from content_browsertests 1) in default mode, 2) with NetworkService
enabled and 3) with NetworkService and OutOfBlinkCORS enabled.

Bug: 870173
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Iec5c9f0e7ddad0ff02627b88dcfab276795e7be9
Reviewed-on: https://chromium-review.googlesource.com/c/1279351Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601174}

content/browser/loader/cross_site_document_blocking_browsertest.cc

@@ -50,10 +50,10 @@ using Action = network::CrossOriginReadBlocking::Action;
 
 namespace {
 
-enum HistogramExpectations {
+enum CorbExpectations {
   kShouldBeBlocked = 1 << 0,
   kShouldBeSniffed = 1 << 1,
-  kShouldHaveContentLength = 1 << 2,
+  kShouldLogContentLengthUma = 1 << 2,
 
   kShouldBeAllowedWithoutSniffing = 0,
   kShouldBeBlockedWithoutSniffing = kShouldBeBlocked,
@@ -61,13 +61,12 @@ enum HistogramExpectations {
   kShouldBeSniffedAndBlocked = kShouldBeSniffed | kShouldBeBlocked,
 };
 
-HistogramExpectations operator|(HistogramExpectations a,
-                                HistogramExpectations b) {
-  return static_cast<HistogramExpectations>(static_cast<int>(a) |
-                                            static_cast<int>(b));
+CorbExpectations operator|(CorbExpectations a, CorbExpectations b) {
+  return static_cast<CorbExpectations>(static_cast<int>(a) |
+                                       static_cast<int>(b));
 }
 
-std::ostream& operator<<(std::ostream& os, const HistogramExpectations& value) {
+std::ostream& operator<<(std::ostream& os, const CorbExpectations& value) {
   if (value == 0) {
     os << "(none)";
     return os;
@@ -78,8 +77,8 @@ std::ostream& operator<<(std::ostream& os, const HistogramExpectations& value) {
     os << "kShouldBeBlocked ";
   if (0 != (value & kShouldBeSniffed))
     os << "kShouldBeSniffed ";
-  if (0 != (value & kShouldHaveContentLength))
-    os << "kShouldHaveContentLength ";
+  if (0 != (value & kShouldLogContentLengthUma))
+    os << "kShouldLogContentLengthUma ";
   os << ")";
   return os;
 }
@@ -87,7 +86,7 @@ std::ostream& operator<<(std::ostream& os, const HistogramExpectations& value) {
 // Ensure the correct histograms are incremented for blocking events.
 // Assumes the resource type is XHR.
 void InspectHistograms(const base::HistogramTester& histograms,
-                       const HistogramExpectations& expectations,
+                       const CorbExpectations& expectations,
                        const std::string& resource_name,
                        ResourceType resource_type) {
   // //services/network doesn't have access to content::ResourceType and
@@ -130,7 +129,7 @@ void InspectHistograms(const base::HistogramTester& histograms,
   if (0 != (expectations & kShouldBeBlocked)) {
     expected_counts[base + ".Blocked.ContentLength.WasAvailable"] = 1;
     bool should_have_content_length =
-        0 != (expectations & kShouldHaveContentLength);
+        0 != (expectations & kShouldLogContentLengthUma);
     histograms.ExpectBucketCount(base + ".Blocked.ContentLength.WasAvailable",
                                  should_have_content_length, 1);
 
@@ -192,6 +191,8 @@ class RequestInterceptor {
                                 base::Unretained(this))) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK(url_to_intercept.is_valid());
+
+    test_client_ptr_info_ = test_client_.CreateInterfacePtr().PassInterface();
   }
 
   // Waits until a request gets intercepted and completed.
@@ -248,6 +249,27 @@ class RequestInterceptor {
     return body_;
   }
 
+  void Verify(CorbExpectations expectations) {
+    if (0 != (expectations & kShouldBeBlocked)) {
+      ASSERT_EQ(net::OK, completion_status().error_code);
+
+      // Verify that the body is empty.
+      EXPECT_EQ("", response_body());
+      EXPECT_EQ(0, completion_status().decoded_body_length);
+
+      // Verify that other response parts have been sanitized.
+      EXPECT_EQ(0u, response_head().content_length);
+      const std::string& headers = response_head().headers->raw_headers();
+      EXPECT_THAT(headers, Not(HasSubstr("Content-Length")));
+      EXPECT_THAT(headers, Not(HasSubstr("Content-Type")));
+
+      // Verify that the console message would have been printed.
+      EXPECT_TRUE(completion_status().should_report_corb_blocking);
+    } else {
+      EXPECT_FALSE(completion_status().should_report_corb_blocking);
+    }
+  }
+
  private:
   bool InterceptorCallback(URLLoaderInterceptor::RequestParams* params) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -264,7 +286,10 @@ class RequestInterceptor {
     // Inject |test_client_| into the request.
     DCHECK(!original_client_);
     original_client_ = std::move(params->client);
-    params->client = test_client_.CreateInterfacePtr();
+    test_client_ptr_.Bind(std::move(test_client_ptr_info_));
+    test_client_binding_ =
+        std::make_unique<mojo::Binding<network::mojom::URLLoaderClient>>(
+            test_client_ptr_.get(), mojo::MakeRequest(&params->client));
 
     // Forward the request to the original URLLoaderFactory.
     return false;
@@ -280,41 +305,32 @@ class RequestInterceptor {
 
     // Reset all temporary mojo bindings.
     original_client_.reset();
-    test_client_.Unbind();
+    test_client_binding_.reset();
+    test_client_ptr_.reset();
   }
 
   const GURL url_to_intercept_;
   URLLoaderInterceptor interceptor_;
-  network::TestURLLoaderClient test_client_;
+
+  // |test_client_ptr_info_| below is used to transition results of
+  // |test_client_.CreateInterfacePtr()| into IO thread.
+  network::mojom::URLLoaderClientPtrInfo test_client_ptr_info_;
 
   // UI thread state:
+  network::TestURLLoaderClient test_client_;
   std::string body_;
   bool request_completed_ = false;
 
   // IO thread state:
   network::mojom::URLLoaderClientPtr original_client_;
   bool request_intercepted_ = false;
+  network::mojom::URLLoaderClientPtr test_client_ptr_;
+  std::unique_ptr<mojo::Binding<network::mojom::URLLoaderClient>>
+      test_client_binding_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestInterceptor);
 };
 
-// Custom ContentBrowserClient that disables web security in the renderer
-// process without actually using --disable-web-security (which disables CORB).
-// This disables the same origin policy to let the renderer see cross-origin
-// fetches if they are received.
-class DisableWebSecurityContentBrowserClient : public TestContentBrowserClient {
- public:
-  DisableWebSecurityContentBrowserClient() : TestContentBrowserClient() {}
-
-  ~DisableWebSecurityContentBrowserClient() override {}
-
-  // ContentBrowserClient overrides:
-  void OverrideWebkitPrefs(RenderViewHost* render_view_host,
-                           WebPreferences* prefs) override {
-    prefs->web_security_enabled = false;
-  }
-};
-
 }  // namespace
 
 // These tests verify that the browser process blocks cross-site HTML, XML,
@@ -323,15 +339,6 @@ class DisableWebSecurityContentBrowserClient : public TestContentBrowserClient {
 // renderer process where they might be accessible via a bug.  Careful attention
 // is paid to allow other cross-site resources necessary for rendering,
 // including cases that may be mislabeled as blocked MIME type.
-//
-// Many of these tests work by turning off the Same Origin Policy in the
-// renderer process via WebPreferences::web_security_enabled, and then trying to
-// access the resource via a cross-origin XHR.  If the response is blocked, the
-// XHR should see an empty response body.
-//
-// Note that this BaseTest class does not specify an isolation mode via
-// command-line flags.  Most of the tests are in the --site-per-process subclass
-// below.
 class CrossSiteDocumentBlockingTestBase : public ContentBrowserTest {
  public:
   CrossSiteDocumentBlockingTestBase() = default;
@@ -356,19 +363,30 @@ class CrossSiteDocumentBlockingTestBase : public ContentBrowserTest {
             ",EXCLUDE localhost");
   }
 
-  void SetUpOnMainThread() override {
-    // Disable web security via the ContentBrowserClient and notify the current
-    // renderer process.
-    old_client = SetBrowserClientForTesting(&new_client);
-    shell()->web_contents()->GetRenderViewHost()->OnWebkitPreferencesChanged();
-  }
+  void VerifyImgRequest(std::string resource, CorbExpectations expectations) {
+    SCOPED_TRACE("... while testing via <img> tag");
 
-  void TearDown() override { SetBrowserClientForTesting(old_client); }
+    // Navigate to the test page while request interceptor is active.
+    GURL resource_url(
+        std::string("http://cross-origin.com/site_isolation/" + resource));
+    RequestInterceptor interceptor(resource_url);
+    EXPECT_TRUE(NavigateToURL(shell(), GURL("http://foo.com/title1.html")));
 
- private:
-  DisableWebSecurityContentBrowserClient new_client;
-  ContentBrowserClient* old_client = nullptr;
+    // Issue the request that will be intercepted.
+    base::HistogramTester histograms;
+    const char kScriptTemplate[] = R"(
+        var img = document.createElement('img');
+        img.src = $1;
+        document.body.appendChild(img); )";
+    EXPECT_TRUE(ExecJs(shell(), JsReplace(kScriptTemplate, resource_url)));
+    interceptor.WaitForRequestCompletion();
+
+    // Verify...
+    InspectHistograms(histograms, expectations, resource, RESOURCE_TYPE_IMAGE);
+    interceptor.Verify(expectations);
+  }
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(CrossSiteDocumentBlockingTestBase);
 };
 
@@ -400,26 +418,9 @@ class CrossSiteDocumentBlockingTest
   DISALLOW_COPY_AND_ASSIGN(CrossSiteDocumentBlockingTest);
 };
 
-IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockDocuments) {
-  // Load a page that issues illegal cross-site document requests to bar.com.
-  // The page uses XHR to request HTML/XML/JSON documents from bar.com, and
-  // inspects if any of them were successfully received. This test is only
-  // possible since we run the browser without the same origin policy, allowing
-  // it to see the response body if it makes it to the renderer (even if the
-  // renderer would normally block access to it).
+IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockImages) {
   embedded_test_server()->StartAcceptingConnections();
 
-  // This test does not work and won't make much sense if kOutOfBlinkCORS is
-  // enabled. We need to rewrite this test once the feature is shipped. We would
-  // like to add other tests for verifying behavior for fetch mode "no-cors".
-  // See crbug.com/736308, and discussion at http://crrev.com/c/1253623.
-  // The test server should be started before to shutdown tests safely.
-  if (base::FeatureList::IsEnabled(network::features::kOutOfBlinkCORS))
-    return;
-
-  GURL foo_url("http://foo.com/cross_site_document_blocking/request.html");
-  EXPECT_TRUE(NavigateToURL(shell(), foo_url));
-
   // The following are files under content/test/data/site_isolation. All
   // should be disallowed for cross site XHR under the document blocking policy.
   //   valid.*        - Correctly labeled HTML/XML/JSON files.
@@ -446,15 +447,8 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockDocuments) {
                                      "nosniff.json-prefixed.js"};
   for (const char* resource : blocked_resources) {
     SCOPED_TRACE(base::StringPrintf("... while testing page: %s", resource));
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), base::StringPrintf("sendRequest('%s');", resource),
-        &was_blocked));
-    EXPECT_TRUE(was_blocked);
-    InspectHistograms(histograms,
-                      kShouldBeSniffedAndBlocked | kShouldHaveContentLength,
-                      resource, RESOURCE_TYPE_XHR);
+    VerifyImgRequest(resource,
+                     kShouldBeSniffedAndBlocked | kShouldLogContentLengthUma);
   }
 
   // These files should be disallowed without sniffing.
@@ -463,14 +457,7 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockDocuments) {
                                              "nosniff.json", "nosniff.txt"};
   for (const char* resource : nosniff_blocked_resources) {
     SCOPED_TRACE(base::StringPrintf("... while testing page: %s", resource));
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), base::StringPrintf("sendRequest('%s');", resource),
-        &was_blocked));
-    EXPECT_TRUE(was_blocked);
-    InspectHistograms(histograms, kShouldBeBlockedWithoutSniffing, resource,
-                      RESOURCE_TYPE_XHR);
+    VerifyImgRequest(resource, kShouldBeBlockedWithoutSniffing);
   }
 
   // These files are allowed for XHR under the document blocking policy because
@@ -500,15 +487,14 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockDocuments) {
                                            "js-html-polyglot2.html"};
   for (const char* resource : sniff_allowed_resources) {
     SCOPED_TRACE(base::StringPrintf("... while testing page: %s", resource));
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), base::StringPrintf("sendRequest('%s');", resource),
-        &was_blocked));
-    EXPECT_FALSE(was_blocked);
-    InspectHistograms(histograms, kShouldBeSniffedAndAllowed, resource,
-                      RESOURCE_TYPE_XHR);
+    VerifyImgRequest(resource, kShouldBeSniffedAndAllowed);
   }
+}
+
+IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockFetches) {
+  embedded_test_server()->StartAcceptingConnections();
+  GURL foo_url("http://foo.com/cross_site_document_blocking/request.html");
+  EXPECT_TRUE(NavigateToURL(shell(), foo_url));
 
   // These files should be allowed for XHR under the document blocking policy.
   //   cors.*  - Correctly labeled documents with valid CORS headers.
@@ -527,58 +513,6 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockDocuments) {
   }
 }
 
-// Verify that range requests disable the sniffing logic, so that attackers
-// can't cause sniffing to fail to force a response to be allowed.  This won't
-// be a problem for script files mislabeled as HTML/XML/JSON/text (i.e., the
-// reason for sniffing), since script tags won't send Range headers.
-IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, RangeRequest) {
-  embedded_test_server()->StartAcceptingConnections();
-
-  // This test does not work and won't make much sense if kOutOfBlinkCORS is
-  // enabled. See detailed comments on BlockDocuments above.
-  if (base::FeatureList::IsEnabled(network::features::kOutOfBlinkCORS))
-    return;
-
-  GURL foo_url("http://foo.com/cross_site_document_blocking/request.html");
-  EXPECT_TRUE(NavigateToURL(shell(), foo_url));
-
-  {
-    // Try to skip the first byte using a range request in an attempt to get the
-    // response to fail sniffing and be allowed through.  It should still be
-    // blocked because sniffing is disabled.
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), "sendRequest('valid.html', 'bytes=1-24');", &was_blocked));
-    EXPECT_TRUE(was_blocked);
-    InspectHistograms(
-        histograms, kShouldBeBlockedWithoutSniffing | kShouldHaveContentLength,
-        "valid.html", RESOURCE_TYPE_XHR);
-  }
-  {
-    // Verify that a response which would have been allowed by MIME type anyway
-    // is still allowed for range requests.
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), "sendRequest('valid.js', 'bytes=1-5');", &was_blocked));
-    EXPECT_FALSE(was_blocked);
-    InspectHistograms(histograms, kShouldBeAllowedWithoutSniffing, "valid.js",
-                      RESOURCE_TYPE_XHR);
-  }
-  {
-    // Verify that a response which would have been allowed by CORS anyway is
-    // still allowed for range requests.
-    base::HistogramTester histograms;
-    bool was_blocked;
-    ASSERT_TRUE(ExecuteScriptAndExtractBool(
-        shell(), "sendRequest('cors.json', 'bytes=2-7');", &was_blocked));
-    EXPECT_FALSE(was_blocked);
-    InspectHistograms(histograms, kShouldBeAllowedWithoutSniffing, "cors.json",
-                      RESOURCE_TYPE_XHR);
-  }
-}
-
 IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockForVariousTargets) {
   // This webpage loads a cross-site HTML page in different targets such as
   // <img>,<link>,<embed>, etc. Since the requested document is blocked, and one
@@ -634,11 +568,6 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest,
 IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockHeaders) {
   embedded_test_server()->StartAcceptingConnections();
 
-  // This test does not work and won't make much sense if kOutOfBlinkCORS is
-  // enabled. See detailed comments on BlockDocuments above.
-  if (base::FeatureList::IsEnabled(network::features::kOutOfBlinkCORS))
-    return;
-
   // Prepare to intercept the network request at the IPC layer.
   // This has to be done before the RenderFrameHostImpl is created.
   //
@@ -653,15 +582,18 @@ IN_PROC_BROWSER_TEST_P(CrossSiteDocumentBlockingTest, BlockHeaders) {
   GURL foo_url("http://foo.com/title1.html");
   EXPECT_TRUE(NavigateToURL(shell(), foo_url));
 
-  // Issue the request that will be intercepted
-  EXPECT_TRUE(ExecuteScript(shell(),
-                            base::StringPrintf("fetch('%s').catch(error => {})",
-                                               bar_url.spec().c_str())));
+  // Issue the request that will be intercepted.
+  const char kScriptTemplate[] = R"(
+      var img = document.createElement('img');
+      img.src = $1;
+      document.body.appendChild(img); )";
+  EXPECT_TRUE(ExecJs(shell(), JsReplace(kScriptTemplate, bar_url)));
   interceptor.WaitForRequestCompletion();
 
-  // Verify that the response completed successfully and was blocked.
-  ASSERT_EQ(net::OK, interceptor.completion_status().error_code);
-  ASSERT_TRUE(interceptor.completion_status().should_report_corb_blocking);
+  // Verify that the response completed successfully, was blocked and was logged
+  // as having initially a non-empty body.
+  interceptor.Verify(kShouldBeBlockedWithoutSniffing |
+                     kShouldLogContentLengthUma);
 
   // Verify that most response headers have been removed by CORB.
   const std::string& headers =
@@ -835,15 +767,8 @@ class CrossSiteDocumentBlockingServiceWorkerTest : public ContentBrowserTest {
     ASSERT_FALSE(SiteInstance::IsSameWebSite(
         shell()->web_contents()->GetBrowserContext(),
         GetURLOnServiceWorkerServer("/"), GetURLOnCrossOriginServer("/")));
-
-    // Disable web security via the ContentBrowserClient and notify the current
-    // renderer process.
-    old_client = SetBrowserClientForTesting(&new_client);
-    shell()->web_contents()->GetRenderViewHost()->OnWebkitPreferencesChanged();
   }
 
-  void TearDown() override { SetBrowserClientForTesting(old_client); }
-
   GURL GetURLOnServiceWorkerServer(const std::string& path) {
     return service_worker_https_server_.GetURL(path);
   }
@@ -896,9 +821,6 @@ class CrossSiteDocumentBlockingServiceWorkerTest : public ContentBrowserTest {
   net::EmbeddedTestServer service_worker_https_server_;
   net::EmbeddedTestServer cross_origin_https_server_;
 
-  DisableWebSecurityContentBrowserClient new_client;
-  ContentBrowserClient* old_client = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(CrossSiteDocumentBlockingServiceWorkerTest);
 };
 
@@ -992,8 +914,7 @@ IN_PROC_BROWSER_TEST_F(CrossSiteDocumentBlockingServiceWorkerTest,
   EXPECT_EQ("error: TypeError: Failed to fetch", response);
 }
 
-// Test class to verify that --disable-web-security turns off CORB.  This
-// inherits from CrossSiteDocumentBlockingTest, so it runs in SitePerProcess.
+// Test class to verify that --disable-web-security turns off CORB.
 class CrossSiteDocumentBlockingDisableWebSecurityTest
     : public CrossSiteDocumentBlockingTestBase {
  public:

content/browser/loader/cross_site_document_resource_handler_unittest.cc

@@ -1056,6 +1056,38 @@ const TestScenario kScenarios[] = {
         Verdict::kBlock,                            // verdict
         0,                                          // verdict_packet
     },
+    {
+        "Allowed: Javascript 206",
+        __LINE__,
+        "http://www.b.com/script.js",           // target_url
+        RESOURCE_TYPE_SCRIPT,                   // resource_type
+        "http://www.a.com/",                    // initiator_origin
+        OriginHeader::kOmit,                    // cors_request
+        "application/javascript",               // response_content_type
+        MimeType::kOthers,                      // canonical_mime_type
+        false,                                  // include_no_sniff_header
+        true,                                   // simulate_range_response
+        AccessControlAllowOriginHeader::kOmit,  // cors_response
+        {"x = 1;"},                             // packets
+        Verdict::kAllow,                        // verdict
+        -1,                                     // verdict_packet
+    },
+    {
+        "Allowed: text/html 206 media with CORS",
+        __LINE__,
+        "http://www.b.com/movie.html",  // target_url
+        RESOURCE_TYPE_MEDIA,            // resource_type
+        "http://www.a.com/",            // initiator_origin
+        OriginHeader::kInclude,         // cors_request
+        "text/html",                    // response_content_type
+        MimeType::kInvalidMimeType,     // canonical_mime_type
+        false,                          // include_no_sniff_header
+        true,                           // simulate_range_response
+        AccessControlAllowOriginHeader::kAllowInitiatorOrigin,  // cors_response
+        {"simulated *middle*-of-html content"},                 // packets
+        Verdict::kAllow,                                        // verdict
+        -1,  // verdict_packet
+    },
     {
         "Allowed: text/plain 206 media",
         __LINE__,