pass the unmodified login path to the native component instead of assuming .gpg

[uninsane]

this is in support of
browserpass/browserpass-native#127.

this has immediate benefit for anyone using the patches shared in that PR today. without this, browserpass doesn't recognize github.aaakk.us.kg.age as a default key for https://github.com, because it fails the substring match. by stripping the extension -- whatever it is -- both github.aaakk.us.kg.gpg and github.aaakk.us.kg.age are recognized as keys for their intended domain.

[erayd]

Needs to only do this for versions of the native host that have the ability to accept such paths. Changes to the extension must be backwards-compatible with older host versions.

[uninsane]

@erayd the filenames here come from the native host. it won’t return any path that didn’t originate in the native host to begin with.

today the host uses a **/*.gpg glob. then in the extension we strip the .gpg to get login.login and append .gpg again before passing it back to the host. i’ve just simplified that here, such that we roundtrip the path no matter the filetype. this has no visible change if the user’s using a **/*.gpg glob — which today is anyone running master.

can you clarify your concern with backward compatibility, and then maybe i can try to fix whatever it is?

[erayd]

@uninsane My mistake then sorry - clearly I reviewed this too fast! I just looked at it, saw filename changes without compatibility checks, and called it a day. If it's just passing the same thing back, then I have no issue with that 🙂.

Other than the replacement method, I'm happy with this.

[maximbaz]

I'm also happy with this and don't have any additional comments, thanks @uninsane!

[erayd]

Looks great. Thanks @uninsane 😁

[maximbaz]

You guys are awesome 👍

[patgmiller]

@erayd @maximbaz did you guys even test this? it breaks anything that has a dot in the path, the regex to remove the extension is too aggressive

[patgmiller]

also, no one even mentioned how close the large edit refactor was to being merged ...

[erayd]

@patgmiller Read but not run, as was reviewing this on my phone at the time. Can you clarify what is too aggressive about the regex? It's end-anchored, so it should not be removing anything but the file extension.

Didn't explicitly mention the edit PR, because it didn't seem particularly relevant to do so - this one addresses something entirely different.

[erayd]

@patgmiller I just tested this commit now, and it seems to be working fine with paths containing a period. I tested the following:

• Where the period is in a folder name (e.g. /example.com/file.gpg)
• Where the period is in a file name, but does not immediately precede the extension (e.g. /folder/example.com.gpg)
• Where the only period immediately precedes the extension (e.g. /folder/example.gpg)

All tested scenarios worked as expected. Can you clarify what specifically this commit is breaking for you?

[erayd]

Have also now tested the bare regex in the regex101 sandbox, as per the attached screenshot, in case I was missing something obvious - but all seems OK there too.

[patgmiller]

@patgmiller Read but not run, as was reviewing this on my phone at the time. Can you clarify what is too aggressive about the regex? It's end-anchored, so it should not be removing anything but the file extension.

the [^.] and the + would be considered aggressive when you're only looking for 3 alpha chars after a period . This is not working on the latest edit-ui changes,

Dec 25 16:57:20 friday browserpass[26345]: time="2022-12-25T16:57:20-07:00" level=error msg="Unable to decrypt the password file 'browserpass/google.gpg' in the password store '{ID:x468bpnd2 Name:pass.go Path:/home/patrick/playground/github.com/w3digitalfoundry/.password-store Settings:{GpgPath:}}': open /home/patrick/playground/github.com/w3digitalfoundry/.password-store/browserpass/google.gpg: no such file or directory"

the actual path for that file is

/home/patrick/playground/github.com/w3digitalfoundry/.password-store/browserpass/google.com/example-extra-lines.gpg

Didn't explicitly mention the edit PR, because it didn't seem particularly relevant to do so - this one addresses something entirely different.

You didn't think there would be conflicts especially considering the amount of refactoring that was done!? I couldn't de-crypt anything after these changes.

So I'll just have to figure out what the problem is. I'm just really tried of more changes and things getting pushed further back

