From d12afeff4097dd78f1311511e0c8f74e1a810d47 Mon Sep 17 00:00:00 2001 From: Frederik Gladhorn Date: Sat, 7 Sep 2019 15:51:34 +0200 Subject: [PATCH] Set default redirect policy to NoLessSafeRedirectPolicy Not following redirects is not a feature, but just a hastle for everyone. The main issue with switching this default is that applications that actually do manual redirect handling will break in various ways. FollowRedirectsAttribute was removed as it no longer serves any purpose beyond duplicating the default value. [ChangeLog][Network] QNetworkAccessManager now follows redirects by default with the NoLessSafeRedirectPolicy. [ChangeLog][Potentially Source-Incompatible Changes] QNetworkRequest::FollowRedirectsAttribute was removed and has been superseded by QNetworkRequest::RedirectsPolicyAttribute Fixes: QTBUG-85901 Change-Id: Ic5b776180a4b84ac4fc895158bb5a66a3c91a042 Reviewed-by: Qt CI Bot Reviewed-by: Timur Pocheptsov --- src/network/access/qnetworkaccessmanager.cpp | 16 +++++------- src/network/access/qnetworkaccessmanager_p.h | 4 +-- src/network/access/qnetworkreply.cpp | 10 +++---- src/network/access/qnetworkreplyhttpimpl.cpp | 4 +-- src/network/access/qnetworkrequest.cpp | 26 ++++++------------- src/network/access/qnetworkrequest.h | 1 - tests/auto/network/access/http2/tst_http2.cpp | 2 +- .../qnetworkreply/tst_qnetworkreply.cpp | 22 ++++++++-------- 8 files changed, 34 insertions(+), 51 deletions(-) diff --git a/src/network/access/qnetworkaccessmanager.cpp b/src/network/access/qnetworkaccessmanager.cpp index b301dcd9b35..2c4b4134117 100644 --- a/src/network/access/qnetworkaccessmanager.cpp +++ b/src/network/access/qnetworkaccessmanager.cpp @@ -1019,16 +1019,13 @@ void QNetworkAccessManager::connectToHost(const QString &hostName, quint16 port) Use this function to enable or disable HTTP redirects on the manager's level. \note When creating a request QNetworkRequest::RedirectAttributePolicy has - the highest priority, next by priority is QNetworkRequest::FollowRedirectsAttribute. - Finally, the manager's policy has the lowest priority. + the highest priority, next by priority the manager's policy. - For backwards compatibility the default value is QNetworkRequest::ManualRedirectPolicy. - This may change in the future and some type of auto-redirect policy will become - the default; clients relying on manual redirect handling are encouraged to set + The default value is QNetworkRequest::NoLessSafeRedirectPolicy. + Clients relying on manual redirect handling are encouraged to set this policy explicitly in their code. - \sa redirectPolicy(), QNetworkRequest::RedirectPolicy, - QNetworkRequest::FollowRedirectsAttribute + \sa redirectPolicy(), QNetworkRequest::RedirectPolicy */ void QNetworkAccessManager::setRedirectPolicy(QNetworkRequest::RedirectPolicy policy) { @@ -1138,9 +1135,8 @@ QNetworkReply *QNetworkAccessManager::createRequest(QNetworkAccessManager::Opera Q_D(QNetworkAccessManager); QNetworkRequest req(originalReq); - if (redirectPolicy() != QNetworkRequest::ManualRedirectPolicy - && req.attribute(QNetworkRequest::RedirectPolicyAttribute).isNull() - && req.attribute(QNetworkRequest::FollowRedirectsAttribute).isNull()) { + if (redirectPolicy() != QNetworkRequest::NoLessSafeRedirectPolicy + && req.attribute(QNetworkRequest::RedirectPolicyAttribute).isNull()) { req.setAttribute(QNetworkRequest::RedirectPolicyAttribute, redirectPolicy()); } diff --git a/src/network/access/qnetworkaccessmanager_p.h b/src/network/access/qnetworkaccessmanager_p.h index 1edcc78f69f..da9f6fd0bde 100644 --- a/src/network/access/qnetworkaccessmanager_p.h +++ b/src/network/access/qnetworkaccessmanager_p.h @@ -85,7 +85,7 @@ class QNetworkAccessManagerPrivate: public QObjectPrivate #endif cookieJarCreated(false), defaultAccessControl(true), - redirectPolicy(QNetworkRequest::ManualRedirectPolicy), + redirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy), authenticationManager(QSharedPointer::create()) { } @@ -145,7 +145,7 @@ class QNetworkAccessManagerPrivate: public QObjectPrivate bool cookieJarCreated; bool defaultAccessControl; - QNetworkRequest::RedirectPolicy redirectPolicy; + QNetworkRequest::RedirectPolicy redirectPolicy = QNetworkRequest::NoLessSafeRedirectPolicy; // The cache with authorization data: QSharedPointer authenticationManager; diff --git a/src/network/access/qnetworkreply.cpp b/src/network/access/qnetworkreply.cpp index f09d7f98385..c69bcc951e0 100644 --- a/src/network/access/qnetworkreply.cpp +++ b/src/network/access/qnetworkreply.cpp @@ -298,13 +298,13 @@ QNetworkReplyPrivate::QNetworkReplyPrivate() \fn void QNetworkReply::redirected(const QUrl &url) \since 5.6 - This signal is emitted if the QNetworkRequest::FollowRedirectsAttribute was + This signal is emitted if the QNetworkRequest::ManualRedirectPolicy was set in the request and the server responded with a 3xx status (specifically 301, 302, 303, 305, 307 or 308 status code) with a valid url in the location header, indicating a HTTP redirect. The \a url parameter contains the new redirect url as returned by the server in the location header. - \sa QNetworkRequest::FollowRedirectsAttribute + \sa QNetworkRequest::RedirectPolicy */ /*! @@ -596,10 +596,10 @@ bool QNetworkReply::isRunning() const /*! Returns the URL of the content downloaded or uploaded. Note that - the URL may be different from that of the original request. If the - QNetworkRequest::FollowRedirectsAttribute was set in the request, then this + the URL may be different from that of the original request. + If redirections were enabled in the request, then this function returns the current url that the network API is accessing, i.e the - url emitted in the QNetworkReply::redirected signal. + url of the resource the request got redirected to. \sa request(), setUrl(), QNetworkRequest::url(), redirected() */ diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index f189f5be20f..88969c64831 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -670,12 +670,10 @@ void QNetworkReplyHttpImplPrivate::postRequest(const QNetworkRequest &newHttpReq } #endif - auto redirectPolicy = QNetworkRequest::ManualRedirectPolicy; + auto redirectPolicy = QNetworkRequest::NoLessSafeRedirectPolicy; const QVariant value = newHttpRequest.attribute(QNetworkRequest::RedirectPolicyAttribute); if (value.isValid()) redirectPolicy = qvariant_cast(value); - else if (newHttpRequest.attribute(QNetworkRequest::FollowRedirectsAttribute).toBool()) - redirectPolicy = QNetworkRequest::NoLessSafeRedirectPolicy; httpRequest.setRedirectPolicy(redirectPolicy); diff --git a/src/network/access/qnetworkrequest.cpp b/src/network/access/qnetworkrequest.cpp index 46c4648cbe8..03a7f0b1760 100644 --- a/src/network/access/qnetworkrequest.cpp +++ b/src/network/access/qnetworkrequest.cpp @@ -168,13 +168,11 @@ QT_BEGIN_NAMESPACE \value RedirectionTargetAttribute Replies only, type: QMetaType::QUrl (no default) If present, it indicates that the server is redirecting the - request to a different URL. The Network Access API does not by - default follow redirections: the application can - determine if the requested redirection should be allowed, - according to its security policies, or it can set - QNetworkRequest::FollowRedirectsAttribute to true (in which case - the redirection will be followed and this attribute will not - be present in the reply). + request to a different URL. The Network Access API does follow + redirections by default, but if + QNetworkRequest::ManualRedirectPolicy is enabled and + the redirect was not handled in redirected() then this + attribute will be present. The returned URL might be relative. Use QUrl::resolved() to create an absolute URL out of it. @@ -288,13 +286,6 @@ QT_BEGIN_NAMESPACE in 100 millisecond intervals. (This value was introduced in 5.5.) - \value FollowRedirectsAttribute - Requests only, type: QMetaType::Bool (default: false) - Indicates whether the Network Access API should automatically follow a - HTTP redirect response or not. Currently redirects that are insecure, - that is redirecting from "https" to "http" protocol, are not allowed. - (This value was introduced in 5.6.) - \value OriginalContentLengthAttribute Replies only, type QMetaType::Int Holds the original content-length attribute before being invalidated and @@ -304,8 +295,8 @@ QT_BEGIN_NAMESPACE \value RedirectPolicyAttribute Requests only, type: QMetaType::Int, should be one of the - QNetworkRequest::RedirectPolicy values (default: ManualRedirectPolicy). - This attribute obsoletes FollowRedirectsAttribute. + QNetworkRequest::RedirectPolicy values + (default: NoLessSafeRedirectPolicy). (This value was introduced in 5.9.) \value Http2DirectAttribute @@ -386,8 +377,6 @@ QT_BEGIN_NAMESPACE \value NoLessSafeRedirectPolicy Only "http"->"http", "http" -> "https" or "https" -> "https" redirects are allowed. - Equivalent to setting the old FollowRedirectsAttribute - to true \value SameOriginRedirectPolicy Require the same protocol, host and port. Note, http://example.com and http://example.com:80 @@ -493,6 +482,7 @@ class QNetworkRequestPrivate: public QSharedData, public QNetworkHeadersPrivate QNetworkRequest::QNetworkRequest() : d(new QNetworkRequestPrivate) { + #if QT_CONFIG(http) // Initial values proposed by RFC 7540 are quite draconian, // so unless an application will set its own parameters, we diff --git a/src/network/access/qnetworkrequest.h b/src/network/access/qnetworkrequest.h index e0476d80369..e4dd5870cfb 100644 --- a/src/network/access/qnetworkrequest.h +++ b/src/network/access/qnetworkrequest.h @@ -90,7 +90,6 @@ class Q_NETWORK_EXPORT QNetworkRequest SynchronousRequestAttribute, // internal BackgroundRequestAttribute, EmitAllUploadProgressSignalsAttribute = BackgroundRequestAttribute + 3, - FollowRedirectsAttribute, Http2AllowedAttribute, Http2WasUsedAttribute, OriginalContentLengthAttribute, diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index 775c9604b99..bc685e5ca98 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -935,7 +935,7 @@ void tst_Http2::sendRequest(int streamNumber, QNetworkRequest request(url); request.setAttribute(QNetworkRequest::Http2AllowedAttribute, QVariant(true)); - request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, QVariant(true)); + request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy); request.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("text/plain")); request.setPriority(priority); request.setHttp2Configuration(h2Config); diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp index 109d4ce4064..6bec0c8ee11 100644 --- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp @@ -8178,7 +8178,7 @@ void tst_QNetworkReply::ioHttpSingleRedirect() localhost.setPort(server.serverPort()); QNetworkRequest request(localhost); - request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); + request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy); QNetworkReplyPtr reply(manager.get(request)); QSignalSpy redSpy(reply.data(), SIGNAL(redirected(QUrl))); @@ -8223,7 +8223,7 @@ void tst_QNetworkReply::ioHttpChangeMaxRedirects() localhost.setPort(server1.serverPort()); QNetworkRequest request(localhost); - request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); + request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy); // Set Max redirects to 1. This will cause TooManyRedirectsError request.setMaximumRedirectsAllowed(1); @@ -8285,7 +8285,7 @@ void tst_QNetworkReply::ioHttpRedirectErrors() server.setDataToTransmit(d2s); QNetworkRequest request(localhost); - request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); + request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy); QNetworkReplyPtr reply(manager.get(request)); if (localhost.scheme() == "https") reply.data()->ignoreSslErrors(); @@ -8366,7 +8366,7 @@ void tst_QNetworkReply::ioHttpRedirectPolicy() redirectServer.responses.push_back(tempRedirectReplyStr().arg(QString(url.toEncoded())).toLatin1()); // This is the default one we preserve between tests. - QCOMPARE(manager.redirectPolicy(), QNetworkRequest::ManualRedirectPolicy); + QCOMPARE(manager.redirectPolicy(), QNetworkRequest::NoLessSafeRedirectPolicy); manager.setRedirectPolicy(policy); QCOMPARE(manager.redirectPolicy(), policy); @@ -8375,7 +8375,7 @@ void tst_QNetworkReply::ioHttpRedirectPolicy() reply->ignoreSslErrors(); // Restore default: - manager.setRedirectPolicy(QNetworkRequest::ManualRedirectPolicy); + manager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy); QSignalSpy redirectSpy(reply.data(), SIGNAL(redirected(QUrl))); QSignalSpy finishedSpy(reply.data(), SIGNAL(finished())); QVERIFY2(waitForFinish(reply) == Success, msgWaitForFinished(reply)); @@ -8447,15 +8447,15 @@ void tst_QNetworkReply::ioHttpRedirectPolicyErrors() QNetworkRequest request(url); request.setMaximumRedirectsAllowed(maxRedirects); - // We always reset the policy to the default one ('Manual') after any related + // We always reset the policy to the default one ('NoLessSafe') after any related // test is finished: - QCOMPARE(manager.redirectPolicy(), QNetworkRequest::ManualRedirectPolicy); + QCOMPARE(manager.redirectPolicy(), QNetworkRequest::NoLessSafeRedirectPolicy); manager.setRedirectPolicy(policy); QCOMPARE(manager.redirectPolicy(), policy); QNetworkReplyPtr reply(manager.get(request)); // Set it back to default: - manager.setRedirectPolicy(QNetworkRequest::ManualRedirectPolicy); + manager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy); if (ssl) reply->ignoreSslErrors(); @@ -8490,7 +8490,7 @@ void tst_QNetworkReply::ioHttpUserVerifiedRedirect() redirectServer.setDataToTransmit(tempRedirectReplyStr().arg(QString(url.toEncoded())).toLatin1()); url.setPort(redirectServer.serverPort()); - QCOMPARE(manager.redirectPolicy(), QNetworkRequest::ManualRedirectPolicy); + QCOMPARE(manager.redirectPolicy(), QNetworkRequest::NoLessSafeRedirectPolicy); manager.setRedirectPolicy(QNetworkRequest::UserVerifiedRedirectPolicy); QCOMPARE(manager.redirectPolicy(), QNetworkRequest::UserVerifiedRedirectPolicy); @@ -8508,8 +8508,8 @@ void tst_QNetworkReply::ioHttpUserVerifiedRedirect() }); // Before any test failed, reset the policy to default: - manager.setRedirectPolicy(QNetworkRequest::ManualRedirectPolicy); - QCOMPARE(manager.redirectPolicy(), QNetworkRequest::ManualRedirectPolicy); + manager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy); + QCOMPARE(manager.redirectPolicy(), QNetworkRequest::NoLessSafeRedirectPolicy); QSignalSpy finishedSpy(reply.data(), SIGNAL(finished())); waitForFinish(reply);