Fix Best-Matching ..again (#5316)

Co-authored-by: Jonathan White <support@dmapps.us>

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<Entry*> pwEntriesToConfirm;
     QList<Entry*> 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<Entry*> selectedEntriesToConfirm = confirmEntries(pwEntriesToConfirm, url, host, submitHost, realm, httpAuth);
+    QList<Entry*> 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<Entry*>
-BrowserService::searchEntries(const QSharedPointer<Database>& db, const QString& url, const QString& submitUrl)
+BrowserService::searchEntries(const QSharedPointer<Database>& db, const QString& siteUrlStr, const QString& formUrlStr)
 {
     QList<Entry*> entries;
     auto* rootGroup = db->rootGroup();
@@ -590,25 +591,29 @@ BrowserService::searchEntries(const QSharedPointer<Database>& 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<Entry*> BrowserService::searchEntries(const QString& url, const QString& submitUrl, const StringPairList& keyList)
+QList<Entry*>
+BrowserService::searchEntries(const QString& siteUrlStr, const QString& formUrlStr, const StringPairList& keyList)
 {
     // Check if database is connected with KeePassXC-Browser
     auto databaseConnected = [&](const QSharedPointer<Database>& db) {
@@ -638,11 +643,11 @@ QList<Entry*> BrowserService::searchEntries(const QString& url, const QString& s
     }
 
     // Search entries matching the hostname
-    QString hostname = QUrl(url).host();
+    QString hostname = QUrl(siteUrlStr).host();
     QList<Entry*> 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<Database> db)
     }
 }
 
-QList<Entry*> BrowserService::sortEntries(QList<Entry*>& pwEntries,
-                                          const QString& host,
-                                          const QString& entryUrl,
-                                          const QString& fullUrl)
+QList<Entry*>
+BrowserService::sortEntries(QList<Entry*>& 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<int, Entry*> 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<Entry*> 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<Entry*> BrowserService::sortEntries(QList<Entry*>& pwEntries,
 }
 
 QList<Entry*> BrowserService::confirmEntries(QList<Entry*>& 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<Entry*> BrowserService::confirmEntries(QList<Entry*>& 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<Entry*> BrowserService::confirmEntries(QList<Entry*>& pwEntriesToConfirm,
         config.save(entry);
     });
 
-    accessControlDialog.setItems(pwEntriesToConfirm, url, httpAuth);
+    accessControlDialog.setItems(pwEntriesToConfirm, siteUrlStr, httpAuth);
 
     QList<Entry*> allowedEntries;
     if (accessControlDialog.exec() == QDialog::Accepted) {
@@ -810,9 +798,9 @@ QList<Entry*> BrowserService::confirmEntries(QList<Entry*>& 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<Database>& 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<int> 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<Database> 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) {