Correct saving files to DropBox/Drive/OneDrive #1385

droidmonkey · 2018-01-14T23:08:11Z

Description

Added backup before save config setting
Added non-atomic save config setting
Adds prompt to fallback to non-atomic saves if saving fails 3 times:

Updated basic settings to be in logical groups:

Fixes #197 and closes #1159

Motivation and context

The risk of data loss is minimal and better than not being able to save at all.

How has this been tested?

Manually and automatically

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

phoerious · 2018-01-15T16:05:18Z

src/core/Database.cpp

+        saveFile.close(); // flush to disk
+
+        if (keepOld) {
+            QFile::remove(filePath + ".old");


.old should be inserted between the base filename and the .kdbx extension.

Why you make this difficult? 😝

phoerious · 2018-01-15T16:09:27Z

src/core/Database.cpp

+        }
+
+        QFile::remove(filePath);
+        if (saveFile.rename(filePath)) {


Are you sure this is going to work? If the temp folder is on a different partition, then renaming a file means copying it. Given this is rare on Windows, I'd still see it as a potential problem, because corruption could occur during copying.
Another problem I see is that on platforms other than Windows, the temp location is often /tmp, which is world-readable (the file may not be, but you can still at least list it). The database is encrypted, but we still shouldn't move it to such a location without the user's consent. Maybe make the QTemporaryFile fix Windows-only and continue using QSaveFile on other platforms. Outside of Dropbox folders it's by far the safer option and this problem is Windows-only.

EDIT: ah, I see the same problem was mentioned in the linked Qt bug report.

Yah I got some education from that bug thread, good to know. I did consider the option of only implementing the temp file scheme in windows.

Another issue is if a database is saved to a non-writable directory, but the database itself is writable, this implementation will actually delete the database and return.

That's indeed a major problem. In that case add a safeguard to check write permissions to the folder and show an error if you can't create new files.

droidmonkey · 2018-01-27T23:34:53Z

This PR is ready for final review & merge.

TheZ3ro

Seems good to me

phoerious

A few more minor changes

phoerious · 2018-01-28T12:44:20Z

src/core/Config.cpp

@@ -114,6 +114,8 @@ void Config::init(const QString& fileName)
    m_defaults.insert("AutoSaveAfterEveryChange", false);


Can we make this a default in 2.3? I don't really see a reason why this should be off by default.

Yes good idea

phoerious · 2018-01-28T12:55:25Z

src/core/Database.cpp

+    backupFilePath.replace(".kdbx", ".old.kdbx", Qt::CaseInsensitive);
+    if (!backupFilePath.endsWith(".old.kdbx")) {
+        // Fallback in case of poorly named file
+        backupFilePath = filePath + ".old";


You could do this a lot safer with a regular expression. ;-)

phoerious · 2018-01-28T13:00:46Z

src/gui/DatabaseTabWidget.cpp

-        QString errorMessage = db->saveToFile(filePath);
+        // TODO: Make this async, but lock out the database widget to prevent re-entrance
+        bool useAtomicSaves = config()->get("UseAtomicSaves").toBool();
+        QString errorMessage = db->saveToFile(filePath, useAtomicSaves, config()->get("BackupBeforeSave").toBool());


At least the UseAtomicSaves settings should have true as default value in Config::init().

It does, i added a default value of true here

Yeah, forget what I said. 😕

phoerious · 2018-01-28T13:03:22Z

src/gui/DatabaseTabWidget.cpp

@@ -336,6 +341,22 @@ bool DatabaseTabWidget::saveDatabase(Database* db, QString filePath)
        } else {
            dbStruct.modified = true;
            updateTabName(db);
+
+            if (++dbStruct.saveAttempts > 2 && useAtomicSaves) {


I would check for write permissions first and show a dedicated error message. Otherwise the user will disable atomic saves for nothing if the problem is actually missing write permissions.
I know this is an edge case, because without write permissions, the DB will be opened in read-only mode, but it can still occur if write permissions are somehow removed mid-air.

You can't easily check for write conditions at the directory level on windows. I did a fair bit of research on the matter while trying to diagnose. The only edge case that the read-only open fails at is opening a writable file in a non writable directory. That doesnt seem like a common scenario.

OK, leave as is then.

phoerious · 2018-01-28T13:08:31Z

src/gui/SettingsWidgetGeneral.ui

+          <item>
+           <widget class="QCheckBox" name="useAtomicSavesCheckBox">
+            <property name="text">
+             <string>Safely save database files (may be incompatible with DropBox, etc)</string>


Dropbox's official spelling is with lowercase b.

louib · 2018-01-28T14:42:10Z

src/gui/DatabaseTabWidget.cpp

+                auto choice = MessageBox::question(this, tr("Disable safe saves?"),
+                                                   tr("KeePassXC has failed to save the database multiple times. "
+                                                   "This is likely caused by file sync services holding a lock on "
+                                                   "the save file.\nDisable safe saves and try again?"),


@droidmonkey shouldn't we also use the atomic saves terminology here?

I was thinking that, too. But I don't know how well users will understand it. Atomic in a non-nuclear context is a very domain-specific word.

Atomic is a developer term. I wanted to dumb it down on the user interface.

User (me) googled it: "write to tmp-file and then (atomic->) move over". (had no idea before searching for the term - sounded a little intimidating to me :) X Atomic save (write temporary file - on success secure replace original with it)

* Replaces QSaveFile with QTemporaryFile * Added backup before save config setting * This method may cause data loss (see comments)

* User is prompted to disable safe saves after three failed attempts * Completely retooled basic settings to group settings logically * Added setting for "atomic saves"

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

droidmonkey added the platform: Windows label Jan 14, 2018

droidmonkey added this to the v2.3.0 milestone Jan 14, 2018

droidmonkey requested review from TheZ3ro and phoerious January 14, 2018 23:08

droidmonkey force-pushed the refactor/saving branch from d668f21 to 0edc83a Compare January 14, 2018 23:28

phoerious requested changes Jan 15, 2018

View reviewed changes

droidmonkey force-pushed the refactor/saving branch from 0edc83a to ca9a590 Compare January 15, 2018 17:38

droidmonkey force-pushed the refactor/saving branch from ca9a590 to bf3de2a Compare January 27, 2018 23:34

droidmonkey force-pushed the refactor/saving branch 2 times, most recently from 7b4a883 to 5dad2da Compare January 27, 2018 23:48

TheZ3ro approved these changes Jan 28, 2018

View reviewed changes

phoerious requested changes Jan 28, 2018

View reviewed changes

louib reviewed Jan 28, 2018

View reviewed changes

phoerious approved these changes Jan 28, 2018

View reviewed changes

phoerious mentioned this pull request Jan 28, 2018

"Automatically save after every change" should be default for new installs #279

Closed

droidmonkey added 2 commits January 28, 2018 11:28

Correct saving files to DropBox/Drive/OneDrive

d7f408e

* Replaces QSaveFile with QTemporaryFile * Added backup before save config setting * This method may cause data loss (see comments)

After 3 failed saves, offer to disable safe saves

3db9a86

* User is prompted to disable safe saves after three failed attempts * Completely retooled basic settings to group settings logically * Added setting for "atomic saves"

droidmonkey force-pushed the refactor/saving branch from d850e96 to 60a67db Compare January 28, 2018 16:28

Minor changes to address comments

6a4b275

droidmonkey force-pushed the refactor/saving branch from 60a67db to 6a4b275 Compare January 28, 2018 16:32

Disable auto save for GUI tests

8c8a61d

phoerious merged commit bed921c into develop Jan 28, 2018

phoerious deleted the refactor/saving branch January 28, 2018 20:19

tomhundt mentioned this pull request Mar 14, 2018

What "Safely save database files" means? #1724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct saving files to DropBox/Drive/OneDrive #1385

Correct saving files to DropBox/Drive/OneDrive #1385

droidmonkey commented Jan 14, 2018 •

edited

Loading

phoerious Jan 15, 2018

droidmonkey Jan 15, 2018

phoerious Jan 15, 2018

phoerious Jan 15, 2018 •

edited

Loading

droidmonkey Jan 15, 2018

phoerious Jan 15, 2018 •

edited

Loading

droidmonkey commented Jan 27, 2018

TheZ3ro left a comment

phoerious left a comment

phoerious Jan 28, 2018

droidmonkey Jan 28, 2018

phoerious Jan 28, 2018

phoerious Jan 28, 2018

droidmonkey Jan 28, 2018 •

edited

Loading

phoerious Jan 28, 2018

phoerious Jan 28, 2018 •

edited

Loading

droidmonkey Jan 28, 2018 •

edited

Loading

phoerious Jan 28, 2018

phoerious Jan 28, 2018

louib Jan 28, 2018

phoerious Jan 28, 2018 •

edited

Loading

droidmonkey Jan 28, 2018

calmarc Jan 29, 2018 •

edited

Loading

		@@ -114,6 +114,8 @@ void Config::init(const QString& fileName)
		m_defaults.insert("AutoSaveAfterEveryChange", false);

Correct saving files to DropBox/Drive/OneDrive #1385

Correct saving files to DropBox/Drive/OneDrive #1385

Conversation

droidmonkey commented Jan 14, 2018 • edited Loading

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

droidmonkey commented Jan 27, 2018

TheZ3ro left a comment

Choose a reason for hiding this comment

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

droidmonkey Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calmarc Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

droidmonkey commented Jan 14, 2018 •

edited

Loading

phoerious Jan 15, 2018 •

edited

Loading

phoerious Jan 15, 2018 •

edited

Loading

droidmonkey Jan 28, 2018 •

edited

Loading

phoerious Jan 28, 2018 •

edited

Loading

droidmonkey Jan 28, 2018 •

edited

Loading

phoerious Jan 28, 2018 •

edited

Loading

calmarc Jan 29, 2018 •

edited

Loading