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

Introduce the ability to return 413: payload too large for requests #3040

Merged
merged 8 commits into from Jan 2, 2023

Conversation

shayonj
Copy link
Contributor

@shayonj shayonj commented Dec 20, 2022

Description

When receiving large payload objects, the server can often slowdown or get fully exhausted if bunch of requests with large payload body size come in. When request with large payload comes in, lot of the time is spent reading the body into buffer, writing it to the IO for rack, before the request is passed to the rails app for further processing.

While there are some workarounds around limiting large request sizes, like at nginx layer by setting client_max_body_size, which would return a 413 back to the client, today that is not possible with puma.

This would be a very nice feature to have, especially when there is no reverse proxy in between client and server.

This approach - allows a user to set http_content_length_limit via a config variable (defaults to nil). This value is then compared against Content-Length http header (when present) before reading the body into buffer. If the user value is higher than the header value, the request body is not loaded and an immediate 413 (Payload too large) http response is returned, from Puma::Request.handle_request.

Without having to buffer in the huge request and return the 413 immediately to the clients that send a Content-Length - could be a nice feature and helpful protection to have.

Additionally, when no Content-Length http header is present, it is compared against the size of the body of the request.
During this time, the body is already loaded, so there isn't much we can do, but it still respects the limit and
returns the 413 status from puma itself, before passing down the request to the app.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@shayonj shayonj force-pushed the s/content-length-limit branch 5 times, most recently from f2cd61e to f6bc181 Compare December 20, 2022 13:40
@shayonj
Copy link
Contributor Author

shayonj commented Dec 20, 2022

Trying it out locally

# config.rb

http_content_length_limit 100
 bundle exec bin/puma -C config.rb test/rackup/hello.ru

Sending a curl with standard body body above 100 bytes

curl -kso /dev/null -XPOST http://localhost:9292 -d "Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularised in the 1960s with the release of Letraset sheets containing Lorem Ipsum passages, and more recently with desktop publishing software like Aldus PageMaker including versions of Lorem Ipsum" -w "==============\n\n        
| dnslookup: %{time_namelookup}\n    
| connect: %{time_connect}\n             
| appconnect: %{time_appconnect}\n 
| pretransfer: %{time_pretransfer}\n 
| starttransfer: %{time_starttransfer}\n 
| total: %{time_total}\n
| size: %{size_download}\n
| HTTPCode=%{http_code}\n\n"
==============

 
| dnslookup: 0.003017
 
| connect: 0.003273
 
| appconnect: 0.000000
 
| pretransfer: 0.003293
 
| starttransfer: 0.003995
 
| total: 0.004048
 
| size: 17
 
| HTTPCode=413

With a large json (40mb)

curl -kso /dev/null -XPOST http://localhost:9292/1 -d @40mb.json -w "==============\n\n      
| dnslookup: %{time_namelookup}\n
| connect: %{time_connect}\n
| appconnect: %{time_appconnect}\n
| pretransfer: %{time_pretransfer}\n
| starttransfer: %{time_starttransfer}\n
| total: %{time_total}\n
| size: %{size_download}\n
| HTTPCode=%{http_code}\n\n"
==============

 
| dnslookup: 0.002976
 
| connect: 0.003273
 
| appconnect: 0.000000
 
| pretransfer: 0.003293
 
| starttransfer: 0.003568
 
| total: 0.004242
 
| size: 17
 
| HTTPCode=413

@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Dec 22, 2022
lib/puma/dsl.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

Great idea. I want to look at the code in a little more depth later, but I think this is a great feature for inclusion.

@@ -210,6 +215,17 @@ def try_to_parse_proxy_protocol
end

def try_to_finish
Copy link
Member

Choose a reason for hiding this comment

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

This is a very important path so we'll need to be careful with anything we add here.

@shayonj
Copy link
Contributor Author

shayonj commented Dec 22, 2022

Sounds good, appreciate the review. Yeah, definitely feels like something very useful. Especially when keeping puma workers snappy and fast by rejecting large requests.

I have been using it locally with our @tines monolith and works well (but ofc its local only). I will try to see if I can roll this out on some test stacks to test internally (but in prod) as well.

shayonj and others added 7 commits December 25, 2022 13:04
When recieving large payload objects, the server can often slowdown
or get fully exhausted if bunch of requests with large payload body size
come in. When request with large payload come, lot of the time is spent
reading it into then, writing it to the IO for rack, before the request
is passed to the rails app for further processing.

While there are some workarounds around limiting large request sizes, like at nginx layer
by setting `client_max_body_size`, which would return a `413` back to the client,
today that is not possible with puma.

This would be a very nice feature to have, especially when there is no reverse
proxy in between client and server.

This approach - allows a user to set `http_content_length_limit_exceeded`
via  a config variable (defaults to `nil`). This value is then compared
against `Content-Length` http header before reading the body into buffer.
If the user value is higher than the header value, the request body is
not loaded and an immediate `413` (`Payload too large`) http response is returned,
from `Puma::Request.handle_request`.

Without having to buffer in the huge request and return the `413`
immediately to the clients that send a `Content-Length` - is a nice feature
and helpful protection to have.
Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>
@shayonj shayonj force-pushed the s/content-length-limit branch 2 times, most recently from 91a0490 to 81938b3 Compare December 26, 2022 15:21
@nateberkopec
Copy link
Member

Thank you for your change @shayonj 🙇

@nateberkopec nateberkopec merged commit 1c7804c into puma:master Jan 2, 2023
@shayonj
Copy link
Contributor Author

shayonj commented Jan 3, 2023

Thanks @nateberkopec! Any ideas, when we can expect a release with this change? 🙌

@shayonj shayonj deleted the s/content-length-limit branch January 3, 2023 13:45
@nateberkopec
Copy link
Member

Probably in about 4 weeks? When I'm not busy, I try to release on a 4-week cadence. We just did a patch release so your commit was the first commit since that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants