Fix URL matching

src/browser/BrowserService.cpp

@@ -18,6 +18,7 @@
  */
 
 #include <QCheckBox>
+#include <QHostAddress>
 #include <QInputDialog>
 #include <QJsonArray>
 #include <QMessageBox>
@@ -600,17 +601,20 @@ BrowserService::searchEntries(const QSharedPointer<Database>& 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), hostname, url)) {
+                    if (key.startsWith(ADDITIONAL_URL)
+                        && handleURL(entry->attributes()->value(key), domain, url)) {
                         entries.append(entry);
                         continue;
                     }
                 }
             }
 
-            if (!handleURL(entry->url(), hostname, url)) {
+            if (!handleURL(entry->url(), domain, url)) {
                 continue;
             }
 
@@ -928,12 +932,15 @@ int BrowserService::sortPriority(const Entry* entry,
 {
     QUrl url(entry->url());
     if (url.scheme().isEmpty()) {
-        url.setScheme("http");
+        url.setScheme("https");
     }
     const QString entryURL = url.toString(QUrl::StripTrailingSlash);
     const QString baseEntryURL =
         url.toString(QUrl::StripTrailingSlash | QUrl::RemovePath | QUrl::RemoveQuery | QUrl::RemoveFragment);
 
+    if (!url.host().contains(".") && url.host() != "localhost") {
+        return 0;
+    }
     if (submitUrl == entryURL) {
         return 100;
     }
@@ -970,7 +977,7 @@ int BrowserService::sortPriority(const Entry* entry,
     return 0;
 }
 
-bool BrowserService::matchUrlScheme(const QString& url)
+bool BrowserService::schemeFound(const QString& url)
 {
     QUrl address(url);
     return !address.scheme().isEmpty();
@@ -995,21 +1002,49 @@ bool BrowserService::removeFirstDomain(QString& hostname)
 
 bool BrowserService::handleURL(const QString& entryUrl, const QString& hostname, const QString& url)
 {
-    QUrl entryQUrl(entryUrl);
+    if (entryUrl.isEmpty()) {
-    QString entryScheme = entryQUrl.scheme();
+        return false;
-    QUrl qUrl(url);
+    }
 
-    // Ignore entry if port or scheme defined in the URL doesn't match
+    QUrl entryQUrl;
-    if ((entryQUrl.port() > 0 && entryQUrl.port() != qUrl.port())
+    if (entryUrl.contains("://")) {
-        || (browserSettings()->matchUrlScheme() && !entryScheme.isEmpty() && entryScheme.compare(qUrl.scheme()) != 0)) {
+        entryQUrl = entryUrl;
+    } else {
+        entryQUrl = QUrl::fromUserInput(entryUrl);
+
+        if (browserSettings()->matchUrlScheme()) {
+            entryQUrl.setScheme("https");
+        }
+    }
+
+    // URL host validation fails
+    if (browserSettings()->matchUrlScheme() && entryQUrl.host().isEmpty()) {
+        return false;
+    }
+
+    // Match port, if used
+    QUrl qUrl(url);
+    if (entryQUrl.port() > 0 && entryQUrl.port() != qUrl.port()) {
+        return false;
+    }
+
+    // Match scheme
+    if (browserSettings()->matchUrlScheme() && !entryQUrl.scheme().isEmpty() && entryQUrl.scheme().compare(qUrl.scheme()) != 0) {
+        return false;
+    }
+
+    // Check for illegal characters
+    QRegularExpression re("[<>\\^`{|}]");
+    auto match = re.match(entryUrl);
+    if (match.hasMatch()) {
         return false;
     }
 
     // Filter to match hostname in URL field
-    if ((!entryUrl.isEmpty() && hostname.contains(entryUrl))
+    if (entryQUrl.host().endsWith(hostname)) {
-        || (matchUrlScheme(entryUrl) && hostname.endsWith(entryQUrl.host()))) {
         return true;
     }
+
     return false;
 };
 
@@ -1018,19 +1053,25 @@ bool BrowserService::handleURL(const QString& entryUrl, const QString& hostname,
  *
  * Returns the base domain, e.g. https://another.example.co.uk -> example.co.uk
  */
-QString BrowserService::baseDomain(const QString& url) const
+QString BrowserService::baseDomain(const QString& hostname) const
 {
-    QUrl qurl = QUrl::fromUserInput(url);
+    QUrl qurl = QUrl::fromUserInput(hostname);
-    QString hostname = qurl.host();
+    QString host = qurl.host();
 
-    if (hostname.isEmpty() || !hostname.contains(qurl.topLevelDomain())) {
+    // If the hostname is an IP address, return it directly
+    QHostAddress hostAddress(hostname);
+    if (!hostAddress.isNull()) {
+        return hostname;
+    }
+
+    if (host.isEmpty() || !host.contains(qurl.topLevelDomain())) {
         return {};
     }
 
     // Remove the top level domain part from the hostname, e.g. https://another.example.co.uk -> https://another.example
-    hostname.chop(qurl.topLevelDomain().length());
+    host.chop(qurl.topLevelDomain().length());
     // Split the URL and select the last part, e.g. https://another.example -> example
-    QString baseDomain = hostname.split('.').last();
+    QString baseDomain = host.split('.').last();
     // Append the top level domain back to the URL, e.g. example -> example.co.uk
     baseDomain.append(qurl.topLevelDomain());
     return baseDomain;
@@ -1070,7 +1111,7 @@ QSharedPointer<Database> BrowserService::selectedDatabase()
         if (res == QDialog::Accepted) {
             const auto selectedDatabase = browserEntrySaveDialog.getSelected();
             if (selectedDatabase.length() > 0) {
-                int index = selectedDatabase[0]->data(Qt::UserRole).toUInt();
+                int index = selectedDatabase[0]->data(Qt::UserRole).toInt();
                 return databaseWidgets[index]->database();
             }
         } else {
@@ -1188,7 +1229,7 @@ void BrowserService::raiseWindow(const bool force)
         m_prevWindowState = WindowState::Minimized;
     }
 #ifdef Q_OS_MACOS
-    Q_UNUSED(force);
+    Q_UNUSED(force)
 
     if (macUtils()->isHidden()) {
         m_prevWindowState = WindowState::Hidden;

src/browser/BrowserService.h

@@ -128,10 +128,10 @@ private:
     Group* getDefaultEntryGroup(const QSharedPointer<Database>& selectedDb = {});
     int
     sortPriority(const Entry* entry, const QString& host, const QString& submitUrl, const QString& baseSubmitUrl) const;
-    bool matchUrlScheme(const QString& url);
+    bool schemeFound(const QString& url);
     bool removeFirstDomain(QString& hostname);
     bool handleURL(const QString& entryUrl, const QString& hostname, const QString& url);
-    QString baseDomain(const QString& url) const;
+    QString baseDomain(const QString& hostname) const;
     QSharedPointer<Database> getDatabase();
     QSharedPointer<Database> selectedDatabase();
     QJsonArray getChildrenFromGroup(Group* group);

tests/TestBrowser.cpp

@@ -147,7 +147,7 @@ void TestBrowser::testSortPriority()
     entry6->setUrl("http://github.com/login");
     entry7->setUrl("github.com");
     entry8->setUrl("github.com/login");
-    entry9->setUrl("https://github");
+    entry9->setUrl("https://github"); // Invalid URL
     entry10->setUrl("github.com");
 
     // The extension uses the submitUrl as default for comparison
@@ -170,7 +170,7 @@ void TestBrowser::testSortPriority()
     QCOMPARE(res6, 0);
     QCOMPARE(res7, 0);
     QCOMPARE(res8, 0);
-    QCOMPARE(res9, 90);
+    QCOMPARE(res9, 0);
     QCOMPARE(res10, 0);
 }
 
@@ -188,7 +188,7 @@ void TestBrowser::testSearchEntries()
     urls.push_back("http://github.com/login");
     urls.push_back("github.com");
     urls.push_back("github.com/login");
-    urls.push_back("https://github");
+    urls.push_back("https://github"); // Invalid URL
     urls.push_back("github.com");
 
     for (int i = 0; i < urls.length(); ++i) {
@@ -202,24 +202,23 @@ void TestBrowser::testSearchEntries()
 
     browserSettings()->setMatchUrlScheme(false);
     auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url
+
+    QCOMPARE(result.length(), 9);
+    QCOMPARE(result[0]->url(), QString("https://github.com/login_page"));
+    QCOMPARE(result[1]->url(), QString("https://github.com/login"));
+    QCOMPARE(result[2]->url(), QString("https://github.com/"));
+    QCOMPARE(result[3]->url(), QString("github.com/login"));
+    QCOMPARE(result[4]->url(), QString("http://github.com"));
+    QCOMPARE(result[5]->url(), QString("http://github.com/login"));
+
+    // 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
     QCOMPARE(result.length(), 7);
     QCOMPARE(result[0]->url(), QString("https://github.com/login_page"));
     QCOMPARE(result[1]->url(), QString("https://github.com/login"));
     QCOMPARE(result[2]->url(), QString("https://github.com/"));
-    QCOMPARE(result[3]->url(), QString("http://github.com"));
+    QCOMPARE(result[3]->url(), QString("github.com/login"));
-    QCOMPARE(result[4]->url(), QString("http://github.com/login"));
-    QCOMPARE(result[5]->url(), QString("github.com"));
-    QCOMPARE(result[6]->url(), QString("github.com"));
-
-    // With matching there should be only 5 results
-    browserSettings()->setMatchUrlScheme(true);
-    result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url
-    QCOMPARE(result.length(), 5);
-    QCOMPARE(result[0]->url(), QString("https://github.com/login_page"));
-    QCOMPARE(result[1]->url(), QString("https://github.com/login"));
-    QCOMPARE(result[2]->url(), QString("https://github.com/"));
-    QCOMPARE(result[3]->url(), QString("github.com"));
-    QCOMPARE(result[4]->url(), QString("github.com"));
 }
 
 void TestBrowser::testSearchEntriesWithPort()
@@ -242,7 +241,7 @@ void TestBrowser::testSearchEntriesWithPort()
 
     auto result = m_browserService->searchEntries(db, "127.0.0.1", "http://127.0.0.1:443"); // db, hostname, url
     QCOMPARE(result.length(), 1);
-    QCOMPARE(result[0]->url(), QString("http://127.0.0.1:443"));
+    QCOMPARE(result[0]->url(), urls[0]);
 }
 
 void TestBrowser::testSearchEntriesWithAdditionalURLs()
@@ -271,12 +270,94 @@ void TestBrowser::testSearchEntriesWithAdditionalURLs()
 
     auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url
     QCOMPARE(result.length(), 1);
-    QCOMPARE(result[0]->url(), QString("https://github.com/"));
+    QCOMPARE(result[0]->url(), urls[0]);
 
     // Search the additional URL. It should return the same entry
     auto additionalResult = m_browserService->searchEntries(db, "keepassxc.org", "https://keepassxc.org");
     QCOMPARE(additionalResult.length(), 1);
-    QCOMPARE(additionalResult[0]->url(), QString("https://github.com/"));
+    QCOMPARE(additionalResult[0]->url(), urls[0]);
+}
+
+void TestBrowser::testInvalidEntries()
+{
+    auto db = QSharedPointer<Database>::create();
+    auto* root = db->rootGroup();
+
+    QList<QString> 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();
+    }
+
+    browserSettings()->setMatchUrlScheme(true);
+    auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url
+    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);
+}
+
+void TestBrowser::testSubdomainsAndPaths()
+{
+    auto db = QSharedPointer<Database>::create();
+    auto* root = db->rootGroup();
+
+    QList<QString> 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
+
+    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();
+    }
+
+    browserSettings()->setMatchUrlScheme(false);
+    auto result = m_browserService->searchEntries(db, "github.com", "https://github.com"); // db, hostname, url
+
+    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
+    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]);
 }
 
 void TestBrowser::testSortEntries()
@@ -293,7 +374,7 @@ void TestBrowser::testSortEntries()
     urls.push_back("http://github.com/login");
     urls.push_back("github.com");
     urls.push_back("github.com/login");
-    urls.push_back("https://github");
+    urls.push_back("https://github"); // Invalid URL
     urls.push_back("github.com");
 
     QList<Entry*> entries;
@@ -313,12 +394,13 @@ void TestBrowser::testSortEntries()
     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 8"));
+    QCOMPARE(result[1]->username(), QString("User 0"));
-    QCOMPARE(result[1]->url(), QString("https://github"));
+    QCOMPARE(result[1]->url(), QString("https://github.com/login_page"));
-    QCOMPARE(result[2]->username(), QString("User 0"));
+    QCOMPARE(result[2]->username(), QString("User 1"));
-    QCOMPARE(result[2]->url(), QString("https://github.com/login_page"));
+    QCOMPARE(result[2]->url(), QString("https://github.com/login"));
-    QCOMPARE(result[3]->username(), QString("User 1"));
+    QCOMPARE(result[3]->username(), QString("User 3"));
-    QCOMPARE(result[3]->url(), QString("https://github.com/login"));
+    QCOMPARE(result[3]->url(), QString("github.com/login"));
+
 }
 
 void TestBrowser::testGetDatabaseGroups()

tests/TestBrowser.h

@@ -43,6 +43,8 @@ private slots:
     void testSearchEntries();
     void testSearchEntriesWithPort();
     void testSearchEntriesWithAdditionalURLs();
+    void testInvalidEntries();
+    void testSubdomainsAndPaths();
     void testSortEntries();
     void testGetDatabaseGroups();