Merge pull request #10291 from Chocobo1/redirect

Fix crash due to network redirections

src/base/net/downloadhandler.cpp

@@ -41,10 +41,11 @@
 #include "base/utils/fs.h"
 #include "base/utils/gzip.h"
 #include "base/utils/misc.h"
-#include "downloadmanager.h"
 
 namespace
 {
+    const int MAX_REDIRECTIONS = 20;  // the common value for web browsers
+
     bool saveToFile(const QByteArray &replyData, QString &filePath)
     {
         QTemporaryFile tmpfile {Utils::Fs::tempPath() + "XXXXXX"};
@@ -95,29 +96,30 @@ QString Net::DownloadHandler::url() const
 
 void Net::DownloadHandler::processFinishedDownload()
 {
-    QString url = m_reply->url().toString();
+    const QString url = m_reply->url().toString();
     qDebug("Download finished: %s", qUtf8Printable(url));
+
     // Check if the request was successful
     if (m_reply->error() != QNetworkReply::NoError) {
         // Failure
         qDebug("Download failure (%s), reason: %s", qUtf8Printable(url), qUtf8Printable(errorCodeToString(m_reply->error())));
         emit downloadFailed(m_downloadRequest.url(), errorCodeToString(m_reply->error()));
         this->deleteLater();
+        return;
     }
-    else {
+
     // Check if the server ask us to redirect somewhere else
     const QVariant redirection = m_reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
     if (redirection.isValid()) {
         // We should redirect
         handleRedirection(redirection.toUrl());
+        return;
     }
-        else {
+
     // Success
-            QByteArray replyData = m_reply->readAll();
-            if (m_reply->rawHeader("Content-Encoding") == "gzip") {
-                // decompress gzip reply
-                replyData = Utils::Gzip::decompress(replyData);
-            }
+    const QByteArray replyData = (m_reply->rawHeader("Content-Encoding") == "gzip")
+        ? Utils::Gzip::decompress(m_reply->readAll())
+        : m_reply->readAll();
 
     if (m_downloadRequest.saveToFile()) {
         QString filePath;
@@ -131,8 +133,6 @@ void Net::DownloadHandler::processFinishedDownload()
     }
 
     this->deleteLater();
-        }
-    }
 }
 
 void Net::DownloadHandler::checkDownloadSize(qint64 bytesReceived, qint64 bytesTotal)
@@ -155,13 +155,17 @@ void Net::DownloadHandler::checkDownloadSize(qint64 bytesReceived, qint64 bytesT
     }
 }
 
-void Net::DownloadHandler::handleRedirection(QUrl newUrl)
+void Net::DownloadHandler::handleRedirection(const QUrl &newUrl)
 {
-    // Resolve relative urls
-    if (newUrl.isRelative())
-        newUrl = m_reply->url().resolved(newUrl);
+    if (m_redirectionCounter >= MAX_REDIRECTIONS) {
+        emit downloadFailed(url(), tr("Exceeded max redirections (%1)").arg(MAX_REDIRECTIONS));
+        this->deleteLater();
+        return;
+    }
 
-    const QString newUrlString = newUrl.toString();
+    // Resolve relative urls
+    const QUrl resolvedUrl = (newUrl.isRelative()) ? m_reply->url().resolved(newUrl) : newUrl;
+    const QString newUrlString = resolvedUrl.toString();
     qDebug("Redirecting from %s to %s...", qUtf8Printable(m_reply->url().toString()), qUtf8Printable(newUrlString));
 
     // Redirect to magnet workaround
@@ -174,9 +178,11 @@ void Net::DownloadHandler::handleRedirection(QUrl newUrl)
             emit downloadFailed(m_downloadRequest.url(), tr("Unexpected redirect to magnet URI."));
 
         this->deleteLater();
+        return;
     }
-    else {
+
     DownloadHandler *redirected = m_manager->download(DownloadRequest(m_downloadRequest).url(newUrlString));
+    redirected->m_redirectionCounter = (m_redirectionCounter + 1);
     connect(redirected, &DownloadHandler::destroyed, this, &DownloadHandler::deleteLater);
     connect(redirected, &DownloadHandler::downloadFailed, this, [this](const QString &, const QString &reason)
     {
@@ -196,7 +202,6 @@ void Net::DownloadHandler::handleRedirection(QUrl newUrl)
     {
         emit downloadFinished(url(), data);
     });
-    }
 }
 
 QString Net::DownloadHandler::errorCodeToString(const QNetworkReply::NetworkError status)

src/base/net/downloadhandler.h

@@ -67,13 +67,14 @@ namespace Net
 
     private:
         void assignNetworkReply(QNetworkReply *reply);
-        void handleRedirection(QUrl newUrl);
+        void handleRedirection(const QUrl &newUrl);
 
         static QString errorCodeToString(QNetworkReply::NetworkError status);
 
         QNetworkReply *m_reply;
         DownloadManager *m_manager;
         const DownloadRequest m_downloadRequest;
+        short m_redirectionCounter = 0;
     };
 }
 

