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

Don't use #eof? when parsing multipart #1201

Conversation

@janko
Copy link
Contributor

janko commented Aug 18, 2017

On chunked requests (Transfer-Encoding: chunked) content length won't be present, so #eof? will be called on the Rack input directly. However, the Rack input doesn't have to respond to #eof?, it's not part of the Rack specification. For example, Rack::Lint::InputWrapper and Unicorn::StreamInput don't respond to #eof?. This means that in development and on Unicorn, multipart parsing will fail for chunked requests.

This change refactors the multipart parser so that it doesn't call #eof?.

On chunked requests (`Transfer-Encoding: chunked`) content length won't
be present, so `#eof?` will be called on the Rack input directly.
However, the Rack input doesn't have to respond to `#eof?`, it's not
part of the Rack specification. For example, `Rack::Lint::InputWrapper`
and `Unicorn::StreamInput` don't respond to `#eof?`. This means that in
development and on Unicorn, multipart parsing will fail for chunked
request.

This change refactors the multipart parser so that it doesn't call
`#eof?`.
@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented Jun 8, 2018

This is a good idea and should be merged!

@rafaelfranca rafaelfranca merged commit 7420be2 into rack:master Jun 11, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@janko janko deleted the janko:make-multipart-parsing-work-for-chunked-requests branch Jun 11, 2018
@stanhu

This comment has been minimized.

Copy link

stanhu commented Nov 27, 2018

@janko-m We just ran into private method errors where eof? in Unicorn::StreamInput. Would you mind tagging a new release with this?

matthewd added a commit to matthewd/rack that referenced this pull request Dec 19, 2018
…-for-chunked-requests

Don't use #eof? when parsing multipart
@dirkjonker

This comment has been minimized.

Copy link

dirkjonker commented Jan 21, 2019

We are experiencing an issue with uploading documents that is very difficult to reproduce but randomly fails because rack is calling eof? as described in this issue on Passenger.

@tenderlove would you consider tagging a new release for this? Technically, currently the latest rack release (2.0.6) does not conform to its own specifications because of this.

@urkle

This comment has been minimized.

Copy link

urkle commented Feb 13, 2019

I also was just bit by this bug. Any timeline of pushing a 2.0.7 rack version that includes this fix?

tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Apr 5, 2019
This update has two important fixes:

1. It reverts the monkey patch introduced in
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23385 since
rack/rack#1201 is now part of the release.

2. Preserve forwarded IP address for trusted proxy chains
(rack/rack#1343).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.