Allow debugger extensions read file access if granted by user

Split MayAffectLocalFiles into MayReadLocalFiles and MayWriteLocalFiles,
where read version is now based on utils::AllowFileAccess.

Now testing both scenarios - with and without file access.

Bug: 928255
Change-Id: Ieed018dba2b00f864aa963e450fd68a904db7b57
Reviewed-on: https://chromium-review.googlesource.com/c/1479928
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635720}

chrome/browser/extensions/api/debugger/debugger_api.cc

@@ -149,7 +149,8 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
   bool MayAttachToRenderer(content::RenderFrameHost* render_frame_host,
                            bool is_webui) override;
   bool MayAttachToBrowser() override;
-  bool MayAffectLocalFiles() override;
+  bool MayReadLocalFiles() override;
+  bool MayWriteLocalFiles() override;
 
  private:
   using PendingRequests =
@@ -386,7 +387,11 @@ bool ExtensionDevToolsClientHost::MayAttachToBrowser() {
   return false;
 }
 
-bool ExtensionDevToolsClientHost::MayAffectLocalFiles() {
+bool ExtensionDevToolsClientHost::MayReadLocalFiles() {
+  return util::AllowFileAccess(extension_->id(), profile_);
+}
+
+bool ExtensionDevToolsClientHost::MayWriteLocalFiles() {
   return false;
 }
 

chrome/test/data/extensions/api_test/debugger/background.js

@@ -15,7 +15,6 @@ var unsupportedMajorProtocolVersion = "100.0";
 var SILENT_FLAG_REQUIRED = "Cannot attach to this target unless " +
     "'silent-debugger-extension-api' flag is enabled.";
 var DETACHED_WHILE_HANDLING = "Detached while handling command.";
-var NOT_ALLOWED = "Not allowed.";
 
 chrome.test.getConfig(config => chrome.test.runTests([
 
@@ -299,32 +298,6 @@ chrome.test.getConfig(config => chrome.test.runTests([
     });
   },
 
-  // https://crbug.com/866426
-  function setDownloadBehavior() {
-    chrome.tabs.create({url: 'inspected.html'}, function(tab) {
-      var debuggee = {tabId: tab.id};
-      chrome.debugger.attach(debuggee, protocolVersion, function() {
-        chrome.test.assertNoLastError();
-        chrome.debugger.sendCommand(debuggee, 'Page.setDownloadBehavior',
-            {behavior: 'allow'}, onResponse);
-
-        function onResponse() {
-          var message;
-          try {
-            message = JSON.parse(chrome.runtime.lastError.message).message;
-          } catch (e) {
-          }
-          chrome.debugger.detach(debuggee, () => {
-            if (message === NOT_ALLOWED)
-              chrome.test.succeed();
-            else
-              chrome.test.fail('' + message + ' instead of ' + NOT_ALLOWED);
-          });
-        }
-      });
-    });
-  },
-
   // http://crbug.com/824174
   function getResponseBodyInvalidChar() {
     let requestId;

chrome/test/data/extensions/api_test/debugger_file_access/background.js

@@ -20,6 +20,32 @@ function openTab(url) {
   });
 }
 
+function runNotAllowedTest(method, params, expectAllowed) {
+  const NOT_ALLOWED = "Not allowed";
+  chrome.tabs.create({url: 'dummy.html'}, function(tab) {
+    var debuggee = {tabId: tab.id};
+    chrome.debugger.attach(debuggee, '1.2', function() {
+      chrome.test.assertNoLastError();
+      chrome.debugger.sendCommand(debuggee, method, params, onResponse);
+
+      function onResponse() {
+        var message;
+        try {
+          message = JSON.parse(chrome.runtime.lastError.message).message;
+        } catch (e) {
+        }
+        chrome.debugger.detach(debuggee, () => {
+          const allowed = message !== NOT_ALLOWED;
+          if (allowed === expectAllowed)
+            chrome.test.succeed();
+          else
+            chrome.test.fail('' + message);
+        });
+      }
+    });
+  });
+}
+
 chrome.test.getConfig((config) => {
   const fileUrl = config.testDataDirectory + '/../body1.html';
   const expectFileAccess = !!config.customArg;
@@ -84,5 +110,19 @@ chrome.test.getConfig((config) => {
         });
       });
     },
+
+    // https://crbug.com/866426
+    function setDownloadBehavior() {
+      // We never allow to write local files.
+      runNotAllowedTest('Page.setDownloadBehavior', {behavior: 'allow'},
+          false);
+    },
+
+    // https://crbug.com/805557
+    function setFileInputFiles() {
+      // We only allow extensions with explicit file access to read local files.
+      runNotAllowedTest('DOM.setFileInputFiles', {nodeId: 1, files: []},
+          expectFileAccess);
+    },
   ]);
 });

