Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http/file_server): fix Range header handling #3354

Merged
merged 7 commits into from
May 4, 2023

Conversation

ayame113
Copy link
Contributor

@ayame113 ayame113 commented May 2, 2023

close #3353

Current file servers' handling of the Range header is different from typical server's handling.
After verifying with google cloud storage, cloudflare, AWS cloudfront, githubusercontent.com, express lib, etc., it seems that Range requests are handled roughly in the manner shown in the table below.

These differences are causing problems like denoland/deploy_feedback#377.
In this PR, I rewrote the handling of the Range header to behave as per spec and behave the same as a typical server.

When requesting a file with content-length of 100:

request header std's implementation before this PR typical server
(e.g. cloud storage, cloudfront, github...) 
range: bytes=0-10 🟡206 Partial Content
(content-range: bytes 0-10/100)
🟡206 Partial Content
(content-range: bytes 0-10/100)
range: bytes=90- 🟡206 Partial Content
(content-range: bytes 90-99/100)
🟡206 Partial Content
(content-range: bytes 90-99/100)
range: bytes=-10 (means the last 10 bytes) 🔴416 Range Not Satisfiable
(no content-range header: wrong)
🟡206 Partial Content
(content-range: bytes 90-99/100)
range: bytes=90-110 🔴416 Range Not Satisfiable
(content-range: bytes 90-110/100: wrong)
🟡206 Partial Content
(content-range: bytes 90-99/100, The range is clamped)
range: bytes=-110 (means the last 110 bytes) 🔴416 Range Not Satisfiable
(no content-range header: wrong)
🟡206 Partial Content
(content-range: bytes 0-99/100, The range is clamped)
range: bytes=110-120 (out of range) 🔴416 Range Not Satisfiable
(content-range: bytes 110-120/100: wrong)
🔴416 Range Not Satisfiable
(content-range: bytes */100)
range: bytes=110- (out of range) 🔴416 Range Not Satisfiable
(content-range: bytes 110-99/100: wrong)
🔴416 Range Not Satisfiable
(content-range: bytes */100)
range: bytes=20-10 (invalid) 🔴416 Range Not Satisfiable
(content-range: bytes 20-10/100: wrong)
🔴416 Range Not Satisfiable
(content-range: bytes */100)
range: bytes=foobar (invalid) 🔴416 Range Not Satisfiable
(no content-range header: wrong)
Fallback to 🟢200 OK
range: bytes=0-10,20-30 🟡206 Partial Content
(content-range: bytes 0-10/100, Only the first part is accepted)
Fallback to 🟢200 OK

Note:

  • content-range: bytes */100 header is required when returning 416 Range Not Satisfiable response.

    A server generating a 416 (Range Not Satisfiable) response to a
    byte-range request SHOULD send a Content-Range header field with an
    unsatisfied-range value, as in the following example:

    Content-Range: bytes */1234
    https://datatracker.ietf.org/doc/html/rfc7233#section-4.2

  • A Range request to an empty file should always return 416 Range Not Satisfiable.
    It seems reasonable to return a 200 OK for a Range request to an empty file.

    There's no way to issue an RFC 7433-conformant request for a range of a 0-length file. A range request is a request for a non-zero-length range of bytes. A 0-length file contains no bytes. I believe that a 416 Range Not Satisfiable response is correct in this situation. There are other correct responses (we could ignore the Range header entirely for 0-length files, for example, and return a 200 response), but I don't see a reason those other possibilities are better. I don't think there's anything surprising about responding to a request for data past the end of a file with, "Sorry, the file isn't that big".
    net/http: handling range requests for empty files golang/go#47021

    Some clients add a Range header to all requests to
    limit the size of the response. If the file is empty,
    ignore the range header and respond with a 200 rather
    than a 416.
    https://github.com/golang/go/blob/0d347544cbca0f42b160424f6bc2458ebcc7b3fc/src/net/http/fs.go#L273-L276

  • Requesting multiple Ranges like range: bytes=0-10,20-30 is allowed by the spec, but as far as I know, few servers supports it.

Sorry for the large git diff! Thank you for your review every time.

@ayame113 ayame113 marked this pull request as ready for review May 2, 2023 19:08
@ayame113 ayame113 requested a review from kt3k as a code owner May 2, 2023 19:08
@ayame113 ayame113 marked this pull request as draft May 3, 2023 08:36
@ayame113 ayame113 marked this pull request as ready for review May 3, 2023 08:48
@kt3k
Copy link
Member

kt3k commented May 4, 2023

Requesting multiple Ranges like range: bytes=0-10,20-30 is allowed by the spec, but as far as I know, few servers supports it.

This sounds interesting

@kt3k
Copy link
Member

kt3k commented May 4, 2023

parseRangeHeader (with satisfiability check) might be worth another utility in std/http

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for the detailed research on the edge cases of range requests/response!

@kt3k kt3k merged commit 42a3a80 into denoland:main May 4, 2023
@ayame113 ayame113 deleted the fix-range-header-handling branch May 4, 2023 07:10
mxdvl pushed a commit to mxdvl/deno_std that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http/file_server: Wrong Content-Range response header
2 participants