From 6145d2a40a129e622d402de0951d3c46d8d3becd Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Tue, 31 Mar 2020 17:57:43 +0200 Subject: [PATCH 1/2] HTTPRequest without body: Content-Length shall not be send --- Sources/AsyncHTTPClient/HTTPClient.swift | 3 + Sources/AsyncHTTPClient/HTTPHandler.swift | 2 +- .../AsyncHTTPClient/RequestValidation.swift | 56 +++++++++----- .../RequestValidationTests+XCTest.swift | 35 +++++++++ .../RequestValidationTests.swift | 73 +++++++++++++++++++ Tests/LinuxMain.swift | 1 + 6 files changed, 152 insertions(+), 18 deletions(-) create mode 100644 Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift create mode 100644 Tests/AsyncHTTPClientTests/RequestValidationTests.swift diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 548f038ad..f736d9579 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -754,6 +754,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case redirectLimitReached case redirectCycleDetected case uncleanShutdown + case traceRequestWithBody } private var code: Code @@ -798,4 +799,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { public static let redirectCycleDetected = HTTPClientError(code: .redirectCycleDetected) /// Unclean shutdown public static let uncleanShutdown = HTTPClientError(code: .uncleanShutdown) + /// A body was sent in a request with method TRACE + public static let traceRequestWithBody = HTTPClientError(code: .traceRequestWithBody) } diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 921c1e678..b77ab02d1 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -771,7 +771,7 @@ extension TaskHandler: ChannelDuplexHandler { } do { - try headers.validate(body: request.body) + try headers.validate(method: request.method, body: request.body) } catch { promise?.fail(error) context.fireErrorCaught(error) diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index be5d7f10f..0802da0ad 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -16,7 +16,7 @@ import NIO import NIOHTTP1 extension HTTPHeaders { - mutating func validate(body: HTTPClient.Body?) throws { + 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) var transferEncoding: String? var contentLength: Int? @@ -29,33 +29,55 @@ extension HTTPHeaders { self.remove(name: "Transfer-Encoding") self.remove(name: "Content-Length") - if let body = body { - guard (encodings.filter { $0 == "chunked" }.count <= 1) else { - throw HTTPClientError.chunkedSpecifiedMultipleTimes + guard let body = body else { + // 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 { + case .GET, .HEAD, .DELETE, .CONNECT, .TRACE: + // A user agent SHOULD NOT send a Content-Length header field when the request + // message does not contain a payload body and the method semantics do not + // anticipate such a body. + return + default: + // A user agent SHOULD send a Content-Length in a request message when + // no Transfer-Encoding is sent and the request method defines a meaning + // for an enclosed payload body. + self.add(name: "Content-Length", value: "0") + return } + } + + if case .TRACE = method { + // A client MUST NOT send a message body in a TRACE request. + // https://tools.ietf.org/html/rfc7230#section-4.3.8 + throw HTTPClientError.traceRequestWithBody + } - if encodings.isEmpty { + guard (encodings.filter { $0 == "chunked" }.count <= 1) else { + throw HTTPClientError.chunkedSpecifiedMultipleTimes + } + + if encodings.isEmpty { + guard let length = body.length else { + throw HTTPClientError.contentLengthMissing + } + contentLength = length + } else { + transferEncoding = encodings.joined(separator: ", ") + if !encodings.contains("chunked") { guard let length = body.length else { throw HTTPClientError.contentLengthMissing } contentLength = length - } else { - transferEncoding = encodings.joined(separator: ", ") - if !encodings.contains("chunked") { - guard let length = body.length else { - throw HTTPClientError.contentLengthMissing - } - contentLength = length - } } - } else { - contentLength = 0 } + // add headers if required if let enc = transferEncoding { self.add(name: "Transfer-Encoding", value: enc) - } - if let length = contentLength { + } else if let length = contentLength { + // A sender MUST NOT send a Content-Length header field in any message + // that contains a Transfer-Encoding header field. self.add(name: "Content-Length", value: String(length)) } } diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift new file mode 100644 index 000000000..b6145a4b2 --- /dev/null +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift @@ -0,0 +1,35 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// +// +// RequestValidationTests+XCTest.swift +// +import XCTest + +/// +/// NOTE: This file was generated by generate_linux_tests.rb +/// +/// Do NOT edit this file directly as it will be regenerated automatically when needed. +/// + +extension RequestValidationTests { + static var allTests: [(String, (RequestValidationTests) -> () throws -> Void)] { + return [ + ("testContentLengthHeaderIsRemovedFromGETIfNoBody", testContentLengthHeaderIsRemovedFromGETIfNoBody), + ("testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody", testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody), + ("testContentLengthHeaderIsChangedIfBodyHasDifferentLength", testContentLengthHeaderIsChangedIfBodyHasDifferentLength), + ("testChunkedEncodingDoesNotHaveContentLengthHeader", testChunkedEncodingDoesNotHaveContentLengthHeader), + ("testTRACERequestMustNotHaveBody", testTRACERequestMustNotHaveBody), + ] + } +} diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift new file mode 100644 index 000000000..72dfa3da8 --- /dev/null +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift @@ -0,0 +1,73 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import AsyncHTTPClient +import NIO +import NIOHTTP1 +import XCTest + +class RequestValidationTests: XCTestCase { + func testContentLengthHeaderIsRemovedFromGETIfNoBody() { + var headers = HTTPHeaders([("Content-Length", "0")]) + XCTAssertNoThrow(try headers.validate(method: .GET, body: .none)) + XCTAssertNil(headers.first(name: "Content-Length")) + } + + func testContentLengthHeaderIsAddedToPOSTAndPUTWithNoBody() { + var putHeaders = HTTPHeaders() + XCTAssertNoThrow(try putHeaders.validate(method: .PUT, body: .none)) + XCTAssertEqual(putHeaders.first(name: "Content-Length"), "0") + + var postHeaders = HTTPHeaders() + XCTAssertNoThrow(try postHeaders.validate(method: .POST, body: .none)) + XCTAssertEqual(postHeaders.first(name: "Content-Length"), "0") + } + + func testContentLengthHeaderIsChangedIfBodyHasDifferentLength() { + var headers = HTTPHeaders([("Content-Length", "0")]) + var buffer = ByteBufferAllocator().buffer(capacity: 200) + buffer.writeBytes([UInt8](repeating: 12, count: 200)) + XCTAssertNoThrow(try headers.validate(method: .PUT, body: .byteBuffer(buffer))) + 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) + } + } +} diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index b46ac9e2f..93e594a06 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -29,5 +29,6 @@ import XCTest testCase(HTTPClientCookieTests.allTests), testCase(HTTPClientInternalTests.allTests), testCase(HTTPClientTests.allTests), + testCase(RequestValidationTests.allTests), ]) #endif From 4e12944d8cb161fe22df399d469bbc0cc1b18991 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Tue, 31 Mar 2020 18:53:12 +0200 Subject: [PATCH 2/2] Verify field names comply with RFC7230 --- Sources/AsyncHTTPClient/HTTPClient.swift | 3 ++ .../AsyncHTTPClient/RequestValidation.swift | 40 ++++++++++++++++++- .../HTTPClientTests.swift | 18 +++------ .../RequestValidationTests+XCTest.swift | 3 ++ .../RequestValidationTests.swift | 36 ++++++++++++++++- 5 files changed, 86 insertions(+), 14 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index f736d9579..eb9833774 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -755,6 +755,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case redirectCycleDetected case uncleanShutdown case traceRequestWithBody + case invalidHeaderFieldNames([String]) } private var code: Code @@ -801,4 +802,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { public static let uncleanShutdown = HTTPClientError(code: .uncleanShutdown) /// A body was sent in a request with method TRACE public static let traceRequestWithBody = HTTPClientError(code: .traceRequestWithBody) + /// Header field names contain invalid characters + public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) } } diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index 0802da0ad..e5061a99a 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -2,7 +2,7 @@ // // This source file is part of the AsyncHTTPClient open source project // -// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Copyright (c) 2018-2020 Apple Inc. and the AsyncHTTPClient project authors // Licensed under Apache License v2.0 // // See LICENSE.txt for license information @@ -29,6 +29,8 @@ extension HTTPHeaders { self.remove(name: "Transfer-Encoding") self.remove(name: "Content-Length") + try self.validateFieldNames() + guard let body = body else { // 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 @@ -81,4 +83,40 @@ extension HTTPHeaders { self.add(name: "Content-Length", value: String(length)) } } + + func validateFieldNames() throws { + let invalidFieldNames = self.compactMap { (name, _) -> String? in + let satisfy = name.utf8.allSatisfy { (char) -> Bool in + switch char { + case UInt8(ascii: "a")...UInt8(ascii: "z"), + UInt8(ascii: "A")...UInt8(ascii: "Z"), + UInt8(ascii: "0")...UInt8(ascii: "9"), + UInt8(ascii: "!"), + UInt8(ascii: "#"), + UInt8(ascii: "$"), + UInt8(ascii: "%"), + UInt8(ascii: "&"), + UInt8(ascii: "'"), + UInt8(ascii: "*"), + UInt8(ascii: "+"), + UInt8(ascii: "-"), + UInt8(ascii: "."), + UInt8(ascii: "^"), + UInt8(ascii: "_"), + UInt8(ascii: "`"), + UInt8(ascii: "|"), + UInt8(ascii: "~"): + return true + default: + return false + } + } + + return satisfy ? nil : name + } + + guard invalidFieldNames.count == 0 else { + throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames) + } + } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 731586db9..986194761 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -830,8 +830,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) @@ -860,8 +859,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) @@ -887,8 +885,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) @@ -916,8 +913,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) @@ -950,8 +946,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) @@ -1166,8 +1161,7 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(XCTAssertEqual(.head(.init(version: .init(major: 1, minor: 1), method: .GET, uri: "/foo", - headers: HTTPHeaders([("Host", "localhost"), - ("Content-Length", "0")]))), + headers: HTTPHeaders([("Host", "localhost")]))), try web.readInbound())) XCTAssertNoThrow(XCTAssertEqual(.end(nil), try web.readInbound())) diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift index b6145a4b2..b4d88344c 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift @@ -30,6 +30,9 @@ extension RequestValidationTests { ("testContentLengthHeaderIsChangedIfBodyHasDifferentLength", testContentLengthHeaderIsChangedIfBodyHasDifferentLength), ("testChunkedEncodingDoesNotHaveContentLengthHeader", testChunkedEncodingDoesNotHaveContentLengthHeader), ("testTRACERequestMustNotHaveBody", testTRACERequestMustNotHaveBody), + ("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody), + ("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames), + ("testValidHeaderFieldNames", testValidHeaderFieldNames), ] } } diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift index 72dfa3da8..5f4db05b7 100644 --- a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift @@ -2,7 +2,7 @@ // // This source file is part of the AsyncHTTPClient open source project // -// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Copyright (c) 2020 Apple Inc. and the AsyncHTTPClient project authors // Licensed under Apache License v2.0 // // See LICENSE.txt for license information @@ -70,4 +70,38 @@ class RequestValidationTests: XCTestCase { XCTAssertEqual($0 as? HTTPClientError, .traceRequestWithBody) } } + + func testGET_HEAD_DELETE_CONNECTRequestCanHaveBody() { + var buffer = ByteBufferAllocator().buffer(capacity: 100) + buffer.writeBytes([UInt8](repeating: 12, count: 100)) + + // GET, HEAD, DELETE and CONNECT requests can have a payload. (though uncommon) + let allowedMethods: [HTTPMethod] = [.GET, .HEAD, .DELETE, .CONNECT] + var headers = HTTPHeaders() + for method in allowedMethods { + XCTAssertNoThrow(try headers.validate(method: method, body: .byteBuffer(buffer))) + } + } + + func testInvalidHeaderFieldNames() { + var headers = HTTPHeaders([ + ("Content-Length", "200"), + ("User Agent", "Haha"), + ]) + + XCTAssertThrowsError(try headers.validate(method: .GET, body: nil)) { error in + XCTAssertEqual(error as? HTTPClientError, HTTPClientError.invalidHeaderFieldNames(["User Agent"])) + } + } + + func testValidHeaderFieldNames() { + var headers = HTTPHeaders([ + ("abcdefghijklmnopqrstuvwxyz", "Haha"), + ("ABCDEFGHIJKLMNOPQRSTUVWXYZ", "Haha"), + ("0123456789", "Haha"), + ("!#$%&'*+-.^_`|~", "Haha"), + ]) + + XCTAssertNoThrow(try headers.validate(method: .GET, body: nil)) + } }