Invalid byte ranges result in 416 responses

src/server/byte_range.cpp

@@ -38,7 +38,7 @@ ByteRange::ByteRange(Kind kind, int64_t first, int64_t last)
 
 ByteRange ByteRange::parse(std::string rangeStr)
 {
-  ByteRange byteRange;
+  ByteRange byteRange(INVALID, 0, INT64_MAX);
   const std::string byteUnitSpec("bytes=");
   if ( kiwix::startsWith(rangeStr, byteUnitSpec) ) {
     rangeStr.erase(0, byteUnitSpec.size());
@@ -53,12 +53,13 @@ ByteRange ByteRange::parse(std::string rangeStr)
         char c;
         if (iss >> c && c=='-') {
           iss >> end; // if this fails, end is not modified, which is OK
-          if (iss.eof())
+          if (iss.eof() && start <= end)
             byteRange = ByteRange(ByteRange::PARSED, start, end);
         }
       }
     }
   }
+
   return byteRange;
 }
 
@@ -67,6 +68,9 @@ ByteRange ByteRange::resolve(int64_t contentSize) const
   if ( kind() == NONE )
     return ByteRange(RESOLVED_FULL_CONTENT, 0, contentSize-1);
 
+  if ( kind() == INVALID )
+    return ByteRange(INVALID, 0, contentSize-1);
+
   const int64_t resolved_first = first() < 0
                                ? std::max(int64_t(0), contentSize + first())
                                : first();

src/server/byte_range.h

@@ -33,6 +33,11 @@ class ByteRange
       // No byte-range was present in the request
       NONE,
 
+      // The value of the Range header is not a valid continuous range.
+      // Note that a valid (according to RFC7233) sequence of byte ranges is
+      // considered invalid in this context.
+      INVALID,
+
       // This byte-range has been parsed from request
       PARSED,
 

src/server/response.cpp

@@ -169,6 +169,20 @@ Response::can_compress(const RequestContext& request) const
       && (m_content.size() > KIWIX_MIN_CONTENT_SIZE_TO_DEFLATE);
 }
 
+MHD_Response*
+Response::create_error_response(const RequestContext& request) const
+{
+  MHD_Response* response = MHD_create_response_from_buffer(0, NULL, MHD_RESPMEM_PERSISTENT);
+  if ( m_returnCode == MHD_HTTP_RANGE_NOT_SATISFIABLE ) {
+    std::ostringstream oss;
+    oss << "bytes */" << m_byteRange.length();
+
+    MHD_add_response_header(response,
+      MHD_HTTP_HEADER_CONTENT_RANGE, oss.str().c_str());
+  }
+  return response;
+}
+
 MHD_Response*
 Response::create_raw_content_mhd_response(const RequestContext& request)
 {
@@ -259,6 +273,9 @@ MHD_Response*
 Response::create_mhd_response(const RequestContext& request)
 {
   switch (m_mode) {
+    case ResponseMode::ERROR:
+      return create_error_response(request);
+
     case ResponseMode::RAW_CONTENT :
       return create_raw_content_mhd_response(request);
 
@@ -275,12 +292,14 @@ int Response::send(const RequestContext& request, MHD_Connection* connection)
 {
   MHD_Response* response = create_mhd_response(request);
 
+  if ( m_mode != ResponseMode::ERROR ) {
     MHD_add_response_header(response, "Access-Control-Allow-Origin", "*");
     MHD_add_response_header(response, MHD_HTTP_HEADER_CACHE_CONTROL,
       m_etag.get_option(ETag::CACHEABLE_ENTITY) ? "max-age=2723040, public" : "no-cache, no-store, must-revalidate");
     const std::string etag = m_etag.get_etag();
     if ( ! etag.empty() )
         MHD_add_response_header(response, MHD_HTTP_HEADER_ETAG, etag.c_str());
+  }
 
   if (m_returnCode == MHD_HTTP_OK && m_byteRange.kind() == ByteRange::RESOLVED_PARTIAL_CONTENT)
     m_returnCode = MHD_HTTP_PARTIAL_CONTENT;
@@ -316,16 +335,17 @@ void Response::set_entry(const Entry& entry, const RequestContext& request) {
   set_mimeType(mimeType);
   set_cacheable();
 
-  // XXX: Which of the Accept-Encoding and Range headers has precedence if both
-  // XXX: are present? Can partial content be compressed?
-  if ( is_compressible_mime_type(mimeType) ) {
+  m_byteRange = request.get_range().resolve(entry.getSize());
+  if ( m_byteRange.kind() == ByteRange::RESOLVED_FULL_CONTENT && is_compressible_mime_type(mimeType) ) {
     zim::Blob raw_content = entry.getBlob();
     const std::string content = string(raw_content.data(), raw_content.size());
 
     set_content(content);
     set_compress(true);
-  } else {
-    m_byteRange = request.get_range().resolve(entry.getSize());
+  } else if ( m_byteRange.kind() == ByteRange::INVALID ) {
+    set_code(MHD_HTTP_RANGE_NOT_SATISFIABLE);
+    set_content("");
+    m_mode = ResponseMode::ERROR;
   }
 }
 

src/server/response.h

@@ -35,6 +35,7 @@ extern "C" {
 namespace kiwix {
 
 enum class ResponseMode {
+  ERROR,
   RAW_CONTENT,
   REDIRECTION,
   ENTRY
@@ -73,6 +74,7 @@ class Response {
 
   private: // functions
     MHD_Response* create_mhd_response(const RequestContext& request);
+    MHD_Response* create_error_response(const RequestContext& request) const;
     MHD_Response* create_raw_content_mhd_response(const RequestContext& request);
     MHD_Response* create_redirection_mhd_response() const;
     MHD_Response* create_entry_mhd_response() const;

test/server.cpp

@@ -458,3 +458,22 @@ TEST_F(ServerTest, ValidSingleRangeByteRangeRequestsAreHandledProperly)
     EXPECT_EQ("bytes", p->get_header_value("Accept-Ranges"));
   }
 }
+
+TEST_F(ServerTest, InvalidAndMultiRangeByteRangeRequestsResultIn416Responses)
+{
+  const char url[] = "/zimfile/I/m/Ray_Charles_classic_piano_pose.jpg";
+
+  const char* invalidRanges[] = {
+    "0-10", "bytes=", "bytes=123", "bytes=-10-20", "bytes=10-20xxx",
+    "bytes=10-0", "bytes=10-20, 30-40"
+  };
+
+  for( const char* range : invalidRanges )
+  {
+    const TestContext ctx{ {"Range", range} };
+    const auto p = zfs1_->GET(url, { {"Range", range } } );
+    EXPECT_EQ(416, p->status) << ctx;
+    EXPECT_TRUE(p->body.empty()) << ctx;
+    EXPECT_EQ("bytes */20077", p->get_header_value("Content-Range")) << ctx;
+  }
+}