Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create right-click menu #284

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ In order to use Browserpass you must also install a [companion native messaging
- [Organizing password store](#organizing-password-store)
- [First steps in browser extension](#first-steps-in-browser-extension)
- [Available keyboard shortcuts](#available-keyboard-shortcuts)
- [Usage via right-click menu](#usage-via-right-click-menu)
- [Password matching and sorting](#password-matching-and-sorting)
- [Searching password entries](#searching-password-entries)
- [OpenID authentication](#openid-authentication)
Expand Down Expand Up @@ -160,6 +161,12 @@ Note: If the cursor is located in the search input field, every shortcut that wo
| <kbd>Ctrl+Shift+G</kbd> | Open URL in the new tab |
| <kbd>Backspace</kbd> (with no search text entered) | Search passwords in the entire password store |

### Usage via right-click menu

You can right-click anywhere a visited website and there will appear a menu with an option `Browserpass - <n> entries`, where `n` is the number of entries that match the host of the visited website. When you select an entry, that one gets automatically filled in, equivalent to the behavior when an entry is selected from the Browserpass popup. This can be helpful if you want to fill credentials in a browser popup window without extension buttons. Selecting single form fields and choosing values to fill in is currently not supported

![The right-click menu of browserpass](https://user-images.githubusercontent.com/15818773/153720814-66b99653-3b8a-456d-a55a-8dd208e66028.gif)

### Password matching and sorting

When you first open the Browserpass popup, you will see a badge with the current domain name in the search input field:
Expand Down Expand Up @@ -300,6 +307,7 @@ Browserpass extension requests the following permissions:
| `tabs` | To get URL of a given tab, used for example to set count of the matching passwords for a given tab |
| `clipboardRead` | To ensure only copied credentials and not other content is cleared from the clipboard after 60 seconds |
| `clipboardWrite` | For "Copy password" and "Copy username" functionality |
| `contextMenus` | To create a context menu, also called right-click menu |
| `nativeMessaging` | To allow communication with the native app |
| `notifications` | To show browser notifications on install or update |
| `webRequest` | For modal HTTP authentication |
Expand Down
69 changes: 69 additions & 0 deletions src/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ chrome.browserAction.setBadgeBackgroundColor({
chrome.tabs.onUpdated.addListener((tabId, info) => {
// unregister any auth listeners for this tab
if (info.status === "complete") {
createContextMenu();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing I managed to get some errors in the console when I was opening a new tab, which is because of concurrent attempts to recreate context menu. Here's what happens:

createContextMenu is async, so by calling it here we don't really block browser, there's no way of knowing when the function completes. In the beginning of createContextMenu we first remove the context menu and then try to recreate it. At the same time, chrome.tabs.onActivated will get called, in turn calling createContextMenu in parallel. The previous run has just created the parent menu, this run removes it, and then the previous run tries to create children to a now gone parent, and crashes.

Of course one way to reduce the chances of a crash is to move chrome.contextMenus.removeAll() from the beginning of the function to as much later as possible - but this will not entirely eliminate the problem, but rather just reduce its chances.

Another problem is that here you actually call createContextMenu only on complete event - I understand why you did it, but it presents a problem on slow websites. I open github.com and my context menu is filled with entries. I then go in this tab to some very slow website, which loads a ton of images, and so complete event will only fire in say 5 seconds. During the first 5 seconds if I right click, I will still see entries for github.

I propose to repeat what updateMatchingPasswordsCount function does:

  • be called on all statuses, not just on complete
  • use some kind of analogue to isRefreshing variable to prevent concurrent runs
  • probably you won't need chrome.tabs.onActivated below, because badge seems to not need it...

Let me know if it makes sense and if you have any questions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough review, I can reproduce but haven't figured out how to solve it completely yet. I'll keep you posted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I figured it out 🎉. I call createContextMenu now on every status, but I need chrome.tabs.onActivated to "kill" callbacks that are being called from an "old" tab.

It's a bit hard to formulate, so here's an example: I'm in tab A with a slow page, I switch to tab B before tab A finished loading, when tab A finally finished loading I don't want it to mess up the context menu.

Copy link
Collaborator

@erayd erayd Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fkneist Would simply including the origin in the callback be sufficient? That way you can just discard any calls that have a different origin than the currently active one.


if (authListeners[tabId]) {
chrome.webRequest.onAuthRequired.removeListener(authListeners[tabId]);
delete authListeners[tabId];
Expand All @@ -51,6 +53,10 @@ chrome.tabs.onUpdated.addListener((tabId, info) => {
updateMatchingPasswordsCount(tabId);
});

chrome.tabs.onActivated.addListener(() => {
createContextMenu();
});

// handle incoming messages
chrome.runtime.onMessage.addListener(function (message, sender, sendResponse) {
receiveMessage(message, sender, sendResponse);
Expand Down Expand Up @@ -1156,3 +1162,66 @@ function onExtensionInstalled(details) {
});
}
}

/**
* Create a context menu, also called right-click menu
*
* @since 3.8.0
*
* @return void
*/
async function createContextMenu() {
await chrome.contextMenus.removeAll();

const menuEntryProps = {
contexts: ["all"],
type: "normal",
};
const menuEntryId = "menuEntry";

const settings = await getFullSettings();
const response = await hostAction(settings, "list");

if (response.status != "ok") {
throw new Error(JSON.stringify(response));
}
const files = helpers.ignoreFiles(response.data.files, settings);
const logins = helpers.prepareLogins(files, settings);
const loginsForThisHost = helpers.filterSortLogins(logins, "", true);
maximbaz marked this conversation as resolved.
Show resolved Hide resolved
const numberOfLoginsForThisHost = loginsForThisHost.length;

if (numberOfLoginsForThisHost > 0) {
await chrome.contextMenus.create({
...menuEntryProps,
title: `Browserpass`,
id: menuEntryId,
});

for (let i = 0; i < numberOfLoginsForThisHost; i++) {
await chrome.contextMenus.create({
...menuEntryProps,
parentId: menuEntryId,
id: "login" + i,
title: loginsForThisHost[i].login,
onclick: () => clickMenuEntry(settings, loginsForThisHost[i]),
});
}
}
}

/**
* Handle the click of a context menu item
*
* @since 3.8.0
*
* @param object settings Full settings object
* @param object login Login object
* @return void
*/
async function clickMenuEntry(settings, login) {
await handleMessage(settings, { action: "fill", login }, (response) => {
if (response.status != "ok") {
throw new Error(JSON.stringify(response));
}
});
}
3 changes: 2 additions & 1 deletion src/manifest-chromium.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"webRequest",
"webRequestBlocking",
"http://*/*",
"https://*/*"
"https://*/*",
"contextMenus"
],
"content_security_policy": "default-src 'none'; font-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self'",
"commands": {
Expand Down
3 changes: 2 additions & 1 deletion src/manifest-firefox.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
"webRequest",
"webRequestBlocking",
"http://*/*",
"https://*/*"
"https://*/*",
"contextMenus"
],
"content_security_policy": "default-src 'none'; font-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self'",
"applications": {
Expand Down