From 460810f61f17c448f9866e72bc00b847b58dd015 Mon Sep 17 00:00:00 2001
From: Rob Pitkin <robpitkin@google.com>
Date: Thu, 06 Aug 2026 10:49:18 -0700
Subject: [PATCH] contacts: Prevent out-of-context launches from BFCache and subframes.

ContactsManagerImpl (a DocumentService) is owned by the document of a
RenderFrameHost. If the document enters the Back-Forward Cache
(BFCache), it remains alive but inactive. The browser-side
ContactsManagerImpl::Select method did not check if the requesting frame
was active and in the primary main frame, which allowed an inactive
frame to trigger the fullscreen Contacts Picker dialog over an unrelated
visible page.

This CL adds checks to verify that the requesting RenderFrameHost is
active and in the primary main frame. If not, the request is safely
rejected by returning std::nullopt via the callback to avoid crashing
the renderer in case of race conditions (e.g. navigation immediately
after API call).

To support this validation, this CL introduces:
- A SetContactsProviderForTesting API to ContactsManagerImpl.
- A unit test suite (contacts_manager_impl_unittest.cc) using a MockContactsProvider
  to verify that:
  1. Active primary main frames can launch the picker.
  2. BFCached frames safely reject the request and do not invoke the provider.
  3. Subframes safely reject the request and do not invoke the provider.

Bug: 533084499
Test: content_unittests --gtest_filter=ContactsManagerImplTest.*
Change-Id: I79521ea0fc4af692d95083a2fdeb9385506e9765
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8178783
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Rob Pitkin <robpitkin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1675096}
---
diff --git a/content/browser/contacts/contacts_manager_impl.cc b/content/browser/contacts/contacts_manager_impl.cc
index 26918f13..4238bde 100644
--- a/content/browser/contacts/contacts_manager_impl.cc
+++ b/content/browser/contacts/contacts_manager_impl.cc
@@ -54,11 +54,31 @@
 
 }  // namespace
 
+// static
+void ContactsManagerImpl::Create(
+    RenderFrameHost* render_frame_host,
+    mojo::PendingReceiver<blink::mojom::ContactsManager> receiver) {
+  CHECK(render_frame_host);
+  new ContactsManagerImpl(*render_frame_host, std::move(receiver),
+                          CreateProvider(*render_frame_host));
+}
+
+// static
+ContactsManagerImpl* ContactsManagerImpl::CreateForTesting(
+    RenderFrameHost* render_frame_host,
+    mojo::PendingReceiver<blink::mojom::ContactsManager> receiver,
+    std::unique_ptr<ContactsProvider> provider) {
+  CHECK(render_frame_host);
+  return new ContactsManagerImpl(*render_frame_host, std::move(receiver),
+                                 std::move(provider));
+}
+
 ContactsManagerImpl::ContactsManagerImpl(
     RenderFrameHost& render_frame_host,
-    mojo::PendingReceiver<blink::mojom::ContactsManager> receiver)
+    mojo::PendingReceiver<blink::mojom::ContactsManager> receiver,
+    std::unique_ptr<ContactsProvider> provider)
     : DocumentService(render_frame_host, std::move(receiver)),
