lukeed / sirv

An optimized middleware & CLI application for serving static files~!
MIT License
1.07k stars 58 forks source link

fix(sirv): a request with range headers appends Content-Length, Conte… #75

Closed imtiazmangerah closed 4 years ago

imtiazmangerah commented 4 years ago

A request with Range headers present will result in the headers object being modified for all subsequent requests.

To reproduce:

curl -X GET  http://localhost:5000/pnggrad16rgb.png  -H 'Cache-Control: no-cache'

{
  'Content-Length': 3974,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT'
}
curl -X GET http://localhost:5000/pnggrad16rgb.png -H 'Cache-Control: no-cache' -H 'Range: bytes=0-499'

{
  'Content-Length': 500,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT',
  'Content-Range': 'bytes 0-499/3974',
  'Accept-Ranges': 'bytes'
}
curl -X GET  http://localhost:5000/pnggrad16rgb.png  -H 'Cache-Control: no-cache'

{
  'Content-Length': 500,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT',
  'Content-Range': 'bytes 0-499/3974',
  'Accept-Ranges': 'bytes'
}

Image used for above test:

pnggrad16rgb

codecov-commenter commented 4 years ago

Codecov Report

Merging #75 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files           2        2           
  Lines          67       67           
=======================================
  Hits           66       66           
  Misses          1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f8ea6a8...f500c6c. Read the comment docs.

lukeed commented 4 years ago

Thanks! Do you think you could add test(s) for this? Could be able to do the same thing you've illustrated with curl but with any of the current assets/fixtures.

imtiazmangerah commented 4 years ago

@lukeed I think the test suite will have to include a few more cases which will fail even with the fix, example:

bytes=0-499, -500

[x,y] would be parsed incorrectly, yet that is a valid header for bytes.

Is it the intention of sirv to have full support for range requests?

lukeed commented 4 years ago

Let's worry about the other Range values separately. That's easy to address, but this PR should focus on the mutable headers reference you identified

imtiazmangerah commented 4 years ago

@lukeed gave the tests a go, first commit of tests passed, cleaned up the tests to avoid repetition and now checks failed. I cant spot what would cause that, any hints?