Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with reloading and handling of externally modified db file #10612

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
103 changes: 91 additions & 12 deletions src/core/Database.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "format/KdbxXmlReader.h"
#include "format/KeePass2Reader.h"
#include "format/KeePass2Writer.h"
#include "streams/HashingStream.h"

#include <QFileInfo>
#include <QJsonObject>
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand All @@ -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;
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -514,6 +567,9 @@ void Database::releaseData()
m_deletedObjects.clear();
m_commonUsernames.clear();
m_tagList.clear();

m_fileBlockHash.clear();
m_ignoreFileChangesUntilSaved = false;
}

/**
Expand Down Expand Up @@ -650,10 +706,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;
Expand Down
11 changes: 10 additions & 1 deletion src/core/Database.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -171,7 +178,7 @@ public slots:
void databaseOpened();
void databaseSaved();
void databaseDiscarded();
void databaseFileChanged();
void databaseFileChanged(bool triggeredBySave);
void databaseNonDataChanged();
void tagListUpdated();

Expand Down Expand Up @@ -223,6 +230,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;
Expand Down
27 changes: 15 additions & 12 deletions src/core/FileWatcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
}

Expand All @@ -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()
Expand Down
1 change: 1 addition & 0 deletions src/core/FileWatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
26 changes: 23 additions & 3 deletions src/gui/DatabaseOpenDialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Database> 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
Expand All @@ -207,7 +230,4 @@ void DatabaseOpenDialog::complete(bool accepted)
} else {
reject();
}

emit dialogFinished(accepted, m_currentDbWidget);
clearForms();
}
3 changes: 3 additions & 0 deletions src/gui/DatabaseOpenDialog.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define KEEPASSX_UNLOCKDATABASEDIALOG_H

#include "core/Global.h"
#include "gui/MessageWidget.h"

#include <QDialog>
#include <QList>
Expand Down Expand Up @@ -50,11 +51,13 @@ class DatabaseOpenDialog : public QDialog
Intent intent() const;
QSharedPointer<Database> 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);

Expand Down
5 changes: 5 additions & 0 deletions src/gui/DatabaseOpenWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading