keepassxreboot / keepassxc-browser

KeePassXC Browser Extension
GNU General Public License v3.0
1.72k stars 177 forks source link

Credential fill button appears at invalid location, never disappears #945

Open oliversalzburg opened 4 years ago

oliversalzburg commented 4 years ago

Overview

Steps to Reproduce

  1. Open a website where login information can be filled by KeePassXC.

Expected Behavior

The "Fill credentials with KeePassXC" button appears in login fields and allows me to fill credentials with KeePassXC.

Actual Behavior

The "Fill credentials with KeePassXC" button appears in random locations and does nothing when clicked.

Context

The button also appears in valid locations, but it also appears in invalid locations in addition to that.

image

I'm seeing this in the Chrome-based Edge.

KeePassXC - Version 2.6.0 Revision: 0765954

Qt 5.15.0 Debugging mode is disabled.

Operating system: Windows 10 Version 2004 CPU architecture: x86_64 Kernel: winnt 10.0.19041

Enabled extensions:

Cryptographic libraries: libgcrypt 1.8.5

Operating System: Windows

varjolintu commented 4 years ago

This happens with every site? Or just the one with the screenshot?

oliversalzburg commented 4 years ago

@varjolintu I've noticed it on several sites. So far, I've not been able to detect the pattern that causes it. I also can't reliably reproduce it yet, but it keeps happening.

varjolintu commented 4 years ago

What browser are you using? The issue template doesn't reveal that.

oliversalzburg commented 4 years ago

That's the new Chrome-based Edge.

varjolintu commented 4 years ago

That explains some because it's still one version number lower than the latest. Microsoft validation takes days.

oliversalzburg commented 4 years ago

By now I've upgraded and this is still happening on the same page. I haven't noticed it anywhere else though. Might be something weird with pgAdmin.

KeePassXC - Version 2.6.1 Revision: 9a35bba

Qt 5.15.0 Debugging mode is disabled.

Operating system: Windows 10 Version 2004 CPU architecture: x86_64 Kernel: winnt 10.0.19041

Enabled extensions:

Cryptographic libraries: libgcrypt 1.8.5

varjolintu commented 4 years ago

Please test this with https://github.com/keepassxreboot/keepassxc-browser/releases/tag/1.7.0-beta1 Instructions: https://github.com/keepassxreboot/keepassxc-browser/wiki/Loading-the-extension-manually

oliversalzburg commented 4 years ago

Seems to be stuck in a "Key exchange was not successful." state.

varjolintu commented 4 years ago

You'll also have to modify the JSON script to include the temporary extension's ID. Did you do that?

oliversalzburg commented 4 years ago

Yes, I did. I copied the ID prefix from the extensions page with it. That's why it wasn't working.

It's working now, and the behavior regarding this issue is unchanged.

varjolintu commented 4 years ago

Ok. Thank you for testing. I would need to setup an identical page for testing it myself.

oliversalzburg commented 4 years ago

This is pgAadmin and I'm running it through Docker. You may use this docker-compose.yml to start a similar environment.

version: "3.7"
services:
    postgres:
        image: postgis/postgis:10-3.0
        ports:
            - "5432:5432"
        environment:
            POSTGRES_PASSWORD: postgres
            PGDATA: /var/lib/postgresql/data/pgdata
    postgres-gui:
        image: dpage/pgadmin4
        ports:
            - "1234:80"
        depends_on:
            - postgres
        environment:
            PGADMIN_DEFAULT_EMAIL: postgres@localhost
            PGADMIN_DEFAULT_PASSWORD: postgres

The pgAdmin interface will be running at localhost:1234 and use postgres@localhost as login with password postgres. The hostname, username and password of the server will be postgres.

When setting up the integration, password entry will be offered at this point:

image

After confirming the dialog, that overlay icon will stay in its place:

image

oliversalzburg commented 4 years ago

Just FYI, the next day, I can't get the browser plugin to work anymore. It's going through these errors:

Going to revert back to stable for now. It also has the same issues, but usually recovers after a couple of reloads.

varjolintu commented 4 years ago

Switching back and forth with a temporary extension can cause that sometimes. Making reloads doesn't sound normal with the stable version.

oliversalzburg commented 4 years ago

I didn't switch back to the old version though. I had the stable browser extension still disabled, the beta enabled. Couldn't get it to work quickly enough and reverted back to stable.

