Skip to content

assume chunked on a stream with no length #247

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Jun 23, 2020
Merged

assume chunked on a stream with no length #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2383b65
assume chunked on zero-length stream
artemredkin Jun 13, 2020
084dee8
linux test and swift format
artemredkin Jun 13, 2020
d8fad2d
Merge branch 'master' into assume_chunked_on_zero_length_stream
artemredkin Jun 15, 2020
83a83e9
Merge branch 'master' into assume_chunked_on_zero_length_stream
weissi Jun 17, 2020
995a98b
Convenience methods for socket paths (#235)
dimitribouniol Jun 19, 2020
9a431d1
wip
artemredkin Jun 19, 2020
eaf203e
fix flaky testContentLengthTooLongFails test (#269)
artemredkin Jun 22, 2020
02b3ed9
All internal connection flow should be executed when shutting down (#…
artemredkin Jun 22, 2020
9f65902
fix missing connect timeout and make tests safer (#267)
artemredkin Jun 22, 2020
f6f99d2
Fix doc comment for redirectConfiguration (#266)
MaxDesiatov Jun 22, 2020
233f18b
Fixed an issue where redirects to socket path-based servers from any …
dimitribouniol Jun 22, 2020
29dc2c7
notify delegate about connect errors (#245)
artemredkin Jun 23, 2020
ac8b433
make all tests pass
artemredkin Jun 23, 2020
aad8fa6
Merge branch 'master' into assume_chunked_on_zero_length_stream
artemredkin Jun 23, 2020
08c7a33
swiftformat
artemredkin Jun 23, 2020
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
3 changes: 3 additions & 0 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
case traceRequestWithBody
case invalidHeaderFieldNames([String])
case bodyLengthMismatch
case incompatibleHeaders
}

private var code: Code
Expand Down Expand Up @@ -1019,4 +1020,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
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)
/// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`.
public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders)
}
16 changes: 12 additions & 4 deletions Sources/AsyncHTTPClient/RequestValidation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import NIOHTTP1
extension HTTPHeaders {
mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws {
// validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1)
if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") {
throw HTTPClientError.incompatibleHeaders
}

var transferEncoding: String?
var contentLength: Int?
let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() }
Expand All @@ -27,11 +31,11 @@ extension HTTPHeaders {
}

self.remove(name: "Transfer-Encoding")
self.remove(name: "Content-Length")

try self.validateFieldNames()

guard let body = body else {
self.remove(name: "Content-Length")
// if we don't have a body we might not need to send the Content-Length field
// https://tools.ietf.org/html/rfc7230#section-3.3.2
switch method {
Expand Down Expand Up @@ -60,11 +64,15 @@ extension HTTPHeaders {
}

if encodings.isEmpty {
guard let length = body.length else {
throw HTTPClientError.contentLengthMissing
if let length = body.length {
self.remove(name: "Content-Length")
contentLength = length
} else if !self.contains(name: "Content-Length") {
transferEncoding = "chunked"
}
contentLength = length
} else {
self.remove(name: "Content-Length")

transferEncoding = encodings.joined(separator: ", ")
if !encodings.contains("chunked") {
guard let length = body.length else {
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ extension HTTPClientTests {
("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher),
("testAllMethodsLog", testAllMethodsLog),
("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground),
("testUploadStreamingNoLength", testUploadStreamingNoLength),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a few more test cases here. Essentially we have quite a big matrix of possibilities:

HTTP Method kind User sets Body Expectation
.GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing nil Neither CL nor chunked
other nothing nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil CL=0
other content-length nil CL=0
.GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil chunked
other transfer-encoding: chunked nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) nil throws error
other CL & chunked (illegal) nil throws error
.GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil Neither CL nor chunked
other nothing not nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length not nil CL
other content-length not nil CL
.GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked not nil chunked
other transfer-encoding: chunked not nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) not nil throws error
other CL & chunked (illegal) not nil throws error

I think we need to test them systematically or else we'll get more bugs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin should we do this in this ticket or create an issue from it? I think it's important because the code looks tricky

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 was planning on doing it in this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing, thank you so much!!

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, I'm not sure some combinations are correct, for example:

Method Header Body Expectation
other nothing nil chunked

but there is this bit in https://tools.ietf.org/html/rfc7230#section-3.3.2:

   no Transfer-Encoding is sent and the request method defines a meaning
   for an enclosed payload body.  For example, a Content-Length header
   field is normally sent in a POST request even when the value is 0
   (indicating an empty payload body).

shouldn't this be CL=0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And, if users sets chunked but with nil body, we know there will be zero bytes, so we should default to Neither CL nor chunked for .GET and to CL=0 for other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And last point is when user sets nothing but provides a body, we again can do two things:
if body length known - use CL, otherwise - chunked

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin sorry, I made a number of C&P errors when building this table :). I agree with all the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should only expect chunked if the user has told us that they have a body but they didn't tell us the length.

We should set CL=0 if the user set body: nil UNLESS it's .GET, .HEAD, .DELETE, .CONNECT, .TRACE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@weissi done!

("testConnectErrorPropagatedToDelegate", testConnectErrorPropagatedToDelegate),
("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL),
("testContentLengthTooLongFails", testContentLengthTooLongFails),
Expand Down
31 changes: 31 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2374,6 +2374,37 @@ class HTTPClientTests: XCTestCase {
self.defaultClient = nil // so it doesn't get shut down again.
}

func testUploadStreamingNoLength() throws {
let server = NIOHTTP1TestServer(group: self.serverGroup)
let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup))
defer {
XCTAssertNoThrow(try client.syncShutdown())
XCTAssertNoThrow(try server.stop())
}

var request = try HTTPClient.Request(url: "http://localhost:\(server.serverPort)/")
request.body = .stream { writer in
writer.write(.byteBuffer(ByteBuffer(string: "1234")))
}

let future = client.execute(request: request)

switch try server.readInbound() {
case .head(let head):
XCTAssertEqual(head.headers["transfer-encoding"], ["chunked"])
default:
XCTFail("Unexpected part")
}

XCTAssertNoThrow(try server.readInbound()) // .body
XCTAssertNoThrow(try server.readInbound()) // .end

XCTAssertNoThrow(try server.writeOutbound(.head(.init(version: .init(major: 1, minor: 1), status: .ok))))
XCTAssertNoThrow(try server.writeOutbound(.end(nil)))

XCTAssertNoThrow(try future.wait())
}

func testConnectErrorPropagatedToDelegate() throws {
class TestDelegate: HTTPClientResponseDelegate {
typealias Response = Void
Expand Down
10 changes: 8 additions & 2 deletions Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,18 @@ extension RequestValidationTests {
("testContentLengthHeaderIsRemovedFromGETIfNoBody", testContentLengthHeaderIsRemovedFromGETIfNoBody),
("testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody", testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody),
("testContentLengthHeaderIsChangedIfBodyHasDifferentLength", testContentLengthHeaderIsChangedIfBodyHasDifferentLength),
("testChunkedEncodingDoesNotHaveContentLengthHeader", testChunkedEncodingDoesNotHaveContentLengthHeader),
("testTRACERequestMustNotHaveBody", testTRACERequestMustNotHaveBody),
("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody),
("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames),
("testValidHeaderFieldNames", testValidHeaderFieldNames),
("testMultipleContentLengthOnNilStreamLength", testMultipleContentLengthOnNilStreamLength),
("testNoHeadersNoBody", testNoHeadersNoBody),
("testNoHeadersHasBody", testNoHeadersHasBody),
("testContentLengthHeaderNoBody", testContentLengthHeaderNoBody),
("testContentLengthHeaderHasBody", testContentLengthHeaderHasBody),
("testTransferEncodingHeaderNoBody", testTransferEncodingHeaderNoBody),
("testTransferEncodingHeaderHasBody", testTransferEncodingHeaderHasBody),
("testBothHeadersNoBody", testBothHeadersNoBody),
("testBothHeadersHasBody", testBothHeadersHasBody),
]
}
}
214 changes: 182 additions & 32 deletions Tests/AsyncHTTPClientTests/RequestValidationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,32 +42,14 @@ class RequestValidationTests: XCTestCase {
XCTAssertEqual(headers.first(name: "Content-Length"), "200")
}

func testChunkedEncodingDoesNotHaveContentLengthHeader() {
var headers = HTTPHeaders([
("Content-Length", "200"),
("Transfer-Encoding", "chunked"),
])
var buffer = ByteBufferAllocator().buffer(capacity: 200)
buffer.writeBytes([UInt8](repeating: 12, count: 200))
XCTAssertNoThrow(try headers.validate(method: .PUT, body: .byteBuffer(buffer)))

// https://tools.ietf.org/html/rfc7230#section-3.3.2
// A sender MUST NOT send a Content-Length header field in any message
// that contains a Transfer-Encoding header field.

XCTAssertNil(headers.first(name: "Content-Length"))
XCTAssertEqual(headers.first(name: "Transfer-Encoding"), "chunked")
}

func testTRACERequestMustNotHaveBody() {
var headers = HTTPHeaders([
("Content-Length", "200"),
("Transfer-Encoding", "chunked"),
])
var buffer = ByteBufferAllocator().buffer(capacity: 200)
buffer.writeBytes([UInt8](repeating: 12, count: 200))
XCTAssertThrowsError(try headers.validate(method: .TRACE, body: .byteBuffer(buffer))) {
XCTAssertEqual($0 as? HTTPClientError, .traceRequestWithBody)
for header in [("Content-Length", "200"), ("Transfer-Encoding", "chunked")] {
var headers = HTTPHeaders([header])
var buffer = ByteBufferAllocator().buffer(capacity: 200)
buffer.writeBytes([UInt8](repeating: 12, count: 200))
XCTAssertThrowsError(try headers.validate(method: .TRACE, body: .byteBuffer(buffer))) {
XCTAssertEqual($0 as? HTTPClientError, .traceRequestWithBody)
}
}
}

Expand Down Expand Up @@ -105,13 +87,181 @@ class RequestValidationTests: XCTestCase {
XCTAssertNoThrow(try headers.validate(method: .GET, body: nil))
}

func testMultipleContentLengthOnNilStreamLength() {
var headers = HTTPHeaders([("Content-Length", "1"), ("Content-Length", "2")])
var buffer = ByteBufferAllocator().buffer(capacity: 10)
buffer.writeBytes([UInt8](repeating: 12, count: 10))
let body: HTTPClient.Body = .stream { writer in
writer.write(.byteBuffer(buffer))
// MARK: - Content-Length/Transfer-Encoding Matrix

// Method kind User sets Body Expectation
// ----------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing nil Neither CL nor chunked
// other nothing nil CL=0
func testNoHeadersNoBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] {
var headers: HTTPHeaders = .init()
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init()
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertEqual(headers["content-length"].first, "0")
XCTAssertFalse(headers["transfer-encoding"].contains("chunked"))
}
}

// Method kind User sets Body Expectation
// --------------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil CL or chunked
// other nothing not nil CL or chunked
func testNoHeadersHasBody() throws {
// Body length is known
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] {
var headers: HTTPHeaders = .init()
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertEqual(headers["content-length"].first, "1")
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}

// Body length is _not_ known
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] {
var headers: HTTPHeaders = .init()
let body: HTTPClient.Body = .stream { writer in
writer.write(.byteBuffer(ByteBuffer(bytes: [0])))
}
XCTAssertNoThrow(try headers.validate(method: method, body: body))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].contains("chunked"))
}

// Body length is known
for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init()
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertEqual(headers["content-length"].first, "1")
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}

// Body length is _not_ known
for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init()
let body: HTTPClient.Body = .stream { writer in
writer.write(.byteBuffer(ByteBuffer(bytes: [0])))
}
XCTAssertNoThrow(try headers.validate(method: method, body: body))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].contains("chunked"))
}
}

// Method kind User sets Body Expectation
// ------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil Neither CL nor chunked
// other content-length nil CL=0
func testContentLengthHeaderNoBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] {
var headers: HTTPHeaders = .init([("Content-Length", "1")])
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Content-Length", "1")])
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertEqual(headers["content-length"].first, "0")
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}
}

