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 raw_post empty bug when when Transfer-Encoding: chunked #47336

Merged
merged 2 commits into from Feb 12, 2023

Conversation

pedro108
Copy link
Contributor

@pedro108 pedro108 commented Feb 9, 2023

Motivation / Background

This PR fixes the issue with request.raw_post property being empty for chunked requests. (Transfer-Encoding header present and without a Content-Length value). It was inspired from this previous PR, but without the need of doing a second rewind of the request body.

A similar fix was done on Puma webserver, but with this fix, raw_post will work as expected for any Rack based http server.

Detail

This pull request changes ActionDispatch::Request#raw_post and ActionDispatch::Request#content_length methods, to take into account the presence of Transfer-Encoding header or not.

According to HTTP/1.1 RFC, when Transfer-Encoding is present, a sender must not send a Content-Length header field on a request, so because of that we can't rely on that field when reading the raw request body stream and calculating its content length for the upstream app.

So what this change does is to read the entire requests's body_stream for raw_post instead of using content_length as its length when Transfer-Encoding header is present.

And it uses the raw_post#bytesize value to calculate the request's content_length instead of reading this value from the Content-Length header, when Transfer-Encoding header is present.

Additional information

To test this case an integration railties test http_request_test.rb was used, in order for it to be similar to real use cases.

It is also recommended to test this change manually with a real Rails app running on different Rack servers (e.g. Puma, Unicorn, Webrick or Thin)

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.

@pedro108 pedro108 force-pushed the fix-raw-post-for-chunked-request branch 2 times, most recently from 525e36a to 27e96a9 Compare February 9, 2023 13:49
Copy link
Contributor

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

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

I'm not on the team so take these with a grain of salt, but this is my feedback.

actionpack/lib/action_dispatch/http/request.rb Outdated Show resolved Hide resolved
actionpack/lib/action_dispatch/http/request.rb Outdated Show resolved Hide resolved
railties/test/railties/http_request_test.rb Outdated Show resolved Hide resolved
@pedro108 pedro108 force-pushed the fix-raw-post-for-chunked-request branch 2 times, most recently from 9387da0 to c31ea8e Compare February 10, 2023 14:37
@pedro108 pedro108 force-pushed the fix-raw-post-for-chunked-request branch from c31ea8e to 75126b8 Compare February 10, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants