Browse Source

chore: cherry-pick 3b5f65c0aeca from chromium (#25859)

* chore: cherry-pick 3b5f65c0aeca from chromium

* update patches

Co-authored-by: Electron Bot <[email protected]>
Pedro Pontes 4 years ago
parent
commit
437701d0ef

+ 1 - 0
patches/chromium/.patches

@@ -142,3 +142,4 @@ cherry-pick-52dceba66599.patch
 cherry-pick-abc6ab85e704.patch
 avoid_use-after-free.patch
 cherry-pick-f06a6cb3a38e.patch
+reland_add_more_checks_for_chrome_debugger_extensions.patch

+ 335 - 0
patches/chromium/reland_add_more_checks_for_chrome_debugger_extensions.patch

@@ -0,0 +1,335 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Andrey Kosyakov <[email protected]>
+Date: Fri, 18 Sep 2020 22:35:09 +0000
+Subject: Reland "Add more checks for chrome.debugger extensions"
+
[email protected]
+
+This reverts commit 5a809a08fd5ca32cb8d594664416db2f2dc8ebdc.
+
+Reason for revert: I don't think the test failure is related. Please note it stopped before the revert landed (build no 91007 vs. 91010). This must have been a flake, or a independent failure that has been fixed by one of the front-end rolls.
+
+Original change's description:
+> Revert "Add more checks for chrome.debugger extensions"
+>
+> This reverts commit 4838b76ae48797760fd8a362b4dc15325ccddcf5.
+>
+> Reason for revert: 1119297
+>
+> Original change's description:
+> > Add more checks for chrome.debugger extensions
+> >
+> > Bug: 1113558, 1113565
+> > Change-Id: I99f2e030f9a38f1ffd6b6adc760ba15e5d231f96
+> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2342277
+> > Commit-Queue: Andrey Kosyakov <[email protected]>
+> > Reviewed-by: Sigurd Schneider <[email protected]>
+> > Reviewed-by: Yang Guo <[email protected]>
+> > Reviewed-by: Devlin <[email protected]>
+> > Reviewed-by: Dmitry Gozman <[email protected]>
+> > Cr-Commit-Position: refs/heads/master@{#799514}
+>
+> [email protected],[email protected],[email protected],[email protected],[email protected]
+>
+> Change-Id: I01ad12ca99ac75197f9073e2c6c9d0eaa0d95147
+> No-Presubmit: true
+> No-Tree-Checks: true
+> No-Try: true
+> Bug: 1113558
+> Bug: 1113565
+> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362920
+> Reviewed-by: Christian Dullweber <[email protected]>
+> Commit-Queue: Christian Dullweber <[email protected]>
+> Cr-Commit-Position: refs/heads/master@{#799558}
+
[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
+
+(cherry picked from commit a064db74c8734fbf47de2f3a3503832514857173)
+
+(cherry picked from commit 9940472e708a4003aee9edf9da42d68fde591e08)
+
+Bug: 1113558
+Bug: 1113565
+Change-Id: Ic98fc037028a210204b7935b0b8e50e4e36e2397
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368446
+Reviewed-by: Andrey Kosyakov <[email protected]>
+Commit-Queue: Andrey Kosyakov <[email protected]>
+Cr-Original-Original-Commit-Position: refs/heads/master@{#800682}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2398884
+Cr-Original-Commit-Position: refs/branch-heads/4240@{#506}
+Cr-Original-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419133
+Cr-Commit-Position: refs/branch-heads/4183@{#1863}
+Cr-Branched-From: 740e9e8a40505392ba5c8e022a8024b3d018ca65-refs/heads/master@{#782793}
+
+diff --git a/chrome/browser/extensions/api/debugger/debugger_apitest.cc b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
+index ea248c27dceafe2d5b16b7a96f8dac67741b1cbe..1e38c95d30dc9292a1ea86cebaaece2fe6550827 100644
+--- a/chrome/browser/extensions/api/debugger/debugger_apitest.cc
++++ b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
+@@ -379,4 +379,14 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessDebuggerExtensionApiTest, Debugger) {
+       << message_;
+ }
+ 
++IN_PROC_BROWSER_TEST_F(SitePerProcessDebuggerExtensionApiTest,
++                       NavigateSubframe) {
++  GURL url(embedded_test_server()->GetURL(
++      "a.com",
++      "/extensions/api_test/debugger_navigate_subframe/inspected_page.html"));
++  ASSERT_TRUE(
++      RunExtensionTestWithArg("debugger_navigate_subframe", url.spec().c_str()))
++      << message_;
++}
++
+ }  // namespace extensions
+diff --git a/chrome/test/data/extensions/api_test/debugger_navigate_subframe/background.js b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/background.js
+new file mode 100644
+index 0000000000000000000000000000000000000000..15bfe092cc00e720cf0abedffce519c5ae6e4806
+--- /dev/null
++++ b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/background.js
+@@ -0,0 +1,76 @@
++// Copyright 2020 The Chromium Authors. All rights reserved.
++// Use of this source code is governed by a BSD-style license that can be
++// found in the LICENSE file.
++
++const protocolVersion = '1.3';
++const DETACHED_WHILE_HANDLING = 'Detached while handling command.';
++
++function openTab(url) {
++  return new Promise((resolve) => {
++    let createdTabId;
++    let completedTabIds = [];
++    chrome.tabs.onUpdated.addListener(
++        function listener(tabId, changeInfo, tab) {
++      if (changeInfo.status !== 'complete')
++        return;  // Tab not done.
++
++      if (createdTabId === undefined) {
++        // A tab completed loading before the chrome.tabs.create callback was
++        // triggered; stash the ID for later comparison to see if it was our
++        // tab.
++        completedTabIds.push(tabId);
++        return;
++      }
++
++      if (tabId !== createdTabId)
++        return;  // Not our tab.
++
++      // It's ours!
++      chrome.tabs.onUpdated.removeListener(listener);
++      resolve(tab);
++    });
++    chrome.tabs.create({url: url}, (tab) => {
++      if (completedTabIds.includes(tab.id))
++        resolve(tab);
++      else
++        createdTabId = tab.id;
++    });
++  });
++}
++
++chrome.test.getConfig(config => chrome.test.runTests([
++  async function testNavigateSubframe() {
++    const topURL = config.customArg;
++    const subframeURL = topURL.replace('http://a.com', 'http://b.com');
++    const tab = await openTab(topURL);
++    const debuggee = {tabId: tab.id};
++    await new Promise(resolve =>
++        chrome.debugger.attach(debuggee, protocolVersion, resolve));
++
++    chrome.debugger.sendCommand(debuggee, 'Page.enable', null);
++    const response = await new Promise(resolve =>
++        chrome.debugger.sendCommand(debuggee, 'Page.getFrameTree', resolve));
++    const subframeId = response.frameTree.childFrames[0].frame.id;
++    const expression = `
++      new Promise(resolve => {
++        const frame = document.body.firstElementChild;
++        frame.onload = resolve;
++        frame.src = '${subframeURL}';
++      })
++    `;
++    await new Promise(resolve =>
++        chrome.debugger.sendCommand(debuggee, 'Runtime.evaluate', {
++            expression,
++            awaitPromise: true
++        }, resolve));
++    chrome.test.assertNoLastError();
++    const result = await new Promise(resolve =>
++      chrome.debugger.sendCommand(debuggee, 'Page.navigate', {
++          frameId: subframeId,
++          url: 'devtools://devtools/bundled/inspector.html'
++      }, resolve));
++    chrome.test.assertLastError(DETACHED_WHILE_HANDLING);
++
++    chrome.test.succeed();
++  }
++]));
+diff --git a/chrome/test/data/extensions/api_test/debugger_navigate_subframe/inspected_page.html b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/inspected_page.html
+new file mode 100644
+index 0000000000000000000000000000000000000000..190e207f7943a910f07e5e0c14bca5a02f7606a1
+--- /dev/null
++++ b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/inspected_page.html
+@@ -0,0 +1,3 @@
++<html>
++<iframe></iframe>
++</html>
+\ No newline at end of file
+diff --git a/chrome/test/data/extensions/api_test/debugger_navigate_subframe/manifest.json b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/manifest.json
+new file mode 100644
+index 0000000000000000000000000000000000000000..67ad49b974431d8869748b56dfc9896d53a6d1ae
+--- /dev/null
++++ b/chrome/test/data/extensions/api_test/debugger_navigate_subframe/manifest.json
+@@ -0,0 +1,11 @@
++{
++  "name": "Debugger API test for CDP-initiated navigation of subframes",
++  "version": "1.0",
++  "manifest_version": 2,
++  "background": {
++    "scripts": ["background.js"]
++  },
++  "permissions": [
++    "debugger"
++  ]
++}
+\ No newline at end of file
+diff --git a/content/browser/devtools/devtools_instrumentation.cc b/content/browser/devtools/devtools_instrumentation.cc
+index 91da6cb62a504cee3412c6c6a42b65924976f9f6..fa266980487bd941c997846260e476fabde7eccb 100644
+--- a/content/browser/devtools/devtools_instrumentation.cc
++++ b/content/browser/devtools/devtools_instrumentation.cc
+@@ -440,12 +440,23 @@ bool WillCreateURLLoaderFactory(
+ 
+ void OnNavigationRequestWillBeSent(
+     const NavigationRequest& navigation_request) {
+-  auto* agent_host = static_cast<RenderFrameDevToolsAgentHost*>(
+-      RenderFrameDevToolsAgentHost::GetFor(
+-          navigation_request.frame_tree_node()));
+-  if (!agent_host)
+-    return;
+-  agent_host->OnNavigationRequestWillBeSent(navigation_request);
++  // Note this intentionally deviates from the usual instrumentation signal
++  // logic and dispatches to all agents upwards from the frame, to make sure
++  // the security checks are properly applied even if no DevTools session is
++  // established for the navigated frame itself. This is because the page
++  // agent may navigate all of its subframes currently.
++  for (RenderFrameHostImpl* rfh =
++           navigation_request.frame_tree_node()->current_frame_host();
++       rfh; rfh = rfh->GetParent()) {
++    // Only check frames that qualify as DevTools targets, i.e. (local)? roots.
++    if (!RenderFrameDevToolsAgentHost::ShouldCreateDevToolsForHost(rfh))
++      continue;
++    auto* agent_host = static_cast<RenderFrameDevToolsAgentHost*>(
++        RenderFrameDevToolsAgentHost::GetFor(rfh));
++    if (!agent_host)
++      continue;
++    agent_host->OnNavigationRequestWillBeSent(navigation_request);
++  }
+ 
+   // Make sure both back-ends yield the same timestamp.
+   auto timestamp = base::TimeTicks::Now();
+diff --git a/content/browser/devtools/render_frame_devtools_agent_host.cc b/content/browser/devtools/render_frame_devtools_agent_host.cc
+index 61fd3685912e6623e79c0e7d2a58b60aefe1f1c4..4112d85d70f69c534aef39b2e0525658c7516b29 100644
+--- a/content/browser/devtools/render_frame_devtools_agent_host.cc
++++ b/content/browser/devtools/render_frame_devtools_agent_host.cc
+@@ -86,10 +86,6 @@ RenderFrameDevToolsAgentHost* FindAgentHost(FrameTreeNode* frame_tree_node) {
+   return it == g_agent_host_instances.Get().end() ? nullptr : it->second;
+ }
+ 
+-bool ShouldCreateDevToolsForHost(RenderFrameHost* rfh) {
+-  return rfh->IsCrossProcessSubframe() || !rfh->GetParent();
+-}
+-
+ bool ShouldCreateDevToolsForNode(FrameTreeNode* ftn) {
+   return !ftn->parent() || ftn->current_frame_host()->IsCrossProcessSubframe();
+ }
+@@ -140,6 +136,12 @@ scoped_refptr<DevToolsAgentHost> RenderFrameDevToolsAgentHost::GetOrCreateFor(
+   return result;
+ }
+ 
++// static
++bool RenderFrameDevToolsAgentHost::ShouldCreateDevToolsForHost(
++    RenderFrameHost* rfh) {
++  return rfh->IsCrossProcessSubframe() || !rfh->GetParent();
++}
++
+ // static
+ scoped_refptr<DevToolsAgentHost>
+ RenderFrameDevToolsAgentHost::CreateForCrossProcessNavigation(
+@@ -627,7 +629,9 @@ void RenderFrameDevToolsAgentHost::OnPageScaleFactorChanged(
+ 
+ void RenderFrameDevToolsAgentHost::OnNavigationRequestWillBeSent(
+     const NavigationRequest& navigation_request) {
+-  const auto& url = navigation_request.common_params().url;
++  GURL url = navigation_request.common_params().url;
++  if (url.SchemeIs(url::kJavaScriptScheme) && frame_host_)
++    url = frame_host_->GetLastCommittedURL();
+   std::vector<DevToolsSession*> restricted_sessions;
+   bool is_webui = frame_host_ && frame_host_->web_ui();
+   for (DevToolsSession* session : sessions()) {
+@@ -837,9 +841,16 @@ bool RenderFrameDevToolsAgentHost::ShouldAllowSession(
+       !manager->delegate()->AllowInspectingRenderFrameHost(frame_host_)) {
+     return false;
+   }
+-  // Note this may be called before navigation is committed.
+-  return session->GetClient()->MayAttachToURL(
+-      frame_host_->GetSiteInstance()->GetSiteURL(), frame_host_->web_ui());
++  auto* root = FrameTreeNode::From(frame_host_);
++  for (FrameTreeNode* node : root->frame_tree()->SubtreeNodes(root)) {
++    // Note this may be called before navigation is committed.
++    RenderFrameHostImpl* rfh = node->current_frame_host();
++    const GURL& url = rfh->GetSiteInstance()->GetSiteURL();
++    if (!session->GetClient()->MayAttachToURL(url, rfh->web_ui())) {
++      return false;
++    }
++  }
++  return true;
+ }
+ 
+ void RenderFrameDevToolsAgentHost::UpdateResourceLoaderFactories() {
+diff --git a/content/browser/devtools/render_frame_devtools_agent_host.h b/content/browser/devtools/render_frame_devtools_agent_host.h
+index 69a660094543476c22c253e7bb6eb5892b46a2a2..30ae26634fbdc3767c4f275cee1d5ec35f74b5be 100644
+--- a/content/browser/devtools/render_frame_devtools_agent_host.h
++++ b/content/browser/devtools/render_frame_devtools_agent_host.h
+@@ -61,6 +61,11 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
+   static scoped_refptr<DevToolsAgentHost> GetOrCreateFor(
+       FrameTreeNode* frame_tree_node);
+ 
++  // Whether the RFH passed may have associated DevTools agent host
++  // (i.e. the specified RFH is a local root). This does not indicate
++  // whether DevToolsAgentHost has actually been created.
++  static bool ShouldCreateDevToolsForHost(RenderFrameHost* rfh);
++
+   // This method is called when new frame is created during cross process
+   // navigation.
+   static scoped_refptr<DevToolsAgentHost> CreateForCrossProcessNavigation(
+diff --git a/content/browser/frame_host/frame_tree_node.cc b/content/browser/frame_host/frame_tree_node.cc
+index 2e2e1420440c751577304e0b79f9bbc44b37766d..55cc2227c2f0da9a9dc12c44a91ca6316e48463b 100644
+--- a/content/browser/frame_host/frame_tree_node.cc
++++ b/content/browser/frame_host/frame_tree_node.cc
+@@ -95,6 +95,13 @@ FrameTreeNode* FrameTreeNode::GloballyFindByID(int frame_tree_node_id) {
+   return it == nodes->end() ? nullptr : it->second;
+ }
+ 
++// static
++FrameTreeNode* FrameTreeNode::From(RenderFrameHost* rfh) {
++  if (!rfh)
++    return nullptr;
++  return static_cast<RenderFrameHostImpl*>(rfh)->frame_tree_node();
++}
++
+ FrameTreeNode::FrameTreeNode(
+     FrameTree* frame_tree,
+     Navigator* navigator,
+diff --git a/content/browser/frame_host/frame_tree_node.h b/content/browser/frame_host/frame_tree_node.h
+index 89ba68e856e90ba55c0a841dfab5a46afdca6ba8..d28993182e7d9ff0e55390cf128010d2db1e449e 100644
+--- a/content/browser/frame_host/frame_tree_node.h
++++ b/content/browser/frame_host/frame_tree_node.h
+@@ -69,6 +69,10 @@ class CONTENT_EXPORT FrameTreeNode {
+   // regardless of which FrameTree it is in.
+   static FrameTreeNode* GloballyFindByID(int frame_tree_node_id);
+ 
++  // Returns the FrameTreeNode for the given |rfh|. Same as
++  // rfh->frame_tree_node(), but also supports nullptrs.
++  static FrameTreeNode* From(RenderFrameHost* rfh);
++
+   // Callers are are expected to initialize sandbox flags separately after
+   // calling the constructor.
+   FrameTreeNode(