diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 2e5088c8ab..392d28c438 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -18,6 +18,7 @@ */ #include +#include #include #include #include @@ -600,17 +601,20 @@ 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), 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); - QString entryScheme = entryQUrl.scheme(); + if (entryUrl.isEmpty()) { + return false; + } + + QUrl entryQUrl; + if (entryUrl.contains("://")) { + 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; + } - // Ignore entry if port or scheme defined in the URL doesn't match - if ((entryQUrl.port() > 0 && entryQUrl.port() != qUrl.port()) - || (browserSettings()->matchUrlScheme() && !entryScheme.isEmpty() && entryScheme.compare(qUrl.scheme()) != 0)) { + // 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)) - || (matchUrlScheme(entryUrl) && hostname.endsWith(entryQUrl.host()))) { + if (entryQUrl.host().endsWith(hostname)) { 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); - QString hostname = qurl.host(); + QUrl qurl = QUrl::fromUserInput(hostname); + QString host = qurl.host(); + + // If the hostname is an IP address, return it directly + QHostAddress hostAddress(hostname); + if (!hostAddress.isNull()) { + return hostname; + } - if (hostname.isEmpty() || !hostname.contains(qurl.topLevelDomain())) { + 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 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; diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h index 81d3ed317a..cb20ecbfb7 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -128,10 +128,10 @@ public slots: Group* getDefaultEntryGroup(const QSharedPointer& 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 getDatabase(); QSharedPointer selectedDatabase(); QJsonArray getChildrenFromGroup(Group* group); diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 576b72c181..bb3318d07a 100644 --- a/tests/TestBrowser.cpp +++ b/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(), 7); + + 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("http://github.com")); - QCOMPARE(result[4]->url(), QString("http://github.com/login")); - QCOMPARE(result[5]->url(), QString("github.com")); - QCOMPARE(result[6]->url(), QString("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 5 results + // 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(), 5); + 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("github.com")); - QCOMPARE(result[4]->url(), QString("github.com")); + QCOMPARE(result[3]->url(), QString("github.com/login")); } 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::create(); + auto* root = db->rootGroup(); + + 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(); + } + + 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::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 + + 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 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]->url(), QString("https://github")); - QCOMPARE(result[2]->username(), QString("User 0")); - QCOMPARE(result[2]->url(), QString("https://github.com/login_page")); - QCOMPARE(result[3]->username(), QString("User 1")); - QCOMPARE(result[3]->url(), QString("https://github.com/login")); + 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")); + } void TestBrowser::testGetDatabaseGroups() diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index 0eed0d23f9..981c1642d1 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -43,6 +43,8 @@ private slots: void testSearchEntries(); void testSearchEntriesWithPort(); void testSearchEntriesWithAdditionalURLs(); + void testInvalidEntries(); + void testSubdomainsAndPaths(); void testSortEntries(); void testGetDatabaseGroups();