From 39567c358b851a3a998e74cae571c2292c822967 Mon Sep 17 00:00:00 2001
From: Tom Anderson <[email protected]>
Date: Fri, 15 May 2026 17:45:28 -0700
Subject: [PATCH] Fix potential shortcut hijacking in Global Shortcuts portal

When using an external registration portal (like XDG Global Shortcuts on
Linux), Chrome was forwarding all manifest-defined commands as suggested
triggers, even if they were internally rejected (e.g. reserved browser
shortcuts like Ctrl+T).

Ensure that all global commands are still forwarded to the system portal
to allow manual user assignment, but their suggested accelerators are
cleared if they are not ACTIVE and GLOBAL. This prevents malicious
extensions from suggesting hijacks of reserved keys without breaking the
ability for users to manually bind them.

Fixed: 513226551

Change-Id: I0bf7719c671588f00fcf3c0732f296c7ac8c5979
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7851299
Reviewed-by: Lei Zhang <[email protected]>
Commit-Queue: Thomas Anderson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1631678}
---

diff --git a/chrome/browser/extensions/extension_commands_global_registry.cc b/chrome/browser/extensions/extension_commands_global_registry.cc
index aa67a2f..cd22521f 100644
--- a/chrome/browser/extensions/extension_commands_global_registry.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry.cc
@@ -138,34 +138,53 @@
 
   extensions::CommandService* command_service =
       extensions::CommandService::Get(browser_context_);
-  if (instance->IsRegistrationHandledExternally()) {
-    if (!command_service->GetNamedCommands(
-            extension->id(), extensions::CommandService::ALL,
-            extensions::CommandService::ANY_SCOPE, commands)) {
-      return false;
-    }
-    PrefService* prefs = user_prefs::UserPrefs::Get(browser_context_);
-    std::string profile_id = prefs->GetString(pref_names::kGlobalShortcutsUuid);
-    if (profile_id.empty()) {
-      auto uuid = base::Uuid::GenerateRandomV4();
-      profile_id = uuid.AsLowercaseString();
-      prefs->SetString(pref_names::kGlobalShortcutsUuid, profile_id);
-    }
 
-    instance->OnCommandsChanged(
-        extension->id(), profile_id, *commands,
-        GetAcceleratedWidgetForContext(browser_context_),
-        base::BindRepeating(&ExtensionCommandsGlobalRegistry::ExecuteCommand,
-                            weak_ptr_factory_.GetWeakPtr()));
+  if (!instance->IsRegistrationHandledExternally()) {
+    // Add all the active global keybindings, if any.
+    return command_service->GetNamedCommands(
+        extension->id(), extensions::CommandService::ACTIVE,
+        extensions::CommandService::GLOBAL, commands);
   }
 
-  // Add all the active global keybindings, if any.
+  // All commands should be sent to the portal so the user can manually
+  // assign them, but they must be sanitized first to prevent hijacking
+  // reserved shortcuts.
+  ui::CommandMap all_commands;
   if (!command_service->GetNamedCommands(
-          extension->id(), extensions::CommandService::ACTIVE,
-          extensions::CommandService::GLOBAL, commands)) {
+          extension->id(), extensions::CommandService::ALL,
+          extensions::CommandService::ANY_SCOPE, &all_commands)) {
     return false;
   }
-  return true;
+
+  ui::CommandMap& active_global_commands = *commands;
+  active_global_commands.clear();
+  // Do not check the return value. An empty map is expected if the extension
+  // only has rejected global commands or regular non-global commands.
+  command_service->GetNamedCommands(
+      extension->id(), extensions::CommandService::ACTIVE,
+      extensions::CommandService::GLOBAL, &active_global_commands);
+
+  for (auto& cmd_pair : all_commands) {
+    if (!active_global_commands.contains(cmd_pair.first)) {
+      cmd_pair.second.set_accelerator(ui::Accelerator());
+    }
+  }
+
+  PrefService* prefs = user_prefs::UserPrefs::Get(browser_context_);
+  std::string profile_id = prefs->GetString(pref_names::kGlobalShortcutsUuid);
+  if (profile_id.empty()) {
+    auto uuid = base::Uuid::GenerateRandomV4();
+    profile_id = uuid.AsLowercaseString();
+    prefs->SetString(pref_names::kGlobalShortcutsUuid, profile_id);
+  }
+
+  instance->OnCommandsChanged(
+      extension->id(), profile_id, all_commands,
+      GetAcceleratedWidgetForContext(browser_context_),
+      base::BindRepeating(&ExtensionCommandsGlobalRegistry::ExecuteCommand,
+                          weak_ptr_factory_.GetWeakPtr()));
+
+  return !active_global_commands.empty();
 }
 
 bool ExtensionCommandsGlobalRegistry::RegisterAccelerator(
diff --git a/chrome/browser/extensions/extension_commands_global_registry_unittest.cc b/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
index 59ca23a3..ec83474 100644
--- a/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
@@ -9,6 +9,7 @@
 #include <string_view>
 
 #include "base/functional/callback.h"
+#include "build/build_config.h"
 #include "chrome/browser/extensions/commands/command_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "extensions/browser/extension_registry.h"
@@ -45,6 +46,25 @@
                 "description": "regular"
               }
             })";
+constexpr std::string_view kManifestWithUnsanitizedShortcuts = R"(
+            "manifest_version": 3,
+            "version": "1",
+            "commands": {
+              "legit_global_shortcut": {
+                "suggested_key": { "default": "Ctrl+Shift+5" },
+                "description": "legit global",
+                "global": true
+              },
+              "legit_regular_shortcut": {
+                "suggested_key": { "default": "Ctrl+Shift+6" },
+                "description": "legit regular"
+              },
+              "hijack_ctrl_t": {
+                "suggested_key": { "default": "Ctrl+T" },
+                "description": "hijack",
+                "global": true
+              }
+            })";
 
 class FakeGlobalAcceleratorListener : public ui::GlobalAcceleratorListener {
  public:
@@ -74,12 +94,13 @@
 
   void PruneStaleCommands() override {}
 
-  void OnCommandsChanged(const std::string&,
-                         const std::string&,
-                         const ui::CommandMap&,
-                         gfx::AcceleratedWidget,
+  void OnCommandsChanged(const std::string& extension_id,
+                         const std::string& profile_id,
+                         const ui::CommandMap& commands,
+                         gfx::AcceleratedWidget widget,
                          ExecuteCommandCallback execute_command) override {
     last_execute_command_callback_ = std::move(execute_command);
+    last_commands_ = commands;
   }
 
   bool has_last_execute_command_callback() const {
@@ -90,6 +111,8 @@
     return last_execute_command_callback_;
   }
 
+  const ui::CommandMap& last_commands() const { return last_commands_; }
+
   void set_registration_handled_externally(bool value) {
     registration_handled_externally_ = value;
   }
@@ -97,6 +120,7 @@
  private:
   bool registration_handled_externally_ = false;
   ExecuteCommandCallback last_execute_command_callback_;
+  ui::CommandMap last_commands_;
 };
 
 class TestExtensionCommandsGlobalRegistry
@@ -114,12 +138,27 @@
     return global_shortcut_listener_;
   }
 
+  bool RegisterAccelerator(const ui::Accelerator& accelerator,
+                           const ExtensionId& extension_id,
+                           const std::string& command_name) override {
+    return true;
+  }
+
  private:
   raw_ptr<ui::GlobalAcceleratorListener> global_shortcut_listener_;
 };
 
 class ExtensionCommandsGlobalRegistryTest : public ExtensionServiceTestBase {
  public:
+  void InitializeExtensionService(ExtensionServiceInitParams params) override {
+    params.testing_factories.emplace_back(
+        ExtensionCommandsGlobalRegistry::GetFactoryInstance(),
+        base::BindRepeating(
+            [](content::BrowserContext* context)
+                -> std::unique_ptr<KeyedService> { return nullptr; }));

diff --git a/chrome/browser/extensions/extension_commands_global_registry_unittest.cc b/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
index 59ca23a3..ec83474 100644
--- a/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry_unittest.cc
@@ -9,6 +9,7 @@
 #include <string_view>
 
 #include "base/functional/callback.h"
+#include "build/build_config.h"
 #include "chrome/browser/extensions/commands/command_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "extensions/browser/extension_registry.h"
@@ -45,6 +46,25 @@
                 "description": "regular"
               }
             })";
+constexpr std::string_view kManifestWithUnsanitizedShortcuts = R"(
+            "manifest_version": 3,
+            "version": "1",
+            "commands": {
+              "legit_global_shortcut": {
+                "suggested_key": { "default": "Ctrl+Shift+5" },
+                "description": "legit global",
+                "global": true
+              },
+              "legit_regular_shortcut": {
+                "suggested_key": { "default": "Ctrl+Shift+6" },
+                "description": "legit regular"
+              },
+              "hijack_ctrl_t": {
+                "suggested_key": { "default": "Ctrl+T" },
+                "description": "hijack",
+                "global": true
+              }
+            })";
 
 class FakeGlobalAcceleratorListener : public ui::GlobalAcceleratorListener {
  public:
@@ -74,12 +94,13 @@
 
   void PruneStaleCommands() override {}
 
-  void OnCommandsChanged(const std::string&,
-                         const std::string&,
-                         const ui::CommandMap&,
-                         gfx::AcceleratedWidget,
+  void OnCommandsChanged(const std::string& extension_id,
+                         const std::string& profile_id,
+                         const ui::CommandMap& commands,
+                         gfx::AcceleratedWidget widget,
                          ExecuteCommandCallback execute_command) override {
     last_execute_command_callback_ = std::move(execute_command);
+    last_commands_ = commands;
   }
 
   bool has_last_execute_command_callback() const {
@@ -90,6 +111,8 @@
     return last_execute_command_callback_;
   }
 
+  const ui::CommandMap& last_commands() const { return last_commands_; }
+
   void set_registration_handled_externally(bool value) {
     registration_handled_externally_ = value;
   }
@@ -97,6 +120,7 @@
  private:
   bool registration_handled_externally_ = false;
   ExecuteCommandCallback last_execute_command_callback_;
+  ui::CommandMap last_commands_;
 };
 
 class TestExtensionCommandsGlobalRegistry
@@ -114,12 +138,27 @@
     return global_shortcut_listener_;
   }
 
+  bool RegisterAccelerator(const ui::Accelerator& accelerator,
+                           const ExtensionId& extension_id,
+                           const std::string& command_name) override {
+    return true;
+  }
+
  private:
   raw_ptr<ui::GlobalAcceleratorListener> global_shortcut_listener_;
 };
 
 class ExtensionCommandsGlobalRegistryTest : public ExtensionServiceTestBase {
  public:
+  void InitializeExtensionService(ExtensionServiceInitParams params) override {
+    params.testing_factories.emplace_back(
+        ExtensionCommandsGlobalRegistry::GetFactoryInstance(),
+        base::BindRepeating(
+            [](content::BrowserContext* context)
+                -> std::unique_ptr<KeyedService> { return nullptr; }));
+    ExtensionServiceTestBase::InitializeExtensionService(std::move(params));
+  }
+
   void SetUp() override {
     ExtensionServiceTestBase::SetUp();
     InitializeEmptyExtensionService();
@@ -230,6 +269,48 @@
   EXPECT_FALSE(listener().has_last_execute_command_callback());
 }
 
+// Tests that PopulateCommands correctly sanitizes reserved shortcuts and
+// regular shortcuts but still forwards all commands to the portal list
+// when registration is handled externally.
+TEST_F(ExtensionCommandsGlobalRegistryTest,
+       PopulateCommandsExternallyHandledSanitizesShortcuts) {
+  auto extension =
+      BuildAndEnableExtension("abcdefghijklmnopabcdefghijklmnop",
+                              std::string(kManifestWithUnsanitizedShortcuts));
+  ASSERT_TRUE(extension);
+
+  EnableExternalHandlingAndUpdate(extension.get());
+
+  ASSERT_TRUE(listener().has_last_execute_command_callback());
+  const ui::CommandMap& commands = listener().last_commands();
+
+  // "legit_global_shortcut" should be present with its accelerator because
+  // it's global and not reserved.
+  auto it_legit_global = commands.find("legit_global_shortcut");
+  ASSERT_NE(it_legit_global, commands.end());
+  EXPECT_EQ(ui::VKEY_5, it_legit_global->second.accelerator().key_code());
+#if BUILDFLAG(IS_MAC)
+  EXPECT_TRUE(it_legit_global->second.accelerator().IsCmdDown());
+#else
+  EXPECT_TRUE(it_legit_global->second.accelerator().IsCtrlDown());
+#endif
+  EXPECT_TRUE(it_legit_global->second.accelerator().IsShiftDown());
+
+  // "legit_regular_shortcut" should be present (for manual assignment) but its
+  // accelerator must be cleared because it's not a global shortcut and
+  // should not be suggested to the global portal.
+  auto it_legit_regular = commands.find("legit_regular_shortcut");
+  ASSERT_NE(it_legit_regular, commands.end());
+  EXPECT_EQ(ui::VKEY_UNKNOWN,
+            it_legit_regular->second.accelerator().key_code());
+
+  // "hijack_ctrl_t" should be present (for manual assignment) but its
+  // accelerator must be cleared because it's a reserved shortcut.
+  auto it_hijack = commands.find("hijack_ctrl_t");
+  ASSERT_NE(it_hijack, commands.end());
+  EXPECT_EQ(ui::VKEY_UNKNOWN, it_hijack->second.accelerator().key_code());
+}
+
 }  // namespace
 
 }  // namespace extensions