diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 9f660d4207c..e1d79f40a01 100644 --- a/proxy/http/HttpTransact.cc +++ b/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(); } } diff --git a/proxy/http/HttpTransactCache.cc b/proxy/http/HttpTransactCache.cc index 622f8017a22..48b3282e601 100644 --- a/proxy/http/HttpTransactCache.cc +++ b/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); } diff --git a/proxy/http/HttpTransactCache.h b/proxy/http/HttpTransactCache.h index eba10037a85..09243fd697f 100644 --- a/proxy/http/HttpTransactCache.h +++ b/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); }; diff --git a/tests/gold_tests/headers/gold/range-200.gold b/tests/gold_tests/headers/gold/range-200.gold new file mode 100644 index 00000000000..0aa6ac90c5e --- /dev/null +++ b/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 diff --git a/tests/gold_tests/headers/gold/range-206-revalidated.gold b/tests/gold_tests/headers/gold/range-206-revalidated.gold new file mode 100644 index 00000000000..e918cc03892 --- /dev/null +++ b/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`` diff --git a/tests/gold_tests/headers/gold/range-206.gold b/tests/gold_tests/headers/gold/range-206.gold new file mode 100644 index 00000000000..8caff80aa1b --- /dev/null +++ b/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`` diff --git a/tests/gold_tests/headers/gold/range-416.gold b/tests/gold_tests/headers/gold/range-416.gold new file mode 100644 index 00000000000..1a87e708d11 --- /dev/null +++ b/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 +`` diff --git a/tests/gold_tests/headers/range.test.py b/tests/gold_tests/headers/range.test.py new file mode 100644 index 00000000000..b2cc3e4a124 --- /dev/null +++ b/tests/gold_tests/headers/range.test.py @@ -0,0 +1,229 @@ +""" +""" +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = """ +Test range requests +""" + +Test.ContinueOnFail = True + +CACHE_SHORT_MAXAGE = 1 + + +def register(microserver, request_hdr, request_body, response_hdr, response_body): + request = { + "headers": "{}\r\n\r\n".format( + "\r\n".join(line for line in request_hdr.split("\n") if line) + ), + "timestamp": "1469733493.993", + "body": request_body, + } + response = { + "headers": "{}\r\n\r\n".format( + "\r\n".join(line for line in response_hdr.split("\n") if line) + ), + "timestamp": "1469733493.993", + "body": response_body, + } + microserver.addResponse("sessionlog.json", request, response) + + +def curl_whole(ts, path=""): + return f"curl -sSv -D - http://127.0.0.1:{ts.Variables.port}/{path}" + + +def curl_range(ts, path="", ifrange=None, start=1, end=5): + opt = f"-H 'If-Range: {ifrange}'" if ifrange else "" + return f"curl -sSv -D - {opt} -H 'Range: bytes={start}-{end}' http://127.0.0.1:{ts.Variables.port}/{path}" + + +# ---- +# Setup Origin Server +# ---- +microserver = Test.MakeOriginServer("microserver") + +request_hdr = """ +GET / HTTP/1.1 +Host: 127.0.0.1 +""" + +response_hdr = """ +HTTP/1.1 200 OK +Server: microserver +Connection: close +Cache-Control: max-age=300 +Last-Modified: Thu, 10 Feb 2022 00:00:00 GMT +ETag: range +""" + +short_cache_request_hdr = """ +GET /short HTTP/1.1 +Host: 127.0.0.1 +""" + +short_cache_response_hdr = f""" +HTTP/1.1 200 OK +Server: microserver +Connection: close +Cache-Control: max-age={CACHE_SHORT_MAXAGE} +Last-Modified: Thu, 10 Feb 2022 00:00:00 GMT +ETag: range +""" + +response_body = f"{''.join(str(i) for i in range(10))}\n" + +register(microserver, request_hdr, "", response_hdr, response_body) +register( + microserver, short_cache_request_hdr, "", short_cache_response_hdr, response_body +) + +# The purpose here is to have a somewhat smarter origin that can respond to If-Modified-Since queries. +# We then can test how the cache server using this origin deals with stale caches. +origin = Test.MakeATSProcess("origin") + +origin.Disk.remap_config.AddLine(f"map / http://127.0.0.1:{microserver.Variables.Port}") + +origin.Disk.records_config.update( + { + "proxy.config.http.cache.http": 1, + "proxy.config.http.wait_for_cache": 1, + "proxy.config.http.insert_age_in_response": 0, + "proxy.config.http.request_via_str": 0, + "proxy.config.http.response_via_str": 0, + "proxy.config.diags.debug.enabled": 1, + "proxy.config.diags.debug.tags": "http", + } +) + +# Make the origin return 304 Not Modified for stale caches +origin.Disk.cache_config.AddLine("dest_ip=127.0.0.1 pin-in-cache=1d") + +# ---- +# Setup ATS +# ---- +# HACK: Don't use a fixed port because it causes ensuing tests to fail. The problem +# appears to be microDNS not allowing address reuse. +# +# https://docs.python.org/3/library/socketserver.html#socketserver.BaseServer.allow_reuse_address +ts = Test.MakeATSProcess("ts") + +ts.Disk.remap_config.AddLine(f"map / http://127.0.0.1:{origin.Variables.port}/") + +ts.Disk.records_config.update( + { + "proxy.config.http.cache.http": 1, + "proxy.config.http.cache.range.write": 1, + "proxy.config.http.response_via_str": 3, + "proxy.config.http.wait_for_cache": 1, + "proxy.config.diags.debug.enabled": 1, + "proxy.config.diags.debug.tags": "http", + } +) +# ---- +# Test Cases +# ---- + +# On cache miss +# --- + +# ATS should ignore the Range header when given an If-Range header with the incorrect etag +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange='"should-not-match"') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore( + microserver, ready=When.PortOpen(microserver.Variables.Port) +) +tr.Processes.Default.StartBefore(origin, ready=When.PortOpen(origin.Variables.port)) +tr.Processes.Default.StartBefore( + Test.Processes.ts, ready=When.PortOpen(ts.Variables.port) +) +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# On cache hit +# --- + +# ATS should respond to Range requests with partial content +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts) +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-206.gold" + +# ATS should respond to Range requests with partial content when given an If-Range header with +# the correct etag +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange='"range"') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-206.gold" + +# ATS should respond to Range requests with partial content when given an If-Range header +# that matches the Last-Modified header of the cached response +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange="Thu, 10 Feb 2022 00:00:00 GMT") +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-206.gold" + +# ATS should respond to Range requests with the full content when given an If-Range header +# that doesn't match the Last-Modified header of the cached response +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange="Thu, 10 Feb 2022 01:00:00 GMT") +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# ATS should respond to Range requests with a 416 error code when the given Range is invalid +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, start=100, end=105) +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-416.gold" + +# ATS should ignore the Range header when given an If-Range header with the incorrect etag +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange='"should-not-match"') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# ATS should ignore the Range header when given an If-Range header with weak etags +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange='W/"range"') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# ATS should ignore the Range header when given an If-Range header with a date older than the +# Last-Modified header of the cached response +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_range(ts, ifrange="Wed, 09 Feb 2022 23:00:00 GMT") +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# Write to the cache by requesting the entire content +tr = Test.AddTestRun() +tr.Processes.Default.Command = curl_whole(ts, path="short") +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-200.gold" + +# ATS should respond to range requests with partial content for stale caches in response to +# valid If-Range requests if the origin responds with 304 Not Modified. +tr = Test.AddTestRun() +tr.Processes.Default.Command = f"sleep {2 * CACHE_SHORT_MAXAGE}; " + curl_range( + ts, path="short", ifrange='"range"' +) +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/range-206-revalidated.gold" + +tr.StillRunningAfter = origin +tr.StillRunningAfter = microserver +tr.StillRunningAfter = ts