Validate sender in terminal private api browser code

See the bugs for why we are doing this.

Notes:
* `tab_id` is removed. `TerminalPrivateAckOutputFunction::Run()` used to
  use `tab_id` to figure out whether it should actually acknowledge the
  output on the process --- it should only do so if the sender owns the
  terminal id. Now that we store the terminal id in the web contents, we
  don't need it anymore.
* We change the signature of api `ackOutput`. It is not going to
  break anything because only `.../externs/terminal_private.js`, which
  is also updated in this CL, but not "libapps" is supposed to call it.
* Potentially, we should kill the renderer if the validation fails, but
  we cannot do it for now because libapps might send a invalid ID before
  it receives a valid one.
* `SetLastActiveTerminal()` is used for setting the CWD for a new
  terminal, but due to a bug, it has been effectively disabled. We don't
  want this CL to change the CWD behavior so we explicitly disable it
  here. We will soon have a follow up CL to deal with the CWD feature.
  Also see https://crbug.com/1113207.

Fixed: 1145053, 1144625
Test: new browser tests, and manual tests
Change-Id: I0f39c544a28e23df3aeefe98b626c2b94b973af7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519182Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Jason Lin <lxj@google.com>
Cr-Commit-Position: refs/heads/master@{#825779}

chrome/browser/extensions/api/terminal/terminal_private_api.h

@@ -62,16 +62,13 @@ class TerminalPrivateOpenTerminalProcessFunction : public ExtensionFunction {
   void OnCrostiniRestarted(
       std::unique_ptr<CrostiniStartupStatus> startup_status,
       const std::string& user_id_hash,
-      int tab_id,
       base::CommandLine cmdline,
       crostini::CrostiniResult result);
 
   void OpenVmshellProcess(const std::string& user_id_hash,
-                          int tab_id,
                           base::CommandLine cmdline);
 
   void OnGetVshSession(const std::string& user_id_hash,
-                       int tab_id,
                        base::CommandLine cmdline,
                        int32_t vsh_pid,
                        bool success,
@@ -79,7 +76,6 @@ class TerminalPrivateOpenTerminalProcessFunction : public ExtensionFunction {
                        int32_t container_shell_pid);
 
   void OpenProcess(const std::string& user_id_hash,
-                   int tab_id,
                    base::CommandLine cmdline);
 
   using ProcessOutputCallback =

chrome/browser/extensions/api/terminal/terminal_private_apitest.cc

@@ -2,11 +2,100 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/location.h"
+#include "base/run_loop.h"
+#include "base/test/bind.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chromeos/process_proxy/process_proxy_registry.h"
+#include "content/public/browser/browser_task_traits.h"
 #include "content/public/test/browser_test.h"
 #include "extensions/common/switches.h"
 
+namespace {
+
+// For running and maintaining a `cat` process using `ProcessProxyRegistry`
+// directly.
+class CatProcess {
+ public:
+  CatProcess() {
+    base::RunLoop run_loop;
+
+    chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          chromeos::ProcessProxyRegistry* registry =
+              chromeos::ProcessProxyRegistry::Get();
+
+          auto on_output_on_process_thread = base::BindLambdaForTesting(
+              [this](const std::string&, const std::string&,
+                     const std::string&) {
+                content::GetUIThreadTaskRunner({})->PostTask(
+                    FROM_HERE, base::BindOnce(&CatProcess::OnOutputOnUIThread,
+                                              base::Unretained(this)));
+              });
+
+          // `user_id_hash` does not seems to matter in test.
+          this->ok_ = registry->OpenProcess(
+              base::CommandLine(base::FilePath("cat")), /*user_id_hash=*/"user",
+              on_output_on_process_thread, &this->process_id_);
+
+          run_loop.Quit();
+        }));
+
+    run_loop.Run();
+  }
+
+  ~CatProcess() {
+    if (ok_) {
+      chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              [](const std::string& process_id) {
+                chromeos::ProcessProxyRegistry::Get()->CloseProcess(process_id);
+              },
+              process_id_));
+    }
+  }
+
+  bool ok() const { return ok_; }
+  bool has_output() const { return has_output_; }
+  const std::string& process_id() const { return process_id_; }
+
+  void send_input_and_wait_output(const std::string& data) {
+    base::RunLoop run_loop;
+    CHECK(on_output_closure_.is_null());
+    on_output_closure_ = run_loop.QuitClosure();
+
+    chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          chromeos::ProcessProxyRegistry::Get()->SendInput(this->process_id_,
+                                                           data);
+        }));
+
+    run_loop.Run();
+  }
+
+ private:
+  void OnOutputOnUIThread() {
+    // We don't bother to call `ProcessProxyRegistry::AckOutput()` here since we
+    // only need to know whether there is some output.
+    has_output_ = true;
+
+    if (!on_output_closure_.is_null()) {
+      std::move(on_output_closure_).Run();
+    }
+  }
+
+  bool ok_;
+  std::string process_id_;
+  bool has_output_{false};
+  base::OnceClosure on_output_closure_;
+};
+
+}  // namespace
+
 class ExtensionTerminalPrivateApiTest : public extensions::ExtensionApiTest {
   void SetUpCommandLine(base::CommandLine* command_line) override {
     extensions::ExtensionApiTest::SetUpCommandLine(command_line);
@@ -16,7 +105,25 @@ class ExtensionTerminalPrivateApiTest : public extensions::ExtensionApiTest {
   }
 };
 
+IN_PROC_BROWSER_TEST_F(ExtensionTerminalPrivateApiTest, CatProcess) {
+  CatProcess cat_process;
+  ASSERT_TRUE(cat_process.ok());
+  ASSERT_FALSE(cat_process.has_output());
+  cat_process.send_input_and_wait_output("hello");
+  ASSERT_TRUE(cat_process.has_output());
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionTerminalPrivateApiTest, TerminalTest) {
-  EXPECT_TRUE(RunExtensionSubtest("terminal/component_extension", "test.html"))
+  CatProcess cat_process;
+  ASSERT_TRUE(cat_process.ok());
+
+  EXPECT_TRUE(
+      RunExtensionSubtest("terminal/component_extension",
+                          "test.html?foreign_id=" + cat_process.process_id()))
       << message_;
+
+  // Double check that test.html cannot write to the cat process here;
+  // Otherwises, we should detect some output.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(cat_process.has_output());
 }

chrome/common/extensions/api/terminal_private.json

@@ -173,11 +173,6 @@
         "type": "function",
         "description": "Called from |onProcessOutput| when the event is dispatched to terminal extension. Observing the terminal process output will be paused after |onProcessOutput| is dispatched until this method is called.",
         "parameters": [
-          {
-            "name": "tabId",
-            "type": "integer",
-            "description": "Tab ID from |onProcessOutput| event."
-          },
           {
             "name": "id",
             "type": "string",

chrome/renderer/resources/extensions/terminal_private_custom_bindings.js

@@ -3,14 +3,12 @@
 // found in the LICENSE file.
 
 // Custom bindings for chrome.terminalPrivate API.
-bindingUtil.registerEventArgumentMassager('terminalPrivate.onProcessOutput',
-                                          function(args, dispatch) {
-  var tabId = args[0];
-  var terminalId = args[1];
-  try {
-    // Remove tabId from event args, as it's not expected by listeners.
-    dispatch(args.slice(1));
-  } finally {
-    chrome.terminalPrivate.ackOutput(tabId, terminalId);
-  }
-});
+bindingUtil.registerEventArgumentMassager(
+    'terminalPrivate.onProcessOutput', function(args, dispatch) {
+      const terminalId = args[0];
+      try {
+        dispatch(args);
+      } finally {
+        chrome.terminalPrivate.ackOutput(terminalId);
+      }
+    });

chrome/test/data/extensions/api_test/terminal/component_extension/test.js

@@ -288,4 +288,23 @@ chrome.test.runTests([
     });
   },
 
+  function invalidTerminalIdTest() {
+    const foreign_id = (new URLSearchParams(location.search)).get('foreign_id');
+    chrome.test.assertTrue(!!foreign_id);
+
+    const callbackFail = chrome.test.callbackFail;
+
+    [foreign_id, 'invalid id'].forEach((id) => {
+      // Ideally, we will also want to test ackOutput, but it does not have a
+      // result callback.
+      chrome.terminalPrivate.closeTerminalProcess(
+          id, callbackFail('invalid terminal id'));
+      // If this manages to write to the `foreign_id` process, we should detect
+      // some output in terminal_private_apitest.cc.
+      chrome.terminalPrivate.sendInput(
+          id, 'hello', callbackFail('invalid terminal id'));
+      chrome.terminalPrivate.onTerminalResize(
+          id, 10, 10, callbackFail('invalid terminal id'));
+    });
+  },
 ]);

third_party/closure_compiler/externs/terminal_private.js

@@ -77,11 +77,10 @@ chrome.terminalPrivate.onTerminalResize = function(id, width, height, callback)
  * Called from |onProcessOutput| when the event is dispatched to terminal
  * extension. Observing the terminal process output will be paused after
  * |onProcessOutput| is dispatched until this method is called.
- * @param {number} tabId Tab ID from |onProcessOutput| event.
  * @param {string} id The id of the process to which |onProcessOutput| was
  *     dispatched.
  */
-chrome.terminalPrivate.ackOutput = function(tabId, id) {};
+chrome.terminalPrivate.ackOutput = function(id) {};
 
 /**
  * Open the Terminal tabbed window.