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

feat: add layer that limits body size #271

Merged
merged 19 commits into from Jun 6, 2022

Conversation

neoeinstein
Copy link
Contributor

@neoeinstein neoeinstein commented May 20, 2022

Motivation

To provide a tower layer that limits the allowable length of the request body to a certain amount.

Solution

Add a new tower layer that wraps the incoming body in http_body::Limited<B>.

A couple of decisions:

  • Do we want this layer to automatically return a 413 Payload Too Large error?
  • Are we okay with the way the errors and response body are all boxed up in that event?

@jplatte
Copy link
Collaborator

jplatte commented May 21, 2022

Do we want this layer to automatically return a 419 Payload Too Large error?

Without having looked at the implementation or considering any previous disussion around this, this seems like an obvious 'Yes' to me.

tower-http/src/limit.rs Outdated Show resolved Hide resolved
tower-http/src/limit.rs Outdated Show resolved Hide resolved
tower-http/src/limit.rs Outdated Show resolved Hide resolved
tower-http/src/limit.rs Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member

Do we want this layer to automatically return a 419 Payload Too Large error?

Yes I think we should. Except we can't do that unless the request has a content-length I think. Otherwise we'd have to buffer the request body first which might be a long stream.

@neoeinstein
Copy link
Contributor Author

neoeinstein commented May 21, 2022

Yes I think we should. Except we can't do that unless the request has a content-length I think. Otherwise we'd have to buffer the request body first which might be a long stream.

If you take a look at how this is set up right now, in combination with http_body::Limited, this will immediately cease reading any more of the incoming body once the limit has been reached and return the 413. If you want more of a guarantee of that, I can set up a test case asserting this.

tower-http/src/limit.rs Outdated Show resolved Hide resolved
tower-http/src/limit.rs Outdated Show resolved Hide resolved
davidpdrsn added a commit to tokio-rs/axum that referenced this pull request May 22, 2022
This adds special handling of `http_body::LengthLimitError` to
`FailedToBufferBody` such that this just works and returns `413 Payload
Too Large` if the limit is exceeded:

```rust
use http_body::Limited;
use tower_http::map_request_body::MapRequestBodyLayer;

Router::new()
    .route("/", post(|body: Bytes| { ... }))
    .layer(MapRequestBodyLayer::new(|body| {
        Limited::new(body, 1024)
    }));
```

This is a draft until tower-rs/tower-http#271 is
merged because I wanna make sure that works as well. The use case I have
in mind is:

```rust
use http_body::Limited;
use tower_http::limit::RequestBodyLimitLayer;

Router::new()
    .route("/", post(|body: Bytes| { ... }))
    .layer(RequestBodyLimitLayer::new(1024));
```
This removes the `StdError + 'static` bound on the service error.
Also sets the read limit for a given message to the lesser of
`Content-Length` or the configured limit.
@neoeinstein
Copy link
Contributor Author

I made some changes based on the feedback. Now the future doesn't attempt to interpret the error returned by the underlying service. This removes the StdError + 'static restriction on S::Error. It does mean that something else (like axum) needs to be able to convert a failure to read from Limited<B> into an appropriate response, but I think that's the better option.

This now also interprets the Content-Length header and returns early with a 413 in the event that the payload would be too large.

Doc examples have been updated as well.

@davidpdrsn
Copy link
Member

Amazing! Thank you for working on this. I'll review it this week :)

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

@neoeinstein I think it looks good! I pushed a bunch of nit picks but had one question I'd like to hear what you think about.

tower-http/src/limit/mod.rs Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn enabled auto-merge (squash) June 6, 2022 15:52
@davidpdrsn davidpdrsn merged commit 5468fc8 into tower-rs:master Jun 6, 2022
@davidpdrsn
Copy link
Member

@neoeinstein Thanks for working on this!

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.

None yet

3 participants