kee-org / KeeFox

Legacy browser and XUL application integration with KeePass Password Safe. See https://github.com/kee-org/browser-addon for the new version for Firefox 57+
https://forum.kee.pm
418 stars 48 forks source link

Loss of data when saving new entry-must synchronize not save #560

Closed jer-sen closed 8 years ago

jer-sen commented 9 years ago

KeeFox saves the database (replace the existing file by a new one) instead of synchronizing it. KeeFox action after creating a new entry is not the same than a click on the "Save" button in KeePass !

It's a big issue when the database file is synchronized on several computers with tools such as Dropbox, GDrive, Sync, ... Some passwords can be lost !

E.g. : 1) A database file (version 1) is opened on computer A 2) The database file is modified (version 2) on an other computer B 3) The database file on computer A (version 1) is replaced (thanks to Dropbox or equivalent) by the new file from computer B (version 2) 4) A new entry is created with KeeFox on computer A 5) KeeFox replaces the database file on computer A (version 2) with a new one (version 3) : the opened database (version 1) after adding the new entry. Thus, KeeFox discards changes made on computer B (version 2 is lost !).

dlech commented 9 years ago

I don't think this is a problem with KeeFox. If you open the database file in your dropbox folder directly in KeePass, you will have this problem even if you don't use KeeFox. There are lots of discussions on the KeePass forum about how to sync using Dropbox. http://sourceforge.net/p/keepass/discussion/search/?q=dropbox

jer-sen commented 9 years ago

I do think this is a problem with KeeFox or KeePassRPC. After creating a new entry with KeeFox, KeePass save the database without checking if there is a conflict. This dangerous behaviour can not be the expected behaviour...

The expected behaviour is the one described here : http://keepass.info/help/base/multiuser.html

Of course one can use a mirror of the database and set a trigger to synchronize after saving. But it's a question of data safety, simplicity and coherence.

Don't you agree with that ?

dlech commented 9 years ago

I do agree. I also did some digging and the function to prompt the Synchronize or Overwrite dialog is private, which means is it not available to plugins. Technically, there are are ways around this, but it appears that the KeePass author did not intend for plugins to use it.

luckyrat commented 9 years ago

Note that you can disable the auto-save feature from within KeePass / KeePassRPC options. Then you can manually initiate the save using the standard KeePass save feature.

Longer-term we can investigate how to access this synchronisation functionality from within KeePassRPC.

jer-sen commented 9 years ago

Thanks for the advice. I know this option but it is also dangerous, if I forget to synchronize the database after creating a new entry then I can lose this entry.

Really I can't see any drawback, KeeFox/KeePassRPC/KeePass should really synchronize or at least not replace the database file without asking.

Thanks for your work on KeePass and your attention on this issue. I hope it will be more short-term than long-term :)

luckyrat commented 9 years ago

Really I can't see any drawback

The drawback is that this would be going against the existing design of KeePass so we need to make sure we get an excellent understanding of how this feature is supposed to work and why it has been hidden from plugins until this point. Depending on what we discover, we may also be hit with practical problems regarding backwards compatibility with older KeePass versions. I'm pretty confident it'll be safely achievable one way or another but I just don't have time to give the issue the attention it requires at the moment so I thought at least pointing out another possible approach might be helpful in the mean-time.

luckyrat commented 8 years ago

@dlech do you remember where the relevant private method is in the KeePass source code? I should be able to investigate this further at the weekend.

dlech commented 8 years ago

https://github.com/dlech/KeePass2.x/blob/VS2015/KeePass/Forms/MainForm.cs#L615

luckyrat commented 8 years ago

Thanks.

It looks like we can just supply a different parameter to host.MainWindow.UIFileSave to get the desired behaviour. I've only given it a brief test but so far it seems to be asking for synchronisation when required.

jer-sen commented 8 years ago

Thanks a lot ! It works great now :)