From 313a95bdd125d50335909e5a68605a6cfa9f68dd Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Wed, 13 Feb 2019 20:09:08 +0800 Subject: [PATCH 1/3] Log DownloadManager SSL errors --- src/base/net/downloadmanager.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/base/net/downloadmanager.cpp b/src/base/net/downloadmanager.cpp index 68a251d57..71a939a6a 100644 --- a/src/base/net/downloadmanager.cpp +++ b/src/base/net/downloadmanager.cpp @@ -42,6 +42,7 @@ #include #include "base/global.h" +#include "base/logger.h" #include "base/preferences.h" #include "downloadhandler.h" #include "proxyconfigurationmanager.h" @@ -272,7 +273,11 @@ void Net::DownloadManager::handleReplyFinished(QNetworkReply *reply) #ifndef QT_NO_OPENSSL void Net::DownloadManager::ignoreSslErrors(QNetworkReply *reply, const QList &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(); } From 409557ef301aeea8e37539d373700bbb02f8a3ee Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Wed, 13 Feb 2019 20:41:58 +0800 Subject: [PATCH 2/3] Clean up code --- src/base/net/downloadhandler.cpp | 108 +++++++++++++++---------------- src/base/net/downloadhandler.h | 2 +- src/base/net/downloadmanager.cpp | 6 +- src/base/net/downloadmanager.h | 10 ++- 4 files changed, 58 insertions(+), 68 deletions(-) diff --git a/src/base/net/downloadhandler.cpp b/src/base/net/downloadhandler.cpp index 7cce44e2a..a194d177d 100644 --- a/src/base/net/downloadhandler.cpp +++ b/src/base/net/downloadhandler.cpp @@ -41,7 +41,6 @@ #include "base/utils/fs.h" #include "base/utils/gzip.h" #include "base/utils/misc.h" -#include "downloadmanager.h" namespace { @@ -95,44 +94,43 @@ 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; + } + + // 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; + } + + // Success + const QByteArray replyData = (m_reply->rawHeader("Content-Encoding") == "gzip") + ? Utils::Gzip::decompress(m_reply->readAll()) + : m_reply->readAll(); + + if (m_downloadRequest.saveToFile()) { + QString filePath; + if (saveToFile(replyData, filePath)) + emit downloadFinished(m_downloadRequest.url(), filePath); + else + emit downloadFailed(m_downloadRequest.url(), tr("I/O Error")); } 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()); - } - else { - // Success - QByteArray replyData = m_reply->readAll(); - if (m_reply->rawHeader("Content-Encoding") == "gzip") { - // decompress gzip reply - replyData = Utils::Gzip::decompress(replyData); - } - - if (m_downloadRequest.saveToFile()) { - QString filePath; - if (saveToFile(replyData, filePath)) - emit downloadFinished(m_downloadRequest.url(), filePath); - else - emit downloadFailed(m_downloadRequest.url(), tr("I/O Error")); - } - else { - emit downloadFinished(m_downloadRequest.url(), replyData); - } - - this->deleteLater(); - } + emit downloadFinished(m_downloadRequest.url(), replyData); } + + this->deleteLater(); } void Net::DownloadHandler::checkDownloadSize(qint64 bytesReceived, qint64 bytesTotal) @@ -155,13 +153,11 @@ 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); - - const QString newUrlString = newUrl.toString(); + 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,29 +170,29 @@ 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)); - connect(redirected, &DownloadHandler::destroyed, this, &DownloadHandler::deleteLater); - connect(redirected, &DownloadHandler::downloadFailed, this, [this](const QString &, const QString &reason) - { - emit downloadFailed(url(), reason); - }); - connect(redirected, &DownloadHandler::redirectedToMagnet, this, [this](const QString &, const QString &magnetUri) - { - emit redirectedToMagnet(url(), magnetUri); - }); - connect(redirected, static_cast(&DownloadHandler::downloadFinished) - , this, [this](const QString &, const QString &fileName) - { - emit downloadFinished(url(), fileName); - }); - connect(redirected, static_cast(&DownloadHandler::downloadFinished) - , this, [this](const QString &, const QByteArray &data) - { - emit downloadFinished(url(), data); - }); - } + + const DownloadHandler *redirected = m_manager->download(DownloadRequest(m_downloadRequest).url(newUrlString)); + connect(redirected, &DownloadHandler::destroyed, this, &DownloadHandler::deleteLater); + connect(redirected, &DownloadHandler::downloadFailed, this, [this](const QString &, const QString &reason) + { + emit downloadFailed(url(), reason); + }); + connect(redirected, &DownloadHandler::redirectedToMagnet, this, [this](const QString &, const QString &magnetUri) + { + emit redirectedToMagnet(url(), magnetUri); + }); + connect(redirected, static_cast(&DownloadHandler::downloadFinished) + , this, [this](const QString &, const QString &fileName) + { + emit downloadFinished(url(), fileName); + }); + connect(redirected, static_cast(&DownloadHandler::downloadFinished) + , this, [this](const QString &, const QByteArray &data) + { + emit downloadFinished(url(), data); + }); } QString Net::DownloadHandler::errorCodeToString(const QNetworkReply::NetworkError status) diff --git a/src/base/net/downloadhandler.h b/src/base/net/downloadhandler.h index 5d154fb38..653e506a6 100644 --- a/src/base/net/downloadhandler.h +++ b/src/base/net/downloadhandler.h @@ -67,7 +67,7 @@ namespace Net private: void assignNetworkReply(QNetworkReply *reply); - void handleRedirection(QUrl newUrl); + void handleRedirection(const QUrl &newUrl); static QString errorCodeToString(QNetworkReply::NetworkError status); diff --git a/src/base/net/downloadmanager.cpp b/src/base/net/downloadmanager.cpp index 71a939a6a..da29e4c2f 100644 --- a/src/base/net/downloadmanager.cpp +++ b/src/base/net/downloadmanager.cpp @@ -131,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); @@ -270,7 +268,6 @@ void Net::DownloadManager::handleReplyFinished(QNetworkReply *reply) handler->disconnect(this); } -#ifndef QT_NO_OPENSSL void Net::DownloadManager::ignoreSslErrors(QNetworkReply *reply, const QList &errors) { QStringList errorList; @@ -281,7 +278,6 @@ void Net::DownloadManager::ignoreSslErrors(QNetworkReply *reply, const QListignoreSslErrors(); } -#endif Net::DownloadRequest::DownloadRequest(const QString &url) : m_url {url} @@ -348,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; } diff --git a/src/base/net/downloadmanager.h b/src/base/net/downloadmanager.h index 3aa1a1399..a04701c89 100644 --- a/src/base/net/downloadmanager.h +++ b/src/base/net/downloadmanager.h @@ -37,8 +37,8 @@ #include #include -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 &); - #endif private: explicit DownloadManager(QObject *parent = nullptr); @@ -123,9 +124,6 @@ namespace Net QSet m_busyServices; QHash> m_waitingJobs; }; - - uint qHash(const ServiceID &serviceID, uint seed); - bool operator==(const ServiceID &lhs, const ServiceID &rhs); } #endif // NET_DOWNLOADMANAGER_H From 8fe1ff87f1c06aa90e54f8ce84d408cee06cda0f Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Wed, 13 Feb 2019 21:56:48 +0800 Subject: [PATCH 3/3] Limit DownloadHandler max redirection to 20 Closes #10219. --- src/base/net/downloadhandler.cpp | 11 ++++++++++- src/base/net/downloadhandler.h | 1 + 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/base/net/downloadhandler.cpp b/src/base/net/downloadhandler.cpp index a194d177d..1515eb83c 100644 --- a/src/base/net/downloadhandler.cpp +++ b/src/base/net/downloadhandler.cpp @@ -44,6 +44,8 @@ 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"}; @@ -155,6 +157,12 @@ void Net::DownloadHandler::checkDownloadSize(qint64 bytesReceived, qint64 bytesT void Net::DownloadHandler::handleRedirection(const QUrl &newUrl) { + if (m_redirectionCounter >= MAX_REDIRECTIONS) { + emit downloadFailed(url(), tr("Exceeded max redirections (%1)").arg(MAX_REDIRECTIONS)); + this->deleteLater(); + return; + } + // Resolve relative urls const QUrl resolvedUrl = (newUrl.isRelative()) ? m_reply->url().resolved(newUrl) : newUrl; const QString newUrlString = resolvedUrl.toString(); @@ -173,7 +181,8 @@ void Net::DownloadHandler::handleRedirection(const QUrl &newUrl) return; } - const DownloadHandler *redirected = m_manager->download(DownloadRequest(m_downloadRequest).url(newUrlString)); + 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) { diff --git a/src/base/net/downloadhandler.h b/src/base/net/downloadhandler.h index 653e506a6..cd13a8448 100644 --- a/src/base/net/downloadhandler.h +++ b/src/base/net/downloadhandler.h @@ -74,6 +74,7 @@ namespace Net QNetworkReply *m_reply; DownloadManager *m_manager; const DownloadRequest m_downloadRequest; + short m_redirectionCounter = 0; }; }