From 718485fdbc0ee00816ffb4ceef3544ba3190b9da Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Fri, 19 Apr 2024 17:17:49 +0200 Subject: [PATCH 1/7] 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. Fixes #5290 --- src/CMakeLists.txt | 1 + src/core/Database.cpp | 103 +++++++++++++++--- src/core/Database.h | 11 +- src/core/FileWatcher.cpp | 27 ++--- 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 | 187 +++++++++++++++++++++++++++------ src/gui/DatabaseWidget.h | 5 +- src/streams/HashingStream.cpp | 102 ++++++++++++++++++ src/streams/HashingStream.h | 52 +++++++++ 13 files changed, 463 insertions(+), 62 deletions(-) create mode 100644 src/streams/HashingStream.cpp create mode 100644 src/streams/HashingStream.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a95e58f94d..d0b1e7c331 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -195,6 +195,7 @@ set(keepassx_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 14bba2f38e..239129e065 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 @@ -63,7 +64,7 @@ Database::Database() }); connect(this, &Database::modified, this, [this] { updateTagList(); }); connect(this, &Database::databaseSaved, this, [this]() { updateCommonUsernames(); }); - connect(m_fileWatcher, &FileWatcher::fileChanged, this, &Database::databaseFileChanged); + connect(m_fileWatcher, &FileWatcher::fileChanged, this, [this] { emit Database::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 requested + 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."); + } + // trigger a reload (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 @@ -327,8 +361,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; } @@ -338,6 +376,9 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt #endif if (saveFile.commit()) { + // store the new hash + m_fileBlockHash = hashingStream.hashingResult(); + // successfully saved database file return true; } @@ -351,8 +392,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 @@ -372,6 +417,8 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt // Retain original creation time tempFile.setFileTime(createTime, QFile::FileBirthTime); #endif + // 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 @@ -393,10 +440,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) { @@ -511,6 +564,9 @@ void Database::releaseData() m_deletedObjects.clear(); m_commonUsernames.clear(); m_tagList.clear(); + + m_fileBlockHash.clear(); + m_ignoreFileChangesUntilSaved = false; } /** @@ -647,10 +703,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 31a839734c..5012ff5c58 100644 --- a/src/core/Database.h +++ b/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; + Metadata* metadata(); const Metadata* metadata() const; Group* rootGroup(); @@ -171,7 +178,7 @@ public slots: void databaseOpened(); void databaseSaved(); void databaseDiscarded(); - void databaseFileChanged(); + void databaseFileChanged(bool triggeredBySave); void databaseNonDataChanged(); void tagListUpdated(); @@ -220,6 +227,8 @@ public slots: 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 c919a64ef4..b8ecddc4b1 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(); } @@ -116,16 +117,18 @@ void FileWatcher::checkFileChanged() // Prevent reentrance m_ignoreFileChange = true; - AsyncTask::runThenCallback([=] { return calculateChecksum(); }, - this, - [=](QByteArray checksum) { - if (checksum != m_fileChecksum) { - m_fileChecksum = checksum; - m_fileChangeDelayTimer.start(0); - } + AsyncTask::runThenCallback( + [this] { return calculateChecksum(); }, + this, + [this](QByteArray checksum) { + if (checksum != m_fileChecksum) { + m_fileChecksum = checksum; + m_fileChangeDelayTimer.start(0); + } - m_ignoreFileChange = false; - }); + m_ignoreFileChange = false; + } + ); } QByteArray FileWatcher::calculateChecksum() diff --git a/src/core/FileWatcher.h b/src/core/FileWatcher.h index 27159d17a8..7a5649764e 100644 --- a/src/core/FileWatcher.h +++ b/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 diff --git a/src/gui/DatabaseOpenDialog.cpp b/src/gui/DatabaseOpenDialog.cpp index c4a52e2a0c..f762185713 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 @@ -207,7 +230,4 @@ void DatabaseOpenDialog::complete(bool accepted) } else { reject(); } - - emit dialogFinished(accepted, m_currentDbWidget); - clearForms(); } diff --git a/src/gui/DatabaseOpenDialog.h b/src/gui/DatabaseOpenDialog.h index b1a59b59a7..1aa24e4d9e 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 @@ -50,11 +51,13 @@ class DatabaseOpenDialog : public QDialog 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 a20c525af9..eec3c1c5e9 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -224,6 +224,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 2236e52d83..2a390de389 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 @@ class DatabaseOpenWidget : public DialogyWidget 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 816d3ace1d..5ad7dce830 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -214,6 +214,7 @@ DatabaseWidget::DatabaseWidget(QSharedPointer db, QWidget* parent) connectDatabaseSignals(); m_blockAutoSave = false; + m_reloading = false; m_autosaveTimer = new QTimer(this); m_autosaveTimer->setSingleShot(true); @@ -1743,6 +1744,11 @@ bool DatabaseWidget::lock() return true; } + // 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())); @@ -1775,6 +1781,19 @@ 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 = QObject::connect(this, &DatabaseWidget::reloadBegin, [&reloadTriggered](bool){ + reloadTriggered = true; + }); + QApplication::processEvents(); + QObject::disconnect(connection); + if (reloadTriggered) { + return false; + } + } } if (!saved) { @@ -1830,52 +1849,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); + 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; } } + // notify + showMessage(tr("Reloading database..."), MessageWidget::Information, false, MessageWidget::DisableAutoHide); + // 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); - - if (result == MessageBox::Merge) { - // Merge the old database into the new one - Merger merger(m_db.data(), db.data()); - merger.merge(); - } + auto reloadFinish = [this](bool hideMsg = true) { + // Return control + m_entryView->setDisabled(false); + m_groupView->setDisabled(false); + m_tagView->setDisabled(false); + + m_reloading = false; + + if (hideMsg) { + hideMessage(); + } + + emit reloadEnd(); + }; + auto reloadAbort = [this, reloadFinish]{ + // Mark db as modified since existing data may differ from file or file was deleted + m_db->markAsModified(); + + showMessage(tr("Reload aborted"), MessageWidget::Warning); + reloadFinish(false); + }; + auto reloadContinue = [this, 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(); @@ -1892,17 +1935,90 @@ 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(); + + showMessage(tr("Reload OK"), MessageWidget::Positive); + reloadFinish(false); + }; + + 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 (db->fileBlockHash() == m_db->fileBlockHash() || db->fileBlockHash().isEmpty()) { + 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()) { + // Ask how to proceed + auto result = MessageBox::question( + this, + tr("Reload database"), + QString("%1.\n%2\n\n%3.\n%4.\n%5.").arg( + tr("The database file \"%1\" was modified externally").arg(displayFileName()), + tr("How to proceed with your unsaved changes?"), + tr("Merge all changes together"), + tr("Discard your changes"), + tr("Ignore the changes in the file on disk")), + MessageBox::Merge | MessageBox::Discard | MessageBox::Ignore | MessageBox::Cancel, + MessageBox::Merge); + + if (result == MessageBox::Cancel) { + reloadAbort(); + return; + } else if (result == MessageBox::Ignore) { + m_db->setIgnoreFileChangesUntilSaved(true); + m_blockAutoSave = false; + reloadFinish(); + return; + } else if (result == MessageBox::Merge) { + changesActionStr = tr("merged"); + merge = true; + } else { + changesActionStr = tr("discarded"); + } + } + + 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); + QObject::connect(openDialog, &QObject::destroyed, [=](QObject*){ dbWidget->deleteLater(); }); + QObject::connect( + openDialog, &DatabaseOpenDialog::dialogFinished, this, + [=](bool accepted, DatabaseWidget* dbWidget) { + if (accepted) { + reloadContinue(openDialog->database(), merge); + } else { + reloadAbort(); + } + } + ); + openDialog->setAttribute(Qt::WA_DeleteOnClose); // free the memory on close + openDialog->addDatabaseTab(dbWidget); + openDialog->setActiveDatabaseTab(dbWidget); + if (!changesActionStr.isEmpty()) { + changesActionStr = QString(" [%1]").arg(tr("Changes are %1").arg(changesActionStr)); + } + openDialog->showMessage( + QString("%1.\n%2%3").arg( + tr("Error: failed to open the database file for reload"), + tr("Unlock to reload"), changesActionStr), + 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 @@ -2057,6 +2173,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(); diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 6812600092..ff75d823f9 100644 --- a/src/gui/DatabaseWidget.h +++ b/src/gui/DatabaseWidget.h @@ -153,6 +153,8 @@ class DatabaseWidget : public QStackedWidget void clearSearch(); void requestGlobalAutoType(const QString& search); void requestSearch(const QString& search); + void reloadBegin(); + void reloadEnd(); public slots: bool lock(); @@ -260,7 +262,7 @@ private slots: void mergeDatabase(bool accepted); void emitCurrentModeChanged(); // Database autoreload slots - void reloadDatabaseFile(); + void reloadDatabaseFile(bool triggeredBySave); void restoreGroupEntryFocus(const QUuid& groupUuid, const QUuid& EntryUuid); private: @@ -308,6 +310,7 @@ private slots: // 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 0000000000..f9f91a3de8 --- /dev/null +++ b/src/streams/HashingStream.cpp @@ -0,0 +1,102 @@ +/* + * 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 0000000000..5f8c2ac3eb --- /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 From bf92abd30906a5591785c1e19cbe9b8542dc1614 Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Fri, 19 Apr 2024 17:31:36 +0200 Subject: [PATCH 2/7] Fix test-case --- tests/TestDatabase.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index e3e957e79c..bc131eba1f 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -124,10 +124,10 @@ void TestDatabase::testSaveAs() QCOMPARE(spyFilePathChanged.count(), 1); QVERIFY(QFile::exists(newDbFileName)); #ifdef Q_OS_WIN - QVERIFY(!QFileInfo::QFileInfo(newDbFileName).isHidden()); + QVERIFY(!QFileInfo(newDbFileName).isHidden()); SetFileAttributes(newDbFileName.toStdString().c_str(), FILE_ATTRIBUTE_HIDDEN); QVERIFY2(db->saveAs(newDbFileName, Database::Atomic, QString(), &error), error.toLatin1()); - QVERIFY(QFileInfo::QFileInfo(newDbFileName).isHidden()); + QVERIFY(QFileInfo(newDbFileName).isHidden()); #endif QFile::remove(newDbFileName); QVERIFY(!QFile::exists(newDbFileName)); @@ -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()); From 27390f5efbf6658766315825322c2d831297d8b4 Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Fri, 19 Apr 2024 20:59:15 +0200 Subject: [PATCH 3/7] Fix incorrect lambda argument --- src/gui/DatabaseWidget.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 5ad7dce830..a22a7ae771 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1785,7 +1785,7 @@ bool DatabaseWidget::lock() if (!saved) { // detect if a reload was triggered bool reloadTriggered = false; - auto connection = QObject::connect(this, &DatabaseWidget::reloadBegin, [&reloadTriggered](bool){ + auto connection = QObject::connect(this, &DatabaseWidget::reloadBegin, [&reloadTriggered] { reloadTriggered = true; }); QApplication::processEvents(); From d5f7879181d02c6c21805272b9c6b7d190f73014 Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Fri, 19 Apr 2024 21:00:10 +0200 Subject: [PATCH 4/7] Add test-case --- tests/TestDatabase.cpp | 38 ++++++++++++++++++++++++++++++++++++++ tests/TestDatabase.h | 1 + 2 files changed, 39 insertions(+) diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index bc131eba1f..78b6dac1df 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -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 9f4bfab567..e23b23cf8a 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 From 26f4fcd1446131f55bcc549cb3248931bb329a51 Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Fri, 19 Apr 2024 21:15:44 +0200 Subject: [PATCH 5/7] Tweak comments --- src/core/Database.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 239129e065..28405aa531 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -275,7 +275,7 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString& return false; } - // Make sure we don't overwrite external modifications unless explicitly requested + // 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()) { @@ -292,7 +292,7 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString& if (error) { *error = tr("Database file has unmerged changes."); } - // trigger a reload (async) + // emit the databaseFileChanged(true) signal async QMetaObject::invokeMethod(this, "databaseFileChanged", Qt::QueuedConnection, Q_ARG(bool, true)); return false; } From a0ae1281c5906468937bdcd94819c3a8c0494316 Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Thu, 25 Apr 2024 21:54:56 +0200 Subject: [PATCH 6/7] Improve reload database message when triggered by db save Mark db as modified when db file is gone or invalid. --- src/gui/DatabaseWidget.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index a22a7ae771..41c49f46ab 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1944,7 +1944,11 @@ void DatabaseWidget::reloadDatabaseFile(bool triggeredBySave) 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 (db->fileBlockHash() == m_db->fileBlockHash() || db->fileBlockHash().isEmpty()) { + 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; @@ -1954,11 +1958,12 @@ void DatabaseWidget::reloadDatabaseFile(bool triggeredBySave) QString changesActionStr; if (triggeredBySave || m_db->isModified() || m_db->hasNonDataChanges()) { // Ask how to proceed + auto prefix = triggeredBySave ? tr("Cannot save because the") : tr("The"); auto result = MessageBox::question( this, tr("Reload database"), - QString("%1.\n%2\n\n%3.\n%4.\n%5.").arg( - tr("The database file \"%1\" was modified externally").arg(displayFileName()), + QString("%1 %2.\n%3\n\n%4.\n%5.\n%6.").arg( + prefix, tr("database file \"%1\" was modified externally").arg(displayFileName()), tr("How to proceed with your unsaved changes?"), tr("Merge all changes together"), tr("Discard your changes"), From d7e54939f7cb86bc4fe5012ad7698044ba736a5b Mon Sep 17 00:00:00 2001 From: vuurvlieg Date: Thu, 25 Apr 2024 22:13:39 +0200 Subject: [PATCH 7/7] Prevent saving when db is being reloaded --- src/gui/DatabaseWidget.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 41c49f46ab..11b6620fa3 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -2238,6 +2238,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();