Cache Storage Permissions on ContentSettingsClientImpl

The result of the IPC call GetContentSettingsManager::AllowStorageAccess
was cached to prevent further redundant IPC calls. But this was only
done for local and session storage permissions (via the AllowStorage()
method).

This change consolidates the caching logic to be used for all storage
types. We also remove the AllowStorage() method and integrate it into
the general purpose AllowStorageAccess() methods, adding new storage
types for local and session storage.

IDBFactory::CachedAllowIndexedDB() is also removed as it is now
redundant.

Change-Id: I518cf05bc1e12feec25c96aa5c1cd2cefebd1a29
Bug: 1122633
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401847Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Shubham Aggarwal <shuagga@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#809892}

components/content_settings/renderer/content_settings_agent_impl.cc

@@ -263,21 +263,20 @@ void ContentSettingsAgentImpl::OnContentSettingsAgentRequest(
 mojom::ContentSettingsManager::StorageType
 ContentSettingsAgentImpl::ConvertToMojoStorageType(StorageType storage_type) {
   switch (storage_type) {
-    case StorageType::kDatabase: {
+    case StorageType::kDatabase:
       return mojom::ContentSettingsManager::StorageType::DATABASE;
-    }
-    case StorageType::kIndexedDB: {
+    case StorageType::kIndexedDB:
       return mojom::ContentSettingsManager::StorageType::INDEXED_DB;
-    }
-    case StorageType::kCacheStorage: {
+    case StorageType::kCacheStorage:
       return mojom::ContentSettingsManager::StorageType::CACHE;
-    }
-    case StorageType::kWebLocks: {
+    case StorageType::kWebLocks:
       return mojom::ContentSettingsManager::StorageType::WEB_LOCKS;
-    }
-    case StorageType::kFileSystem: {
+    case StorageType::kFileSystem:
       return mojom::ContentSettingsManager::StorageType::FILE_SYSTEM;
-    }
+    case StorageType::kLocalStorage:
+      return mojom::ContentSettingsManager::StorageType::LOCAL_STORAGE;
+    case StorageType::kSessionStorage:
+      return mojom::ContentSettingsManager::StorageType::SESSION_STORAGE;
   }
 }
 
@@ -290,11 +289,30 @@ void ContentSettingsAgentImpl::AllowStorageAccess(
     return;
   }
 
+  StoragePermissionsKey key(url::Origin(frame->GetSecurityOrigin()),
+                            storage_type);
+  const auto permissions = cached_storage_permissions_.find(key);
+  if (permissions != cached_storage_permissions_.end()) {
+    std::move(callback).Run(permissions->second);
+    return;
+  }
+
+  // Passing the |cache_storage_permissions_| ref to the callback is safe here
+  // as the mojo::Remote is owned by |this| and won't invoke the callback if
+  // |this| (and in turn |cache_storage_permissions_|) is destroyed.
+  base::OnceCallback<void(bool)> new_cb = base::BindOnce(
+      [](base::OnceCallback<void(bool)> original_cb, StoragePermissionsKey key,
+         base::flat_map<StoragePermissionsKey, bool>& cache_map, bool result) {
+        cache_map[key] = result;
+        std::move(original_cb).Run(result);
+      },
+      std::move(callback), key, std::ref(cached_storage_permissions_));
+
   GetContentSettingsManager().AllowStorageAccess(
       routing_id(), ConvertToMojoStorageType(storage_type),
       frame->GetSecurityOrigin(),
       frame->GetDocument().SiteForCookies().RepresentativeUrl(),
-      frame->GetDocument().TopFrameOrigin(), std::move(callback));
+      frame->GetDocument().TopFrameOrigin(), std::move(new_cb));
 }
 
 bool ContentSettingsAgentImpl::AllowStorageAccessSync(
@@ -303,12 +321,19 @@ bool ContentSettingsAgentImpl::AllowStorageAccessSync(
   if (IsFrameWithOpaqueOrigin(frame))
     return false;
 
+  StoragePermissionsKey key(url::Origin(frame->GetSecurityOrigin()),
+                            storage_type);
+  const auto permissions = cached_storage_permissions_.find(key);
+  if (permissions != cached_storage_permissions_.end())
+    return permissions->second;
+
   bool result = false;
   GetContentSettingsManager().AllowStorageAccess(
       routing_id(), ConvertToMojoStorageType(storage_type),
       frame->GetSecurityOrigin(),
       frame->GetDocument().SiteForCookies().RepresentativeUrl(),
       frame->GetDocument().TopFrameOrigin(), &result);
+  cached_storage_permissions_[key] = result;
   return result;
 }
 
@@ -382,29 +407,6 @@ bool ContentSettingsAgentImpl::AllowScriptFromSource(
   return allow || IsWhitelistedForContentSettings();
 }
 
-bool ContentSettingsAgentImpl::AllowStorage(bool local) {
-  WebLocalFrame* frame = render_frame()->GetWebFrame();
-  if (IsFrameWithOpaqueOrigin(frame))
-    return false;
-
-  StoragePermissionsKey key(
-      url::Origin(frame->GetDocument().GetSecurityOrigin()).GetURL(), local);
-  const auto permissions = cached_storage_permissions_.find(key);
-  if (permissions != cached_storage_permissions_.end())
-    return permissions->second;
-
-  bool result = false;
-  GetContentSettingsManager().AllowStorageAccess(
-      routing_id(),
-      local ? mojom::ContentSettingsManager::StorageType::LOCAL_STORAGE
-            : mojom::ContentSettingsManager::StorageType::SESSION_STORAGE,
-      frame->GetSecurityOrigin(),
-      frame->GetDocument().SiteForCookies().RepresentativeUrl(),
-      frame->GetDocument().TopFrameOrigin(), &result);
-  cached_storage_permissions_[key] = result;
-  return result;
-}
-
 bool ContentSettingsAgentImpl::AllowReadFromClipboard(bool default_value) {
   return delegate_->AllowReadFromClipboard().value_or(default_value);
 }

components/content_settings/renderer/content_settings_agent_impl.h

@@ -24,6 +24,7 @@
 #include "mojo/public/cpp/bindings/associated_remote.h"
 #include "third_party/blink/public/platform/web_content_settings_client.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace blink {
 class WebFrame;
@@ -91,7 +92,6 @@ class ContentSettingsAgentImpl
   bool AllowScript(bool enabled_per_settings) override;
   bool AllowScriptFromSource(bool enabled_per_settings,
                              const blink::WebURL& script_url) override;
-  bool AllowStorage(bool local) override;
   bool AllowReadFromClipboard(bool default_value) override;
   bool AllowWriteToClipboard(bool default_value) override;
   bool AllowMutationEvents(bool default_value) override;
@@ -166,8 +166,8 @@ class ContentSettingsAgentImpl
   // Stores if images, scripts, and plugins have actually been blocked.
   base::flat_set<ContentSettingsType> content_blocked_;
 
-  // Caches the result of AllowStorage.
-  using StoragePermissionsKey = std::pair<GURL, bool>;
+  // Caches the result of AllowStorageAccess.
+  using StoragePermissionsKey = std::pair<url::Origin, StorageType>;
   base::flat_map<StoragePermissionsKey, bool> cached_storage_permissions_;
 
   // Caches the result of AllowScript.

components/content_settings/renderer/content_settings_agent_impl_browsertest.cc

@@ -223,18 +223,41 @@ TEST_F(ContentSettingsAgentImplBrowserTest, DidBlockContentType) {
   EXPECT_EQ(1, mock_agent.on_content_blocked_count());
 }
 
-// Tests that multiple invokations of AllowDOMStorage result in a single IPC.
-TEST_F(ContentSettingsAgentImplBrowserTest, AllowDOMStorage) {
+// Tests that multiple invocations of AllowStorageAccessSync result in a single
+// IPC.
+TEST_F(ContentSettingsAgentImplBrowserTest, AllowStorageAccessSync) {
   // Load some HTML, so we have a valid security origin.
   LoadHTMLWithUrlOverride("<html></html>", "https://example.com/");
   MockContentSettingsAgentImpl mock_agent(view_->GetMainRenderFrame());
-  mock_agent.AllowStorage(true);
+  mock_agent.AllowStorageAccessSync(
+      blink::WebContentSettingsClient::StorageType::kLocalStorage);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, mock_agent.allow_storage_access_count());
 
   // Accessing localStorage from the same origin again shouldn't result in a
   // new IPC.
-  mock_agent.AllowStorage(true);
+  mock_agent.AllowStorageAccessSync(
+      blink::WebContentSettingsClient::StorageType::kLocalStorage);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, mock_agent.allow_storage_access_count());
+}
+
+// Tests that multiple invocations of AllowStorageAccess result in a single IPC.
+TEST_F(ContentSettingsAgentImplBrowserTest, AllowStorageAccess) {
+  // Load some HTML, so we have a valid security origin.
+  LoadHTMLWithUrlOverride("<html></html>", "https://example.com/");
+  MockContentSettingsAgentImpl mock_agent(view_->GetMainRenderFrame());
+  mock_agent.AllowStorageAccess(
+      blink::WebContentSettingsClient::StorageType::kLocalStorage,
+      base::DoNothing());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, mock_agent.allow_storage_access_count());
+
+  // Accessing localStorage from the same origin again shouldn't result in a
+  // new IPC.
+  mock_agent.AllowStorageAccess(
+      blink::WebContentSettingsClient::StorageType::kLocalStorage,
+      base::DoNothing());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, mock_agent.allow_storage_access_count());
 }

