From 870bd4af38c2b5f4e250cb93def6312af5af8a3b Mon Sep 17 00:00:00 2001 From: Loose Cannon Date: Mon, 2 Mar 2026 12:19:56 -0500 Subject: [PATCH] consolidate device ID sanitization and strengthen isPerDeviceMode check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move all sanitization (strip non-alphanumeric, truncate to 32, fallback to DEFAULT) into setDeviceId() so it's consistent regardless of call site. deviceId() now delegates to setDeviceId() and re-reads the sanitized value. isPerDeviceMode() now uses QFileInfo::isDir() as a positive filesystem check — no extension-based heuristics. Tests updated to create real temp directories instead of using synthetic paths. --- src/keeshare/KeeShare.cpp | 13 ++++----- src/keeshare/KeeShareSettings.cpp | 5 ++-- tests/TestSharing.cpp | 45 +++++++++++++++++++++++-------- tests/TestSharing.h | 3 +++ 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/src/keeshare/KeeShare.cpp b/src/keeshare/KeeShare.cpp index 65016270c..9dab173dc 100644 --- a/src/keeshare/KeeShare.cpp +++ b/src/keeshare/KeeShare.cpp @@ -67,22 +67,23 @@ QString KeeShare::deviceId() // Last resort: use hostname id = QSysInfo::machineHostName(); } - // Sanitize to [A-Za-z0-9] only - id.remove(QRegularExpression("[^A-Za-z0-9]")); - if (id.isEmpty()) { - id = "DEFAULT"; - } setDeviceId(id); + // Re-read the sanitized value + id = config()->get(Config::KeeShare_DeviceId).toString(); } return id; } void KeeShare::setDeviceId(const QString& id) { - // Sanitize to [A-Za-z0-9] only and enforce max length + // All sanitization consolidated here: strip non-alphanumeric, enforce max + // length, and fall back to DEFAULT if empty QString sanitized = id; sanitized.remove(QRegularExpression("[^A-Za-z0-9]")); sanitized.truncate(32); + if (sanitized.isEmpty()) { + sanitized = QStringLiteral("DEFAULT"); + } config()->set(Config::KeeShare_DeviceId, sanitized); } diff --git a/src/keeshare/KeeShareSettings.cpp b/src/keeshare/KeeShareSettings.cpp index 5eea29112..33425a85f 100644 --- a/src/keeshare/KeeShareSettings.cpp +++ b/src/keeshare/KeeShareSettings.cpp @@ -25,6 +25,7 @@ #include "gui/DatabaseIcons.h" #include +#include #include #include @@ -288,9 +289,7 @@ namespace KeeShareSettings bool Reference::isPerDeviceMode() const { - return !path.isEmpty() - && !path.endsWith(".kdbx", Qt::CaseInsensitive) - && !path.endsWith(".kdbx.share", Qt::CaseInsensitive); + return !path.isEmpty() && QFileInfo(path).isDir(); } bool Reference::operator<(const Reference& other) const diff --git a/tests/TestSharing.cpp b/tests/TestSharing.cpp index 4034a9882..c89035c77 100644 --- a/tests/TestSharing.cpp +++ b/tests/TestSharing.cpp @@ -17,6 +17,7 @@ #include "TestSharing.h" +#include #include #include @@ -35,6 +36,14 @@ Q_DECLARE_METATYPE(KeeShareSettings::Certificate) void TestSharing::initTestCase() { QVERIFY(Crypto::init()); + m_tempDir = new QTemporaryDir(); + QVERIFY(m_tempDir->isValid()); +} + +void TestSharing::cleanupTestCase() +{ + delete m_tempDir; + m_tempDir = nullptr; } void TestSharing::testNullObjects() @@ -192,18 +201,28 @@ void TestSharing::testPerDeviceMode_data() QTest::addColumn("path"); QTest::addColumn("expectedPerDevice"); - // Classic mode paths (file-based) + // Classic mode paths (file-based — don't need to exist on disk) QTest::newRow("kdbx file") << "/some/path/share.kdbx" << false; QTest::newRow("kdbx.share file") << "/some/path/share.kdbx.share" << false; QTest::newRow("KDBX uppercase") << "/some/path/share.KDBX" << false; QTest::newRow("KDBX.SHARE uppercase") << "/some/path/share.KDBX.SHARE" << false; QTest::newRow("empty path") << "" << false; + QTest::newRow("nonexistent path") << "/nonexistent/path/nowhere" << false; - // Per-device mode paths (directory-based) - QTest::newRow("directory path") << "/some/sync/dir" << true; - QTest::newRow("directory trailing slash") << "/some/sync/dir/" << true; - QTest::newRow("relative directory") << "sync/shared" << true; - QTest::newRow("directory with dots") << "/some/path.d/sync" << true; + // Per-device mode paths (real directories on disk) + auto base = m_tempDir->path(); + + QDir(base).mkpath("syncdir"); + QTest::newRow("directory path") << base + "/syncdir" << true; + + QDir(base).mkpath("syncdir_slash"); + QTest::newRow("directory trailing slash") << base + "/syncdir_slash/" << true; + + QDir(base).mkpath("sub/shared"); + QTest::newRow("nested directory") << base + "/sub/shared" << true; + + QDir(base).mkpath("path.d/sync"); + QTest::newRow("directory with dots") << base + "/path.d/sync" << true; } void TestSharing::testPerDeviceModeImportExport() @@ -228,16 +247,20 @@ void TestSharing::testPerDeviceModeImportExport_data() QTest::addColumn("expectedImporting"); QTest::addColumn("expectedExporting"); + auto base = m_tempDir->path(); + QDir(base).mkpath("importexport"); + auto dir = base + "/importexport"; + QTest::newRow("per-device sync imports") - << "/some/dir" << int(KeeShareSettings::SynchronizeWith) << true << true; + << dir << int(KeeShareSettings::SynchronizeWith) << true << true; QTest::newRow("per-device import only") - << "/some/dir" << int(KeeShareSettings::ImportFrom) << true << false; + << dir << int(KeeShareSettings::ImportFrom) << true << false; QTest::newRow("per-device export only") - << "/some/dir" << int(KeeShareSettings::ExportTo) << false << true; + << dir << int(KeeShareSettings::ExportTo) << false << true; QTest::newRow("classic file sync") - << "/some/dir/share.kdbx" << int(KeeShareSettings::SynchronizeWith) << true << true; + << dir + "/share.kdbx" << int(KeeShareSettings::SynchronizeWith) << true << true; QTest::newRow("inactive per-device") - << "/some/dir" << int(KeeShareSettings::Inactive) << false << false; + << dir << int(KeeShareSettings::Inactive) << false << false; } const QSharedPointer TestSharing::stubkey(int index) diff --git a/tests/TestSharing.h b/tests/TestSharing.h index 1c5d5cc20..7d2c994a3 100644 --- a/tests/TestSharing.h +++ b/tests/TestSharing.h @@ -19,6 +19,7 @@ #define KEEPASSXC_TESTSHARING_H #include +#include namespace Botan { @@ -30,6 +31,7 @@ class TestSharing : public QObject private slots: void initTestCase(); + void cleanupTestCase(); void testNullObjects(); void testKeySerialization(); void testReferenceSerialization(); @@ -43,6 +45,7 @@ private slots: private: const QSharedPointer stubkey(int index = 0); + QTemporaryDir* m_tempDir = nullptr; }; #endif // KEEPASSXC_TESTSHARING_H