diff --git a/src/server/byte_range.cpp b/src/server/byte_range.cpp index e00238af6..3102f0745 100644 --- a/src/server/byte_range.cpp +++ b/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(); diff --git a/src/server/byte_range.h b/src/server/byte_range.h index c2c15af7e..e302cece4 100644 --- a/src/server/byte_range.h +++ b/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, diff --git a/src/server/response.cpp b/src/server/response.cpp index b0811f63f..aafe1336b 100644 --- a/src/server/response.cpp +++ b/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); - 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_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; } } diff --git a/src/server/response.h b/src/server/response.h index 9fba5a191..9ef0d9486 100644 --- a/src/server/response.h +++ b/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; diff --git a/test/server.cpp b/test/server.cpp index 9d41ebb69..22a60490a 100644 --- a/test/server.cpp +++ b/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; + } +}