http2: Fix authentication code and race

By attempting to get credentials and potentially emitting error
during header parsing we may not have gotten the DATA frames yet
which would leave us emitting error() and finished() without any
body.

Pick-to: 6.6 6.5 6.2
Change-Id: Ibc5fb78193af80ddabaca2c9e4149bbcac9789a1
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

2 files changed, 78 insertions, 57 deletions

diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp
index 23724cc0f0..464c658193 100644
--- a/src/network/access/qhttp2protocolhandler.cpp
+++ b/src/network/access/qhttp2protocolhandler.cpp
@@ -1122,63 +1122,6 @@ void QHttp2ProtocolHandler::updateStream(Stream &stream, const HPack::HttpHeader
         }
     }
 
-    const auto handleAuth = [&, this](QByteArrayView authField, bool isProxy) -> bool {
-        Q_ASSERT(httpReply);
-        const QByteArrayView auth = authField.trimmed();
-        if (auth.startsWith("Negotiate") || auth.startsWith("NTLM")) {
-            // @todo: We're supposed to fall back to http/1.1:
-            // https://docs.microsoft.com/en-us/iis/get-started/whats-new-in-iis-10/http2-on-iis#when-is-http2-not-supported
-            // "Windows authentication (NTLM/Kerberos/Negotiate) is not supported with HTTP/2.
-            // 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) {
-                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:
-                httpRequest.d->needResendWithCredentials = true;
-                m_channel->h2RequestsToSend.insert(httpRequest.priority(), stream.httpPair);
-                httpReply->d_func()->clearHeaders();
-                // If we have data we were uploading we need to reset it:
-                if (stream.data()) {
-                    stream.data()->reset();
-                    httpReplyPrivate->totallyUploadedData = 0;
-                }
-                return true;
-            } // else: Authentication failed or was cancelled
-        }
-        return false;
-    };
-
-    if (httpReply) {
-        // See Note further down. These statuses would in HTTP/1.1 be handled
-        // by QHttpNetworkConnectionChannel::handleStatus. But because h2 has
-        // multiple streams/requests in a single channel this structure does not
-        // map properly to that function.
-        if (httpReply->statusCode() == 401) {
-            const auto wwwAuth = httpReply->headerField("www-authenticate");
-            if (handleAuth(wwwAuth, false)) {
-                sendRST_STREAM(stream.streamID, CANCEL);
-                markAsReset(stream.streamID);
-                // The stream is finalized and deleted after returning
-                return;
-            } // else: errors handled later
-        } else if (httpReply->statusCode() == 407) {
-            const auto proxyAuth = httpReply->headerField("proxy-authenticate");
-            if (handleAuth(proxyAuth, true)) {
-                sendRST_STREAM(stream.streamID, CANCEL);
-                markAsReset(stream.streamID);
-                // The stream is finalized and deleted after returning
-                return;
-            } // else: errors handled later
-        }
-    }
-
     if (QHttpNetworkReply::isHttpRedirect(statusCode) && httpRequest.isFollowRedirects()) {
         QHttpNetworkConnectionPrivate::ParseRedirectResult result =
                 m_connection->d_func()->parseRedirectResponse(httpReply);
@@ -1253,6 +1196,74 @@ void QHttp2ProtocolHandler::updateStream(Stream &stream, const Frame &frame,
     }
 }
 
+// After calling this function, either the request will be re-sent or
+// the reply will be finishedWithError! Do not emit finished() or similar on the
+// reply after this!
+void QHttp2ProtocolHandler::handleAuthorization(Stream &stream)
+{
+    auto *httpReply = stream.reply();
+    auto *httpReplyPrivate = httpReply->d_func();
+    auto &httpRequest = stream.request();
+
+    Q_ASSERT(httpReply && (httpReply->statusCode() == 401 || httpReply->statusCode() == 407));
+
+    const auto handleAuth = [&, this](QByteArrayView authField, bool isProxy) -> bool {
+        Q_ASSERT(httpReply);
+        const QByteArrayView auth = authField.trimmed();
+        if (auth.startsWith("Negotiate") || auth.startsWith("NTLM")) {
+            // @todo: We're supposed to fall back to http/1.1:
+            // https://docs.microsoft.com/en-us/iis/get-started/whats-new-in-iis-10/http2-on-iis#when-is-http2-not-supported
+            // "Windows authentication (NTLM/Kerberos/Negotiate) is not supported with HTTP/2.
+            // 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) {
+                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:
+                httpRequest.d->needResendWithCredentials = true;
+                m_channel->h2RequestsToSend.insert(httpRequest.priority(), stream.httpPair);
+                httpReply->d_func()->clearHeaders();
+                // If we have data we were uploading we need to reset it:
+                if (stream.data()) {
+                    stream.data()->reset();
+                    httpReplyPrivate->totallyUploadedData = 0;
+                }
+                // 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
+        }
+        return false;
+    };
+
+    // These statuses would in HTTP/1.1 be handled by
+    // QHttpNetworkConnectionChannel::handleStatus. But because h2 has
+    // multiple streams/requests in a single channel this structure does not
+    // map properly to that function.
+    bool authOk = true;
+    switch (httpReply->statusCode()) {
+    case 401:
+        authOk = handleAuth(httpReply->headerField("www-authenticate"), false);
+        break;
+    case 407:
+        authOk = handleAuth(httpReply->headerField("proxy-authenticate"), true);
+        break;
+    default:
+        Q_UNREACHABLE();
+    }
+    if (authOk) {
+        markAsReset(stream.streamID);
+        deleteActiveStream(stream.streamID);
+    } // else: errors handled inside handleAuth
+}
+
+// Called when we have received a frame with the END_STREAM flag set
 void QHttp2ProtocolHandler::finishStream(Stream &stream, Qt::ConnectionType connectionType)
 {
     Q_ASSERT(stream.state == Stream::remoteReserved || stream.reply());
@@ -1260,6 +1271,15 @@ void QHttp2ProtocolHandler::finishStream(Stream &stream, Qt::ConnectionType conn
     stream.state = Stream::closed;
     auto httpReply = stream.reply();
     if (httpReply) {
+        int statusCode = httpReply->statusCode();
+        if (statusCode == 401 || statusCode == 407) {
+            // handleAuthorization will either re-send the request or
+            // finishWithError. In either case we don't want to emit finished
+            // here.
+            handleAuthorization(stream);
+            return;
+        }
+
         httpReply->disconnect(this);
         if (stream.data())
             stream.data()->disconnect(this);
diff --git a/src/network/access/qhttp2protocolhandler_p.h b/src/network/access/qhttp2protocolhandler_p.h
index b75e3eb93e..3b818771a6 100644
--- a/src/network/access/qhttp2protocolhandler_p.h
+++ b/src/network/access/qhttp2protocolhandler_p.h
@@ -94,6 +94,7 @@ private:
 
     bool acceptSetting(Http2::Settings identifier, quint32 newValue);
 
+    void handleAuthorization(Stream &stream);
     void updateStream(Stream &stream, const HPack::HttpHeader &headers,
                       Qt::ConnectionType connectionType = Qt::DirectConnection);
     void updateStream(Stream &stream, const Http2::Frame &dataFrame,