keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.96k stars 1.45k forks source link

Ability to disable explicit delete before overwriting the database file #1874

Closed bahchis closed 2 years ago

bahchis commented 6 years ago

When syncing to Google Drive, the regular drive behavior is to create a new version of the file if a file with the same name exists.

I was not able to achieve the same behavior using safe deletes and without them. From what I noticed, KeePassXC explicitly deletes the database file before moving the newly saved version, resulting in the old file in the trash instead of having another version of the file.

An option to disable the delete could also help use other storage cloud providers (e.g. Dropbox) in a more native way regarding the file history.

Expected Behavior

When the database is saved in a synced folder, a new file version created.

Current Behavior

When the database is saved in a synced folder, an entirely new file is created and the old is moved to trash.

Possible Solution

When moving the temporary database file, have an option to disable explicit delete of the database file and let the filesystem/sync process handle it.

Context

  1. A potential danger of losing the database if something goes wrong during mid-sync and the trash is emptied.
  2. The official Google drive sync client sometimes see files as "filename - copy 1" instead of syncing the original file. Makes it difficult to keep track of the up to date file.
  3. Hard to work in teams.

Operating system: Ubuntu 16.04 CPU architecture: x64 Kernel: 4.13.0-38-generic

Enabled extensions:

droidmonkey commented 6 years ago

The only other option we have at this time (with the state of the current Qt distributions) is to open the database file and overwrite the contents. This is SUPER unsafe saving as any hiccup, crash, or whatever will result in database corruption. The current unsafe saving feature makes sure all writing is completed successfully before replacing the existing database file.

phoerious commented 6 years ago

What would have been the alternative? Wait and let people keep complaining about truncated databases?

phoerious commented 6 years ago

You shouldn't use unsafe saves on Linux when you don't need it. When you want to know why it leads to truncation, I suggest you read the description of the Qt bug. QSaveFile uses a temporary file and when it fails to move it into place of the original file, it just silently leaves a truncated file.

droidmonkey commented 6 years ago

Geez pedantic much? @rockihack the unsafe save feature is disabled by default and a user configuration option. I'm not sure why that is a problem. I'll have you know one of the main reasons they even fixed the QSaveFile bug is I stirred the pot up over there on the qt board (notice that it lay dormant for about a year).

bahchis commented 6 years ago

Hi @droidmonkey I understand that there are some constraints like bugs that could make this feature "unsafe".

The question I have, Instead of deleting can we just copy the file from the temp name with overwrite option of some sort and let the OS file system do the rest? From what I understand, Keepassxc never writes to the destination file directly.

Btw, this is merely a suggestion. Any other methods that will make cloud storage such as Google Drive treat the new file as a version of the previous file, will do as well.

Thanks

droidmonkey commented 6 years ago

When you overwrite a file, the OS is deleting the old one and moving the new one in place in the background. The only way around that is to open the existing file for writing and dump the new file's contents in. This is pretty risky and from an OS perspective many more steps then delete and "move".

bahchis commented 6 years ago

Hi @droidmonkey On Linux, when a file gets overwritten, it's filename points to a new inode but the old inode remains until nothing is using it anymore.

In my case, I am using Ubuntu and https://github.com/astrada/google-drive-ocamlfuse to sync to Google Drive. I did a small test to see if those tools are able to handle file versions correctly:

echo "version 1" > /tmp/test_versions.txt
cp /tmp/test_versions.txt /my/google/drive/folder/test_versions.txt
echo "version 2" > /tmp/test_versions.txt
cp /tmp/test_versions.txt /my/google/drive/folder/test_versions.txt

I ended up having 1 test_versions.txt file with 2 version in Google Drive.

I think MacOS is behaving the same, but I am not sure how Windows handles it and I am too lazy to check atm. I am not saying it should be the default behavior, but I do believe there should be an option to let the OS file system and the sync tools do their work without forcing a certain flow.

I think this will enable a better intergration with 3rd party cloud storage providers and frankly speaking, the sole drawback so far compared to using paid password solutions.

Hope this makes sense.

droidmonkey commented 6 years ago

Yes makes a lot of sense. I may be incorrectly deriving my knowledge of file system functions from Qt which does not let you simply replace a file. (Ie, the rename function fails if the file already exists)

bahchis commented 6 years ago

I see @droidmonkey I think I have figured out the problem. I ran the same test as above with mv commands and the exact behavior of the file being moved to trash happens. It is because of the mv changing the inodes (as described above) in the Google Drive folder, I saw they are different while when copying they are not. So it must be a copy and not move.

So I suggest the following:

  1. No change to the default behavior of "safe saves"
  2. Change only the "unsafe save" to use copy instead of rename.
  3. Document (super scary) to make sure people understand that it is intended for file systems that support revisions, like Google Drive. I think this way it should be safe enough since they will always be able to restore the previous version.

Unfortunately, it seems that Qt will not help here either since it won't overwrite files in any case: http://doc.qt.io/qt-5/qfile.html#copy

Can we check if using the C copy method can work in this case?

Thanks.

bahchis commented 6 years ago

@droidmonkey do you think this is something worth doing?

Thanks

droidmonkey commented 6 years ago

It might be worth it, using basic c file commands is a pain mixed with Qt though.

bahchis commented 6 years ago

Hi @droidmonkey How do you believe this could make progress and when?

Thanks.

4javier commented 5 years ago

I second this one because of a different problem: Removing the old file on gdrive and replacing it with a new one assign a new id to the file. While this is not a problem on pc sincyng by gvfs (gnome-online-accounts) given that it reference the file by name, it becomes problematic for keepass client on android like KeepassDX, that uses AOSP filemanager's content-provider to access data on cloud, that in turn return the gdrive's file ID instead of its name.

kabili207 commented 5 years ago

This also causes problems when a file is linked in multiple locations on Google Drive (done by selecting a file/folder on the website and clicking Shift+Z). When KeePassXC deletes the original file, Google Drive removes it from ALL places it's linked. This isn't really an issue with clients that only sync periodically, because they won't realize it's a different file. FUSE based file systems, such as google-drive-ocamlfuse or GDriveFS, on the other hand, write directly to Google Drive and will happily break all those links.

I understand why it works the way it currently does, but risk of a truncated file on is, at worst, a rare and minor inconvenience with Google Drive or Dropbox. The issues it causes for cloud clients, however, are exceedingly frequent and very annoying.

Having an option for "cloud friendly" saves would be greatly appreciated.

Tschebbischeff commented 5 years ago

Since I am running into the exact same problem here, using google-drive-ocamlfuse to synchronize to google drive I will give this my +1. Even though google-drive-ocamlfuse has some problem with the moving of files, fixing it won't fix that keepass is recreating the file. An option to let it write to the file directly and let something else handle the consistency (like versioning in gdrive) would be very appreciated. The option could be off by default after all.

kolokolovg commented 5 years ago

any progress habbend?

droidmonkey commented 5 years ago

I'm personally not comfortable making any more changes to the way unsafe saves work. We made many improvements in 2.4.0 and 2.4.1. I use google drive sync on Windows and have never had an issue with versioning and sharing with Android. This seems to be a Linux specific problem. Qt allegedly fixed QSaveFile in version 5.11 so if you are running a recent version of Qt just re-enable safe file saves.

staltux commented 4 years ago

here in 2020 and the problem still persist, android client keep saying the file is trashed, very annoyn

the-dbp commented 3 years ago

here in 2021, confirming google drive overwriting is a big problem causing me lost credentials

droidmonkey commented 3 years ago

6594