chore: Show FIDO devices in the chooser if allowed (#40216)

* chore: Show FIDO devices in the chooser if allowed

* chore: tweak HidChooserContext::IsFidoAllowedForOrigin

* chore: feedback from review

---------

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

shell/browser/hid/electron_hid_delegate.cc

@@ -29,6 +29,8 @@ namespace {
 
 electron::HidChooserContext* GetChooserContext(
     content::BrowserContext* browser_context) {
+  if (!browser_context)
+    return nullptr;
   return electron::HidChooserContextFactory::GetForBrowserContext(
       browser_context);
 }
@@ -110,6 +112,7 @@ std::unique_ptr<content::HidChooser> ElectronHidDelegate::RunChooser(
     std::vector<blink::mojom::HidDeviceFilterPtr> filters,
     std::vector<blink::mojom::HidDeviceFilterPtr> exclusion_filters,
     content::HidChooser::Callback callback) {
+  DCHECK(render_frame_host);
   auto* browser_context = render_frame_host->GetBrowserContext();
 
   // Start observing HidChooserContext for permission and device events.
@@ -133,6 +136,9 @@ std::unique_ptr<content::HidChooser> ElectronHidDelegate::RunChooser(
 bool ElectronHidDelegate::CanRequestDevicePermission(
     content::BrowserContext* browser_context,
     const url::Origin& origin) {
+  if (!browser_context)
+    return false;
+
   base::Value::Dict details;
   details.Set("securityOrigin", origin.GetURL().spec());
   auto* permission_manager = static_cast<ElectronPermissionManager*>(
@@ -147,31 +153,38 @@ bool ElectronHidDelegate::HasDevicePermission(
     content::BrowserContext* browser_context,
     const url::Origin& origin,
     const device::mojom::HidDeviceInfo& device) {
-  return GetChooserContext(browser_context)
-      ->HasDevicePermission(origin, device);
+  return browser_context && GetChooserContext(browser_context)
+                                ->HasDevicePermission(origin, device);
 }
 
 void ElectronHidDelegate::RevokeDevicePermission(
     content::BrowserContext* browser_context,
     const url::Origin& origin,
     const device::mojom::HidDeviceInfo& device) {
-  return GetChooserContext(browser_context)
-      ->RevokeDevicePermission(origin, device);
+  if (browser_context) {
+    GetChooserContext(browser_context)->RevokeDevicePermission(origin, device);
+  }
 }
 
 device::mojom::HidManager* ElectronHidDelegate::GetHidManager(
     content::BrowserContext* browser_context) {
+  if (!browser_context)
+    return nullptr;
   return GetChooserContext(browser_context)->GetHidManager();
 }
 
 void ElectronHidDelegate::AddObserver(content::BrowserContext* browser_context,
                                       Observer* observer) {
+  if (!browser_context)
+    return;
   GetContextObserver(browser_context)->AddObserver(observer);
 }
 
 void ElectronHidDelegate::RemoveObserver(
     content::BrowserContext* browser_context,
     content::HidDelegate::Observer* observer) {
+  if (!browser_context)
+    return;
   DCHECK(base::Contains(observations_, browser_context));
   GetContextObserver(browser_context)->RemoveObserver(observer);
 }
@@ -180,14 +193,16 @@ const device::mojom::HidDeviceInfo* ElectronHidDelegate::GetDeviceInfo(
     content::BrowserContext* browser_context,
     const std::string& guid) {
   auto* chooser_context = GetChooserContext(browser_context);
+  if (!chooser_context)
+    return nullptr;
   return chooser_context->GetDeviceInfo(guid);
 }
 
 bool ElectronHidDelegate::IsFidoAllowedForOrigin(
     content::BrowserContext* browser_context,
     const url::Origin& origin) {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kDisableHidBlocklist);
+  auto* chooser_context = GetChooserContext(browser_context);
+  return chooser_context && chooser_context->IsFidoAllowedForOrigin(origin);
 }
 
 bool ElectronHidDelegate::IsServiceWorkerAllowedForOrigin(

shell/browser/hid/hid_chooser_context.cc

@@ -29,9 +29,14 @@
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "third_party/blink/public/common/permissions/permission_utils.h"
-
 #include "ui/base/l10n/l10n_util.h"
 
+#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
+#include "base/containers/fixed_flat_set.h"
+#include "base/strings/string_piece.h"
+#include "extensions/common/constants.h"
+#endif  // BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
+
 namespace electron {
 
 const char kHidDeviceNameKey[] = "name";
@@ -181,6 +186,26 @@ bool HidChooserContext::HasDevicePermission(
       origin, DeviceInfoToValue(device), browser_context_);
 }
 
+bool HidChooserContext::IsFidoAllowedForOrigin(const url::Origin& origin) {
+#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
+  static constexpr auto kPrivilegedExtensionIds =
+      base::MakeFixedFlatSet<base::StringPiece>({
+          "ckcendljdlmgnhghiaomidhiiclmapok",  // gnubbyd-v3 dev
+          "lfboplenmmjcmpbkeemecobbadnmpfhi",  // gnubbyd-v3 prod
+      });
+
+  if (origin.scheme() == extensions::kExtensionScheme &&
+      base::Contains(kPrivilegedExtensionIds, origin.host())) {
+    return true;
+  }
+#endif  // BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
+
+  // This differs from upstream - we want to allow users greater
+  // ability to communicate with FIDO devices in Electron.
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kDisableHidBlocklist);
+}
+
 void HidChooserContext::AddDeviceObserver(DeviceObserver* observer) {
   EnsureHidManagerConnection();
   device_observer_list_.AddObserver(observer);

shell/browser/hid/hid_chooser_context.h

@@ -78,6 +78,9 @@ class HidChooserContext : public KeyedService,
   bool HasDevicePermission(const url::Origin& origin,
                            const device::mojom::HidDeviceInfo& device);
 
+  // Returns true if `origin` is allowed to access FIDO reports.
+  bool IsFidoAllowedForOrigin(const url::Origin& origin);
+
   // For ScopedObserver.
   void AddDeviceObserver(DeviceObserver* observer);
   void RemoveDeviceObserver(DeviceObserver* observer);

shell/browser/hid/hid_chooser_controller.cc

@@ -85,12 +85,16 @@ HidChooserController::HidChooserController(
       filters_(std::move(filters)),
       exclusion_filters_(std::move(exclusion_filters)),
       callback_(std::move(callback)),
+      initiator_document_(render_frame_host->GetWeakDocumentPtr()),
       origin_(content::WebContents::FromRenderFrameHost(render_frame_host)
                   ->GetPrimaryMainFrame()
                   ->GetLastCommittedOrigin()),
-      frame_tree_node_id_(render_frame_host->GetFrameTreeNodeId()),
       hid_delegate_(hid_delegate),
       render_frame_host_id_(render_frame_host->GetGlobalId()) {
+  // The use above of GetMainFrame is safe as content::HidService instances are
+  // not created for fenced frames.
+  DCHECK(!render_frame_host->IsNestedWithinFencedFrame());
+
   chooser_context_ = HidChooserContextFactory::GetForBrowserContext(
                          web_contents->GetBrowserContext())
                          ->AsWeakPtr();
@@ -129,6 +133,7 @@ void HidChooserController::OnDeviceAdded(
     const device::mojom::HidDeviceInfo& device) {
   if (!DisplayDevice(device))
     return;
+
   if (AddDeviceInfo(device)) {
     api::Session* session = GetSession();
     if (session) {
@@ -142,8 +147,6 @@ void HidChooserController::OnDeviceAdded(
       session->Emit("hid-device-added", details);
     }
   }
-
-  return;
 }
 
 void HidChooserController::OnDeviceRemoved(
@@ -151,7 +154,8 @@ void HidChooserController::OnDeviceRemoved(
   if (!base::Contains(items_, PhysicalDeviceIdFromDeviceInfo(device)))
     return;
 
-  if (api::Session* session = GetSession(); session != nullptr) {
+  api::Session* session = GetSession();
+  if (session) {
     auto* rfh = content::RenderFrameHost::FromID(render_frame_host_id_);
     v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
     v8::HandleScope scope(isolate);
@@ -223,9 +227,8 @@ void HidChooserController::OnGotDevices(
 
   for (auto& device : devices) {
     if (DisplayDevice(*device)) {
-      if (AddDeviceInfo(*device)) {
+      if (AddDeviceInfo(*device))
         devicesToDisplay.push_back(device->Clone());
-      }
     }
   }
 
@@ -233,6 +236,7 @@ void HidChooserController::OnGotDevices(
   // enumeration.
   if (chooser_context_)
     observation_.Observe(chooser_context_.get());
+
   bool prevent_default = false;
   api::Session* session = GetSession();
   if (session) {
@@ -255,26 +259,41 @@ void HidChooserController::OnGotDevices(
 
 bool HidChooserController::DisplayDevice(
     const device::mojom::HidDeviceInfo& device) const {
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableHidBlocklist)) {
-    // Do not pass the device to the chooser if it is excluded by the blocklist.
-    if (device.is_excluded_by_blocklist)
-      return false;
+  // Check if `device` has a top-level collection with a FIDO usage. FIDO
+  // devices may be displayed if the origin is privileged or the blocklist is
+  // disabled.
+  const bool has_fido_collection =
+      base::Contains(device.collections, device::mojom::kPageFido,
+                     [](const auto& c) { return c->usage->usage_page; });
 
-    // Do not pass the device to the chooser if it has a top-level collection
-    // with the FIDO usage page.
-    //
-    // Note: The HID blocklist also blocks top-level collections with the FIDO
-    // usage page, but will not block the device if it has other (non-FIDO)
-    // collections. The check below will exclude the device from the chooser
-    // if it has any top-level FIDO collection.
-    auto find_it =
-        std::find_if(device.collections.begin(), device.collections.end(),
-                     [](const device::mojom::HidCollectionInfoPtr& c) {
-                       return c->usage->usage_page == device::mojom::kPageFido;
-                     });
-    if (find_it != device.collections.end())
-      return false;
+  if (has_fido_collection) {
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kDisableHidBlocklist) ||
+        (chooser_context_ &&
+         chooser_context_->IsFidoAllowedForOrigin(origin_))) {
+      return FilterMatchesAny(device) && !IsExcluded(device);
+    }
+
+    AddMessageToConsole(
+        blink::mojom::ConsoleMessageLevel::kInfo,
+        base::StringPrintf(
+            "Chooser dialog is not displaying a FIDO HID device: vendorId=%d, "
+            "productId=%d, name='%s', serial='%s'",
+            device.vendor_id, device.product_id, device.product_name.c_str(),
+            device.serial_number.c_str()));
+    return false;
+  }
+
+  if (device.is_excluded_by_blocklist) {
+    AddMessageToConsole(
+        blink::mojom::ConsoleMessageLevel::kInfo,
+        base::StringPrintf(
+            "Chooser dialog is not displaying a device excluded by "
+            "the HID blocklist: vendorId=%d, "
+            "productId=%d, name='%s', serial='%s'",
+            device.vendor_id, device.product_id, device.product_name.c_str(),
+            device.serial_number.c_str()));
+    return false;
   }
 
   return FilterMatchesAny(device) && !IsExcluded(device);
@@ -303,6 +322,15 @@ bool HidChooserController::IsExcluded(
   return false;
 }
 
+void HidChooserController::AddMessageToConsole(
+    blink::mojom::ConsoleMessageLevel level,
+    const std::string& message) const {
+  if (content::RenderFrameHost* rfh =
+          initiator_document_.AsRenderFrameHostIfValid()) {
+    rfh->AddMessageToConsole(level, message);
+  }
+}
+
 bool HidChooserController::AddDeviceInfo(
     const device::mojom::HidDeviceInfo& device) {
   const auto& id = PhysicalDeviceIdFromDeviceInfo(device);
@@ -340,10 +368,8 @@ void HidChooserController::UpdateDeviceInfo(
   auto physical_device_it = device_map_.find(id);
   DCHECK(physical_device_it != device_map_.end());
   auto& device_infos = physical_device_it->second;
-  auto device_it = base::ranges::find_if(
-      device_infos, [&device](const device::mojom::HidDeviceInfoPtr& d) {
-        return d->guid == device.guid;
-      });
+  auto device_it = base::ranges::find(device_infos, device.guid,
+                                      &device::mojom::HidDeviceInfo::guid);
   DCHECK(device_it != device_infos.end());
   *device_it = device.Clone();
 }

shell/browser/hid/hid_chooser_controller.h

@@ -13,6 +13,7 @@
 #include "base/scoped_observation.h"
 #include "content/public/browser/global_routing_id.h"
 #include "content/public/browser/hid_chooser.h"
+#include "content/public/browser/weak_document_ptr.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "services/device/public/mojom/hid.mojom-forward.h"
@@ -20,6 +21,7 @@
 #include "shell/browser/hid/electron_hid_delegate.h"
 #include "shell/browser/hid/hid_chooser_context.h"
 #include "shell/common/gin_converters/frame_converter.h"
+#include "third_party/blink/public/mojom/devtools/console_message.mojom.h"
 #include "third_party/blink/public/mojom/hid/hid.mojom.h"
 
 namespace content {
@@ -76,6 +78,8 @@ class HidChooserController
   bool DisplayDevice(const device::mojom::HidDeviceInfo& device) const;
   bool FilterMatchesAny(const device::mojom::HidDeviceInfo& device) const;
   bool IsExcluded(const device::mojom::HidDeviceInfo& device) const;
+  void AddMessageToConsole(blink::mojom::ConsoleMessageLevel level,
+                           const std::string& message) const;
 
   // Add |device_info| to |device_map_|. The device is added to the chooser item
   // representing the physical device. If the chooser item does not yet exist, a
@@ -98,8 +102,8 @@ class HidChooserController
   std::vector<blink::mojom::HidDeviceFilterPtr> filters_;
   std::vector<blink::mojom::HidDeviceFilterPtr> exclusion_filters_;
   content::HidChooser::Callback callback_;
+  content::WeakDocumentPtr initiator_document_;
   const url::Origin origin_;
-  const int frame_tree_node_id_;
 
   // The lifetime of the chooser context is tied to the browser context used to
   // create it, and may be destroyed while the chooser is still active.