Check opt-in isolation before command-line isolation.

This CL switches the order of checking origin isolation so that opt-in
isolation (which is the stronger of the two), is checked before command-
line isolation.

Bug: 1067389, 1085275
Change-Id: I82eaa73eb4be67f35e8829334cf3e7f3868f87d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346906
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797232}

content/browser/child_process_security_policy_impl.cc

@@ -1890,8 +1890,21 @@ bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
   // available IsolatedOriginEntries.
   BrowsingInstanceId browsing_instance_id(
       isolation_context.browsing_instance_id());
-  if (browsing_instance_id.is_null())
+
+  if (browsing_instance_id.is_null()) {
     browsing_instance_id = SiteInstanceImpl::NextBrowsingInstanceId();
+  } else {
+    // Check the opt-in isolation status of |origin| in |isolation_context|.
+    // Note that while IsolatedOrigins considers any sub-origin of an isolated
+    // origin as also being isolated, with opt-in we will always either return
+    // false, or true with result set to |origin|. We give priority to origins
+    // requesting opt-in isolation over command-line isolation, but don't check
+    // for opt-in if we didn't get a valid BrowsingInstance id.
+    if (ShouldOriginGetOptInIsolation(isolation_context, origin)) {
+      *result = origin;
+      return true;
+    }
+  }
 
   // Look up the list of origins corresponding to |origin|'s site.
   auto it = isolated_origins_.find(site_url);
@@ -1957,16 +1970,6 @@ bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
     }
   }
 
-  // If no match was found via IsolatedOrigins, then check the opt-in
-  // isolation status of |origin| in |isolation_context|. Note that while
-  // IsolatedOrigins considers any sub-origin of an isolated origin as also
-  // being isolated, with opt-in we will always either return false, or true
-  // with result set to |origin|.
-  if (!found && ShouldOriginGetOptInIsolation(isolation_context, origin)) {
-    *result = origin;
-    found = true;
-  }
-
   return found;
 }
 

content/browser/isolated_origin_browsertest.cc

@@ -146,15 +146,19 @@ class OriginIsolationOptInServerTest : public IsolatedOriginTestBase {
     IsolateAllSitesForTesting(command_line);
     command_line->AppendSwitch(switches::kIgnoreCertificateErrors);
     feature_list_.InitAndEnableFeature(feature_switch());
-  }
 
-  void SetUpOnMainThread() override {
-    IsolatedOriginTestBase::SetUpOnMainThread();
+    // Start the HTTPS server here so derived tests can use it if they override
+    // SetUpCommandLine().
     https_server()->AddDefaultHandlers(GetTestDataFilePath());
     https_server()->RegisterRequestHandler(
         base::BindRepeating(&OriginIsolationOptInServerTest::HandleResponse,
                             base::Unretained(this)));
     ASSERT_TRUE(https_server()->Start());
+  }
+
+  void SetUpOnMainThread() override {
+    IsolatedOriginTestBase::SetUpOnMainThread();
+
     host_resolver()->AddRule("*", "127.0.0.1");
     embedded_test_server()->StartAcceptingConnections();
   }
@@ -281,6 +285,81 @@ class OriginIsolationOptInHeaderTest : public OriginIsolationOptInServerTest {
   DISALLOW_COPY_AND_ASSIGN(OriginIsolationOptInHeaderTest);
 };
 
+// This class allows testing the interaction of OptIn isolation and command-line
+// isolation for origins. Tests using this class will isolate foo.com and
+// bar.com by default using command-line isolation, but any opt-in isolation
+// will override this.
+class OriginIsolationOptInOriginPolicyCommandLineTest
+    : public OriginIsolationOptInOriginPolicyTest {
+ public:
+  OriginIsolationOptInOriginPolicyCommandLineTest() = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    OriginIsolationOptInOriginPolicyTest::SetUpCommandLine(command_line);
+    // The base class should already have started the HTTPS server so we can use
+    // it here to generate origins to specify on the command line.
+    ASSERT_TRUE(https_server()->Started());
+
+    std::string origin_list = https_server()->GetURL("foo.com", "/").spec() +
+                              "," +
+                              https_server()->GetURL("bar.com", "/").spec();
+    command_line->AppendSwitchASCII(switches::kIsolateOrigins, origin_list);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(OriginIsolationOptInOriginPolicyCommandLineTest);
+};
+
+// This test verifies that opt-in isolation takes precedence over command-line
+// isolation. It loads an opt-in isolated base origin (which would have
+// otherwise been isolated via command-line isolation), and then loads a child
+// frame sub-origin which should-not be isolated (but would have been if the
+// base origin was command-line isolated).
+IN_PROC_BROWSER_TEST_F(OriginIsolationOptInOriginPolicyCommandLineTest,
+                       OptInOverridesCommandLine) {
+  SetOriginPolicyManifest(R"({ "ids": ["my-policy"], "isolation": true })");
+  // Start off with an isolated base-origin in an a(a) configuration, then
+  // navigate the subframe to a sub-origin not requesting isolation.
+  // Note: this works because we serve mock headers with the base origin's html
+  // file, requesting loading the origin policy.
+  GURL isolated_base_origin_url(https_server()->GetURL(
+      "foo.com", "/isolated_base_origin_with_subframe.html"));
+  GURL non_isolated_sub_origin(
+      https_server()->GetURL("non_isolated.foo.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), isolated_base_origin_url));
+  EXPECT_EQ(2u, shell()->web_contents()->GetAllFrames().size());
+
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  FrameTreeNode* child_frame_node = root->child_at(0);
+  NavigateFrameToURL(child_frame_node, non_isolated_sub_origin);
+
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  EXPECT_TRUE(policy->ShouldOriginGetOptInIsolation(
+      root->current_frame_host()->GetSiteInstance()->GetIsolationContext(),
+      url::Origin::Create(isolated_base_origin_url)));
+  EXPECT_FALSE(policy->ShouldOriginGetOptInIsolation(
+      root->current_frame_host()->GetSiteInstance()->GetIsolationContext(),
+      url::Origin::Create(non_isolated_sub_origin)));
+  // Make sure the child (i.e. sub-origin) is not isolated.
+  EXPECT_NE(root->current_frame_host()->GetSiteInstance(),
+            child_frame_node->current_frame_host()->GetSiteInstance());
+  EXPECT_EQ(
+      GURL("https://foo.com"),
+      child_frame_node->current_frame_host()->GetSiteInstance()->GetSiteURL());
+  // The following test passes because IsIsolatedOrigin doesn't distinguish
+  // between command-line isolation and opt-in isolation.
+  EXPECT_TRUE(policy->IsIsolatedOrigin(
+      root->current_frame_host()->GetSiteInstance()->GetIsolationContext(),
+      url::Origin::Create(non_isolated_sub_origin)));
+
+  // Make sure the master opt-in list has the base origin isolated and the sub
+  // origin not isolated.
+  EXPECT_TRUE(policy->HasOriginEverRequestedOptInIsolation(
+      url::Origin::Create(isolated_base_origin_url)));
+  EXPECT_FALSE(policy->HasOriginEverRequestedOptInIsolation(
+      url::Origin::Create(non_isolated_sub_origin)));
+}
+
 // This tests that origin policy opt-in causes the origin to end up in the
 // isolated origins list.
 IN_PROC_BROWSER_TEST_F(OriginIsolationOptInOriginPolicyTest, Basic) {