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

@@ -73,16 +73,30 @@ namespace
 void IconDownloader::setUrl(const QString& entryUrl)
 {
     m_url = entryUrl;
-    QUrl url(m_url);
+    QUrl url = QUrl::fromUserInput(m_url);
-    if (!url.isValid()) {
+    if (!url.isValid() || url.host().isEmpty()) {
         return;
     }
 
     m_redirects = 0;
     m_urlsToTry.clear();
 
-    if (url.scheme().isEmpty()) {
+    // Fall back to https if no scheme is specified
-        url.setUrl(QString("https://%1").arg(url.toString()));
+    // 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<QHostAddress> hostAddressess = QHostInfo::fromName(fullyQualifiedDomain).addresses();
-    for (const auto& addr : hostAddressess) {
+    hostIsIp =
-        if (addr.toString() == fullyQualifiedDomain) {
+        std::any_of(hostAddressess.begin(), hostAddressess.end(), [&fullyQualifiedDomain](const QHostAddress& addr) {
-            hostIsIp = true;
+            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) {
+    if (!hostIsIp && fullyQualifiedDomain != secondLevelDomain && !secondLevelDomain.isEmpty()) {
-        m_urlsToTry.append(QUrl(url.scheme() + "://" + secondLevelDomain + "/favicon.ico"));
+        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);
+        }
     }
 }
 

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

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)

tests/TestIconDownloader.cpp

@@ -0,0 +1,71 @@
+#include "TestIconDownloader.h"
+#include <QTest>
+#include <gui/IconDownloader.h>
+
+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<QString>("url");
+    QTest::addColumn<QStringList>("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"};
+}

tests/TestIconDownloader.h

@@ -0,0 +1,35 @@
+/*
+ *  Copyright (C) 2011 Felix Geyer <debfx@fobos.de>
+ *  Copyright (C) 2019 KeePassXC Team <team@keepassxc.org>
+ *
+ *  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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef KEEPASSXC_TESTICONDOWNLOADER_HPP
+#define KEEPASSXC_TESTICONDOWNLOADER_HPP
+
+#include <QList>
+#include <QObject>
+#include <QString>
+
+class TestIconDownloader : public QObject
+{
+    Q_OBJECT
+
+private slots:
+    void testIconDownloader();
+    void testIconDownloader_data();
+};
+
+#endif // KEEPASSXC_TESTICONDOWNLOADER_HPP