keepassxc-browser icon indicating copy to clipboard operation
keepassxc-browser copied to clipboard

Credential fill button appears at invalid location, never disappears

Open oliversalzburg opened this issue 4 years ago • 48 comments

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:

  • Auto-Type
  • Browser Integration
  • SSH Agent
  • KeeShare (signed and unsigned sharing)
  • YubiKey

Cryptographic libraries: libgcrypt 1.8.5

Operating System: Windows

oliversalzburg avatar Jul 15 '20 11:07 oliversalzburg

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

varjolintu avatar Jul 15 '20 11:07 varjolintu

@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.

oliversalzburg avatar Jul 15 '20 12:07 oliversalzburg

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

varjolintu avatar Jul 15 '20 13:07 varjolintu

That's the new Chrome-based Edge.

oliversalzburg avatar Jul 15 '20 15:07 oliversalzburg

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

varjolintu avatar Jul 15 '20 15:07 varjolintu

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:

  • Auto-Type
  • Browser Integration
  • SSH Agent
  • KeeShare (signed and unsigned sharing)
  • YubiKey

Cryptographic libraries: libgcrypt 1.8.5

oliversalzburg avatar Aug 24 '20 08:08 oliversalzburg

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

varjolintu avatar Aug 24 '20 09:08 varjolintu

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

oliversalzburg avatar Aug 24 '20 09:08 oliversalzburg

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

varjolintu avatar Aug 24 '20 09:08 varjolintu

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.

oliversalzburg avatar Aug 24 '20 09:08 oliversalzburg

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

varjolintu avatar Aug 24 '20 09:08 varjolintu

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 avatar Aug 24 '20 09:08 oliversalzburg

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

  • Cannot connect to KeePassXC. Check that browser integration is enabled in KeePassXC settings.
  • Key exchange was not successful.

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

oliversalzburg avatar Aug 25 '20 06:08 oliversalzburg

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

varjolintu avatar Aug 25 '20 07:08 varjolintu

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.

oliversalzburg avatar Aug 25 '20 08:08 oliversalzburg

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

varjolintu avatar Aug 25 '20 08:08 varjolintu

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

oliversalzburg avatar Aug 25 '20 09:08 oliversalzburg

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.

oliversalzburg avatar Sep 02 '20 09:09 oliversalzburg

@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.

varjolintu avatar Sep 02 '20 09:09 varjolintu

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.

oliversalzburg avatar Sep 02 '20 10:09 oliversalzburg

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

varjolintu avatar Sep 02 '20 10:09 varjolintu

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

oliversalzburg avatar Sep 02 '20 10:09 oliversalzburg

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

oliversalzburg avatar Sep 02 '20 11:09 oliversalzburg

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

varjolintu avatar Sep 02 '20 11:09 varjolintu

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.

oliversalzburg avatar Sep 02 '20 15:09 oliversalzburg

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!

varjolintu avatar Sep 02 '20 15:09 varjolintu

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 :/

oliversalzburg avatar Sep 02 '20 15:09 oliversalzburg

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

varjolintu avatar Sep 02 '20 15:09 varjolintu

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

oliversalzburg avatar Sep 02 '20 15:09 oliversalzburg

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.

florealcab avatar Sep 08 '20 08:09 florealcab