content/browser/devtools/protocol/page_handler.cc

@@ -885,7 +885,7 @@ Response PageHandler::BringToFront() {
 Response PageHandler::SetDownloadBehavior(const std::string& behavior,
                                           Maybe<std::string> download_path) {
   if (!allow_set_download_behavior_)
-    return Response::Error("Not allowed.");
+    return Response::Error("Not allowed");
 
   WebContentsImpl* web_contents = GetWebContents();
   if (!web_contents)

content/browser/devtools/protocol/target_handler.cc

@@ -28,7 +28,7 @@ namespace protocol {
 
 namespace {
 
-static const char kNotAllowedError[] = "Not allowed.";
+static const char kNotAllowedError[] = "Not allowed";
 static const char kMethod[] = "method";
 static const char kResumeMethod[] = "Runtime.runIfWaitingForDebugger";
 

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -278,7 +278,7 @@ bool RenderFrameDevToolsAgentHost::AttachSession(DevToolsSession* session) {
       new protocol::EmulationHandler();
   session->AddHandler(base::WrapUnique(new protocol::BrowserHandler()));
   session->AddHandler(base::WrapUnique(
-      new protocol::DOMHandler(session->client()->MayAffectLocalFiles())));
+      new protocol::DOMHandler(session->client()->MayReadLocalFiles())));
   session->AddHandler(base::WrapUnique(emulation_handler));
   session->AddHandler(base::WrapUnique(new protocol::InputHandler()));
   session->AddHandler(base::WrapUnique(new protocol::InspectorHandler()));
@@ -301,7 +301,7 @@ bool RenderFrameDevToolsAgentHost::AttachSession(DevToolsSession* session) {
           : protocol::TargetHandler::AccessMode::kAutoAttachOnly,
       GetId(), GetRendererChannel(), session->GetRootSession())));
   session->AddHandler(base::WrapUnique(new protocol::PageHandler(
-      emulation_handler, session->client()->MayAffectLocalFiles())));
+      emulation_handler, session->client()->MayWriteLocalFiles())));
   session->AddHandler(base::WrapUnique(new protocol::SecurityHandler()));
   if (!frame_tree_node_ || !frame_tree_node_->parent()) {
     session->AddHandler(base::WrapUnique(

content/public/browser/devtools_agent_host_client.cc

@@ -16,7 +16,11 @@ bool DevToolsAgentHostClient::MayAttachToBrowser() {
   return true;
 }
 
-bool DevToolsAgentHostClient::MayAffectLocalFiles() {
+bool DevToolsAgentHostClient::MayReadLocalFiles() {
+  return true;
+}
+
+bool DevToolsAgentHostClient::MayWriteLocalFiles() {
   return true;
 }
 

content/public/browser/devtools_agent_host_client.h

@@ -37,9 +37,13 @@ class CONTENT_EXPORT DevToolsAgentHostClient {
   // manipulate browser altogether.
   virtual bool MayAttachToBrowser();
 
-  // Returns true if the client is allowed to affect local files over the
+  // Returns true if the client is allowed to read local files over the
+  // protocol. Example would be exposing file content to the page under debug.
+  virtual bool MayReadLocalFiles();
+
+  // Returns true if the client is allowed to write local files over the
   // protocol. Example would be manipulating a deault downloads path.
-  virtual bool MayAffectLocalFiles();
+  virtual bool MayWriteLocalFiles();
 
   // Determines protocol message format.
   virtual bool UsesBinaryProtocol();