New entries can be discarded if user only presses Apply

[wgreenberg]

(using KeepassXC 2.3.3)

Clicking Apply (but not Ok) on a new entry can lead to data loss in some circumstances. For example, if a user is creating a new entry and only presses Apply, they can exit keepass without a modal asking them to save. When the user relaunches keepass, the entry will be gone.

Similarly, if a user creates a new entry and only Applys it, the inactivity lock can come up without a warning modal and cause the entry to be lost.

Expected Behavior

The modals for preventing loss of unsaved data should come up when an entry has only been Applyd.

Current Behavior

A new entry which has been Applyd will not prompt the warning modals when exiting keepass or when the database is locked.

Steps to Reproduce (for bugs)

1. Create new entry w/ some contents
2. Press Apply (but not Ok)
3. Exit keepass (note that no warning modal appears)
4. Re-open keepass, note that the entry is gone

OR

1. Set an inactivity timeout
2. Create new entry w/ some contents
3. Press Apply (but not Ok)
4. Wait for timeout to lock the database
5. Unlock database, note that the entry is gone

Context

I've lost irrecoverable credentials due to an inactivity lock screen. Clicking Apply displays a nice green alert saying "Entry successfully updated", which gave me a false sense of security about my data.

Debug Info

KeePassXC - Version 2.3.3
Revision: 0a155d8

Libraries:
- Qt 5.9.1
- libgcrypt 1.7.8

Operating system: Ubuntu 17.10
CPU architecture: x86_64
Kernel: linux 4.13.0-46-generic

Enabled extensions:
- Auto-Type
- Browser Integration
- Legacy Browser Integration (KeePassHTTP)
- SSH Agent
- YubiKey

[droidmonkey]

oh wow good catch, this is a big deal.

[droidmonkey]

After debugging this it looks like it cannot be fixed without a major refactor. Basically the changes are not notified up the chain to the database because the new entry is not added to the current group until AFTER the EditEntryWidget is closed by the DatabaseWidget. Further, the actual group the entry should belong to is not given to the EditEntryWidget so it can do that job instead (hence the refactor).

I think disabling the apply button in 2.3.4 for new entries is an acceptable option and does not require a refactor. Any thoughts?

[TheZ3ro]

We had many issue with it, what about removing it entirely?

Anyway I agree we should disable it in the meantime

[droidmonkey]

I am going to use this issue to springboard into a refactor of the open/edit/save process for entries and groups in 2.4. The whole chain is very fragile and based on timed/hidden signal/slot calls. Even trying to debug this issue itself took over 2 hours of finding the right place to put the breakpoint because nothing at all is linear about this process, it's also spread across 5 different source files.

[hifi]

Even when it's obvious, I'll just add that lock on lid close / suspend can also trigger this or just manually locking a database while the dialog is open (like Ctrl-L).

Guess who just lost a newly created password when my system automatically suspended on low battery 😉