fix: properly process If-Range headers in client requests (#8741)

* fix: properly process If-Range headers in client requests

* chore: add autest for range requests

* refactor: only check the presence of Range headers once

Author: midchildan

proxy/http/HttpTransact.cc

@@ -3025,7 +3025,6 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
     s->next_action       = SM_ACTION_INTERNAL_CACHE_NOOP;
     break;
 
-  case HTTP_STATUS_RANGE_NOT_SATISFIABLE:
   // Check if cached response supports Range. If it does, append
   // Range transformation plugin
   // A little misnomer. HTTP_STATUS_RANGE_NOT_SATISFIABLE
@@ -3042,7 +3041,8 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
       // Check if cached response supports Range. If it does, append
       // Range transformation plugin
       // only if the cached response is a 200 OK
-      if (client_response_code == HTTP_STATUS_OK && client_request->presence(MIME_PRESENCE_RANGE)) {
+      if (client_response_code == HTTP_STATUS_OK && client_request->presence(MIME_PRESENCE_RANGE) &&
+          HttpTransactCache::validate_ifrange_header_if_any(client_request, cached_response)) {
         s->state_machine->do_range_setup_if_necessary();
         if (s->range_setup == RANGE_NOT_SATISFIABLE) {
           build_error_response(s, HTTP_STATUS_RANGE_NOT_SATISFIABLE, "Requested Range Not Satisfiable", "default");
@@ -4210,6 +4210,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
           s->cache_info.action = CACHE_DO_DELETE;
           s->next_action       = SM_ACTION_SERVER_READ;
         } else {
+          // No need to worry about If-Range headers because the request isn't conditional
           if (s->hdr_info.client_request.presence(MIME_PRESENCE_RANGE)) {
             s->state_machine->do_range_setup_if_necessary();
             // Check client request range header if we cached a stealed content with cacheable=false
@@ -4250,7 +4251,10 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
           s->cache_info.action = CACHE_DO_UPDATE;
           s->next_action       = SM_ACTION_SERVER_READ;
         } else {
-          if (s->hdr_info.client_request.presence(MIME_PRESENCE_RANGE)) {
+          auto *client_request  = &s->hdr_info.client_request;
+          auto *cached_response = s->cache_info.object_read->response_get();
+          if (client_request->presence(MIME_PRESENCE_RANGE) &&
+              HttpTransactCache::validate_ifrange_header_if_any(client_request, cached_response)) {
             s->state_machine->do_range_setup_if_necessary();
             // Note that even if the Range request is not satisfiable, we
             // update and serve this cache. This will give a 200 response to
@@ -4438,7 +4442,9 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
         ink_assert(s->cache_info.object_read != nullptr);
         s->cache_info.action = CACHE_DO_REPLACE;
 
-        if (s->hdr_info.client_request.presence(MIME_PRESENCE_RANGE)) {
+        auto *client_request = &s->hdr_info.client_request;
+        if (client_request->presence(MIME_PRESENCE_RANGE) &&
+            HttpTransactCache::validate_ifrange_header_if_any(client_request, base_response)) {
           s->state_machine->do_range_setup_if_necessary();
         }
       }
@@ -4450,7 +4456,9 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
         s->cache_info.action = CACHE_DO_NO_ACTION;
       } else {
         s->cache_info.action = CACHE_DO_WRITE;
-        if (s->hdr_info.client_request.presence(MIME_PRESENCE_RANGE)) {
+        auto *client_request = &s->hdr_info.client_request;
+        if (client_request->presence(MIME_PRESENCE_RANGE) &&
+            HttpTransactCache::validate_ifrange_header_if_any(client_request, base_response)) {
           s->state_machine->do_range_setup_if_necessary();
         }
       }

proxy/http/HttpTransactCache.cc

@@ -1224,14 +1224,16 @@ HttpTransactCache::CalcVariability(const OverridableHttpConfigParams *http_confi
   HTTP_STATUS_PRECONDITION_FAILED is returned if one fails; otherwise,
   the response's status code is returned.
 
-  If the request is a RANGE request with If-range,
-  HTTP_STATUS_RANGE_NOT_SATISFIABLE is returned if the If-range condition
-  is not satisfied (or fails); that means the document is changed and
-  the whole document should be returned with 200 status code. Otherwise,
-  the response's status code is returned.
+  If the request is a RANGE request with If-range, the response's
+  status code is returned. This is because in the case of If-range
+  headers, it's not so useful to perform checks on it at the same time
+  as other conditional headers. Upon encountering an invalid If-range
+  header, the server must ignore the RANGE and If-range headers
+  entirely. As such, it's more appropriate to validate If-range
+  headers when processing RANGE headers. On other occasions, it's
+  easier to treat If-range requests as plain non-conditional ones.
 
-  @return status code: HTTP_STATUS_NOT_MODIFIED,
-    HTTP_STATUS_PRECONDITION_FAILED, or HTTP_STATUS_RANGE_NOT_SATISFIABLE.
+  @return status code: HTTP_STATUS_NOT_MODIFIED or HTTP_STATUS_PRECONDITION_FAILED
 
 */
 HTTPStatus
@@ -1244,7 +1246,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
   ink_assert(response->status_get() != HTTP_STATUS_RANGE_NOT_SATISFIABLE);
 
   if (!(request->presence(MIME_PRESENCE_IF_MODIFIED_SINCE | MIME_PRESENCE_IF_NONE_MATCH | MIME_PRESENCE_IF_UNMODIFIED_SINCE |
-                          MIME_PRESENCE_IF_MATCH | MIME_PRESENCE_RANGE))) {
+                          MIME_PRESENCE_IF_MATCH))) {
     return response->status_get();
   }
 
@@ -1357,47 +1359,50 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
     return response_code;
   }
 
-  // Handling If-Range header:
-  // As Range && If-Range don't occur often, we want to put the
-  // If-Range code in the end
-  if (request->presence(MIME_PRESENCE_RANGE) && request->presence(MIME_PRESENCE_IF_RANGE)) {
-    int raw_len, comma_sep_list_len;
+  return response->status_get();
+}
 
-    const char *if_value = request->value_get(MIME_FIELD_IF_RANGE, MIME_LEN_IF_RANGE, &comma_sep_list_len);
+/**
+  Validates the contents of If-range headers in client requests. The presence of Range
+  headers needs to be checked before calling this method.
 
-    // this is an ETag, similar to If-Match
-    if (!if_value || if_value[0] == '"' || (comma_sep_list_len > 1 && if_value[1] == '/')) {
-      if (!if_value) {
-        if_value           = "";
-        comma_sep_list_len = 0;
-      }
+  @return Whether the condition specified by If-range is met, if there is any.
+    If there's no If-range header, then true.
 
-      const char *raw_etags = response->value_get(MIME_FIELD_ETAG, MIME_LEN_ETAG, &raw_len);
+*/
+bool
+HttpTransactCache::validate_ifrange_header_if_any(HTTPHdr *request, HTTPHdr *response)
+{
+  if (!request->presence(MIME_PRESENCE_IF_RANGE)) {
+    return true;
+  }
 
-      if (!raw_etags) {
-        raw_etags = "";
-        raw_len   = 0;
-      }
+  int raw_len, comma_sep_list_len;
 
-      if (do_strings_match_strongly(raw_etags, raw_len, if_value, comma_sep_list_len)) {
-        return response->status_get();
-      } else {
-        return HTTP_STATUS_RANGE_NOT_SATISFIABLE;
-      }
+  const char *if_value = request->value_get(MIME_FIELD_IF_RANGE, MIME_LEN_IF_RANGE, &comma_sep_list_len);
+
+  // this is an ETag, similar to If-Match
+  if (!if_value || if_value[0] == '"' || (comma_sep_list_len > 1 && if_value[1] == '/')) {
+    if (!if_value) {
+      if_value           = "";
+      comma_sep_list_len = 0;
     }
-    // this a Date, similar to If-Unmodified-Since but must be an exact match
-    else {
-      // lm_value is zero if Last-modified not exists
-      ink_time_t lm_value = response->get_last_modified();
 
-      // condition fails if Last-modified not exists
-      if ((request->get_if_range_date() != lm_value) || (lm_value == 0)) {
-        return HTTP_STATUS_RANGE_NOT_SATISFIABLE;
-      } else {
-        return response->status_get();
-      }
+    const char *raw_etags = response->value_get(MIME_FIELD_ETAG, MIME_LEN_ETAG, &raw_len);
+
+    if (!raw_etags) {
+      raw_etags = "";
+      raw_len   = 0;
     }
+
+    return do_strings_match_strongly(raw_etags, raw_len, if_value, comma_sep_list_len);
   }
 
-  return response->status_get();
+  // this a Date, similar to If-Unmodified-Since but must be an exact match
+
+  // lm_value is zero if Last-modified not exists
+  ink_time_t lm_value = response->get_last_modified();
+
+  // condition fails if Last-modified not exists
+  return (request->get_if_range_date() == lm_value) && (lm_value != 0);
 }

proxy/http/HttpTransactCache.h

@@ -80,4 +80,6 @@ class HttpTransactCache
 
   static HTTPStatus match_response_to_request_conditionals(HTTPHdr *ua_request, HTTPHdr *c_response,
                                                            ink_time_t response_received_time);
+
+  static bool validate_ifrange_header_if_any(HTTPHdr *ua_request, HTTPHdr *c_response);
 };

tests/gold_tests/headers/gold/range-200.gold

@@ -0,0 +1,11 @@
+HTTP/1.1 200 OK
+Server: ATS/``
+Cache-Control: max-age=``
+Last-Modified: Thu, 10 Feb 2022 00:00:00 GMT
+ETag: range
+Content-Length: 11
+Date: ``
+Age: ``
+Connection: keep-alive
+
+0123456789

tests/gold_tests/headers/gold/range-206-revalidated.gold

@@ -0,0 +1,12 @@
+HTTP/1.1 206 Partial Content
+Server: ATS/``
+Cache-Control: max-age=1
+Last-Modified: Thu, 10 Feb 2022 00:00:00 GMT
+Content-Length: 5
+Date: ``
+Etag: range
+Age: ``
+Content-Range: bytes 1-5/11
+Connection: keep-alive
+
+12345``

tests/gold_tests/headers/gold/range-206.gold

@@ -0,0 +1,12 @@
+HTTP/1.1 206 Partial Content
+Server: ATS/``
+Cache-Control: max-age=300
+Last-Modified: Thu, 10 Feb 2022 00:00:00 GMT
+ETag: range
+Content-Length: 5
+Date: ``
+Age: ``
+Content-Range: bytes 1-5/11
+Connection: keep-alive
+
+12345``

tests/gold_tests/headers/gold/range-416.gold

@@ -0,0 +1,6 @@
+HTTP/1.1 416 Requested Range Not Satisfiable
+Date: ``
+Connection: keep-alive
+Server: ATS/``
+Cache-Control: no-store
+``