Pass IsolatedOriginSource to SiteInstance::StartIsolatingSite().

Currently, SiteInstance::StartIsolatingSite implicitly assumes that the added isolated origin is from the USER_TRIGGERED IsolatedOriginSource. This is no longer the case with the addition of isolation triggered by COOP headers, which should use WEB_TRIGGERED. This CL adds a param to StartIsolatingSite, so callers can specify the proper source, and fixes the COOP isolation caller to pass WEB_TRIGGERED. A couple of unit tests are fixed to pass the TEST source. There should be no behavior changes for the other callers -- site isolation based on passwords, OAuth, and payment API is still considered USER_TRIGGERED. This will be needed in followup work on preserving COOP-isolated origins across restarts, since the storage policies for web-triggered isolated origins will be different from user-triggered ones. Bug: 1018656 Change-Id: Ic6124848b4ad19294e88df007463facdd88c0957 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2877364 Reviewed-by: Peter Conn <peconn@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Clark DuVall <cduvall@chromium.org> Reviewed-by: rajendrant <rajendrant@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: rajendrant <rajendrant@chromium.org> Auto-Submit: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#880526}
Denger-Network · May 7, 2021 · 53cbc6d · 53cbc6d
1 parent b0fb0bc
commit 53cbc6d
Show file tree

Hide file tree

Showing 12 changed files with 88 additions and 51 deletions.
diff --git a/...droid/java/src/org/chromium/chrome/browser/browserservices/digitalgoods/SiteIsolator.java b/...droid/java/src/org/chromium/chrome/browser/browserservices/digitalgoods/SiteIsolator.java
@@ -9,7 +9,9 @@
 import org.chromium.url.GURL;
 
 /**
- * This class calls into native to request that a give tab starts site isolation.
+ * This class calls into native to request that a given tab starts site
+ * isolation for the provided url's site.  Note that the site will be isolated
+ * with a USER_TRIGGERED IsolatedOriginSource.
  */
 public class SiteIsolator {
     private SiteIsolator() {}

diff --git a/chrome/browser/android/browserservices/site_isolator.cc b/chrome/browser/android/browserservices/site_isolator.cc
@@ -18,5 +18,7 @@ void JNI_SiteIsolator_StartIsolatingSite(JNIEnv* env,
   std::unique_ptr<GURL> gurl = url::GURLAndroid::ToNativeGURL(env, j_gurl);
 
   content::SiteInstance::StartIsolatingSite(
-      browser_context::BrowserContextFromJavaHandle(j_profile), *gurl);
+      browser_context::BrowserContextFromJavaHandle(j_profile), *gurl,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+          USER_TRIGGERED);
 }
diff --git a/chrome/browser/chrome_navigation_browsertest.cc b/chrome/browser/chrome_navigation_browsertest.cc
@@ -1902,6 +1902,13 @@ class SiteIsolationForPasswordSitesBrowserTest
         {features::kSitePerProcess});
   }
 
+  void StartIsolatingSite(Profile* profile, const GURL& url) {
+    content::SiteInstance::StartIsolatingSite(
+        profile, url,
+        content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+            USER_TRIGGERED);
+  }
+
   std::vector<std::string> GetSavedIsolatedSites() {
     return GetSavedIsolatedSites(browser()->profile());
   }
@@ -2095,9 +2102,9 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
 
   // Isolate saved.com and saved2.com persistently.
   GURL saved_url(embedded_test_server()->GetURL("saved.com", "/title1.html"));
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
   GURL saved2_url(embedded_test_server()->GetURL("saved2.com", "/title1.html"));
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved2_url);
+  StartIsolatingSite(browser()->profile(), saved2_url);
 
   // Check that saved.com utilizes a dedicated process in future navigations.
   // Open a new tab to force creation of a new BrowsingInstance.
@@ -2146,9 +2153,9 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
 IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
                        IsolatedSiteIsSavedOnlyOnce) {
   GURL saved_url(embedded_test_server()->GetURL("saved.com", "/title1.html"));
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("http://saved.com"));
 }
@@ -2160,7 +2167,7 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
                        IncognitoWithIsolatedSites) {
   // Isolate saved.com and verify it's been saved to disk.
   GURL saved_url(embedded_test_server()->GetURL("saved.com", "/title1.html"));
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("http://saved.com"));
 
@@ -2183,7 +2190,7 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
   // process, and the site is not persisted for either the main or incognito
   // profiles.
   GURL foo_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
-  content::SiteInstance::StartIsolatingSite(incognito->profile(), foo_url);
+  StartIsolatingSite(incognito->profile(), foo_url);
 
   AddBlankTabAndShow(incognito);
   ui_test_utils::NavigateToURL(incognito, foo_url);
@@ -2215,7 +2222,7 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationForPasswordSitesBrowserTest,
 
   // Isolate saved.com and verify it's been saved to disk.
   GURL saved_url(https_server.GetURL("saved.com", "/clear_site_data.html"));
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), saved_url);
+  StartIsolatingSite(browser()->profile(), saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("https://saved.com"));
 

diff --git a/chrome/browser/login_detection/login_detection_browsertest.cc b/chrome/browser/login_detection/login_detection_browsertest.cc
@@ -75,8 +75,11 @@ IN_PROC_BROWSER_TEST_F(LoginDetectionBrowserTest,
   ui_test_utils::NavigateToURL(browser(), test_url);
   ExpectLoginDetectionTypeMetric(LoginDetectionType::kNoLogin);
 
-  // Use site-isolaiton to save the site to manual passworded list.
-  content::SiteInstance::StartIsolatingSite(browser()->profile(), test_url);
+  // Use site isolation to save the site to manual passworded list.
+  content::SiteInstance::StartIsolatingSite(
+      browser()->profile(), test_url,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+          USER_TRIGGERED);
 
   // Subsequent navigation be detected as login.
   ResetHistogramTester();

diff --git a/components/password_manager/content/browser/content_password_manager_driver.cc b/components/password_manager/content/browser/content_password_manager_driver.cc
@@ -261,7 +261,9 @@ void ContentPasswordManagerDriver::InformAboutUserInput(
     // not used, such as on Android.
     content::SiteInstance::StartIsolatingSite(
         render_frame_host_->GetSiteInstance()->GetBrowserContext(),
-        form_data.url);
+        form_data.url,
+        content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+            USER_TRIGGERED);
   }
 }
 

diff --git a/components/site_isolation/site_isolation_policy.cc b/components/site_isolation/site_isolation_policy.cc
@@ -182,8 +182,14 @@ void SiteIsolationPolicy::IsolateNewOAuthURL(
   if (!IsIsolationForOAuthSitesEnabled())
     return;
 
-  content::SiteInstance::StartIsolatingSite(browser_context, signed_in_url,
-                                            false /* should_persist */);
+  // OAuth information is currently persisted and restored by other layers. See
+  // login_detection::prefs::SaveSiteToOAuthSignedInList().
+  constexpr bool kShouldPersist = false;
+
+  content::SiteInstance::StartIsolatingSite(
+      browser_context, signed_in_url,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource::USER_TRIGGERED,
+      kShouldPersist);
 }
 
 // static

diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -5294,9 +5294,11 @@ void RenderFrameHostImpl::MaybeIsolateForUserActivation() {
                           GetLastCommittedOrigin())
                     : true;
     if (is_same_origin_activation) {
-      SiteInstance::StartIsolatingSite(GetSiteInstance()->GetBrowserContext(),
-                                       GetMainFrame()->GetLastCommittedURL(),
-                                       false /* should_persist */);
+      SiteInstance::StartIsolatingSite(
+          GetSiteInstance()->GetBrowserContext(),
+          GetMainFrame()->GetLastCommittedURL(),
+          ChildProcessSecurityPolicy::IsolatedOriginSource::WEB_TRIGGERED,
+          false /* should_persist */);
     }
   }
 }

diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
@@ -1773,9 +1773,11 @@ bool SiteInstanceImpl::IsCrossOriginIsolated() const {
 }
 
 // static
-void SiteInstance::StartIsolatingSite(BrowserContext* context,
-                                      const GURL& url,
-                                      bool should_persist) {
+void SiteInstance::StartIsolatingSite(
+    BrowserContext* context,
+    const GURL& url,
+    ChildProcessSecurityPolicy::IsolatedOriginSource source,
+    bool should_persist) {
   if (!SiteIsolationPolicy::AreDynamicIsolatedOriginsEnabled())
     return;
 
@@ -1794,10 +1796,7 @@ void SiteInstance::StartIsolatingSite(BrowserContext* context,
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
   url::Origin site_origin(url::Origin::Create(site));
-  policy->AddFutureIsolatedOrigins(
-      {site_origin},
-      ChildProcessSecurityPolicy::IsolatedOriginSource::USER_TRIGGERED,
-      context);
+  policy->AddFutureIsolatedOrigins({site_origin}, source, context);
 
   // This function currently assumes the new isolated site should persist
   // across restarts, so ask the embedder to save it, excluding off-the-record

diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc
@@ -1584,20 +1584,28 @@ TEST_F(SiteInstanceTest, StartIsolatingSite) {
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
 
   // StartIsolatingSite() should convert the URL to a site before isolating it.
-  SiteInstance::StartIsolatingSite(context(),
-                                   GURL("http://bar.foo.com/foo/bar.html"));
+  SiteInstance::StartIsolatingSite(
+      context(), GURL("http://bar.foo.com/foo/html.bar"),
+      ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);
   EXPECT_TRUE(IsIsolatedOrigin(GURL("http://foo.com")));
-  SiteInstance::StartIsolatingSite(context(), GURL("https://a.b.c.com:8000/"));
+  SiteInstance::StartIsolatingSite(
+      context(), GURL("https://a.b.c.com:8000/"),
+      ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);
   EXPECT_TRUE(IsIsolatedOrigin(GURL("https://c.com")));
-  SiteInstance::StartIsolatingSite(context(),
-                                   GURL("http://bar.com/foo/bar.html"));
+  SiteInstance::StartIsolatingSite(
+      context(), GURL("http://bar.com/foo/bar.html"),
+      ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);
   EXPECT_TRUE(IsIsolatedOrigin(GURL("http://bar.com")));
 
   // Attempts to isolate an unsupported isolated origin should be ignored.
   GURL data_url("data:,");
   GURL blank_url(url::kAboutBlankURL);
-  SiteInstance::StartIsolatingSite(context(), data_url);
-  SiteInstance::StartIsolatingSite(context(), blank_url);
+  SiteInstance::StartIsolatingSite(
+      context(), data_url,
+      ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);
+  SiteInstance::StartIsolatingSite(
+      context(), blank_url,
+      ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);
   EXPECT_FALSE(IsIsolatedOrigin(data_url));
   EXPECT_FALSE(IsIsolatedOrigin(blank_url));
 

diff --git a/content/public/browser/site_instance.h b/content/public/browser/site_instance.h
@@ -10,6 +10,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/site_instance_process_assignment.h"
 #include "url/gurl.h"
 
@@ -216,7 +217,8 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
   // in via OAuth on that site).  The site will be determined from |url|'s
   // scheme and eTLD+1. If |context| is non-null, the site will be isolated
   // only within that BrowserContext; if |context| is null, the site will be
-  // isolated globally for all BrowserContexts.
+  // isolated globally for all BrowserContexts. |source| specifies why the new
+  // site is being isolated.
   //
   // Note that this has no effect if site isolation is turned off, such as via
   // the kDisableSiteIsolation cmdline flag or enterprise policy -- see also
@@ -230,9 +232,11 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
   // until the end of the current browsing session.  This is appropriate if the
   // site's persistence is not desired or is managed separately (e.g., sites
   // isolated due to OAuth logins are saved and in another component).
-  static void StartIsolatingSite(BrowserContext* context,
-                                 const GURL& url,
-                                 bool should_persist = true);
+  static void StartIsolatingSite(
+      BrowserContext* context,
+      const GURL& url,
+      ChildProcessSecurityPolicy::IsolatedOriginSource source,
+      bool should_persist = true);
 
  protected:
   friend class base::RefCounted<SiteInstance>;

diff --git a/weblayer/browser/password_manager_driver_factory.cc b/weblayer/browser/password_manager_driver_factory.cc
@@ -56,7 +56,9 @@ class PasswordManagerDriverFactory::PasswordManagerDriver
       // isolation is not used, such as on Android.
       content::SiteInstance::StartIsolatingSite(
           render_frame_host_->GetSiteInstance()->GetBrowserContext(),
-          form_data.url);
+          form_data.url,
+          content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+              USER_TRIGGERED);
     }
   }
   void DynamicFormSubmission(autofill::mojom::SubmissionIndicatorEvent

diff --git a/weblayer/browser/site_isolation_browsertest.cc b/weblayer/browser/site_isolation_browsertest.cc
@@ -84,6 +84,13 @@ class SiteIsolationBrowserTest : public WebLayerBrowserTest {
     return static_cast<TabImpl*>(shell()->tab())->web_contents();
   }
 
+  void StartIsolatingSite(const GURL& url) {
+    content::SiteInstance::StartIsolatingSite(
+        GetProfile()->GetBrowserContext(), url,
+        content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+            USER_TRIGGERED);
+  }
+
  private:
   // A browser client which forces off strict site isolation, so the test can
   // assume password isolation is enabled.
@@ -151,12 +158,10 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationBrowserTest,
   // Isolate saved.com and saved2.com persistently.
   GURL saved_url =
       embedded_test_server()->GetURL("saved.com", "/simple_page.html");
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
+  StartIsolatingSite(saved_url);
   GURL saved2_url =
       embedded_test_server()->GetURL("saved2.com", "/simple_page.html");
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved2_url);
+  StartIsolatingSite(saved2_url);
 
   NavigateAndWaitForCompletion(saved_url, shell());
   EXPECT_TRUE(GetWebContents()
@@ -199,12 +204,9 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationBrowserTest,
 IN_PROC_BROWSER_TEST_F(SiteIsolationBrowserTest, IsolatedSiteIsSavedOnlyOnce) {
   GURL saved_url =
       embedded_test_server()->GetURL("saved.com", "/simple_page.html");
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
+  StartIsolatingSite(saved_url);
+  StartIsolatingSite(saved_url);
+  StartIsolatingSite(saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("http://saved.com"));
 }
@@ -221,8 +223,7 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationBrowserTest,
 
   // Isolate saved.com and verify it's been saved to disk.
   GURL saved_url = https_server.GetURL("saved.com", "/clear_site_data.html");
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
+  StartIsolatingSite(saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("https://saved.com"));
 
@@ -238,8 +239,7 @@ IN_PROC_BROWSER_TEST_F(SiteIsolationBrowserTest,
                        ExplicitClearBrowsingDataClearsSavedIsolatedSites) {
   GURL saved_url =
       embedded_test_server()->GetURL("saved.com", "/simple_page.html");
-  content::SiteInstance::StartIsolatingSite(GetProfile()->GetBrowserContext(),
-                                            saved_url);
+  StartIsolatingSite(saved_url);
   EXPECT_THAT(GetSavedIsolatedSites(),
               UnorderedElementsAre("http://saved.com"));