From 811887e59140d5a3fa6f5fdbe784219b83b5eefe Mon Sep 17 00:00:00 2001 From: vuurvli3g <85685555+vuurvli3g@users.noreply.github.com> Date: Sat, 1 Feb 2025 17:58:45 +0100 Subject: [PATCH] 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 Co-authored-by: Jonathan White --- share/translations/keepassxc_en.ts | 66 ++++++--- src/CMakeLists.txt | 1 + src/core/Database.cpp | 105 +++++++++++++-- src/core/Database.h | 11 +- src/core/FileWatcher.cpp | 9 +- src/core/FileWatcher.h | 1 + src/gui/DatabaseOpenDialog.cpp | 26 +++- src/gui/DatabaseOpenDialog.h | 3 + src/gui/DatabaseOpenWidget.cpp | 5 + src/gui/DatabaseOpenWidget.h | 2 + src/gui/DatabaseWidget.cpp | 210 ++++++++++++++++++++++++----- src/gui/DatabaseWidget.h | 5 +- src/streams/HashingStream.cpp | 101 ++++++++++++++ src/streams/HashingStream.h | 52 +++++++ tests/TestDatabase.cpp | 40 +++++- tests/TestDatabase.h | 1 + 16 files changed, 564 insertions(+), 74 deletions(-) create mode 100644 src/streams/HashingStream.cpp create mode 100644 src/streams/HashingStream.h diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 571100639..1bc3afec6 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -1513,6 +1513,10 @@ Backup database located at %2 Recycle Bin + + Database file read error. + + DatabaseOpenDialog @@ -2683,24 +2687,6 @@ Save changes? File has changed - - The database file has changed. Do you want to load the changes? - - - - Merge Request - - - - The database file has changed and you have unsaved changes. -Do you want to merge your changes? - - - - Could not open the new database file while attempting to autoreload. -Error: %1 - - Disable safe saves? @@ -2787,6 +2773,50 @@ Disable safe saves and try again? Do you want to remove the passkey from this entry? + + The database file "%1" was modified externally + + + + Do you want to load the changes? + + + + Reload database + + + + Reloading database… + + + + Reload canceled + + + + Reload successful + + + + Reload pending user action… + + + + The database file "%1" was modified externally.<br>How would you like to proceed?<br><br>Merge all changes<br>Ignore the changes on disk until save<br>Discard unsaved changes + + + + The database file "%1" was modified externally.<br>How would you like to proceed?<br><br>Merge all changes then save<br>Overwrite the changes on disk<br>Discard unsaved changes + + + + Database file overwritten. + + + + Database file on disk cannot be unlocked with current credentials.<br>Enter new credentials and/or present hardware key to continue. + + EditEntryWidget diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 84c6090ba..f6a20e107 100644 --- a/src/CMakeLists.txt +++ b/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 diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 5734f9521..d3254c5b1 100644 --- a/src/core/Database.cpp +++ b/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 #include @@ -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 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,8 +359,12 @@ 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; } @@ -334,6 +372,9 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt 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 Database::deletedObjects() { return m_deletedObjects; diff --git a/src/core/Database.h b/src/core/Database.h index 29314650e..0d183e778 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -75,6 +75,9 @@ public: ~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 @@ public: 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 @@ signals: void databaseOpened(); void databaseSaved(); void databaseDiscarded(); - void databaseFileChanged(); + void databaseFileChanged(bool triggeredBySave); void databaseNonDataChanged(); void tagListUpdated(); @@ -233,6 +240,8 @@ private: void startModifiedTimer(); void stopModifiedTimer(); + QByteArray m_fileBlockHash; + bool m_ignoreFileChangesUntilSaved; QPointer const m_metadata; DatabaseData m_data; QPointer m_rootGroup; diff --git a/src/core/FileWatcher.cpp b/src/core/FileWatcher.cpp index 1cf3be8e6..3a3c6d83b 100644 --- a/src/core/FileWatcher.cpp +++ b/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); diff --git a/src/core/FileWatcher.h b/src/core/FileWatcher.h index 27159d17a..7a5649764 100644 --- a/src/core/FileWatcher.h +++ b/src/core/FileWatcher.h @@ -56,6 +56,7 @@ private: QTimer m_fileChecksumTimer; int m_fileChecksumSizeBytes = -1; bool m_ignoreFileChange = false; + bool m_paused = false; }; #endif // KEEPASSXC_FILEWATCHER_H diff --git a/src/gui/DatabaseOpenDialog.cpp b/src/gui/DatabaseOpenDialog.cpp index fa9383ac2..881db4087 100644 --- a/src/gui/DatabaseOpenDialog.cpp +++ b/src/gui/DatabaseOpenDialog.cpp @@ -192,11 +192,34 @@ void DatabaseOpenDialog::clearForms() m_tabBar->blockSignals(false); } +void DatabaseOpenDialog::showMessage(const QString& text, MessageWidget::MessageType type, int autoHideTimeout) +{ + m_view->showMessage(text, type, autoHideTimeout); +} + QSharedPointer DatabaseOpenDialog::database() const { return m_db; } +void DatabaseOpenDialog::done(int result) +{ + hide(); + + emit dialogFinished(result == QDialog::Accepted, m_currentDbWidget); + clearForms(); + + QDialog::done(result); + +#if QT_VERSION < QT_VERSION_CHECK(6, 3, 0) + // CDialogs are not really closed, just hidden, pre Qt 6.3? + if (testAttribute(Qt::WA_DeleteOnClose)) { + setAttribute(Qt::WA_DeleteOnClose, false); + deleteLater(); + } +#endif +} + void DatabaseOpenDialog::complete(bool accepted) { // save DB, since DatabaseOpenWidget will reset its data after accept() is called @@ -210,9 +233,6 @@ void DatabaseOpenDialog::complete(bool accepted) } else { reject(); } - - emit dialogFinished(accepted, m_currentDbWidget); - clearForms(); } void DatabaseOpenDialog::closeEvent(QCloseEvent* e) diff --git a/src/gui/DatabaseOpenDialog.h b/src/gui/DatabaseOpenDialog.h index d630ec67b..ba6fcb4e8 100644 --- a/src/gui/DatabaseOpenDialog.h +++ b/src/gui/DatabaseOpenDialog.h @@ -19,6 +19,7 @@ #define KEEPASSX_UNLOCKDATABASEDIALOG_H #include "core/Global.h" +#include "gui/MessageWidget.h" #include #include @@ -51,11 +52,13 @@ public: Intent intent() const; QSharedPointer database() const; void clearForms(); + void showMessage(const QString& text, MessageWidget::MessageType type, int autoHideTimeout); signals: void dialogFinished(bool accepted, DatabaseWidget* dbWidget); public slots: + void done(int result) override; void complete(bool accepted); void tabChanged(int index); diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 11179da83..1a84a7185 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -220,6 +220,11 @@ bool DatabaseOpenWidget::unlockingDatabase() return m_unlockingDatabase; } +void DatabaseOpenWidget::showMessage(const QString& text, MessageWidget::MessageType type, int autoHideTimeout) +{ + m_ui->messageWidget->showMessage(text, type, autoHideTimeout); +} + void DatabaseOpenWidget::load(const QString& filename) { clearForms(); diff --git a/src/gui/DatabaseOpenWidget.h b/src/gui/DatabaseOpenWidget.h index e0a250e3d..7c52717a0 100644 --- a/src/gui/DatabaseOpenWidget.h +++ b/src/gui/DatabaseOpenWidget.h @@ -25,6 +25,7 @@ #include "config-keepassx.h" #include "gui/DialogyWidget.h" +#include "gui/MessageWidget.h" #ifdef WITH_XC_YUBIKEY #include "osutils/DeviceListener.h" #endif @@ -51,6 +52,7 @@ public: void enterKey(const QString& pw, const QString& keyFile); QSharedPointer database(); bool unlockingDatabase(); + void showMessage(const QString& text, MessageWidget::MessageType type, int autoHideTimeout); // Quick Unlock helper functions bool canPerformQuickUnlock() const; diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 2b4266c90..57edca636 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -225,6 +225,7 @@ DatabaseWidget::DatabaseWidget(QSharedPointer db, QWidget* parent) connectDatabaseSignals(); m_blockAutoSave = false; + m_reloading = false; m_autosaveTimer = new QTimer(this); m_autosaveTimer->setSingleShot(true); @@ -1984,6 +1985,11 @@ bool DatabaseWidget::lock() return isLocked(); } + // ignore when reloading + if (m_reloading) { + return false; + } + // Don't try to lock the database while saving, this will cause a deadlock if (m_db->isSaving()) { QTimer::singleShot(200, this, SLOT(lock())); @@ -2027,6 +2033,18 @@ bool DatabaseWidget::lock() if (config()->get(Config::AutoSaveOnExit).toBool() || config()->get(Config::AutoSaveAfterEveryChange).toBool()) { saved = save(); + + if (!saved) { + // detect if a reload was triggered + bool reloadTriggered = false; + auto connection = + connect(this, &DatabaseWidget::reloadBegin, [&reloadTriggered] { reloadTriggered = true; }); + QApplication::processEvents(); + disconnect(connection); + if (reloadTriggered) { + return false; + } + } } if (!saved) { @@ -2085,52 +2103,76 @@ bool DatabaseWidget::lock() return true; } -void DatabaseWidget::reloadDatabaseFile() +void DatabaseWidget::reloadDatabaseFile(bool triggeredBySave) { - // Ignore reload if we are locked, saving, or currently editing an entry or group - if (!m_db || isLocked() || isEntryEditActive() || isGroupEditActive() || isSaving()) { + if (triggeredBySave) { + // not a failed save attempt due to file locking + m_saveAttempts = 0; + } + // Ignore reload if we are locked, saving, reloading, or currently editing an entry or group + if (!m_db || isLocked() || isEntryEditActive() || isGroupEditActive() || isSaving() || m_reloading) { return; } m_blockAutoSave = true; + m_reloading = true; - if (!config()->get(Config::AutoReloadOnChange).toBool()) { + emit reloadBegin(); + + if (!triggeredBySave && !config()->get(Config::AutoReloadOnChange).toBool()) { // Ask if we want to reload the db - auto result = MessageBox::question(this, - tr("File has changed"), - tr("The database file has changed. Do you want to load the changes?"), - MessageBox::Yes | MessageBox::No); + auto result = MessageBox::question( + this, + tr("File has changed"), + QString("%1.\n%2").arg(tr("The database file \"%1\" was modified externally").arg(displayFileName()), + tr("Do you want to load the changes?")), + MessageBox::Yes | MessageBox::No); if (result == MessageBox::No) { // Notify everyone the database does not match the file m_db->markAsModified(); + m_reloading = false; + + emit reloadEnd(); return; } } + // Remove any latent error messages and switch to progress updates + hideMessage(); + emit updateSyncProgress(0, tr("Reloading database…")); + // Lock out interactions m_entryView->setDisabled(true); m_groupView->setDisabled(true); m_tagView->setDisabled(true); QApplication::processEvents(); - QString error; - auto db = QSharedPointer::create(m_db->filePath()); - if (db->open(database()->key(), &error)) { - if (m_db->isModified() || db->hasNonDataChanges()) { - // Ask if we want to merge changes into new database - auto result = MessageBox::question( - this, - tr("Merge Request"), - tr("The database file has changed and you have unsaved changes.\nDo you want to merge your changes?"), - MessageBox::Merge | MessageBox::Discard, - MessageBox::Merge); + auto reloadFinish = [this](bool hideMsg = true) { + // Return control + m_entryView->setDisabled(false); + m_groupView->setDisabled(false); + m_tagView->setDisabled(false); - if (result == MessageBox::Merge) { - // Merge the old database into the new one - Merger merger(m_db.data(), db.data()); - merger.merge(); - } + m_reloading = false; + + // Keep the previous message visible for 2 seconds if not hiding + QTimer::singleShot(hideMsg ? 0 : 2000, this, [this] { emit updateSyncProgress(-1, ""); }); + + emit reloadEnd(); + }; + auto reloadCanceled = [this, reloadFinish] { + // Mark db as modified since existing data may differ from file or file was deleted + m_db->markAsModified(); + + emit updateSyncProgress(100, tr("Reload canceled")); + reloadFinish(false); + }; + auto reloadContinue = [this, triggeredBySave, reloadFinish](QSharedPointer db, bool merge) { + if (merge) { + // Merge the old database into the new one + Merger merger(m_db.data(), db.data()); + merger.merge(); } QUuid groupBeforeReload = m_db->rootGroup()->uuid(); @@ -2147,17 +2189,108 @@ void DatabaseWidget::reloadDatabaseFile() processAutoOpen(); restoreGroupEntryFocus(groupBeforeReload, entryBeforeReload); m_blockAutoSave = false; - } else { - showMessage(tr("Could not open the new database file while attempting to autoreload.\nError: %1").arg(error), - MessageWidget::Error); - // Mark db as modified since existing data may differ from file or file was deleted - m_db->markAsModified(); + + emit updateSyncProgress(100, tr("Reload successful")); + reloadFinish(false); + + // If triggered by save, attempt another save + if (triggeredBySave) { + save(); + } + }; + + auto db = QSharedPointer::create(m_db->filePath()); + bool openResult = db->open(database()->key()); + + // skip if the db is unchanged, or the db file is gone or for sure not a kp-db + if (bool sameHash = db->fileBlockHash() == m_db->fileBlockHash() || db->fileBlockHash().isEmpty()) { + if (!sameHash) { + // db file gone or invalid so mark modified + m_db->markAsModified(); + } + m_blockAutoSave = false; + reloadFinish(); + return; } - // Return control - m_entryView->setDisabled(false); - m_groupView->setDisabled(false); - m_tagView->setDisabled(false); + bool merge = false; + QString changesActionStr; + if (triggeredBySave || m_db->isModified() || m_db->hasNonDataChanges()) { + emit updateSyncProgress(50, tr("Reload pending user action…")); + + // Ask how to proceed + auto message = tr("The database file \"%1\" was modified externally.
" + "How would you like to proceed?

" + "Merge all changes
" + "Ignore the changes on disk until save
" + "Discard unsaved changes") + .arg(displayFileName()); + auto buttons = MessageBox::Merge | MessageBox::Discard | MessageBox::Ignore | MessageBox::Cancel; + // Different message if we are attempting to save + if (triggeredBySave) { + message = tr("The database file \"%1\" was modified externally.
" + "How would you like to proceed?

" + "Merge all changes then save
" + "Overwrite the changes on disk
" + "Discard unsaved changes") + .arg(displayFileName()); + buttons = MessageBox::Merge | MessageBox::Discard | MessageBox::Overwrite | MessageBox::Cancel; + } + + auto result = MessageBox::question(this, tr("Reload database"), message, buttons, MessageBox::Merge); + switch (result) { + case MessageBox::Cancel: + reloadCanceled(); + return; + case MessageBox::Overwrite: + case MessageBox::Ignore: + m_db->setIgnoreFileChangesUntilSaved(true); + m_blockAutoSave = false; + reloadFinish(!triggeredBySave); + // If triggered by save, attempt another save + if (triggeredBySave) { + save(); + emit updateSyncProgress(100, tr("Database file overwritten.")); + } + return; + case MessageBox::Merge: + merge = true; + default: + break; + } + } + + // Database file on disk previously opened successfully + if (openResult) { + reloadContinue(std::move(db), merge); + return; + } + + // The user needs to provide credentials + auto dbWidget = new DatabaseWidget(std::move(db)); + auto openDialog = new DatabaseOpenDialog(this); + connect(openDialog, &QObject::destroyed, [=](QObject*) { dbWidget->deleteLater(); }); + connect(openDialog, &DatabaseOpenDialog::dialogFinished, this, [=](bool accepted, DatabaseWidget*) { + if (accepted) { + reloadContinue(openDialog->database(), merge); + } else { + reloadCanceled(); + } + }); + openDialog->setAttribute(Qt::WA_DeleteOnClose); + openDialog->addDatabaseTab(dbWidget); + openDialog->setActiveDatabaseTab(dbWidget); + openDialog->showMessage(tr("Database file on disk cannot be unlocked with current credentials.
" + "Enter new credentials and/or present hardware key to continue."), + MessageWidget::Error, + MessageWidget::DisableAutoHide); + + // ensure the main window is visible for this + getMainWindow()->bringToFront(); + // show the unlock dialog + openDialog->show(); + openDialog->raise(); + openDialog->activateWindow(); } int DatabaseWidget::numberOfSelectedEntries() const @@ -2320,6 +2453,11 @@ bool DatabaseWidget::save() return true; } + // Do no try to save if the database is being reloaded + if (m_reloading) { + return false; + } + // Read-only and new databases ask for filename if (m_db->filePath().isEmpty()) { return saveAs(); @@ -2334,6 +2472,7 @@ bool DatabaseWidget::save() m_saveAttempts = 0; m_blockAutoSave = false; m_autosaveTimer->stop(); // stop autosave delay to avoid triggering another save + hideMessage(); return true; } @@ -2375,6 +2514,11 @@ bool DatabaseWidget::saveAs() return true; } + // Do no try to save if the database is being reloaded + if (m_reloading) { + return false; + } + QString oldFilePath = m_db->filePath(); if (!QFileInfo::exists(oldFilePath)) { QString defaultFileName = config()->get(Config::DefaultDatabaseFileName).toString(); diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 1e85dd312..a96c9d488 100644 --- a/src/gui/DatabaseWidget.h +++ b/src/gui/DatabaseWidget.h @@ -170,6 +170,8 @@ signals: void clearSearch(); void requestGlobalAutoType(const QString& search); void requestSearch(const QString& search); + void reloadBegin(); + void reloadEnd(); public slots: bool lock(); @@ -287,7 +289,7 @@ private slots: void finishSync(const RemoteParams* params, RemoteHandler::RemoteResult result); void emitCurrentModeChanged(); // Database autoreload slots - void reloadDatabaseFile(); + void reloadDatabaseFile(bool triggeredBySave); void restoreGroupEntryFocus(const QUuid& groupUuid, const QUuid& EntryUuid); void onConfigChanged(Config::ConfigKey key); @@ -338,6 +340,7 @@ private: // Autoreload bool m_blockAutoSave; + bool m_reloading; // Autosave delay QPointer m_autosaveTimer; diff --git a/src/streams/HashingStream.cpp b/src/streams/HashingStream.cpp new file mode 100644 index 000000000..5139dae87 --- /dev/null +++ b/src/streams/HashingStream.cpp @@ -0,0 +1,101 @@ +/* + * Copyright (C) 2024 KeePassXC Team + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 or (at your option) + * version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "HashingStream.h" + +HashingStream::HashingStream(QIODevice* baseDevice) + : LayeredStream(baseDevice) + , m_hash(QCryptographicHash::Md5) + , m_sizeToHash(0) +{ + init(); +} + +HashingStream::HashingStream(QIODevice* baseDevice, QCryptographicHash::Algorithm hashAlgo, qint64 sizeToHash) + : LayeredStream(baseDevice) + , m_hash(hashAlgo) + , m_sizeToHash(sizeToHash) +{ + init(); +} + +HashingStream::~HashingStream() +{ + close(); +} + +void HashingStream::init() +{ + m_sizeHashed = 0; + m_sizeStreamed = 0; + m_hashFinalized = false; +} + +bool HashingStream::reset() +{ + init(); + m_hash.reset(); + return LayeredStream::reset(); +} + +QByteArray HashingStream::hashingResult() +{ + if (m_sizeHashed <= 0 || (m_sizeToHash > 0 && m_sizeHashed != m_sizeToHash)) { + setErrorString("Not enough data to compute hash"); + return {}; + } + m_hashFinalized = true; + return m_hash.result(); +} + +qint64 HashingStream::readData(char* data, qint64 maxSize) +{ + auto sizeRead = LayeredStream::readData(data, maxSize); + if (sizeRead > 0) { + if (!m_hashFinalized) { + qint64 sizeToHash = sizeRead; + if (m_sizeToHash > 0) { + sizeToHash = qMin(m_sizeToHash - m_sizeStreamed, sizeRead); + } + if (sizeToHash > 0) { + m_hash.addData(data, sizeToHash); + m_sizeHashed += sizeToHash; + } + } + m_sizeStreamed += sizeRead; + } + return sizeRead; +} + +qint64 HashingStream::writeData(const char* data, qint64 maxSize) +{ + auto sizeWritten = LayeredStream::writeData(data, maxSize); + if (sizeWritten > 0) { + if (!m_hashFinalized) { + qint64 sizeToHash = sizeWritten; + if (m_sizeToHash > 0) { + sizeToHash = qMin(m_sizeToHash - m_sizeStreamed, sizeWritten); + } + if (sizeToHash > 0) { + m_hash.addData(data, sizeToHash); + m_sizeHashed += sizeToHash; + } + } + m_sizeStreamed += sizeWritten; + } + return sizeWritten; +} diff --git a/src/streams/HashingStream.h b/src/streams/HashingStream.h new file mode 100644 index 000000000..5f8c2ac3e --- /dev/null +++ b/src/streams/HashingStream.h @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2024 KeePassXC Team + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 or (at your option) + * version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifndef KEEPASSX_HASHINGSTREAM_H +#define KEEPASSX_HASHINGSTREAM_H + +#include + +#include "streams/LayeredStream.h" + +class HashingStream : public LayeredStream +{ + Q_OBJECT + +public: + explicit HashingStream(QIODevice* baseDevice); + HashingStream(QIODevice* baseDevice, QCryptographicHash::Algorithm hashAlgo, qint64 sizeToHash); + ~HashingStream() override; + + bool reset() override; + + QByteArray hashingResult(); + +protected: + void init(); + + qint64 readData(char* data, qint64 maxSize) override; + qint64 writeData(const char* data, qint64 maxSize) override; + +protected: + QCryptographicHash m_hash; + bool m_hashFinalized; + qint64 m_sizeToHash; + qint64 m_sizeHashed; + qint64 m_sizeStreamed; +}; + +#endif // KEEPASSX_HASHINGSTREAM_H \ No newline at end of file diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index 3b20d6fc5..78b6dac1d 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -164,7 +164,7 @@ void TestDatabase::testSignals() // Short delay to allow file system settling to reduce test failures Tools::wait(100); - QSignalSpy spyFileChanged(db.data(), SIGNAL(databaseFileChanged())); + QSignalSpy spyFileChanged(db.data(), &Database::databaseFileChanged); QVERIFY(tempFile.copyFromFile(dbFileName)); QTRY_COMPARE(spyFileChanged.count(), 1); QTRY_VERIFY(!db->isModified()); @@ -268,3 +268,41 @@ void TestDatabase::testCustomIcons() QCOMPARE(iconData.name, QString("Test")); QCOMPARE(iconData.lastModified, date); } + +void TestDatabase::testExternallyModified() +{ + TemporaryFile tempFile; + QVERIFY(tempFile.copyFromFile(dbFileName)); + + auto db = QSharedPointer::create(); + auto key = QSharedPointer::create(); + key->addKey(QSharedPointer::create("a")); + + QString error; + QVERIFY(db->open(tempFile.fileName(), key, &error) == true); + db->metadata()->setName("test2"); + QVERIFY(db->save(Database::Atomic, {}, &error)); + + QSignalSpy spyFileChanged(db.data(), &Database::databaseFileChanged); + QVERIFY(tempFile.copyFromFile(dbFileName)); + QTRY_COMPARE(spyFileChanged.count(), 1); + // the first argument of the databaseFileChanged signal (triggeredBySave) should be false + QVERIFY(spyFileChanged.at(0).length() == 1); + QVERIFY(spyFileChanged.at(0).at(0).type() == QVariant::Bool); + QVERIFY(spyFileChanged.at(0).at(0).toBool() == false); + spyFileChanged.clear(); + // shouldn't be able to save due to external changes + QVERIFY(db->save(Database::Atomic, {}, &error) == false); + QApplication::processEvents(); + // save should have triggered another databaseFileChanged signal + QVERIFY(spyFileChanged.count() >= 1); + // the first argument of the databaseFileChanged signal (triggeredBySave) should be true + QVERIFY(spyFileChanged.at(0).at(0).type() == QVariant::Bool); + QVERIFY(spyFileChanged.at(0).at(0).toBool() == true); + + // should be able to overwrite externally modified changes when explicitly requested + db->setIgnoreFileChangesUntilSaved(true); + QVERIFY(db->save(Database::Atomic, {}, &error)); + // ignoreFileChangesUntilSaved should reset after save + QVERIFY(db->ignoreFileChangesUntilSaved() == false); +} diff --git a/tests/TestDatabase.h b/tests/TestDatabase.h index 9f4bfab56..e23b23cf8 100644 --- a/tests/TestDatabase.h +++ b/tests/TestDatabase.h @@ -36,6 +36,7 @@ private slots: void testEmptyRecycleBinOnEmpty(); void testEmptyRecycleBinWithHierarchicalData(); void testCustomIcons(); + void testExternallyModified(); }; #endif // KEEPASSX_TESTDATABASE_H