From 087822d5c2debaa323611cbb03d29ca63b7c6b19 Mon Sep 17 00:00:00 2001
From: Matthias Koerber <[email protected]>
Date: Mon, 22 Jun 2026 14:02:49 -0700
Subject: [PATCH] [Actor] Fix coordinate scaling in GetFieldIdFromPageTarget

This CL fixes a logic error in GetFieldIdFromPageTarget where coordinates
were incorrectly scaled before performing a compositor hit-test validation.

1. APC lookup (FindLastObservedNodeForActionTarget) now correctly scales
   up DIP coordinates to device pixels (BlinkSpace), which is the
   expected coordinate space for APC geometry.
2. Compositor hit-test (FindWidgetAtPoint) now uses unscaled DIP
   coordinates as expected by the API. Previously, it incorrectly
   multiplied coordinates by the Device Scale Factor due to a logic error
   using InvScale(1.0f / dsf).

These changes ensure that the anti-spoofing validation correctly identifies
the target frame even on high-DPI displays.

Bug: 521753402
Fixed: 517789833
Change-Id: I5fc631f72bed42c9cdc33a78bb765517beb46ed5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7925869
Reviewed-by: David Bokan <[email protected]>
Auto-Submit: Matthias Körber <[email protected]>
Commit-Queue: David Bokan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1650566}
---

diff --git a/chrome/browser/actor/tools/page_target_util.cc b/chrome/browser/actor/tools/page_target_util.cc
index dda385d3..df752cef 100644
--- a/chrome/browser/actor/tools/page_target_util.cc
+++ b/chrome/browser/actor/tools/page_target_util.cc
@@ -14,6 +14,7 @@
 #include "content/public/browser/web_contents.h"
 #include "third_party/abseil-cpp/absl/functional/overload.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/point_f.h"
 
 namespace actor {
@@ -60,36 +61,6 @@
   return root_frame;
 }
 
-}  // namespace
-
-RenderFrameHost* FindTargetLocalRootFrame(tabs::TabHandle tab_handle,
-                                          PageTarget target) {
-  tabs::TabInterface* tab = tab_handle.Get();
-  if (!tab) {
-    return nullptr;
-  }
-
-  content::WebContents& contents = *tab->GetContents();
-
-  if (std::holds_alternative<gfx::Point>(target)) {
-    content::RenderWidgetHost* target_rwh =
-        contents.FindWidgetAtPoint(gfx::PointF(std::get<gfx::Point>(target)));
-    if (!target_rwh) {
-      return nullptr;
-    }
-    return GetRootFrameForWidget(contents, target_rwh);
-  }
-
-  CHECK(std::holds_alternative<DomNode>(target));
-
-  content::RenderFrameHost* target_frame =
-      optimization_guide::GetRenderFrameForDocumentIdentifier(
-          *tab->GetContents(), std::get<DomNode>(target).document_identifier);
-
-  // After finding the target frame, walk up to its local root.
-  return GetLocalRoot(target_frame);
-}
-
 // Return TargetNodeInfo from hit test against last observed APC. Returns
 // std::nullopt if Target does not hit any node.
 std::optional<TargetNodeInfo> FindLastObservedNodeForActionTargetId(
@@ -116,32 +87,85 @@
     return std::nullopt;
   }
 
+  // TODO(crbug.com/426021822): FindNodeAtPoint does not handle corner cases
+  // like clip paths. Need more checks to ensure we don't drop actions
+  // unnecessarily.
   // TODO(rodneyding): Refactor FindNode* API to include optional target frame
   // document identifier to reduce search space.
   return optimization_guide::FindNodeAtPoint(*apc, target_blink_pixels);
 }
 
+}  // namespace
+
+// Return `TargetNodeInfo` from hit test against last observed APC. Returns
+// std::nullopt if Target does not hit any node.
+//
+// PageTarget coordinates are view-relative DIPs. This function handles
+// scaling to visual-viewport-relative device pixels (BlinkSpace) required by
+// APC hit testing.
 std::optional<TargetNodeInfo> FindLastObservedNodeForActionTarget(
     const AnnotatedPageContent* apc,
-    const PageTarget& target) {
+    const PageTarget& target,
+    tabs::TabInterface* tab) {
   return std::visit(
       absl::Overload{
           [&](const DomNode& node) {
             return FindLastObservedNodeForActionTargetId(apc, node);
           },
-          [&](const gfx::Point& point) {
-            return FindLastObservedNodeForActionTargetPoint(apc, point);
+          [&](const gfx::Point& point_dip) {
+            float dsf = 1.0f;
+            content::WebContents* contents = tab ? tab->GetContents() : nullptr;
+            content::RenderWidgetHostView* view =
+                contents ? contents->GetRenderWidgetHostView() : nullptr;
+            if (view) {
+              dsf = view->GetDeviceScaleFactor();
+            }
+            // APC hit testing requires visual-viewport-relative
+            // device pixels (BlinkSpace). PageTarget points are
+            // provided in DIPs.
+            return FindLastObservedNodeForActionTargetPoint(
+                apc, gfx::ScaleToRoundedPoint(point_dip, dsf));
           },
       },
       target);
 }
 
+RenderFrameHost* FindTargetLocalRootFrame(tabs::TabHandle tab_handle,
+                                          PageTarget target) {
+  tabs::TabInterface* tab = tab_handle.Get();
+  content::WebContents* contents = tab ? tab->GetContents() : nullptr;
+  if (!contents) {
+    return nullptr;
+  }
+
+  if (std::holds_alternative<gfx::Point>(target)) {
+    content::RenderWidgetHost* target_rwh =
+        contents->FindWidgetAtPoint(gfx::PointF(std::get<gfx::Point>(target)));
+    if (!target_rwh) {
+      return nullptr;
+    }
+    return GetRootFrameForWidget(*contents, target_rwh);
+  }
+
+  CHECK(std::holds_alternative<DomNode>(target));
+
+  content::RenderFrameHost* target_frame =
+      optimization_guide::GetRenderFrameForDocumentIdentifier(
+          *contents, std::get<DomNode>(target).document_identifier);
+
+  // After finding the target frame, walk up to its local root.
+  return GetLocalRoot(target_frame);
+}
+
 autofill::FieldGlobalId GetFieldIdFromPageTarget(
     const AnnotatedPageContent* last_observation,
     tabs::TabInterface* tab,
     const PageTarget& target) {
+  if (!tab) {
+    return {};
+  }
   if (std::optional<TargetNodeInfo> node_info =
-          FindLastObservedNodeForActionTarget(last_observation, target)) {
+          FindLastObservedNodeForActionTarget(last_observation, target, tab)) {
     if (content::WebContents* web_contents = tab->GetContents()) {
       if (RenderFrameHost* rfh =
               optimization_guide::GetRenderFrameForDocumentIdentifier(
@@ -153,25 +177,11 @@
         // a coordinate-targeted action to its own frame via a spoofed
         // popup_window.
         if (std::holds_alternative<gfx::Point>(target)) {
+          // FindWidgetAtPoint expects view-relative DIPs. PageTarget points are
+          // already in DIPs.
           const gfx::Point& point = std::get<gfx::Point>(target);
-          float dsf = 1.0f;
-          if (web_contents->GetRenderWidgetHostView()) {
-            dsf =
-                web_contents->GetRenderWidgetHostView()->GetDeviceScaleFactor();
-          }
-
-          // The target point is provided in visual-viewport-relative device
-          // pixels (BlinkSpace). We must scale it to view-relative DIPs first
-          // before passing to FindWidgetAtPoint. Testing the unscaled point
-          // would introduce an aliasing vulnerability under non-1.0 scale
-          // factors.
-          gfx::PointF point_dip(point);
-          if (dsf > 0.0f) {
-            point_dip.InvScale(1.0f / dsf);
-          }
-
           content::RenderWidgetHost* actual_rwh =
-              web_contents->FindWidgetAtPoint(point_dip);
+              web_contents->FindWidgetAtPoint(gfx::PointF(point));
           if (!actual_rwh || actual_rwh != rfh->GetRenderWidgetHost()) {
             return {};
           }
diff --git a/chrome/browser/actor/tools/page_target_util.h b/chrome/browser/actor/tools/page_target_util.h
index 3c1fe998..d47e0388 100644
--- a/chrome/browser/actor/tools/page_target_util.h

diff --git a/chrome/browser/actor/tools/page_target_util_browsertest.cc b/chrome/browser/actor/tools/page_target_util_browsertest.cc
index 15d2dcee..963e6c3 100644
--- a/chrome/browser/actor/tools/page_target_util_browsertest.cc
+++ b/chrome/browser/actor/tools/page_target_util_browsertest.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/actor/actor_tab_data.h"
 #include "chrome/browser/actor/tools/tools_test_util.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -19,6 +20,7 @@
 #include "components/tabs/public/tab_interface.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
@@ -114,4 +116,105 @@
   EXPECT_FALSE(resolved_id);
 }
 
+class PageTargetUtilSecurityHiDpiTest : public PageTargetUtilSecurityTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    PageTargetUtilSecurityTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitchASCII("force-device-scale-factor", "2");
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PageTargetUtilSecurityHiDpiTest,
+                       GetFieldIdFromPageTarget_InvScaleBypassAtHiDpi) {
+  // 1. Load a page and inject a cross-origin OOPIF.
+  GURL main_url = embedded_https_test_server().GetURL("a.com", "/empty.html");
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), main_url));
+
+  content::RenderFrameHost* main_frame = web_contents()->GetPrimaryMainFrame();
+  ASSERT_TRUE(
+      content::ExecJs(main_frame,
+                      "document.body.style.margin = '0';"
+                      "var f = document.createElement('iframe');"
+                      "f.id = 'test';"
+                      "f.style.position = 'absolute'; f.style.left = '100px'; "
+                      "f.style.top = '0px';"
+                      "f.style.width = '200px'; f.style.height = '200px';"
+                      "f.style.border = 'none';"
+                      "document.body.appendChild(f);"));
+
+  GURL iframe_url =
+      embedded_https_test_server().GetURL("b.com", "/title1.html");
+  ASSERT_TRUE(content::NavigateIframeToURL(web_contents(), "test", iframe_url));
+
+  content::RenderFrameHost* child_frame = content::ChildFrameAt(main_frame, 0);
+  ASSERT_TRUE(child_frame->IsCrossProcessSubframe());
+
+  // Wait for the hit test data to be fully propagated.
+  content::WaitForHitTestData(child_frame);
+
+  // Verify DSF=2.0
+  ASSERT_TRUE(web_contents()->GetRenderWidgetHostView());
+  float dsf = web_contents()->GetRenderWidgetHostView()->GetDeviceScaleFactor();
+  ASSERT_EQ(dsf, 2.0f);
+
+  // 2. Setup the security scenario.
+  // Target Point: DIP (150, 50). This is physically over the OOPIF (x:100-300).
+  //
+  // SECURITY SCENARIO:
+  // - A compromised renderer sends a spoofed APC claiming its own frame
+  //   (child_frame) has a popup at (150, 50) DIPs.
+  // - Because of the fix, the browser scales (150, 50) DIP -> (300, 100) DP.
+  // - FindLastObservedNodeForActionTarget(300, 100) DP correctly hits the
+  //   attacker's popup in the APC.
+  // - BUT, the compositor hit-test at (150, 50) DIP must confirm the frame.
+  // - If the compositor hit-test returns the main frame (due to clipping,
+  //   occlusion, or builder environmental quirks), the action MUST be blocked.
+  gfx::Point target_point_dip(150, 50);
+
+  // 3. Create a spoofed APC with a popup_window belonging to the attacker.
+  optimization_guide::proto::AnnotatedPageContent apc;
+  auto* popup = apc.mutable_popup_window();
+
+  auto* user_data = optimization_guide::DocumentIdentifierUserData::
+      GetOrCreateForCurrentDocument(child_frame);
+  std::string attacker_doc_id = user_data->serialized_token();
+
+  popup->mutable_opener_document_id()->set_serialized_token(attacker_doc_id);
+  auto* root_node = popup->mutable_root_node();
+  root_node->mutable_content_attributes()->set_common_ancestor_dom_node_id(
+      4242);
+  root_node->mutable_content_attributes()
+      ->mutable_interaction_info()
+      ->set_document_scoped_z_order(1);
+
+  // Set popup bounds in APC coordinates (visual-viewport device pixels).
+  // Input DIP (150, 50) scales to (300, 100) device pixels.
+  auto* geometry = root_node->mutable_content_attributes()->mutable_geometry();
+  auto* box = geometry->mutable_visible_bounding_box();
+  box->set_x(290);
+  box->set_y(90);
+  box->set_width(20);
+  box->set_height(20);
+
+  // 4. Test GetFieldIdFromPageTarget.
+  autofill::FieldGlobalId resolved_id =
+      GetFieldIdFromPageTarget(&apc, active_tab(), target_point_dip);
+
+  content::RenderWidgetHost* actual_rwh =
+      web_contents()->FindWidgetAtPoint(gfx::PointF(target_point_dip));
+
+  if (actual_rwh == child_frame->GetRenderWidgetHost()) {
+    // If the compositor resolves the frame correctly, the tool must also
+    // succeed. This verifies that coordinate scaling for APC lookup is correct.
+    ASSERT_TRUE(resolved_id);
+    EXPECT_EQ(resolved_id.frame_token,
+              autofill::LocalFrameToken(child_frame->GetFrameToken().value()));
+  } else {
+    // If the compositor hit-test doesn't reach the OOPIF (e.g. ChromeOS builder
+    // environment quirks), we must return empty to prevent a frame mismatch.
+    // This confirms the anti-spoofing security check is working.
+    EXPECT_FALSE(resolved_id);
+  }
+}
+
 }  // namespace actor