content/web_test/renderer/web_test_content_settings_client.cc

@@ -53,7 +53,8 @@ bool WebTestContentSettingsClient::AllowScriptFromSource(
   return allowed;
 }
 
-bool WebTestContentSettingsClient::AllowStorage(bool enabled_per_settings) {
+bool WebTestContentSettingsClient::AllowStorageAccessSync(
+    StorageType storage_type) {
   return flags_->storage_allowed();
 }
 

content/web_test/renderer/web_test_content_settings_client.h

@@ -36,7 +36,7 @@ class WebTestContentSettingsClient : public blink::WebContentSettingsClient {
   bool AllowScript(bool enabled_per_settings) override;
   bool AllowScriptFromSource(bool enabled_per_settings,
                              const blink::WebURL& script_url) override;
-  bool AllowStorage(bool local) override;
+  bool AllowStorageAccessSync(StorageType storage_type) override;
   bool AllowRunningInsecureContent(bool enabled_per_settings,
                                    const blink::WebURL& url) override;
 

third_party/blink/public/platform/web_content_settings_client.h

@@ -29,7 +29,9 @@ class WebContentSettingsClient {
     kCacheStorage,
     kIndexedDB,
     kFileSystem,
-    kWebLocks
+    kWebLocks,
+    kLocalStorage,
+    kSessionStorage
   };
 
   // Controls whether access to the given StorageType is allowed for this frame.
@@ -66,11 +68,6 @@ class WebContentSettingsClient {
     return enabled_per_settings;
   }
 
-  // Controls whether HTML5 Web Storage is allowed for this frame.
-  // If local is true, then this is for local storage, otherwise it's for
-  // session storage.
-  virtual bool AllowStorage(bool local) { return true; }
-
   // Controls whether access to read the clipboard is allowed for this frame.
   virtual bool AllowReadFromClipboard(bool default_value) {
     return default_value;

third_party/blink/renderer/core/workers/shared_worker_content_settings_proxy.cc

@@ -35,7 +35,7 @@ bool SharedWorkerContentSettingsProxy::AllowStorageAccessSync(
       GetService()->RequestFileSystemAccessSync(&result);
       break;
     }
-    case StorageType::kDatabase: {
+    default: {
       // TODO(shuagga@microsoft.com): Revisit this default in the future.
       return true;
     }

third_party/blink/renderer/modules/indexeddb/idb_factory.cc

@@ -268,7 +268,7 @@ ScriptPromise IDBFactory::GetDatabaseInfo(ScriptState* script_state,
     return resolver->Promise();
   }
 
-  if (!CachedAllowIndexedDB(script_state)) {
+  if (!AllowIndexedDB(script_state)) {
     exception_state.ThrowDOMException(DOMExceptionCode::kUnknownError,
                                       kPermissionDeniedErrorMessage);
     resolver->Reject();
@@ -304,7 +304,7 @@ IDBRequest* IDBFactory::GetDatabaseNames(ScriptState* script_state,
                       WebFeature::kFileAccessedDatabase);
   }
 
-  if (!CachedAllowIndexedDB(script_state)) {
+  if (!AllowIndexedDB(script_state)) {
     request->HandleResponse(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kUnknownError, kPermissionDeniedErrorMessage));
     return request;
@@ -365,7 +365,7 @@ IDBOpenDBRequest* IDBFactory::OpenInternal(ScriptState* script_state,
       script_state, database_callbacks, std::move(transaction_backend),
       transaction_id, version, std::move(metrics), GetObservedFeature());
 
-  if (!CachedAllowIndexedDB(script_state)) {
+  if (!AllowIndexedDB(script_state)) {
     request->HandleResponse(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kUnknownError, kPermissionDeniedErrorMessage));
     return request;
@@ -436,7 +436,7 @@ IDBOpenDBRequest* IDBFactory::DeleteDatabaseInternal(
       IDBDatabaseMetadata::kDefaultVersion, std::move(metrics),
       GetObservedFeature());
 
-  if (!CachedAllowIndexedDB(script_state)) {
+  if (!AllowIndexedDB(script_state)) {
     request->HandleResponse(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kUnknownError, kPermissionDeniedErrorMessage));
     return request;
@@ -506,14 +506,6 @@ bool IDBFactory::AllowIndexedDB(ScriptState* script_state) {
       WebContentSettingsClient::StorageType::kIndexedDB);
 }
 
-bool IDBFactory::CachedAllowIndexedDB(ScriptState* script_state) {
-  if (!cached_allowed_.has_value()) {
-    // Cache the AllowIndexedDB() call because it triggers a sync IPC.
-    cached_allowed_.emplace(AllowIndexedDB(script_state));
-  }
-  return cached_allowed_.value();
-}
-
 mojo::PendingAssociatedRemote<mojom::blink::IDBCallbacks>
 IDBFactory::GetCallbacksProxy(std::unique_ptr<WebIDBCallbacks> callbacks_impl) {
   mojo::PendingAssociatedRemote<mojom::blink::IDBCallbacks> pending_callbacks;

third_party/blink/renderer/modules/indexeddb/idb_factory.h

@@ -95,7 +95,6 @@ class MODULES_EXPORT IDBFactory final : public ScriptWrappable {
                                            bool);
 
   bool AllowIndexedDB(ScriptState* script_state);
-  bool CachedAllowIndexedDB(ScriptState* script_state);
 
   mojo::PendingAssociatedRemote<mojom::blink::IDBCallbacks> GetCallbacksProxy(
       std::unique_ptr<WebIDBCallbacks> callbacks);
@@ -107,8 +106,6 @@ class MODULES_EXPORT IDBFactory final : public ScriptWrappable {
   mojo::Remote<mojom::blink::IDBFactory> factory_;
   mojo::Remote<mojom::blink::FeatureObserver> feature_observer_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-
-  base::Optional<bool> cached_allowed_;
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/storage/storage_controller.cc

@@ -56,8 +56,14 @@ StorageController* StorageController::GetInstance() {
 bool StorageController::CanAccessStorageArea(LocalFrame* frame,
                                              StorageArea::StorageType type) {
   if (auto* settings_client = frame->GetContentSettingsClient()) {
-    return settings_client->AllowStorage(
-        type == StorageArea::StorageType::kLocalStorage);
+    switch (type) {
+      case StorageArea::StorageType::kLocalStorage:
+        return settings_client->AllowStorageAccessSync(
+            WebContentSettingsClient::StorageType::kLocalStorage);
+      case StorageArea::StorageType::kSessionStorage:
+        return settings_client->AllowStorageAccessSync(
+            WebContentSettingsClient::StorageType::kSessionStorage);
+    }
   }
   return true;
 }