Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Sources/AsyncHTTPClient/HTTPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
case uncleanShutdown
case traceRequestWithBody
case invalidHeaderFieldNames([String])
case bodyLengthMismatch
}

private var code: Code
Expand Down Expand Up @@ -969,10 +970,12 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
public static let redirectLimitReached = HTTPClientError(code: .redirectLimitReached)
/// Redirect Cycle detected.
public static let redirectCycleDetected = HTTPClientError(code: .redirectCycleDetected)
/// Unclean shutdown
/// Unclean shutdown.
public static let uncleanShutdown = HTTPClientError(code: .uncleanShutdown)
/// A body was sent in a request with method TRACE
/// A body was sent in a request with method TRACE.
public static let traceRequestWithBody = HTTPClientError(code: .traceRequestWithBody)
/// Header field names contain invalid characters
/// Header field names contain invalid characters.
public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) }
/// Body length is not equal to `Content-Length`.
public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch)
}
11 changes: 11 additions & 0 deletions Sources/AsyncHTTPClient/HTTPHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,8 @@ internal class TaskHandler<Delegate: HTTPClientResponseDelegate>: RemovableChann
let logger: Logger // We are okay to store the logger here because a TaskHandler is just for one request.

var state: State = .idle
var expectedBodyLength: Int? = nil
var actualBodyLength: Int = 0
var pendingRead = false
var mayRead = true
var closing = false {
Expand Down Expand Up @@ -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) }
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to error if there's more than one CL header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

shouldn't validateHeaders do that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add a return from validate with body length, as an alternative

Copy link
Contributor

@weissi weissi Jun 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this code is run always, if the content-length was not specified, then expectedLength will be nil and final check will not be executed

Copy link
Contributor

Choose a reason for hiding this comment

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

alright, let's just add the assertion&test (if not present for all the possible cases) in case somebody modifies the sanitisation code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!


context.write(wrapOutboundOut(.head(head))).map {
self.callOutToDelegateFireAndForget(value: head, self.delegate.didSendRequestHead)
}.flatMap {
self.writeBody(request: request, context: context)
}.flatMap {
if let expectedBodyLength = self.expectedBodyLength, expectedBodyLength != self.actualBodyLength {
self.state = .end
let error = HTTPClientError.bodyLengthMismatch
self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError)
return context.eventLoop.makeFailedFuture(error)
}
context.eventLoop.assertInEventLoop()
return context.writeAndFlush(self.wrapOutboundOut(.end(nil)))
}.map {
Expand Down Expand Up @@ -836,6 +846,7 @@ extension TaskHandler: ChannelDuplexHandler {
}

return promise.futureResult.map {
self.actualBodyLength += part.readableBytes
Copy link
Contributor

Choose a reason for hiding this comment

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

  • assertion about the correct EL missing
  • test that would fail this assertion right now probably missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved the increment to where it is certainly on correct EL

self.callOutToDelegateFireAndForget(value: part, self.delegate.didSendRequestPart)
}
})
Expand Down
31 changes: 31 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2051,4 +2051,35 @@ class HTTPClientTests: XCTestCase {

XCTAssertNoThrow(try future.wait())
}

func testContentLengthTooLongFails() {
let url = self.defaultHTTPBinURLPrefix + "/post"
XCTAssertThrowsError(
try self.defaultClient.execute(request:
Request(url: url,
body: .stream(length: 10) { streamWriter in
streamWriter.write(.byteBuffer(ByteBuffer(string: "1")))
})).wait()) { error in
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())
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, do we even need this part? we already have a test that checks that, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, I think I know what you mean!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

}

// currently gets stuck because of #250 the server just never replies
func testContentLengthTooShortFails() {
let url = self.defaultHTTPBinURLPrefix + "/post"
let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n"
XCTAssertThrowsError(
try self.defaultClient.execute(request:
Request(url: url,
body: .stream(length: 1) { streamWriter in
streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong)))
})).wait()) { error in
XCTAssertEqual(error as! HTTPClientError, HTTPClientError.bodyLengthMismatch)
}
// 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())
Copy link
Contributor

Choose a reason for hiding this comment

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

similar here, we should validate request/connection number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

}
}