From 8d12fea02a097cdabadbd7c84b45fc63e6b7b908 Mon Sep 17 00:00:00 2001
From: mark a. foltz <mfoltz@chromium.org>
Date: Sat, 05 Sep 2026 17:50:21 -0700
Subject: [PATCH] [actor] Enforce strict profile validation for ActorTask and tools

This change addresses cross-profile security vulnerabilities in the Actor
subsystem (Fix 1 and Fix 3):

1. ActorTask tab validation (Fix 1):
   - Updates ActorTask::CheckCrossProfileAndLog to fail closed if
     the tab does not exist or if tab->GetProfile() != GetProfile().
   - Compares profiles directly via tab->GetProfile() rather than
     tab->GetContents()->GetBrowserContext(), ensuring tabs without
     an attached WebContents are properly validated.
   - Reorders checks in ActorTask::AddTab so that existence and
     profile validation are performed before checking controlled_tabs_.
   - Adds unit and browser tests verifying that unissued handles,
     non-existent tabs, and cross-profile tabs are rejected.

2. Tool & Keyed Service Hardening (Fix 3):
   - Centralizes target tab existence and profile validation in
     ToolController::CreateToolAndValidate and ToolController::Invoke
     using Tool::GetTargetTab(). This eliminates boilerplate checks across
     individual tools and prevents tools from accidentally skipping validation.
   - Fixes WaitTool::GetTargetTab() to return observe_tab_handle_.
   - Removes redundant ValidateTab() calls from individual tab tools
     (AttemptFormFillingTool, AttemptLoginTool, NavigateTool, PageTool,
     TabManagementTool), and removes Tool::ValidateTab().
   - Retains per-tool window validation (ValidateBrowserWindow and
     ValidateWindowId) for window-targeting tools.
   - Updates WindowManagementTool::CheckCrossProfile to fail closed
     if the browser window is null or belongs to a different profile.
   - Updates LoadAndExtractContentTool to validate that the target
     window matches the task profile in both Validate() and Invoke().
   - Hardens ActorKeyedService::CreateActorTab to reject cross-profile
     initiator tabs with an error (nullptr) instead of falling back.
     Adds browser test CreateActorTabRejectsCrossProfileInitiatorTab.
   - Hardens ActorKeyedService::RequestTabObservation to defensively
     reject cross-profile tabs.

3. Error reporting:
   - Adds a new result code kActionTargetCrossProfile which is
     returned if there is a mismatch between a tool target and the
     task's profile.

Fixed: 551177608,517703787,502497790,502768228
Change-Id: I9f3e9d6840b46bf0d4052cabe192be9192f787ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8338389
Commit-Queue: Mark Foltz <mfoltz@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1692990}
---

diff --git a/chrome/browser/actor/actor_keyed_service.cc b/chrome/browser/actor/actor_keyed_service.cc
index 790564ce..5f6f248 100644
--- a/chrome/browser/actor/actor_keyed_service.cc
+++ b/chrome/browser/actor/actor_keyed_service.cc
@@ -266,6 +266,15 @@
 
   BrowserWindowInterface* window_for_new_tab = nullptr;
   tabs::TabInterface* initiator_tab = initiator_tab_handle.Get();
+  if (initiator_tab && initiator_tab->GetProfile() != profile_.get()) {
+    GetJournal().Log(
+        GURL(), task_id, "CreateActorTab",
+        JournalDetailsBuilder()
+            .AddError("Initiator tab belongs to a different profile")
+            .Build());
+    std::move(callback).Run(nullptr);
+    return;
+  }
 
   // Special case: if the initiator tab is the NTP, no need to create a new
   // tab, reuse it.
@@ -339,18 +348,17 @@
 #endif
 
   // If the initiating tab is still live, create the new tab in the same window.
