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
Conversation
…uteBuilder protocol
…nder There was also an issue when the request body existed before we get to the responder, the size of the body was not checked. We check it now
…stConfigurableMaxBodySize test case
@@ -1,7 +1,13 @@ | |||
public protocol RoutesBuilder { | |||
var defaultMaxBodySize: Int? { get } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks!
Sources/Vapor/Routing/Routes.swift
Outdated
public init() { | ||
self.defaultMaxBodySize = 1_000_000 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
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?)
}
Side note, I think we should add a 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 |
Can I help with this? This is a serious issue right now. |
@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. |
@Joannis any updates on this? Happy to jump in and help if you don't have time, this is critical for me as well |
@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. |
@Joannis going to give this a shot now - I'll keep you updated! |
@Joannis @calebkleveter @tanner0101 Just pushed fixes to address the comments. I think I hit all of the requested changes - the |
@@ -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)) |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
These changes are now available in 4.5.0 |
Docs have been updated here https://docs.vapor.codes/4.0/routing/#body-streaming |
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 thecollect
case of thebody
parameter inRouteBuilder.on
.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.
Route specific maximum size will always take precedence over the application default.
.collect(maxSize: nil)
will now result in the application's default maximum body size being used.