From 57af7c131d828318c0cd2cfbb7150934c30569e0 Mon Sep 17 00:00:00 2001 From: Patrick Klein Date: Sun, 4 Oct 2020 20:27:02 +0200 Subject: [PATCH] Fix favicon download from URL with non-standard port. Fixes #5001. The favicon download URL was constructed from scheme and host only. This is fixed by simply replacing the path of the original URL with "/favicon.ico", thus keeping scheme, host, auth and port intact. Further modification: URL's with a non-http schema are now rejected. --- src/gui/IconDownloader.cpp | 49 ++++++++++++++++++------- src/gui/IconDownloader.h | 2 + tests/CMakeLists.txt | 2 + tests/TestIconDownloader.cpp | 71 ++++++++++++++++++++++++++++++++++++ tests/TestIconDownloader.h | 35 ++++++++++++++++++ 5 files changed, 146 insertions(+), 13 deletions(-) create mode 100644 tests/TestIconDownloader.cpp create mode 100644 tests/TestIconDownloader.h diff --git a/src/gui/IconDownloader.cpp b/src/gui/IconDownloader.cpp index 1cb030c1e..d0940f55d 100644 --- a/src/gui/IconDownloader.cpp +++ b/src/gui/IconDownloader.cpp @@ -73,16 +73,30 @@ namespace void IconDownloader::setUrl(const QString& entryUrl) { m_url = entryUrl; - QUrl url(m_url); - if (!url.isValid()) { + QUrl url = QUrl::fromUserInput(m_url); + if (!url.isValid() || url.host().isEmpty()) { return; } m_redirects = 0; m_urlsToTry.clear(); - if (url.scheme().isEmpty()) { - url.setUrl(QString("https://%1").arg(url.toString())); + // Fall back to https if no scheme is specified + // fromUserInput defaults to http. Hence, we need to replace the default scheme should we detect that it has + // been added by fromUserInput + if (!entryUrl.startsWith(url.scheme())) { + url.setScheme("https"); + } else if (url.scheme() != "https" && url.scheme() != "http") { + return; + } + + // Remove query string - if any + if (url.hasQuery()) { + url.setQuery(QString()); + } + // Remove fragment - if any + if (url.hasFragment()) { + url.setFragment(QString()); } QString fullyQualifiedDomain = url.host(); @@ -91,16 +105,15 @@ void IconDownloader::setUrl(const QString& entryUrl) // searching for a match with the returned address(es). bool hostIsIp = false; QList hostAddressess = QHostInfo::fromName(fullyQualifiedDomain).addresses(); - for (const auto& addr : hostAddressess) { - if (addr.toString() == fullyQualifiedDomain) { - hostIsIp = true; - } - } + hostIsIp = + std::any_of(hostAddressess.begin(), hostAddressess.end(), [&fullyQualifiedDomain](const QHostAddress& addr) { + return addr.toString() == fullyQualifiedDomain; + }); // Determine the second-level domain, if available QString secondLevelDomain; if (!hostIsIp) { - secondLevelDomain = getSecondLevelDomain(m_url); + secondLevelDomain = getSecondLevelDomain(url); } // Start with the "fallback" url (if enabled) to try to get the best favicon @@ -117,11 +130,21 @@ void IconDownloader::setUrl(const QString& entryUrl) } // Add a direct pull of the website's own favicon.ico file - m_urlsToTry.append(QUrl(url.scheme() + "://" + fullyQualifiedDomain + "/favicon.ico")); + QUrl favicon_url = url; + favicon_url.setPath("/favicon.ico"); + m_urlsToTry.append(favicon_url); // Also try a direct pull of the second-level domain (if possible) - if (!hostIsIp && fullyQualifiedDomain != secondLevelDomain) { - m_urlsToTry.append(QUrl(url.scheme() + "://" + secondLevelDomain + "/favicon.ico")); + if (!hostIsIp && fullyQualifiedDomain != secondLevelDomain && !secondLevelDomain.isEmpty()) { + favicon_url.setHost(secondLevelDomain); + m_urlsToTry.append(favicon_url); + if (!favicon_url.userInfo().isEmpty() || favicon_url.port() != -1) { + // Remove additional fields from the URL as a fallback. Keep only host and scheme + // Fragment and query have been removed earlier + favicon_url.setPort(-1); + favicon_url.setUserInfo(QString()); + m_urlsToTry.append(favicon_url); + } } } diff --git a/src/gui/IconDownloader.h b/src/gui/IconDownloader.h index 008e57aab..f3d555300 100644 --- a/src/gui/IconDownloader.h +++ b/src/gui/IconDownloader.h @@ -59,6 +59,8 @@ private: QNetworkReply* m_reply; QTimer m_timeout; int m_redirects; + + friend class TestIconDownloader; }; #endif // KEEPASSXC_ICONDOWNLOADER_H \ No newline at end of file diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b532acba9..845bb8593 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -152,6 +152,8 @@ add_unit_test(NAME testopvaultreader SOURCES TestOpVaultReader.cpp if(WITH_XC_NETWORKING) add_unit_test(NAME testupdatecheck SOURCES TestUpdateCheck.cpp LIBS ${TEST_LIBRARIES}) + + add_unit_test(NAME testicondownloader SOURCES TestIconDownloader.cpp LIBS ${TEST_LIBRARIES}) endif() if(WITH_XC_AUTOTYPE) diff --git a/tests/TestIconDownloader.cpp b/tests/TestIconDownloader.cpp new file mode 100644 index 000000000..a0a1fa491 --- /dev/null +++ b/tests/TestIconDownloader.cpp @@ -0,0 +1,71 @@ +#include "TestIconDownloader.h" +#include +#include + +QTEST_GUILESS_MAIN(TestIconDownloader) + +void TestIconDownloader::testIconDownloader() +{ + QFETCH(QString, url); + QFETCH(QStringList, expectation); + + IconDownloader downloader; + downloader.setUrl(url); + QStringList resolved_urls; + for (const auto& resolved_url : downloader.m_urlsToTry) { + resolved_urls.push_back(resolved_url.toString()); + } + + QCOMPARE(resolved_urls, expectation); +} + +void TestIconDownloader::testIconDownloader_data() +{ + QTest::addColumn("url"); + QTest::addColumn("expectation"); + + const QString keepassxc_favicon("https://keepassxc.org/favicon.ico"); + + QTest::newRow("Invalid URL") << "http:sk/s.com" << QStringList{}; + QTest::newRow("Unsupported schema") << "ftp://google.com" << QStringList{}; + QTest::newRow("Missing schema") << "keepassxc.org" << QStringList{"https://keepassxc.org/favicon.ico"}; + QTest::newRow("Missing host") << "https:///register" << QStringList{}; + QTest::newRow("URL with path") << "https://keepassxc.org/register/here" << QStringList{keepassxc_favicon}; + QTest::newRow("URL with path and query") + << "https://keepassxc.org/register/here?login=me" << QStringList{keepassxc_favicon}; + QTest::newRow("URL with port") << "https://keepassxc.org:8080" + << QStringList{"https://keepassxc.org:8080/favicon.ico"}; + QTest::newRow("2nd level domain") << "https://login.keepassxc.org" + << QStringList{"https://login.keepassxc.org/favicon.ico", keepassxc_favicon}; + QTest::newRow("2nd level domain with additional fields") + << "https://user:password@login.keepassxc.org:2021?test#1" + << QStringList{"https://user:password@login.keepassxc.org:2021/favicon.ico", + "https://user:password@keepassxc.org:2021/favicon.ico", + keepassxc_favicon}; + QTest::newRow("2nd level domain (.co.uk special case), with subdomain") + << "https://login.keepassxc.co.uk" + << QStringList{"https://login.keepassxc.co.uk/favicon.ico", "https://keepassxc.co.uk/favicon.ico"}; + QTest::newRow("2nd level domain .co.uk special case") + << "https://keepassxc.co.uk" << QStringList{"https://keepassxc.co.uk/favicon.ico"}; + QTest::newRow("2nd level domain with several subdomains") + << "https://de.login.keepassxc.org" + << QStringList{"https://de.login.keepassxc.org/favicon.ico", keepassxc_favicon}; + QTest::newRow("Raw IP with schema") << "https://134.130.155.184" + << QStringList{"https://134.130.155.184/favicon.ico"}; + QTest::newRow("Raw IP") << "134.130.155.184" << QStringList{"https://134.130.155.184/favicon.ico"}; + QTest::newRow("Raw IP with schema and path") + << "https://134.130.155.184/with/path" << QStringList{"https://134.130.155.184/favicon.ico"}; + QTest::newRow("Raw IP with schema (https), path, and port") + << "https://134.130.155.184:8080/test" << QStringList{"https://134.130.155.184:8080/favicon.ico"}; + QTest::newRow("Raw IP with schema (http), path, and port") + << "134.130.155.184:8080/test" << QStringList{"https://134.130.155.184:8080/favicon.ico"}; + QTest::newRow("URL with username and password") + << "https://user:password@keepassxc.org" << QStringList{"https://user:password@keepassxc.org/favicon.ico"}; + QTest::newRow("URL with username and password, several subdomains") + << "https://user:password@de.login.keepassxc.org" + << QStringList{"https://user:password@de.login.keepassxc.org/favicon.ico", + "https://user:password@keepassxc.org/favicon.ico", + "https://keepassxc.org/favicon.ico"}; + QTest::newRow("Raw IP with username and password") + << "https://user:password@134.130.155.184" << QStringList{"https://user:password@134.130.155.184/favicon.ico"}; +} diff --git a/tests/TestIconDownloader.h b/tests/TestIconDownloader.h new file mode 100644 index 000000000..52db4d5ec --- /dev/null +++ b/tests/TestIconDownloader.h @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2011 Felix Geyer + * Copyright (C) 2019 KeePassXC Team + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 or (at your option) + * version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifndef KEEPASSXC_TESTICONDOWNLOADER_HPP +#define KEEPASSXC_TESTICONDOWNLOADER_HPP + +#include +#include +#include + +class TestIconDownloader : public QObject +{ + Q_OBJECT + +private slots: + void testIconDownloader(); + void testIconDownloader_data(); +}; + +#endif // KEEPASSXC_TESTICONDOWNLOADER_HPP