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

Default max body size for streaming request body collection #2312

Merged
merged 16 commits into from May 1, 2020

Conversation

calebkleveter
Copy link
Member

@calebkleveter calebkleveter commented Apr 14, 2020

Adds app.routes.defaultMaxBodySize for configuring default maximum body size for streaming body collection (#2312).

A ByteCount type has been added for easily expressing byte counts as strings like "1mb", "200GB", or "42 kb". This type is now usable where maximum body size integers were previously accepted (#2312).

Vapor collects streaming bodies up to 16KB in size automatically before calling route closures. This makes it easier to decode content from requests since you can assume the entire request is already available in memory.

To increase the maximum allowable size for streaming body collection, you can pass an arbitrary maxSize to the collect case of the body parameter in RouteBuilder.on.

app.on(.POST, "upload", body: .collect(maxSize: "1mb")) { req in
    // Handle request. 
}

For more information on this API, visit https://docs.vapor.codes/4.0/routing/#body-streaming.

Now, in addition to setting this parameter for each route, you can change the global default Vapor uses.

app.routes.defaultMaxBodySize = "10mb"

Route specific maximum size will always take precedence over the application default.

Note: This maximum size only affects streaming request bodies. Non-streaming request bodies (request bodies that arrive in a single buffer from SwiftNIO) will not be subject to the maximum size restriction.

⚠️ Using .collect(maxSize: nil) will now result in the application's default maximum body size being used.

@calebkleveter calebkleveter added enhancement New feature or request semver-minor Contains new API labels Apr 14, 2020
@calebkleveter calebkleveter self-assigned this Apr 14, 2020
@calebkleveter calebkleveter added this to Awaiting Review in Vapor 4 via automation Apr 14, 2020
@@ -1,7 +1,13 @@
public protocol RoutesBuilder {
var defaultMaxBodySize: Int? { get }
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not super happy that I have to add this (at least we can have a default value). If there are any ideas on how it can be removed, I'll take them.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let me take a stab at this and see if I can get it to work without this requirement.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

+1, thanks!

public init() {
self.defaultMaxBodySize = 1_000_000
Copy link
Member

Choose a reason for hiding this comment

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

Should we do 1<<20 so it's 1MB and not 1MiB?

/// - defaultMaxBodySize: The maximum number of bytes that a request body can contain in the new group
/// `nil` means there is no limit.
/// - returns: A newly created `Router` with a new max body size.
public func group(maxSize defaultMaxBodySize: Int?) -> RoutesBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use the full defaultMaxBodySize label here. I'm not sure maxSize or even maxBodySize are clear enough.

/// - configure: Closure to configure the newly created `Router`.
/// - builder: The new builder with the new max body size.
/// - returns: A newly created `Router` with a new max body size.
public func grouped(maxSize defaultMaxBodySize: Int?, configure: (_ builder: RoutesBuilder) throws -> ()) rethrows {
Copy link
Member

Choose a reason for hiding this comment

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

This should be group. The ...ed ones return a route builder.

/// - defaultMaxBodySize: The maximum number of bytes that a request body can contain in the new group
/// `nil` means there is no limit.
/// - returns: A newly created `Router` with a new max body size.
public func group(maxSize defaultMaxBodySize: Int?) -> RoutesBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

This should be grouped

let collected: EventLoopFuture<Void>

if let data = request.body.data {
collected = request.eventLoop.tryFuture {
Copy link
Member

Choose a reason for hiding this comment

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

indentation

@@ -140,21 +140,33 @@ extension RoutesBuilder {
public func on<Response>(
_ method: HTTPMethod,
_ path: [PathComponent],
body: HTTPBodyStreamStrategy = .collect,
body: HTTPBodyStreamStrategy? = nil,
Copy link
Member

Choose a reason for hiding this comment

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

We should keep this as .collect

Copy link
Member Author

Choose a reason for hiding this comment

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

If we make this .collect, how do we use the default max body size set for the routes builder?

Copy link
Member

Choose a reason for hiding this comment

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

Adding to #2312 (comment), my previous comments were intending that .collect(maxSize: nil) would use the default max body size. I think an enum could also make sense.

I don't think optionality makes sense for HTTPBodyStreamStrategy as a whole though. Max body size only affects the collect case, stream is never capped. So we should express the optionality within the collect case. Either with an optional or an enum.

///
/// See `collect(maxSize:)` to set a lower max body size.
public static var collect: HTTPBodyStreamStrategy {
return .collect(maxSize: 1 << 14)
return .collect(maxSize: 1_000_000)
Copy link
Member

Choose a reason for hiding this comment

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

This should go back to being nil so that it relies on the defaultMaxBodySize.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't nil supposed to mean 'just keep accepting data until we reach the end of the body'?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking nil here would mean "defer to global default". If that is also nil, then you could get unlimited body size. Maybe it's worth making this more explicit with an enum:

enum MaxSize {
    // Defer to application config
    case default
    // Provide a custom value
    // - warning: Passing `nil` here means no restrictions on how much memory an incoming request can use.
    case override(ByteCount?)
}

@tanner0101
Copy link
Member

Side note, I think we should add a ByteCount struct. Something like:

let bytes: ByteCount = "1mb"
print(bytes.count) // 1048576

let bytes: ByteCount = 1_000_000
print(bytes.count) // 1000000

let bytes: ByteCount = "2kib"
print(bytes.count) // 2000

@tanner0101 tanner0101 moved this from Awaiting Review to Awaiting Updates in Vapor 4 Apr 14, 2020
@Joannis
Copy link
Member

Joannis commented Apr 17, 2020

Can I help with this? This is a serious issue right now.

@calebkleveter
Copy link
Member Author

@Joannis By all means, please do. I don't have a whole lot of time to put into it right now, and since it's not needed for any of the work projects I am on right now other things take priority.

@jdmcd
Copy link
Member

jdmcd commented Apr 24, 2020

@Joannis any updates on this? Happy to jump in and help if you don't have time, this is critical for me as well

@Joannis
Copy link
Member

Joannis commented Apr 24, 2020

@mcdappdev I've been working on a few tasks that were more urgent to me. I can start working on it (after) this weekend. I didn't start yet, but I doubt it'll take long. Let me know if you're picking it up so I won't.

@jdmcd
Copy link
Member

jdmcd commented Apr 25, 2020

@Joannis going to give this a shot now - I'll keep you updated!

@jdmcd
Copy link
Member

jdmcd commented Apr 25, 2020

@Joannis @calebkleveter @tanner0101 Just pushed fixes to address the comments. I think I hit all of the requested changes - the ByteCount implementation is a little rudimentary right now so feel free to make suggestions on cleaning that up if you'd like.

@@ -42,6 +42,10 @@ extension Request {
return buffer
}
case .collected(let buffer):
if let max = max, buffer.readableBytes > max {
return self.request.eventLoop.future(error: Abort(.payloadTooLarge))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this check here. The point of max is to avoid allocating too much memory. In this case, the memory has already been allocated we are just passing them a reference.

@@ -1,7 +1,13 @@
public protocol RoutesBuilder {
var defaultMaxBodySize: Int? { get }
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let me take a stab at this and see if I can get it to work without this requirement.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

I've updated this PR to not need defaultMaxBodySize added to the RoutesBuilder protocol. It instead uses req.application.routes.defaultMaxBodySize inside the responder.

Let me know what you guys think.

@@ -95,7 +95,7 @@ final class ServerTests: XCTestCase {

let payload = [UInt8].random(count: 1 << 20)

app.on(.POST, "payload", body: .collect(maxSize: nil)) { req -> HTTPStatus in
app.on(.POST, "payload", body: .collect(maxSize: "1gb")) { req -> HTTPStatus in
Copy link
Member

Choose a reason for hiding this comment

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

Passing nil no longer means "unlimited". It now means "use default". I think this is a better API but this brings up a couple of questions:

  • Is this too breaking for a minor release?

I don't think so since I view the current behavior as a bug.

  • Should there be a way to have truly unlimited body size?

I don't think so as this opens your server up to an easy attack vector. If you really want to get around this you can use Int.max as the body size. This is the maximum theoretical size anyway since we need to be able to represent the body size as an integer.

@tanner0101 tanner0101 moved this from Awaiting Updates to Awaiting Review in Vapor 4 Apr 28, 2020
@jdmcd
Copy link
Member

jdmcd commented Apr 28, 2020

This looks good to me @tanner0101!


for suffix in validSuffixes {
guard value.hasSuffix(suffix.key) else { continue }
guard let stringIntValue = value.components(separatedBy: suffix.key).first else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we make this a little fuzzier? Such as lowercasing the input before splitting it.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, lowercasing is a good idea. Maybe removing whitespace too?

@tanner0101 tanner0101 changed the title Global Request Body Max Size Configuration Default max body size for streaming request body collection May 1, 2020
@tanner0101 tanner0101 merged commit c70d45f into master May 1, 2020
Vapor 4 automation moved this from Awaiting Review to Done May 1, 2020
@tanner0101 tanner0101 deleted the verify-body-size-handler branch May 1, 2020 20:39
@tanner0101
Copy link
Member

These changes are now available in 4.5.0

@tanner0101
Copy link
Member

Docs have been updated here https://docs.vapor.codes/4.0/routing/#body-streaming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new API
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants