Match standard HTTP semantics of inclusive end value in Range and Content-Range headers (#7643)

Co-authored-by: Amaury Chamayou <amchamay@microsoft.com>

Author: eddyashton

include/ccf/http_consts.h

@@ -20,6 +20,7 @@ namespace ccf
       static constexpr auto DIGEST = "digest";
       static constexpr auto HOST = "host";
       static constexpr auto LOCATION = "location";
+      static constexpr auto RANGE = "range";
       static constexpr auto RETRY_AFTER = "retry-after";
       static constexpr auto TRAILER = "trailer";
       static constexpr auto WWW_AUTHENTICATE = "www-authenticate";

src/node/rpc/file_serving_handlers.h

@@ -160,7 +160,8 @@ namespace ccf::node
     size_t range_start = 0;
     size_t range_end = total_size;
     {
-      const auto range_header = ctx.rpc_ctx->get_request_header("range");
+      const auto range_header =
+        ctx.rpc_ctx->get_request_header(ccf::http::headers::RANGE);
       if (range_header.has_value())
       {
         LOG_TRACE_FMT("Parsing range header {}", range_header.value());
@@ -231,10 +232,14 @@ namespace ccf::node
 
           if (!s_range_end.empty())
           {
+            // Range end in header is inclusive, but we prefer to reason about
+            // exclusive range end (ie - one past the end)
+            size_t inclusive_range_end = 0;
+
             // Fully-specified range, like "X-Y"
             {
               const auto [p, ec] = std::from_chars(
-                s_range_end.begin(), s_range_end.end(), range_end);
+                s_range_end.begin(), s_range_end.end(), inclusive_range_end);
               if (ec != std::errc())
               {
                 ctx.rpc_ctx->set_error(
@@ -248,6 +253,8 @@ namespace ccf::node
               }
             }
 
+            range_end = inclusive_range_end + 1;
+
             if (range_end > total_size)
             {
               LOG_DEBUG_FMT(
@@ -333,11 +340,15 @@ namespace ccf::node
       ccf::http::headervalues::contenttype::OCTET_STREAM);
     ctx.rpc_ctx->set_response_body(std::move(contents));
 
+    // Convert back to HTTP-style inclusive range end
+    const auto inclusive_range_end = range_end - 1;
+
     // Partial Content responses describe the current response in
     // Content-Range
     ctx.rpc_ctx->set_response_header(
       ccf::http::headers::CONTENT_RANGE,
-      fmt::format("bytes {}-{}/{}", range_start, range_end, total_size));
+      fmt::format(
+        "bytes {}-{}/{}", range_start, inclusive_range_end, total_size));
   }
 
   static void init_file_serving_handlers(

src/snapshots/fetch.h

@@ -43,7 +43,7 @@ namespace snapshots
   struct ContentRangeHeader
   {
     size_t range_start;
-    size_t range_end;
+    size_t inclusive_range_end;
     size_t total_size;
   };
 
@@ -93,7 +93,7 @@ namespace snapshots
 
     {
       const auto [p, ec] = std::from_chars(
-        range_end.begin(), range_end.end(), parsed_values.range_end);
+        range_end.begin(), range_end.end(), parsed_values.inclusive_range_end);
       if (ec != std::errc())
       {
         throw std::runtime_error(fmt::format(
@@ -115,6 +115,65 @@ namespace snapshots
       }
     }
 
+    {
+      // Use content-length to determine whether the sender used an exclusive or
+      // inclusive range end
+      auto length_it = headers.find(ccf::http::headers::CONTENT_LENGTH);
+      if (length_it == headers.end())
+      {
+        throw std::runtime_error(
+          "Response is missing expected content-length header");
+      }
+
+      size_t content_length = 0;
+
+      {
+        const auto& length_s = length_it->second;
+
+        const auto [p, ec] = std::from_chars(
+          length_s.data(), length_s.data() + length_s.size(), content_length);
+
+        if (ec != std::errc())
+        {
+          throw std::runtime_error(fmt::format(
+            "Could not parse length from content-length header: {}",
+            length_it->second));
+        }
+      }
+
+      const auto range_length =
+        parsed_values.inclusive_range_end - parsed_values.range_start;
+      if (range_length == content_length)
+      {
+        LOG_INFO_FMT(
+          "Server sent an exclusive-end content-range header. "
+          "content-length={}, content-range={}. Adjusting this to local "
+          "inclusive-end representation. This should be a temporary "
+          "mismatch, between 6.x and 7.x nodes in a mixed network",
+          length_it->second,
+          it->second);
+        parsed_values.inclusive_range_end -= 1;
+      }
+      else if (range_length + 1 == content_length)
+      {
+        LOG_DEBUG_FMT(
+          "Server sent an inclusive-end content-range header. "
+          "content-length={}, content-range={}. This is expected for 7.x to "
+          "7.x nodes",
+          length_it->second,
+          it->second);
+      }
+      else
+      {
+        throw std::runtime_error(fmt::format(
+          "content-range ({}, {} bytes) and content-length ({}) headers do not "
+          "agree",
+          it->second,
+          range_length + 1,
+          length_it->second));
+      }
+    }
+
     return parsed_values;
   }
 
@@ -142,6 +201,7 @@ namespace snapshots
       constexpr size_t range_size = 4L * 1024 * 1024;
       size_t range_start = 0;
       size_t range_end = range_size;
+      size_t inclusive_range_end = range_end - 1;
       bool fetched_all = false;
 
       auto process_partial_response =
@@ -160,26 +220,29 @@ namespace snapshots
 
           // The server may give us _less_ than we requested (since they know
           // where the file ends), but should never give us more
-          if (content_range.range_end > range_end)
+          if (content_range.inclusive_range_end > inclusive_range_end)
           {
             throw std::runtime_error(fmt::format(
               "Unexpected range response. Requested bytes {}-{}, received "
               "range ending at {}",
               range_start,
-              range_end,
-              content_range.range_end));
+              inclusive_range_end,
+              content_range.inclusive_range_end));
           }
 
+          const auto content_range_exclusive_range_end =
+            content_range.inclusive_range_end + 1;
+
           const auto range_size =
-            content_range.range_end - content_range.range_start;
+            content_range_exclusive_range_end - content_range.range_start;
           LOG_TRACE_FMT(
             "Received {}-byte chunk from {}. Now have {}/{}",
             range_size,
             request.get_url(),
-            content_range.range_end,
+            content_range_exclusive_range_end,
             content_range.total_size);
 
-          if (content_range.range_end == content_range.total_size)
+          if (content_range_exclusive_range_end == content_range.total_size)
           {
             fetched_all = true;
           }
@@ -188,6 +251,7 @@ namespace snapshots
             // Advance range for next request
             range_start = range_end;
             range_end = range_start + range_size;
+            inclusive_range_end = range_end - 1;
           }
         };
 
@@ -203,7 +267,8 @@ namespace snapshots
 
         ccf::curl::UniqueSlist headers;
         headers.append(
-          "Range", fmt::format("bytes={}-{}", range_start, range_end));
+          ccf::http::headers::RANGE,
+          fmt::format("bytes={}-{}", range_start, inclusive_range_end));
 
         CURLcode curl_response = CURLE_FAILED_INIT;
         long status_code = 0;
@@ -281,7 +346,8 @@ namespace snapshots
       {
         ccf::curl::UniqueSlist headers;
         headers.append(
-          "Range", fmt::format("bytes={}-{}", range_start, range_end));
+          ccf::http::headers::RANGE,
+          fmt::format("bytes={}-{}", range_start, inclusive_range_end));
 
         std::unique_ptr<ccf::curl::CurlRequest> snapshot_range_request;
         CURLcode curl_response = CURLE_OK;

tests/e2e_operations.py

@@ -357,25 +357,37 @@ def do_request(http_verb, *args, **kwargs):
             assert r.headers["accept-ranges"] == "bytes", r.headers
             total_size = int(r.headers["content-length"])
 
+            # Use HTTP-style inclusive range end value
+            range_max = total_size - 1
+
             a = total_size // 3
             b = a * 2
             for start, end in [
                 (0, None),
-                (0, total_size),
+                (0, 0),
+                (0, range_max),
                 (0, a),
                 (a, a),
                 (a, b),
                 (b, b),
-                (b, total_size),
+                (b, range_max),
                 (b, None),
+                (range_max, range_max),
             ]:
                 range_header_value = f"{start}-{'' if end is None else end}"
                 r = do_request(
                     "GET", path, headers={"range": f"bytes={range_header_value}"}
                 )
                 assert r.status_code == http.HTTPStatus.PARTIAL_CONTENT.value, r
+                headers = r.headers
+                implied_end = range_max if end is None else end
+                assert int(headers["content-length"]) == implied_end - start + 1
+                assert (
+                    headers["content-range"]
+                    == f"bytes {start}-{implied_end}/{total_size}"
+                )
 
-                expected = snapshot_data[start:end]
+                expected = snapshot_data[start : (None if end is None else end + 1)]
                 actual = r.body.data()
                 assert (
                     expected == actual