[erayd]

the [^.] and the + would be considered aggressive when you're only looking for 3 alpha chars after a period

Not all file extensions are only three chars; longer ones are common. Unless we are needing to handle files with no extension at all, then the length is irrelevant here; what matters is correctly identifying the final dot. Which this regex does.

This is not working on the latest edit-ui changes,

I can guarantee it's not broken because of the regex (see screenshot below). I'll see if I can locate the issue for you. It doesn't break on master, which is what this PR is against. Stand by.

You didn't think there would be conflicts especially considering the amount of refactoring that was done!?

With this specific issue, no - because it shouldn't even be touching your bit; it's a six-line change to switch from .gpg-only, to any extension.

So I'll just have to figure out what the problem is. I'm just really tried of more changes and things getting pushed further back

This commit should not have required changes from you, unless you also changed these lines (and if the latter is the case, then merging this in should be trivial). I'm not going to deny merge for six trivial lines for the sake of an enormous PR that is entirely unrelated. Picking up this kind of thing is what rebasing is for.

[erayd]

@patgmiller So, the root cause appears to be a bug in your PR, which calls the helper function prepareLogin() twice on a path - once during prepareLogins(), and then again on its own output via Login.get(). This results in the "remove extension" code running twice, which is what ate the rest of that file path. It's not anything to do with #312 at all. However, #312 highlighted it, because it widened the scope of situations where the bug would be triggered.

To reproduce it on your edit-ui branch, without this PR merged, just add an entry named whatever.gpg (so the file will be whatever.gpg.gpg). You'll then see the same decryption failure as you reported above (unless you happen to have a file named whatever.gpg, in which case it will incorrectly try to decrypt that one instead).

I will produce a diff for you against edit-ui that both fixes your bug, and merges this PR.

[erayd]

This should sort it. The bugfix is in interface.js, and ensures that prepareLogin() gets called with the original file path, rather than with the stripped-path output of preparelogin().

I'll take a more retailed look though later and see if I can spot any other filename-related issues (e.g. hardcoded gpg suffixes).

diff --git a/src/background.js b/src/background.js
index a153c29..01bc73a 100644
--- a/src/background.js
+++ b/src/background.js
@@ -944,7 +944,7 @@ function hostAction(settings, action, params = {}) {
 async function parseFields(settings, login) {
     var response = await hostAction(settings, "fetch", {
         storeId: login.store.id,
-        file: login.login + ".gpg",
+        file: login.loginPath,
     });
     if (response.status != "ok") {
         throw new Error(JSON.stringify(response)); // TODO handle host error
diff --git a/src/helpers.js b/src/helpers.js
index dba00e4..9fd2a49 100644
--- a/src/helpers.js
+++ b/src/helpers.js
@@ -213,10 +213,12 @@ function prepareLogins(files, settings) {
  * @return object of login
  */
 function prepareLogin(settings, storeId, file, index = 0, origin = undefined) {
+    const loginName = file.replace(/\.[^.]+$/u, ""); // remove the file-name extension
     const login = {
         index: index > -1 ? parseInt(index) : 0,
         store: settings.stores[storeId],
-        login: file.replace(/\.gpg$/i, ""),
+        login: loginName,
+        loginPath: file,
         allowFill: true,
     };
 
diff --git a/src/popup/interface.js b/src/popup/interface.js
index 64746a3..4054e45 100644
--- a/src/popup/interface.js
+++ b/src/popup/interface.js
@@ -159,7 +159,9 @@ function renderMainView(ctl, params) {
                             title: "Open Details | <Ctrl+O>",
                             oncreate: m.route.link,
                             onupdate: m.route.link,
-                            href: `/details/${result.store.id}/${encodeURIComponent(result.login)}`,
+                            href: `/details/${result.store.id}/${encodeURIComponent(
+                                result.loginPath
+                            )}`,
                         }),
                     ]
                 );