1654403 - commands.update does not check for aliases with Firefox native shortcuts or other extensions

Status: NEW

(WebExtensions :: Frontend, enhancement, P3)

Description

[Jakub Okoński]

browser.commands.update() performs syntax validation only. It does not check for aliasing keybindings against other extensions or even Firefox shortcuts itself. The about:addons form for editing shortcuts forbids things like Ctrl+W and aliases of other commands. This API however does no such checks.

As far as I can see, there are no security implications of this. Ctrl+W is handled separately from webext commands and although it's possible to bind to it, and even though about:addons will happily show this binding, the extension will not receive such event.

But it is possible to create aliases of other extension's bindings. I did not confirm if both event handlers are dispatched (for two extensions), but I did run into a conflict where an update to one extension configured a shortcut that conflicted with my existing one for another extension, making that extension unusable.

I have not checked if installation of an extension with static (manifest.json and not using commands.update()) commands that conflict with my existing shortcuts causes problems. It does make me worried, however, that it might also be possibile.

Comment 1

[Rob Wu [:robwu]]

The behavior of commands.update API is consistent with the commands key in manifest.json:
manifest.json can declare a command (which may overlap with a built-in shortcut or another extension's shortcut).
The fact that it can override a built-in shortcut is a desired feature (despite it not working consistently, and neither on all platforms - bug 1325692).

In about:addons we do check whether the shortcut is in use, because we have enough context to offer meaningful feedback to the user, and offering the feedback provides a better user experience.

We've previously decided that we wouldn't offer the ability to query shortcuts in bug 1627315, because it's quite involved and there is already a built-in page that provides the ability to configure shortcuts (and resolve conflicts if desired).

Comment 2

[Jakub Okoński]

I find the idea that my extensions are allowed to do more than me questionable, but it seems like you've given it a lot of thought, so there's no point in arguing.

Comment 3

[Rob Wu [:robwu]]

The issue that you reported is not unique to commands.update: extensions can replace shortcuts on update, or even replace them on install.

I haven't given enough thought on how the issue of shortcut hijacking can be resolved, and am willing to consider proposals (possibly in a new bug that refers back to this bug) to improve that. Just changing commands.update doesn't solve the main issue though.

There needs to be a good balance between helping the user and annoying them with prompts.
E.g. an inconspicuous notification that there are conflicting shortcuts may help without being annoying, but is also likely a wasted effort if it's not seen by users.

Comment 4

[klaus]

'conflicting shortcuts' is, stated clearly, a malfunctioning. As such, it should be worth a visible notification.

In the case of addons, the user has decided to install addon A. He should be informed if it cannot work due to conflicting shortcuts.

If there is no clear visible notification, the user expects the installation to be error free, which it is not. Some part of its functionality is not working, and the addon author currently has no means to take care of that.

Comment 7

[Rob Wu [:robwu]]

Because this behavior is not ideal and independent reports keep popping up, we have reconsidered the previous decision and are supportive of offering a way to allow extensions to resolve conflicts.

This will be in the form of a new method to query whether a shortcut is available (using the same logic as what we use in the shortcuts manager at about:addons). This is backwards-compatible with extension consumers of the commands.update API while allowing cooperating extensions to avoid registering a conflicting shortcut.

This is a best-efforts method:

• Extensions are not notified when a shortcut is taken (whether by the user or another extension).
• We have full control over the extension framework and intent to at least offer the ability to avoid conflicts between extensions. Recognizing built-in shortcuts is a nice-to-have, but not a blocker for the resolution of this bug.

Comment 8

[fixmebug]

I have another request. Knowing the conflict is important, but if possible I want a way to get a list of shortcuts that aren't currently in use. When a user wants to assign a shortcut key to an add-on, it is useful to have a bird's eye view of which keys are unused. If there is an API, we can create an add-on to do that, or it would be nice if about:addons shows abailable keys.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

If this is the first "good first bug" issue you are working on, we advice to read the following wiki page to setup your development environment and get some other information useful to get you started:

• https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

The goal of this issue is to add a new API method to the commands WebExtensions API.

We could call the new API method commands.query and it would:

• get a shortcut string as its only parameter
• return a Promise that resolves to an object that describes the availability status of the shortcut key, e.g. { shortcut: "...", assignedTo: "available / same-extension / other-extension" } where:
  • shortcut is a string with the same value passed to the API method
  • commandName, if it is set is a string with the command associated to the shortcut (it is null if the shortcut is not assigned or not assigned to the current extension)
  • assigned is a boolean (true is the shortcut is assigned)
  • isExtension is a boolean (set to true if the shortcut is assigned to an extension, instead of being used by the browser application and not available to the extensions)

Some of the internals that are relevant for this issue:

• the commands API JSONSchema: https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/commands.json
  (which is where the new API method schema should be defined)
• the commands API implementation: https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-commands.js
• the existing tests for the commands API: https://searchfox.org/mozilla-central/search?q=browser_ext_commands&path=

Comment 10

[Ankush Dua]

Attachment: Bug 1654403 - Adding new API method to the commands WebExtensions API named commands.query r=rpl

Comment 11

[Ankush Dua]

Hi rpl!... I was trying to look into how can I get a list of all the registered shortcuts for a particular browser profile. and
here is what I have tried:

• I asked over at #[e-mail] matrix channel and someone pointed out that there is a command reassigning UI that knows of all the shortcuts that are registered.
• then I tried using let allAddons = await AddonManager.getAddonsByTypes(["extension"]); to get all the addons installed on a browser (because this is what shortcut.js is using to get all the addons here: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/shortcuts.js#661 ) and then filterout all the shortcuts that are registered.

Now here's where I am stuck:
I logging allAddons from the above code line by calling it in a new queryCommand method that I defined in ExtensionShortcuts.jsm(included in the patch) and using that new method query in commands API to call queryCommand in my temporary extension. But I get an error saying AddonManager is not defined.

If I may get some guidance on how to proceed that would be awesome.

Thank You!...

(PS: I will update this comment if I get further help from matrix channel towards this.)

Comment 12

[Ankush Dua]

Update: I was making a silly mistake of not importing AddonManager. For some reason, I thought it would work with import 😅.

Comment 13

[Ankush Dua]

without**

Comment 14

[Ankush Dua]

Update: I am starting to think using AddonManager to get all the extensions and then getting all the shortcuts off of those extensions may not be the right course of action here, because I could not figure out a way to get all the registered shortcuts.

Could you please guide me on how to get shortcuts for all the extensions?

Comment 15

[Ankush Dua]

Update: I was able to get shortcuts of all the addons if available. Now I am looking for a way to check if those a shortcut collides with any system shortcut. Any Suggestions?

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Ankush Dua from comment #15)

Update: I was able to get shortcuts of all the addons if available. Now I am looking for a way to check if those a shortcut collides with any system shortcut. Any Suggestions?

My apologies for letting you wait for a reply to your needinfo.

As briefly discussed on the addons Matrix channel:

The part of shortcuts.js that does validate the shortcut strings and also checking if it does conflict with a system shortcut should be here: https://searchfox.org/mozilla-central/rev/07342ce09126c513540c1c343476e026cfa907bf/toolkit/mozapps/extensions/content/shortcuts.js#428-461

The shortcut validation is using the utilities exported by ShortcutUtils.jsm.

Comment 17

[Ankush Dua]

@rpl I was having a hard time figuring out the strategy to use here.
So far what I was trying to do is to get all the extensions and then try to find if any commands are associated with those extensions.
if there are any commands then I try to get shortcut related to those commands.
To store those shortcuts for the purposes of detecting a collision, I try to make a shortcutKeyMap just like it is done In shortcut.js here https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/shortcuts.js#311

Now I have made some progress on the patch. I am looking to make sure that I am on the correct path.

Question:
In shortcut.js it takes chromeWindow as an argument to check for shortcut collision for system. When I try to use the same logic in queryCommand function I get an error saying window is not defined, here is concerned code: https://searchfox.org/mozilla-central/rev/07342ce09126c513540c1c343476e026cfa907bf/toolkit/mozapps/extensions/content/shortcuts.js#432-433

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Ankush Dua from comment #17)

@rpl I was having a hard time figuring out the strategy to use here.
So far what I was trying to do is to get all the extensions and then try to find if any commands are associated with those extensions.
if there are any commands then I try to get shortcut related to those commands.
To store those shortcuts for the purposes of detecting a collision, I try to make a shortcutKeyMap just like it is done In shortcut.js here https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/shortcuts.js#311

Now I have made some progress on the patch. I am looking to make sure that I am on the correct path.

I haven't looked to the patch in detail yet, but I'll come back to it asap.
In the meantime I wanted to answer your question below.

Question:
In shortcut.js it takes chromeWindow as an argument to check for shortcut collision for system. When I try to use the same logic in queryCommand function I get an error saying window is not defined, here is concerned code: https://searchfox.org/mozilla-central/rev/07342ce09126c513540c1c343476e026cfa907bf/toolkit/mozapps/extensions/content/shortcuts.js#432-433

Ok, to get a better picture about why something that works in shortcut.js may not work as is if we do it in ExtensionShortcuts.jsm we need to look into "where those two files are being executed"

shortcut.js

• shortcut.js is a script part of the about:addons tab (to be precise it is currently loaded in a sub-frame of the about:addons page, but that is going to change soon enough once we will land Bug 1525179 patch), its global is the frame where that script has been loaded
• in shortcut.js, window.windowRoot.ownerGlobal means "get me the browser window that contains this privileged tab" ([1]), you can also double-check that by running that statement in a webconsole opened on an about:addons tab.

ExtensionShortcuts.jsm

• ExtensionShortcuts.jsm is a javascript module, its global is the module itself and it doesn't have a window property available by default as a script loaded in a HTML page would have.
• ExtensionShortcuts.jsm is a privileged javascript module, which means that it does have the same privileged of the browser and can import and use other privileged internals not available to a script loaded in a regular web page
• ExtensionShortcuts.jsm is only loaded in the main process (not all jsm are loaded in the main process, or only in the main process, it depends what they are used for) and so technically it is possible to get a reference to a browser window (at least is one is open at the moment, [2])
• Services.wm.getMostRecentWindow("navigator:browser") would return as a reference to the most recent browser window if one is open (you can also look for it on searchfox to see how we use it in some other extension-related jsm files

Let me know if these additional details are not fully answering your question (or if they are raising more questions and/or doubts ;-))

---

[1]: as an additional side note: about:addons is a privileged page and it does run in the main process as the rest of the Firefox UI, and so both about:addons page and the Firefox UI do live in the same process and winodw.windowRoot.ownerGlobal will return a reference to a browser window. On the contrary when we load a web page in a tab, it is going to run in a separate "web" child process and so window.windowRoot.ownerGlobal wouldn't get you the Firefox browser window (and that is also expected, a regular webpage shouldn't be able to get a reference to the browser window, which is a privileged component and internal implementation detail not part of the "web")
[2]: there is usually a browser window open while the Firefox instance is running, but that isn't always the case, in particular:

• on all systems, if you open a non browser Firefox windows (e.g. the Browser Console), you may close all the Firefox browser windows and the Firefox instance will still be running until the last Firefox window is closed (e.g. until you close the Browser Console in this specific example)
• on MacOS, a user may close all Firefox windows, but Firefox instance may still be running (that's common for MacOS apps)

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Re-assigning to Ankush (he is actively working on it)

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Re-assigning to Ankush (he is actively working on it)

Comment 23

[Caitlin Neiman [:caitmuenster]]

Hey Ankush, it's been awhile since we've heard from you and we wanted to check in. :) Are you still working on this bug? If not, no worries -- we'll open it up to other contributors and you can take on another bug when you have more time.

Comment 24

[Ankush Dua]

Hi Caitlin, Due to my final semester exams, I haven't been able to come back to it. But rest assured that I will be back on it within a couple of weeks.

Comment 25

[Ankush Dua]

Hi Caitlin, Due to recent development in my schedule, I may not be able to come back to it for a couple of months. I would have loved to work further on this issue however I don't want the issue to be sitting idle for that long.
Would you mind opening it to other developers?.

Thank you!..

Comment 26

[Caitlin Neiman [:caitmuenster]]

No problem at all, Ankush! If this hasn't been fixed by the time you're ready to come back, feel free to dive back in. :)