M96: [FileAPI] Move origin checks in BlobURLStore sooner.

Rather than waiting to verify if a valid origin was passed in until
register/revoke time, check if the origin is valid at the time the
mojo interface is requested. This avoids the need to pass the
delegate on to BlobURLStoreImpl, further decoupling this from
BlobRegistryImpl.

(cherry picked from commit 15cfa2bed3ce9dcdd0a06d3cd3b8330e3591acdc)

Bug: 1262183
Change-Id: I4889685d03501158abfe4f87c647dc468d005f78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3264353
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#940015}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3285385
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1078}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}

diff --git a/chrome/browser/chrome_security_exploit_browsertest.cc b/chrome/browser/chrome_security_exploit_browsertest.cc
index 5388f84..15d887d 100644
--- a/chrome/browser/chrome_security_exploit_browsertest.cc
+++ b/chrome/browser/chrome_security_exploit_browsertest.cc

@@ -497,8 +497,8 @@
 
   // If the process is killed, this test passes.
   EXPECT_EQ(
-      "Received bad user message: Non committable URL passed to "
-      "BlobURLStore::Register",
+      "Received bad user message: "
+      "URL with invalid origin passed to BlobURLStore::Register",
       crash_observer.Wait());
 }
 
@@ -541,7 +541,7 @@
 
   // If the process is killed, this test passes.
   EXPECT_EQ(
-      "Received bad user message: Non committable URL passed to "
-      "BlobURLStore::Register",
+      "Received bad user message: "
+      "URL with invalid origin passed to BlobURLStore::Register",
       crash_observer.Wait());
 }

diff --git a/content/browser/blob_storage/blob_url_unittest.cc b/content/browser/blob_storage/blob_url_unittest.cc
index a058081..1f206e4 100644
--- a/content/browser/blob_storage/blob_url_unittest.cc
+++ b/content/browser/blob_storage/blob_url_unittest.cc

@@ -187,15 +187,14 @@
 
   void TestRequest(const std::string& method,
                    const net::HttpRequestHeaders& extra_headers) {
-    GURL url("blob:blah");
+    auto origin = url::Origin::Create(GURL("https://example.com"));
+    auto url = GURL("blob:" + origin.Serialize() + "/id1");
     network::ResourceRequest request;
     request.url = url;
     request.method = method;
     request.headers = extra_headers;
 
-    storage::MockBlobRegistryDelegate delegate;
-    storage::BlobURLStoreImpl url_store(blob_url_registry_.AsWeakPtr(),
-                                        &delegate);
+    storage::BlobURLStoreImpl url_store(origin, blob_url_registry_.AsWeakPtr());
 
     mojo::PendingRemote<blink::mojom::Blob> blob_remote;
     storage::BlobImpl::Create(

diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 6fbf9e5..d83d0602 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc

@@ -767,7 +767,7 @@
   // If the process is killed, this test passes.
   EXPECT_EQ(
       "Received bad user message: "
-      "Non committable URL passed to BlobURLStore::Register",
+      "URL with invalid origin passed to BlobURLStore::Register",
       crash_observer.Wait());
 }
 

diff --git a/storage/browser/blob/blob_registry_impl.cc b/storage/browser/blob/blob_registry_impl.cc
index cdb4946..b6ba9e9 100644
--- a/storage/browser/blob/blob_registry_impl.cc
+++ b/storage/browser/blob/blob_registry_impl.cc

@@ -642,13 +642,14 @@
 void BlobRegistryImpl::URLStoreForOrigin(
     const url::Origin& origin,
     mojo::PendingAssociatedReceiver<blink::mojom::BlobURLStore> receiver) {
-  // TODO(mek): Pass origin on to BlobURLStoreImpl so it can use it to generate
-  // Blob URLs, and verify at this point that the renderer can create URLs for
-  // that origin.
   Delegate* delegate = receivers_.current_context().get();
   DCHECK(delegate);
+  if (!origin.opaque() && !delegate->CanCommitURL(origin.GetURL())) {
+    mojo::ReportBadMessage(
+        "Non committable origin passed to BlobRegistryImpl::URLStoreForOrigin");
+  }
   auto self_owned_associated_receiver = mojo::MakeSelfOwnedAssociatedReceiver(
-      std::make_unique<BlobURLStoreImpl>(url_registry_, delegate),
+      std::make_unique<BlobURLStoreImpl>(origin, url_registry_),
       std::move(receiver));
   if (g_url_store_creation_hook)
     g_url_store_creation_hook->Run(self_owned_associated_receiver);

diff --git a/storage/browser/blob/blob_url_store_impl.cc b/storage/browser/blob/blob_url_store_impl.cc
index 09a93bc..de63745 100644
--- a/storage/browser/blob/blob_url_store_impl.cc
+++ b/storage/browser/blob/blob_url_store_impl.cc

@@ -5,6 +5,7 @@
 #include "storage/browser/blob/blob_url_store_impl.h"
 
 #include "base/bind.h"
+#include "base/strings/strcat.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "storage/browser/blob/blob_impl.h"
 #include "storage/browser/blob/blob_url_loader_factory.h"
@@ -58,9 +59,9 @@
   const base::UnguessableToken token_;
 };
 
-BlobURLStoreImpl::BlobURLStoreImpl(base::WeakPtr<BlobUrlRegistry> registry,
-                                   BlobRegistryImpl::Delegate* delegate)
-    : registry_(std::move(registry)), delegate_(delegate) {}
+BlobURLStoreImpl::BlobURLStoreImpl(const url::Origin& origin,
+                                   base::WeakPtr<BlobUrlRegistry> registry)
+    : origin_(origin), registry_(std::move(registry)) {}
 
 BlobURLStoreImpl::~BlobURLStoreImpl() {
   if (registry_) {
@@ -75,20 +76,9 @@
     // TODO(https://crbug.com/1224926): Remove this once experiment is over.
     const base::UnguessableToken& unsafe_agent_cluster_id,
     RegisterCallback callback) {
-  if (!url.SchemeIsBlob()) {
-    mojo::ReportBadMessage("Invalid scheme passed to BlobURLStore::Register");
-    std::move(callback).Run();
-    return;
-  }
-  if (!delegate_->CanCommitURL(url)) {
-    mojo::ReportBadMessage(
-        "Non committable URL passed to BlobURLStore::Register");
-    std::move(callback).Run();
-    return;
-  }
-  if (BlobUrlUtils::UrlHasFragment(url)) {
-    mojo::ReportBadMessage(
-        "URL with fragment passed to BlobURLStore::Register");
+  // TODO(mek): Generate blob URLs here, rather than validating the URLs the
+  // renderer process generated.
+  if (!BlobUrlIsValid(url, "Register")) {
     std::move(callback).Run();
     return;
   }
@@ -100,19 +90,8 @@
 }
 
 void BlobURLStoreImpl::Revoke(const GURL& url) {
-  if (!url.SchemeIsBlob()) {
-    mojo::ReportBadMessage("Invalid scheme passed to BlobURLStore::Revoke");
+  if (!BlobUrlIsValid(url, "Revoke"))
     return;
-  }
-  if (!delegate_->CanCommitURL(url)) {
-    mojo::ReportBadMessage(
-        "Non committable URL passed to BlobURLStore::Revoke");
-    return;
-  }
-  if (BlobUrlUtils::UrlHasFragment(url)) {
-    mojo::ReportBadMessage("URL with fragment passed to BlobURLStore::Revoke");
-    return;
-  }
 
   if (registry_)
     registry_->RemoveUrlMapping(url);
@@ -156,4 +135,38 @@
   std::move(callback).Run(registry_->GetUnsafeAgentClusterID(url));
 }
 
+bool BlobURLStoreImpl::BlobUrlIsValid(const GURL& url,
+                                      const char* method) const {
+  if (!url.SchemeIsBlob()) {
+    mojo::ReportBadMessage(
+        base::StrCat({"Invalid scheme passed to BlobURLStore::", method}));
+    return false;
+  }
+  url::Origin url_origin = url::Origin::Create(url);
+  // For file:// origins blink sometimes creates blob URLs with "null" as origin
+  // and other times "file://" (based on a runtime setting). On the other hand,
+  // `origin_` will always be a non-opaque file: origin for pages loaded from
+  // file:// URLs. To deal with this, we treat file:// origins and
+  // opaque origins separately from non-opaque origins.
+  bool valid_origin = true;
+  if (url_origin.scheme() == url::kFileScheme) {
+    valid_origin = origin_.scheme() == url::kFileScheme;
+  } else if (url_origin.opaque()) {
+    valid_origin = origin_.opaque() || origin_.scheme() == url::kFileScheme;
+  } else {
+    valid_origin = origin_ == url_origin;
+  }
+  if (!valid_origin) {
+    mojo::ReportBadMessage(base::StrCat(
+        {"URL with invalid origin passed to BlobURLStore::", method}));
+    return false;
+  }
+  if (BlobUrlUtils::UrlHasFragment(url)) {
+    mojo::ReportBadMessage(
+        base::StrCat({"URL with fragment passed to BlobURLStore::", method}));
+    return false;
+  }
+  return true;
+}
+
 }  // namespace storage

diff --git a/storage/browser/blob/blob_url_store_impl.h b/storage/browser/blob/blob_url_store_impl.h
index f68c5a5..3db3a95d 100644
--- a/storage/browser/blob/blob_url_store_impl.h
+++ b/storage/browser/blob/blob_url_store_impl.h

@@ -12,8 +12,9 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
-#include "storage/browser/blob/blob_registry_impl.h"
+#include "storage/browser/blob/blob_url_registry.h"
 #include "third_party/blink/public/mojom/blob/blob_url_store.mojom.h"
+#include "url/origin.h"
 
 namespace storage {
 
@@ -22,8 +23,8 @@
 class COMPONENT_EXPORT(STORAGE_BROWSER) BlobURLStoreImpl
     : public blink::mojom::BlobURLStore {
  public:
-  BlobURLStoreImpl(base::WeakPtr<BlobUrlRegistry> registry,
-                   BlobRegistryImpl::Delegate* delegate);
+  BlobURLStoreImpl(const url::Origin& origin,
+                   base::WeakPtr<BlobUrlRegistry> registry);
 
   BlobURLStoreImpl(const BlobURLStoreImpl&) = delete;
   BlobURLStoreImpl& operator=(const BlobURLStoreImpl&) = delete;
@@ -48,8 +49,12 @@
       ResolveForNavigationCallback callback) override;
 
  private:
+  // Checks if the passed in url is a valid blob url for this blob url store.
+  // Returns false and reports a bad mojo message if not.
+  bool BlobUrlIsValid(const GURL& url, const char* method) const;
+
+  const url::Origin origin_;
   base::WeakPtr<BlobUrlRegistry> registry_;
-  BlobRegistryImpl::Delegate* delegate_;
 
   std::set<GURL> urls_;
 

diff --git a/storage/browser/blob/blob_url_store_impl_unittest.cc b/storage/browser/blob/blob_url_store_impl_unittest.cc
index daf2ea3..04c4479 100644
--- a/storage/browser/blob/blob_url_store_impl_unittest.cc
+++ b/storage/browser/blob/blob_url_store_impl_unittest.cc

@@ -19,7 +19,6 @@
 #include "storage/browser/blob/blob_impl.h"
 #include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/blob/blob_url_registry.h"
-#include "storage/browser/test/mock_blob_registry_delegate.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using blink::mojom::BlobURLStore;
@@ -72,9 +71,9 @@
 
   mojo::PendingRemote<BlobURLStore> CreateURLStore() {
     mojo::PendingRemote<BlobURLStore> result;
-    mojo::MakeSelfOwnedReceiver(std::make_unique<BlobURLStoreImpl>(
-                                    url_registry_.AsWeakPtr(), &delegate_),
-                                result.InitWithNewPipeAndPassReceiver());
+    mojo::MakeSelfOwnedReceiver(
+        std::make_unique<BlobURLStoreImpl>(kOrigin, url_registry_.AsWeakPtr()),
+        result.InitWithNewPipeAndPassReceiver());
     return result;
   }
 
@@ -110,15 +109,19 @@
   }
 
   const std::string kId = "id";
-  const GURL kValidUrl = GURL("blob:id");
+  const url::Origin kOrigin = url::Origin::Create(GURL("https://example.com"));
+  const GURL kValidUrl = GURL("blob:" + kOrigin.Serialize() + "/id1");
+  const GURL kValidUrl2 = GURL("blob:" + kOrigin.Serialize() + "/id2");
   const GURL kInvalidUrl = GURL("bolb:id");
-  const GURL kFragmentUrl = GURL("blob:id#fragment");
+  const GURL kFragmentUrl = GURL(kValidUrl.spec() + "#fragment");
+  const url::Origin kWrongOrigin =
+      url::Origin::Create(GURL("https://test.com"));
+  const GURL kWrongOriginUrl = GURL("blob:" + kWrongOrigin.Serialize() + "/id");
 
  protected:
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<BlobStorageContext> context_;
   BlobUrlRegistry url_registry_;
-  MockBlobRegistryDelegate delegate_;
   std::vector<std::string> bad_messages_;
   base::UnguessableToken agent_cluster_id_;
 };
@@ -128,7 +131,7 @@
       CreateBlobFromString(kId, "hello world");
 
   // Register a URL and make sure the URL keeps the blob alive.
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
   RegisterURL(&url_store, std::move(blob), kValidUrl);
 
   blob = url_registry_.GetBlobFromUrl(kValidUrl);
@@ -156,15 +159,13 @@
   EXPECT_EQ(1u, bad_messages_.size());
 }
 
-TEST_F(BlobURLStoreImplTest, RegisterCantCommit) {
+TEST_F(BlobURLStoreImplTest, RegisterWrongOrigin) {
   mojo::PendingRemote<blink::mojom::Blob> blob =
       CreateBlobFromString(kId, "hello world");
 
-  delegate_.can_commit_url_result = false;
-
   mojo::Remote<BlobURLStore> url_store(CreateURLStore());
-  RegisterURL(url_store.get(), std::move(blob), kValidUrl);
-  EXPECT_FALSE(url_registry_.GetBlobFromUrl(kValidUrl));
+  RegisterURL(url_store.get(), std::move(blob), kWrongOriginUrl);
+  EXPECT_FALSE(url_registry_.GetBlobFromUrl(kWrongOriginUrl));
   EXPECT_EQ(1u, bad_messages_.size());
 }
 
@@ -179,14 +180,13 @@
 }
 
 TEST_F(BlobURLStoreImplTest, ImplicitRevoke) {
-  const GURL kValidUrl2("blob:id2");
   mojo::Remote<blink::mojom::Blob> blob(
       CreateBlobFromString(kId, "hello world"));
   mojo::PendingRemote<blink::mojom::Blob> blob2;
   blob->Clone(blob2.InitWithNewPipeAndPassReceiver());
 
   auto url_store =
-      std::make_unique<BlobURLStoreImpl>(url_registry_.AsWeakPtr(), &delegate_);
+      std::make_unique<BlobURLStoreImpl>(kOrigin, url_registry_.AsWeakPtr());
   RegisterURL(url_store.get(), blob.Unbind(), kValidUrl);
   EXPECT_TRUE(url_registry_.GetBlobFromUrl(kValidUrl));
   RegisterURL(url_store.get(), std::move(blob2), kValidUrl2);
@@ -202,8 +202,8 @@
   mojo::PendingRemote<blink::mojom::Blob> blob =
       CreateBlobFromString(kId, "hello world");
 
-  BlobURLStoreImpl url_store1(url_registry_.AsWeakPtr(), &delegate_);
-  BlobURLStoreImpl url_store2(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store1(kOrigin, url_registry_.AsWeakPtr());
+  BlobURLStoreImpl url_store2(kOrigin, url_registry_.AsWeakPtr());
 
   RegisterURL(&url_store1, std::move(blob), kValidUrl);
   EXPECT_TRUE(url_registry_.GetBlobFromUrl(kValidUrl));
@@ -219,11 +219,9 @@
   EXPECT_EQ(1u, bad_messages_.size());
 }
 
-TEST_F(BlobURLStoreImplTest, RevokeCantCommit) {
-  delegate_.can_commit_url_result = false;
-
+TEST_F(BlobURLStoreImplTest, RevokeWrongOrigin) {
   mojo::Remote<BlobURLStore> url_store(CreateURLStore());
-  url_store->Revoke(kValidUrl);
+  url_store->Revoke(kWrongOriginUrl);
   url_store.FlushForTesting();
   EXPECT_EQ(1u, bad_messages_.size());
 }
@@ -239,7 +237,7 @@
   mojo::PendingRemote<blink::mojom::Blob> blob =
       CreateBlobFromString(kId, "hello world");
 
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
   RegisterURL(&url_store, std::move(blob), kValidUrl);
 
   blob = ResolveURL(&url_store, kValidUrl);
@@ -254,7 +252,7 @@
 }
 
 TEST_F(BlobURLStoreImplTest, ResolveNonExistentURL) {
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
 
   mojo::PendingRemote<blink::mojom::Blob> blob =
       ResolveURL(&url_store, kValidUrl);
@@ -264,7 +262,7 @@
 }
 
 TEST_F(BlobURLStoreImplTest, ResolveInvalidURL) {
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
 
   mojo::PendingRemote<blink::mojom::Blob> blob =
       ResolveURL(&url_store, kInvalidUrl);
@@ -275,7 +273,7 @@
   mojo::PendingRemote<blink::mojom::Blob> blob =
       CreateBlobFromString(kId, "hello world");
 
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
   RegisterURL(&url_store, std::move(blob), kValidUrl);
 
   mojo::Remote<network::mojom::URLLoaderFactory> factory;
@@ -312,7 +310,7 @@
   mojo::PendingRemote<blink::mojom::Blob> blob =
       CreateBlobFromString(kId, "hello world");
 
-  BlobURLStoreImpl url_store(url_registry_.AsWeakPtr(), &delegate_);
+  BlobURLStoreImpl url_store(kOrigin, url_registry_.AsWeakPtr());
   RegisterURL(&url_store, std::move(blob), kValidUrl);
 
   base::RunLoop loop0;