Merge pull request #9247 from nextcloud/bugfix/noid/qt6-redirects

nilsding · web-flow · commit 815a59aeb410 · 2025-12-15T13:24:54.000+01:00
fix(accessmanager): manually handle redirects
diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp
@@ -52,7 +52,7 @@ class WebFlowCredentialsAccessManager : public AccessManager
     QNetworkReply *createRequest(Operation op, const QNetworkRequest &request, QIODevice *outgoingData) override
     {
         QNetworkRequest req(request);
-        if (!req.attribute(WebFlowCredentials::DontAddCredentialsAttribute).toBool()) {
+        if (!req.attribute(AbstractCredentials::DontAddCredentialsAttribute).toBool()) {
             if (_cred && !_cred->password().isEmpty()) {
                 QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();
                 req.setRawHeader("Authorization", "Basic " + credHash);
diff --git a/src/gui/creds/webflowcredentials.h b/src/gui/creds/webflowcredentials.h
@@ -37,9 +37,6 @@ class WebFlowCredentials : public AbstractCredentials
     friend class WebFlowCredentialsAccessManager;
 
 public:
-    /// Don't add credentials if this is set on a QNetworkRequest
-    static constexpr QNetworkRequest::Attribute DontAddCredentialsAttribute = QNetworkRequest::User;
-
     explicit WebFlowCredentials();
     WebFlowCredentials(
             const QString &user,
diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp
@@ -273,10 +273,24 @@ void AbstractNetworkJob::slotFinished()
                     }
                     _requestBody->seek(0);
                 }
+
+                auto request = reply()->request();
+
+                if (!(requestedUrl.host() == redirectUrl.host() && requestedUrl.port() == redirectUrl.port())) {
+                    qCWarning(lcNetworkJob).nospace() << "redirect target mismatches origin, removing credentials"
+                        << " origin=" << requestedUrl.host() << ":" << requestedUrl.port()
+                        << " target=" << redirectUrl.host() << ":" << redirectUrl.port();
+
+                    auto headers = request.headers();
+                    headers.removeAll(QHttpHeaders::WellKnownHeader::Authorization);
+                    request.setHeaders(headers);
+                    request.setAttribute(AbstractCredentials::DontAddCredentialsAttribute, true);
+                }
+
                 sendRequest(
                     verb,
                     redirectUrl,
-                    reply()->request(),
+                    request,
                     _requestBody);
                 return;
             }
diff --git a/src/libsync/accessmanager.cpp b/src/libsync/accessmanager.cpp
@@ -82,6 +82,11 @@ QNetworkReply *AccessManager::createRequest(QNetworkAccessManager::Operation op,
     }
 #endif
 
+    // We handle redirects ourselves in AbstractNetworkJob::slotFinished
+    // Qt's automatic handling of redirects will transmit all set headers from the original
+    // request again, including e.g. `Authorization`.
+    newRequest.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::ManualRedirectPolicy);
+
     const auto reply = QNetworkAccessManager::createRequest(op, newRequest, outgoingData);
     HttpLogger::logRequest(reply, op, outgoingData);
     return reply;
diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h
@@ -9,6 +9,7 @@
 #define MIRALL_CREDS_ABSTRACT_CREDENTIALS_H
 
 #include <QObject>
+#include <QNetworkRequest>
 
 #include <csync.h>
 #include "owncloudlib.h"
@@ -25,6 +26,9 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject
     Q_OBJECT
 
 public:
+    /// Don't add credentials if this is set on a QNetworkRequest
+    static constexpr QNetworkRequest::Attribute DontAddCredentialsAttribute = QNetworkRequest::User;
+
     AbstractCredentials();
     // No need for virtual destructor - QObject already has one.
 
diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp
@@ -51,7 +51,7 @@ class HttpCredentialsAccessManager : public AccessManager
     QNetworkReply *createRequest(Operation op, const QNetworkRequest &request, QIODevice *outgoingData) override
     {
         QNetworkRequest req(request);
-        if (!req.attribute(HttpCredentials::DontAddCredentialsAttribute).toBool()) {
+        if (!req.attribute(AbstractCredentials::DontAddCredentialsAttribute).toBool()) {
             if (_cred && !_cred->password().isEmpty()) {
                 QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();
                 req.setRawHeader("Authorization", "Basic " + credHash);
diff --git a/src/libsync/creds/httpcredentials.h b/src/libsync/creds/httpcredentials.h
@@ -67,9 +67,6 @@ class OWNCLOUDSYNC_EXPORT HttpCredentials : public AbstractCredentials
     friend class HttpCredentialsAccessManager;
 
 public:
-    /// Don't add credentials if this is set on a QNetworkRequest
-    static constexpr QNetworkRequest::Attribute DontAddCredentialsAttribute = QNetworkRequest::User;
-
     HttpCredentials();
     explicit HttpCredentials(const QString &user, const QString &password,
             const QByteArray &clientCertBundle = QByteArray(), const QByteArray &clientCertPassword = QByteArray());
diff --git a/src/libsync/creds/tokencredentials.cpp b/src/libsync/creds/tokencredentials.cpp
@@ -47,8 +47,10 @@ class TokenCredentialsAccessManager : public AccessManager
 
         QNetworkRequest req(request);
 
-        QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();
-        req.setRawHeader(QByteArray("Authorization"), QByteArray("Basic ") + credHash);
+        if (!req.attribute(AbstractCredentials::DontAddCredentialsAttribute).toBool()) {
+            QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();
+            req.setRawHeader(QByteArray("Authorization"), QByteArray("Basic ") + credHash);
+        }
 
         // A pre-authenticated cookie
         QByteArray token = _cred->_token.toUtf8();
diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp
@@ -1181,7 +1181,7 @@ void DetermineAuthTypeJob::start()
 
     QNetworkRequest req;
     // Prevent HttpCredentialsAccessManager from setting an Authorization header.
-    req.setAttribute(HttpCredentials::DontAddCredentialsAttribute, true);
+    req.setAttribute(AbstractCredentials::DontAddCredentialsAttribute, true);
     // Don't reuse previous auth credentials
     req.setAttribute(QNetworkRequest::AuthenticationReuseAttribute, QNetworkRequest::Manual);
 
diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp
@@ -5,6 +5,7 @@
  */
 
 #include "config.h"
+#include "libsync/creds/abstractcredentials.h"
 #include "owncloudpropagator_p.h"
 #include "propagatedownload.h"
 #include "networkjobs.h"
@@ -122,6 +123,7 @@ void GETFileJob::start()
         sendRequest("GET", makeDavUrl(path()), req);
     } else {
         // Use direct URL
+        req.setAttribute(AbstractCredentials::DontAddCredentialsAttribute, true);
         sendRequest("GET", _directDownloadUrl, req);
     }
 
diff --git a/test/testnextcloudpropagator.cpp b/test/testnextcloudpropagator.cpp
@@ -12,6 +12,7 @@
 #include <QDebug>
 
 #include "accessmanager.h"
+#include "creds/webflowcredentials.h"
 #include "propagatedownload.h"
 #include "owncloudpropagator_p.h"
 #include "syncenginetestutils.h"
@@ -196,6 +197,140 @@ private slots:
             QCOMPARE(receivedContent.size(), decompressedContent.size());
         }
     }
+
+    void testDirectUrlCredentials()
+    {
+        // for this one we need two HTTP servers:
+        // - one that acts as a host for direct downloads
+        // - and another one that's our pretend-Nextcloud server
+        //
+        // in any case the requests made to the direct download server should
+        // never contain the `Authorization` header
+
+        QHttpServer directDownloadServer;
+        directDownloadServer.route("/data/directData", [](const QHttpServerRequest &request, QHttpServerResponder &responder) -> void {
+            QVERIFY(!request.headers().contains(QHttpHeaders::WellKnownHeader::Authorization));
+
+            QHttpHeaders headers;
+            headers.append(QHttpHeaders::WellKnownHeader::ContentType, "text/plain"_ba);
+            responder.write("OK directData"_ba, headers);
+        });
+        directDownloadServer.route("/data/redirectToOtherUrl", [](const QHttpServerRequest &request, QHttpServerResponder &responder) -> void {
+            QVERIFY(!request.headers().contains(QHttpHeaders::WellKnownHeader::Authorization));
+
+            QHttpHeaders headers;
+            headers.append(QHttpHeaders::WellKnownHeader::Location, "/someOtherPath/redirectTarget"_ba);
+            responder.write(""_ba, headers, QHttpServerResponder::StatusCode::Found);
+        });
+        directDownloadServer.route("/someOtherPath/redirectTarget", [](const QHttpServerRequest &request, QHttpServerResponder &responder) -> void {
+            QVERIFY(!request.headers().contains(QHttpHeaders::WellKnownHeader::Authorization));
+
+            QHttpHeaders headers;
+            headers.append(QHttpHeaders::WellKnownHeader::ContentType, "text/plain"_ba);
+            headers.append("OC-ETag"_ba, "0123456789abcdef"_ba);
+            headers.append("ETag"_ba, "0123456789abcdef"_ba);
+            responder.write("OK redirectTarget"_ba, headers);
+        });
+
+        QTcpServer directDownloadTcpServer;
+        QVERIFY(directDownloadTcpServer.listen(QHostAddress::LocalHost));
+        QVERIFY(directDownloadServer.bind(&directDownloadTcpServer));
+        const QString directDownloadBaseUrl = "http://%1:%2"_L1.arg(directDownloadTcpServer.serverAddress().toString(), QString::number(directDownloadTcpServer.serverPort()));
+        qInfo() << "Listening on" << directDownloadBaseUrl;
+
+        QHttpServer nextcloudServer;
+        nextcloudServer.route("/remote.php/dav/files/propagatortest/someFile", [](const QHttpServerRequest &request, QHttpServerResponder &responder) -> void {
+            QVERIFY(request.headers().contains(QHttpHeaders::WellKnownHeader::Authorization));
+
+            QHttpHeaders headers;
+            headers.append(QHttpHeaders::WellKnownHeader::ContentType, "text/plain"_ba);
+            headers.append("OC-ETag"_ba, "0123456789abcdef"_ba);
+            headers.append("ETag"_ba, "0123456789abcdef"_ba);
+            responder.write("OK someFile"_ba, headers);
+        });
+        nextcloudServer.route("/remote.php/dav/files/propagatortest/redirectedFile", [&directDownloadBaseUrl](const QHttpServerRequest &request, QHttpServerResponder &responder) -> void {
+            QVERIFY(request.headers().contains(QHttpHeaders::WellKnownHeader::Authorization));
+
+            QHttpHeaders headers;
+            headers.append(QHttpHeaders::WellKnownHeader::ContentType, "text/plain"_ba);
+            headers.append(QHttpHeaders::WellKnownHeader::Location, directDownloadBaseUrl + "/someOtherPath/redirectTarget"_L1);
+            responder.write(""_ba, headers, QHttpServerResponder::StatusCode::Found);
+        });
+
+        QTcpServer nextcloudTcpServer;
+        QVERIFY(nextcloudTcpServer.listen(QHostAddress::LocalHost));
+        QVERIFY(nextcloudServer.bind(&nextcloudTcpServer));
+        const QString nextcloudBaseUrl = "http://%1:%2"_L1.arg(nextcloudTcpServer.serverAddress().toString(), QString::number(nextcloudTcpServer.serverPort()));
+        qInfo() << "Listening on" << nextcloudBaseUrl;
+
+
+        auto account = OCC::Account::create();
+        // using WebFlowCredentials here to to make sure it works as expected for real
+        account->setCredentials(new WebFlowCredentials(u"propagatortest"_s, u"invalid"_s));
+        account->setDavUser(u"propagatortest"_s);
+        account->setUrl(nextcloudBaseUrl);
+
+
+        {
+            qInfo() << "Test: direct URL";
+            QBuffer receivedContent;
+            receivedContent.open(QIODevice::ReadWrite);
+
+            auto job = GETFileJob(account, QUrl{directDownloadBaseUrl + "/data/directData"_L1}, &receivedContent, {}, {}, 0);
+            QSignalSpy spy(&job, &GETFileJob::finishedSignal);
+
+            job.start();
+            spy.wait(1000);
+
+            // The temporary web servers verify the received headers
+            QCOMPARE(receivedContent.data(), "OK directData"_ba);
+        }
+
+        {
+            qInfo() << "Test: direct URL with redirect";
+            QBuffer receivedContent;
+            receivedContent.open(QIODevice::ReadWrite);
+
+            auto job = GETFileJob(account, QUrl{directDownloadBaseUrl + "/data/redirectToOtherUrl"_L1}, &receivedContent, {}, {}, 0);
+            QSignalSpy spy(&job, &GETFileJob::finishedSignal);
+
+            job.start();
+            spy.wait(1000);
+
+            // The temporary web servers verify the received headers
+            QCOMPARE(receivedContent.data(), "OK redirectTarget"_ba);
+        }
+
+        {
+            qInfo() << "Test: standard dav path";
+            QBuffer receivedContent;
+            receivedContent.open(QIODevice::ReadWrite);
+
+            auto job = GETFileJob(account, "/someFile"_L1, &receivedContent, {}, {}, 0);
+            QSignalSpy spy(&job, &GETFileJob::finishedSignal);
+
+            job.start();
+            spy.wait(1000);
+
+            // The temporary web servers verify the received headers
+            QCOMPARE(receivedContent.data(), "OK someFile"_ba);
+        }
+
+        {
+            qInfo() << "Test: standard dav path with a redirect to a different origin";
+            QBuffer receivedContent;
+            receivedContent.open(QIODevice::ReadWrite);
+
+            auto job = GETFileJob(account, "/redirectedFile"_L1, &receivedContent, {}, {}, 0);
+            QSignalSpy spy(&job, &GETFileJob::finishedSignal);
+
+            job.start();
+            spy.wait(1000);
+
+            // The temporary web servers verify the received headers
+            QCOMPARE(receivedContent.data(), "OK redirectTarget"_ba);
+        }
+    }
 #endif
 };
 

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ class WebFlowCredentialsAccessManager : public AccessManager`
`52`	`52`	`QNetworkReply createRequest(Operation op, const QNetworkRequest &request, QIODevice outgoingData) override`
`53`	`53`	`{`
`54`	`54`	`QNetworkRequest req(request);`
`55`		`- if (!req.attribute(WebFlowCredentials::DontAddCredentialsAttribute).toBool()) {`
	`55`	`+ if (!req.attribute(AbstractCredentials::DontAddCredentialsAttribute).toBool()) {`
`56`	`56`	`if (_cred && !_cred->password().isEmpty()) {`
`57`	`57`	`QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();`
`58`	`58`	`req.setRawHeader("Authorization", "Basic " + credHash);`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ class HttpCredentialsAccessManager : public AccessManager`
`51`	`51`	`QNetworkReply createRequest(Operation op, const QNetworkRequest &request, QIODevice outgoingData) override`
`52`	`52`	`{`
`53`	`53`	`QNetworkRequest req(request);`
`54`		`- if (!req.attribute(HttpCredentials::DontAddCredentialsAttribute).toBool()) {`
	`54`	`+ if (!req.attribute(AbstractCredentials::DontAddCredentialsAttribute).toBool()) {`
`55`	`55`	`if (_cred && !_cred->password().isEmpty()) {`
`56`	`56`	`QByteArray credHash = QByteArray(_cred->user().toUtf8() + ":" + _cred->password().toUtf8()).toBase64();`
`57`	`57`	`req.setRawHeader("Authorization", "Basic " + credHash);`