From 820b0d2ed3d4873da6e10dd470252f2aea92276b Mon Sep 17 00:00:00 2001
From: Mikel Astiz <mastiz@chromium.org>
Date: Mon, 21 Sep 2026 02:17:29 -0700
Subject: [PATCH] [sync] Fix unsafe downcasting in LoopbackServer

Previously, various codepaths including
`PersistentBookmarkEntity::CreateUpdatedVersion()` assumed
that any non-deleted entity being handled was an instance of
`PersistentBookmarkEntity`, and performed an unchecked
`static_cast`.

However, in advanced scenarios like corrupt data on disk, this condition
may not hold true.

To fix this, this patch replaces all static_cast usages with
newly-introduced virtual functions in LoopbackServerEntity that allow
safe downcasting (return null otherwise).

Change-Id: I2b37122213124728bff619285fe9041ce46e34bd
Fixed: 513394775
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8425824
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/main@{#1701610}
---
diff --git a/components/sync/engine/loopback_server/loopback_server.cc b/components/sync/engine/loopback_server/loopback_server.cc
index a15dd88aa..a70931e 100644
--- a/components/sync/engine/loopback_server/loopback_server.cc
+++ b/components/sync/engine/loopback_server/loopback_server.cc
@@ -912,13 +912,14 @@
     const sync_pb::EntitySpecifics& updated_specifics) {
   EntityMap::const_iterator iter = entities_.find(id);
   if (iter == entities_.end() ||
-      iter->second->GetDataType() != syncer::BOOKMARKS ||
       GetDataTypeFromSpecifics(updated_specifics) != syncer::BOOKMARKS) {
     return false;
   }
 
-  PersistentBookmarkEntity* entity =
-      static_cast<PersistentBookmarkEntity*>(iter->second.get());
+  PersistentBookmarkEntity* entity = iter->second->AsBookmarkEntity();
+  if (!entity) {
+    return false;
+  }
 
   entity->SetParentId(parent_id);
   entity->SetSpecifics(updated_specifics);
@@ -962,9 +963,10 @@
   for (int i = 0; i < proto.entities_size(); ++i) {
     std::unique_ptr<LoopbackServerEntity> entity =
         LoopbackServerEntity::CreateEntityFromProto(proto.entities(i));
-    // Silently drop entities that cannot be successfully deserialized.
-    if (entity) {
-      entities_[proto.entities(i).entity().id_string()] = std::move(entity);
+    // Silently drop entities that cannot be successfully deserialized or have
+    // a mismatched ID.
+    if (entity && entity->GetId() == proto.entities(i).entity().id_string()) {
+      entities_[entity->GetId()] = std::move(entity);
     }
   }
 
diff --git a/components/sync/engine/loopback_server/loopback_server_entity.cc b/components/sync/engine/loopback_server/loopback_server_entity.cc
index e5416ad..219836a7 100644
--- a/components/sync/engine/loopback_server/loopback_server_entity.cc
+++ b/components/sync/engine/loopback_server/loopback_server_entity.cc
@@ -116,6 +116,14 @@
   return false;
 }
 
+PersistentBookmarkEntity* LoopbackServerEntity::AsBookmarkEntity() {
+  return nullptr;
+}
+
+const PersistentBookmarkEntity* LoopbackServerEntity::AsBookmarkEntity() const {
+  return nullptr;
+}
+
 sync_pb::LoopbackServerEntity_Type
 LoopbackServerEntity::GetLoopbackServerEntityType() const {
   NOTREACHED();
diff --git a/components/sync/engine/loopback_server/loopback_server_entity.h b/components/sync/engine/loopback_server/loopback_server_entity.h
index c6327bb..c4d0efc 100644
--- a/components/sync/engine/loopback_server/loopback_server_entity.h
+++ b/components/sync/engine/loopback_server/loopback_server_entity.h
@@ -22,6 +22,8 @@
 
 namespace syncer {
 
+class PersistentBookmarkEntity;
+
 // The representation of a Sync entity for the loopback server.
 class LoopbackServerEntity {
  public:
@@ -67,6 +69,8 @@
   virtual bool IsDeleted() const;
   virtual bool IsFolder() const;
   virtual bool IsPermanent() const;
+  virtual PersistentBookmarkEntity* AsBookmarkEntity();
+  virtual const PersistentBookmarkEntity* AsBookmarkEntity() const;
 
   virtual void SerializeAsLoopbackServerEntity(
       sync_pb::LoopbackServerEntity* entity) const;
diff --git a/components/sync/engine/loopback_server/persistent_bookmark_entity.cc b/components/sync/engine/loopback_server/persistent_bookmark_entity.cc
index 22507a8..0e16f3b 100644
--- a/components/sync/engine/loopback_server/persistent_bookmark_entity.cc
+++ b/components/sync/engine/loopback_server/persistent_bookmark_entity.cc
@@ -31,7 +31,7 @@
 PersistentBookmarkEntity::~PersistentBookmarkEntity() = default;
 
 // static
-std::unique_ptr<LoopbackServerEntity> PersistentBookmarkEntity::CreateNew(
+std::unique_ptr<PersistentBookmarkEntity> PersistentBookmarkEntity::CreateNew(
     const sync_pb::SyncEntity& client_entity,
     const string& parent_id,
     const string& originator_cache_guid,
@@ -60,7 +60,7 @@
 }
 
 // static
-std::unique_ptr<LoopbackServerEntity>
+std::unique_ptr<PersistentBookmarkEntity>
 PersistentBookmarkEntity::CreateUpdatedVersion(
     const sync_pb::SyncEntity& client_entity,
     const LoopbackServerEntity& current_server_entity,
@@ -80,24 +80,24 @@
   std::string originator_client_item_id;
   std::string client_tag_hash;
 
-  if (current_server_entity.IsDeleted()) {
-    // Handle undeletions.
+  if (const PersistentBookmarkEntity* current_bookmark_entity =
+          current_server_entity.AsBookmarkEntity()) {
+    // Regular case (non-undeletion).
+    originator_cache_guid = current_bookmark_entity->originator_cache_guid_;
+    originator_client_item_id =
+        current_bookmark_entity->originator_client_item_id_;
+    // Note that the client tag provided by the client in `client_entity` is
+    // ignored during non-creations updates, since it's meant to be immutable.
+    client_tag_hash = current_bookmark_entity->client_tag_hash_;
+  } else {
+    // Handle undeletions (or unexpected non-bookmark server entities) by
+    // replacing the entity using the committing client's metadata.
     originator_cache_guid = updating_client_cache_guid;
     originator_client_item_id =
         LoopbackServerEntity::GetInnerIdFromId(client_entity.id_string());
     // An undeletion is similar to a creation, so let's honor the client tag
     // provided by the client (if any).
     client_tag_hash = client_entity.client_tag_hash();
-  } else {
-    // Regular case (non-undeletion).
-    const PersistentBookmarkEntity& current_bookmark_entity =
-        static_cast<const PersistentBookmarkEntity&>(current_server_entity);
-    originator_cache_guid = current_bookmark_entity.originator_cache_guid_;
-    originator_client_item_id =
-        current_bookmark_entity.originator_client_item_id_;
-    // Note that the client tag provided by the client in `client_entity` is
-    // ignored during non-creations updates, since it's meant to be immutable.
-    client_tag_hash = current_bookmark_entity.client_tag_hash_;
   }
 
   // Using a version of 0 is okay here as it'll be updated before this entity is
@@ -111,7 +111,7 @@
 }
 
 // static
-std::unique_ptr<LoopbackServerEntity>
+std::unique_ptr<PersistentBookmarkEntity>
 PersistentBookmarkEntity::CreateFromEntity(
     const sync_pb::SyncEntity& client_entity) {
   if (!IsBookmark(client_entity)) {
@@ -214,4 +214,13 @@
   return is_folder_;
 }
 
+PersistentBookmarkEntity* PersistentBookmarkEntity::AsBookmarkEntity() {
+  return this;
+}
+
+const PersistentBookmarkEntity* PersistentBookmarkEntity::AsBookmarkEntity()
+    const {
+  return this;
+}
+
 }  // namespace syncer
diff --git a/components/sync/engine/loopback_server/persistent_bookmark_entity.h b/components/sync/engine/loopback_server/persistent_bookmark_entity.h
index 8a24dac..11b429d 100644
--- a/components/sync/engine/loopback_server/persistent_bookmark_entity.h
+++ b/components/sync/engine/loopback_server/persistent_bookmark_entity.h
@@ -30,7 +30,7 @@
 
   // Factory function for PersistentBookmarkEntity. This factory should be used
   // only for the first time that a specific bookmark is seen by the server.
-  static std::unique_ptr<LoopbackServerEntity> CreateNew(
+  static std::unique_ptr<PersistentBookmarkEntity> CreateNew(
       const sync_pb::SyncEntity& client_entity,
       const std::string& parent_id,
       const std::string& originator_cache_guid,
@@ -40,15 +40,15 @@
   // for this ID, `current_server_entity`, is passed here because the client
   // does not always send the complete entity over the wire. This requires
   // copying of some of the existing entity when creating a new entity.
-  static std::unique_ptr<LoopbackServerEntity> CreateUpdatedVersion(
+  static std::unique_ptr<PersistentBookmarkEntity> CreateUpdatedVersion(
       const sync_pb::SyncEntity& client_entity,
       const LoopbackServerEntity& current_server_entity,
       const std::string& parent_id,
-      const std::string& originator_cache_guid);
+      const std::string& updating_client_cache_guid);
 
   // Factory function for PersistentBookmarkEntity used when de-serializing the
   // information stored in the persistent storage.
-  static std::unique_ptr<LoopbackServerEntity> CreateFromEntity(
+  static std::unique_ptr<PersistentBookmarkEntity> CreateFromEntity(
       const sync_pb::SyncEntity& client_entity);
 
   PersistentBookmarkEntity(const std::string& id,
@@ -72,6 +72,8 @@
   std::string GetParentId() const override;
   void SerializeAsProto(sync_pb::SyncEntity* proto) const override;
   bool IsFolder() const override;
+  PersistentBookmarkEntity* AsBookmarkEntity() override;
+  const PersistentBookmarkEntity* AsBookmarkEntity() const override;
   sync_pb::LoopbackServerEntity_Type GetLoopbackServerEntityType()
       const override;
 
diff --git a/components/sync/engine/loopback_server/loopback_server_unittest.cc b/components/sync/engine/loopback_server/loopback_server_unittest.cc
index 2ef338b..c6ca628 100644
--- a/components/sync/engine/loopback_server/loopback_server_unittest.cc
+++ b/components/sync/engine/loopback_server/loopback_server_unittest.cc
@@ -362,4 +362,22 @@
   EXPECT_FALSE(bookmarks[id].has_client_tag_hash());
 }
 
+TEST_F(LoopbackServerTest,
+       ShouldReplaceNonBookmarkServerEntityOnBookmarkCommit) {
+  // Ensure the permanent bookmark folders (e.g. `32904_bookmark_bar`, which is
+  // a PersistentPermanentEntity) are created on the server.
+  GetUpdatesForType(EntitySpecifics::kBookmarkFieldNumber);
+
+  // Committing a bookmark update when the existing server entity is not a
+  // PersistentBookmarkEntity should safely replace the previous entity without
+  // downcasting it to PersistentBookmarkEntity.
+  SyncEntity entity;
+  entity.set_id_string("32904_bookmark_bar");
+  entity.set_parent_id_string("32904_google_chrome_bookmarks");
+  entity.set_version(1);
+  entity.mutable_specifics()->mutable_bookmark()->set_url(kUrl1);
+
+  EXPECT_EQ("32904_bookmark_bar", CommitVerifySuccess(entity));
+}
+
 }  // namespace syncer
diff --git a/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc b/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
index 232b4d3..0ab6b61 100644
--- a/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
+++ b/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
@@ -4,7 +4,10 @@
 
 #include "components/sync/engine/loopback_server/persistent_bookmark_entity.h"
 
+#include <memory>
+
 #include "base/uuid.h"
+#include "components/sync/engine/loopback_server/persistent_unique_client_entity.h"
 #include "components/sync/protocol/sync_entity.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -30,12 +33,15 @@
 
 TEST(PersistentBookmarkEntityTest, CreateUpdatedVersion) {
   sync_pb::SyncEntity client_entity;
+  client_entity.set_id_string(
+      "32904_" + base::Uuid::GenerateRandomV4().AsLowercaseString());
   client_entity.mutable_specifics()->mutable_bookmark();
-  auto server_entity =
+  std::unique_ptr<PersistentBookmarkEntity> server_entity =
       PersistentBookmarkEntity::CreateFromEntity(client_entity);
   ASSERT_TRUE(server_entity);
+  EXPECT_EQ(server_entity->AsBookmarkEntity(), server_entity.get());
 
-  // Fails with since there's no version
+  // Fails since there's no version.
   ASSERT_FALSE(PersistentBookmarkEntity::CreateUpdatedVersion(
       client_entity, *server_entity, "parent_id", "updating_guid"));
 
@@ -44,7 +50,27 @@
   ASSERT_TRUE(PersistentBookmarkEntity::CreateUpdatedVersion(
       client_entity, *server_entity, "parent_id", "updating_guid"));
 
-  // But fails when not actually a bookmark.
+  // Succeeds when the existing server entity is not a bookmark entity (e.g. a
+  // tombstone or unexpected non-bookmark entity), replacing it using the
+  // committing client's metadata without downcasting.
+  sync_pb::EntitySpecifics unique_specifics;
+  unique_specifics.mutable_preference();
+  std::unique_ptr<LoopbackServerEntity> non_bookmark_server_entity =
+      PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
+          "non_unique_name", "client_tag", unique_specifics,
+          /*creation_time=*/0, /*last_modified_time=*/0);
+  ASSERT_TRUE(non_bookmark_server_entity);
+  EXPECT_EQ(non_bookmark_server_entity->AsBookmarkEntity(), nullptr);
+  std::unique_ptr<PersistentBookmarkEntity> replaced_entity =
+      PersistentBookmarkEntity::CreateUpdatedVersion(
+          client_entity, *non_bookmark_server_entity, "parent_id",
+          "updating_guid");
+  ASSERT_TRUE(replaced_entity);
+  sync_pb::SyncEntity replaced_proto;
+  replaced_entity->SerializeAsProto(&replaced_proto);
+  EXPECT_EQ(replaced_proto.originator_cache_guid(), "updating_guid");
+
+  // Fails when client_entity is not actually a bookmark.
   client_entity.clear_specifics();
   client_entity.mutable_specifics()->mutable_preference();
   ASSERT_FALSE(PersistentBookmarkEntity::CreateUpdatedVersion(

diff --git a/components/sync/engine/loopback_server/loopback_server_unittest.cc b/components/sync/engine/loopback_server/loopback_server_unittest.cc
index 2ef338b..c6ca628 100644
--- a/components/sync/engine/loopback_server/loopback_server_unittest.cc
+++ b/components/sync/engine/loopback_server/loopback_server_unittest.cc
@@ -362,4 +362,22 @@
   EXPECT_FALSE(bookmarks[id].has_client_tag_hash());
 }
 
+TEST_F(LoopbackServerTest,
+       ShouldReplaceNonBookmarkServerEntityOnBookmarkCommit) {
+  // Ensure the permanent bookmark folders (e.g. `32904_bookmark_bar`, which is
+  // a PersistentPermanentEntity) are created on the server.
+  GetUpdatesForType(EntitySpecifics::kBookmarkFieldNumber);
+
+  // Committing a bookmark update when the existing server entity is not a
+  // PersistentBookmarkEntity should safely replace the previous entity without
+  // downcasting it to PersistentBookmarkEntity.
+  SyncEntity entity;
+  entity.set_id_string("32904_bookmark_bar");
+  entity.set_parent_id_string("32904_google_chrome_bookmarks");
+  entity.set_version(1);
+  entity.mutable_specifics()->mutable_bookmark()->set_url(kUrl1);
+
+  EXPECT_EQ("32904_bookmark_bar", CommitVerifySuccess(entity));
+}
+
 }  // namespace syncer
diff --git a/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc b/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
index 232b4d3..0ab6b61 100644
--- a/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
+++ b/components/sync/engine/loopback_server/persistent_bookmark_entity_unittest.cc
@@ -4,7 +4,10 @@
 
 #include "components/sync/engine/loopback_server/persistent_bookmark_entity.h"
 
+#include <memory>
+
 #include "base/uuid.h"
+#include "components/sync/engine/loopback_server/persistent_unique_client_entity.h"
 #include "components/sync/protocol/sync_entity.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -30,12 +33,15 @@
 
 TEST(PersistentBookmarkEntityTest, CreateUpdatedVersion) {
   sync_pb::SyncEntity client_entity;
+  client_entity.set_id_string(
+      "32904_" + base::Uuid::GenerateRandomV4().AsLowercaseString());
   client_entity.mutable_specifics()->mutable_bookmark();
-  auto server_entity =
+  std::unique_ptr<PersistentBookmarkEntity> server_entity =
       PersistentBookmarkEntity::CreateFromEntity(client_entity);
   ASSERT_TRUE(server_entity);
+  EXPECT_EQ(server_entity->AsBookmarkEntity(), server_entity.get());
 
-  // Fails with since there's no version
+  // Fails since there's no version.
   ASSERT_FALSE(PersistentBookmarkEntity::CreateUpdatedVersion(
       client_entity, *server_entity, "parent_id", "updating_guid"));
 
@@ -44,7 +50,27 @@
   ASSERT_TRUE(PersistentBookmarkEntity::CreateUpdatedVersion(
       client_entity, *server_entity, "parent_id", "updating_guid"));
 
-  // But fails when not actually a bookmark.
+  // Succeeds when the existing server entity is not a bookmark entity (e.g. a
+  // tombstone or unexpected non-bookmark entity), replacing it using the
+  // committing client's metadata without downcasting.
+  sync_pb::EntitySpecifics unique_specifics;
+  unique_specifics.mutable_preference();
+  std::unique_ptr<LoopbackServerEntity> non_bookmark_server_entity =
+      PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
+          "non_unique_name", "client_tag", unique_specifics,
+          /*creation_time=*/0, /*last_modified_time=*/0);
+  ASSERT_TRUE(non_bookmark_server_entity);
+  EXPECT_EQ(non_bookmark_server_entity->AsBookmarkEntity(), nullptr);
+  std::unique_ptr<PersistentBookmarkEntity> replaced_entity =
+      PersistentBookmarkEntity::CreateUpdatedVersion(
+          client_entity, *non_bookmark_server_entity, "parent_id",
+          "updating_guid");
+  ASSERT_TRUE(replaced_entity);
+  sync_pb::SyncEntity replaced_proto;
+  replaced_entity->SerializeAsProto(&replaced_proto);
+  EXPECT_EQ(replaced_proto.originator_cache_guid(), "updating_guid");
+
+  // Fails when client_entity is not actually a bookmark.
   client_entity.clear_specifics();
   client_entity.mutable_specifics()->mutable_preference();
   ASSERT_FALSE(PersistentBookmarkEntity::CreateUpdatedVersion(