Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error when inputting incorrect password #813

Closed
mfc opened this issue Jul 23, 2017 · 20 comments
Closed

Improve error when inputting incorrect password #813

mfc opened this issue Jul 23, 2017 · 20 comments
Assignees
Milestone

Comments

@mfc
Copy link

mfc commented Jul 23, 2017

This is a hold-over from KeePassX and now that we have responsive developers it would be great to improve it.

Expected Behavior

When you input the wrong password, it should tell you you inputted the incorrect password.

Current Behavior

When you input the wrong password, you get: Unable to open the database. Wrong key or database file is corrupt. Noticeably absent from that is the possibility you inputted the wrong password. I have found many new KeePassX users confused by this error message and think that their database is corrupt because it is not immediately evident that they may have inputted the wrong password.

Possible Solution

Change the error text, for instance to: Unable to open the database. Invalid credentials provided.

Debug Info

KeePassXC - 2.2.0
Revision: caa49a8

@droidmonkey
Copy link
Member

I agree with this, but not the wording

@mfc
Copy link
Author

mfc commented Jul 25, 2017

updated the example of new wording. obviously feel free to propose your own :)

@louib
Copy link
Member

louib commented Jul 26, 2017

@mfc @droidmonkey what about Unable to open the database. Invalid credentials or the database file is corrupt.. That would cover the password, the key file and even the yubikey.

@droidmonkey
Copy link
Member

Invalid credentials provided...

@mfc
Copy link
Author

mfc commented Jul 27, 2017

Invalid credentials provided

yeah i think not mentioning the database potentially being corrupted would be a good idea. there are a lot of other potential things that may be wrong, but they are all highly unlikely compared to invalid credentials being provided.

@droidmonkey
Copy link
Member

The message might add "database is corrupt" at the end after 3 failed tries.

@phoerious
Copy link
Member

I would add it add first try, just not as the first thing.

@macau23
Copy link

macau23 commented Aug 31, 2017

Telling an end user that their database might be corrupt when they enter the wrong password is terrifying for them.

Can we not distinguish between a corrupt database and an incorrect password?

@phoerious
Copy link
Member

No, we can't.

@macau23
Copy link

macau23 commented Sep 5, 2017

Could we? :)

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Sep 5, 2017

@macau23 No, If you try to decrypt a database with a wrong password, the decryption will work just fine but the output will be just garbage, the same thing happen if you use a correct password on a corrupt database, garbage output

@macau23
Copy link

macau23 commented Sep 5, 2017

So there can't be a checksum on the encrypted database?

@mfc
Copy link
Author

mfc commented Sep 14, 2017

I think the simplest solution is just improve the language of the error text. I think @droidmonkey's suggestion works best: Invalid credentials provided.

If the database is indeed corrupt, the user is not going to know what to do with that information anyway, and will need to go online to search for information about it. If you want to add database checksum functionality I see that as an enhancement, and if it is seen as worthwhile it can be implemented at some point. But that should probably be a separate ticket.

For a simple solution to this big UX problem I think changing to error text to Invalid credentials provided is the appropriate step forward.

@droidmonkey droidmonkey added this to the v2.3.0 milestone Sep 15, 2017
@droidmonkey droidmonkey self-assigned this Sep 15, 2017
@phoerious phoerious modified the milestones: v2.3.0, v2.4.0 Jan 28, 2018
@lindhe
Copy link
Contributor

lindhe commented Feb 24, 2018

Since it is hard or impossible to distinguish between a corrupted password file or a password file which you provide incorrect credentials to, I would suggest the following:

  1. The first time KeePassXC opens the file, write something sensible like "Could not open file with supplied credentials." if the user fails. Otherwise, open the file.
  2. After the file is opened, make a hash of the encrypted file, so that we know a fingerprint of the latest version of the file which we know is valid. This hash needs to be updated each time we write to it.
  3. Cache the file along with the filename (you already save a list of recently opened files; let's keep it at the same place or wherever).
  4. Close the file.
  5. The next time we try to open the file, if the hash saved matches the hash of the current file we know that the file has not been changed or corrupted. If we can not open it, we know for sure (right?) that the user provided incorrect credentials for the file.
  6. If the saved hash does not match the current hash of the encrypted file, it must have been corrupted, or changed by a third party so we are back at step 1.

Since we hash the encrypted file, this should not be a security concern. And since you already keep a list of recently used files, I see no privacy issues either.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Feb 24, 2018

Seems just an overcomplicated solution for a very simple UX problem.
I'm not even sure there is even a problem in the first place.

My 2 cent:

Unable to open the database. Invalid credentials provided, please retry.
If this happen again your database file is likely to be corrupt.

@phoerious
Copy link
Member

I was thinking the same. Keep it simple.

@droidmonkey droidmonkey modified the milestones: v2.4.0, v2.5.0 Jan 16, 2019
@mfc
Copy link
Author

mfc commented Apr 4, 2019

hi folks, given there is agreement on the language change and it is one string (maybe the most important string), could this be bumped up in priority? or could you point me to the part of the code where this exists and I will create pull request? would be nice to have it done within 2 years of it being reported, thanks.

@droidmonkey droidmonkey modified the milestones: v2.5.0, v2.4.1 Apr 4, 2019
@droidmonkey
Copy link
Member

I got it covered, this issue got lost in the forest. Thanks for pinging

@mfc
Copy link
Author

mfc commented Apr 4, 2019

great, thanks @droidmonkey

@phoerious
Copy link
Member

We should also show a warning if capslock is enabled.

droidmonkey added a commit that referenced this issue Apr 5, 2019
* Reduced wording and confusion
* Streamlined delivery format
* Fix #813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants