keepassxreboot / keepassxc-browser

KeePassXC Browser Extension
GNU General Public License v3.0
1.77k stars 187 forks source link

Continuous access browser requests #1794

Open WhyNotHugo opened 1 year ago

WhyNotHugo commented 1 year ago

Expected Behavior

KeePassXC should only prompt to grant access to credentials when required.

Current Behavior

When I switch to a tab for example.com, if I have credentials saved for example.com, I immediately get a "Browser Access Request" prompt from KeePassXC due to the extension requesting access to credentials.

This often happens when there's a login field in view, but sometimes there's no login field visible on the page, and this still happens.

Dismissing the access request, switching to another tab and back result in yet another prompt.

Possible Solution

Some possible solutions:

Steps to Reproduce (for bugs)

  1. Open any domain for which there are credentials saved.

For a few domains, this doesn't seem to happen, but I can't find an obvious pattern.

Debug info

KeePassXC - Version 2.7.4
Revision: 63b2394

Qt 5.15.7
Debugging mode is disabled.

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 6.0.9-arch1-1

Enabled extensions:
- Auto-Type
- Browser Integration
- SSH Agent
- KeeShare
- YubiKey
- Secret Service Integration

Cryptographic libraries:
- Botan 2.19.3
varjolintu commented 1 year ago

This is clearly related to some fields that are hidden but the extension still finds them. It would help a lot to find a page where this can be reproduced every time.

WhyNotHugo commented 1 year ago

Darn, I can reproduce this on my insurance provider's page (but the page is not public; you have to log in to get there), or other similar non-public pages.

Lemme find a public one where I can reproduce and report it back. I'm realising that the issue is a two part though. Half of the issue is the hidden login field. The other half is being prompted repeatedly.

For this last scenario, you can repro with:

It seems that any time a tab gains focus there's a new prompt, even if there's been no navigation or interaction with the tab.

varjolintu commented 1 year ago

What's the value in your "Clear credentials from non-active tabs after (seconds, 0-3600)." setting? If that's zero, then it's totally normal behavior. The default is ten seconds, so there shouldn't be another request for the same tab if you switch back to it before the timout is met.

WhyNotHugo commented 1 year ago

I found an example of a site that prompts over and over even with no visible login fields:

https://app.satoshitango.com/settings

This site does allow public registrations, AFAIK.

varjolintu commented 1 year ago

That site also triggers the request only after the timeout has passed. Did you take a look at your setting?

b0ssi commented 1 year ago

Hi there, same issue here. To me, the slightly confusing part is that the KeePassXC - Browser Access Request popup does offer to Remember the decision made, which I, as a user, would interpret as making the decision (to either allow or deny this domain/URI to selected entries) fully persistent, overriding any additional timeouts or limits set elsewhere. This also used to be the behavior until a bit over a year ago.

NB: KeePassXC does have a configuration entry for those that I have tested along the lines of: key: KeePassXC-Browser Settings value: {"Allow":["<domain>","<IP>"],"Deny":[],"Realm":""} I have not confirmed, however, whether or not this configuration is currently (re)created - i.e. whether or not KeePassXC/Browser Integration actually uses it.

From discussion above I currently infer that either the Clear credentials from non-active tabs after (seconds, 0-3600). setting (in the browser extension's General section) overrides this or the the Remember feature to make the decision persistent is used no longer, though still offered in the popup.

It looks like there is some room for clarification/improvement here, either way. Any thoughts would be much appreciated, thanks!

varjolintu commented 1 year ago

@b0ssi That option is persistent, until Plugin Data is removed from the entry.

b0ssi commented 1 year ago

@varjolintu Thanks for confirming. I just tried to reproduce behavior by deleting the KeePassXC-Browser Settings k:v pair, saved and refresh corresponding website:

Can you confirm that this is intended overriding behavior - proxy requests to "always-permit-for-given-context" but expired entries getting blocked?

varjolintu commented 1 year ago

@b0ssi Relevant option in Browser Integration settings in KeePassXC: [ ] Allow returning expired credentials. Plus https://github.com/keepassxreboot/keepassxc/pull/9136.

b0ssi commented 1 year ago

Thanks @varjolintu. I have Allow returning expired credentials enabled, so that isn't it. The two bugs addressed by the PR you reference above sound to be in the neighborhood of this but ultimately unrelated.

Is your response above implying that this should work, even for the one specific case I have isolated this to (entry configured to always return automatically for given URI but expired, blocking auto-return of entry, i.e. confirmation dialog keeps popping up; changing entry to not expired solves the issue)?

varjolintu commented 1 year ago

@b0ssi After looking at the code it seems this is an intended behavior. If your entry is expired, confirmation dialog will be shown every time, to make sure you really want to use an expired credential.

The relevant part: https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserService.cpp#L915

b0ssi commented 1 year ago

Great, thanks for confirming @varjolintu. The reason I was trying to get to the bottom of this is that I thought this might seem a tad misleading from a user's point of view: To my mind, this additional check you're confirming here stands a bit in conflict with the application's Allow returning expired credentials and introduces ambiguity.

Would it add to clarity & ease of use (without taking away any safety precaution) if there were one single setting governing whether or not expired entries shall be allowed to be returned, instead of having two disparate settings on separate ends (application/browser extension) that also behave inconsistently (application: explicit; browser extension: implicit)?

I would suggest to consolidate these two, e.g. have the application dictate behavior through the Allow returning expired credentials setting and the browser extension adhere to it without applying its own ruling, especially if it's implicit, this is quite confusing as it's explicitly unexpected.

Am I missing a conceptual detail or use case that would justify the current behavior?

varjolintu commented 1 year ago

@b0ssi I think we should change the behavior that expired credentials are not asked for the permission if it's already allowed. After all the browser extension will show [Expired] text in the extension popup and Autocomplete Menu. So it's already clear for the user that this credential is actually expired.

b0ssi commented 1 year ago

@varjolintu exactly, sounds like we're on the same page. I think that would de-mystify this use case without any compromise; and yes, the [Expired] tag that's already flagging the entries only aids in that effort. Many thanks on behalf~ :+1:

varjolintu commented 1 year ago

@b0ssi Fixed this here: https://github.com/keepassxreboot/keepassxc/pull/9595. I did not refer to this issue in the PR because this was originally opened for another bug.

b0ssi commented 1 year ago

Cheers @varjolintu. Apologies if this ended up hijacking a related but different issue - hope it didn't get in the way. If it helps with accounting I could open a new issue for this bug, though it sounds like it might not be necessary; let me know.

varjolintu commented 1 year ago

@b0ssi No need. I should've directed you to open a separate issue earlier.

6XGate commented 10 months ago

I would think that KeePassXC should not present that dialog after it's been dismissed the first time during a tab's lifespan.