// Method kind User sets Body Expectation
// --------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT content-length not nil CL=1
// other content-length nit nil CL=1
func testContentLengthHeaderHasBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] {
var headers: HTTPHeaders = .init([("Content-Length", "1")])
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertEqual(headers["content-length"].first, "1")
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Content-Length", "1")])
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertEqual(headers["content-length"].first, "1")
XCTAssertTrue(headers["transfer-encoding"].isEmpty)
}
}

// Method kind User sets Body Expectation
// ------------------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil nil
// other transfer-encoding: chunked nil nil
func testTransferEncodingHeaderNoBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] {
var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")])
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertFalse(headers["transfer-encoding"].contains("chunked"))
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")])
XCTAssertNoThrow(try headers.validate(method: method, body: nil))
XCTAssertEqual(headers["content-length"].first, "0")
XCTAssertFalse(headers["transfer-encoding"].contains("chunked"))
}
}

// Method kind User sets Body Expectation
// --------------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT transfer-encoding: chunked not nil chunked
// other transfer-encoding: chunked not nil chunked
func testTransferEncodingHeaderHasBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT] {
var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")])
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].contains("chunked"))
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Transfer-Encoding", "chunked")])
XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
XCTAssertTrue(headers["content-length"].isEmpty)
XCTAssertTrue(headers["transfer-encoding"].contains("chunked"))
}
}

// Method kind User sets Body Expectation
// ---------------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) nil throws error
// other CL & chunked (illegal) nil throws error
func testBothHeadersNoBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] {
var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")])
XCTAssertThrowsError(try headers.validate(method: method, body: nil))
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")])
XCTAssertThrowsError(try headers.validate(method: method, body: nil))
}
}

// Method kind User sets Body Expectation
// -------------------------------------------------------------------------------------------
// .GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) not nil throws error
// other CL & chunked (illegal) not nil throws error
func testBothHeadersHasBody() throws {
for method: HTTPMethod in [.GET, .HEAD, .DELETE, .CONNECT, .TRACE] {
var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")])
XCTAssertThrowsError(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
}

for method: HTTPMethod in [.POST, .PUT] {
var headers: HTTPHeaders = .init([("Content-Length", "1"), ("Transfer-Encoding", "chunked")])
XCTAssertThrowsError(try headers.validate(method: method, body: .byteBuffer(ByteBuffer(bytes: [0]))))
}
XCTAssertThrowsError(try headers.validate(method: .PUT, body: body))
}
}