check body length #255
check body length #255
Conversation
| @@ -794,11 +796,19 @@ extension TaskHandler: ChannelDuplexHandler { | |||
| assert(head.version == HTTPVersion(major: 1, minor: 1), | |||
| "Sending a request in HTTP version \(head.version) which is unsupported by the above `if`") | |||
|
|
|||
| self.expectedBodyLength = head.headers[canonicalForm: "content-length"].first.flatMap { Int($0) } | |||
There was a problem hiding this comment.
this needs to error if there's more than one CL header
There was a problem hiding this comment.
shouldn't validateHeaders do that?
There was a problem hiding this comment.
I can add a return from validate with body length, as an alternative
There was a problem hiding this comment.
I'm happy if we have a test that proves that we error if the user sends two content-lengths. In this code, I think we should just add an
assert(head.headers[canonicalForm: "content-length"].count <= 1)
to be sure. That makes the code much easier to read because you can immediately dismiss that thought if you see the assertion.
There was a problem hiding this comment.
it's even better, we remove all content length headers and insert them ourselves :) except one case, I'll add a test for that (and an assert).
There was a problem hiding this comment.
That will happen only if the stream has no length is not specified. In all other cases we actually ignore the user-provided headers, you think we should fail on those?
There was a problem hiding this comment.
@artemredkin wait, why are we looking for the first CL header then if this code is only run if the user specified no CL headers?
There was a problem hiding this comment.
this code is run always, if the content-length was not specified, then expectedLength will be nil and final check will not be executed
There was a problem hiding this comment.
alright, let's just add the assertion&test (if not present for all the possible cases) in case somebody modifies the sanitisation code.
| @@ -836,6 +846,7 @@ extension TaskHandler: ChannelDuplexHandler { | |||
| } | |||
|
|
|||
| return promise.futureResult.map { | |||
| self.actualBodyLength += part.readableBytes | |||
There was a problem hiding this comment.
- assertion about the correct EL missing
- test that would fail this assertion right now probably missing.
There was a problem hiding this comment.
moved the increment to where it is certainly on correct EL
| XCTAssertEqual(error as! HTTPClientError, HTTPClientError.bodyLengthMismatch) | ||
| } | ||
| // Quickly try another request and check that it works. | ||
| XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "/get").wait()) |
There was a problem hiding this comment.
Sorry, I know I wrote this test but we should check the correct connectionNumber and requestNumber from
internal struct RequestInfo: Codable {
var data: String
var requestNumber: Int
var connectionNumber: Int
}
just to be sure we get a fresh connection.
There was a problem hiding this comment.
hmm, do we even need this part? we already have a test that checks that, no?
There was a problem hiding this comment.
ah, I think I know what you mean!
| } | ||
| // Quickly try another request and check that it works. If we by accident wrote some extra bytes into the | ||
| // stream (and reuse the connection) that could cause problems. | ||
| XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "/get").wait()) |
There was a problem hiding this comment.
similar here, we should validate request/connection number.
| @@ -357,8 +361,8 @@ internal struct HTTPResponseBuilder { | |||
| } | |||
| } | |||
|
|
|||
| let globalRequestCounter = NIOAtomic<Int>.makeAtomic(value: 0) | |||
| let globalConnectionCounter = NIOAtomic<Int>.makeAtomic(value: 0) | |||
| //let globalRequestCounter = NIOAtomic<Int>.makeAtomic(value: 0) | |||
There was a problem hiding this comment.
oops, fixed, thanks!
|
@swift-server-bot test this please |
artemredkin
force-pushed
the
check_body_length
branch
6 times, most recently
from
c161ff5 to
610af18
Compare
artemredkin
force-pushed
the
check_body_length
branch
from
610af18 to
715ab14
Compare
|
@swift-server-bot test this please |
Adds checking for supplied body length.
Motivation:
We need to guard against incorrect body length since we can re-use connection, this is a potential security issue.
Modifications:
HTTPTaskHandlerthat if validated headers containContent-Length, user sends exactly that amount of body data.Result:
Closes #251