I will open a new ticket about the reload behavior. So far I haven't been able to fully understand the pattern, but it happens regularly. I wake my machine from sleep, unlock KPXC, open my browser and the connection to the extension isn't there (red X on the icon). I reload it, it fails, I reload it again, connection established. It always goes something like that.

varjolintu commented 4 years ago

Ah. The sleep causes this. For some reason it cannot recover from that situation without a reload.

oliversalzburg commented 4 years ago

Ok, cool. Glad I didn't discover any new issue 😄 Sleep is causing other issues too, I'll manage 😊

oliversalzburg commented 4 years ago

So the textbox for the password input remains in the DOM. It seems like the modal is only shown/hidden through CSS.

image

Most relevant here appears to be the .ajs-hidden class.

varjolintu commented 4 years ago

@oliversalzburg I know where the bug is but still haven't figured out how to fix it properly. Have you already tried the new 1.7.0 update? It included one more change for CSS style changes.

oliversalzburg commented 4 years ago

Okay, then I won't post more information :)

I had reverted back to the stable release for now. I'd be happy to verify the 1.7 release, but the previews were causing me too much trouble.

varjolintu commented 4 years ago

The 1.7.0 is already officially released. It should update to your browser very soon.

oliversalzburg commented 4 years ago

Excellent. I'll keep watching for it. Edge always takes a while to my understanding :)

oliversalzburg commented 4 years ago

I'm still seeing this behavior in 1.7.0 on Chrome. I'd assume it would be the same on Edge.

varjolintu commented 4 years ago

If you want to debug it quickly, put a breakpoint here before you close the password dialog and see how many mutations there are, and where it goes, if possible: https://github.com/keepassxreboot/keepassxc-browser/blob/develop/keepassxc-browser/content/keepassxc-browser.js#L1601

oliversalzburg commented 4 years ago

It identifies 8 mutations at this time. Some of which relate to the dialog wrapper that is being hidden. You can see the dialog and the contained password field in this DOM snapshot:

image

It will enter kpxcObserverHelper.handleObserverAdd() where it will bail out because inputs.length === 0 after kpxcObserverHelper.getInputs(target);. The input.length === 0 is a result of the if (!ignoreVisibility && !kpxcFields.isVisible(field)) check.

Not sure if that helps.

varjolintu commented 4 years ago

So the line 1621 is where it triggers? This problem might be related to this issue: https://github.com/keepassxreboot/keepassxc-browser/issues/850

And thanks for the help!

oliversalzburg commented 4 years ago

I put the breakpoint at line 1601 per your instructions and followed it to 1621 and further.

The check in kpxcFields.isVisible also seems fine to me. When the dialog is closed, the password form field is passed into it and correctly identified as being invisible (elemStyle.visibility === 'hidden').

I would have to follow an entire correct code path to understand what's wrong, I guess :/

varjolintu commented 4 years ago

Maybe I'll install docker etc. when I have the time and try to test this myself.

oliversalzburg commented 4 years ago

I just realized you can repro this at https://www.pgadmin.org/try/ :)

florealcab commented 4 years ago

It seems appear when the navigation is done with javascript, so there is no full page reload. It's the case on pgadmin, and I have the same problem with another website which change page through JS and without full page reload (it's a private website at work, so I can't share it)

I have the same issue on Firefox 80 on Ubuntu 20.04 OS.

oliversalzburg commented 4 years ago

@florealcab To my understanding, the extension is designed for this scenario, but there are just a lot of different variations to show/hide elements. It requires a very sophisticated approach.

oliversalzburg commented 3 years ago

I looked into this again, and it appears like it just runs into the section commented // There's an issue here. We cannot know for sure if the class attribute if added or removed.. Which seems to pretty clearly identify the issue, as this is invoked when the element is removed.

varjolintu commented 3 years ago

@oliversalzburg That's exactly the reason. The only way it could be identified if the class was added or removed is to check it with getComputedStyle() to get the calculated CSS values. The problem is that it's a very slow function and can possibly affect the page loading times. So far I haven't been able to do that otherwise.

almereyda commented 3 years ago

This also happens with the Lantronix Spider Duo KVM console e.g. at Hetzner https://fsn1-kvm50.hetzner.com/

Screenshot 2021-07-31 at 12-17-48 Spider Duo Authentication

Bildschirmfoto von 2021-06-16 01-43-35

Gael-Mifsud commented 1 year ago

Hello,

The same problem arise in the interface of the Orange Box (V4 to V6), on Firefox (80 to 114). After the authentication, the keepassxc icon stay in the middle of the screen, and disappear only if I reload the page.

uphlewis commented 12 months ago

This seems to happen in several SPA (single page application)s. Just tested our own and the username input field is indeed removed from the DOM upon successful login, but the keepass icon persists above all the content until a full page refresh.

eNTi commented 6 months ago

This is still an issue for me too. I'm just installing stuff on my ZimaBoard / CasaOS and the green login icon sticks in the middle of the screen. Is there no way to fix this without a full page refresh?

varjolintu commented 6 months ago

This is still an issue for me too. I'm just installing stuff on my ZimaBoard / CasaOS and the green login icon sticks in the middle of the screen. Is there no way to fix this without a full page refresh?

If we cannot detect the icon disappearing in JavaScript side (might be a pure CSS change), then it's quite difficult to fix.

lindhe commented 1 month ago

How about making the icon go away when "log in" is pressed? Would that be possible to detect?

Lacking that, maybe we can add a keyboard shortcut to manually hide it?

I do not have any recent experience with other password browser extensions. Anyone knows of some extension that does this well?

Another alternative might be to design our way around the issue by removing the icon altogether. If it's broken and cannot be fixed, maybe it's not good to keep it.

varjolintu commented 1 month ago

@lindhe There's no 100% reliable way. Adding a separate keyboard shortcut sounds a strange feature.

The root of the problem is that it's hard to detect when input field actually disappears and is not visible anymore. We have multiple detection mechanisms for this, but sometimes they all fail.

lindhe commented 1 month ago

Adding a separate keyboard shortcut sounds a strange feature.

Indeed. So is a floating icon. 😄

We have multiple detection mechanisms for this, but sometimes they all fail.

In my experience, it's more often than not that they fail. Either by putting the icon in the wrong place or by leaving it floating in the middle of the page after login. That's why I suggested we might want to redesign to remove the icon, considering how unreliable it is and that you say that this is a hard thing to detect.

varjolintu commented 1 month ago

Adding a separate keyboard shortcut sounds a strange feature.

Indeed. So is a floating icon. 😄

We have multiple detection mechanisms for this, but sometimes they all fail.

In my experience, it's more often than not that they fail. Either by putting the icon in the wrong place or by leaving it floating in the middle of the page after login. That's why I suggested we might want to redesign to remove the icon, considering how unreliable it is and that you say that this is a hard thing to detect.

The icon provides very useful way of filling credentials, and even reconnecting the extension before fill. In my daily browsing I rarely see a site where the icon remains somewhere after login. The solution for this is to adjust the visibility detection, not removing the icon.

stdedos commented 1 month ago

Also happens at TrueNAS Scale login screen (Vue-based iirc)

droidmonkey commented 1 month ago

@lindhe can you provide sample site this happens on?

@varjolintu perhaps we can setup a periodic visibility check of the known credential fields to make sure they are still there. If gone, then remove the icon. Maybe once per second or slower.

varjolintu commented 1 month ago

@varjolintu perhaps we can setup a periodic visibility check of the known credential fields to make sure they are still there. If gone, then remove the icon. Maybe once per second or slower.

We are already using MutationObserver for the content and IntersectionObserver as extra for the icons. If the input field is hidden in a way that it cannot be detected reliably as visible -> invisible, periodic check might not help. But it could be worth trying.

lindhe commented 1 month ago

@lindhe can you provide sample site this happens on?

Sure! Here are two examples from the top of my head. I'll update this post when more comes to mind.

https://sso.redhat.com

Here the icon floats way to the right for the password field: ![Screenshot 2024-07-24 at 13-43-54 Log In Red Hat IDP](https://github.com/user-attachments/assets/9d6c08c0-f398-4371-a932-b9f200bfdafc)

Rubrik Security Cloud (Polaris)

This is what my instance looks like after I log in: ![Screenshot 2024-07-24 at 14-04-16 Data Protection Kubernetes Rubrik](https://github.com/user-attachments/assets/2e7ba57b-092d-4301-9ba8-2cfa454d57eb)

I'm using Firefox 128 on Windows 11.

varjolintu commented 1 month ago

@lindhe With RedHat's login site the icon moves to the password input field when I tried to reproduce the problem. I was using the latest Firefox release. Have you enabled the Username-Only option for the site?

oliversalzburg commented 1 month ago

While I appreciate any work on similar issues as my original one, could we maybe close this one and track new issues in new tickets? It's been 4 years and things have changed a lot.