diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index fba012b09..96db4dfbd 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -1024,6 +1024,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case uncleanShutdown case traceRequestWithBody case invalidHeaderFieldNames([String]) + case invalidHeaderFieldValues([String]) case bodyLengthMismatch case writeAfterRequestSent @available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.") @@ -1092,6 +1093,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { return "Trace request with body" case .invalidHeaderFieldNames: return "Invalid header field names" + case .invalidHeaderFieldValues: + return "Invalid header field values" case .bodyLengthMismatch: return "Body length mismatch" case .writeAfterRequestSent: @@ -1158,6 +1161,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { 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)) } + /// Header field values contain invalid characters. + public static func invalidHeaderFieldValues(_ values: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldValues(values)) } /// Body length is not equal to `Content-Length`. public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch) /// Body part was written after request was fully sent. diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift index e23c35423..87224a3b2 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -21,6 +21,7 @@ extension HTTPHeaders { bodyLength: RequestBodyLength ) throws -> RequestFramingMetadata { try self.validateFieldNames() + try self.validateFieldValues() if case .TRACE = method { switch bodyLength { @@ -80,6 +81,56 @@ extension HTTPHeaders { } } + private func validateFieldValues() throws { + let invalidValues = self.compactMap { _, value -> String? in + let satisfy = value.utf8.allSatisfy { char -> Bool in + /// Validates a byte of a given header field value against the definition in RFC 9110. + /// + /// The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid + /// characters as the following: + /// + /// ``` + /// field-value = *field-content + /// field-content = field-vchar + /// [ 1*( SP / HTAB / field-vchar ) field-vchar ] + /// field-vchar = VCHAR / obs-text + /// obs-text = %x80-FF + /// ``` + /// + /// Additionally, it makes the following note: + /// + /// "Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the + /// varying ways that implementations might parse and interpret those characters; a recipient + /// of CR, LF, or NUL within a field value MUST either reject the message or replace each of + /// those characters with SP before further processing or forwarding of that message. Field + /// values containing other CTL characters are also invalid; however, recipients MAY retain + /// such characters for the sake of robustness when they appear within a safe context (e.g., + /// an application-specific quoted string that will not be processed by any downstream HTTP + /// parser)." + /// + /// As we cannot guarantee the context is safe, this code will reject all ASCII control characters + /// directly _except_ for HTAB, which is explicitly allowed. + switch char { + case UInt8(ascii: "\t"): + // HTAB, explicitly allowed. + return true + case 0...0x1f, 0x7F: + // ASCII control character, forbidden. + return false + default: + // Printable or non-ASCII, allowed. + return true + } + } + + return satisfy ? nil : value + } + + guard invalidValues.count == 0 else { + throw HTTPClientError.invalidHeaderFieldValues(invalidValues) + } + } + private mutating func setTransportFraming( method: HTTPMethod, bodyLength: RequestBodyLength diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 2427d6cbf..dc154aa5e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -144,6 +144,10 @@ extension HTTPClientTests { ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail), ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), + ("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1), + ("testRejectsInvalidCharactersInHeaderFieldNames_http2", testRejectsInvalidCharactersInHeaderFieldNames_http2), + ("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1), + ("testRejectsInvalidCharactersInHeaderFieldValues_http2", testRejectsInvalidCharactersInHeaderFieldValues_http2), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 3c275c5eb..d03cdb1ea 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3468,4 +3468,113 @@ class HTTPClientTests: XCTestCase { let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) XCTAssertNoThrow(try httpClient.shutdown().wait()) } + + func testRejectsInvalidCharactersInHeaderFieldNames_http1() throws { + try self._rejectsInvalidCharactersInHeaderFieldNames(mode: .http1_1(ssl: true)) + } + + func testRejectsInvalidCharactersInHeaderFieldNames_http2() throws { + try self._rejectsInvalidCharactersInHeaderFieldNames(mode: .http2(compress: false)) + } + + private func _rejectsInvalidCharactersInHeaderFieldNames(mode: HTTPBin.Mode) throws { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } + let client = HTTPClient(eventLoopGroupProvider: .shared(group)) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let bin = HTTPBin(mode) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + // The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid + // characters as the following: + // + // ``` + // field-name = token + // + // token = 1*tchar + // + // tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" + // / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" + // / DIGIT / ALPHA + // ; any VCHAR, except delimiters + let weirdAllowedFieldName = "!#$%&'*+-.^_`|~0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: weirdAllowedFieldName, value: "present") + + // This should work fine. + let response = try client.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + // Now, let's confirm all other bytes are rejected. We want to stay within the ASCII space as the HTTPHeaders type will forbid anything else. + for byte in UInt8(0)...UInt8(127) { + // Skip bytes that we already believe are allowed. + if weirdAllowedFieldName.utf8.contains(byte) { + continue + } + let forbiddenFieldName = weirdAllowedFieldName + String(decoding: [byte], as: UTF8.self) + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: forbiddenFieldName, value: "present") + + XCTAssertThrowsError(try client.execute(request: request).wait()) { error in + XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldNames([forbiddenFieldName])) + } + } + } + + func testRejectsInvalidCharactersInHeaderFieldValues_http1() throws { + try self._rejectsInvalidCharactersInHeaderFieldValues(mode: .http1_1(ssl: true)) + } + + func testRejectsInvalidCharactersInHeaderFieldValues_http2() throws { + try self._rejectsInvalidCharactersInHeaderFieldValues(mode: .http2(compress: false)) + } + + private func _rejectsInvalidCharactersInHeaderFieldValues(mode: HTTPBin.Mode) throws { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } + let client = HTTPClient(eventLoopGroupProvider: .shared(group)) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let bin = HTTPBin(mode) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + // We reject all ASCII control characters except HTAB and tolerate everything else. + let weirdAllowedFieldValue = "!\" \t#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: weirdAllowedFieldValue) + + // This should work fine. + let response = try client.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + // Now, let's confirm all other bytes in the ASCII range ar rejected + for byte in UInt8(0)...UInt8(127) { + // Skip bytes that we already believe are allowed. + if weirdAllowedFieldValue.utf8.contains(byte) { + continue + } + let forbiddenFieldValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self) + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: forbiddenFieldValue) + + XCTAssertThrowsError(try client.execute(request: request).wait()) { error in + XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldValues([forbiddenFieldValue])) + } + } + + // All the bytes outside the ASCII range are fine though. + for byte in UInt8(128)...UInt8(255) { + let evenWeirderAllowedValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self) + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: evenWeirderAllowedValue) + + // This should work fine. + let response = try client.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + } + } }