From e391dd182deb9c80ac61800d4c0936c67adb2b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20V=C3=A4nttinen?= Date: Sun, 13 Sep 2020 17:38:19 +0300 Subject: [PATCH] Fix Best-Matching ..again (#5316) Co-authored-by: Jonathan White --- src/browser/BrowserAction.cpp | 8 +- src/browser/BrowserService.cpp | 281 ++++++++++++++++----------------- src/browser/BrowserService.h | 36 ++--- tests/TestBrowser.cpp | 259 +++++++++++++++++++----------- tests/TestBrowser.h | 2 + 5 files changed, 329 insertions(+), 257 deletions(-) diff --git a/src/browser/BrowserAction.cpp b/src/browser/BrowserAction.cpp index 8e0c26909..65fe4cb7b 100644 --- a/src/browser/BrowserAction.cpp +++ b/src/browser/BrowserAction.cpp @@ -267,8 +267,8 @@ QJsonObject BrowserAction::handleGetLogins(const QJsonObject& json, const QStrin return getErrorReply(action, ERROR_KEEPASS_CANNOT_DECRYPT_MESSAGE); } - const QString url = decrypted.value("url").toString(); - if (url.isEmpty()) { + const QString siteUrl = decrypted.value("url").toString(); + if (siteUrl.isEmpty()) { return getErrorReply(action, ERROR_KEEPASS_NO_URL_PROVIDED); } @@ -281,10 +281,10 @@ QJsonObject BrowserAction::handleGetLogins(const QJsonObject& json, const QStrin } const QString id = decrypted.value("id").toString(); - const QString submit = decrypted.value("submitUrl").toString(); + const QString formUrl = decrypted.value("submitUrl").toString(); const QString auth = decrypted.value("httpAuth").toString(); const bool httpAuth = auth.compare(TRUE_STR, Qt::CaseSensitive) == 0 ? true : false; - const QJsonArray users = browserService()->findMatchingEntries(id, url, submit, "", keyList, httpAuth); + const QJsonArray users = browserService()->findMatchingEntries(id, siteUrl, formUrl, "", keyList, httpAuth); if (users.isEmpty()) { return getErrorReply(action, ERROR_KEEPASS_NO_LOGINS_FOUND); diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index eb752996c..08c9f88da 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -371,8 +371,8 @@ QString BrowserService::getKey(const QString& id) } QJsonArray BrowserService::findMatchingEntries(const QString& dbid, - const QString& url, - const QString& submitUrl, + const QString& siteUrlStr, + const QString& formUrlStr, const QString& realm, const StringPairList& keyList, const bool httpAuth) @@ -380,13 +380,13 @@ QJsonArray BrowserService::findMatchingEntries(const QString& dbid, Q_UNUSED(dbid); const bool alwaysAllowAccess = browserSettings()->alwaysAllowAccess(); const bool ignoreHttpAuth = browserSettings()->httpAuthPermission(); - const QString host = QUrl(url).host(); - const QString submitHost = QUrl(submitUrl).host(); + const QString siteHost = QUrl(siteUrlStr).host(); + const QString formHost = QUrl(formUrlStr).host(); // Check entries for authorization QList pwEntriesToConfirm; QList pwEntries; - for (auto* entry : searchEntries(url, submitUrl, keyList)) { + for (auto* entry : searchEntries(siteUrlStr, formUrlStr, keyList)) { if (entry->customData()->contains(BrowserService::OPTION_HIDE_ENTRY) && entry->customData()->value(BrowserService::OPTION_HIDE_ENTRY) == TRUE_STR) { continue; @@ -403,7 +403,7 @@ QJsonArray BrowserService::findMatchingEntries(const QString& dbid, continue; } - switch (checkAccess(entry, host, submitHost, realm)) { + switch (checkAccess(entry, siteHost, formHost, realm)) { case Denied: continue; @@ -422,7 +422,8 @@ QJsonArray BrowserService::findMatchingEntries(const QString& dbid, } // Confirm entries - QList selectedEntriesToConfirm = confirmEntries(pwEntriesToConfirm, url, host, submitHost, realm, httpAuth); + QList selectedEntriesToConfirm = + confirmEntries(pwEntriesToConfirm, siteUrlStr, siteHost, formHost, realm, httpAuth); if (!selectedEntriesToConfirm.isEmpty()) { pwEntries.append(selectedEntriesToConfirm); } @@ -437,7 +438,7 @@ QJsonArray BrowserService::findMatchingEntries(const QString& dbid, } // Sort results - pwEntries = sortEntries(pwEntries, host, submitUrl, url); + pwEntries = sortEntries(pwEntries, siteUrlStr, formUrlStr); // Fill the list QJsonArray result; @@ -451,8 +452,8 @@ QJsonArray BrowserService::findMatchingEntries(const QString& dbid, void BrowserService::addEntry(const QString& dbid, const QString& login, const QString& password, - const QString& url, - const QString& submitUrl, + const QString& siteUrlStr, + const QString& formUrlStr, const QString& realm, const QString& group, const QString& groupUuid, @@ -467,8 +468,8 @@ void BrowserService::addEntry(const QString& dbid, auto* entry = new Entry(); entry->setUuid(QUuid::createUuid()); - entry->setTitle(QUrl(url).host()); - entry->setUrl(url); + entry->setTitle(QUrl(siteUrlStr).host()); + entry->setUrl(siteUrlStr); entry->setIcon(KEEPASSXCBROWSER_DEFAULT_ICON); entry->setUsername(login); entry->setPassword(password); @@ -487,8 +488,8 @@ void BrowserService::addEntry(const QString& dbid, entry->setGroup(getDefaultEntryGroup(db)); } - const QString host = QUrl(url).host(); - const QString submitHost = QUrl(submitUrl).host(); + const QString host = QUrl(siteUrlStr).host(); + const QString submitHost = QUrl(formUrlStr).host(); BrowserEntryConfig config; config.allow(host); @@ -505,8 +506,8 @@ bool BrowserService::updateEntry(const QString& dbid, const QString& uuid, const QString& login, const QString& password, - const QString& url, - const QString& submitUrl) + const QString& siteUrlStr, + const QString& formUrlStr) { // TODO: select database based on this key id Q_UNUSED(dbid); @@ -518,7 +519,7 @@ bool BrowserService::updateEntry(const QString& dbid, Entry* entry = db->rootGroup()->findEntryByUuid(Tools::hexToUuid(uuid)); if (!entry) { // If entry is not found for update, add a new one to the selected database - addEntry(dbid, login, password, url, submitUrl, "", "", "", db); + addEntry(dbid, login, password, siteUrlStr, formUrlStr, "", "", "", db); return true; } @@ -547,7 +548,7 @@ bool BrowserService::updateEntry(const QString& dbid, dialogResult = MessageBox::question( nullptr, tr("KeePassXC: Update Entry"), - tr("Do you want to update the information in %1 - %2?").arg(QUrl(url).host(), username), + tr("Do you want to update the information in %1 - %2?").arg(QUrl(siteUrlStr).host(), username), MessageBox::Save | MessageBox::Cancel, MessageBox::Cancel, MessageBox::Raise); @@ -570,7 +571,7 @@ bool BrowserService::updateEntry(const QString& dbid, } QList -BrowserService::searchEntries(const QSharedPointer& db, const QString& url, const QString& submitUrl) +BrowserService::searchEntries(const QSharedPointer& db, const QString& siteUrlStr, const QString& formUrlStr) { QList entries; auto* rootGroup = db->rootGroup(); @@ -590,25 +591,29 @@ BrowserService::searchEntries(const QSharedPointer& db, const QString& // Search for additional URL's starting with KP2A_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), siteUrlStr, formUrlStr) && !entries.contains(entry)) { entries.append(entry); continue; } } - if (!handleURL(entry->url(), url, submitUrl)) { + if (!handleURL(entry->url(), siteUrlStr, formUrlStr)) { continue; } - entries.append(entry); + // Additional URL check may have already inserted the entry to the list + if (!entries.contains(entry)) { + entries.append(entry); + } } } return entries; } -QList BrowserService::searchEntries(const QString& url, const QString& submitUrl, const StringPairList& keyList) +QList +BrowserService::searchEntries(const QString& siteUrlStr, const QString& formUrlStr, const StringPairList& keyList) { // Check if database is connected with KeePassXC-Browser auto databaseConnected = [&](const QSharedPointer& db) { @@ -638,11 +643,11 @@ QList BrowserService::searchEntries(const QString& url, const QString& s } // Search entries matching the hostname - QString hostname = QUrl(url).host(); + QString hostname = QUrl(siteUrlStr).host(); QList entries; do { for (const auto& db : databases) { - entries << searchEntries(db, url, submitUrl); + entries << searchEntries(db, siteUrlStr, formUrlStr); } } while (entries.isEmpty() && removeFirstDomain(hostname)); @@ -722,47 +727,30 @@ void BrowserService::convertAttributesToCustomData(QSharedPointer db) } } -QList BrowserService::sortEntries(QList& pwEntries, - const QString& host, - const QString& entryUrl, - const QString& fullUrl) +QList +BrowserService::sortEntries(QList& pwEntries, const QString& siteUrlStr, const QString& formUrlStr) { - QUrl url(entryUrl); - if (url.scheme().isEmpty()) { - url.setScheme("https"); - } - - const QString submitUrl = url.toString(QUrl::StripTrailingSlash); - const QString baseSubmitUrl = - url.toString(QUrl::StripTrailingSlash | QUrl::RemovePath | QUrl::RemoveQuery | QUrl::RemoveFragment); - // Build map of prioritized entries QMultiMap priorities; for (auto* entry : pwEntries) { - priorities.insert(sortPriority(entry, host, submitUrl, baseSubmitUrl, fullUrl), entry); + priorities.insert(sortPriority(getEntryURLs(entry), siteUrlStr, formUrlStr), entry); } + auto keys = priorities.uniqueKeys(); + std::sort(keys.begin(), keys.end(), [](int l, int r) { return l > r; }); + QList results; - QString field = browserSettings()->sortByTitle() ? "Title" : "UserName"; - for (int i = 100; i >= 0; i -= 5) { - if (priorities.count(i) > 0) { - // Sort same priority entries by Title or UserName - auto entries = priorities.values(i); - std::sort(entries.begin(), entries.end(), [&field](Entry* left, Entry* right) { - return (QString::localeAwareCompare(left->attributes()->value(field), right->attributes()->value(field)) - < 0) - || ((QString::localeAwareCompare(left->attributes()->value(field), - right->attributes()->value(field)) - == 0) - && (QString::localeAwareCompare(left->attributes()->value("UserName"), - right->attributes()->value("UserName")) - < 0)); - }); - results << entries; - if (browserSettings()->bestMatchOnly() && !pwEntries.isEmpty()) { - // Early out once we find the highest batch of matches - break; - } + auto sortField = browserSettings()->sortByTitle() ? EntryAttributes::TitleKey : EntryAttributes::UserNameKey; + for (auto key : keys) { + // Sort same priority entries by Title or UserName + auto entries = priorities.values(key); + std::sort(entries.begin(), entries.end(), [&sortField](Entry* left, Entry* right) { + return QString::localeAwareCompare(left->attribute(sortField), right->attribute(sortField)); + }); + results << entries; + if (browserSettings()->bestMatchOnly() && !results.isEmpty()) { + // Early out once we find the highest batch of matches + break; } } @@ -770,9 +758,9 @@ QList BrowserService::sortEntries(QList& pwEntries, } QList BrowserService::confirmEntries(QList& pwEntriesToConfirm, - const QString& url, - const QString& host, - const QString& submitHost, + const QString& siteUrlStr, + const QString& siteHost, + const QString& formUrlStr, const QString& realm, const bool httpAuth) { @@ -790,9 +778,9 @@ QList BrowserService::confirmEntries(QList& pwEntriesToConfirm, auto entry = pwEntriesToConfirm[item->row()]; BrowserEntryConfig config; config.load(entry); - config.deny(host); - if (!submitHost.isEmpty() && host != submitHost) { - config.deny(submitHost); + config.deny(siteHost); + if (!formUrlStr.isEmpty() && siteHost != formUrlStr) { + config.deny(formUrlStr); } if (!realm.isEmpty()) { config.setRealm(realm); @@ -800,7 +788,7 @@ QList BrowserService::confirmEntries(QList& pwEntriesToConfirm, config.save(entry); }); - accessControlDialog.setItems(pwEntriesToConfirm, url, httpAuth); + accessControlDialog.setItems(pwEntriesToConfirm, siteUrlStr, httpAuth); QList allowedEntries; if (accessControlDialog.exec() == QDialog::Accepted) { @@ -810,9 +798,9 @@ QList BrowserService::confirmEntries(QList& pwEntriesToConfirm, if (accessControlDialog.remember()) { BrowserEntryConfig config; config.load(entry); - config.allow(host); - if (!submitHost.isEmpty() && host != submitHost) { - config.allow(submitHost); + config.allow(siteHost); + if (!formUrlStr.isEmpty() && siteHost != formUrlStr) { + config.allow(formUrlStr); } if (!realm.isEmpty()) { config.setRealm(realm); @@ -871,7 +859,7 @@ QJsonObject BrowserService::prepareEntry(const Entry* entry) } BrowserService::Access -BrowserService::checkAccess(const Entry* entry, const QString& host, const QString& submitHost, const QString& realm) +BrowserService::checkAccess(const Entry* entry, const QString& siteHost, const QString& formHost, const QString& realm) { if (entry->isExpired()) { return browserSettings()->allowExpiredCredentials() ? Allowed : Denied; @@ -881,10 +869,10 @@ BrowserService::checkAccess(const Entry* entry, const QString& host, const QStri if (!config.load(entry)) { return Unknown; } - if ((config.isAllowed(host)) && (submitHost.isEmpty() || config.isAllowed(submitHost))) { + if ((config.isAllowed(siteHost)) && (formHost.isEmpty() || config.isAllowed(formHost))) { return Allowed; } - if ((config.isDenied(host)) || (!submitHost.isEmpty() && config.isDenied(submitHost))) { + if ((config.isDenied(siteHost)) || (!formHost.isEmpty() && config.isDenied(formHost))) { return Denied; } if (!realm.isEmpty() && config.realm() != realm) { @@ -919,66 +907,72 @@ Group* BrowserService::getDefaultEntryGroup(const QSharedPointer& sele return group; } -int BrowserService::sortPriority(const Entry* entry, - const QString& host, - const QString& submitUrl, - const QString& baseSubmitUrl, - const QString& fullUrl) const +// Returns the maximum sort priority given a set of match urls and the +// extension provided site and form url. +int BrowserService::sortPriority(const QStringList& urls, const QString& siteUrlStr, const QString& formUrlStr) { - QUrl url(entry->url()); - if (url.scheme().isEmpty()) { - url.setScheme("https"); - } + QList priorityList; + // NOTE: QUrl::matches is utterly broken in Qt < 5.11, so we work around that + // by removing parts of the url that we don't match and direct matching others + const auto stdOpts = QUrl::RemoveFragment | QUrl::RemoveUserInfo; + const auto siteUrl = QUrl(siteUrlStr).adjusted(stdOpts); + const auto formUrl = QUrl(formUrlStr).adjusted(stdOpts); - // Add the empty path to the URL if it's missing - if (url.path().isEmpty() && !url.hasFragment() && !url.hasQuery()) { - url.setPath("/"); - } + auto getPriority = [&](const QString& givenUrl) { + auto url = QUrl::fromUserInput(givenUrl).adjusted(stdOpts); - const QString entryURL = url.toString(QUrl::StripTrailingSlash); - const QString baseEntryURL = - url.toString(QUrl::StripTrailingSlash | QUrl::RemovePath | QUrl::RemoveQuery | QUrl::RemoveFragment); + // Default to https scheme if undefined + if (url.scheme().isEmpty() || !givenUrl.contains("://")) { + url.setScheme("https"); + } - if (!url.host().contains(".") && url.host() != "localhost") { + // Add the empty path to the URL if it's missing. + // URL's from the extension always have a path set, entry URL's can be without. + if (url.path().isEmpty() && !url.hasFragment() && !url.hasQuery()) { + url.setPath("/"); + } + + // Reject invalid urls and hosts, except 'localhost', and scheme mismatch + if (!url.isValid() || (!url.host().contains(".") && url.host() != "localhost") + || url.scheme() != siteUrl.scheme()) { + return 0; + } + + // Exact match with site url or form url + if (url.matches(siteUrl, QUrl::None) || url.matches(formUrl, QUrl::None)) { + return 100; + } + + // Exact match without the query string + if (url.matches(siteUrl, QUrl::RemoveQuery) || url.matches(formUrl, QUrl::RemoveQuery)) { + return 90; + } + + // Match without path (ie, FQDN match), form url prioritizes lower than site url + if (url.host() == siteUrl.host()) { + return 80; + } + if (url.host() == formUrl.host()) { + return 70; + } + + // Site/form url ends with given url (subdomain mismatch) + if (siteUrl.host().endsWith(url.host())) { + return 60; + } + if (formUrl.host().endsWith(url.host())) { + return 50; + } + + // No valid match found return 0; + }; + + for (const auto& entryUrl : urls) { + priorityList << getPriority(entryUrl); } - if (fullUrl == entryURL) { - return 100; - } - if (submitUrl == entryURL) { - return 95; - } - if (submitUrl.startsWith(entryURL) && entryURL != host && baseSubmitUrl != entryURL) { - return 90; - } - if (submitUrl.startsWith(baseEntryURL) && entryURL != host && baseSubmitUrl != baseEntryURL) { - return 80; - } - if (entryURL == host) { - return 70; - } - if (entryURL == baseSubmitUrl) { - return 60; - } - if (entryURL.startsWith(submitUrl)) { - return 50; - } - if (entryURL.startsWith(baseSubmitUrl) && baseSubmitUrl != host) { - return 40; - } - if (submitUrl.startsWith(entryURL)) { - return 30; - } - if (submitUrl.startsWith(baseEntryURL)) { - return 20; - } - if (entryURL.startsWith(host)) { - return 10; - } - if (host.startsWith(entryURL)) { - return 5; - } - return 0; + + return *std::max_element(priorityList.begin(), priorityList.end()); } bool BrowserService::schemeFound(const QString& url) @@ -1004,7 +998,7 @@ bool BrowserService::removeFirstDomain(QString& hostname) return false; } -bool BrowserService::handleURL(const QString& entryUrl, const QString& url, const QString& submitUrl) +bool BrowserService::handleURL(const QString& entryUrl, const QString& siteUrlStr, const QString& formUrlStr) { if (entryUrl.isEmpty()) { return false; @@ -1022,8 +1016,8 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& url, cons } // Make a direct compare if a local file is used - if (url.contains("file://")) { - return entryUrl == submitUrl; + if (siteUrlStr.contains("file://")) { + return entryUrl == formUrlStr; } // URL host validation fails @@ -1032,7 +1026,7 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& url, cons } // Match port, if used - QUrl siteQUrl(url); + QUrl siteQUrl(siteUrlStr); if (entryQUrl.port() > 0 && entryQUrl.port() != siteQUrl.port()) { return false; } @@ -1056,17 +1050,7 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& url, cons // Match the subdomains with the limited wildcard if (siteQUrl.host().endsWith(entryQUrl.host())) { - if (!browserSettings()->bestMatchOnly()) { - return true; - } - - // Match the exact subdomain and path, or start of the path when entry's path is longer than plain "/" - if (siteQUrl.host() == entryQUrl.host()) { - if (siteQUrl.path() == entryQUrl.path() - || (entryQUrl.path().size() > 1 && siteQUrl.path().startsWith(entryQUrl.path()))) { - return true; - } - } + return true; } return false; @@ -1212,6 +1196,21 @@ bool BrowserService::checkLegacySettings(QSharedPointer db) return dialogResult == MessageBox::Yes; } +QStringList BrowserService::getEntryURLs(const Entry* entry) +{ + QStringList urlList; + urlList << entry->url(); + + // Handle additional URL's + for (const auto& key : entry->attributes()->keys()) { + if (key.startsWith(ADDITIONAL_URL)) { + urlList << entry->attributes()->value(key); + } + } + + return urlList; +} + void BrowserService::hideWindow() const { if (m_prevWindowState == WindowState::Minimized) { diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index f52b502b3..f84bf2880 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -63,8 +63,8 @@ public: void addEntry(const QString& dbid, const QString& login, const QString& password, - const QString& url, - const QString& submitUrl, + const QString& siteUrlStr, + const QString& formUrlStr, const QString& realm, const QString& group, const QString& groupUuid, @@ -73,12 +73,12 @@ public: const QString& uuid, const QString& login, const QString& password, - const QString& url, - const QString& submitUrl); + const QString& siteUrlStr, + const QString& formUrlStr); QJsonArray findMatchingEntries(const QString& dbid, - const QString& url, - const QString& submitUrl, + const QString& siteUrlStr, + const QString& formUrlStr, const QString& realm, const StringPairList& keyList, const bool httpAuth = false); @@ -118,35 +118,31 @@ private: Hidden }; - QList searchEntries(const QSharedPointer& db, const QString& url, const QString& submitUrl); - QList searchEntries(const QString& url, const QString& submitUrl, const StringPairList& keyList); QList - sortEntries(QList& pwEntries, const QString& host, const QString& submitUrl, const QString& fullUrl); + searchEntries(const QSharedPointer& db, const QString& siteUrlStr, const QString& formUrlStr); + QList searchEntries(const QString& siteUrlStr, const QString& formUrlStr, const StringPairList& keyList); + QList sortEntries(QList& pwEntries, const QString& siteUrlStr, const QString& formUrlStr); QList confirmEntries(QList& pwEntriesToConfirm, - const QString& url, - const QString& host, - const QString& submitUrl, + const QString& siteUrlStr, + const QString& siteHost, + const QString& formUrlStr, const QString& realm, const bool httpAuth); QJsonObject prepareEntry(const Entry* entry); QJsonArray getChildrenFromGroup(Group* group); - Access checkAccess(const Entry* entry, const QString& host, const QString& submitHost, const QString& realm); + Access checkAccess(const Entry* entry, const QString& siteHost, const QString& formHost, const QString& realm); Group* getDefaultEntryGroup(const QSharedPointer& selectedDb = {}); - int sortPriority(const Entry* entry, - const QString& host, - const QString& submitUrl, - const QString& baseSubmitUrl, - const QString& fullUrl) const; + int sortPriority(const QStringList& urls, const QString& siteUrlStr, const QString& formUrlStr); bool schemeFound(const QString& url); bool removeFirstDomain(QString& hostname); - bool handleURL(const QString& entryUrl, const QString& url, const QString& submitUrl); + bool handleURL(const QString& entryUrl, const QString& siteUrlStr, const QString& formUrlStr); QString baseDomain(const QString& hostname) const; QSharedPointer getDatabase(); QSharedPointer selectedDatabase(); QString getDatabaseRootUuid(); QString getDatabaseRecycleBinUuid(); - bool checkLegacySettings(QSharedPointer db); + QStringList getEntryURLs(const Entry* entry); void hideWindow() const; void raiseWindow(const bool force = false); diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 95189fec7..c3a90e37c 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -128,59 +128,52 @@ void TestBrowser::testBaseDomain() void TestBrowser::testSortPriority() { - QString host = "github.com"; - QString submitUrl = "https://github.com/session"; - QString baseSubmitUrl = "https://github.com"; - QString fullUrl = "https://github.com/login"; + QFETCH(QString, entryUrl); + QFETCH(QString, siteUrl); + QFETCH(QString, formUrl); + QFETCH(int, expectedScore); - QScopedPointer entry1(new Entry()); - QScopedPointer entry2(new Entry()); - QScopedPointer entry3(new Entry()); - QScopedPointer entry4(new Entry()); - QScopedPointer entry5(new Entry()); - QScopedPointer entry6(new Entry()); - QScopedPointer entry7(new Entry()); - QScopedPointer entry8(new Entry()); - QScopedPointer entry9(new Entry()); - QScopedPointer entry10(new Entry()); - QScopedPointer entry11(new Entry()); + QScopedPointer entry(new Entry()); + entry->setUrl(entryUrl); - entry1->setUrl("https://github.com/login"); - entry2->setUrl("https://github.com/login"); - entry3->setUrl("https://github.com/"); - entry4->setUrl("github.com/login"); - entry5->setUrl("http://github.com"); - entry6->setUrl("http://github.com/login"); - entry7->setUrl("github.com"); - entry8->setUrl("github.com/login"); - entry9->setUrl("https://github"); // Invalid URL - entry10->setUrl("github.com"); - entry11->setUrl("https://github.com/login"); // Exact match + QCOMPARE(m_browserService->sortPriority(m_browserService->getEntryURLs(entry.data()), siteUrl, formUrl), + expectedScore); +} - // The extension uses the submitUrl as default for comparison - auto res1 = m_browserService->sortPriority(entry1.data(), host, "https://github.com/login", baseSubmitUrl, fullUrl); - auto res2 = m_browserService->sortPriority(entry2.data(), host, submitUrl, baseSubmitUrl, baseSubmitUrl); - auto res3 = m_browserService->sortPriority(entry3.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res4 = m_browserService->sortPriority(entry4.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res5 = m_browserService->sortPriority(entry5.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res6 = m_browserService->sortPriority(entry6.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res7 = m_browserService->sortPriority(entry7.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res8 = m_browserService->sortPriority(entry8.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res9 = m_browserService->sortPriority(entry9.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res10 = m_browserService->sortPriority(entry10.data(), host, submitUrl, baseSubmitUrl, fullUrl); - auto res11 = m_browserService->sortPriority(entry11.data(), host, submitUrl, baseSubmitUrl, fullUrl); +void TestBrowser::testSortPriority_data() +{ + const QString siteUrl = "https://github.com/login"; + const QString formUrl = "https://github.com/session"; - QCOMPARE(res1, 100); - QCOMPARE(res2, 40); - QCOMPARE(res3, 90); - QCOMPARE(res4, 0); - QCOMPARE(res5, 0); - QCOMPARE(res6, 0); - QCOMPARE(res7, 0); - QCOMPARE(res8, 0); - QCOMPARE(res9, 0); - QCOMPARE(res10, 0); - QCOMPARE(res11, 100); + QTest::addColumn("entryUrl"); + QTest::addColumn("siteUrl"); + QTest::addColumn("formUrl"); + QTest::addColumn("expectedScore"); + + QTest::newRow("Exact Match") << siteUrl << siteUrl << siteUrl << 100; + QTest::newRow("Exact Match (site)") << siteUrl << siteUrl << formUrl << 100; + QTest::newRow("Exact Match (form)") << siteUrl << "https://github.net" << siteUrl << 100; + QTest::newRow("Exact Match No Trailing Slash") << "https://github.com" + << "https://github.com/" << formUrl << 100; + QTest::newRow("Exact Match No Scheme") << "github.com/login" << siteUrl << formUrl << 100; + QTest::newRow("Exact Match with Query") << "https://github.com/login?test=test#fragment" + << "https://github.com/login?test=test" << formUrl << 100; + + QTest::newRow("Site Query Mismatch") << siteUrl << siteUrl + "?test=test" << formUrl << 90; + + QTest::newRow("Path Mismatch (site)") << "https://github.com/" << siteUrl << formUrl << 80; + QTest::newRow("Path Mismatch (site) No Scheme") << "github.com" << siteUrl << formUrl << 80; + QTest::newRow("Path Mismatch (form)") << "https://github.com/" + << "https://github.net" << formUrl << 70; + + QTest::newRow("Subdomain Mismatch (site)") << siteUrl << "https://sub.github.com/" + << "https://github.net/" << 60; + QTest::newRow("Subdomain Mismatch (form)") << siteUrl << "https://github.net/" + << "https://sub.github.com/" << 50; + + QTest::newRow("Scheme Mismatch") << "http://github.com" << siteUrl << formUrl << 0; + QTest::newRow("Scheme Mismatch w/path") << "http://github.com/login" << siteUrl << formUrl << 0; + QTest::newRow("Invalid URL") << "http://github" << siteUrl << formUrl << 0; } void TestBrowser::testSearchEntries() @@ -344,14 +337,14 @@ void TestBrowser::testSubdomainsAndPaths() createEntries(entryURLs, root); - result = m_browserService->searchEntries(db, "https://accounts.example.com", "https://accounts.example.com"); + 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"); + 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 @@ -381,33 +374,32 @@ void TestBrowser::testSortEntries() "http://github.com", "http://github.com/login", "github.com", - "github.com/login", + "github.com/login?test=test", "https://github", // Invalid URL "github.com"}; auto entries = createEntries(urls, root); browserSettings()->setBestMatchOnly(false); - auto result = m_browserService->sortEntries( - entries, "github.com", "https://github.com/session", "https://github.com"); // entries, host, submitUrl + browserSettings()->setSortByUsername(true); + auto result = m_browserService->sortEntries(entries, "https://github.com/login", "https://github.com/session"); QCOMPARE(result.size(), 10); - QCOMPARE(result[0]->username(), QString("User 2")); - QCOMPARE(result[0]->url(), QString("https://github.com/")); - QCOMPARE(result[1]->username(), QString("User 0")); - QCOMPARE(result[1]->url(), QString("https://github.com/login_page")); - QCOMPARE(result[2]->username(), QString("User 1")); - QCOMPARE(result[2]->url(), QString("https://github.com/login")); - QCOMPARE(result[3]->username(), QString("User 3")); - QCOMPARE(result[3]->url(), QString("github.com/login")); + QCOMPARE(result[0]->username(), QString("User 1")); + QCOMPARE(result[0]->url(), urls[1]); + QCOMPARE(result[1]->username(), QString("User 3")); + QCOMPARE(result[1]->url(), urls[3]); + QCOMPARE(result[2]->username(), QString("User 7")); + QCOMPARE(result[2]->url(), urls[7]); + QCOMPARE(result[3]->username(), QString("User 0")); + QCOMPARE(result[3]->url(), urls[0]); // Test with a perfect match. That should be first in the list. - result = m_browserService->sortEntries( - entries, "github.com", "https://github.com/session", "https://github.com/login_page"); + result = m_browserService->sortEntries(entries, "https://github.com/login_page", "https://github.com/session"); QCOMPARE(result.size(), 10); QCOMPARE(result[0]->username(), QString("User 0")); QCOMPARE(result[0]->url(), QString("https://github.com/login_page")); - QCOMPARE(result[1]->username(), QString("User 2")); - QCOMPARE(result[1]->url(), QString("https://github.com/")); + QCOMPARE(result[1]->username(), QString("User 1")); + QCOMPARE(result[1]->url(), QString("https://github.com/login")); } QList TestBrowser::createEntries(QStringList& urls, Group* root) const @@ -458,45 +450,128 @@ void TestBrowser::testBestMatchingCredentials() browserSettings()->setBestMatchOnly(true); - auto result = m_browserService->searchEntries(db, "https://github.com/loginpage", "https://github.com/loginpage"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://github.com/loginpage")); + QString siteUrl = "https://github.com/loginpage"; + auto result = m_browserService->searchEntries(db, siteUrl, siteUrl); + auto sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), siteUrl); - result = m_browserService->searchEntries(db, "https://github.com/justsomepage", "https://github.com/justsomepage"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://github.com/justsomepage")); + siteUrl = "https://github.com/justsomepage"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), siteUrl); - result = m_browserService->searchEntries(db, "https://github.com/", "https://github.com/"); - m_browserService->sortEntries(entries, "github.com", "https://github.com/", "https://github.com/"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://github.com/")); + siteUrl = "https://github.com/"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(entries, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), siteUrl); + // Without best-matching the URL with the path should be returned first browserSettings()->setBestMatchOnly(false); - result = m_browserService->searchEntries(db, "https://github.com/loginpage", "https://github.com/loginpage"); - QCOMPARE(result.size(), 3); - QCOMPARE(result[0]->url(), QString("https://github.com/loginpage")); + siteUrl = "https://github.com/loginpage"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 3); + QCOMPARE(sorted[0]->url(), siteUrl); // Test with subdomains QStringList subdomainsUrls = {"https://sub.github.com/loginpage", "https://sub.github.com/justsomepage", - "https://bus.github.com/justsomepage"}; + "https://bus.github.com/justsomepage", + "https://subdomain.example.com/", + "https://subdomain.example.com", + "https://example.com"}; entries = createEntries(subdomainsUrls, root); browserSettings()->setBestMatchOnly(true); + siteUrl = "https://sub.github.com/justsomepage"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), siteUrl); - result = m_browserService->searchEntries( - db, "https://sub.github.com/justsomepage", "https://sub.github.com/justsomepage"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://sub.github.com/justsomepage")); + siteUrl = "https://github.com/justsomepage"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), siteUrl); - result = m_browserService->searchEntries(db, "https://github.com/justsomepage", "https://github.com/justsomepage"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://github.com/justsomepage")); + siteUrl = "https://sub.github.com/justsomepage?wehavesomeextra=here"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), QString("https://sub.github.com/justsomepage")); - result = m_browserService->searchEntries(db, - "https://sub.github.com/justsomepage?wehavesomeextra=here", - "https://sub.github.com/justsomepage?wehavesomeextra=here"); - QCOMPARE(result.size(), 1); - QCOMPARE(result[0]->url(), QString("https://sub.github.com/justsomepage")); + // The matching should not care if there's a / path or not. + siteUrl = "https://subdomain.example.com/"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + QCOMPARE(sorted.size(), 2); + QCOMPARE(sorted[0]->url(), QString("https://subdomain.example.com/")); + QCOMPARE(sorted[1]->url(), QString("https://subdomain.example.com")); + + // Entries with https://example.com should be still returned even if the site URL has a subdomain. Those have the + // best match. + db = QSharedPointer::create(); + root = db->rootGroup(); + QStringList domainUrls = {"https://example.com", "https://example.com", "https://other.example.com"}; + entries = createEntries(domainUrls, root); + siteUrl = "https://subdomain.example.com"; + result = m_browserService->searchEntries(db, siteUrl, siteUrl); + sorted = m_browserService->sortEntries(result, siteUrl, siteUrl); + + QCOMPARE(sorted.size(), 2); + QCOMPARE(sorted[0]->url(), QString("https://example.com")); + QCOMPARE(sorted[1]->url(), QString("https://example.com")); + + // https://github.com/keepassxreboot/keepassxc/issues/4754 + db = QSharedPointer::create(); + root = db->rootGroup(); + QStringList fooUrls = {"https://example.com/foo", "https://example.com/bar"}; + entries = createEntries(fooUrls, root); + + for (const auto& url : fooUrls) { + result = m_browserService->searchEntries(db, url, url); + sorted = m_browserService->sortEntries(result, url, url); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), QString(url)); + } + + // https://github.com/keepassxreboot/keepassxc/issues/4734 + db = QSharedPointer::create(); + root = db->rootGroup(); + QStringList testUrls = {"http://some.domain.tld/somePath", "http://some.domain.tld/otherPath"}; + entries = createEntries(testUrls, root); + + for (const auto& url : testUrls) { + result = m_browserService->searchEntries(db, url, url); + sorted = m_browserService->sortEntries(result, url, url); + QCOMPARE(sorted.size(), 1); + QCOMPARE(sorted[0]->url(), QString(url)); + } +} + +void TestBrowser::testBestMatchingWithAdditionalURLs() +{ + auto db = QSharedPointer::create(); + auto* root = db->rootGroup(); + + QStringList urls = {"https://github.com/loginpage", "https://test.github.com/", "https://github.com/"}; + + auto entries = createEntries(urls, root); + browserSettings()->setBestMatchOnly(true); + + // Add an additional URL to the first entry + entries.first()->attributes()->set(BrowserService::ADDITIONAL_URL, "https://test.github.com/anotherpage"); + + // The first entry should be triggered + auto result = m_browserService->searchEntries( + db, "https://test.github.com/anotherpage", "https://test.github.com/anotherpage"); + auto sorted = m_browserService->sortEntries( + result, "https://test.github.com/anotherpage", "https://test.github.com/anotherpage"); + QCOMPARE(sorted.length(), 1); + QCOMPARE(sorted[0]->url(), urls[0]); } diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index c8be3d6ca..d6140e886 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -40,6 +40,7 @@ private slots: void testBaseDomain(); void testSortPriority(); + void testSortPriority_data(); void testSearchEntries(); void testSearchEntriesWithPort(); void testSearchEntriesWithAdditionalURLs(); @@ -48,6 +49,7 @@ private slots: void testSortEntries(); void testValidURLs(); void testBestMatchingCredentials(); + void testBestMatchingWithAdditionalURLs(); private: QList createEntries(QStringList& urls, Group* root) const;