Merge pull request #21023 from Karlson2k/matrix-MHD_fix_03

[WEBSERVER][Backport] Fixed duplicated `Content-Length:` headers

4 files changed, 125 insertions, 136 deletions

diff --git a/xbmc/network/WebServer.cpp b/xbmc/network/WebServer.cpp
index c9eae2269a..a4b6d818c9 100644
--- a/xbmc/network/WebServer.cpp
+++ b/xbmc/network/WebServer.cpp
@@ -209,7 +209,7 @@ MHD_RESULT CWebServer::HandlePartialRequest(struct MHD_Connection* connection,
     if (handler != nullptr)
     {
       // if we got a GET request we need to check if it should be cached
-      if (request.method == GET)
+      if (request.method == GET || request.method == HEAD)
       {
         if (handler->CanBeCached())
         {
@@ -441,11 +441,6 @@ MHD_RESULT CWebServer::FinalizeRequest(const std::shared_ptr<IHTTPRequestHandler
   else
     handler->AddResponseHeader(MHD_HTTP_HEADER_ACCEPT_RANGES, "none");
 
-  // add MHD_HTTP_HEADER_CONTENT_LENGTH
-  if (responseDetails.totalLength > 0)
-    handler->AddResponseHeader(MHD_HTTP_HEADER_CONTENT_LENGTH,
-                               StringUtils::Format("{}", responseDetails.totalLength));
-
   // add all headers set by the request handler
   for (const auto& it : responseDetails.headers)
     AddHeader(response, it.first, it.second);
@@ -597,7 +592,9 @@ bool CWebServer::ProcessPostData(const HTTPRequest& request,
     if (!postDataHandled)
     {
       m_logger->error("failed to handle HTTP POST data for {}", request.pathUrl);
-#if (MHD_VERSION >= 0x00095213)
+#if (MHD_VERSION >= 0x00097400)
+      connectionHandler->errorStatus = MHD_HTTP_CONTENT_TOO_LARGE;
+#elif (MHD_VERSION >= 0x00095213)
       connectionHandler->errorStatus = MHD_HTTP_PAYLOAD_TOO_LARGE;
 #else
       connectionHandler->errorStatus = MHD_HTTP_REQUEST_ENTITY_TOO_LARGE;
@@ -825,113 +822,96 @@ MHD_RESULT CWebServer::CreateFileDownloadResponse(
     mimeType = CreateMimeTypeFromExtension(ext.c_str());
   }
 
-  if (request.method != HEAD)
-  {
-    uint64_t totalLength = 0;
-    std::unique_ptr<HttpFileDownloadContext> context(new HttpFileDownloadContext());
-    context->file = file;
-    context->contentType = mimeType;
-    context->boundaryWritten = false;
-    context->writePosition = 0;
-
-    if (handler->IsRequestRanged())
-    {
-      if (!request.ranges.IsEmpty())
-        context->ranges = request.ranges;
-      else
-        HTTPRequestHandlerUtils::GetRequestedRanges(request.connection, fileLength,
-                                                    context->ranges);
-    }
+  uint64_t totalLength = 0;
+  std::unique_ptr<HttpFileDownloadContext> context(new HttpFileDownloadContext());
+  context->file = file;
+  context->contentType = mimeType;
+  context->boundaryWritten = false;
+  context->writePosition = 0;
 
-    uint64_t firstPosition = 0;
-    uint64_t lastPosition = 0;
-    // if there are no ranges, add the whole range
-    if (context->ranges.IsEmpty())
-      context->ranges.Add(CHttpRange(0, fileLength - 1));
+  if (handler->IsRequestRanged())
+  {
+    if (!request.ranges.IsEmpty())
+      context->ranges = request.ranges;
     else
-    {
-      handler->SetResponseStatus(MHD_HTTP_PARTIAL_CONTENT);
-
-      // we need to remember that we are ranged because the range length might change and won't be
-      // reliable anymore for length comparisons
-      ranged = true;
+      HTTPRequestHandlerUtils::GetRequestedRanges(request.connection, fileLength, context->ranges);
+  }
 
-      context->ranges.GetFirstPosition(firstPosition);
-      context->ranges.GetLastPosition(lastPosition);
-    }
+  uint64_t firstPosition = 0;
+  uint64_t lastPosition = 0;
+  // if there are no ranges, add the whole range
+  if (context->ranges.IsEmpty())
+    context->ranges.Add(CHttpRange(0, fileLength - 1));
+  else
+  {
+    handler->SetResponseStatus(MHD_HTTP_PARTIAL_CONTENT);
 
-    // remember the total number of ranges
-    context->rangeCountTotal = context->ranges.Size();
-    // remember the total length
-    totalLength = context->ranges.GetLength();
+    // we need to remember that we are ranged because the range length might change and won't be
+    // reliable anymore for length comparisons
+    ranged = true;
 
-    // adjust the MIME type and range length in case of multiple ranges which requires multipart
-    // boundaries
-    if (context->rangeCountTotal > 1)
-    {
-      context->boundary = HttpRangeUtils::GenerateMultipartBoundary();
-      mimeType = HttpRangeUtils::GenerateMultipartBoundaryContentType(context->boundary);
-
-      // build part of the boundary with the optional Content-Type header
-      // "--<boundary>\r\nContent-Type: <content-type>\r\n
-      context->boundaryWithHeader = HttpRangeUtils::GenerateMultipartBoundaryWithHeader(
-          context->boundary, context->contentType);
-      context->boundaryEnd = HttpRangeUtils::GenerateMultipartBoundaryEnd(context->boundary);
-
-      // for every range, we need to add a boundary with header
-      for (HttpRanges::const_iterator range = context->ranges.Begin();
-           range != context->ranges.End(); ++range)
-      {
-        // we need to temporarily add the Content-Range header to the boundary to be able to
-        // determine the length
-        std::string completeBoundaryWithHeader =
-            HttpRangeUtils::GenerateMultipartBoundaryWithHeader(context->boundaryWithHeader,
-                                                                &*range);
-        totalLength += completeBoundaryWithHeader.size();
-
-        // add a newline before any new multipart boundary
-        if (range != context->ranges.Begin())
-          totalLength += strlen(HEADER_NEWLINE);
-      }
-      // and at the very end a special end-boundary "\r\n--<boundary>--"
-      totalLength += context->boundaryEnd.size();
-    }
+    context->ranges.GetFirstPosition(firstPosition);
+    context->ranges.GetLastPosition(lastPosition);
+  }
 
-    // set the initial write position
-    context->ranges.GetFirstPosition(context->writePosition);
+  // remember the total number of ranges
+  context->rangeCountTotal = context->ranges.Size();
+  // remember the total length
+  totalLength = context->ranges.GetLength();
 
-    // create the response object
-    response =
-        MHD_create_response_from_callback(totalLength, 2048, &CWebServer::ContentReaderCallback,
-                                          context.get(), &CWebServer::ContentReaderFreeCallback);
-    if (response == nullptr)
+  // adjust the MIME type and range length in case of multiple ranges which requires multipart
+  // boundaries
+  if (context->rangeCountTotal > 1)
+  {
+    context->boundary = HttpRangeUtils::GenerateMultipartBoundary();
+    mimeType = HttpRangeUtils::GenerateMultipartBoundaryContentType(context->boundary);
+
+    // build part of the boundary with the optional Content-Type header
+    // "--<boundary>\r\nContent-Type: <content-type>\r\n
+    context->boundaryWithHeader = HttpRangeUtils::GenerateMultipartBoundaryWithHeader(
+        context->boundary, context->contentType);
+    context->boundaryEnd = HttpRangeUtils::GenerateMultipartBoundaryEnd(context->boundary);
+
+    // for every range, we need to add a boundary with header
+    for (HttpRanges::const_iterator range = context->ranges.Begin(); range != context->ranges.End();
+         ++range)
     {
-      m_logger->error("failed to create a HTTP response for {} to be filled from{}",
-                      request.pathUrl, filePath);
-      return MHD_NO;
+      // we need to temporarily add the Content-Range header to the boundary to be able to
+      // determine the length
+      std::string completeBoundaryWithHeader =
+          HttpRangeUtils::GenerateMultipartBoundaryWithHeader(context->boundaryWithHeader, &*range);
+      totalLength += completeBoundaryWithHeader.size();
+
+      // add a newline before any new multipart boundary
+      if (range != context->ranges.Begin())
+        totalLength += strlen(HEADER_NEWLINE);
     }
+    // and at the very end a special end-boundary "\r\n--<boundary>--"
+    totalLength += context->boundaryEnd.size();
+  }
 
-    context.release(); // ownership was passed to mhd
+  // set the initial write position
+  context->ranges.GetFirstPosition(context->writePosition);
 
-    // add Content-Range header
-    if (ranged)
-      handler->AddResponseHeader(
-          MHD_HTTP_HEADER_CONTENT_RANGE,
-          HttpRangeUtils::GenerateContentRangeHeaderValue(firstPosition, lastPosition, fileLength));
-  }
-  else
+  // create the response object
+  response =
+      MHD_create_response_from_callback(totalLength, 2048, &CWebServer::ContentReaderCallback,
+                                        context.get(), &CWebServer::ContentReaderFreeCallback);
+  if (response == nullptr)
   {
-    response = create_response(0, nullptr, MHD_NO, MHD_NO);
-    if (response == nullptr)
-    {
-      m_logger->error("failed to create a HTTP HEAD response for {}", request.pathUrl);
-      return MHD_NO;
-    }
-
-    handler->AddResponseHeader(MHD_HTTP_HEADER_CONTENT_LENGTH,
-                               StringUtils::Format("{}", fileLength));
+    m_logger->error("failed to create a HTTP response for {} to be filled from{}", request.pathUrl,
+                    filePath);
+    return MHD_NO;
   }
 
+  context.release(); // ownership was passed to mhd
+
+  // add Content-Range header
+  if (ranged)
+    handler->AddResponseHeader(
+        MHD_HTTP_HEADER_CONTENT_RANGE,
+        HttpRangeUtils::GenerateContentRangeHeaderValue(firstPosition, lastPosition, fileLength));
+
   // set the Content-Type header
   if (!mimeType.empty())
     handler->AddResponseHeader(MHD_HTTP_HEADER_CONTENT_TYPE, mimeType);
@@ -947,20 +927,17 @@ MHD_RESULT CWebServer::CreateErrorResponse(struct MHD_Connection* connection,
   size_t payloadSize = 0;
   const void* payload = nullptr;
 
-  if (method != HEAD)
+  switch (responseType)
   {
-    switch (responseType)
-    {
-      case MHD_HTTP_NOT_FOUND:
-        payloadSize = strlen(PAGE_FILE_NOT_FOUND);
-        payload = (const void*)PAGE_FILE_NOT_FOUND;
-        break;
-
-      case MHD_HTTP_NOT_IMPLEMENTED:
-        payloadSize = strlen(NOT_SUPPORTED);
-        payload = (const void*)NOT_SUPPORTED;
-        break;
-    }
+    case MHD_HTTP_NOT_FOUND:
+      payloadSize = strlen(PAGE_FILE_NOT_FOUND);
+      payload = (const void*)PAGE_FILE_NOT_FOUND;
+      break;
+
+    case MHD_HTTP_NOT_IMPLEMENTED:
+      payloadSize = strlen(NOT_SUPPORTED);
+      payload = (const void*)NOT_SUPPORTED;
+      break;
   }
 
   response = create_response(payloadSize, payload, MHD_NO, MHD_NO);
@@ -1244,11 +1221,11 @@ struct MHD_Daemon* CWebServer::StartMHD(unsigned int flags, int port)
             | MHD_USE_SSL,
         port, 0, 0, &CWebServer::AnswerToConnection, this,
 
-        MHD_OPTION_CONNECTION_LIMIT, 512, MHD_OPTION_CONNECTION_TIMEOUT, timeout,
-        MHD_OPTION_URI_LOG_CALLBACK, &CWebServer::UriRequestLogger, this,
-        MHD_OPTION_EXTERNAL_LOGGER, &logFromMHD, 0, MHD_OPTION_THREAD_STACK_SIZE,
-        m_thread_stacksize, MHD_OPTION_HTTPS_MEM_KEY, m_key.c_str(), MHD_OPTION_HTTPS_MEM_CERT,
-        m_cert.c_str(), MHD_OPTION_HTTPS_PRIORITIES, ciphers, MHD_OPTION_END);
+        MHD_OPTION_EXTERNAL_LOGGER, &logFromMHD, 0, MHD_OPTION_CONNECTION_LIMIT, 512,
+        MHD_OPTION_CONNECTION_TIMEOUT, timeout, MHD_OPTION_URI_LOG_CALLBACK,
+        &CWebServer::UriRequestLogger, this, MHD_OPTION_THREAD_STACK_SIZE, m_thread_stacksize,
+        MHD_OPTION_HTTPS_MEM_KEY, m_key.c_str(), MHD_OPTION_HTTPS_MEM_CERT, m_cert.c_str(),
+        MHD_OPTION_HTTPS_PRIORITIES, ciphers, MHD_OPTION_END);
 
   // No SSL
   return MHD_start_daemon(
@@ -1265,9 +1242,10 @@ struct MHD_Daemon* CWebServer::StartMHD(unsigned int flags, int port)
       ,
       port, 0, 0, &CWebServer::AnswerToConnection, this,
 
-      MHD_OPTION_CONNECTION_LIMIT, 512, MHD_OPTION_CONNECTION_TIMEOUT, timeout,
-      MHD_OPTION_URI_LOG_CALLBACK, &CWebServer::UriRequestLogger, this, MHD_OPTION_EXTERNAL_LOGGER,
-      &logFromMHD, 0, MHD_OPTION_THREAD_STACK_SIZE, m_thread_stacksize, MHD_OPTION_END);
+      MHD_OPTION_EXTERNAL_LOGGER, &logFromMHD, 0, MHD_OPTION_CONNECTION_LIMIT, 512,
+      MHD_OPTION_CONNECTION_TIMEOUT, timeout, MHD_OPTION_URI_LOG_CALLBACK,
+      &CWebServer::UriRequestLogger, this, MHD_OPTION_THREAD_STACK_SIZE, m_thread_stacksize,
+      MHD_OPTION_END);
 }
 
 bool CWebServer::Start(uint16_t port, const std::string& username, const std::string& password)
@@ -1424,12 +1402,9 @@ MHD_RESULT CWebServer::AddHeader(struct MHD_Response* response,
   if (CServiceBroker::GetLogging().CanLogComponent(LOGWEBSERVER))
     m_logger->debug("[OUT] {}: {}", name, value);
 
-#if MHD_VERSION >= 0x00096800
   if (name == MHD_HTTP_HEADER_CONTENT_LENGTH)
-  {
-    MHD_set_response_options(response, MHD_RF_INSANITY_HEADER_CONTENT_LENGTH, MHD_RO_END);
-  }
-#endif
+    m_logger->warn("Attempt to override MHD automatic \"Content-Length\" header");
+
   return MHD_add_response_header(response, name.c_str(), value.c_str());
 }
 
diff --git a/xbmc/network/httprequesthandler/HTTPImageTransformationHandler.cpp b/xbmc/network/httprequesthandler/HTTPImageTransformationHandler.cpp
index 5b6783b7dd..3d2dccb275 100644
--- a/xbmc/network/httprequesthandler/HTTPImageTransformationHandler.cpp
+++ b/xbmc/network/httprequesthandler/HTTPImageTransformationHandler.cpp
@@ -110,15 +110,6 @@ MHD_RESULT CHTTPImageTransformationHandler::HandleRequest()
   if (m_response.type == HTTPError)
     return MHD_YES;
 
-  // nothing else to do if this is a HEAD request
-  if (m_request.method == HEAD)
-  {
-    m_response.status = MHD_HTTP_OK;
-    m_response.type = HTTPMemoryDownloadNoFreeNoCopy;
-
-    return MHD_YES;
-  }
-
   // get the transformation options
   std::map<std::string, std::string> options;
   HTTPRequestHandlerUtils::GetRequestHeaderValues(m_request.connection, MHD_GET_ARGUMENT_KIND, options);
diff --git a/xbmc/network/httprequesthandler/HTTPJsonRpcHandler.cpp b/xbmc/network/httprequesthandler/HTTPJsonRpcHandler.cpp
index 5e6d72d89b..9c67c514b7 100644
--- a/xbmc/network/httprequesthandler/HTTPJsonRpcHandler.cpp
+++ b/xbmc/network/httprequesthandler/HTTPJsonRpcHandler.cpp
@@ -52,7 +52,7 @@ MHD_RESULT CHTTPJsonRpcHandler::HandleRequest()
 
     isRequest = true;
   }
-  else if (m_request.method == GET)
+  else if (m_request.method == GET || m_request.method == HEAD)
   {
     std::map<std::string, std::string>::const_iterator argument = arguments.find("request");
     if (argument != arguments.end() && !argument->second.empty())
diff --git a/xbmc/network/test/TestWebServer.cpp b/xbmc/network/test/TestWebServer.cpp
index a87d9f4ba9..bd9c34b3e9 100644
--- a/xbmc/network/test/TestWebServer.cpp
+++ b/xbmc/network/test/TestWebServer.cpp
@@ -152,6 +152,8 @@ protected:
 
     // Content-Type must be "text/html"
     EXPECT_STREQ("text/html", httpHeader.GetMimeType().c_str());
+    // Must be only one "Content-Length" header
+    ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
     // Content-Length must be "4"
     EXPECT_STREQ("4", httpHeader.GetValue(MHD_HTTP_HEADER_CONTENT_LENGTH).c_str());
     // Accept-Ranges must be "bytes"
@@ -173,6 +175,9 @@ protected:
     // get the HTTP header details
     const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+    // Only zero or one "Content-Length" headers
+    ASSERT_GE(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
+
     // check the protocol line for the expected HTTP status
     std::string httpStatusString = StringUtils::Format(" %d ", httpStatus);
     std::string protocolLine = httpHeader.GetProtoLine();
@@ -184,7 +189,10 @@ protected:
     if (empty)
       EXPECT_STREQ("0", httpHeader.GetValue(MHD_HTTP_HEADER_CONTENT_LENGTH).c_str());
     else
+    {
+      ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
       EXPECT_STREQ("20", httpHeader.GetValue(MHD_HTTP_HEADER_CONTENT_LENGTH).c_str());
+    }
     // Accept-Ranges must be "bytes"
     EXPECT_STREQ("bytes", httpHeader.GetValue(MHD_HTTP_HEADER_ACCEPT_RANGES).c_str());
 
@@ -204,6 +212,9 @@ protected:
     // get the HTTP header details
     const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+    // Only zero or one "Content-Length" headers
+    ASSERT_GE(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
+
     // check the protocol line for the expected HTTP status
     std::string httpStatusString = StringUtils::Format(" %d ", MHD_HTTP_PARTIAL_CONTENT);
     std::string protocolLine = httpHeader.GetProtoLine();
@@ -225,6 +236,7 @@ protected:
     // If there's no range Content-Length must be "20"
     if (ranges.IsEmpty())
     {
+      ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
       EXPECT_STREQ("20", httpHeader.GetValue(MHD_HTTP_HEADER_CONTENT_LENGTH).c_str());
       EXPECT_STREQ(TEST_FILES_DATA_RANGES, result.c_str());
       return;
@@ -250,6 +262,7 @@ protected:
       EXPECT_STREQ(expectedContent.c_str(), result.c_str());
 
       // and Content-Length
+      ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
       EXPECT_STREQ(StringUtils::Format("%u", static_cast<unsigned int>(expectedContent.size())).c_str(), httpHeader.GetValue(MHD_HTTP_HEADER_CONTENT_LENGTH).c_str());
 
       return;
@@ -362,6 +375,8 @@ TEST_F(TestWebServer, CanGetJsonRpcApiDescriptionWithHttpGet)
   // get the HTTP header details
   const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+  // Content-Length header must be present
+  ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
   // Content-Type must be "application/json"
   EXPECT_STREQ("application/json", httpHeader.GetMimeType().c_str());
   // Accept-Ranges must be "none"
@@ -392,6 +407,8 @@ TEST_F(TestWebServer, CanReadDataOverJsonRpcWithHttpGet)
   // get the HTTP header details
   const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+  // Content-Length header must be present
+  ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
   // Content-Type must be "application/json"
   EXPECT_STREQ("application/json", httpHeader.GetMimeType().c_str());
   // Accept-Ranges must be "none"
@@ -429,6 +446,8 @@ TEST_F(TestWebServer, CannotModifyOverJsonRpcWithHttpGet)
   // get the HTTP header details
   const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+  // Content-Length header must be present
+  ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
   // Content-Type must be "application/json"
   EXPECT_STREQ("application/json", httpHeader.GetMimeType().c_str());
   // Accept-Ranges must be "none"
@@ -463,6 +482,8 @@ TEST_F(TestWebServer, CanReadDataOverJsonRpcWithHttpPost)
   // get the HTTP header details
   const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+  // Content-Length header must be present
+  ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
   // Content-Type must be "application/json"
   EXPECT_STREQ("application/json", httpHeader.GetMimeType().c_str());
   // Accept-Ranges must be "none"
@@ -500,6 +521,8 @@ TEST_F(TestWebServer, CanModifyOverJsonRpcWithHttpPost)
   // get the HTTP header details
   const CHttpHeader& httpHeader = curl.GetHttpHeader();
 
+  // Content-Length header must be present
+  ASSERT_EQ(1U, httpHeader.GetValues(MHD_HTTP_HEADER_CONTENT_LENGTH).size());
   // Content-Type must be "application/json"
   EXPECT_STREQ("application/json", httpHeader.GetMimeType().c_str());
   // Accept-Ranges must be "none"