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

range request on empty file should return 200 instead of 416 #1937

Closed
sourtin opened this issue Oct 12, 2020 · 4 comments · Fixed by #2014
Closed

range request on empty file should return 200 instead of 416 #1937

sourtin opened this issue Oct 12, 2020 · 4 comments · Fixed by #2014
Labels
Milestone

Comments

@sourtin
Copy link

sourtin commented Oct 12, 2020

When attempting to access any range on an empty file, Werkzeug returns a 416 Requested Range Not Satisfiable error. Whilst this is perhaps reasonable, it is inconsistent with behaviour for non-empty files. For example, the range bytes=0-999999 on a small file, say 10 bytes in length, will succeed with a Content-Range: bytes 0-9/10 response header.

The issue is that Content-Range is inclusive, and so ostensibly the only way to succeed for an empty file would be to return the (invalid) response header Content-Range: bytes 0--1/0. As such, Werkzeug's behaviour is technically correct with respect to the specification, but I would argue it is undesirable/unexpected.

Two potential alternative approaches are:

  1. Respond with Content-Range: bytes */0 to indicate an unknown range.
  2. 'Ignore' the range request in this case and return 200 OK.

Using curl -ir 0-0 against an empty file shows that different servers take different approaches. For sourtin.github.io/empty.txt, it gives a 416 error but responds with bytes */0, whereas for Apache/2.4.18 it gives 200 OK and ignores the range (i.e. approach 2).

Adding a special case is in its own way undesirable, but if either approach is acceptable I'm happy to submit an appropriate PR. I'd lean towards approach 2, personally.

@davidism davidism changed the title Possibly undesirable behaviour for range requests range request on empty file shoud return 200 instead of 416 Jan 14, 2021
@davidism davidism changed the title range request on empty file shoud return 200 instead of 416 range request on empty file should return 200 instead of 416 Jan 14, 2021
@davidism
Copy link
Member

davidism commented Jan 18, 2021

Returning 200 seems fine. That said, is there anything in relevant RFCs about this case?

@hannah-earley
Copy link

The relevant RFC is 7233. Skimming it, I think this edge case is impossible to handle according to the spec, and I couldn't (quickly) find any comment on how to handle this specific case. The intention is that, if someone asks for a range 0-10 for example, and the file is only 5 bytes long, then the whole contents of the file are returned:

If the last-byte-pos value is absent, or if the value is greater than or equal to the current length of the representation data, the byte range is interpreted as the remainder of the representation (i.e., the server replaces the value of last-byte-pos with a value that is one less than the current length of the selected representation).

But if the file is 0 bytes, then the range returned should be 0-(-1), which doesn't make sense as last-byte-pos must be a non-negative integer. The problem is that the range is inclusive, i.e. 0-0 is bytes [0,0] should return 1 byte. If the range was instead left-closed and right-open, [0,0), then there wouldn't be an edge case here...

As range requests are optional though,

A server MAY ignore the Range header field.

An origin server MUST ignore a Range header field that contains a range unit it does not understand.

it would seem consistent to pretend in this case that we don't understand the range header or that this particular file doesn't support range requests, and therefore to send 200 OK.

@hannah-earley
Copy link

It's been a while since I last looked at this issue so I can't remember what my thoughts for writing a PR were back then, but looking at the source now there are a few obvious approaches:

  1. in wz.utils.send_file, don't call wz.response.Response.make_conditional if size == 0
  2. in wz.response.Response.make_conditional, return early if complete_length == 0
  3. in wz.response.Response._process_range_request, return early if complete_length == 0

The problem is that make_conditional doesn't just do range requests but also checks modification status, so each of these would interfere with that behaviour. As the content length would be 0, though, unconditionally sending the file anyway feels like a reasonable thing to do.

Another issue is that size/complete_length can be None and I'm not sure what the appropriate thing to do in that case is. It looks like that is already handled by falling back to not doing a range request—possibly erroring out if the file isn't modified—and so probably the size == 0 condition is still fine.

Basically I'm just not sure whether there would be any unintended consequences to this special casing...

@davidism davidism added this to the 2.0.0 milestone Jan 21, 2021
@davidism
Copy link
Member

Thanks for the details. Seems acceptable to return 200 in this case.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants