From d3213f433baaa53a3c3542e8869110ba75f24f40 Mon Sep 17 00:00:00 2001
From: Danil Somsikov <dsv@chromium.org>
Date: Tue, 22 Sep 2026 22:39:40 -0700
Subject: [PATCH] [DevTools] Fix multiple bypasses of DevToolsManagerDelegate allowlist This CL addresses a series of DevToolsManagerDelegate filtering bypasses where restricted targets could be impermissibly activated, closed, or attached to via side channels: 1. `/json/activate` and `/json/close` bypass:    The HTTP endpoints for activating and closing targets by ID failed    to evaluate `AllowInspectingTarget` prior to acting on the target.    This permitted attackers to activate or close forbidden targets (like    extension background pages). The required delegate check has been added    to gracefully return an HTTP 403. 2. WebSocket Upgrade connection bypass:    `OnWebSocketRequest` instantiated `DevToolsAgentHostClientImpl`    without directly enforcing `AllowInspectingTarget`. This allowed    an upgrade request to establish an open WebSocket session to a forbidden    target (even if subsequent Protocol messages were blackholed). A    delegate check is now performed during the HTTP handshake to return    HTTP 403 on such WebSocket upgrade attempts. 3. Persistent Session Navigation bypass:    If DevTools was legally attached to a permitted target (e.g., `about:blank`),    that target could subsequently navigate to a restricted URL (such as a    chrome-extension:// page) within the same `RenderFrameHost` process.    Because `RenderFrameDevToolsAgentHost::UpdateFrameHost` previously    early-returned when `frame_host == frame_host_`, it skipped re-evaluating    the delegate's `AllowInspectingRenderFrameHost` constraint on the new URL.    This CL modifies `RenderFrameDevToolsAgentHost` and    `WebContentsDevToolsAgentHost` to actively re-evaluate    `ShouldAllowSession()` upon every committed navigation, eagerly    detaching unauthorized inspectors so they do not persist post-navigation.

Bug: 533644359
Test: content_unittests --gtest_filter='DevToolsHttpHandlerWithServerTest.*RespectsAllowlist'
TAG=agy
CONV=40055dc5-b234-4951-959b-446627c09f2e

Fix: 533644359
Change-Id: Ic60f6f8aba91a11bbbef8df224ff8155c26d8d27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8309334
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Auto-Submit: Danil Somsikov <dsv@chromium.org>
Commit-Queue: Danil Somsikov <dsv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1703193}
---
diff --git a/content/browser/devtools/devtools_http_handler.cc b/content/browser/devtools/devtools_http_handler.cc
index 45ff52e..fab13300 100644
--- a/content/browser/devtools/devtools_http_handler.cc
+++ b/content/browser/devtools/devtools_http_handler.cc
@@ -708,6 +708,14 @@
       return;
     }
 
+    DevToolsManager* manager = DevToolsManager::GetInstance();
+    if (manager->delegate() &&
+        !manager->delegate()->AllowInspectingTarget(agent_host.get())) {
+      SendJson(connection_id, net::HTTP_FORBIDDEN, std::nullopt,
+               "Target is not inspectable");
+      return;
+    }
+
     if (command == "activate") {
       if (agent_host->Activate()) {
         SendJson(connection_id, net::HTTP_OK, std::nullopt, "Target activated");
@@ -891,6 +899,13 @@
     return;
   }
 
+  DevToolsManager* manager = DevToolsManager::GetInstance();
+  if (manager->delegate() &&
+      !manager->delegate()->AllowInspectingTarget(agent.get())) {
+    Send403(connection_id, "Target is not inspectable");
+    return;
+  }
+
   connection_to_client_[connection_id] =
       std::make_unique<DevToolsAgentHostClientImpl>(
           thread_->task_runner(), server_wrapper_.get(), connection_id, agent);
diff --git a/content/browser/devtools/render_frame_devtools_agent_host.cc b/content/browser/devtools/render_frame_devtools_agent_host.cc
index 0b74f95b..46b4abb0 100644
--- a/content/browser/devtools/render_frame_devtools_agent_host.cc
+++ b/content/browser/devtools/render_frame_devtools_agent_host.cc
@@ -606,12 +606,6 @@
 
 void RenderFrameDevToolsAgentHost::UpdateFrameHost(
     RenderFrameHostImpl* frame_host) {
-  if (frame_host == frame_host_) {
-    if (frame_host && !render_frame_alive_)
-      UpdateFrameAlive();
-    return;
-  }
-
   if (frame_host && !ShouldCreateDevToolsForHost(frame_host)) {
     DestroyOnRenderFrameGone();
     // |this| may be deleted at this point.
@@ -630,6 +624,12 @@
     ForceDetachRestrictedSessions(restricted_sessions);
   }
 
+  if (frame_host == frame_host_) {
+    if (frame_host && !render_frame_alive_)
+      UpdateFrameAlive();
+    return;
+  }
+
   RenderFrameHostImpl* old_host = frame_host_;
   ChangeFrameHostAndObservedProcess(frame_host);
   if (IsAttached()) {
diff --git a/content/browser/devtools/web_contents_devtools_agent_host.cc b/content/browser/devtools/web_contents_devtools_agent_host.cc
index e4d821f..e704626 100644
--- a/content/browser/devtools/web_contents_devtools_agent_host.cc
+++ b/content/browser/devtools/web_contents_devtools_agent_host.cc
@@ -451,4 +451,10 @@
   return retain_this;
 }
 
+void WebContentsDevToolsAgentHost::DidFinishNavigation(
+    NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInPrimaryMainFrame() && navigation_handle->HasCommitted()) {
+    std::ignore = RevalidateSessionAccess();
+  }
+}
 }  // namespace content
diff --git a/content/browser/devtools/web_contents_devtools_agent_host.h b/content/browser/devtools/web_contents_devtools_agent_host.h
index a90e817..3146db4 100644
--- a/content/browser/devtools/web_contents_devtools_agent_host.h
+++ b/content/browser/devtools/web_contents_devtools_agent_host.h
@@ -86,6 +86,7 @@
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override;
   void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override;
   void FrameDeleted(FrameTreeNodeId frame_tree_node_id) override;
 
   DevToolsAgentHostImpl* GetPrimaryFrameAgent();
diff --git a/content/browser/devtools/devtools_http_handler_unittest.cc b/content/browser/devtools/devtools_http_handler_unittest.cc
index 84b2feb..99b13fb3 100644
--- a/content/browser/devtools/devtools_http_handler_unittest.cc
+++ b/content/browser/devtools/devtools_http_handler_unittest.cc
@@ -157,6 +157,8 @@
     last_instance = this;
     EXPECT_CALL(*this, SetActiveWebSocketConnections(testing::_))
         .Times(testing::AtLeast(0));
+    ON_CALL(*this, AllowInspectingTarget(testing::_))
+        .WillByDefault(testing::Return(true));
   }
   MOCK_METHOD(scoped_refptr<DevToolsAgentHost>,
               CreateNewTarget,
@@ -174,6 +176,10 @@
               SetActiveWebSocketConnections,
               (size_t count),
               (override));
+  MOCK_METHOD(bool,
+              AllowInspectingTarget,
+              (DevToolsAgentHost*),
+              (override));
 };
 
 MockDevToolsManagerDelegate* MockDevToolsManagerDelegate::last_instance =
@@ -774,4 +780,41 @@
   StopServer();
 }
 
+
+TEST_F(DevToolsHttpHandlerWithServerTest, JsonActivateCloseRespectsAllowlist) {
+  scoped_refptr<DevToolsAgentHost> host = DevToolsAgentHost::CreateForDiscovery();
+  int port = StartServer();
+
+  EXPECT_CALL(*MockDevToolsManagerDelegate::last_instance, AllowInspectingTarget(testing::_))
+      .WillRepeatedly(testing::Return(false));
+
+  auto request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/json/activate/%s", port, host->GetId().c_str()));
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/json/close/%s", port, host->GetId().c_str()));
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  StopServer();
+}
+
+TEST_F(DevToolsHttpHandlerWithServerTest, WebSocketRespectsAllowlist) {
+  scoped_refptr<DevToolsAgentHost> host = DevToolsAgentHost::CreateForDiscovery();
+  int port = StartServer();
+
+  EXPECT_CALL(*MockDevToolsManagerDelegate::last_instance, AllowInspectingTarget(testing::_))
+      .WillRepeatedly(testing::Return(false));
+
+  auto request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/devtools/page/%s", port, host->GetId().c_str()),
+      {{"connection", "upgrade"}, {"upgrade", "websocket"}});
+
+  // Expect HTTP 403 Forbidden because AllowInspectingTarget returned false
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  StopServer();
+}
+
+
 }  // namespace content

diff --git a/content/browser/devtools/devtools_http_handler_unittest.cc b/content/browser/devtools/devtools_http_handler_unittest.cc
index 84b2feb..99b13fb3 100644
--- a/content/browser/devtools/devtools_http_handler_unittest.cc
+++ b/content/browser/devtools/devtools_http_handler_unittest.cc
@@ -157,6 +157,8 @@
     last_instance = this;
     EXPECT_CALL(*this, SetActiveWebSocketConnections(testing::_))
         .Times(testing::AtLeast(0));
+    ON_CALL(*this, AllowInspectingTarget(testing::_))
+        .WillByDefault(testing::Return(true));
   }
   MOCK_METHOD(scoped_refptr<DevToolsAgentHost>,
               CreateNewTarget,
@@ -174,6 +176,10 @@
               SetActiveWebSocketConnections,
               (size_t count),
               (override));
+  MOCK_METHOD(bool,
+              AllowInspectingTarget,
+              (DevToolsAgentHost*),
+              (override));
 };
 
 MockDevToolsManagerDelegate* MockDevToolsManagerDelegate::last_instance =
@@ -774,4 +780,41 @@
   StopServer();
 }
 
+
+TEST_F(DevToolsHttpHandlerWithServerTest, JsonActivateCloseRespectsAllowlist) {
+  scoped_refptr<DevToolsAgentHost> host = DevToolsAgentHost::CreateForDiscovery();
+  int port = StartServer();
+
+  EXPECT_CALL(*MockDevToolsManagerDelegate::last_instance, AllowInspectingTarget(testing::_))
+      .WillRepeatedly(testing::Return(false));
+
+  auto request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/json/activate/%s", port, host->GetId().c_str()));
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/json/close/%s", port, host->GetId().c_str()));
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  StopServer();
+}
+
+TEST_F(DevToolsHttpHandlerWithServerTest, WebSocketRespectsAllowlist) {
+  scoped_refptr<DevToolsAgentHost> host = DevToolsAgentHost::CreateForDiscovery();
+  int port = StartServer();
+
+  EXPECT_CALL(*MockDevToolsManagerDelegate::last_instance, AllowInspectingTarget(testing::_))
+      .WillRepeatedly(testing::Return(false));
+
+  auto request = RunRequestUntilCompletion(
+      base::StringPrintf("http://127.0.0.1:%d/devtools/page/%s", port, host->GetId().c_str()),
+      {{"connection", "upgrade"}, {"upgrade", "websocket"}});
+
+  // Expect HTTP 403 Forbidden because AllowInspectingTarget returned false
+  EXPECT_EQ(403, request->response_info().headers->response_code());
+
+  StopServer();
+}
+
+
 }  // namespace content