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

New entry screen causes loss passwords during entry creation #870

Closed
t4777sd opened this issue Aug 14, 2017 · 6 comments
Closed

New entry screen causes loss passwords during entry creation #870

t4777sd opened this issue Aug 14, 2017 · 6 comments

Comments

@t4777sd
Copy link

t4777sd commented Aug 14, 2017

Currently when you create a new entry you get a blank password box. You click on the icon for the password generator. It has a password box that says "password" that is filled in. You would think that the filled in password would become the password for the entry. However, that is not the case. If you click "Apply", you will then be creating an entry with an empty password when the entry should have the password that was filled in the "password" box in the password generator.

Expected Behavior

In KeePass, I am 99% sure it does not function this way. When you are in the password generator with an empty password, the password the generator comes up with will be the password that gets assigned to entry. It will not be an empty password (forcing you to copy the password from one "password" field to another "password" field). Hitting apply, when the password box is empty, should assign the password that is in the "password" field in the generator. The fact that it is named "password" and filled-in on the entry-form indicates that it will become the password.

Possible Solution

  1. Best update header file for Qt5 #1: Hitting apply with an empty password field and an open generator form with a filled in "password" field will assign the password in the generator to the entry

  2. Best "KeePassHttp is not running" error in Pass|Fox #2: Password generator should be its own popup window. Hitting Apply for password generator will fill in the password for the entry (This is how keepass works). This avoids the confusion caused by having a form within a form which makes all context ambiguous.

  3. Minimum: At a minimum, if "apply" is hit with an empty password field there should be a warning confirmation box that says "No password has been entered for this entry. Are you sure you want to save it?"

Context

This already cost me pretty big when I created an entry, copying the password in the generator, clicking apply assuming the entry would have the password the generator just generated especially because the form now had a filled in "password" (with the generator open there are now two "password" form fields. Having a form within a form makes context / behavior ambiguous) and I then lost access to my bitcoin wallet with no way to recover it. So, this issue is pretty serious that can cost people lots of money.

Debug Info

KeePassXC - Version 2.2.0
Revision: 9a7e685

Operating system: Linux

@droidmonkey
Copy link
Member

There is a problem with your implementation. If you open an entry with a password, open the password generator, then hit apply you will do the exact opposite of your concern... It will overwrite the existing password and lock you out. Although it will likely be saved in the history.

There is an apply button on the generator window that pops up. Another good option is to ask the user if they are sure they want to close the new entry editor if a password hasn't been set. Especially if the generator window is open.

@t4777sd
Copy link
Author

t4777sd commented Aug 15, 2017

Okay, I have remade some passwords to figure out what happened. I guess it was some user error, but I think it was due to having a "form within a form". So, while I am an idiot, it may be many other people would be tricked in the same way.

When the password generator opens it opens within the same context as the password form. It would be like websites having an "advanced options" menu with two "submit" buttons for the form. To me, it is confusing to have two separate "submit" buttons in the same context.

So, when I opened the password generator, I saw the "password" field was filled in (the one that the generator has). I didn't see the secondary "apply" buttons that are within the generator itself. Instead, I hit "Apply" that is in the bottom of the screen attempting to save the entire entry (password generated, I saw it generated because "password" field was filled in, so I wanted to apply the entire entry to save it). Then, when that did nothing, I hit OK. That is when the password was lost.

I understand that there was a lot of user error in there. However, I think it is because the UI design of having two separate forms on one single form page is not an intuitive model. It is better to have the Password Generator popup in its own context (a window) which is what Keepass does or at least give a warning to the user when they exit a password that has no password assign.

@droidmonkey
Copy link
Member

I can agree with the warning

@pimeys
Copy link

pimeys commented Dec 7, 2017

I agree too. Today I lost some cryptocurrency because of the user interface here. I created a new entry, generated a password, copied it and used it to encrypt my wallet, accidentally clicked OK instead of Apply.

I know, my mistake. But the UI here should either warn you or the generator should keep track on the last generated passwords.

@t4777sd
Copy link
Author

t4777sd commented Jan 28, 2018

I am glad I am not alone in this mistake. If you can believe, it happened again today. Luckily with no money involved, but it seems to be a UI quirk that is easy to fall trap to.

@phoerious
Copy link
Member

Resolved in #1499

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

4 participants