Browse Source

Cover the non-206 Range paths and drop the duplicated test route

The added test only exercised a 403 with a single range, leaving three
paths that were equally broken untested: a suffix range, whose first_pos
is still -1 when the bounds asserts are compiled out under NDEBUG; two
ranges, where apply_ranges() makes no boundary for a non-206 response and
the multipart branch wrote an empty one; and a 2xx that is not a 206,
which was validated but still announced the full content length.

Fold the copied route back into /streamed-with-range with a status query
parameter, following the ?error idiom already used there.
yhirose 1 week ago
parent
commit
5539a66c63
2 changed files with 49 additions and 44 deletions
  1. 7 8
      httplib.h
  2. 42 36
      test/test.cc

+ 7 - 8
httplib.h

@@ -11845,16 +11845,15 @@ Server::write_content_with_provider(Stream &strm, const Request &req,
   };
 
   if (res.content_length_ > 0) {
-    // Only a 206 response is served as a partial representation. That is the
-    // condition `apply_ranges()` used when it decided the Content-Length and
-    // the multipart boundary, and `detail::range_error()` only validates
-    // `req.ranges` against the content length for a 2xx status. Honouring the
-    // ranges here under any other status would write a body that disagrees
-    // with the header we already sent, from an offset nothing has checked.
-    auto partial =
+    // Only a 206 response is served as a partial representation, matching the
+    // condition `apply_ranges()` used to decide the Content-Length and the
+    // multipart boundary. Since `detail::range_error()` validates `req.ranges`
+    // only for a 2xx status, slicing under any other status would write a body
+    // that disagrees with the header already sent, from an unchecked offset.
+    auto is_partial =
         !req.ranges.empty() && res.status == StatusCode::PartialContent_206;
 
-    if (!partial) {
+    if (!is_partial) {
       return detail::write_content(strm, res.content_provider_, 0,
                                    res.content_length_, is_shutting_down);
     } else if (req.ranges.size() == 1) {

+ 42 - 36
test/test.cc

@@ -4540,6 +4540,15 @@ protected:
         .Get("/streamed-with-range",
              [&](const Request &req, Response &res) {
                auto data = new std::string("abcdefg");
+               // An explicit status keeps the server from picking the 206 it
+               // would otherwise choose for a ranged request, so the response
+               // is not a partial representation.
+               auto status = req.get_param_value("status");
+               if (status == "200") {
+                 res.status = StatusCode::OK_200;
+               } else if (status == "403") {
+                 res.status = StatusCode::Forbidden_403;
+               }
                res.set_content_provider(
                    data->size(), "text/plain",
                    [data](size_t offset, size_t length, DataSink &sink) {
@@ -4557,24 +4566,6 @@ protected:
                      delete data;
                    });
              })
-        .Get("/streamed-with-range-and-error-status",
-             [&](const Request & /*req*/, Response &res) {
-               auto data = new std::string("abcdefg");
-               res.status = StatusCode::Forbidden_403;
-               res.set_content_provider(
-                   data->size(), "text/plain",
-                   [data](size_t offset, size_t length, DataSink &sink) {
-                     size_t DATA_CHUNK_SIZE = 4;
-                     const auto &d = *data;
-                     auto out_len =
-                         std::min(static_cast<size_t>(length), DATA_CHUNK_SIZE);
-                     auto ret =
-                         sink.write(&d[static_cast<size_t>(offset)], out_len);
-                     EXPECT_TRUE(ret);
-                     return true;
-                   },
-                   [data](bool /*success*/) { delete data; });
-             })
         .Get("/streamed-cancel",
              [&](const Request & /*req*/, Response &res) {
                res.set_content_provider(
@@ -6088,25 +6079,40 @@ TEST_F(ServerTest, GetStreamedWithRangeSuffix2) {
   EXPECT_EQ(0U, res->body.size());
 }
 
-TEST_F(ServerTest, GetStreamedWithRangeAndErrorStatus) {
-  // The handler answers with a non-2xx status, so `detail::range_error()`
-  // never validated the ranges and `apply_ranges()` reported the full
-  // content length. The body must match that header and the content provider
-  // must not be asked for an offset outside the representation.
-  auto res = cli_.Get("/streamed-with-range-and-error-status",
-                      Headers{{make_range_header({{3, 5}})}});
-  ASSERT_TRUE(res) << "Error: " << to_string(res.error());
-  EXPECT_EQ(StatusCode::Forbidden_403, res->status);
-  EXPECT_EQ("7", res->get_header_value("Content-Length"));
-  EXPECT_EQ(false, res->has_header("Content-Range"));
-  EXPECT_EQ(std::string("abcdefg"), res->body);
+TEST_F(ServerTest, GetStreamedWithRangeAndNonPartialStatus) {
+  // Only a 206 is served as a partial representation. Under any other status
+  // `apply_ranges()` reported the full content length, so the body must match
+  // that header, and the content provider must never be asked for an offset
+  // outside the representation.
+  auto check = [&](int status, const char *range) {
+    auto path =
+        std::string("/streamed-with-range?status=") + std::to_string(status);
+    auto ctx = path + " Range: " + range;
+
+    auto res = cli_.Get(path, Headers{{"Range", range}});
+    ASSERT_TRUE(res) << ctx << " Error: " << to_string(res.error());
+    EXPECT_EQ(status, res->status) << ctx;
+    EXPECT_EQ("7", res->get_header_value("Content-Length")) << ctx;
+    EXPECT_EQ("text/plain", res->get_header_value("Content-Type")) << ctx;
+    EXPECT_FALSE(res->has_header("Content-Range")) << ctx;
+    EXPECT_EQ(std::string("abcdefg"), res->body) << ctx;
+  };
 
-  auto res2 = cli_.Get("/streamed-with-range-and-error-status",
-                       Headers{{"Range", "bytes=100000-100200"}});
-  ASSERT_TRUE(res2) << "Error: " << to_string(res2.error());
-  EXPECT_EQ(StatusCode::Forbidden_403, res2->status);
-  EXPECT_EQ("7", res2->get_header_value("Content-Length"));
-  EXPECT_EQ(std::string("abcdefg"), res2->body);
+  // Non-2xx: `detail::range_error()` never validated these ranges at all.
+  check(403, "bytes=3-5");
+  // The offset is far past the representation.
+  check(403, "bytes=100000-100200");
+  // `first_pos` is still -1 here, and the bounds asserts in
+  // `get_range_offset_and_length()` are compiled out under NDEBUG.
+  check(403, "bytes=-3");
+  // `apply_ranges()` makes no boundary for a non-206 response, so the
+  // multipart branch would have written one that is empty.
+  check(403, "bytes=1-2, 4-5");
+
+  // 2xx but not 206: the ranges were validated, yet the Content-Length still
+  // covers the whole representation.
+  check(200, "bytes=3-5");
+  check(200, "bytes=1-2, 4-5");
 }
 
 TEST_F(ServerTest, GetStreamedWithRangeError) {