-  if (initiator_tab) {
-    if (initiator_tab->IsInNormalWindow()) {
-      window_for_new_tab = initiator_tab->GetBrowserWindowInterface();
-      if (window_for_new_tab) {
-        GetJournal().Log(GURL(), task_id, "CreateActorTab",
-                         JournalDetailsBuilder()
-                             .Add("Using initiator_tab's window",
-                                  window_for_new_tab->GetSessionID().id())
-                             .Build());
-      }
+  // (Cross-profile initiator tabs were already rejected above.)
+  if (initiator_tab && initiator_tab->IsInNormalWindow()) {
+    window_for_new_tab = initiator_tab->GetBrowserWindowInterface();
+    if (window_for_new_tab) {
+      GetJournal().Log(GURL(), task_id, "CreateActorTab",
+                       JournalDetailsBuilder()
+                           .Add("Using initiator_tab's window",
+                                window_for_new_tab->GetSessionID().id())
+                           .Build());
     }
-  } else {
+  } else if (!initiator_tab) {
     // TODO(b/482430429): Figure out how to proceed from just a window ID on
     // Android.
 #if !BUILDFLAG(IS_ANDROID)
@@ -358,11 +366,17 @@
     // task initiation).
     window_for_new_tab =
         BrowserWindowInterface::FromSessionID(initiator_window_id);
-    GetJournal().Log(
-        GURL(), task_id, "CreateActorTab",
-        JournalDetailsBuilder()
-            .Add("Using initiator_window", initiator_window_id.id())
-            .Build());
+    if (window_for_new_tab &&
+        window_for_new_tab->GetProfile() != profile_.get()) {
+      window_for_new_tab = nullptr;
+    }
+    if (window_for_new_tab) {
+      GetJournal().Log(
+          GURL(), task_id, "CreateActorTab",
+          JournalDetailsBuilder()
+              .Add("Using initiator_window", initiator_window_id.id())
+              .Build());
+    }
 #endif
   }
 
@@ -594,6 +608,18 @@
         screenshot_collection_options,
     base::OnceCallback<void(TabObservationResult)> callback) {
   TRACE_EVENT0("actor", "ActorKeyedService::RequestTabObservation");
+  if (tab.GetProfile() != profile_.get()) {
+    journal_.Log(GURL(), task_id, "RequestTabObservation",
+                 JournalDetailsBuilder()
+                     .AddError("Cross-profile tab observation denied")
+                     .Build());
+    std::move(callback).Run(
+        base::unexpected(page_content_annotations::FetchPageContextErrorDetails{
+            .error_code = page_content_annotations::FetchPageContextError::
+                kPageContextNotEligible,
+            .message = "Cross-profile tab observation denied"}));
+    return;
+  }
   const GURL& last_committed_url = tab.GetContents()->GetLastCommittedURL();
   auto journal_entry = journal_.CreatePendingAsyncEntry(
       last_committed_url, task_id, MakeBrowserTrackUUID(task_id),
diff --git a/chrome/browser/actor/actor_keyed_service_browsertest.cc b/chrome/browser/actor/actor_keyed_service_browsertest.cc
index 97ef41e..84ba8bb4 100644
--- a/chrome/browser/actor/actor_keyed_service_browsertest.cc
+++ b/chrome/browser/actor/actor_keyed_service_browsertest.cc
@@ -20,11 +20,13 @@
 #include "chrome/browser/actor/tools/navigate_tool_request.h"
 #include "chrome/browser/optimization_guide/browser_test_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/browser_window/public/browser_window_interface.h"
 #include "chrome/browser/ui/browser_window/public/create_browser_window.h"
 #include "chrome/common/actor.mojom.h"
 #include "chrome/common/actor/action_result.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/platform_browser_test.h"
 #if !BUILDFLAG(IS_ANDROID)
@@ -480,7 +482,7 @@
 
   auto result = future.Take();
   ASSERT_TRUE(result);
-  EXPECT_EQ(result->code, mojom::ActionResultCode::kTaskWentAway);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kActionTargetCrossProfile);
 
   browser2->GetWindow()->Close();
 }
@@ -510,6 +512,115 @@
 
   browser2->GetWindow()->Close();
 }
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       AddTabRejectsUnissuedTabHandle) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+  ActorTask* task = actor_keyed_service()->GetTask(task_id);
+
+  tabs::TabHandle unissued_handle(99999);
+  base::test::TestFuture<mojom::ActionResultPtr> future;
+  task->AddTab(unissued_handle, /*stop_task_on_detach=*/true,
+               future.GetCallback());
+
+  auto result = future.Take();
+  ASSERT_TRUE(result);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kTabWentAway);
+  EXPECT_FALSE(task->HasTab(unissued_handle));
+  EXPECT_FALSE(task->GetTabs().contains(unissued_handle));
+}
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       CreateActorTabRejectsCrossProfileInitiatorWindow) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();

diff --git a/chrome/browser/actor/actor_keyed_service_browsertest.cc b/chrome/browser/actor/actor_keyed_service_browsertest.cc
index 97ef41e..84ba8bb4 100644
--- a/chrome/browser/actor/actor_keyed_service_browsertest.cc
+++ b/chrome/browser/actor/actor_keyed_service_browsertest.cc
@@ -20,11 +20,13 @@
 #include "chrome/browser/actor/tools/navigate_tool_request.h"
 #include "chrome/browser/optimization_guide/browser_test_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/browser_window/public/browser_window_interface.h"
 #include "chrome/browser/ui/browser_window/public/create_browser_window.h"
 #include "chrome/common/actor.mojom.h"
 #include "chrome/common/actor/action_result.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/platform_browser_test.h"
 #if !BUILDFLAG(IS_ANDROID)
@@ -480,7 +482,7 @@
 
   auto result = future.Take();
   ASSERT_TRUE(result);
-  EXPECT_EQ(result->code, mojom::ActionResultCode::kTaskWentAway);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kActionTargetCrossProfile);
 
   browser2->GetWindow()->Close();
 }
@@ -510,6 +512,115 @@
 
   browser2->GetWindow()->Close();
 }
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       AddTabRejectsUnissuedTabHandle) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+  ActorTask* task = actor_keyed_service()->GetTask(task_id);
+
+  tabs::TabHandle unissued_handle(99999);
+  base::test::TestFuture<mojom::ActionResultPtr> future;
+  task->AddTab(unissued_handle, /*stop_task_on_detach=*/true,
+               future.GetCallback());
+
+  auto result = future.Take();
+  ASSERT_TRUE(result);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kTabWentAway);
+  EXPECT_FALSE(task->HasTab(unissued_handle));
+  EXPECT_FALSE(task->GetTabs().contains(unissued_handle));
+}
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       CreateActorTabRejectsCrossProfileInitiatorWindow) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  base::FilePath profile_path =
+      profile_manager->GenerateNextProfileDirectoryPath();
+  Profile& profile2 =
+      profiles::testing::CreateProfileSync(profile_manager, profile_path);
+
+  BrowserWindowInterface* browser2 = CreateBrowserWindow(
+      BrowserWindowCreateParams(&profile2, /*from_user_gesture=*/true));
+  chrome::NewTab(browser2, NewTabTypes::kNoUserAction);
+  const int profile2_tab_count_before = browser2->GetTabStripModel()->count();
+
+  base::test::TestFuture<tabs::TabInterface*> future;
+  actor_keyed_service()->CreateActorTab(
+      task_id, /*open_in_background=*/false,
+      /*initiator_tab_handle=*/tabs::TabHandle::Null(),
+      browser2->GetSessionID(), future.GetCallback());
+
+  tabs::TabInterface* new_tab = future.Take();
+  ASSERT_NE(new_tab, nullptr);
+  EXPECT_EQ(new_tab->GetProfile(), GetProfile());
+  EXPECT_EQ(profile2_tab_count_before, browser2->GetTabStripModel()->count());
+
+  browser2->GetWindow()->Close();
+}
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       CreateActorTabRejectsCrossProfileInitiatorTab) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  base::FilePath profile_path =
+      profile_manager->GenerateNextProfileDirectoryPath();
+  Profile& profile2 =
+      profiles::testing::CreateProfileSync(profile_manager, profile_path);
+
+  BrowserWindowInterface* browser2 = CreateBrowserWindow(
+      BrowserWindowCreateParams(&profile2, /*from_user_gesture=*/true));
+  chrome::NewTab(browser2, NewTabTypes::kNoUserAction);
+  tabs::TabInterface* tab2 = browser2->GetActiveTabInterface();
+  ASSERT_NE(tab2, nullptr);
+  ASSERT_TRUE(content::NavigateToURL(tab2->GetContents(),
+                                     GURL(chrome::kChromeUINewTabURL)));
+  ASSERT_TRUE(search::IsNTPURL(
+      tab2->GetContents()->GetPrimaryMainFrame()->GetLastCommittedURL()));
+  const int profile2_tab_count_before = browser2->GetTabStripModel()->count();
+
+  base::test::TestFuture<tabs::TabInterface*> future;
+  actor_keyed_service()->CreateActorTab(
+      task_id, /*open_in_background=*/false, tab2->GetHandle(),
+      browser2->GetSessionID(), future.GetCallback());
+
+  tabs::TabInterface* new_tab = future.Take();
+  EXPECT_EQ(new_tab, nullptr);
+  EXPECT_EQ(profile2_tab_count_before, browser2->GetTabStripModel()->count());
+
+  browser2->GetWindow()->Close();
+}
+
+IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
+                       RequestTabObservationRejectsCrossProfileTab) {
+  TaskId task_id = actor_keyed_service()->CreateTask(
+      TestTaskSourceInfo(), NoEnterprisePolicyChecker());
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  base::FilePath profile_path =
+      profile_manager->GenerateNextProfileDirectoryPath();
+  Profile& profile2 =
+      profiles::testing::CreateProfileSync(profile_manager, profile_path);
+
+  BrowserWindowInterface* browser2 = CreateBrowserWindow(
+      BrowserWindowCreateParams(&profile2, /*from_user_gesture=*/true));
+  chrome::NewTab(browser2, NewTabTypes::kNoUserAction);
+  tabs::TabInterface* tab2 = browser2->GetActiveTabInterface();
+
+  base::test::TestFuture<ActorKeyedService::TabObservationResult> future;
+  actor_keyed_service()->RequestTabObservation(
+      *tab2, task_id, /*screenshot_collection_options=*/std::nullopt,
+      future.GetCallback());
+
+  auto result = future.Take();
+  EXPECT_FALSE(result.has_value());
+
+  browser2->GetWindow()->Close();
+}
 #endif
 
 IN_PROC_BROWSER_TEST_F(ActorKeyedServiceBrowserTest,
diff --git a/chrome/browser/actor/actor_keyed_service_unittest.cc b/chrome/browser/actor/actor_keyed_service_unittest.cc
index 6ac8506..2c01c51 100644
--- a/chrome/browser/actor/actor_keyed_service_unittest.cc
+++ b/chrome/browser/actor/actor_keyed_service_unittest.cc
@@ -29,6 +29,7 @@
 #include "components/actor/core/actor_switches.h"
 #include "components/actor/core/task_source_info.h"
 #include "components/actor/public/mojom/actor_types.mojom.h"
+#include "components/tabs/public/mock_tab_interface.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -73,6 +74,12 @@
 
   TestingProfile* profile() { return profile_.get(); }
 
+  std::unique_ptr<tabs::MockTabInterface> CreateMockTab() {
+    auto mock_tab = std::make_unique<tabs::MockTabInterface>();
+    ON_CALL(*mock_tab, GetProfile).WillByDefault(testing::Return(profile()));
+    return mock_tab;
+  }
+
   void RunTasksUntilIdle() { task_environment_.RunUntilIdle(); }
 
  protected:
@@ -102,9 +109,11 @@
                                         NoEnterprisePolicyChecker());
 
   // Add a tab to the task
+  auto mock_tab = CreateMockTab();
+  const tabs::TabHandle tab_handle = mock_tab->GetHandle();
   base::WeakPtr<ActorTask> task = actor_service->GetTask(id)->GetWeakPtr();
   base::RunLoop loop;
-  task->AddTab(tabs::TabHandle(123),
+  task->AddTab(tab_handle,
                /*stop_task_on_detach=*/true,
                base::BindLambdaForTesting([&](mojom::ActionResultPtr result) {
                  EXPECT_TRUE(IsOk(*result));
@@ -112,8 +121,8 @@
                }));
   loop.Run();
 
-  EXPECT_TRUE(task->IsActingOnTab(tabs::TabHandle(123)));
-  EXPECT_TRUE(task->HasTab(tabs::TabHandle(123)));
+  EXPECT_TRUE(task->IsActingOnTab(tab_handle));
+  EXPECT_TRUE(task->HasTab(tab_handle));
   actor_service->StopTask(id, ActorTask::StoppedReason::kTaskComplete);
 
   // Tasks are deleted asynchronously.
@@ -150,7 +159,8 @@
 
   base::WeakPtr<ActorTask> task = actor_service->GetTask(id)->GetWeakPtr();
   ASSERT_TRUE(task);
-  const tabs::TabHandle tab_handle(123);
+  auto mock_tab = CreateMockTab();
+  const tabs::TabHandle tab_handle = mock_tab->GetHandle();
 
   // Pause the task and try to add a tab.
   task->Pause(/*from_actor=*/true);
@@ -184,7 +194,8 @@
 
   base::WeakPtr<ActorTask> task = actor_service->GetTask(id)->GetWeakPtr();
   ASSERT_TRUE(task);
-  const tabs::TabHandle tab_handle(123);
+  auto mock_tab = CreateMockTab();
+  const tabs::TabHandle tab_handle = mock_tab->GetHandle();
 
   {
     base::test::TestFuture<mojom::ActionResultPtr> future;
@@ -277,7 +288,8 @@
 
 TEST_F(ActorKeyedServiceTest, InitialTabAssociationOnCreate) {
   auto* actor_service = ActorKeyedService::Get(profile());
-  const tabs::TabHandle tab_handle(123);
+  auto mock_tab = CreateMockTab();
+  const tabs::TabHandle tab_handle = mock_tab->GetHandle();
   auto options = webui::mojom::TaskOptions::New();
   options->actuation_tab_id = tab_handle.raw_value();
diff --git a/chrome/browser/actor/actor_task_unittest.cc b/chrome/browser/actor/actor_task_unittest.cc
index 0f34e40..a449ea0 100644
--- a/chrome/browser/actor/actor_task_unittest.cc
+++ b/chrome/browser/actor/actor_task_unittest.cc
@@ -132,7 +132,10 @@
         .Times(1);
   }
 
-  void AddTabAndVerify(tabs::TabInterface& tab) {
+  void AddTabAndVerify(tabs::MockTabInterface& tab) {
+    if (!tab.GetProfile()) {
+      ON_CALL(tab, GetProfile).WillByDefault(::testing::Return(profile_.get()));
+    }
     ExpectTabAddedNotification(tab.GetHandle());
     AddTabToTask(tab, *task_);
     EXPECT_TRUE(task_->HasTab(tab.GetHandle()));
@@ -183,10 +186,25 @@
     service->ResetForTesting();
   }
 
+  std::unique_ptr<tabs::MockTabInterface> CreateCrossProfileMockTab(
+      Profile* other_profile = nullptr) {
+    if (!other_profile) {
+      if (!other_profile_) {
+        other_profile_ = TestingProfile::Builder().Build();
+      }
+      other_profile = other_profile_.get();
+    }
+    auto mock_tab = std::make_unique<tabs::MockTabInterface>();
+    ON_CALL(*mock_tab, GetProfile)
+        .WillByDefault(::testing::Return(other_profile));
+    return mock_tab;
+  }
+
  protected:
   content::BrowserTaskEnvironment task_environment_;
   base::test::ScopedFeatureList scoped_feature_list_;
   std::unique_ptr<TestingProfile> profile_;
+  std::unique_ptr<TestingProfile> other_profile_;
   MockActorTaskDelegate mock_delegate_;
   raw_ptr<ActorTask> task_;
   raw_ptr<ui::MockUiEventDispatcher> mock_ui_event_dispatcher_;
@@ -774,6 +792,61 @@
   histograms.ExpectTotalCount("Actor.Task.Count.Completed.Other", 0);
 }
 
+TEST_F(ActorTaskTest, AddTab_RejectsNonExistentTab) {
+  tabs::TabHandle non_existent_handle(99999);
+  base::test::TestFuture<mojom::ActionResultPtr> add_tab_future;
+  task_->AddTab(non_existent_handle, /*stop_task_on_detach=*/true,
+                add_tab_future.GetCallback());
+  auto result = add_tab_future.Take();
+  ASSERT_TRUE(result);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kTabWentAway);
+  EXPECT_FALSE(task_->HasTab(non_existent_handle));
+  EXPECT_FALSE(task_->GetTabs().contains(non_existent_handle));
+}
+
+TEST_F(ActorTaskTest, AddTab_RejectsCrossProfileTab) {
+  std::unique_ptr<tabs::MockTabInterface> cross_profile_tab =
+      CreateCrossProfileMockTab();
+
+  base::test::TestFuture<mojom::ActionResultPtr> add_tab_future;
+  task_->AddTab(cross_profile_tab->GetHandle(), /*stop_task_on_detach=*/true,
+                add_tab_future.GetCallback());
+  auto result = add_tab_future.Take();
+  ASSERT_TRUE(result);
+  EXPECT_EQ(result->code, mojom::ActionResultCode::kActionTargetCrossProfile);
+  EXPECT_FALSE(task_->HasTab(cross_profile_tab->GetHandle()));
+  EXPECT_FALSE(task_->GetTabs().contains(cross_profile_tab->GetHandle()));
+}
+
+TEST_F(ActorTaskTest, AddTab_RejectsCrossProfileTabEvenWithNullContents) {
+  std::unique_ptr<tabs::MockTabInterface> cross_profile_tab =
+      CreateCrossProfileMockTab();
+  ON_CALL(*cross_profile_tab, GetContents)
+      .WillByDefault(::testing::Return(nullptr));
+
+  base::test::TestFuture<mojom::ActionResultPtr> add_tab_future;
... (truncated)