Closed projectgus closed 2 years ago
I appreciate your work, I look forward to more updates and reviewing these changes with you. Thank you.
On Thu, Feb 16, 2017 at 3:26 AM Angus Gratton notifications@github.com wrote:
As discussed in #563 https://github.com/pfn/passifox/issues/563. Allows Electrolysis (multi-process windows) feature to function in recent Firefox, and also provides a path for support once XUL-based addons are disabled in Firefox.
Support is not quite as tightly integrated as it was with passifox (ie there's no integration to Firefox's own "Fill Password" functions any more), but it seems very usable all the same.
Still a work in progress:
- Changed all chrome.xxx APIs to use browser.. Some are implementated in Firefox and work as-is, but a few (such as chrome.extension.sendMessage() are not. Seemed cleaner to change them all over.
- Add Mozilla's webextension-polyfill for Chrome compatibility. Needed to cherry-pick some PRs before things came good (details in commit message). The basics seem to work.
- Thoroughly test for broken Chrome functionality. I suspect there will still be some things which don't work.
- Firefox doesn't support webRequest.onAuthRequired (yet?) so can't fill HTTP auth popups from Chromeipass. Need to at least re-enable for Chrome, and hide the relevant Settings on Firefox.
- Haven't confirmed extension persistent storage (association to keypass, etc.) is working properly in FF. I think this is because I can only load the extension temporarily, so it's removing localStorage whenever I restart the browser. Firefox production builds don't allow unsigned addons at all any more, except temporarily.
- Has a Chrome Web Store link in the Settings when running on Firefox
- jQuery ("cIPJQ") seems to have some issues on FF that may be breaking functionality.
- Detecting password fields for generation of new passwords doesn't yet work on FF, I think related to the jQuery errors but I'm not sure.
Also, I don't know if this would be a good time to change the name? Or even if you're interested in adding this support at all - given it's a fairly major change to what seems like a stable Chrome extension.
You can view, comment on, or merge this pull request online at:
https://github.com/pfn/passifox/pull/595 Commit Summary
- chromeipass: Initial WebExtension support - can fill passwords in Firefox
- chromeipass: Add browser-polyfill for Chrome compatibility (part tested)
File Changes
- M chromeipass/background/browserAction.js https://github.com/pfn/passifox/pull/595/files#diff-0 (8)
- M chromeipass/background/event.js https://github.com/pfn/passifox/pull/595/files#diff-1 (9)
- M chromeipass/background/init.js https://github.com/pfn/passifox/pull/595/files#diff-2 (36)
- M chromeipass/background/keepass.js https://github.com/pfn/passifox/pull/595/files#diff-3 (2)
- M chromeipass/background/page.js https://github.com/pfn/passifox/pull/595/files#diff-4 (8)
- A chromeipass/browser-polyfill.min.js https://github.com/pfn/passifox/pull/595/files#diff-5 (9)
- M chromeipass/chromeipass.js https://github.com/pfn/passifox/pull/595/files#diff-6 (76)
- M chromeipass/manifest.json https://github.com/pfn/passifox/pull/595/files#diff-7 (11)
- M chromeipass/options/options.html https://github.com/pfn/passifox/pull/595/files#diff-8 (1)
- M chromeipass/options/options.js https://github.com/pfn/passifox/pull/595/files#diff-9 (33)
- M chromeipass/popups/popup.html https://github.com/pfn/passifox/pull/595/files#diff-10 (1)
- M chromeipass/popups/popup.js https://github.com/pfn/passifox/pull/595/files#diff-11 (18)
- M chromeipass/popups/popup_functions.js https://github.com/pfn/passifox/pull/595/files#diff-12 (17)
- M chromeipass/popups/popup_httpauth.js https://github.com/pfn/passifox/pull/595/files#diff-13 (9)
- M chromeipass/popups/popup_login.html https://github.com/pfn/passifox/pull/595/files#diff-14 (1)
- M chromeipass/popups/popup_login.js https://github.com/pfn/passifox/pull/595/files#diff-15 (8)
- M chromeipass/popups/popup_multiple-fields.html https://github.com/pfn/passifox/pull/595/files#diff-16 (1)
- M chromeipass/popups/popup_remember.html https://github.com/pfn/passifox/pull/595/files#diff-17 (1)
- M chromeipass/popups/popup_remember.js https://github.com/pfn/passifox/pull/595/files#diff-18 (28)
- M documentation/chromeIPass.md https://github.com/pfn/passifox/pull/595/files#diff-19 (9)
Patch Links:
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pfn/passifox/pull/595, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfQxeaClLKFbf3CRVeYQiRlr1mXkNjcks5rdDJZgaJpZM4MC5QR .
Thanks @pfn :)
I'm hoping to work on it in bits and pieces over the next little while. My day job is embedded development so playing with Javascript again is a breath of fresh air.
At the same time, if anyone else is reading this and wants to jump in and tackle any of the items mentioned above, please be my guest.
Coming along pretty well.
Decided I may not need to fix the jQuery errors I'm seeing in FF console, they don't appear to have any bearing on behaviour.
Could I help you with some testing? Don't have much experience with browser extensions but willing to try/learn.
Could I help you with some testing? Don't have much experience with browser extensions but willing to try/learn.
Sure, that'd be great! To install a development version in Firefox:
Usual caveats about development versions, trusting with your data, etc. apply. :)
As the addon is unsigned, it will uninstall when Firefox exits and you'll need to reload it. So far I've needed to re-pair it with KeepassXC each time this happens (there are some options in about:config, "extensions.webextensions.keepStorageOnUninstall" and "extensions.webextensions.keepUuidOnUninstall" but this has not been enough to make the association persist.) Still haven't looked to see if this is a bug in the addon or a security limitation of Firefox that can possibly be bypassed another way.
My current status on this is that I started yak shaving by adding support for context menu accelerator keys to firefox WebExtensions.
If you want to dig deeper, the Mozilla docs for WebExtensions are good: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
So far works great.
Have so far only noticed the bug you are working on atm, which seems to be a FF issue. I feel like your patch is ready to be pulled, while the compatibility is added to FF. Or would you rather wait till you have that fixed too?
Remaining bugs (bullet 3) will be found when more people start using this and start reporting them, and 5 should be fixed when this gets signed...
I signed a version for testing, if anyone wants to give this a go. Just drag and drop the xpi into your Firefox window, should install and persist across sessions.
chromeipass-2.8.0-an+fx.xpi.zip
I can confirm that number 5 is done when using this signed and packaged addon.
I can confirm that number 5 is done when using this signed and packaged addon.
Excellent, thank you!
I feel like your patch is ready to be pulled, while the compatibility is added to FF.
I think I generally agree. There's only bug I've seen now, sometimes if you click "Fill User & Pass" in the contextual menu then a debugging window pops up due to a fatal error. Doesn't seem to happen every time.
Other than this, the contextual menu titles look a bit weird in Firefox but it's easy enough to open a separate issue to track this, until Firefox supports them.
Angus
Works fine for me on FF 52.0.1 with GNOME on kernel 4.10.5.
The only issue I see is that Google's login (split over two pages) does not auto-fill on the first page, i.e. I have to manually add in my user name, then on the second page I have to manually click 're-scan credentials fields' for it to work.
multi-page logins will likely never be supported.
On Mon, Mar 27, 2017 at 1:30 AM Taijian notifications@github.com wrote:
Works fine for me on FF 52.0.1 with GNOME on kernel 4.10.5.
The only issue I see is that Google's login (split over two pages) does not auto-fill on the first page, i.e. I have to manually add in my user name, then on the second page I have to manually click 're-scan credentials fields' for it to work.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pfn/passifox/pull/595#issuecomment-289387393, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfQxeR7oYuX4sThJvIUPPyzyy7PFaIXks5rp3OQgaJpZM4MC5QR .
@pfn What work is still needed to get this PR merged?
@Ivan0xFF From my point of view I'd be happy for this to be merged now, and I'll open an issue to track the contextual menu thing.
If it stays open for a few more days then over the weekend I was hoping to try and track down the occasional error I was getting when selecting "Fill User & Pass". Although it seems like it may require some special combination of events to reproduce, as I've only seen it once or twice. So I don't think it's a showstopper.
In the end it's all up to @pfn as maintainer, of course. :)
(I've checked "test for broken Chrome functionality" off my list as I've put Chromium through its major paces with this PR. I think if there are more subtle bugs they'll only be found once a larger number of users start using it, unfortunately.)
Thanks, I'll look at getting this reviewed and merged soon.
On Wed, Mar 29, 2017 at 3:11 PM Angus Gratton notifications@github.com wrote:
@Ivan0xFF https://github.com/Ivan0xFF From my point of view I'd be happy for this to be merged now, and I'll open an issue to track the contextual menu thing.
If it stays open for a few more days then over the weekend I was hoping to try and track down the occasional error I was getting when selecting "Fill User & Pass". Although it seems like it may require some special combination of events to reproduce, as I've only seen it once or twice. So I don't think it's a showstopper.
In the end it's all up to @pfn https://github.com/pfn as maintainer, of course. :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pfn/passifox/pull/595#issuecomment-290242231, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfQxZJlUsqtqDo6F3TD5ibu8hbQbhLiks5rqtcJgaJpZM4MC5QR .
I just tried @Ivan0xFF's version. It works with all of my daily sites. Awesome!
Just noticed that there are quite a few errors like this in the terminal:
JavaScript error: jar:file:///home/yen/.mozilla/firefox/468widst.default/extensions/%7B5affb487-fdda-435f-b4b8-2bfdba33e51f%7D.xpi!/chromeipass.js, line 1142: TypeError: cip.init is not a function
Is it harmless or a potential sign for a bug?
There's another issue: in the legacy Firefox addon, there's usually only one KeepassHTTP confirmation dialog for an password field. With this version, a KeepassHTTP confirmation dialog jumps out whenever a web page loses focus and gets focus again. This is somewhat annoying.
You are free to look at the changes I made to keepassxc-browser. Basically it's chromeIPass but with several changes. External scripts are updates, functions are asynchronous, and the best part; it's WebExtension compatible. I also noticed the bug with context menu access keys. Temporarily I removed them but going to put them back.
@varjolintu Mhh, doing duplicate work is not really good. Also this extension here is looking for a new maintainer (https://github.com/pfn/passifox/issues/593) and you both could possibly do that. And if this extension will be replaced by a WebExtension fork, then that's also good, but currently we have a legacy extension with a (nearly) complete PR for a rewrite to a WebExtension and a new fork with the same capabilities.
For a user the current state is really confusing and I don't really know what to use…
@rugk It's confusing yes. When I started my fork my main goal was just to implement Native Messaging to the extension. Then I saw all the deprecated API functions, old libraries etc.. The main goal was not to replace chromeIPass but to get rid of KeePassHTTP. This official plugin is still the best for KeePassHTTP supported stuff.
And there's also a guy who made a fork of my fork and he's trying a complete rewrite.. More confusing :)
And there's also a guy who made a fork of my fork and he's trying a complete rewrite.. More confusing :)
Oh, yeah…
This official plugin is still the best for KeePassHTTP supported stuff.
And which one is official?
@rugk In my opinion these (chromeIPass and PassIFox) are the official ones.
@Ivan0xFF
I gave the one you attached a try in Firefox. It installs and runs, but it doesn't seem to be able to connect to KeePassXC. I'm (stuck) using Firefox ESR 52.x. I'm not sure if that's related or not.
I made a quick WebExtension transform to the plugin
I haven't paid much attention to this discussion lately (sorry), but I don't really understand why you did this rather than expanding the work that's already here in this PR. Can you explain?
Yet another fork…
I haven't paid much attention to this discussion lately (sorry), but I don't really understand why you did this rather than expanding the work that's already here in this PR. Can you explain?
I'm sorry, you are absolutely correct. There's no need for such repo. So never mind.
Is the last Firefox bug the main reason this pull request haven't been accepted yet?
Is it possible to accept this PR without the "Context menu access keys..." feature ? Does it block the main functionality of the addon? I suppose that this PR could be useful in the current state (multiprocess, FF 57+). Seems like mozilla is not going to fix the bug with context menus in the near future.
Been using the extension provided in https://github.com/pfn/passifox/pull/595#issuecomment-286599824 with nightly and no problems so far.
The one in #595 doesn't seem to be working right in Firefox ESR 52.2.1 (which I need to keep using for the time being for XUL addon compatibility)
It installs and runs, but it can't seem to connect, where as the one on AMO does.
It installs and runs, but it can't seem to connect, where as the one on AMO does.
Can you give any more details about what happens, for example exactly what text appears in the Chromeipass popup when you click the icon?
If you go to about:debugging, check "Enable Add-On Debugging", then click "Debug" next to chromeIPass, do you get anything much in the Console tab?
(The only time I've seen connecting failing, ie popup frame gets stuck at "Updating Status...", is when I had a proxy configured which wasn't being bypassed for localhost, but there are probably other cases..)
@projectgus I can't seem to replicate it since upgrading the ESR to the latest 52. I'm thinking (hoping) it was bug in Firefox itself they fixed.
I'm having other issues with it not filling on some pages, but I think that's related to the specific pages rather than the original issue, and would probably be better addressed in a separate issue once a newer release comes out.
So for now, I rescind my original issue, since I can't replicate it myself with a fresh copy of everything.
The signed version above seems to work great, been using it for about a day. Just switched to Firefox as my daily driver, currently on Nightly 57.0a1 (2017-08-14) (64-bit).
sorry if this muddies the water more, but i was having issues getting the above signed extension to work in waterfox (a firefox fork). i made a small change which seems to fix the issue, that you can see in my forked repo from @projectgus's hard work.
i also noticed that firefox added support for the onAuthRequired callback in firefox 54, so I made some changes, which appears to work (with my own limited testing).
i've attached a signed xpi for testing.
also, @pfn i would be interested in discussing taking over as the maintainer for passifox & keepasshttp, if you're still looking for someone. feel free to e-mail me!
@smorks not sure if this is limited to only your extension, but the key icon to generate a password is not coming up for me, at least not on https://www.mozilla.org/en-US/firefox/accounts/
@ibrokemypie does it work on other pages? maybe the github login page? i'll do some more investigation to see if i can see why it's not working there.
Some pages use a div that's hidden until you press a login button. This means the browser extension ignores those forms until they are visible. Currently there's no feature for redetecting the form fields automatically/continously.
With the current build(s) the only thing that works is to use a keyboard shortcut or fill the user/password combo through the context menu.
@smorks I'm not able to install your extension in waterfox 55. Waterfox claims it's corrupt.
@hultberg Have you try to unzip it first?
@hultberg have you tried the one from my releases page?
@thbkrshw Yes, of course.
@smorks Your downloads on the release page worked!
@smorks Thank you for keeping this addon alive.
Until yesterday, I was still able to use Passifox 1.2.2 with Firefox 55.0.3 32 bit on a Win 7 64 bit machine. However, now Firefox crashes every time Passifox tries to fill out an HTTP Auth login dialog (on HTTPS), so I needed a replacement.
I installed your chromeipass-2.8.4-an.fx.xpi and it works great. The only problem that I have noticed so far is that the settings - including the pairing with KeePassHTTP - is lost every time I restart my browser. Is it possible that you need to add the "storage" permission to your manifest.json?
Thanks again for your great work.
@davelit i've opened an issue in my repo.
anyone who has issues with my extension, please open issues in my fork, just so this thread doesn't get any more cluttered than it needs to be.
@smorks @pfn Is it helpful if I merge your changes into this PR branch as well? They seem great.
I'm not sure what the path forward from here is, exactly.
Some pages use a div that's hidden until you press a login button. This means the browser extension ignores those forms until they are visible. Currently there's no feature for redetecting the form fields automatically/continously.
@varjolintu I only use Chromium a little bit, but from what I recall it seems like this is an issue there as well? Or is it only an issue when running the WebExtension on Firefox?
@projectgus i'm fine with merging my changes into this PR, not sure if it will help with moving things along at all.
Cool. Well, this PR branch is now level with your 2.8.5 release tag. I left out the most recent commit which changes README URLs, as if this PR is to be merged then we don't want that.
The other change which we may not want here is the new addon id. But I'm not going to bother changing it back until we know what @pfn plans to do.
(BTW, @smorks, I think the idea of you taking over as maintainer provided this works for @pfn.)
@projectgus It happens with both browsers. I have an idea how to solve it but haven't tested it yet. Basically you could monitor any hidden divs that contain forms and do a re-detect when any of those hidden div's are visible again.
@smorks & @projectgus I really like these changes you've been making. If it's ok to you I would like to merge some of those to keepassxc-browser (and give you a credit of course).
@varjolintu If it's not Firefox-specific then probably best to open a separate issue to discuss it. I agree it would be nice to fix if possible (I do a lot of clicking on the chromeipass icon and saying "Redetect credential fields").
Regarding merging my changes, I have no problems with them being merged anywhere they are useful.
@varjolintu i'm fine with you merging my changes too.
Will this ever get merged?
As discussed in #563. Allows Electrolysis (multi-process windows) feature to function in recent Firefox, and also provides a path for support once XUL-based addons are disabled in Firefox.
Support is not quite as tightly integrated as it was with passifox (ie there's no integration to Firefox's own "Fill Password" functions any more), but it seems very usable all the same.
Still a work in progress:
chrome.xxx
APIs to usebrowser.
. Some chrome APIs are implementated in Firefox and work as-is, but a few (such aschrome.extension.sendMessage()
are not. Seemed cleaner to change them all over.browser.xxx
. Needed to cherry-pick some PRs before things came good (details in commit message). The basics seem to work.webRequest.onAuthRequired
(yet?) so can't fill HTTP auth popups from Chromeipass. Need to at least re-enable for Chrome, and hide the relevant Settings on Firefox.localStorage
whenever I restart the browser. Firefox production builds don't allow unsigned addons at all any more, except temporarily.Also, I don't know if this would be a good time to change the name? Or even if you're interested in adding this support at all - given it's a fairly major change to what seems like a stable Chrome extension.