From a5902899007ef6f4befea89a0af00ca315052ce6 Mon Sep 17 00:00:00 2001 From: Sergey Vilgelm Date: Fri, 15 Nov 2019 09:37:14 -0600 Subject: [PATCH 01/14] Add a new line after in Analyze command Adding a new line after the message "Evaluating database entries against HIBP file, this will take a while..." helps to separate a report and the comment. --- src/cli/Analyze.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cli/Analyze.cpp b/src/cli/Analyze.cpp index 3e6edcebf..7fc00c8ef 100644 --- a/src/cli/Analyze.cpp +++ b/src/cli/Analyze.cpp @@ -55,7 +55,8 @@ int Analyze::executeWithDatabase(QSharedPointer database, QSharedPoint return EXIT_FAILURE; } - outputTextStream << QObject::tr("Evaluating database entries against HIBP file, this will take a while..."); + outputTextStream << QObject::tr("Evaluating database entries against HIBP file, this will take a while...") + << endl; QList> findings; QString error; From e2c95f75f1869f86181eeecbec3458c5cd325f85 Mon Sep 17 00:00:00 2001 From: varjolintu Date: Tue, 12 Nov 2019 22:38:20 +0200 Subject: [PATCH 02/14] Fix subdomain matching --- src/browser/BrowserService.cpp | 34 ++-- src/browser/BrowserService.h | 6 +- tests/TestBrowser.cpp | 273 +++++++++++++++++---------------- tests/TestBrowser.h | 2 + 4 files changed, 167 insertions(+), 148 deletions(-) diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 392d28c43..2ec8d7fae 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -380,7 +380,7 @@ QJsonArray BrowserService::findMatchingEntries(const QString& id, // Check entries for authorization QList pwEntriesToConfirm; QList pwEntries; - for (auto* entry : searchEntries(url, keyList)) { + for (auto* entry : searchEntries(url, submitUrl, keyList)) { if (entry->customData()->contains(BrowserService::OPTION_HIDE_ENTRY) && entry->customData()->value(BrowserService::OPTION_HIDE_ENTRY) == "true") { continue; @@ -583,7 +583,7 @@ BrowserService::ReturnValue BrowserService::updateEntry(const QString& id, } QList -BrowserService::searchEntries(const QSharedPointer& db, const QString& hostname, const QString& url) +BrowserService::searchEntries(const QSharedPointer& db, const QString& url, const QString& submitUrl) { QList entries; auto* rootGroup = db->rootGroup(); @@ -601,20 +601,18 @@ BrowserService::searchEntries(const QSharedPointer& db, const QString& continue; } - auto domain = baseDomain(hostname); - // Search for additional URL's starting with KP2A_URL if (entry->attributes()->keys().contains(ADDITIONAL_URL)) { for (const auto& key : entry->attributes()->keys()) { if (key.startsWith(ADDITIONAL_URL) - && handleURL(entry->attributes()->value(key), domain, url)) { + && handleURL(entry->attributes()->value(key), url, submitUrl)) { entries.append(entry); continue; } } } - if (!handleURL(entry->url(), domain, url)) { + if (!handleURL(entry->url(), url, submitUrl)) { continue; } @@ -625,7 +623,7 @@ BrowserService::searchEntries(const QSharedPointer& db, const QString& return entries; } -QList BrowserService::searchEntries(const QString& url, const StringPairList& keyList) +QList BrowserService::searchEntries(const QString& url, const QString& submitUrl, const StringPairList& keyList) { // Check if database is connected with KeePassXC-Browser auto databaseConnected = [&](const QSharedPointer& db) { @@ -662,7 +660,7 @@ QList BrowserService::searchEntries(const QString& url, const StringPair QList entries; do { for (const auto& db : databases) { - entries << searchEntries(db, hostname, url); + entries << searchEntries(db, url, submitUrl); } } while (entries.isEmpty() && removeFirstDomain(hostname)); @@ -1000,7 +998,7 @@ bool BrowserService::removeFirstDomain(QString& hostname) return false; } -bool BrowserService::handleURL(const QString& entryUrl, const QString& hostname, const QString& url) +bool BrowserService::handleURL(const QString& entryUrl, const QString& url, const QString& submitUrl) { if (entryUrl.isEmpty()) { return false; @@ -1017,31 +1015,35 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& hostname, } } + // Make a direct compare if a local file is used + if (url.contains("file://")) { + return entryUrl == submitUrl; + } + // URL host validation fails - if (browserSettings()->matchUrlScheme() && entryQUrl.host().isEmpty()) { + if (entryQUrl.host().isEmpty()) { return false; } // Match port, if used - QUrl qUrl(url); - if (entryQUrl.port() > 0 && entryQUrl.port() != qUrl.port()) { + QUrl siteQUrl(url); + if (entryQUrl.port() > 0 && entryQUrl.port() != siteQUrl.port()) { return false; } // Match scheme - if (browserSettings()->matchUrlScheme() && !entryQUrl.scheme().isEmpty() && entryQUrl.scheme().compare(qUrl.scheme()) != 0) { + if (browserSettings()->matchUrlScheme() && !entryQUrl.scheme().isEmpty() && entryQUrl.scheme().compare(siteQUrl.scheme()) != 0) { return false; } // Check for illegal characters QRegularExpression re("[<>\\^`{|}]"); - auto match = re.match(entryUrl); - if (match.hasMatch()) { + if (re.match(entryUrl).hasMatch()) { return false; } // Filter to match hostname in URL field - if (entryQUrl.host().endsWith(hostname)) { + if (siteQUrl.host().endsWith(entryQUrl.host())) { return true; } diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index cb20ecbfb..6990eeda7 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -63,8 +63,8 @@ public: const QString& group, const QString& groupUuid, const QSharedPointer& selectedDb = {}); - QList searchEntries(const QSharedPointer& db, const QString& hostname, const QString& url); - QList searchEntries(const QString& url, const StringPairList& keyList); + QList searchEntries(const QSharedPointer& db, const QString& url, const QString& submitUrl); + QList searchEntries(const QString& url, const QString& submitUrl, const StringPairList& keyList); void convertAttributesToCustomData(const QSharedPointer& currentDb = {}); public: @@ -130,7 +130,7 @@ private: sortPriority(const Entry* entry, const QString& host, const QString& submitUrl, const QString& baseSubmitUrl) const; bool schemeFound(const QString& url); bool removeFirstDomain(QString& hostname); - bool handleURL(const QString& entryUrl, const QString& hostname, const QString& url); + bool handleURL(const QString& entryUrl, const QString& url, const QString& submitUrl); QString baseDomain(const QString& hostname) const; QSharedPointer getDatabase(); QSharedPointer selectedDatabase(); diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index bb3318d07..039d0b15e 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -179,29 +179,23 @@ void TestBrowser::testSearchEntries() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QList urls; - urls.push_back("https://github.com/login_page"); - urls.push_back("https://github.com/login"); - urls.push_back("https://github.com/"); - urls.push_back("github.com/login"); - urls.push_back("http://github.com"); - urls.push_back("http://github.com/login"); - urls.push_back("github.com"); - urls.push_back("github.com/login"); - urls.push_back("https://github"); // Invalid URL - urls.push_back("github.com"); + QStringList urls = { + "https://github.com/login_page", + "https://github.com/login", + "https://github.com/", + "github.com/login", + "http://github.com", + "http://github.com/login", + "github.com", + "github.com/login", + "https://github", // Invalid URL + "github.com" + }; - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - } + createEntries(urls, root); browserSettings()->setMatchUrlScheme(false); - auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + auto result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); // db, url, submitUrl QCOMPARE(result.length(), 9); QCOMPARE(result[0]->url(), QString("https://github.com/login_page")); @@ -213,7 +207,7 @@ void TestBrowser::testSearchEntries() // With matching there should be only 3 results + 4 without a scheme browserSettings()->setMatchUrlScheme(true); - result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); QCOMPARE(result.length(), 7); QCOMPARE(result[0]->url(), QString("https://github.com/login_page")); QCOMPARE(result[1]->url(), QString("https://github.com/login")); @@ -226,22 +220,16 @@ void TestBrowser::testSearchEntriesWithPort() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QList urls; - urls.push_back("http://127.0.0.1:443"); - urls.push_back("http://127.0.0.1:80"); + QStringList urls = { + "http://127.0.0.1:443", + "http://127.0.0.1:80" + }; - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - } + createEntries(urls, root); - auto result = m_browserService->searchEntries(db, "127.0.0.1", "http://127.0.0.1:443"); // db, hostname, url + auto result = m_browserService->searchEntries(db, "http://127.0.0.1:443", "http://127.0.0.1"); QCOMPARE(result.length(), 1); - QCOMPARE(result[0]->url(), urls[0]); + QCOMPARE(result[0]->url(), QString("http://127.0.0.1:443")); } void TestBrowser::testSearchEntriesWithAdditionalURLs() @@ -249,70 +237,59 @@ void TestBrowser::testSearchEntriesWithAdditionalURLs() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QList entries; - QList urls; - urls.push_back("https://github.com/"); - urls.push_back("https://www.example.com"); - urls.push_back("http://domain.com"); + QStringList urls = { + "https://github.com/", + "https://www.example.com", + "http://domain.com" + }; - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - entries.push_back(entry); - } + auto entries = createEntries(urls, root); // Add an additional URL to the first entry entries.first()->attributes()->set(BrowserService::ADDITIONAL_URL, "https://keepassxc.org"); - auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + auto result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); QCOMPARE(result.length(), 1); - QCOMPARE(result[0]->url(), urls[0]); + QCOMPARE(result[0]->url(), QString("https://github.com/")); // Search the additional URL. It should return the same entry - auto additionalResult = m_browserService->searchEntries(db, "keepassxc.org", "https://keepassxc.org"); + auto additionalResult = m_browserService->searchEntries(db, "https://keepassxc.org", "https://keepassxc.org"); QCOMPARE(additionalResult.length(), 1); - QCOMPARE(additionalResult[0]->url(), urls[0]); + QCOMPARE(additionalResult[0]->url(), QString("https://github.com/")); } void TestBrowser::testInvalidEntries() { auto db = QSharedPointer::create(); auto* root = db->rootGroup(); + const QString url("https://github.com"); + const QString submitUrl("https://github.com/session"); - QList urls; - urls.push_back("https://github.com/login"); - urls.push_back("https:///github.com/"); // Extra '/' - urls.push_back("http://github.com/**//*"); - urls.push_back("http://*.github.com/login"); - urls.push_back("//github.com"); // fromUserInput() corrects this one. - urls.push_back("github.com/{}<>"); - - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - } + QStringList urls = { + "https://github.com/login", + "https:///github.com/", // Extra '/' + "http://github.com/**//*", + "http://*.github.com/login", + "//github.com", // fromUserInput() corrects this one. + "github.com/{}<>", + "http:/example.com", + }; + + createEntries(urls, root); browserSettings()->setMatchUrlScheme(true); - auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + auto result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); QCOMPARE(result.length(), 2); QCOMPARE(result[0]->url(), QString("https://github.com/login")); QCOMPARE(result[1]->url(), QString("//github.com")); // Test the URL's directly - QCOMPARE(m_browserService->handleURL(urls[0], "github.com", "https://github.com"), true); - QCOMPARE(m_browserService->handleURL(urls[1], "github.com", "https://github.com"), false); - QCOMPARE(m_browserService->handleURL(urls[2], "github.com", "https://github.com"), false); - QCOMPARE(m_browserService->handleURL(urls[3], "github.com", "https://github.com"), false); - QCOMPARE(m_browserService->handleURL(urls[4], "github.com", "https://github.com"), true); - QCOMPARE(m_browserService->handleURL(urls[5], "github.com", "https://github.com"), false); + QCOMPARE(m_browserService->handleURL(urls[0], url, submitUrl), true); + QCOMPARE(m_browserService->handleURL(urls[1], url, submitUrl), false); + QCOMPARE(m_browserService->handleURL(urls[2], url, submitUrl), false); + QCOMPARE(m_browserService->handleURL(urls[3], url, submitUrl), false); + QCOMPARE(m_browserService->handleURL(urls[4], url, submitUrl), true); + QCOMPARE(m_browserService->handleURL(urls[5], url, submitUrl), false); } void TestBrowser::testSubdomainsAndPaths() @@ -320,44 +297,74 @@ void TestBrowser::testSubdomainsAndPaths() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QList urls; - urls.push_back("https://www.github.com/login/page.xml"); - urls.push_back("https://login.github.com/"); - urls.push_back("https://github.com"); - urls.push_back("http://www.github.com"); - urls.push_back("http://login.github.com/pathtonowhere"); - urls.push_back(".github.com"); // Invalid URL - urls.push_back("www.github.com/"); - urls.push_back("https://github"); // Invalid URL + QStringList urls = { + "https://www.github.com/login/page.xml", + "https://login.github.com/", + "https://github.com", + "http://www.github.com", + "http://login.github.com/pathtonowhere", + ".github.com", // Invalid URL + "www.github.com/", + "https://github" // Invalid URL + }; - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - } + createEntries(urls, root); browserSettings()->setMatchUrlScheme(false); - auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + auto result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); + QCOMPARE(result.length(), 1); + QCOMPARE(result[0]->url(), QString("https://github.com")); - QCOMPARE(result.length(), 6); - QCOMPARE(result[0]->url(), urls[0]); - QCOMPARE(result[1]->url(), urls[1]); - QCOMPARE(result[2]->url(), urls[2]); - QCOMPARE(result[3]->url(), urls[3]); - QCOMPARE(result[4]->url(), urls[4]); - QCOMPARE(result[5]->url(), urls[6]); - - // With matching there should be only 3 results - browserSettings()->setMatchUrlScheme(true); - result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url + // With www subdomain + result = m_browserService->searchEntries(db, "https://www.github.com", "https://www.github.com/session"); QCOMPARE(result.length(), 4); - QCOMPARE(result[0]->url(), urls[0]); - QCOMPARE(result[1]->url(), urls[1]); - QCOMPARE(result[2]->url(), urls[2]); - QCOMPARE(result[3]->url(), urls[6]); + QCOMPARE(result[0]->url(), QString("https://www.github.com/login/page.xml")); + QCOMPARE(result[1]->url(), QString("https://github.com")); // Accepts any subdomain + QCOMPARE(result[2]->url(), QString("http://www.github.com")); + QCOMPARE(result[3]->url(), QString("www.github.com/")); + + // With scheme matching there should be only 1 result + browserSettings()->setMatchUrlScheme(true); + result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); + QCOMPARE(result.length(), 1); + QCOMPARE(result[0]->url(), QString("https://github.com")); + + // Test site with subdomain in the site URL + QStringList entryURLs = { + "https://accounts.example.com", + "https://accounts.example.com/path", + "https://subdomain.example.com/", + "https://another.accounts.example.com/", + "https://another.subdomain.example.com/", + "https://example.com/", + "https://example" // Invalid URL + }; + + createEntries(entryURLs, root); + + result = m_browserService->searchEntries(db, "https://accounts.example.com", "https://accounts.example.com"); + QCOMPARE(result.length(), 3); + QCOMPARE(result[0]->url(), QString("https://accounts.example.com")); + QCOMPARE(result[1]->url(), QString("https://accounts.example.com/path")); + QCOMPARE(result[2]->url(), QString("https://example.com/")); // Accepts any subdomain + + result = m_browserService->searchEntries(db, "https://another.accounts.example.com", "https://another.accounts.example.com"); + QCOMPARE(result.length(), 4); + QCOMPARE(result[0]->url(), QString("https://accounts.example.com")); // Accepts any subdomain under accounts.example.com + QCOMPARE(result[1]->url(), QString("https://accounts.example.com/path")); + QCOMPARE(result[2]->url(), QString("https://another.accounts.example.com/")); + QCOMPARE(result[3]->url(), QString("https://example.com/")); // Accepts one or more subdomains + + // Test local files. It should be a direct match. + QStringList localFiles = { + "file:///Users/testUser/tests/test.html" + }; + + createEntries(localFiles, root); + + // With local files, url is always set to the file scheme + ://. Submit URL holds the actual URL. + result = m_browserService->searchEntries(db, "file://", "file:///Users/testUser/tests/test.html"); + QCOMPARE(result.length(), 1); } void TestBrowser::testSortEntries() @@ -365,28 +372,20 @@ void TestBrowser::testSortEntries() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QList urls; - urls.push_back("https://github.com/login_page"); - urls.push_back("https://github.com/login"); - urls.push_back("https://github.com/"); - urls.push_back("github.com/login"); - urls.push_back("http://github.com"); - urls.push_back("http://github.com/login"); - urls.push_back("github.com"); - urls.push_back("github.com/login"); - urls.push_back("https://github"); // Invalid URL - urls.push_back("github.com"); + QStringList urls = { + "https://github.com/login_page", + "https://github.com/login", + "https://github.com/", + "github.com/login", + "http://github.com", + "http://github.com/login", + "github.com", + "github.com/login", + "https://github", // Invalid URL + "github.com" + }; - QList entries; - for (int i = 0; i < urls.length(); ++i) { - auto entry = new Entry(); - entry->setGroup(root); - entry->beginUpdate(); - entry->setUrl(urls[i]); - entry->setUsername(QString("User %1").arg(i)); - entry->endUpdate(); - entries.push_back(entry); - } + auto entries = createEntries(urls, root); browserSettings()->setBestMatchOnly(false); auto result = @@ -458,3 +457,19 @@ void TestBrowser::testGetDatabaseGroups() auto lastChild = lastChildren.at(0); QCOMPARE(lastChild.toObject()["name"].toString(), QString("group2_1_1")); } + +QList TestBrowser::createEntries(QStringList& urls, Group* root) const +{ + QList entries; + for (int i = 0; i < urls.length(); ++i) { + auto entry = new Entry(); + entry->setGroup(root); + entry->beginUpdate(); + entry->setUrl(urls[i]); + entry->setUsername(QString("User %1").arg(i)); + entry->endUpdate(); + entries.push_back(entry); + } + + return entries; +} \ No newline at end of file diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index 981c1642d..8b2dc3e3c 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -49,6 +49,8 @@ private slots: void testGetDatabaseGroups(); private: + QList createEntries(QStringList& urls, Group* root) const; + QScopedPointer m_browserAction; QScopedPointer m_browserService; }; From a66f8ec04db33e2a5cbc224c03b7ec9865e110c9 Mon Sep 17 00:00:00 2001 From: Aetf Date: Thu, 14 Nov 2019 16:06:10 -0500 Subject: [PATCH 03/14] FdoSecrets: fix crash when enabling the plugin on a non-exposed database --- src/fdosecrets/objects/Collection.h | 5 +++++ src/fdosecrets/objects/Service.cpp | 26 +++++++++++++++++--------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index de9db3a49..902860165 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -74,6 +74,10 @@ namespace FdoSecrets public: DBusReturn setProperties(const QVariantMap& properties); + bool isValid() const { + return backend(); + } + DBusReturn removeAlias(QString alias); DBusReturn addAlias(QString alias); const QSet aliases() const; @@ -106,6 +110,7 @@ namespace FdoSecrets private slots: void onDatabaseLockChanged(); void onDatabaseExposedGroupChanged(); + // force reload info from backend, potentially delete self void reloadBackend(); private: diff --git a/src/fdosecrets/objects/Service.cpp b/src/fdosecrets/objects/Service.cpp index 6bca1f12c..eeded79ba 100644 --- a/src/fdosecrets/objects/Service.cpp +++ b/src/fdosecrets/objects/Service.cpp @@ -93,7 +93,24 @@ namespace FdoSecrets void Service::onDatabaseTabOpened(DatabaseWidget* dbWidget, bool emitSignal) { + // The Collection will monitor the database's exposed group. + // When the Collection finds that no exposed group, it will delete itself. + // Thus the service also needs to monitor it and recreate the collection if the user changes + // from no exposed to exposed something. + if (!dbWidget->isLocked()) { + monitorDatabaseExposedGroup(dbWidget); + } + connect(dbWidget, &DatabaseWidget::databaseUnlocked, this, [this, dbWidget]() { + monitorDatabaseExposedGroup(dbWidget); + }); + auto coll = new Collection(this, dbWidget); + // Creation may fail if the database is not exposed. + // This is okay, because we monitor the expose settings above + if (!coll->isValid()) { + coll->deleteLater(); + return; + } m_collections << coll; m_dbToCollection[dbWidget] = coll; @@ -127,15 +144,6 @@ namespace FdoSecrets emit collectionDeleted(coll); }); - // a special case: the database changed from no expose to expose something. - // in this case, there is no collection out there monitoring it, so create a new collection - if (!dbWidget->isLocked()) { - monitorDatabaseExposedGroup(dbWidget); - } - connect(dbWidget, &DatabaseWidget::databaseUnlocked, this, [this, dbWidget]() { - monitorDatabaseExposedGroup(dbWidget); - }); - if (emitSignal) { emit collectionCreated(coll); } From 024e00cc97356dec78f7cdc4e6e1efe44c84c1d7 Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Sat, 16 Nov 2019 21:36:17 +0000 Subject: [PATCH 04/14] Only show warning about snap browsers on Linux --- src/browser/BrowserOptionDialog.cpp | 4 ++++ src/browser/BrowserOptionDialog.ui | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/browser/BrowserOptionDialog.cpp b/src/browser/BrowserOptionDialog.cpp index a5bb921da..56c6cceb8 100644 --- a/src/browser/BrowserOptionDialog.cpp +++ b/src/browser/BrowserOptionDialog.cpp @@ -68,6 +68,10 @@ BrowserOptionDialog::BrowserOptionDialog(QWidget* parent) connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), m_ui->customProxyLocationBrowseButton, SLOT(setEnabled(bool))); connect(m_ui->customProxyLocationBrowseButton, SIGNAL(clicked()), this, SLOT(showProxyLocationFileDialog())); +#ifndef Q_OS_LINUX + m_ui->snapWarningLabel->setVisible(false); +#endif + #ifdef Q_OS_WIN // Brave uses Chrome's registry settings m_ui->braveSupport->setHidden(true); diff --git a/src/browser/BrowserOptionDialog.ui b/src/browser/BrowserOptionDialog.ui index 638c400aa..84fc5bdbf 100755 --- a/src/browser/BrowserOptionDialog.ui +++ b/src/browser/BrowserOptionDialog.ui @@ -60,7 +60,7 @@ - + Browsers installed as snaps are currently not supported. From 697f26524927677fa3cc6f5573610bcf5a80cf7b Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Sun, 17 Nov 2019 19:33:28 +0000 Subject: [PATCH 05/14] Correctly initialize standalone PW generator mode --- src/gui/MainWindow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index 0d53d88a8..6452c051b 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -885,8 +885,8 @@ void MainWindow::switchToPasswordGen(bool enabled) if (enabled) { m_ui->passwordGeneratorWidget->loadSettings(); m_ui->passwordGeneratorWidget->regeneratePassword(); - m_ui->passwordGeneratorWidget->setStandaloneMode(true); m_ui->stackedWidget->setCurrentIndex(PasswordGeneratorScreen); + m_ui->passwordGeneratorWidget->setStandaloneMode(true); } else { m_ui->passwordGeneratorWidget->saveSettings(); switchToDatabases(); From 5c54dfe581893853401307fa55bc4f3fec330f72 Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Sun, 17 Nov 2019 13:03:05 +0000 Subject: [PATCH 06/14] Release database before exiting CLI interactive mode --- src/cli/keepassxc-cli.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cli/keepassxc-cli.cpp b/src/cli/keepassxc-cli.cpp index e16be9b29..98cc6be06 100644 --- a/src/cli/keepassxc-cli.cpp +++ b/src/cli/keepassxc-cli.cpp @@ -149,6 +149,7 @@ void enterInteractiveMode(const QStringList& arguments) prompt += "> "; command = reader->readLine(prompt); if (reader->isFinished()) { + currentDatabase->releaseData(); return; } @@ -162,6 +163,7 @@ void enterInteractiveMode(const QStringList& arguments) errorTextStream << QObject::tr("Unknown command %1").arg(args[0]) << "\n"; continue; } else if (cmd->name == "quit" || cmd->name == "exit") { + currentDatabase->releaseData(); return; } From ed60a3dcce8402f36a6264b435f4885f72ffc6db Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Thu, 21 Nov 2019 22:14:57 +0000 Subject: [PATCH 07/14] HTML encode url in ElidedLabel * Fix #3905 - prevent double quotes and other invalid HTML characters from impeding on display of url in ElidedLabel --- src/gui/widgets/ElidedLabel.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/gui/widgets/ElidedLabel.cpp b/src/gui/widgets/ElidedLabel.cpp index bc2771764..187c2fc43 100644 --- a/src/gui/widgets/ElidedLabel.cpp +++ b/src/gui/widgets/ElidedLabel.cpp @@ -105,8 +105,10 @@ void ElidedLabel::updateElidedText() const QFontMetrics metrix(font()); displayText = metrix.elidedText(m_rawText, m_elideMode, width() - 2); } - setText(m_url.isEmpty() ? displayText : htmlLinkTemplate.arg(m_url, displayText)); - setOpenExternalLinks(!m_url.isEmpty()); + + bool hasUrl = !m_url.isEmpty(); + setText(hasUrl ? htmlLinkTemplate.arg(m_url.toHtmlEscaped(), displayText) : displayText); + setOpenExternalLinks(!hasUrl); } void ElidedLabel::resizeEvent(QResizeEvent* event) From dc6c9186c9e2a05c080d23c27ae2dc7526d4c1dd Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sat, 23 Nov 2019 09:53:32 -0500 Subject: [PATCH 08/14] Fix start minimized to tray for unix --- src/core/Bootstrap.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/Bootstrap.cpp b/src/core/Bootstrap.cpp index 204942f4d..46aff92d5 100644 --- a/src/core/Bootstrap.cpp +++ b/src/core/Bootstrap.cpp @@ -106,7 +106,11 @@ namespace Bootstrap { // start minimized if configured if (config()->get("GUI/MinimizeOnStartup").toBool()) { +#ifdef Q_OS_WIN mainWindow.showMinimized(); +#else + mainWindow.hideWindow(); +#endif } else { mainWindow.bringToFront(); } From 6c65b486e4d6ecdd1f4ab819bf01f4ca696b293a Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Mon, 18 Nov 2019 08:17:26 -0500 Subject: [PATCH 09/14] Disable database unlock form while decrypting --- src/gui/DatabaseOpenWidget.cpp | 5 +++++ src/gui/DatabaseOpenWidget.ui | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index a409aadc3..7190fb389 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -204,9 +204,14 @@ void DatabaseOpenWidget::openDatabase() m_db.reset(new Database()); QString error; + QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); + m_ui->passwordFormFrame->setEnabled(false); + QCoreApplication::processEvents(); bool ok = m_db->open(m_filename, masterKey, &error, false); QApplication::restoreOverrideCursor(); + m_ui->passwordFormFrame->setEnabled(true); + if (!ok) { if (m_ui->editPassword->text().isEmpty() && !m_retryUnlockWithEmptyPassword) { QScopedPointer msgBox(new QMessageBox(this)); diff --git a/src/gui/DatabaseOpenWidget.ui b/src/gui/DatabaseOpenWidget.ui index 14a1337c6..60b2feadc 100644 --- a/src/gui/DatabaseOpenWidget.ui +++ b/src/gui/DatabaseOpenWidget.ui @@ -132,7 +132,7 @@ 15 - + 400 From c0b1c9e1062afbad86b365eac1c13fa3d82488a1 Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Mon, 18 Nov 2019 06:57:04 +0000 Subject: [PATCH 10/14] Run code formatter --- src/browser/BrowserService.cpp | 6 +- src/cli/Analyze.cpp | 3 +- src/core/Database.h | 2 +- src/fdosecrets/objects/Collection.h | 3 +- src/gui/DatabaseOpenWidget.cpp | 8 ++- src/gui/DatabaseTabWidget.cpp | 3 +- src/gui/dbsettings/DatabaseSettingsWidget.cpp | 1 - src/gui/masterkey/KeyFileEditWidget.h | 1 - tests/TestBrowser.cpp | 71 ++++++++----------- tests/TestCli.cpp | 2 +- tests/TestFdoSecrets.cpp | 4 +- tests/util/TemporaryFile.cpp | 2 +- 12 files changed, 48 insertions(+), 58 deletions(-) diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 2ec8d7fae..e548b370c 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -604,8 +604,7 @@ BrowserService::searchEntries(const QSharedPointer& db, const QString& // Search for additional URL's starting with KP2A_URL if (entry->attributes()->keys().contains(ADDITIONAL_URL)) { for (const auto& key : entry->attributes()->keys()) { - if (key.startsWith(ADDITIONAL_URL) - && handleURL(entry->attributes()->value(key), url, submitUrl)) { + if (key.startsWith(ADDITIONAL_URL) && handleURL(entry->attributes()->value(key), url, submitUrl)) { entries.append(entry); continue; } @@ -1032,7 +1031,8 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& url, cons } // Match scheme - if (browserSettings()->matchUrlScheme() && !entryQUrl.scheme().isEmpty() && entryQUrl.scheme().compare(siteQUrl.scheme()) != 0) { + if (browserSettings()->matchUrlScheme() && !entryQUrl.scheme().isEmpty() + && entryQUrl.scheme().compare(siteQUrl.scheme()) != 0) { return false; } diff --git a/src/cli/Analyze.cpp b/src/cli/Analyze.cpp index 7fc00c8ef..6095e988b 100644 --- a/src/cli/Analyze.cpp +++ b/src/cli/Analyze.cpp @@ -55,8 +55,7 @@ int Analyze::executeWithDatabase(QSharedPointer database, QSharedPoint return EXIT_FAILURE; } - outputTextStream << QObject::tr("Evaluating database entries against HIBP file, this will take a while...") - << endl; + outputTextStream << QObject::tr("Evaluating database entries against HIBP file, this will take a while...") << endl; QList> findings; QString error; diff --git a/src/core/Database.h b/src/core/Database.h index 9c9929945..d5f2092b2 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -29,8 +29,8 @@ #include "crypto/kdf/AesKdf.h" #include "crypto/kdf/Kdf.h" #include "format/KeePass2.h" -#include "keys/PasswordKey.h" #include "keys/CompositeKey.h" +#include "keys/PasswordKey.h" class Entry; enum class EntryReferenceType; diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h index 902860165..06d45a50e 100644 --- a/src/fdosecrets/objects/Collection.h +++ b/src/fdosecrets/objects/Collection.h @@ -74,7 +74,8 @@ namespace FdoSecrets public: DBusReturn setProperties(const QVariantMap& properties); - bool isValid() const { + bool isValid() const + { return backend(); } diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 7190fb389..c610a773d 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -375,9 +375,11 @@ void DatabaseOpenWidget::browseKeyFile() QString filename = fileDialog()->getOpenFileName(this, tr("Select key file"), QString(), filters); if (QFileInfo(filename).canonicalFilePath() == QFileInfo(m_filename).canonicalFilePath()) { - MessageBox::warning(this, tr("Cannot use database file as key file"), - tr("You cannot use your database file as a key file.\nIf you do not have a key file, please leave the field empty."), - MessageBox::Button::Ok); + MessageBox::warning(this, + tr("Cannot use database file as key file"), + tr("You cannot use your database file as a key file.\nIf you do not have a key file, " + "please leave the field empty."), + MessageBox::Button::Ok); filename = ""; } diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index 9cbfa8fd7..c80726417 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -166,7 +166,8 @@ void DatabaseTabWidget::addDatabaseTab(const QString& filePath, for (int i = 0, c = count(); i < c; ++i) { auto* dbWidget = databaseWidgetFromIndex(i); Q_ASSERT(dbWidget); - if (dbWidget && dbWidget->database()->canonicalFilePath().compare(canonicalFilePath, FILE_CASE_SENSITIVE) == 0) { + if (dbWidget + && dbWidget->database()->canonicalFilePath().compare(canonicalFilePath, FILE_CASE_SENSITIVE) == 0) { dbWidget->performUnlockDatabase(password, keyfile); if (!inBackground) { // switch to existing tab if file is already open diff --git a/src/gui/dbsettings/DatabaseSettingsWidget.cpp b/src/gui/dbsettings/DatabaseSettingsWidget.cpp index 224c4e565..c7f7f400d 100644 --- a/src/gui/dbsettings/DatabaseSettingsWidget.cpp +++ b/src/gui/dbsettings/DatabaseSettingsWidget.cpp @@ -48,4 +48,3 @@ const QSharedPointer DatabaseSettingsWidget::getDatabase() const { return m_db; } - diff --git a/src/gui/masterkey/KeyFileEditWidget.h b/src/gui/masterkey/KeyFileEditWidget.h index 7d5868e88..dd414e133 100644 --- a/src/gui/masterkey/KeyFileEditWidget.h +++ b/src/gui/masterkey/KeyFileEditWidget.h @@ -32,7 +32,6 @@ class KeyFileEditWidget : public KeyComponentWidget { Q_OBJECT - public: explicit KeyFileEditWidget(DatabaseSettingsWidget* parent); Q_DISABLE_COPY(KeyFileEditWidget); diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 039d0b15e..938a7e4e5 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -179,23 +179,22 @@ void TestBrowser::testSearchEntries() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QStringList urls = { - "https://github.com/login_page", - "https://github.com/login", - "https://github.com/", - "github.com/login", - "http://github.com", - "http://github.com/login", - "github.com", - "github.com/login", - "https://github", // Invalid URL - "github.com" - }; + QStringList urls = {"https://github.com/login_page", + "https://github.com/login", + "https://github.com/", + "github.com/login", + "http://github.com", + "http://github.com/login", + "github.com", + "github.com/login", + "https://github", // Invalid URL + "github.com"}; createEntries(urls, root); browserSettings()->setMatchUrlScheme(false); - auto result = m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); // db, url, submitUrl + auto result = + m_browserService->searchEntries(db, "https://github.com", "https://github.com/session"); // db, url, submitUrl QCOMPARE(result.length(), 9); QCOMPARE(result[0]->url(), QString("https://github.com/login_page")); @@ -220,10 +219,7 @@ void TestBrowser::testSearchEntriesWithPort() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QStringList urls = { - "http://127.0.0.1:443", - "http://127.0.0.1:80" - }; + QStringList urls = {"http://127.0.0.1:443", "http://127.0.0.1:80"}; createEntries(urls, root); @@ -237,11 +233,7 @@ void TestBrowser::testSearchEntriesWithAdditionalURLs() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QStringList urls = { - "https://github.com/", - "https://www.example.com", - "http://domain.com" - }; + QStringList urls = {"https://github.com/", "https://www.example.com", "http://domain.com"}; auto entries = createEntries(urls, root); @@ -274,7 +266,7 @@ void TestBrowser::testInvalidEntries() "github.com/{}<>", "http:/example.com", }; - + createEntries(urls, root); browserSettings()->setMatchUrlScheme(true); @@ -348,17 +340,17 @@ void TestBrowser::testSubdomainsAndPaths() QCOMPARE(result[1]->url(), QString("https://accounts.example.com/path")); QCOMPARE(result[2]->url(), QString("https://example.com/")); // Accepts any subdomain - result = m_browserService->searchEntries(db, "https://another.accounts.example.com", "https://another.accounts.example.com"); + result = m_browserService->searchEntries( + db, "https://another.accounts.example.com", "https://another.accounts.example.com"); QCOMPARE(result.length(), 4); - QCOMPARE(result[0]->url(), QString("https://accounts.example.com")); // Accepts any subdomain under accounts.example.com + QCOMPARE(result[0]->url(), + QString("https://accounts.example.com")); // Accepts any subdomain under accounts.example.com QCOMPARE(result[1]->url(), QString("https://accounts.example.com/path")); QCOMPARE(result[2]->url(), QString("https://another.accounts.example.com/")); QCOMPARE(result[3]->url(), QString("https://example.com/")); // Accepts one or more subdomains // Test local files. It should be a direct match. - QStringList localFiles = { - "file:///Users/testUser/tests/test.html" - }; + QStringList localFiles = {"file:///Users/testUser/tests/test.html"}; createEntries(localFiles, root); @@ -372,18 +364,16 @@ void TestBrowser::testSortEntries() auto db = QSharedPointer::create(); auto* root = db->rootGroup(); - QStringList urls = { - "https://github.com/login_page", - "https://github.com/login", - "https://github.com/", - "github.com/login", - "http://github.com", - "http://github.com/login", - "github.com", - "github.com/login", - "https://github", // Invalid URL - "github.com" - }; + QStringList urls = {"https://github.com/login_page", + "https://github.com/login", + "https://github.com/", + "github.com/login", + "http://github.com", + "http://github.com/login", + "github.com", + "github.com/login", + "https://github", // Invalid URL + "github.com"}; auto entries = createEntries(urls, root); @@ -399,7 +389,6 @@ void TestBrowser::testSortEntries() QCOMPARE(result[2]->url(), QString("https://github.com/login")); QCOMPARE(result[3]->username(), QString("User 3")); QCOMPARE(result[3]->url(), QString("github.com/login")); - } void TestBrowser::testGetDatabaseGroups() diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index 8a9ab50ce..9a2756eac 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -23,13 +23,13 @@ #include "core/Global.h" #include "core/Tools.h" #include "crypto/Crypto.h" -#include "keys/drivers/YubiKey.h" #include "format/Kdbx3Reader.h" #include "format/Kdbx3Writer.h" #include "format/Kdbx4Reader.h" #include "format/Kdbx4Writer.h" #include "format/KdbxXmlReader.h" #include "format/KeePass2.h" +#include "keys/drivers/YubiKey.h" #include "cli/Add.h" #include "cli/AddGroup.h" diff --git a/tests/TestFdoSecrets.cpp b/tests/TestFdoSecrets.cpp index 6994f60ab..30d8da5c7 100644 --- a/tests/TestFdoSecrets.cpp +++ b/tests/TestFdoSecrets.cpp @@ -21,9 +21,9 @@ #include "core/EntrySearcher.h" #include "fdosecrets/GcryptMPI.h" -#include "fdosecrets/objects/SessionCipher.h" #include "fdosecrets/objects/Collection.h" #include "fdosecrets/objects/Item.h" +#include "fdosecrets/objects/SessionCipher.h" #include "crypto/Crypto.h" @@ -96,8 +96,8 @@ void TestFdoSecrets::testDhIetf1024Sha256Aes128CbcPkcs7() void TestFdoSecrets::testCrazyAttributeKey() { - using FdoSecrets::Item; using FdoSecrets::Collection; + using FdoSecrets::Item; const QScopedPointer root(new Group()); const QScopedPointer e1(new Entry()); diff --git a/tests/util/TemporaryFile.cpp b/tests/util/TemporaryFile.cpp index 19622faed..3b1e3a589 100644 --- a/tests/util/TemporaryFile.cpp +++ b/tests/util/TemporaryFile.cpp @@ -71,7 +71,7 @@ bool TemporaryFile::copyFromFile(const QString& otherFileName) } QByteArray data; - while(!(data = otherFile.read(1024)).isEmpty()) { + while (!(data = otherFile.read(1024)).isEmpty()) { write(data); } From 936312304715b9ef85f6a6737ee408b5646c4985 Mon Sep 17 00:00:00 2001 From: Balazs Gyurak Date: Mon, 18 Nov 2019 06:55:35 +0000 Subject: [PATCH 11/14] Add ability to hide a protected attribute after reveal --- src/gui/entry/EditEntryWidget.cpp | 9 +++++++-- src/gui/entry/EditEntryWidget.h | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp index 3fa75e3b8..b80a4850d 100644 --- a/src/gui/entry/EditEntryWidget.cpp +++ b/src/gui/entry/EditEntryWidget.cpp @@ -201,7 +201,7 @@ void EditEntryWidget::setupAdvanced() connect(m_advancedUi->editAttributeButton, SIGNAL(clicked()), SLOT(editCurrentAttribute())); connect(m_advancedUi->removeAttributeButton, SIGNAL(clicked()), SLOT(removeCurrentAttribute())); connect(m_advancedUi->protectAttributeButton, SIGNAL(toggled(bool)), SLOT(protectCurrentAttribute(bool))); - connect(m_advancedUi->revealAttributeButton, SIGNAL(clicked(bool)), SLOT(revealCurrentAttribute())); + connect(m_advancedUi->revealAttributeButton, SIGNAL(clicked(bool)), SLOT(toggleCurrentAttributeVisibility())); connect(m_advancedUi->attributesView->selectionModel(), SIGNAL(currentChanged(QModelIndex,QModelIndex)), SLOT(updateCurrentAttribute())); @@ -1297,6 +1297,7 @@ void EditEntryWidget::displayAttribute(QModelIndex index, bool showProtected) // Block signals to prevent modified being set m_advancedUi->protectAttributeButton->blockSignals(true); m_advancedUi->attributesEdit->blockSignals(true); + m_advancedUi->revealAttributeButton->setText(tr("Reveal")); if (index.isValid()) { QString key = m_attributesModel->keyByIndex(index); @@ -1348,7 +1349,7 @@ void EditEntryWidget::protectCurrentAttribute(bool state) } } -void EditEntryWidget::revealCurrentAttribute() +void EditEntryWidget::toggleCurrentAttributeVisibility() { if (!m_advancedUi->attributesEdit->isEnabled()) { QModelIndex index = m_advancedUi->attributesView->currentIndex(); @@ -1359,6 +1360,10 @@ void EditEntryWidget::revealCurrentAttribute() m_advancedUi->attributesEdit->setEnabled(true); m_advancedUi->attributesEdit->blockSignals(oldBlockSignals); } + m_advancedUi->revealAttributeButton->setText(tr("Hide")); + } else { + protectCurrentAttribute(true); + m_advancedUi->revealAttributeButton->setText(tr("Reveal")); } } diff --git a/src/gui/entry/EditEntryWidget.h b/src/gui/entry/EditEntryWidget.h index 39b5fc5d9..300220cd0 100644 --- a/src/gui/entry/EditEntryWidget.h +++ b/src/gui/entry/EditEntryWidget.h @@ -93,7 +93,7 @@ private slots: void removeCurrentAttribute(); void updateCurrentAttribute(); void protectCurrentAttribute(bool state); - void revealCurrentAttribute(); + void toggleCurrentAttributeVisibility(); void updateAutoTypeEnabled(); void openAutotypeHelp(); void insertAutoTypeAssoc(); From 6dd9702b79b7fb186bd153e4be58f4d83c964e66 Mon Sep 17 00:00:00 2001 From: Aetf Date: Fri, 22 Nov 2019 13:36:08 -0500 Subject: [PATCH 12/14] FdoSecrets: handle the exposed group being moved to recycle bin --- src/fdosecrets/objects/Collection.cpp | 40 +++++++++++++------ .../DatabaseSettingsWidgetFdoSecrets.cpp | 9 ++++- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index c826c1db0..30a8f2d0c 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -328,7 +328,11 @@ namespace FdoSecrets // when creation finishes in backend, we will already have item item = m_entryToItem.value(entry, nullptr); - Q_ASSERT(item); + + if (!item) { + // may happen if entry somehow ends up in recycle bin + return DBusReturn<>::Error(QStringLiteral(DBUS_ERROR_SECRET_NO_SUCH_OBJECT)); + } } ret = item->setProperties(properties); @@ -439,7 +443,7 @@ namespace FdoSecrets auto newUuid = FdoSecrets::settings()->exposedGroup(m_backend->database()); auto newGroup = m_backend->database()->rootGroup()->findGroupByUuid(newUuid); - if (!newGroup) { + if (!newGroup || inRecycleBin(newGroup)) { // no exposed group, delete self doDelete(); return; @@ -451,10 +455,11 @@ namespace FdoSecrets m_exposedGroup = newGroup; // Attach signal to update exposed group settings if the group was removed. + // // The lifetime of the connection is bound to the database object, because - // in Database::~Database, groups are also deleted, but we don't want to - // trigger this. - // This rely on the fact that QObject disconnects signals BEFORE deleting + // in Database::~Database, groups are also deleted as children, but we don't + // want to trigger this. + // This works because the fact that QObject disconnects signals BEFORE deleting // children. QPointer db = m_backend->database().data(); connect(m_exposedGroup.data(), &Group::groupAboutToRemove, db, [db](Group* toBeRemoved) { @@ -468,6 +473,13 @@ namespace FdoSecrets FdoSecrets::settings()->setExposedGroup(db, {}); } }); + // Another possibility is the group being moved to recycle bin. + connect(m_exposedGroup.data(), &Group::groupModified, this, [this]() { + if (inRecycleBin(m_exposedGroup->parentGroup())) { + // reset the exposed group to none + FdoSecrets::settings()->setExposedGroup(m_backend->database().data(), {}); + } + }); // Monitor exposed group settings connect(m_backend->database()->metadata()->customData(), &CustomData::customDataModified, this, [this]() { @@ -646,17 +658,21 @@ namespace FdoSecrets { Q_ASSERT(m_backend); - if (!m_backend->database()->metadata()->recycleBin()) { + if (!group) { + // just to be safe + return true; + } + + if (!m_backend->database()->metadata()) { return false; } - while (group) { - if (group->uuid() == m_backend->database()->metadata()->recycleBin()->uuid()) { - return true; - } - group = group->parentGroup(); + auto recycleBin = m_backend->database()->metadata()->recycleBin(); + if (!recycleBin) { + return false; } - return false; + + return group->uuid() == recycleBin->uuid() || group->isRecycled(); } bool Collection::inRecycleBin(Entry* entry) const diff --git a/src/fdosecrets/widgets/DatabaseSettingsWidgetFdoSecrets.cpp b/src/fdosecrets/widgets/DatabaseSettingsWidgetFdoSecrets.cpp index fadd01542..16a780de7 100644 --- a/src/fdosecrets/widgets/DatabaseSettingsWidgetFdoSecrets.cpp +++ b/src/fdosecrets/widgets/DatabaseSettingsWidgetFdoSecrets.cpp @@ -85,7 +85,7 @@ protected: // can not call mapFromSource, which internally calls filterAcceptsRow auto group = groupFromSourceIndex(source_idx); - return group->uuid() != recycleBin->uuid(); + return group && !group->isRecycled() && group->uuid() != recycleBin->uuid(); } }; @@ -118,8 +118,13 @@ void DatabaseSettingsWidgetFdoSecrets::loadSettings(QSharedPointer db) m_model.reset(new GroupModelNoRecycle(m_db.data())); m_ui->selectGroup->setModel(m_model.data()); + Group* recycleBin = nullptr; + if (m_db->metadata() && m_db->metadata()->recycleBin()) { + recycleBin = m_db->metadata()->recycleBin(); + } + auto group = m_db->rootGroup()->findGroupByUuid(FdoSecrets::settings()->exposedGroup(m_db)); - if (!group) { + if (!group || group->isRecycled() || (recycleBin && group->uuid() == recycleBin->uuid())) { m_ui->radioDonotExpose->setChecked(true); } else { auto idx = m_model->indexFromGroup(group); From f9cb2bd5dfaf58b3fd45272e8b0427aa0d71b0fa Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sun, 8 Dec 2019 09:52:01 -0500 Subject: [PATCH 13/14] Correct multiple issues with database saving * Mark the database as clean after fully completing the file save operation INSTEAD of when merely writing the database to a file. * Stop the modified timer when marking the database as clean, this prevents latent erroneous modified signals from being emitted. * Do not restart the modified timer after a new change is detected while it is still running. --- src/core/Database.cpp | 27 ++++++++------------------- src/core/Database.h | 8 ++------ src/gui/DatabaseWidget.cpp | 2 +- 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 4cccd6d53..066abc4e3 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -42,7 +42,6 @@ Database::Database() : m_metadata(new Metadata(this)) , m_data() , m_rootGroup(nullptr) - , m_timer(new QTimer(this)) , m_fileWatcher(new FileWatcher(this)) , m_emitModified(false) , m_uuid(QUuid::createUuid()) @@ -50,12 +49,12 @@ Database::Database() setRootGroup(new Group()); rootGroup()->setUuid(QUuid::createUuid()); rootGroup()->setName(tr("Root", "Root group name")); - m_timer->setSingleShot(true); + m_modifiedTimer.setSingleShot(true); s_uuidMap.insert(m_uuid, this); connect(m_metadata, SIGNAL(metadataModified()), SLOT(markAsModified())); - connect(m_timer, SIGNAL(timeout()), SIGNAL(databaseModified())); + connect(&m_modifiedTimer, SIGNAL(timeout()), SIGNAL(databaseModified())); connect(this, SIGNAL(databaseOpened()), SLOT(updateCommonUsernames())); connect(this, SIGNAL(databaseSaved()), SLOT(updateCommonUsernames())); connect(m_fileWatcher, SIGNAL(fileChanged()), SIGNAL(databaseFileChanged())); @@ -229,6 +228,7 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool auto& canonicalFilePath = QFileInfo::exists(filePath) ? QFileInfo(filePath).canonicalFilePath() : filePath; bool ok = performSave(canonicalFilePath, error, atomic, backup); if (ok) { + markAsClean(); setFilePath(filePath); m_fileWatcher->start(canonicalFilePath, 30, 1); } else { @@ -343,7 +343,6 @@ bool Database::writeDatabase(QIODevice* device, QString* error) return false; } - markAsClean(); return true; } @@ -832,7 +831,7 @@ void Database::emptyRecycleBin() void Database::setEmitModified(bool value) { if (m_emitModified && !value) { - m_timer->stop(); + m_modifiedTimer.stop(); } m_emitModified = value; @@ -846,8 +845,9 @@ bool Database::isModified() const void Database::markAsModified() { m_modified = true; - if (m_emitModified) { - startModifiedTimer(); + if (m_emitModified && !m_modifiedTimer.isActive()) { + // Small time delay prevents numerous consecutive saves due to repeated signals + m_modifiedTimer.start(150); } } @@ -855,6 +855,7 @@ void Database::markAsClean() { bool emitSignal = m_modified; m_modified = false; + m_modifiedTimer.stop(); if (emitSignal) { emit databaseSaved(); } @@ -869,18 +870,6 @@ Database* Database::databaseByUuid(const QUuid& uuid) return s_uuidMap.value(uuid, nullptr); } -void Database::startModifiedTimer() -{ - if (!m_emitModified) { - return; - } - - if (m_timer->isActive()) { - m_timer->stop(); - } - m_timer->start(150); -} - QSharedPointer Database::key() const { return m_data.key; diff --git a/src/core/Database.h b/src/core/Database.h index d5f2092b2..d3d88e7d2 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -21,9 +21,9 @@ #include #include -#include #include #include +#include #include "config-keepassx.h" #include "crypto/kdf/AesKdf.h" @@ -37,7 +37,6 @@ enum class EntryReferenceType; class FileWatcher; class Group; class Metadata; -class QTimer; class QIODevice; struct DeletedObject @@ -155,9 +154,6 @@ signals: void databaseDiscarded(); void databaseFileChanged(); -private slots: - void startModifiedTimer(); - private: struct DatabaseData { @@ -211,7 +207,7 @@ private: DatabaseData m_data; QPointer m_rootGroup; QList m_deletedObjects; - QPointer m_timer; + QTimer m_modifiedTimer; QPointer m_fileWatcher; bool m_initialized = false; bool m_modified = false; diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 29942571c..04fad9a00 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1370,7 +1370,7 @@ bool DatabaseWidget::lock() if (m_db->isModified()) { bool saved = false; // Attempt to save on exit, but don't block locking if it fails - if (config()->get("AutoSaveOnExit").toBool()) { + if (config()->get("AutoSaveOnExit").toBool() || config()->get("AutoSaveAfterEveryChange").toBool()) { saved = save(); } From 8e76c30dd104ce8044ecf0c53176f0f716e55f29 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Thu, 12 Dec 2019 22:40:17 -0500 Subject: [PATCH 14/14] Prevent reloading database while editing an entry or group * Fix #3933 and fix #3857. Interaction with entries and groups is disabled while the database is being reloaded or saved to prevent changes from occurring. Prevent the database from being reloading if an entry or group is currently being edited. * Fix #3941 - Only notify components when the database file actually changes (determined by checksum). This prevents spurious merge requests when the file is merely touched by another service (e.g., DropBox). * Fix code format of ElidedLabel.cpp --- src/core/FileWatcher.cpp | 30 +++++++++++++----------------- src/core/FileWatcher.h | 7 +++---- src/gui/DatabaseWidget.cpp | 30 ++++++++++++++++++++++++++++-- src/gui/DatabaseWidget.h | 1 + src/gui/widgets/ElidedLabel.cpp | 2 +- 5 files changed, 46 insertions(+), 24 deletions(-) diff --git a/src/core/FileWatcher.cpp b/src/core/FileWatcher.cpp index 0d1def31a..fb8e95128 100644 --- a/src/core/FileWatcher.cpp +++ b/src/core/FileWatcher.cpp @@ -35,11 +35,10 @@ namespace FileWatcher::FileWatcher(QObject* parent) : QObject(parent) - , m_ignoreFileChange(false) { - connect(&m_fileWatcher, SIGNAL(fileChanged(QString)), SLOT(onWatchedFileChanged())); + connect(&m_fileWatcher, SIGNAL(fileChanged(QString)), SLOT(checkFileChanged())); + connect(&m_fileChecksumTimer, SIGNAL(timeout()), SLOT(checkFileChanged())); connect(&m_fileChangeDelayTimer, SIGNAL(timeout()), SIGNAL(fileChanged())); - connect(&m_fileChecksumTimer, SIGNAL(timeout()), SLOT(checkFileChecksum())); m_fileChangeDelayTimer.setSingleShot(true); m_fileIgnoreDelayTimer.setSingleShot(true); } @@ -101,17 +100,6 @@ void FileWatcher::resume() } } -void FileWatcher::onWatchedFileChanged() -{ - // Don't notify if we are ignoring events or already started a notification chain - if (shouldIgnoreChanges()) { - return; - } - - m_fileChecksum = calculateChecksum(); - m_fileChangeDelayTimer.start(0); -} - bool FileWatcher::shouldIgnoreChanges() { return m_filePath.isEmpty() || m_ignoreFileChange || m_fileIgnoreDelayTimer.isActive() @@ -123,15 +111,23 @@ bool FileWatcher::hasSameFileChecksum() return calculateChecksum() == m_fileChecksum; } -void FileWatcher::checkFileChecksum() +void FileWatcher::checkFileChanged() { if (shouldIgnoreChanges()) { return; } - if (!hasSameFileChecksum()) { - onWatchedFileChanged(); + // Prevent reentrance + m_ignoreFileChange = true; + + // Only trigger the change notice if there is a checksum mismatch + auto checksum = calculateChecksum(); + if (checksum != m_fileChecksum) { + m_fileChecksum = checksum; + m_fileChangeDelayTimer.start(0); } + + m_ignoreFileChange = false; } QByteArray FileWatcher::calculateChecksum() diff --git a/src/core/FileWatcher.h b/src/core/FileWatcher.h index fea05fc84..9b55badc1 100644 --- a/src/core/FileWatcher.h +++ b/src/core/FileWatcher.h @@ -43,8 +43,7 @@ public slots: void resume(); private slots: - void onWatchedFileChanged(); - void checkFileChecksum(); + void checkFileChanged(); private: QByteArray calculateChecksum(); @@ -56,8 +55,8 @@ private: QTimer m_fileChangeDelayTimer; QTimer m_fileIgnoreDelayTimer; QTimer m_fileChecksumTimer; - int m_fileChecksumSizeBytes; - bool m_ignoreFileChange; + int m_fileChecksumSizeBytes = -1; + bool m_ignoreFileChange = false; }; class BulkFileWatcher : public QObject diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 04fad9a00..5c4bf39ac 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -275,6 +275,11 @@ bool DatabaseWidget::isEntryEditActive() const return currentWidget() == m_editEntryWidget; } +bool DatabaseWidget::isGroupEditActive() const +{ + return currentWidget() == m_editGroupWidget; +} + bool DatabaseWidget::isEditWidgetModified() const { if (currentWidget() == m_editEntryWidget) { @@ -387,6 +392,8 @@ void DatabaseWidget::createEntry() void DatabaseWidget::replaceDatabase(QSharedPointer db) { + Q_ASSERT(!isEntryEditActive() && !isGroupEditActive()); + // Save off new parent UUID which will be valid when creating a new entry QUuid newParentUuid; if (m_newParent) { @@ -1421,7 +1428,8 @@ bool DatabaseWidget::lock() void DatabaseWidget::reloadDatabaseFile() { - if (!m_db || isLocked()) { + // Ignore reload if we are locked or currently editing an entry or group + if (!m_db || isLocked() || isEntryEditActive() || isGroupEditActive()) { return; } @@ -1441,6 +1449,11 @@ void DatabaseWidget::reloadDatabaseFile() } } + // Lock out interactions + m_entryView->setDisabled(true); + m_groupView->setDisabled(true); + QApplication::processEvents(); + QString error; auto db = QSharedPointer::create(m_db->filePath()); if (db->open(database()->key(), &error)) { @@ -1480,6 +1493,10 @@ void DatabaseWidget::reloadDatabaseFile() // Mark db as modified since existing data may differ from file or file was deleted m_db->markAsModified(); } + + // Return control + m_entryView->setDisabled(false); + m_groupView->setDisabled(false); } int DatabaseWidget::numberOfSelectedEntries() const @@ -1620,11 +1637,20 @@ bool DatabaseWidget::save() m_blockAutoSave = true; ++m_saveAttempts; - // TODO: Make this async, but lock out the database widget to prevent re-entrance + // TODO: Make this async + // Lock out interactions + m_entryView->setDisabled(true); + m_groupView->setDisabled(true); + QApplication::processEvents(); + bool useAtomicSaves = config()->get("UseAtomicSaves", true).toBool(); QString errorMessage; bool ok = m_db->save(&errorMessage, useAtomicSaves, config()->get("BackupBeforeSave").toBool()); + // Return control + m_entryView->setDisabled(false); + m_groupView->setDisabled(false); + if (ok) { m_saveAttempts = 0; m_blockAutoSave = false; diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 6f40c65c5..9f0c5c976 100644 --- a/src/gui/DatabaseWidget.h +++ b/src/gui/DatabaseWidget.h @@ -81,6 +81,7 @@ public: bool isLocked() const; bool isSearchActive() const; bool isEntryEditActive() const; + bool isGroupEditActive() const; QString getCurrentSearch(); void refreshSearch(); diff --git a/src/gui/widgets/ElidedLabel.cpp b/src/gui/widgets/ElidedLabel.cpp index 187c2fc43..749f075c8 100644 --- a/src/gui/widgets/ElidedLabel.cpp +++ b/src/gui/widgets/ElidedLabel.cpp @@ -105,7 +105,7 @@ void ElidedLabel::updateElidedText() const QFontMetrics metrix(font()); displayText = metrix.elidedText(m_rawText, m_elideMode, width() - 2); } - + bool hasUrl = !m_url.isEmpty(); setText(hasUrl ? htmlLinkTemplate.arg(m_url.toHtmlEscaped(), displayText) : displayText); setOpenExternalLinks(!hasUrl);