src/base/net/downloadmanager.cpp

@@ -42,6 +42,7 @@
 #include <QUrl>
 
 #include "base/global.h"
+#include "base/logger.h"
 #include "base/preferences.h"
 #include "downloadhandler.h"
 #include "proxyconfigurationmanager.h"
@@ -130,9 +131,7 @@ Net::DownloadManager *Net::DownloadManager::m_instance = nullptr;
 Net::DownloadManager::DownloadManager(QObject *parent)
     : QObject(parent)
 {
-#ifndef QT_NO_OPENSSL
     connect(&m_networkManager, &QNetworkAccessManager::sslErrors, this, &Net::DownloadManager::ignoreSslErrors);
-#endif
     connect(&m_networkManager, &QNetworkAccessManager::finished, this, &DownloadManager::handleReplyFinished);
     connect(ProxyConfigurationManager::instance(), &ProxyConfigurationManager::proxyConfigurationChanged
             , this, &DownloadManager::applyProxySettings);
@@ -269,14 +268,16 @@ void Net::DownloadManager::handleReplyFinished(QNetworkReply *reply)
     handler->disconnect(this);
 }
 
-#ifndef QT_NO_OPENSSL
 void Net::DownloadManager::ignoreSslErrors(QNetworkReply *reply, const QList<QSslError> &errors)
 {
-    Q_UNUSED(errors)
+    QStringList errorList;
+    for (const QSslError &error : errors)
+        errorList += error.errorString();
+    LogMsg(tr("Ignoring SSL error, URL: \"%1\", errors: \"%2\"").arg(reply->url().toString(), errorList.join(". ")), Log::WARNING);
+
     // Ignore all SSL errors
     reply->ignoreSslErrors();
 }
-#endif
 
 Net::DownloadRequest::DownloadRequest(const QString &url)
     : m_url {url}
@@ -343,7 +344,7 @@ Net::ServiceID Net::ServiceID::fromURL(const QUrl &url)
     return {url.host(), url.port(80)};
 }
 
-uint Net::qHash(const ServiceID &serviceID, uint seed)
+uint Net::qHash(const ServiceID &serviceID, const uint seed)
 {
     return ::qHash(serviceID.hostName, seed) ^ serviceID.port;
 }

src/base/net/downloadmanager.h

@@ -37,8 +37,8 @@
 #include <QQueue>
 #include <QSet>
 
-class QNetworkReply;
 class QNetworkCookie;
+class QNetworkReply;
 class QSslError;
 class QUrl;
 
@@ -83,6 +83,9 @@ namespace Net
         static ServiceID fromURL(const QUrl &url);
     };
 
+    uint qHash(const ServiceID &serviceID, uint seed);
+    bool operator==(const ServiceID &lhs, const ServiceID &rhs);
+
     class DownloadManager : public QObject
     {
         Q_OBJECT
@@ -106,9 +109,7 @@ namespace Net
         static bool hasSupportedScheme(const QString &url);
 
     private slots:
-    #ifndef QT_NO_OPENSSL
         void ignoreSslErrors(QNetworkReply *, const QList<QSslError> &);
-    #endif
 
     private:
         explicit DownloadManager(QObject *parent = nullptr);
@@ -123,9 +124,6 @@ namespace Net
         QSet<ServiceID> m_busyServices;
         QHash<ServiceID, QQueue<DownloadHandler *>> m_waitingJobs;
     };
-
-    uint qHash(const ServiceID &serviceID, uint seed);
-    bool operator==(const ServiceID &lhs, const ServiceID &rhs);
 }
 
 #endif // NET_DOWNLOADMANAGER_H