trafficserver icon indicating copy to clipboard operation
trafficserver copied to clipboard

Published 20 hours ago • verified publisher icon apache

fix: properly process If-Range headers in client requests

Open midchildan opened this issue 3 years ago • 9 comments

Problem

ATS doesn't respond correctly to conditional range requests. According to the spec, 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.

midchildan avatar Mar 18 '22 09:03 midchildan

@ezelkow1 can you kick off ci for this pr?

randall avatar Mar 21 '22 15:03 randall

[approve ci]

ezelkow1 avatar Mar 21 '22 16:03 ezelkow1

Looks good overall. Excellent description!

SolidWallOfCode avatar Mar 22 '22 03:03 SolidWallOfCode

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?

traeak avatar Jun 23 '22 15:06 traeak

I tried reproducing this again, and it looks like the reproduction steps were insufficient. It looks like I need to set proxy.config.http.cache.range.write to 1 to reproduce. I'm working on an autest fix.

midchildan avatar Jun 29 '22 16:06 midchildan

[approve ci autest]

traeak avatar Jul 07 '22 14:07 traeak

#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.

midchildan avatar Jul 07 '22 15:07 midchildan

Anything new on this PR?

traeak avatar Jul 28 '22 16:07 traeak

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 HTTP_STATUS_RANGE_NOT_SATISFIABLE, it responds with HTTP_STATUS_PARTIAL_CONTENT when it should be giving the whole response. And while the pre-#8657 code gave an incorrect response regardless of the content of the If-Range header, the current code only responds incorrectly when given an non-matching If-Range condition.

midchildan avatar Aug 08 '22 16:08 midchildan