fix: properly process If-Range headers in client requests #8741
Conversation
|
@ezelkow1 can you kick off ci for this pr? |
|
[approve ci] |
proxy/http/HttpTransactCache.cc
Outdated
| bool | ||
| HttpTransactCache::validate_ifrange_header_if_any(HTTPHdr *request, HTTPHdr *response) | ||
| { | ||
| if (!(request->presence(MIME_PRESENCE_RANGE) && request->presence(MIME_PRESENCE_IF_RANGE))) { |
There was a problem hiding this comment.
Why is MIME_PRESENCE_RANGE checked here, when it's already been checked at both call sites?
There was a problem hiding this 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?
There was a problem hiding this comment.
you can remove with a comment if you like.
|
Looks good overall. Excellent description! |
|
I've been trying to recreate the issue and I just ran the autests in this PR against master without this PR and they all pass. Did some other PR already fix this? |
|
I tried reproducing this again, and it looks like the reproduction steps were insufficient. It looks like I need to set |
|
[approve ci autest] |
|
#8657 might've affected the test result, assuming that the CI does a merge with the current master branch before running the them. I'll try rebasing this PR. |
|
Anything new on this PR? |
midchildan
force-pushed
the
fix/if-range
branch
from
ad16518 to
24dc54e
Compare
midchildan
force-pushed
the
fix/if-range
branch
from
24dc54e to
c1ea24a
Compare
|
Sorry for the delay. I've rebased and tweaked the changes against the master branch. It turns out #8657 did change a few things, but the same issue still persists. Now instead of |
* fix: properly process If-Range headers in client requests * chore: add autest for range requests * refactor: only check the presence of Range headers once
6 participants
Problem
ATS doesn't respond correctly to conditional range requests. According to the spec, ATS must ignore the
Rangeheader and send back the full content when the condition in theIf-Rangeheader isn't satisified. ATS responds with416 Requested Range Not Satisfiableinstead.How to reproduce
curl -vv http://localhost)curl -vv -H 'If-Range: W/"this_wont_match"' http://localhost)Cause
ATS validates
If-Rangeheaders inmatch_response_to_request_conditionals(). If the condition specified inIf-Rangeisn't met,match_response_to_request_conditionals()returns a response code ofHTTP_STATUS_RANGE_NOT_SATISFIABLE. However, ATS can't determine whether aHTTP_STATUS_RANGE_NOT_SATISFIABLEresponse code came frommatch_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
If-Rangeheader validation code out ofmatch_response_to_request_conditionals()so that it would run right before processing theRangeheader. This makes it easier to determine the correct response.