Fix issues with reloading and handling of externally modified db file

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