feat: allow access to protected usb classes from webusb

[penx]

Description of Change

Allow access to all USB devices.

Closes discussion at #14545 (comment)

Why?

By default, Chromium limits access to protected interface classes.

This was announced here https://groups.google.com/a/chromium.org/g/blink-dev/c/LZXocaeCwDw/m/GLfAffGLAAAJ

The reasoning for this restriction is related to web applications, so as far as I'm aware does not apply to Electron.

I personally need to access protected devices as I am using Electron to build a mixer interface for some USB audio hardware, where the official software has been discontinued and does not work on newer operating systems.

Electron applications can already access protected USB devices via node-usb (which uses libusb), so as far as I'm aware, preventing access to these protected interfaces provides no additional security benefits.

edit: I have since realised that node-usb requires access to node apis that, in newer versions of Electron, are not available from the renderer. Perhaps the same restriction is needed here, or an option to make this feature opt-in.

This change will allow me to use the WebUSB API, removing my dependency on node-usb and libusb and replacing them with standard APIs.

How?

A recent Chromium change means the UsbDelegate interface has an AdjustProtectedInterfaceClasses which, as far as I'm aware, was intentionally added to allow frameworks like Electron to override the protected classes.

This is backed up by @7ombie and @reillyeon's comments here.

How will access to protected interface classes work? It seems ok to ignore that aspect of the spec entirely.

I have a work-in-progress Chromium change which does the refactoring necessary for //content embedders like Electron to support WebUSB. In that change the new UsbDelegate interface has an AdjustProtectedInterfaceClasses method which Electron can override to control how that part of the specification is implemented

ElectronUsbDelegate::AdjustProtectedInterfaceClasses() just seems like a verbatim copy of the version from ChromeUsbDelegate. It'll need to be updated to either give developers control or just clear the list of protected interface classes entirely

This change modifies Electron's AdjustProtectedInterfaceClasses method to clear all protected classes.

Note I don't have a lot of C experience, nor do I know the Electron code base, the but this minimal change seemed ok to me, so I thought I would propose it. If something more advanced is needed such as giving developers control to reset it, I would be keen if someone who understands C and Electron picks this up :)

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation, tutorials, templates and examples are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Allow access to protected USB classes when using the WebUSB API.

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[codebytere]

Can you please elaborate on your intended goal and use case for this change? You've not provided a PR description nor release notes. Without those as well as further explanation we cannot further review this PR.

[penx]

@codebytere apologies, I was intending to revisit to add these, I should probably have put this in draft :) I've added a description and release notes now.

[penx]

@codebytere would you like me to create an issue/feature request for what I'm trying to achieve here? May be easier to track that way, as perhaps something more complex is needed than my proposed change here.

[jkleinsc]

LGTM.
Thanks for the PR @penx!

[codebytere]

No worries @penx, and thanks for the follow-up. I'm also good with this change given that context (at least personally i'm not familiar enough with the intricacies of this specific code to be able to review without a better understanding of motivation/background) and appreciate the write-up!

LGTM.

[reillyeon]

My only concern here is that this means that any content using the WebUSB API will no longer be constrained by the protected interface class list. I'm not familiar with how Electron considers such security issues but it might be something that developers should have to opt into.

[dsanders11]

I think this should be opt-in, rather than the default for renderers. Feels like a security footgun to disable this security consideration by default.

EDIT: @reillyeon beat me by a few minutes. 🙂

[jkleinsc]

@reillyeon and @dsanders11 make a good point. Instead of automatically clearing the protected classes, we should add a permission handler that lets developers opt in to allowing these protected classes.

[jkleinsc]

For more clarification on my comment above we should add a handler like this to Session:

ses.setUSBProtectedClassesHandler(handler)

• handler Function<string[]> | null
  • details Object
    • protectedClasses string[] - The current list of protected USB classes. Possible class values are:
      • audio
      • hid
      • mass_storage
      • smart_card
      • video
      • audio_video
      • wireless

Sets the handler which can be used to override which USB classes are protected.
Returning an empty string array from the handler will allow all USB classes; returning the passed in array will maintain the default list of protected USB classes (this is also the default behavior if a handler is not defined).
To clear the handler, call setUSBProtectedClassesHandler(null).

const { app, BrowserWindow } = require('electron');

let win = null;

app.whenReady().then(() => {
  win = new BrowserWindow();

  win.webContents.session.setUSBProtectedClassesHandler((details) => {
    // Allow all classes
    return [];
    // Keep the current set of protected classes
    return details.protectedClasses;
    // Selectively remove classes
    return details.protectedClasses.filter((usbClass) => {
      //Allow any class that is audio
      return usbClass.indexOf('audio') > -1;
    });
  });
});

[penx]

I agree that it should be opt in. At the point I made this change I thought Electron had full access via node-usb but later read about sandboxing and restricted access to node APIs, so agree this could open up a security issue and should be opt in.

I could try to make the recommended changes myself, however:

• I would likely need assistance as I don't have much C experience nor experience with Electron
• I am using Electron for a personal project, not commercially as part of my job, so will need to be when I can find spare time!

If anyone is able to pick this up, please shout, otherwise I'll have a go at it following @jkleinsc 's advice above when I next get a chance.

win.webContents.session.setUSBProtectedClassesHandler

@jkleinsc would setUSBProtectedClassesHandler only be accessible from main? Or could it be called from a renderer too? If the latter, doesn't this mean the security issue still exists?

[jkleinsc]

@penx I am going to close this PR as it stands. If you or someone else wants to implement the handler, that can be done in a new PR.

In regards to your question, that new handler lives on session which by default is only available in the main process, so renderer code could not arbitrarily call the setting of this handler.

[jkleinsc]

@penx I am going to try to implement this in a new PR.