From 8c2518bee8c2b98c95b202d94b1fdb803651ef6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 2 Apr 2024 11:11:52 +0200 Subject: [PATCH] Http2: fix handling unsuppported authenticate challenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When adding/fixing parts earlier it was missed that it was not handling the _unsupported_ case, when authentication is not handled and there is no resend. But there _is_ a challenge header. Pick-to: 6.5 Fixes: QTBUG-123891 Change-Id: I21470df0ce2528bad3babffc6e9f19b7afd29d20 Reviewed-by: Edward Welbourne (cherry picked from commit 4f9387f2aee19e38f05cab76a71f0d067b8d80dd) Reviewed-by: Mårten Nordheim --- src/network/access/qhttp2protocolhandler.cpp | 26 +++--- tests/auto/network/access/http2/tst_http2.cpp | 86 +++++++++++++++++++ 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index f2508383d8b..0abd99b9bc2 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -1197,12 +1197,14 @@ void QHttp2ProtocolHandler::handleAuthorization(Stream &stream) // In this case IIS will fall back to HTTP/1.1." // Though it might be OK to ignore this. The server shouldn't let us connect with // HTTP/2 if it doesn't support us using it. - } else if (!auth.isEmpty()) { - // Somewhat mimics parts of QHttpNetworkConnectionChannel::handleStatus - bool resend = false; - const bool authenticateHandled = m_connection->d_func()->handleAuthenticateChallenge( - m_socket, httpReply, isProxy, resend); - if (authenticateHandled && resend) { + return false; + } + // Somewhat mimics parts of QHttpNetworkConnectionChannel::handleStatus + bool resend = false; + const bool authenticateHandled = m_connection->d_func()->handleAuthenticateChallenge( + m_socket, httpReply, isProxy, resend); + if (authenticateHandled) { + if (resend) { httpReply->d_func()->eraseData(); // Add the request back in queue, we'll retry later now that // we've gotten some username/password set on it: @@ -1217,11 +1219,15 @@ void QHttp2ProtocolHandler::handleAuthorization(Stream &stream) // We automatically try to send new requests when the stream is // closed, so we don't need to call sendRequest ourselves. return true; - } // else: Authentication failed or was cancelled + } // else: we're just not resending the request. + // @note In the http/1.x case we (at time of writing) call close() + // for the connectionChannel (which is a bit weird, we could surely + // reuse the open socket outside "connection:close"?), but in http2 + // we only have one channel, so we won't close anything. } else { - // No authentication header, but we got a 401/407 so we cannot - // succeed. We need to emit signals for headers and data, and then - // finishWithError. + // No authentication header or authentication isn't supported, but + // we got a 401/407 so we cannot succeed. We need to emit signals + // for headers and data, and then finishWithError. emit httpReply->headerChanged(); emit httpReply->readyRead(); QNetworkReply::NetworkError error = httpReply->statusCode() == 401 diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index 111de8478a4..74be5afdd7a 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -93,6 +94,8 @@ private slots: void authenticationRequired_data(); void authenticationRequired(); + void unsupportedAuthenticateChallenge(); + void h2cAllowedAttribute_data(); void h2cAllowedAttribute(); @@ -1177,6 +1180,89 @@ void tst_Http2::authenticationRequired() QTRY_VERIFY(serverGotSettingsACK); } +void tst_Http2::unsupportedAuthenticateChallenge() +{ + clearHTTP2State(); + serverPort = 0; + + if (defaultConnectionType() == H2Type::h2c) + QSKIP("This test requires TLS with ALPN to work"); + + ServerPtr targetServer(newServer(defaultServerSettings, defaultConnectionType())); + QByteArray responseBody = "Hello"_ba; + targetServer->setResponseBody(responseBody); + targetServer->setAuthenticationHeader("Bearer realm=\"qt.io accounts\""); + + QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + + QVERIFY(serverPort != 0); + + nRequests = 1; + + QUrl url = requestUrl(defaultConnectionType()); + url.setPath("/index.html"); + QNetworkRequest request(url); + + QByteArray expectedBody = "Hello, World!"; + request.setHeader(QNetworkRequest::ContentTypeHeader, "application/x-www-form-urlencoded"); + QScopedPointer reply; + reply.reset(manager->post(request, expectedBody)); + + bool authenticationRequested = false; + connect(manager.get(), &QNetworkAccessManager::authenticationRequired, reply.get(), + [&](QNetworkReply *, QAuthenticator *auth) { + authenticationRequested = true; + }); + + bool finishedReceived = false; + connect(reply.get(), &QNetworkReply::finished, reply.get(), + [&]() { finishedReceived = true; }); + bool errorReceived = false; + connect(reply.get(), &QNetworkReply::errorOccurred, reply.get(), + [&]() { errorReceived = true; }); + + QSet receivedDataOnStreams; + connect(targetServer.get(), &Http2Server::receivedDATAFrame, reply.get(), + [&receivedDataOnStreams](quint32 streamID, const QByteArray &body) { + Q_UNUSED(body); + receivedDataOnStreams.insert(streamID); + }); + + // Use queued connection so that the finished signal can be emitted and the + // isFinished property can be set. + connect(reply.get(), &QNetworkReply::errorOccurred, this, + &tst_Http2::replyFinishedWithError, Qt::QueuedConnection); + + // Since we're using self-signed certificates, ignore SSL errors: + reply->ignoreSslErrors(); + + runEventLoop(); + STOP_ON_FAILURE + QVERIFY2(reply->isFinished(), + "The reply should error out if authentication fails, or finish if it succeeds"); + + QCOMPARE(reply->error(), QNetworkReply::AuthenticationRequiredError); + QVERIFY(reply->isFinished()); + QVERIFY(errorReceived); + QVERIFY(finishedReceived); + QCOMPARE(receivedDataOnStreams.size(), 1); + QVERIFY(receivedDataOnStreams.contains(1)); // the original, failed, request + + QVERIFY(!authenticationRequested); + + // We should not have sent any authentication headers to the server, since + // we don't support the challenge. + const QByteArray reqAuthHeader = targetServer->requestAuthorizationHeader(); + QVERIFY(reqAuthHeader.isEmpty()); + + // In the `!success` case we need to wait for the server to emit this or it might cause issues + // in the next test running after this. In the `success` case we anyway expect it to have been + // received. + QTRY_VERIFY(serverGotSettingsACK); + +} + void tst_Http2::h2cAllowedAttribute_data() { QTest::addColumn("h2cAllowed");