Fix issues with reloading and handling of externally modified db file (…

…#10612)

Fixes #5290 
Fixes #9062
Fixes #8545 

* Fix data loss on failed reload

- External modifications to the db file can no longer be missed.
- Fixed dialogFinished signal of DatabaseOpenDialog was not emitted when dialog was closed via the 'X' (close) button
- For reloading with a modified db, an additional choice has been added to allow the user to ignore the changes in the file on disk.
- User is now presented with an unlock database dialog if reload fails to open the db automatically. For example when the user removed the YubiKey, failed to touch the YubiKey within the timeout period, or db pw has been changed.
- Mark db as modified when db file is gone or invalid.
- Prevent saving when db is being reloaded
- If merge is triggered by a save action, continue on with the save action after the user makes their choice

---------

Co-authored-by: vuurvlieg <vuurvli3g@protonmail.com>
Co-authored-by: Jonathan White <support@dmapps.us>

share/translations/keepassxc_en.ts

@@ -1513,6 +1513,10 @@ Backup database located at %2</source>
         <source>Recycle Bin</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Database file read error.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>DatabaseOpenDialog</name>
@@ -2683,24 +2687,6 @@ Save changes?</source>
         <source>File has changed</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>The database file has changed. Do you want to load the changes?</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>Merge Request</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>The database file has changed and you have unsaved changes.
-Do you want to merge your changes?</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>Could not open the new database file while attempting to autoreload.
-Error: %1</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Disable safe saves?</source>
         <translation type="unfinished"></translation>
@@ -2787,6 +2773,50 @@ Disable safe saves and try again?</source>
         <source>Do you want to remove the passkey from this entry?</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>The database file &quot;%1&quot; was modified externally</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Do you want to load the changes?</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reload database</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reloading database…</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reload canceled</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reload successful</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Reload pending user action…</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The database file &quot;%1&quot; was modified externally.&lt;br&gt;How would you like to proceed?&lt;br&gt;&lt;br&gt;Merge all changes&lt;br&gt;Ignore the changes on disk until save&lt;br&gt;Discard unsaved changes</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The database file &quot;%1&quot; was modified externally.&lt;br&gt;How would you like to proceed?&lt;br&gt;&lt;br&gt;Merge all changes then save&lt;br&gt;Overwrite the changes on disk&lt;br&gt;Discard unsaved changes</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Database file overwritten.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Database file on disk cannot be unlocked with current credentials.&lt;br&gt;Enter new credentials and/or present hardware key to continue.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>EditEntryWidget</name>

src/CMakeLists.txt

@@ -96,6 +96,7 @@ set(core_SOURCES
         keys/PasswordKey.cpp
         keys/ChallengeResponseKey.cpp
         streams/HashedBlockStream.cpp
+        streams/HashingStream.cpp
         streams/HmacBlockStream.cpp
         streams/LayeredStream.cpp
         streams/qtiocompressor.cpp

src/core/Database.cpp

@@ -25,6 +25,7 @@
 #include "format/KdbxXmlReader.h"
 #include "format/KeePass2Reader.h"
 #include "format/KeePass2Writer.h"
+#include "streams/HashingStream.h"
 
 #include <QFileInfo>
 #include <QJsonObject>
@@ -62,8 +63,8 @@ Database::Database()
         updateTagList();
     });
     connect(this, &Database::modified, this, [this] { updateTagList(); });
-    connect(this, &Database::databaseSaved, this, [this]() { updateCommonUsernames(); });
-    connect(m_fileWatcher, &FileWatcher::fileChanged, this, &Database::databaseFileChanged);
+    connect(this, &Database::databaseSaved, this, [this] { updateCommonUsernames(); });
+    connect(m_fileWatcher, &FileWatcher::fileChanged, this, [this] { emit databaseFileChanged(false); });
 
     // static uuid map
     s_uuidMap.insert(m_uuid, this);
@@ -151,6 +152,20 @@ bool Database::open(const QString& filePath, QSharedPointer<const CompositeKey>
 
     setEmitModified(false);
 
+    // update the hash of the first block
+    m_fileBlockHash.clear();
+    auto fileBlockData = dbFile.peek(kFileBlockToHashSizeBytes);
+    if (fileBlockData.size() != kFileBlockToHashSizeBytes) {
+        if (dbFile.size() >= kFileBlockToHashSizeBytes) {
+            if (error) {
+                *error = tr("Database file read error.");
+            }
+            return false;
+        }
+    } else {
+        m_fileBlockHash = QCryptographicHash::hash(fileBlockData, QCryptographicHash::Md5);
+    }
+
     KeePass2Reader reader;
     if (!reader.readDatabase(&dbFile, std::move(key), this)) {
         if (error) {
@@ -260,14 +275,33 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString&
         return false;
     }
 
-    if (filePath == m_data.filePath) {
-        // Fail-safe check to make sure we don't overwrite underlying file changes
-        // that have not yet triggered a file reload/merge operation.
-        if (!m_fileWatcher->hasSameFileChecksum()) {
-            if (error) {
-                *error = tr("Database file has unmerged changes.");
+    // Make sure we don't overwrite external modifications unless explicitly allowed
+    if (!m_ignoreFileChangesUntilSaved && !m_fileBlockHash.isEmpty() && filePath == m_data.filePath) {
+        QFile dbFile(filePath);
+        if (dbFile.exists()) {
+            if (!dbFile.open(QIODevice::ReadOnly)) {
+                if (error) {
+                    *error = tr("Unable to open file %1.").arg(filePath);
+                }
+                return false;
+            }
+            auto fileBlockData = dbFile.read(kFileBlockToHashSizeBytes);
+            if (fileBlockData.size() == kFileBlockToHashSizeBytes) {
+                auto hash = QCryptographicHash::hash(fileBlockData, QCryptographicHash::Md5);
+                if (m_fileBlockHash != hash) {
+                    if (error) {
+                        *error = tr("Database file has unmerged changes.");
+                    }
+                    // emit the databaseFileChanged(true) signal async
+                    QMetaObject::invokeMethod(this, "databaseFileChanged", Qt::QueuedConnection, Q_ARG(bool, true));
+                    return false;
+                }
+            } else if (dbFile.size() >= kFileBlockToHashSizeBytes) {
+                if (error) {
+                    *error = tr("Database file read error.");
+                }
+                return false;
             }
-            return false;
         }
     }
 
@@ -302,7 +336,7 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString&
             SetFileAttributes(realFilePath.toStdString().c_str(), FILE_ATTRIBUTE_HIDDEN);
         }
 #endif
-
+        m_ignoreFileChangesUntilSaved = false;
         m_fileWatcher->start(realFilePath, 30, 1);
     } else {
         // Saving failed, don't rewatch file since it does not represent our database
@@ -325,15 +359,22 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt
     case Atomic: {
         QSaveFile saveFile(filePath);
         if (saveFile.open(QIODevice::WriteOnly)) {
+            HashingStream hashingStream(&saveFile, QCryptographicHash::Md5, kFileBlockToHashSizeBytes);
+            if (!hashingStream.open(QIODevice::WriteOnly)) {
+                return false;
+            }
             // write the database to the file
-            if (!writeDatabase(&saveFile, error)) {
+            if (!writeDatabase(&hashingStream, error)) {
                 return false;
             }
 
             // Retain original creation time
             saveFile.setFileTime(createTime, QFile::FileBirthTime);
 
             if (saveFile.commit()) {
+                // store the new hash
+                m_fileBlockHash = hashingStream.hashingResult();
+
                 // successfully saved database file
                 return true;
             }
@@ -347,8 +388,12 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt
     case TempFile: {
         QTemporaryFile tempFile;
         if (tempFile.open()) {
+            HashingStream hashingStream(&tempFile, QCryptographicHash::Md5, kFileBlockToHashSizeBytes);
+            if (!hashingStream.open(QIODevice::WriteOnly)) {
+                return false;
+            }
             // write the database to the file
-            if (!writeDatabase(&tempFile, error)) {
+            if (!writeDatabase(&hashingStream, error)) {
                 return false;
             }
             tempFile.close(); // flush to disk
@@ -366,6 +411,8 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt
                 QFile::setPermissions(filePath, perms);
                 // Retain original creation time
                 tempFile.setFileTime(createTime, QFile::FileBirthTime);
+                // store the new hash
+                m_fileBlockHash = hashingStream.hashingResult();
                 return true;
             } else if (backupFilePath.isEmpty() || !restoreDatabase(filePath, backupFilePath)) {
                 // Failed to copy new database in place, and
@@ -387,10 +434,16 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt
         // Open the original database file for direct-write
         QFile dbFile(filePath);
         if (dbFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) {
-            if (!writeDatabase(&dbFile, error)) {
+            HashingStream hashingStream(&dbFile, QCryptographicHash::Md5, kFileBlockToHashSizeBytes);
+            if (!hashingStream.open(QIODevice::WriteOnly)) {
+                return false;
+            }
+            if (!writeDatabase(&hashingStream, error)) {
                 return false;
             }
             dbFile.close();
+            // store the new hash
+            m_fileBlockHash = hashingStream.hashingResult();
             return true;
         }
         if (error) {
@@ -508,6 +561,9 @@ void Database::releaseData()
     m_deletedObjects.clear();
     m_commonUsernames.clear();
     m_tagList.clear();
+
+    m_fileBlockHash.clear();
+    m_ignoreFileChangesUntilSaved = false;
 }
 
 /**
@@ -644,10 +700,33 @@ void Database::setFilePath(const QString& filePath)
         m_data.filePath = filePath;
         // Don't watch for changes until the next open or save operation
         m_fileWatcher->stop();
+        m_ignoreFileChangesUntilSaved = false;
         emit filePathChanged(oldPath, filePath);
     }
 }
 
+const QByteArray& Database::fileBlockHash() const
+{
+    return m_fileBlockHash;
+}
+
+void Database::setIgnoreFileChangesUntilSaved(bool ignore)
+{
+    if (m_ignoreFileChangesUntilSaved != ignore) {
+        m_ignoreFileChangesUntilSaved = ignore;
+        if (ignore) {
+            m_fileWatcher->pause();
+        } else {
+            m_fileWatcher->resume();
+        }
+    }
+}
+
+bool Database::ignoreFileChangesUntilSaved() const
+{
+    return m_ignoreFileChangesUntilSaved;
+}
+
 QList<DeletedObject> Database::deletedObjects()
 {
     return m_deletedObjects;

src/core/Database.h

@@ -75,6 +75,9 @@ class Database : public ModifiableObject
     ~Database() override;
 
 private:
+    // size of the block of file data to hash for detecting external changes
+    static const quint32 kFileBlockToHashSizeBytes = 1024; // 1 KiB
+
     bool writeDatabase(QIODevice* device, QString* error = nullptr);
     bool backupDatabase(const QString& filePath, const QString& destinationFilePath);
     bool restoreDatabase(const QString& filePath, const QString& fromBackupFilePath);
@@ -108,6 +111,10 @@ class Database : public ModifiableObject
     QString canonicalFilePath() const;
     void setFilePath(const QString& filePath);
 
+    const QByteArray& fileBlockHash() const;
+    void setIgnoreFileChangesUntilSaved(bool ignore);
+    bool ignoreFileChangesUntilSaved() const;
+
     QString publicName();
     void setPublicName(const QString& name);
     QString publicColor();
@@ -181,7 +188,7 @@ public slots:
     void databaseOpened();
     void databaseSaved();
     void databaseDiscarded();
-    void databaseFileChanged();
+    void databaseFileChanged(bool triggeredBySave);
     void databaseNonDataChanged();
     void tagListUpdated();
 
@@ -233,6 +240,8 @@ public slots:
     void startModifiedTimer();
     void stopModifiedTimer();
 
+    QByteArray m_fileBlockHash;
+    bool m_ignoreFileChangesUntilSaved;
     QPointer<Metadata> const m_metadata;
     DatabaseData m_data;
     QPointer<Group> m_rootGroup;

src/core/FileWatcher.cpp

@@ -79,17 +79,18 @@ void FileWatcher::stop()
     m_fileChecksum.clear();
     m_fileChecksumTimer.stop();
     m_fileChangeDelayTimer.stop();
+    m_paused = false;
 }
 
 void FileWatcher::pause()
 {
-    m_ignoreFileChange = true;
+    m_paused = true;
     m_fileChangeDelayTimer.stop();
 }
 
 void FileWatcher::resume()
 {
-    m_ignoreFileChange = false;
+    m_paused = false;
     // Add a short delay to start in the next event loop
     if (!m_fileIgnoreDelayTimer.isActive()) {
         m_fileIgnoreDelayTimer.start(0);
@@ -98,7 +99,7 @@ void FileWatcher::resume()
 
 bool FileWatcher::shouldIgnoreChanges()
 {
-    return m_filePath.isEmpty() || m_ignoreFileChange || m_fileIgnoreDelayTimer.isActive()
+    return m_filePath.isEmpty() || m_ignoreFileChange || m_paused || m_fileIgnoreDelayTimer.isActive()
            || m_fileChangeDelayTimer.isActive();
 }
 
@@ -118,7 +119,7 @@ void FileWatcher::checkFileChanged()
 
     AsyncTask::runThenCallback([this] { return calculateChecksum(); },
                                this,
-                               [this](QByteArray checksum) {
+                               [this](const QByteArray& checksum) {
                                    if (checksum != m_fileChecksum) {
                                        m_fileChecksum = checksum;
                                        m_fileChangeDelayTimer.start(0);

src/core/FileWatcher.h

@@ -56,6 +56,7 @@ private slots:
     QTimer m_fileChecksumTimer;
     int m_fileChecksumSizeBytes = -1;
     bool m_ignoreFileChange = false;
+    bool m_paused = false;
 };
 
 #endif // KEEPASSXC_FILEWATCHER_H