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 range handling for oversized and out of range situations #373

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

jnardone
Copy link
Contributor

Range requests were doing a few things wrong:

  1. always returning true regardless of whether or not the range was successful. If the range is completely out of bounds a 416 must be returned
  2. returning the requested range as the size and not the actual amount of data returned

This broke things that depend on range requests (notably s3-blob-store & feathers-blob) which we use extensively. Hoping for a quick merge & tag so we can keep pulling from the real upstream.

Thanks for a great project!

@kherock
Copy link
Collaborator

kherock commented Jan 24, 2019

Thanks for the simple fix, and nice use of fs.ReadStream#bytesRead. With that, could you update the engine version in package.json to be >= 6.4.0? LGTM otherwise.

@jnardone
Copy link
Contributor Author

Will do/did. Engine ver bumped and added an appropriate response header for the 416 response. Let me know if you need anything else.

@jnardone
Copy link
Contributor Author

Anything else needed for this to get merged?

@leontastic leontastic merged commit 979b99f into jamhall:master Jan 28, 2019
@leontastic
Copy link
Collaborator

@jnardone I've cut a release v2.2.8 with your fix and published to npm. Thank you for your contribution!

kherock added a commit that referenced this pull request Feb 7, 2019
This ensures no file handles are left open after receiving an unsatisfiable range request by never opening the stream in the first place.
kherock added a commit that referenced this pull request Feb 7, 2019
This ensures no file handles are left open after receiving an unsatisfiable range request by never opening the stream in the first place.
kherock added a commit that referenced this pull request Feb 7, 2019
object.range = {
start: options.start || 0,
end: options.end || object.size - 1
Copy link

Choose a reason for hiding this comment

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

I think this broke the range response header in 2.2.8, but looks like it's fixed on master.

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.

4 participants