Report native messaging connection errors back to the native app.

When a native application attempts to initiate a native messaging
connection with a cooperating extension, that extension may not be
installed; in this case, the native app should be informed by launching
its native messaging host to notify it of this error. This is also
needed if the native app provides invalid input when requesting this
communication channel.

Add NativeMessagingHostErrorReporter, a self-owning class responsible
for starting the native messaging host with flag specific to the error
and keeping Chrome alive until either the native messaging host
terminates (upon informing the original native app of the error), or a
10 second timeout to avoid Chrome getting stuck due to a misbehaving
native messaging host.

Add plumbing to the existing native messaging host launch code to handle
the additional parameter.

Bug: 967262
Change-Id: Ie68cbae094de226fa43244719abda434d6200590
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1675587Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712836}

chrome/browser/extensions/BUILD.gn

@@ -1046,6 +1046,7 @@ jumbo_static_library("extensions") {
       "default_apps.cc",
       "default_apps.h",
     ]
+    deps += [ "//components/keep_alive_registry" ]
     if (is_posix) {
       sources += [ "api/messaging/native_process_launcher_posix.cc" ]
     }

chrome/browser/extensions/api/messaging/native_message_process_host.cc

@@ -108,7 +108,7 @@ std::unique_ptr<NativeMessageHost> NativeMessageHost::Create(
           GetProfilePathIfEnabled(Profile::FromBrowserContext(browser_context),
                                   source_extension_id, native_host_name),
           /* require_native_initiated_connections = */ false,
-          /* connect_id = */ ""));
+          /* connect_id = */ "", /* error_arg = */ ""));
 }
 
 // static

chrome/browser/extensions/api/messaging/native_messaging_launch_from_native.cc

@@ -7,14 +7,26 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
+#include "base/task/post_task.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/messaging/native_message_port.h"
 #include "chrome/browser/extensions/api/messaging/native_message_process_host.h"
 #include "chrome/browser/extensions/api/messaging/native_process_launcher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/manifest_handlers/natively_connectable_handler.h"
+#include "components/keep_alive_registry/keep_alive_types.h"
+#include "components/keep_alive_registry/scoped_keep_alive.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/api/messaging/channel_endpoint.h"
 #include "extensions/browser/api/messaging/message_service.h"
+#include "extensions/browser/api/messaging/native_message_host.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/api/messaging/messaging_endpoint.h"
@@ -27,6 +39,88 @@ namespace {
 ScopedAllowNativeAppConnectionForTest* g_allow_native_app_connection_for_test =
     nullptr;
 
+constexpr base::TimeDelta kNativeMessagingHostErrorTimeout =
+    base::TimeDelta::FromSeconds(10);
+
+ScopedNativeMessagingErrorTimeoutOverrideForTest*
+    g_native_messaging_host_timeout_override = nullptr;
+
+// A self-owning class responsible for starting a native messaging host with
+// command-line parameters reporting an error, keeping Chrome alive until the
+// host terminates, or a timeout is reached.
+//
+// This lives on the IO thread, but its public factory static method should be
+// called on the UI thread.
+class NativeMessagingHostErrorReporter : public NativeMessageHost::Client {
+ public:
+  using MovableScopedKeepAlive =
+      std::unique_ptr<ScopedKeepAlive,
+                      content::BrowserThread::DeleteOnUIThread>;
+
+  static void Report(const std::string& extension_id,
+                     const std::string& host_id,
+                     const std::string& connection_id,
+                     Profile* profile,
+                     const std::string& error_arg) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    std::unique_ptr<NativeMessageHost> host =
+        NativeMessageProcessHost::CreateWithLauncher(
+            extension_id, host_id,
+            NativeProcessLauncher::CreateDefault(
+                /* allow_user_level = */ true,
+                /* native_view = */ nullptr, profile->GetPath(),
+                /* require_native_initiated_connections = */ false,
+                connection_id, error_arg));
+    MovableScopedKeepAlive keep_alive(
+        new ScopedKeepAlive(KeepAliveOrigin::NATIVE_MESSAGING_HOST_ERROR_REPORT,
+                            KeepAliveRestartOption::DISABLED));
+    base::PostTask(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(&NativeMessagingHostErrorReporter::ReportOnIoThread,
+                       std::move(host), std::move(keep_alive)));
+  }
+
+ private:
+  static void ReportOnIoThread(std::unique_ptr<NativeMessageHost> process,
+                               MovableScopedKeepAlive keep_alive) {
+    new NativeMessagingHostErrorReporter(std::move(process),
+                                         std::move(keep_alive));
+  }
+
+  NativeMessagingHostErrorReporter(std::unique_ptr<NativeMessageHost> process,
+                                   MovableScopedKeepAlive keep_alive)
+      : keep_alive_(std::move(keep_alive)), process_(std::move(process)) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    timeout_.Start(
+        FROM_HERE,
+        g_native_messaging_host_timeout_override
+            ? g_native_messaging_host_timeout_override->timeout()
+            : kNativeMessagingHostErrorTimeout,
+        base::BindOnce(&base::DeletePointer<NativeMessagingHostErrorReporter>,
+                       base::Unretained(this)));
+    process_->Start(this);
+  }
+
+ private:
+  // NativeMessageHost::Client:
+  void PostMessageFromNativeHost(const std::string& message) override {}
+
+  void CloseChannel(const std::string& error_message) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    timeout_.AbandonAndStop();
+
+    base::SequencedTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  }
+
+  MovableScopedKeepAlive keep_alive_;
+  std::unique_ptr<NativeMessageHost> process_;
+  base::OneShotTimer timeout_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeMessagingHostErrorReporter);
+};
+
 }  // namespace
 
 bool ExtensionSupportsConnectionFromNativeApp(const std::string& extension_id,
@@ -95,6 +189,19 @@ ScopedAllowNativeAppConnectionForTest::
   g_allow_native_app_connection_for_test = nullptr;
 }
 
+ScopedNativeMessagingErrorTimeoutOverrideForTest::
+    ScopedNativeMessagingErrorTimeoutOverrideForTest(base::TimeDelta timeout)
+    : timeout_(timeout) {
+  DCHECK(!g_native_messaging_host_timeout_override);
+  g_native_messaging_host_timeout_override = this;
+}
+
+ScopedNativeMessagingErrorTimeoutOverrideForTest::
+    ~ScopedNativeMessagingErrorTimeoutOverrideForTest() {
+  DCHECK_EQ(this, g_native_messaging_host_timeout_override);
+  g_native_messaging_host_timeout_override = nullptr;
+}
+
 bool IsValidConnectionId(const base::StringPiece connection_id) {
   return connection_id.size() <= 20 &&
          base::ContainsOnlyChars(
@@ -107,12 +214,16 @@ void LaunchNativeMessageHostFromNativeApp(const std::string& extension_id,
                                           const std::string& connection_id,
                                           Profile* profile) {
   if (!IsValidConnectionId(connection_id)) {
-    // TODO(crbug.com/967262): Report errors to the native messaging host.
+    NativeMessagingHostErrorReporter::Report(extension_id, host_id,
+                                             /* connect_id = */ {}, profile,
+                                             "--invalid-connect-id");
     return;
   }
   if (!ExtensionSupportsConnectionFromNativeApp(extension_id, host_id, profile,
                                                 /* log_errors = */ true)) {
-    // TODO(crbug.com/967262): Report errors to the native messaging host.
+    NativeMessagingHostErrorReporter::Report(extension_id, host_id,
+                                             connection_id, profile,
+                                             "--extension-not-installed");
     return;
   }
   const extensions::PortId port_id(base::UnguessableToken::Create(),
@@ -125,7 +236,8 @@ void LaunchNativeMessageHostFromNativeApp(const std::string& extension_id,
       NativeProcessLauncher::CreateDefault(
           /* allow_user_level = */ true, /* native_view = */ nullptr,
           profile->GetPath(),
-          /* require_native_initiated_connections = */ true, connection_id));
+          /* require_native_initiated_connections = */ true, connection_id,
+          ""));
   auto native_message_port = std::make_unique<extensions::NativeMessagePort>(
       message_service->GetChannelDelegate(), port_id,
       std::move(native_message_host));

chrome/browser/extensions/api/messaging/native_messaging_launch_from_native.h

@@ -7,6 +7,7 @@
 
 #include <string>
 #include "base/macros.h"
+#include "base/time/time.h"
 
 class Profile;
 
@@ -40,6 +41,19 @@ class ScopedAllowNativeAppConnectionForTest {
   DISALLOW_COPY_AND_ASSIGN(ScopedAllowNativeAppConnectionForTest);
 };
 
+class ScopedNativeMessagingErrorTimeoutOverrideForTest {
+ public:
+  explicit ScopedNativeMessagingErrorTimeoutOverrideForTest(
+      base::TimeDelta timeout);
+  ~ScopedNativeMessagingErrorTimeoutOverrideForTest();
+
+  base::TimeDelta timeout() const { return timeout_; }
+
+ private:
+  const base::TimeDelta timeout_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedNativeMessagingErrorTimeoutOverrideForTest);
+};
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_MESSAGING_NATIVE_MESSAGING_LAUNCH_FROM_NATIVE_H_

chrome/browser/extensions/api/messaging/native_messaging_test_util.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
@@ -95,10 +96,19 @@ void ScopedTestNativeMessagingHost::RegisterTestHost(bool user_level) {
       temp_dir_.GetPath()));
 #endif
 
+  base::CopyFile(test_user_data_dir.AppendASCII("echo.py"),
+                 temp_dir_.GetPath().AppendASCII("echo.py"));
+#if defined(OS_WIN)
+  base::FilePath host_path = temp_dir_.GetPath().AppendASCII("echo.bat");
+  base::CopyFile(test_user_data_dir.AppendASCII("echo.bat"), host_path);
+#endif
+
 #if defined(OS_POSIX)
-  base::FilePath host_path = test_user_data_dir.AppendASCII("echo.py");
-#else
-  base::FilePath host_path = test_user_data_dir.AppendASCII("echo.bat");
+  base::FilePath host_path = temp_dir_.GetPath().AppendASCII("echo.py");
+  ASSERT_TRUE(base::SetPosixFilePermissions(
+      host_path, base::FILE_PERMISSION_READ_BY_USER |
+                     base::FILE_PERMISSION_WRITE_BY_USER |
+                     base::FILE_PERMISSION_EXECUTE_BY_USER));
 #endif
   ASSERT_NO_FATAL_FAILURE(WriteTestNativeHostManifest(
       temp_dir_.GetPath(), kHostName, host_path, user_level, false));

chrome/browser/extensions/api/messaging/native_messaging_test_util.h

@@ -38,6 +38,8 @@ class ScopedTestNativeMessagingHost {
 
   void RegisterTestHost(bool user_level);
 
+  const base::FilePath& temp_dir() { return temp_dir_.GetPath(); }
+
  private:
   base::ScopedTempDir temp_dir_;
 

chrome/browser/extensions/api/messaging/native_process_launcher.cc

@@ -49,7 +49,8 @@ class NativeProcessLauncherImpl : public NativeProcessLauncher {
                             intptr_t native_window,
                             const base::FilePath& profile_directory,
                             bool require_native_initiated_connections,
-                            const std::string& connect_id);
+                            const std::string& connect_id,
+                            const std::string& error_arg);
   ~NativeProcessLauncherImpl() override;
 
   void Launch(const GURL& origin,
@@ -63,7 +64,8 @@ class NativeProcessLauncherImpl : public NativeProcessLauncher {
          intptr_t native_window,
          const base::FilePath& profile_directory,
          bool require_native_initiated_connections,
-         const std::string& connect_id);
+         const std::string& connect_id,
+         const std::string& error_arg);
     void Launch(const GURL& origin,
                 const std::string& native_host_name,
                 const LaunchedCallback& callback);
@@ -96,6 +98,8 @@ class NativeProcessLauncherImpl : public NativeProcessLauncher {
     const bool require_native_initiated_connections_;
 
     const std::string connect_id_;
+    const std::string error_arg_;
+
 #if defined(OS_WIN)
     // Handle of the native window corresponding to the extension.
     intptr_t window_handle_;
@@ -113,13 +117,15 @@ NativeProcessLauncherImpl::Core::Core(bool allow_user_level_hosts,
                                       intptr_t window_handle,
                                       const base::FilePath& profile_directory,
                                       bool require_native_initiated_connections,
-                                      const std::string& connect_id)
+                                      const std::string& connect_id,
+                                      const std::string& error_arg)
     : detached_(false),
       allow_user_level_hosts_(allow_user_level_hosts),
       profile_directory_(profile_directory),
       require_native_initiated_connections_(
           require_native_initiated_connections),
-      connect_id_(connect_id)
+      connect_id_(connect_id),
+      error_arg_(error_arg)
 #if defined(OS_WIN)
       ,
       window_handle_(window_handle)
@@ -234,8 +240,13 @@ void NativeProcessLauncherImpl::Core::DoLaunchOnThreadPool(
       base::StringPrintf("--parent-window=%" PRIdPTR, window_handle_));
 #endif  // !defined(OS_WIN)
 
-  if (manifest->supports_native_initiated_connections() &&
-      !profile_directory_.empty()) {
+  bool send_connect_id = false;
+  if (!error_arg_.empty()) {
+    send_connect_id = true;
+    command_line.AppendArg(error_arg_);
+  } else if (manifest->supports_native_initiated_connections() &&
+             !profile_directory_.empty()) {
+    send_connect_id = true;
     base::FilePath exe_path;
     base::PathService::Get(base::FILE_EXE, &exe_path);
 
@@ -267,11 +278,11 @@ void NativeProcessLauncherImpl::Core::DoLaunchOnThreadPool(
     base::Base64Encode(encoded_reconnect_command, &encoded_reconnect_command);
     command_line.AppendArg(
         base::StrCat({"--reconnect-command=", encoded_reconnect_command}));
+  }
 
-    if (!connect_id_.empty()) {
-      command_line.AppendArg(base::StrCat(
-          {"--", switches::kNativeMessagingConnectId, "=", connect_id_}));
-    }
+  if (send_connect_id && !connect_id_.empty()) {
+    command_line.AppendArg(base::StrCat(
+        {"--", switches::kNativeMessagingConnectId, "=", connect_id_}));
   }
 
   base::Process process;
@@ -327,12 +338,14 @@ NativeProcessLauncherImpl::NativeProcessLauncherImpl(
     intptr_t window_handle,
     const base::FilePath& profile_directory,
     bool require_native_initiated_connections,
-    const std::string& connect_id)
+    const std::string& connect_id,
+    const std::string& error_arg)
     : core_(base::MakeRefCounted<Core>(allow_user_level_hosts,
                                        window_handle,
                                        profile_directory,
                                        require_native_initiated_connections,
-                                       connect_id)) {}
+                                       connect_id,
+                                       error_arg)) {}
 
 NativeProcessLauncherImpl::~NativeProcessLauncherImpl() {
   core_->Detach();
@@ -352,7 +365,8 @@ std::unique_ptr<NativeProcessLauncher> NativeProcessLauncher::CreateDefault(
     gfx::NativeView native_view,
     const base::FilePath& profile_directory,
     bool require_native_initiated_connections,
-    const std::string& connect_id) {
+    const std::string& connect_id,
+    const std::string& error_arg) {
   intptr_t window_handle = 0;
 #if defined(OS_WIN)
   window_handle = reinterpret_cast<intptr_t>(
@@ -360,7 +374,7 @@ std::unique_ptr<NativeProcessLauncher> NativeProcessLauncher::CreateDefault(
 #endif
   return std::make_unique<NativeProcessLauncherImpl>(
       allow_user_level_hosts, window_handle, profile_directory,
-      require_native_initiated_connections, connect_id);
+      require_native_initiated_connections, connect_id, error_arg);
 }
 
 }  // namespace extensions

chrome/browser/extensions/api/messaging/native_process_launcher.h

@@ -47,12 +47,15 @@ class NativeProcessLauncher {
   // the host. If |require_native_initiated_connections| is true, the connection
   // will be allowed only if the native messaging host sets
   // "supports_native_initiated_connections" to true in its manifest.
+  // If |error_arg| is non-empty, the reconnect args are omitted, and instead
+  // the error value is passed as a command line argument to the host.
   static std::unique_ptr<NativeProcessLauncher> CreateDefault(
       bool allow_user_level_hosts,
       gfx::NativeView native_view,
       const base::FilePath& profile_directory,
       bool require_native_initiated_connections,
-      const std::string& connect_id);
+      const std::string& connect_id,
+      const std::string& error_arg);
 
   NativeProcessLauncher() {}
   virtual ~NativeProcessLauncher() {}

chrome/browser/extensions/service_worker_messaging_apitest.cc

@@ -19,6 +19,9 @@ class ServiceWorkerMessagingTest : public ExtensionApiTest {
   ServiceWorkerMessagingTest() = default;
   ~ServiceWorkerMessagingTest() override = default;
 
+ protected:
+  extensions::ScopedTestNativeMessagingHost test_host_;
+
  private:
   ScopedWorkerBasedExtensionsChannel current_channel_;
 
@@ -76,8 +79,7 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerMessagingTest, TabToWorker) {
 // Tests chrome.runtime.sendNativeMessage from SW extension to a native
 // messaging host.
 IN_PROC_BROWSER_TEST_F(ServiceWorkerMessagingTest, NativeMessagingBasic) {
-  extensions::ScopedTestNativeMessagingHost test_host;
-  ASSERT_NO_FATAL_FAILURE(test_host.RegisterTestHost(false));
+  ASSERT_NO_FATAL_FAILURE(test_host_.RegisterTestHost(false));
   ASSERT_TRUE(RunExtensionTest("service_worker/messaging/send_native_message"))
       << message_;
 }
@@ -85,8 +87,7 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerMessagingTest, NativeMessagingBasic) {
 // Tests chrome.runtime.connectNative from SW extension to a native messaging
 // host.
 IN_PROC_BROWSER_TEST_F(ServiceWorkerMessagingTest, ConnectNative) {
-  extensions::ScopedTestNativeMessagingHost test_host;
-  ASSERT_NO_FATAL_FAILURE(test_host.RegisterTestHost(false));
+  ASSERT_NO_FATAL_FAILURE(test_host_.RegisterTestHost(false));
   ASSERT_TRUE(RunExtensionTest("service_worker/messaging/connect_native"))
       << message_;
 }

chrome/test/data/native_messaging/native_hosts/echo.py

@@ -13,6 +13,7 @@ import os
 import platform
 import sys
 import struct
+import time
 
 def WriteMessage(message):
   try:
@@ -29,6 +30,10 @@ def ParseArgs():
   parser.add_argument('--parent-window', type=int)
   parser.add_argument('--reconnect-command')
   parser.add_argument('--native-messaging-connect-id')
+  parser.add_argument('--extension-not-installed', action='store_true',
+                      default=False)
+  parser.add_argument('--invalid-connect-id', action='store_true',
+                      default=False)
   parser.add_argument('origin')
   return parser.parse_args()
 
@@ -43,6 +48,25 @@ def Main():
         "URL of the calling application is not specified as the first arg.\n")
     return 1
 
+  if args.extension_not_installed:
+    with open('connect_id.txt', 'w') as f:
+      if args.reconnect_command:
+        f.write('Unexpected reconnect command: ' + args.reconnect_command)
+      else:
+        f.write('--connect-id=' + args.native_messaging_connect_id)
+    # The timeout in the test is 2 seconds, so sleep for longer than that to
+    # force a timeout.
+    time.sleep(5)
+    return 1
+
+  if args.invalid_connect_id:
+    with open('invalid_connect_id.txt', 'w') as f:
+      if args.reconnect_command:
+        f.write('Unexpected reconnect command: ' + args.reconnect_command)
+      else:
+        f.write('--invalid-connect-id')
+    return 1
+
   # Verify that the process was started in the correct directory.
   cwd = os.getcwd()
   script_path = os.path.dirname(os.path.abspath(sys.argv[0]))

components/keep_alive_registry/keep_alive_types.cc

@@ -51,6 +51,8 @@ std::ostream& operator<<(std::ostream& out, const KeepAliveOrigin& origin) {
       return out << "USER_MANAGER_VIEW";
     case KeepAliveOrigin::CREDENTIAL_PROVIDER_SIGNIN_DIALOG:
       return out << "CREDENTIAL_PROVIDER_SIGNIN_DIALOG";
+    case KeepAliveOrigin::NATIVE_MESSAGING_HOST_ERROR_REPORT:
+      return out << "NATIVE_MESSAGING_HOST_ERROR_REPORT";
   }
 
   NOTREACHED();

components/keep_alive_registry/keep_alive_types.h

@@ -31,6 +31,9 @@ enum class KeepAliveOrigin {
   LOGIN_DISPLAY_HOST_WEBUI,
   PIN_MIGRATION,
 
+  // c/b/extensions
+  NATIVE_MESSAGING_HOST_ERROR_REPORT,
+
   // c/b/notifications
   NOTIFICATION,
   PENDING_NOTIFICATION_CLICK_EVENT,