Fixes raw_post being empty for "Transfer-Encoding: chunked"

[hahmed]

Summary

We had an issue raised in puma (puma/puma#1839) for chunked requests where the raw_post is empty, after digging through the issue I realised this is a bug in rails and not puma (tried with thin web server and raw_post is empty too).

Example request:

curl -H "Transfer-Encoding: chunked" -H "Content-Type: application/json" -d '{"abc": "123"}' -X POST http://localhost:3000/posts/create

I have repro the issue -- https://github.com/hahmed/chunk-puma-test

EDIT: I tested this PR with the app (chunk-puma-test) aforementioned, which no produces the correct output for request.raw_post

[benedikt]

I think the problem is related to the fact that for Transfer-Encoding: chunked requests, the Content-Length header is missing. Rails converts the missing Content-Length value to 0 and uses it to read that amount of bytes from the stream.

I think the solution should be something like #20337, which for some reason never made it into master.

[hahmed]

for chunked requests had to rewind again before reading for some reason

[jonathanhefner]

I commented out this line, and ran the tests in test_case_test.rb and they all passed. Are you sure this rewind is necessary? And if so, could a test be added to ensure it doesn't regress?

[hahmed]

The rewind is necessary, in the rails app I linked, if this line is commented out, it never worked.

Will test this out again and make sure it's working as it should (perhaps this test itself is not properly testing out the feature), will test and confirm.

Thanks for the reminder on this PR, been a while since I looked at it. 👍

[pjmartorell]

It fixes #15079

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[mhenrixon]

Running into this exact issue on kubernetes and ingress. Would love to see this PR merged but can monkey patch for now.

[hahmed]

@mhenrixon thanks for the reminder, I need to take a look into the test to make sure it's doing what it should. 👍

[mhenrixon]

So we just tried this patch out on a rails 5.2 project but no dice unfortunately.

I am on a k8s cluster using ingress and ingress chunks requests by default to avoid the possibility of getting completely taken down by multiple large requests.

Due to this bug, I had to patch ingress in an unsafe way to buffer the entire request into memory before passing it on to the rails server which is not cool.

Would love to have a working solution back-ported but could be persuaded to upgrade to newer rails version if needed.

Bottom line is that chucked requests are something that should be supported.

Currently looking into if the Transfer-Encoding header is actually set by ingress.

[hahmed]

@mhenrixon thanks for the update.

I managed to replicate this a while back and here is a sample app: https://github.com/hahmed/chunk-puma-test

Readme describes how I replicated the bug and this patch seemed to fix the issue I encountered. This bug was originally found/reported in puma.

If you have a demo app that I could take a look at, that would be awesome.

[mhenrixon]

@hahmed I might have rushed my answer. I suspect the header was not set by our ingress controller. Currently looking into it 👨‍💻

[mhenrixon]

Alright! After making sure ingress sets the header properly this patch works like a charm!

[hahmed]

@mhenrixon thanks for verifying it works, unfortunately I tried to find a way to write up a test for this scenario but I cannot seem to do so. The current test does passes with/without this code.

In the tests we seem to be getting a StringIO stream, whereas in the app I linked in the description, its a File stream which is likely the reason why this fails. If I manage to look into it again, that's likely where I will start.

@jonathanhefner unfortunately I'm going to have to close this down, thanks for your review on this, much appreciated!

[eugeneius]

I think this problem can be addressed in Puma: puma/puma#2287

Although, Rails should probably also accept requests without a Content-Length header, since they're allowed by HTTP 1.1. I'll look into that too 🙂

[hahmed]

Thanks for looking into this @eugeneius

When testing out the issue, I used the the app linked in the description.

I found that raw_post was empty for all the web servers I tested: puma, falcon and thin.

[emad-elsaid]

I ran into this problem couple days ago also in rails 6

[pengfeidong]

I ran into this problem couple days ago also in rails 6

I confirm, but after update to puma (5.2.2) the problem is resolved