Unify isolated origin persistence code and plumb IsolatedOriginSource…

… to it. The Chrome and Weblayer implementations of ContentBrowserClient::PersistIsolatedOrigin() basically do the same thing. This CL moves this code into a common location in the site_isolation component. It also plumbs through the IsolatedOriginSource for the origin being persisted. Currently, this should always be USER_TRIGGERED, but the persistence implementation will soon be expanded to also support WEB_TRIGGERED sources (with different policies and in a different pref). This refactor will allow us to do this in just one common place. Bug: 1018656 Change-Id: I26be003b3527aed8dcec9f7731971ea9559a28f6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875962 Reviewed-by: Clark DuVall <cduvall@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#880575}
Denger-Network · May 7, 2021 · 54aa00d · 54aa00d
1 parent bdf67a8
commit 54aa00d
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 28 deletions.
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -2099,15 +2099,10 @@ ChromeContentBrowserClient::GetAdditionalSiteIsolationModes() {
 
 void ChromeContentBrowserClient::PersistIsolatedOrigin(
     content::BrowserContext* context,
-    const url::Origin& origin) {
-  DCHECK(!context->IsOffTheRecord());
-  Profile* profile = Profile::FromBrowserContext(context);
-  ListPrefUpdate update(profile->GetPrefs(),
-                        site_isolation::prefs::kUserTriggeredIsolatedOrigins);
-  base::ListValue* list = update.Get();
-  base::Value value(origin.Serialize());
-  if (!base::Contains(list->GetList(), value))
-    list->Append(std::move(value));
+    const url::Origin& origin,
+    content::ChildProcessSecurityPolicy::IsolatedOriginSource source) {
+  site_isolation::SiteIsolationPolicy::PersistIsolatedOrigin(context, origin,
+                                                             source);
 }
 
 bool ChromeContentBrowserClient::IsFileAccessAllowed(

diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
@@ -23,6 +23,7 @@
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/startup_data.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/buildflags/buildflags.h"
@@ -217,8 +218,11 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   bool ShouldEnableStrictSiteIsolation() override;
   bool ShouldDisableSiteIsolation() override;
   std::vector<std::string> GetAdditionalSiteIsolationModes() override;
-  void PersistIsolatedOrigin(content::BrowserContext* context,
-                             const url::Origin& origin) override;
+  void PersistIsolatedOrigin(
+      content::BrowserContext* context,
+      const url::Origin& origin,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource source)
+      override;
   bool IsFileAccessAllowed(const base::FilePath& path,
                            const base::FilePath& absolute_path,
                            const base::FilePath& profile_path) override;

diff --git a/components/site_isolation/site_isolation_policy.cc b/components/site_isolation/site_isolation_policy.cc
@@ -9,11 +9,11 @@
 #include "base/system/sys_info.h"
 #include "build/build_config.h"
 #include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
 #include "components/site_isolation/features.h"
 #include "components/site_isolation/pref_names.h"
 #include "components/user_prefs/user_prefs.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/site_isolation_policy.h"
 
@@ -129,6 +129,24 @@ bool SiteIsolationPolicy::ShouldDisableSiteIsolationDueToMemoryThreshold() {
   return false;
 }
 
+// static
+void SiteIsolationPolicy::PersistIsolatedOrigin(
+    content::BrowserContext* context,
+    const url::Origin& origin,
+    content::ChildProcessSecurityPolicy::IsolatedOriginSource source) {
+  DCHECK(!context->IsOffTheRecord());
+  // TODO(alexmos): Support web-triggered IsolatedOriginSources.
+  DCHECK_EQ(source, content::ChildProcessSecurityPolicy::IsolatedOriginSource::
+                        USER_TRIGGERED);
+
+  ListPrefUpdate update(user_prefs::UserPrefs::Get(context),
+                        site_isolation::prefs::kUserTriggeredIsolatedOrigins);
+  base::ListValue* list = update.Get();
+  base::Value value(origin.Serialize());
+  if (!base::Contains(list->GetList(), value))
+    list->Append(std::move(value));
+}
+
 // static
 void SiteIsolationPolicy::ApplyPersistedIsolatedOrigins(
     content::BrowserContext* browser_context) {

diff --git a/components/site_isolation/site_isolation_policy.h b/components/site_isolation/site_isolation_policy.h
@@ -5,10 +5,11 @@
 #ifndef COMPONENTS_SITE_ISOLATION_SITE_ISOLATION_POLICY_H_
 #define COMPONENTS_SITE_ISOLATION_SITE_ISOLATION_POLICY_H_
 
-#include "base/macros.h"
-
 #include <vector>
 
+#include "base/macros.h"
+#include "content/public/browser/child_process_security_policy.h"
+
 class GURL;
 
 namespace content {
@@ -42,6 +43,15 @@ class SiteIsolationPolicy {
   // Spectre-like attacks on such devices).
   static bool IsEnterprisePolicyApplicable();
 
+  // Saves a new dynamic isolated origin to user prefs associated with
+  // `context` so that it can be persisted across restarts. `source`
+  // specifies why the isolated origin was added; different sources may have
+  // different persistence policies.
+  static void PersistIsolatedOrigin(
+      content::BrowserContext* context,
+      const url::Origin& origin,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource source);
+
   // Reads and applies any isolated origins stored in user prefs associated with
   // |browser_context|.  This is expected to be called on startup after user
   // prefs have been loaded.

diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
@@ -1801,8 +1801,10 @@ void SiteInstance::StartIsolatingSite(
   // This function currently assumes the new isolated site should persist
   // across restarts, so ask the embedder to save it, excluding off-the-record
   // profiles.
-  if (!context->IsOffTheRecord() && should_persist)
-    GetContentClient()->browser()->PersistIsolatedOrigin(context, site_origin);
+  if (!context->IsOffTheRecord() && should_persist) {
+    GetContentClient()->browser()->PersistIsolatedOrigin(context, site_origin,
+                                                         source);
+  }
 }
 
 void SiteInstanceImpl::WriteIntoTrace(perfetto::TracedValue context) {

diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
@@ -29,6 +29,7 @@
 #include "content/common/content_export.h"
 #include "content/public/browser/allow_service_worker_result.h"
 #include "content/public/browser/certificate_request_result_type.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/generated_code_cache_settings.h"
 #include "content/public/browser/mojo_binder_policy_map.h"
 #include "content/public/browser/storage_partition_config.h"
@@ -589,8 +590,10 @@ class CONTENT_EXPORT ContentBrowserClient {
   // Called when a new dynamic isolated origin was added in |context|, and the
   // origin desires to be persisted across restarts, to give the embedder an
   // opportunity to save this isolated origin to disk.
-  virtual void PersistIsolatedOrigin(BrowserContext* context,
-                                     const url::Origin& origin) {}
+  virtual void PersistIsolatedOrigin(
+      BrowserContext* context,
+      const url::Origin& origin,
+      ChildProcessSecurityPolicy::IsolatedOriginSource source) {}
 
   // Indicates whether a file path should be accessible via file URL given a
   // request from a browser context which lives within |profile_path|.

diff --git a/weblayer/browser/content_browser_client_impl.cc b/weblayer/browser/content_browser_client_impl.cc
@@ -638,14 +638,10 @@ ContentBrowserClientImpl::GetAdditionalSiteIsolationModes() {
 
 void ContentBrowserClientImpl::PersistIsolatedOrigin(
     content::BrowserContext* context,
-    const url::Origin& origin) {
-  DCHECK(!context->IsOffTheRecord());
-  ListPrefUpdate update(user_prefs::UserPrefs::Get(context),
-                        site_isolation::prefs::kUserTriggeredIsolatedOrigins);
-  base::ListValue* list = update.Get();
-  base::Value value(origin.Serialize());
-  if (!base::Contains(list->GetList(), value))
-    list->Append(std::move(value));
+    const url::Origin& origin,
+    content::ChildProcessSecurityPolicy::IsolatedOriginSource source) {
+  site_isolation::SiteIsolationPolicy::PersistIsolatedOrigin(context, origin,
+                                                             source);
 }
 
 base::OnceClosure ContentBrowserClientImpl::SelectClientCertificate(

diff --git a/weblayer/browser/content_browser_client_impl.h b/weblayer/browser/content_browser_client_impl.h
@@ -13,6 +13,7 @@
 #include "base/files/file_path.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/content_browser_client.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 
@@ -109,8 +110,11 @@ class ContentBrowserClientImpl : public content::ContentBrowserClient {
       content::PageVisibilityState* visibility_state) override;
   bool ShouldDisableSiteIsolation() override;
   std::vector<std::string> GetAdditionalSiteIsolationModes() override;
-  void PersistIsolatedOrigin(content::BrowserContext* context,
-                             const url::Origin& origin) override;
+  void PersistIsolatedOrigin(
+      content::BrowserContext* context,
+      const url::Origin& origin,
+      content::ChildProcessSecurityPolicy::IsolatedOriginSource source)
+      override;
   base::OnceClosure SelectClientCertificate(
       content::WebContents* web_contents,
       net::SSLCertRequestInfo* cert_request_info,