-      contacts_provider_(CreateProvider(render_frame_host)) {
+      contacts_provider_(std::move(provider)) {
   CHECK(!render_frame_host.IsInLifecycleState(
       RenderFrameHost::LifecycleState::kPrerendering));
   source_id_ = render_frame_host.GetPageUkmSourceId();
@@ -73,6 +93,12 @@
                                  bool include_addresses,
                                  bool include_icons,
                                  SelectCallback mojom_callback) {
+  if (!render_frame_host().IsActive() ||
+      !render_frame_host().IsInPrimaryMainFrame()) {
+    std::move(mojom_callback).Run(std::nullopt);
+    return;
+  }
+
   if (contacts_provider_) {
     contacts_provider_->Select(
         multiple, include_names, include_emails, include_tel, include_addresses,
diff --git a/content/browser/contacts/contacts_manager_impl.h b/content/browser/contacts/contacts_manager_impl.h
index 5ada2c1c..d5ad7eb6 100644
--- a/content/browser/contacts/contacts_manager_impl.h
+++ b/content/browser/contacts/contacts_manager_impl.h
@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_CONTACTS_CONTACTS_MANAGER_IMPL_H_
 
 #include "content/browser/contacts/contacts_provider.h"
+#include "content/common/content_export.h"
 #include "content/public/browser/document_service.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "third_party/blink/public/mojom/contacts/contacts_manager.mojom.h"
@@ -14,17 +15,17 @@
 
 class RenderFrameHost;
 
-class ContactsManagerImpl
+class CONTENT_EXPORT ContactsManagerImpl
     : public DocumentService<blink::mojom::ContactsManager> {
  public:
   static void Create(
       RenderFrameHost* render_frame_host,
-      mojo::PendingReceiver<blink::mojom::ContactsManager> receiver) {
-    CHECK(render_frame_host);
-    // The object is bound to the lifetime of `render_frame_host`'s logical
-    // document by virtue of being a `DocumentService` implementation.
-    new ContactsManagerImpl(*render_frame_host, std::move(receiver));
-  }
+      mojo::PendingReceiver<blink::mojom::ContactsManager> receiver);
+
+  static ContactsManagerImpl* CreateForTesting(
+      RenderFrameHost* render_frame_host,
+      mojo::PendingReceiver<blink::mojom::ContactsManager> receiver,
+      std::unique_ptr<ContactsProvider> provider);
 
   ContactsManagerImpl(const ContactsManagerImpl&) = delete;
   ContactsManagerImpl& operator=(const ContactsManagerImpl&) = delete;
@@ -42,7 +43,8 @@
  private:
   explicit ContactsManagerImpl(
       RenderFrameHost& render_frame_host,
-      mojo::PendingReceiver<blink::mojom::ContactsManager> receiver);
+      mojo::PendingReceiver<blink::mojom::ContactsManager> receiver,
+      std::unique_ptr<ContactsProvider> provider);
 
   std::unique_ptr<ContactsProvider> contacts_provider_;
 
diff --git a/content/browser/contacts/contacts_manager_impl_unittest.cc b/content/browser/contacts/contacts_manager_impl_unittest.cc
new file mode 100644
index 0000000..80df5ac5
--- /dev/null
+++ b/content/browser/contacts/contacts_manager_impl_unittest.cc
@@ -0,0 +1,166 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/contacts/contacts_manager_impl.h"
+
+#include "base/memory/raw_ptr.h"
+#include "base/test/gmock_callback_support.h"
+#include "base/test/mock_callback.h"
+#include "base/test/test_future.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "content/test/test_render_frame_host.h"
+#include "content/test/test_web_contents.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/mojom/contacts/contacts_manager.mojom.h"
+
+namespace content {
+
+namespace {
+
+class MockContactsProvider : public ContactsProvider {
+ public:
+  MockContactsProvider() = default;
+  ~MockContactsProvider() override = default;
+
+  MOCK_METHOD(void,
+              Select,
+              (bool multiple,
+               bool include_names,
+               bool include_emails,
+               bool include_tel,
+               bool include_addresses,
+               bool include_icons,
+               ContactsProvider::ContactsSelectedCallback callback),
+              (override));
+};
+
+}  // namespace
+
+class ContactsManagerImplTest : public RenderViewHostImplTestHarness {
+ public:
+  ContactsManagerImplTest() = default;
+  ~ContactsManagerImplTest() override = default;
+
+  void SetUp() override {
+    RenderViewHostImplTestHarness::SetUp();
+    RenderFrameHostTester::For(main_rfh())->InitializeRenderFrameIfNeeded();
+    NavigateAndCommit(GURL("https://example.com"));
+  }
+
+  void TearDown() override {
+    contacts_manager_impl_ = nullptr;
+    RenderViewHostImplTestHarness::TearDown();
+  }
+
+  void InitService(std::unique_ptr<ContactsProvider> provider) {
+    contacts_manager_impl_ = ContactsManagerImpl::CreateForTesting(
+        main_rfh(), contacts_manager_remote_.BindNewPipeAndPassReceiver(),
+        std::move(provider));
+  }
+
+  mojo::Remote<blink::mojom::ContactsManager>& remote() {
+    return contacts_manager_remote_;
+  }
+
+ private:
+  mojo::Remote<blink::mojom::ContactsManager> contacts_manager_remote_;
+  raw_ptr<ContactsManagerImpl> contacts_manager_impl_ = nullptr;
+};
+
+TEST_F(ContactsManagerImplTest, SelectActivePrimaryMainFrame) {
+  auto mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* mock_provider_ptr = mock_provider.get();
+
+  // Set up expectation on mock provider.
+  std::vector<blink::mojom::ContactInfoPtr> expected_contacts;
+  auto contact = blink::mojom::ContactInfo::New();
+  contact->name = std::vector<std::string>{"John Doe"};
+  expected_contacts.push_back(std::move(contact));
+
+  EXPECT_CALL(*mock_provider_ptr, Select)
+      .WillOnce(base::test::RunOnceCallback<6>(std::move(expected_contacts),
+                                               /*percentage_shared=*/100,
+                                               ContactsPickerProperties()));
+
+  InitService(std::move(mock_provider));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  remote()->Select(/*multiple=*/false, /*include_names=*/true,
+                   /*include_emails=*/false, /*include_tel=*/false,
+                   /*include_addresses=*/false, /*include_icons=*/false,
+                   future.GetCallback());
+
+  ASSERT_TRUE(future.Get().has_value());
+  EXPECT_EQ(future.Get()->size(), 1u);
+  EXPECT_EQ(future.Get()->at(0)->name->at(0), "John Doe");
+}
+
+TEST_F(ContactsManagerImplTest, SelectBFCachedFrame) {
+  auto mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* mock_provider_ptr = mock_provider.get();
+
+  EXPECT_CALL(*mock_provider_ptr, Select).Times(0);
+
+  InitService(std::move(mock_provider));
+
+  // Put the frame into BFCache.
+  static_cast<TestRenderFrameHost*>(main_rfh())->DidEnterBackForwardCache();
+  EXPECT_TRUE(main_rfh()->IsInLifecycleState(
+      RenderFrameHost::LifecycleState::kInBackForwardCache));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  // Call Select. It should return nullopt safely.
+  remote()->Select(/*multiple=*/false, /*include_names=*/true,
+                   /*include_emails=*/false, /*include_tel=*/false,
+                   /*include_addresses=*/false, /*include_icons=*/false,
+                   future.GetCallback());
+
+  EXPECT_EQ(future.Get(), std::nullopt);
+}
+
+TEST_F(ContactsManagerImplTest, SelectSubframe) {
+  // Create a subframe.
+  RenderFrameHost* subframe =
+      RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  ASSERT_TRUE(subframe);
+  RenderFrameHostTester::For(subframe)->InitializeRenderFrameIfNeeded();
+
+  // Navigate the subframe.
+  auto navigation = NavigationSimulator::CreateRendererInitiated(
+      GURL("https://example.com/subframe"), subframe);
+  navigation->Commit();
+  subframe = navigation->GetFinalRenderFrameHost();
+
+  mojo::Remote<blink::mojom::ContactsManager> subframe_remote;
+  auto subframe_mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* subframe_mock_provider_ptr =
+      subframe_mock_provider.get();
+
+  EXPECT_CALL(*subframe_mock_provider_ptr, Select).Times(0);
+
+  ContactsManagerImpl::CreateForTesting(
+      subframe, subframe_remote.BindNewPipeAndPassReceiver(),
+      std::move(subframe_mock_provider));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  // Call Select on subframe. It should return nullopt safely because it is not
+  // a main frame.
+  subframe_remote->Select(/*multiple=*/false, /*include_names=*/true,
+                          /*include_emails=*/false, /*include_tel=*/false,
+                          /*include_addresses=*/false, /*include_icons=*/false,
+                          future.GetCallback());
+
+  EXPECT_EQ(future.Get(), std::nullopt);
+}
+
+}  // namespace content
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn
index 35124eb..5e75e2c93 100644
--- a/content/test/BUILD.gn
+++ b/content/test/BUILD.gn
@@ -2606,6 +2606,7 @@
     "../browser/code_cache/dedicated_task_runner_for_resource_unittest.cc",
     "../browser/code_cache/generated_code_cache_unittest.cc",
     "../browser/code_cache/simple_lru_cache_unittest.cc",
+    "../browser/contacts/contacts_manager_impl_unittest.cc",
     "../browser/content_index/content_index_database_unittest.cc",
     "../browser/content_index/content_index_service_impl_unittest.cc",
     "../browser/cookie_store/cookie_store_manager_unittest.cc",

diff --git a/content/browser/contacts/contacts_manager_impl_unittest.cc b/content/browser/contacts/contacts_manager_impl_unittest.cc
new file mode 100644
index 0000000..80df5ac5
--- /dev/null
+++ b/content/browser/contacts/contacts_manager_impl_unittest.cc
@@ -0,0 +1,166 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/contacts/contacts_manager_impl.h"
+
+#include "base/memory/raw_ptr.h"
+#include "base/test/gmock_callback_support.h"
+#include "base/test/mock_callback.h"
+#include "base/test/test_future.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "content/test/test_render_frame_host.h"
+#include "content/test/test_web_contents.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/mojom/contacts/contacts_manager.mojom.h"
+
+namespace content {
+
+namespace {
+
+class MockContactsProvider : public ContactsProvider {
+ public:
+  MockContactsProvider() = default;
+  ~MockContactsProvider() override = default;
+
+  MOCK_METHOD(void,
+              Select,
+              (bool multiple,
+               bool include_names,
+               bool include_emails,
+               bool include_tel,
+               bool include_addresses,
+               bool include_icons,
+               ContactsProvider::ContactsSelectedCallback callback),
+              (override));
+};
+
+}  // namespace
+
+class ContactsManagerImplTest : public RenderViewHostImplTestHarness {
+ public:
+  ContactsManagerImplTest() = default;
+  ~ContactsManagerImplTest() override = default;
+
+  void SetUp() override {
+    RenderViewHostImplTestHarness::SetUp();
+    RenderFrameHostTester::For(main_rfh())->InitializeRenderFrameIfNeeded();
+    NavigateAndCommit(GURL("https://example.com"));
+  }
+
+  void TearDown() override {
+    contacts_manager_impl_ = nullptr;
+    RenderViewHostImplTestHarness::TearDown();
+  }
+
+  void InitService(std::unique_ptr<ContactsProvider> provider) {
+    contacts_manager_impl_ = ContactsManagerImpl::CreateForTesting(
+        main_rfh(), contacts_manager_remote_.BindNewPipeAndPassReceiver(),
+        std::move(provider));
+  }
+
+  mojo::Remote<blink::mojom::ContactsManager>& remote() {
+    return contacts_manager_remote_;
+  }
+
+ private:
+  mojo::Remote<blink::mojom::ContactsManager> contacts_manager_remote_;
+  raw_ptr<ContactsManagerImpl> contacts_manager_impl_ = nullptr;
+};
+
+TEST_F(ContactsManagerImplTest, SelectActivePrimaryMainFrame) {
+  auto mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* mock_provider_ptr = mock_provider.get();
+
+  // Set up expectation on mock provider.
+  std::vector<blink::mojom::ContactInfoPtr> expected_contacts;
+  auto contact = blink::mojom::ContactInfo::New();
+  contact->name = std::vector<std::string>{"John Doe"};
+  expected_contacts.push_back(std::move(contact));
+
+  EXPECT_CALL(*mock_provider_ptr, Select)
+      .WillOnce(base::test::RunOnceCallback<6>(std::move(expected_contacts),
+                                               /*percentage_shared=*/100,
+                                               ContactsPickerProperties()));
+
+  InitService(std::move(mock_provider));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  remote()->Select(/*multiple=*/false, /*include_names=*/true,
+                   /*include_emails=*/false, /*include_tel=*/false,
+                   /*include_addresses=*/false, /*include_icons=*/false,
+                   future.GetCallback());
+
+  ASSERT_TRUE(future.Get().has_value());
+  EXPECT_EQ(future.Get()->size(), 1u);
+  EXPECT_EQ(future.Get()->at(0)->name->at(0), "John Doe");
+}
+
+TEST_F(ContactsManagerImplTest, SelectBFCachedFrame) {
+  auto mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* mock_provider_ptr = mock_provider.get();
+
+  EXPECT_CALL(*mock_provider_ptr, Select).Times(0);
+
+  InitService(std::move(mock_provider));
+
+  // Put the frame into BFCache.
+  static_cast<TestRenderFrameHost*>(main_rfh())->DidEnterBackForwardCache();
+  EXPECT_TRUE(main_rfh()->IsInLifecycleState(
+      RenderFrameHost::LifecycleState::kInBackForwardCache));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  // Call Select. It should return nullopt safely.
+  remote()->Select(/*multiple=*/false, /*include_names=*/true,
+                   /*include_emails=*/false, /*include_tel=*/false,
+                   /*include_addresses=*/false, /*include_icons=*/false,
+                   future.GetCallback());
+
+  EXPECT_EQ(future.Get(), std::nullopt);
+}
+
+TEST_F(ContactsManagerImplTest, SelectSubframe) {
+  // Create a subframe.
+  RenderFrameHost* subframe =
+      RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  ASSERT_TRUE(subframe);
+  RenderFrameHostTester::For(subframe)->InitializeRenderFrameIfNeeded();
+
+  // Navigate the subframe.
+  auto navigation = NavigationSimulator::CreateRendererInitiated(
+      GURL("https://example.com/subframe"), subframe);
+  navigation->Commit();
+  subframe = navigation->GetFinalRenderFrameHost();
+
+  mojo::Remote<blink::mojom::ContactsManager> subframe_remote;
+  auto subframe_mock_provider = std::make_unique<MockContactsProvider>();
+  MockContactsProvider* subframe_mock_provider_ptr =
+      subframe_mock_provider.get();
+
+  EXPECT_CALL(*subframe_mock_provider_ptr, Select).Times(0);
+
+  ContactsManagerImpl::CreateForTesting(
+      subframe, subframe_remote.BindNewPipeAndPassReceiver(),
+      std::move(subframe_mock_provider));
+
+  base::test::TestFuture<
+      std::optional<std::vector<blink::mojom::ContactInfoPtr>>>
+      future;
+  // Call Select on subframe. It should return nullopt safely because it is not
+  // a main frame.
+  subframe_remote->Select(/*multiple=*/false, /*include_names=*/true,
+                          /*include_emails=*/false, /*include_tel=*/false,
+                          /*include_addresses=*/false, /*include_icons=*/false,
+                          future.GetCallback());
+
+  EXPECT_EQ(future.Get(), std::nullopt);
+}
+
+}  // namespace content
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn
index 35124eb..5e75e2c93 100644
--- a/content/test/BUILD.gn
+++ b/content/test/BUILD.gn
@@ -2606,6 +2606,7 @@
     "../browser/code_cache/dedicated_task_runner_for_resource_unittest.cc",
     "../browser/code_cache/generated_code_cache_unittest.cc",
     "../browser/code_cache/simple_lru_cache_unittest.cc",
+    "../browser/contacts/contacts_manager_impl_unittest.cc",
     "../browser/content_index/content_index_database_unittest.cc",
     "../browser/content_index/content_index_service_impl_unittest.cc",
     "../browser/cookie_store/cookie_store_manager_unittest.cc",