You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/03/18 09:17:38 UTC
[GitHub] [trafficserver] midchildan opened a new pull request #8741: fix: properly process If-Range headers in client requests
midchildan opened a new pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741
# Problem
ATS doesn't respond correctly to conditional range requests. According to [the spec][1], ATS must ignore the `Range` header and send back the full content when the condition in the `If-Range` header isn't satisified. ATS responds with `416 Requested Range Not Satisfiable` instead.
# How to reproduce
1. Prepare a cacheable HTTP endpoint for ATS (e.g., http://localhost/)
2. Launch ATS
3. Send a normal request for the entire content so that it'd be cached in ATS (e.g., `curl -vv http://localhost`)
4. Send a conditional range request for the cached content. Specify a weak etag so that it'd fail to meet the requirements needed to receive a partial response. (e.g., `curl -vv -H 'If-Range: W/"this_wont_match"' http://localhost`)
# Cause
ATS validates `If-Range` headers in `match_response_to_request_conditionals()`. If the condition specified in `If-Range` isn't met, `match_response_to_request_conditionals()` returns a response code of `HTTP_STATUS_RANGE_NOT_SATISFIABLE`. However, ATS can't determine whether a `HTTP_STATUS_RANGE_NOT_SATISFIABLE` response code came from `match_response_to_request_conditionals()` or the response of the original content. This confusion led ATS to return incorrect responses to conditional range requests.
# Changes made
- Move the `If-Range` header validation code out of `match_response_to_request_conditionals()` so that it would run right before processing the `Range` header. This makes it easier to determine the correct response.
- Add autest test cases to check how ATS responds to range requests.
[1]: https://datatracker.ietf.org/doc/html/rfc7233#section-3.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] midchildan commented on a change in pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
midchildan commented on a change in pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#discussion_r831777922
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1357,47 +1359,49 @@ 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.
- // 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_RANGE) && request->presence(MIME_PRESENCE_IF_RANGE))) {
Review comment:
It's only meant as a precautionary measure. Should I turn it into an assertion, or would it be better to remove it completely?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#issuecomment-1074081175
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall edited a comment on pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
randall edited a comment on pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#issuecomment-1074018992
@ezelkow1 can you kick off ci for this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#issuecomment-1074018992
@ezelkow1 can you kick off ci for this or?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#discussion_r831737326
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1357,47 +1359,49 @@ 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.
- // 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_RANGE) && request->presence(MIME_PRESENCE_IF_RANGE))) {
Review comment:
Why is `MIME_PRESENCE_RANGE` checked here, when it's already been checked at both call sites?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #8741: fix: properly process If-Range headers in client requests
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #8741:
URL: https://github.com/apache/trafficserver/pull/8741#issuecomment-1074692074
Looks good overall. Excellent description!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org