Add feature to launch URLs & support basic auth #224

erayd · 2018-03-20T21:49:03Z

Host app will now return a "url" field
Browser plugin has a "launch URL" button for each entry
Browser plugin has a new hotkey (g) to launch the URL
Supply basic auth credentials if necessary when launching via the extension

Note that basic auth will only work in Firefox >= 54. Older versions don't have the required API to do this.

Closes #214.
Closes #103.

maximbaz · 2018-03-20T22:06:59Z

Nice! Let me throw some ideas first.

Could you please check this out? When we are opening a new tab, can we try to additionally override onAuthRequired and prefill the user credentials, so that user is already logged in? As a bonus, this will also solve Support HTTP basic authentication (browser pop-up window) #103.
When host app does not return a url, instead of just saying "sorry we can't do it", can we open the url of the entry name itself, if it looks like a domain? In other words, if my file name is google.com.gpg, I'd like browserpass to open google without having to add url: https://google.com to the file. Assuming the majority of password entries are named as a valid domain, it's not nice to render the button to open URL, when it cannot actually open the URL (unless I add this url: metadata to each and every password entry).

erayd · 2018-03-20T22:38:40Z

@maximbaz

Will take a look and see what I can do.
I will add this. Note that this will require bundling a TLD list with the extension - simply guessing which path component is the URL doesn't feel like a good idea (because it could result in things like http://work or http://myUsername).

maximbaz · 2018-03-20T22:55:20Z

Thanks! Hmm, you are right, it is not very clear how to select the domain part from the entry name...

Although I wouldn't suggest building a super comprehensive domain matcher, because the workaround is very simple (adding url: to the password entry) - it should just work in most common cases.

erayd · 2018-03-21T00:51:51Z

@maximbaz I've implemented extracting a URL from the entry path. What do you think?

erayd · 2018-03-21T04:26:59Z

@maximbaz HTTP basic auth now works too :-).

maximbaz

Really nice work! I left some notes and questions.

maximbaz · 2018-03-21T10:53:32Z

chrome/background.js

+            }
+          });
+          // only supply credentials if this is the first time for this tab
+          if (!authAttempted) {


Could you walk me through this logic? So we register the listener immediately after a tab is created, remove the listener after the tab is loaded (is it guaranteed that info.status will become complete only after basic auth is filled in?), and at the same time you additionally prevent supplying credentials more than once (why is this needed? won't removing the listener suffice?).

Create a new tab, and get the tab ID in the callback

Add onAuthRequired listener

When the auth callback fires, add a status listener to the tab

Supply login credentials if we haven't already supplied them

When the status listener fires and the status is complete (i.e. the page has loaded), remove both listeners.

The reason both the authCompleted and the listeners are required:

The onAuthRequired listener cannot remove itself, as doing so crashes the extension.

Because it cannot remove itself, it may fire multiple times (e.g. if the password is incorrect)

So the boolean restricts it to one time only

And removing it in the status listener once the page is complete ensures that the event is no longer live, and avoids a crash.

is it guaranteed that info.status will become complete only after basic auth is filled in?

I could not find any specific documentation guaranteeing this, and I looked pretty hard. However, the documentation does say that this occurs when the page load is complete, and blocking authentication requirements prevent the page from loading until they are satisfied. I've tested it extensively, and I'm comfortable that this does seem to be the case, but I cannot absolutely guarantee that it is so.

thank you, makes sense!

maximbaz · 2018-03-21T10:53:43Z

chrome/background.js

+          // only supply credentials if this is the first time for this tab
+          if (!authAttempted) {
+            authAttempted = true;
+            // ask the user before sending credentials over an insecure connection


maximbaz · 2018-03-21T10:54:50Z

chrome/background.js

+          }
+        },
+        {urls: ["*://*/*"], tabId: tab.id},
+        ["blocking"]


Is this the guarantee that info.status will become complete only after basic auth is filled in? Or what is this for?

It's not possible to mess with the request asynchronously (including providing authentication details). The "blocking" option ensures that the request waits for our callback to complete before proceeding.

I see, thanks!

maximbaz · 2018-03-21T10:55:35Z

chrome/manifest.json

@@ -7,7 +7,7 @@
  "author": "Danny van Kooten",
  "homepage_url": "https://github.com/dannyvankooten/browserpass",
  "background": {
-    "persistent": false,
+    "persistent": true,


Could you comment on why this is needed?

Chrome doesn't allow non-persistent event pages to use the webRequest API. If you have this set to false, you'll get a massive warning when you try to load the extension telling you that this isn't possible, and then Chrome will refuse to load the extension.

maximbaz · 2018-03-21T11:01:01Z

chrome/script.browserify.js

+    function(response) {
+      if (chrome.runtime.lastError) {
+        error = chrome.runtime.lastError.message;
+        m.redraw();


nitpick: could you please reduce the nesting?
It's a little difficult to see which "else" belongs to which "if" 🙂

if (error) { ... return; }

maximbaz · 2018-03-21T11:27:41Z

chrome/script.browserify.js

+      } else {
+        // get url from login path if not available in the host app response
+        if (!response.hasOwnProperty("url") || response.url.length == 0) {
+          var parts = entry.split(/\//).reverse();


I see pros and cons in reversing the parts, and so far I found only one case among my passwords where this is not ideal. We will probably keep this as it is, but I want to ask anyway, what is your reasoning for and against reversing?

It seems more likely that someone will have a workplace that is a valid domain (e.g. "I work for example.com"), than a username that is a valid domain. It also seems more likely that if domains are nested, the deepest one is the most likely to be what the user wants. So reversing it prioritises domains that are found closer to the end of the entry.

maximbaz · 2018-03-21T11:32:32Z

chrome/script.browserify.js

+        if (response.hasOwnProperty("url") && response.url.length > 0) {
+          var url = response.url.match(/^([a-z]+:)?\/\//i) ? response.url : "http://" + response.url;
+          chrome.runtime.sendMessage({action: "launch", url: url, username: response.u, password: response.p});
+          window.close();


here's another potentially good place to do return and reduce nesting.

maximbaz · 2018-03-21T11:50:11Z

chrome/script.browserify.js

+        } else {
+          // no url available
+          if (!response.hasOwnProperty("url")) {
+            resetWithError("Your host application is too old - must be at least 2.0.14 for URL launch.");


First brick in building #164, I like this 👍

However, we need to adjust the message, because with added parsing of domain it's not consistent anymore. With the old host application I can open url for password entries like "personal/google.com" just fine, until one day I try to open url for the entry "personal/notes" and suddenly get the error "Your host application is too old".

I think we should say something like this:

Unable to detect the URL to open for this entry. If you defined it in the password file, update your host application to the latest version.

Good point - done.

maximbaz · 2018-03-21T11:52:04Z

chrome/script.browserify.js

+          if (!response.hasOwnProperty("url")) {
+            resetWithError("Your host application is too old - must be at least 2.0.14 for URL launch.");
+          } else {
+            resetWithError("No URL is available for this login.");


In my mind something like this will sound better, maybe there is a URL but it's just us who cannot detect it:

Unable to detect the URL to open for this entry.

maximbaz · 2018-03-21T12:30:36Z

Attaching the compiled extension for the current code for others to test:

chrome-browserpass.crx.zip

firefox.zip

- Host app will now return a "url" field - Browser plugin has a "launch URL" button for each entry - Browser plugin has a new hotkey (g) to launch the URL

Not compatible with Firefox < v54 https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onAuthRequired#Browser_compatibility

maximbaz · 2018-03-21T21:04:23Z

chrome/background.js

+      chrome.webRequest.onAuthRequired.addListener(
+        function authListener(requestDetails) {
+          // only supply credentials if this is the first time for this tab
+          if (!authAttempted) {


could you invert this too, i.e. if (authAttempted) { return {}; }? Also judging by line 123 returning {} is important in this case too.

No problem - done :-).

maximbaz

Thanks once again, this is awesome! I wanted to get some additional feedback from people in #103, but we can as well make a release and wait for people to submit issues if they experience anything weird 😉

maximbaz · 2018-03-21T21:10:39Z

Just as a final confirmation, do you want to do anything else here or can I merge?

erayd · 2018-03-21T21:11:17Z

Thanks once again, this is awesome!

Most welcome :-).

Just as a final confirmation, do you want to do anything else here or can I merge?

If you could give me two minutes, I want to tweak the 'insecure connection' error message slightly. Stand by.

erayd · 2018-03-21T21:14:34Z

@maximbaz Thanks - I just wanted to clarify that message a bit; I figured the original wording wasn't the clearest.

I'm happy with it now - all good to merge :-). Thanks very much for your help with this, and for your extremely speedy code reviews!

maximbaz · 2018-03-22T10:21:16Z

chrome/background.js

+          authAttempted = true;
+          // remove event listeners once tab loading is complete
+          chrome.tabs.onUpdated.addListener(function statusListener(tabId, info) {
+              if (info.status === "complete") {


@erayd do you think this code should also compare tabId with the tab.id before removing the listeners?

It doesn't need to, because the listeners are specific to the tab anyway.

There is a security problem, but that's not it. I'm filing an issue now.

Are you sure that this doesn't register a listener that fires for all tabs? Maybe it's just a weird API, but the tab object is not used when registering the listener, and the tabId is provided as a first argument for a reason, both of these lead me to think that this will fire for all tabs...

I know that this is not about the security problem, it's just what I accidentally stumbled upon 🙂

Ooh, good point. The onAuthRequested handler is specific to the tab, but the status listener is not. My bad.

I'm going to roll the security fix PR right now - I feel quite bad I didn't notice that in the first place; I'm normally more careful than that. I will fix this at the same time.

uri · 2018-03-22T19:50:06Z

Aside from this feature addressing the basic auth issue #103, what is the purpose of this? It seems strange to have this on all pass entries. Personally I don't have a single basic auth entry, so for me the button is a worse way to navigate to url? Is there another common use case for this that I'm missing?

maximbaz · 2018-03-22T20:02:31Z

See #214 for more details, people seem to like this way of opening websites in Lastpass and other password managers, and they carry their habits to browserpass.

In addition it now allows you to use url: metadata in your pass files. For example, you might have a file called github.com.gpg, but inside of it you add url: https://github.com/login, and thus when you open github via browserpass, it opens the login form if you are unauthorized, or the home page if you are already authorized.

uri · 2018-03-23T15:40:49Z

In addition it now allows you to use url:

Oh now that is useful.

#235

erayd changed the title ~~Add feature to launch URLs if available~~ Add feature to launch URLs & support basic auth Mar 21, 2018

erayd force-pushed the launch-url branch from 0f9e83a to a543740 Compare March 21, 2018 04:52

maximbaz mentioned this pull request Mar 21, 2018

Support HTTP basic authentication (browser pop-up window) #103

Closed

maximbaz suggested changes Mar 21, 2018

View reviewed changes

erayd added 6 commits March 22, 2018 07:22

Add feature to launch URLs if available

0efc727

- Host app will now return a "url" field - Browser plugin has a "launch URL" button for each entry - Browser plugin has a new hotkey (g) to launch the URL

Add extraction of URL from entry path, if not available from the file

91cc637

If there are multiple url fields, use the first one

a309160

Add support for auto-supplying credentials when launching a URL

6dc747d

Not compatible with Firefox < v54 https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onAuthRequired#Browser_compatibility

Don't indent with tabs

5ec6d76

Don't register the status listener more than once

dd9a741

erayd force-pushed the launch-url branch from 6bb7e3c to 57f7908 Compare March 21, 2018 18:38

Nesting & error message changes

57f7908

maximbaz reviewed Mar 21, 2018

View reviewed changes

erayd added 2 commits March 22, 2018 10:08

Invert condition check to reduce nesting

21de295

Move the assignment closer to the condition to make things more obvious

207c798

maximbaz approved these changes Mar 21, 2018

View reviewed changes

Update error message

fcfe36f

maximbaz merged commit 604f560 into browserpass:master Mar 21, 2018

erayd deleted the launch-url branch March 21, 2018 21:17

maximbaz reviewed Mar 22, 2018

View reviewed changes

erayd mentioned this pull request Mar 22, 2018

[SECURITY] Credential leak vector #229

Closed

erayd mentioned this pull request Mar 22, 2018

[SECURITY] Credential leak vector #230

Closed

Add feature to launch URLs & support basic auth #224

Add feature to launch URLs & support basic auth #224

Conversation

erayd commented Mar 20, 2018 • edited Loading

maximbaz commented Mar 20, 2018 • edited Loading

erayd commented Mar 20, 2018

maximbaz commented Mar 20, 2018

erayd commented Mar 21, 2018

erayd commented Mar 21, 2018

maximbaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz left a comment

Choose a reason for hiding this comment

maximbaz commented Mar 21, 2018

erayd commented Mar 21, 2018

erayd commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uri commented Mar 22, 2018

maximbaz commented Mar 22, 2018

uri commented Mar 23, 2018

erayd commented Mar 20, 2018 •

edited

Loading

maximbaz commented Mar 20, 2018 •

edited

Loading

erayd Mar 21, 2018 •

edited

Loading

maximbaz commented Mar 21, 2018 •

edited

Loading

erayd commented Mar 21, 2018 •

edited

Loading

erayd Mar 22, 2018 •

edited

Loading