From 8e7c9986df3f674ce5ea4c2e7ab937b9b276c4d8 Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Wed, 13 Aug 2025 12:16:57 +0100 Subject: [PATCH 1/6] Add test for desired behaviour --- Sources/NIOHTTP1/HTTPPipelineSetup.swift | 53 +++++++++++++++++++ .../HTTPHeaderValidationTests.swift | 37 +++++++++++++ 2 files changed, 90 insertions(+) diff --git a/Sources/NIOHTTP1/HTTPPipelineSetup.swift b/Sources/NIOHTTP1/HTTPPipelineSetup.swift index a7645377d6d..7e2bc8337ea 100644 --- a/Sources/NIOHTTP1/HTTPPipelineSetup.swift +++ b/Sources/NIOHTTP1/HTTPPipelineSetup.swift @@ -913,12 +913,65 @@ extension ChannelPipeline.SynchronousOperations { ) } + /// Configure a `ChannelPipeline` for use as a HTTP server. + /// + /// This function knows how to set up all first-party HTTP channel handlers appropriately + /// for server use. It supports the following features: + /// + /// 1. Providing assistance handling clients that pipeline HTTP requests, using the + /// `HTTPServerPipelineHandler`. + /// 2. Supporting HTTP upgrade, using the `HTTPServerUpgradeHandler`. + /// 3. Providing assistance handling protocol errors. + /// 4. Validating outbound header fields to protect against response splitting attacks. + /// 5. Specifying whether the header validation should return a response + /// + /// This method will likely be extended in future with more support for other first-party + /// features. + /// + /// - important: This **must** be called on the Channel's event loop. + /// - Parameters: + /// - position: Where in the pipeline to add the HTTP server handlers, defaults to `.last`. + /// - pipelining: Whether to provide assistance handling HTTP clients that pipeline + /// their requests. Defaults to `true`. If `false`, users will need to handle + /// clients that pipeline themselves. + /// - upgrade: Whether to add a `HTTPServerUpgradeHandler` to the pipeline, configured for + /// HTTP upgrade. Defaults to `nil`, which will not add the handler to the pipeline. If + /// provided should be a tuple of an array of `HTTPServerProtocolUpgrader` and the upgrade + /// completion handler. See the documentation on `HTTPServerUpgradeHandler` for more + /// details. + /// - errorHandling: Whether to provide assistance handling protocol errors (e.g. + /// failure to parse the HTTP request) by sending 400 errors. Defaults to `true`. + /// - headerValidation: Whether to validate outbound request headers to confirm that they meet + /// spec compliance. Defaults to `true`. + /// - encoderConfiguration: The configuration for the ``HTTPRequestEncoder``. + /// - headerValidationResponse: Whether to return a response when the header validation fails. + /// - Throws: If the pipeline could not be configured. + public func configureHTTPServerPipeline( + position: ChannelPipeline.SynchronousOperations.Position = .last, + withPipeliningAssistance pipelining: Bool = true, + withServerUpgrade upgrade: NIOHTTPServerUpgradeConfiguration? = nil, + withErrorHandling errorHandling: Bool = true, + withOutboundHeaderValidation headerValidation: Bool = true, + withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init(), + withOutboundHeaderValidationRepsonse headerValidationResponse: Bool + ) throws { + try self._configureHTTPServerPipeline( + position: position, + withPipeliningAssistance: pipelining, + withServerUpgrade: upgrade, + withErrorHandling: errorHandling, + withOutboundHeaderValidation: headerValidation, + withOutboundHeaderValidationRepsonse: headerValidationResponse, withEncoderConfiguration: encoderConfiguration + ) + } + private func _configureHTTPServerPipeline( position: ChannelPipeline.SynchronousOperations.Position = .last, withPipeliningAssistance pipelining: Bool = true, withServerUpgrade upgrade: NIOHTTPServerUpgradeConfiguration? = nil, withErrorHandling errorHandling: Bool = true, withOutboundHeaderValidation headerValidation: Bool = true, + withOutboundHeaderValidationRepsonse headerValidationResponse: Bool = true, withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init() ) throws { self.eventLoop.assertInEventLoop() diff --git a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift index 346a64a8591..738c30bd1cc 100644 --- a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift @@ -635,6 +635,43 @@ final class HTTPHeaderValidationTests: XCTestCase { XCTAssertEqual(maybeReceivedHeadBytes, toleratedRequestBytes) XCTAssertEqual(maybeReceivedTrailerBytes, toleratedTrailerBytes) } + + func testBadRequestResponseIsReturnedIfHeadersInvalidAndConfiguredToDoSo() throws { + let channel = EmbeddedChannel() + try channel.pipeline.syncOperations.configureHTTPServerPipeline(withOutboundHeaderValidationRepsonse: true) + try channel.primeForResponse() + + func assertReadHead(from channel: EmbeddedChannel) throws { + if case .head = try channel.readInbound(as: HTTPServerRequestPart.self) { + () + } else { + XCTFail("Expected 'head'") + } + } + + func assertReadEnd(from channel: EmbeddedChannel) throws { + if case .end = try channel.readInbound(as: HTTPServerRequestPart.self) { + () + } else { + XCTFail("Expected 'end'") + } + } + + // Read the first request. + try assertReadHead(from: channel) + try assertReadEnd(from: channel) + XCTAssertNil(try channel.readInbound(as: HTTPServerRequestPart.self)) + + // Respond with bad headers; they should cause an error and result in the rest of the + // response being dropped. + let head = HTTPResponseHead(version: .http1_1, status: .ok, headers: [":pseudo-header": "not-here"]) + XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.head(head))) + XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) + XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.body(.byteBuffer(ByteBuffer())))) + XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) + XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.end(nil))) + XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) + } } extension EmbeddedChannel { From 355101ee700c8f815dd3060f3f3e40dd22ca6db0 Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Wed, 13 Aug 2025 12:27:23 +0100 Subject: [PATCH 2/6] Get test passing --- Sources/NIOHTTP1/HTTPHeaderValidator.swift | 15 +++++++++++++++ Sources/NIOHTTP1/HTTPPipelineSetup.swift | 4 ++-- .../HTTPHeaderValidationTests.swift | 16 +++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/Sources/NIOHTTP1/HTTPHeaderValidator.swift b/Sources/NIOHTTP1/HTTPHeaderValidator.swift index 11eeb8b3049..73979ff5136 100644 --- a/Sources/NIOHTTP1/HTTPHeaderValidator.swift +++ b/Sources/NIOHTTP1/HTTPHeaderValidator.swift @@ -71,9 +71,16 @@ public final class NIOHTTPResponseHeadersValidator: ChannelOutboundHandler, Remo } private var state: State + private let sendResponseOnInvalidHeader: Bool public init() { self.state = .validating + self.sendResponseOnInvalidHeader = false + } + + public init(sendResponseOnInvalidHeader: Bool) { + self.state = .validating + self.sendResponseOnInvalidHeader = sendResponseOnInvalidHeader } public func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise?) { @@ -82,6 +89,14 @@ public final class NIOHTTPResponseHeadersValidator: ChannelOutboundHandler, Remo if head.headers.areValidToSend { context.write(data, promise: promise) } else { + // We won't write another header since we drop them going forward to write + // out a response if configured to do so + if self.sendResponseOnInvalidHeader { + let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")]) + let head = HTTPResponseHead(version: .http1_1, status: .badRequest, headers: headers) + context.write(Self.wrapOutboundOut(.head(head)), promise: nil) + context.writeAndFlush(Self.wrapOutboundOut(.end(nil)), promise: nil) + } self.state = .dropping promise?.fail(HTTPParserError.invalidHeaderToken) context.fireErrorCaught(HTTPParserError.invalidHeaderToken) diff --git a/Sources/NIOHTTP1/HTTPPipelineSetup.swift b/Sources/NIOHTTP1/HTTPPipelineSetup.swift index 7e2bc8337ea..b9d417dc270 100644 --- a/Sources/NIOHTTP1/HTTPPipelineSetup.swift +++ b/Sources/NIOHTTP1/HTTPPipelineSetup.swift @@ -971,7 +971,7 @@ extension ChannelPipeline.SynchronousOperations { withServerUpgrade upgrade: NIOHTTPServerUpgradeConfiguration? = nil, withErrorHandling errorHandling: Bool = true, withOutboundHeaderValidation headerValidation: Bool = true, - withOutboundHeaderValidationRepsonse headerValidationResponse: Bool = true, + withOutboundHeaderValidationRepsonse headerValidationResponse: Bool = false, withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init() ) throws { self.eventLoop.assertInEventLoop() @@ -986,7 +986,7 @@ extension ChannelPipeline.SynchronousOperations { } if headerValidation { - handlers.append(NIOHTTPResponseHeadersValidator()) + handlers.append(NIOHTTPResponseHeadersValidator(sendResponseOnInvalidHeader: headerValidationResponse)) } if errorHandling { diff --git a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift index 738c30bd1cc..ba7d64ec48a 100644 --- a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift @@ -666,7 +666,21 @@ final class HTTPHeaderValidationTests: XCTestCase { // response being dropped. let head = HTTPResponseHead(version: .http1_1, status: .ok, headers: [":pseudo-header": "not-here"]) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.head(head))) - XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) + + // We expect exactly one ByteBuffer in the output. + guard var written = try channel.readOutbound(as: ByteBuffer.self) else { + XCTFail("No writes") + return + } + + XCTAssertNoThrow(XCTAssertNil(try channel.readOutbound())) + + // Check the response. + assertResponseIs( + response: written.readString(length: written.readableBytes)!, + expectedResponseLine: "HTTP/1.1 400 Bad Request", + expectedResponseHeaders: ["Connection: close", "Content-Length: 0"] + ) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.body(.byteBuffer(ByteBuffer())))) XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.end(nil))) From 50fca337a4c3f0bf6062f3782c632460662c3ed3 Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Wed, 13 Aug 2025 12:36:40 +0100 Subject: [PATCH 3/6] Close the channel --- Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift index ba7d64ec48a..3416214baeb 100644 --- a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift @@ -663,7 +663,7 @@ final class HTTPHeaderValidationTests: XCTestCase { XCTAssertNil(try channel.readInbound(as: HTTPServerRequestPart.self)) // Respond with bad headers; they should cause an error and result in the rest of the - // response being dropped. + // response being dropped, but a fallback response being sent let head = HTTPResponseHead(version: .http1_1, status: .ok, headers: [":pseudo-header": "not-here"]) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.head(head))) @@ -685,6 +685,8 @@ final class HTTPHeaderValidationTests: XCTestCase { XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.end(nil))) XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self)) + + _ = try? channel.finish() } } From 5991c3001854d598806e37ab37d0dead8300b0fc Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Wed, 13 Aug 2025 18:58:28 +0100 Subject: [PATCH 4/6] Return a 500 error instead --- Sources/NIOHTTP1/HTTPHeaderValidator.swift | 2 +- Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/NIOHTTP1/HTTPHeaderValidator.swift b/Sources/NIOHTTP1/HTTPHeaderValidator.swift index 73979ff5136..ce39be82753 100644 --- a/Sources/NIOHTTP1/HTTPHeaderValidator.swift +++ b/Sources/NIOHTTP1/HTTPHeaderValidator.swift @@ -93,7 +93,7 @@ public final class NIOHTTPResponseHeadersValidator: ChannelOutboundHandler, Remo // out a response if configured to do so if self.sendResponseOnInvalidHeader { let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")]) - let head = HTTPResponseHead(version: .http1_1, status: .badRequest, headers: headers) + let head = HTTPResponseHead(version: .http1_1, status: .internalServerError, headers: headers) context.write(Self.wrapOutboundOut(.head(head)), promise: nil) context.writeAndFlush(Self.wrapOutboundOut(.end(nil)), promise: nil) } diff --git a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift index 3416214baeb..f08ba2c8a3f 100644 --- a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift @@ -678,7 +678,7 @@ final class HTTPHeaderValidationTests: XCTestCase { // Check the response. assertResponseIs( response: written.readString(length: written.readableBytes)!, - expectedResponseLine: "HTTP/1.1 400 Bad Request", + expectedResponseLine: "HTTP/1.1 500 Internal Server Error", expectedResponseHeaders: ["Connection: close", "Content-Length: 0"] ) XCTAssertThrowsError(try channel.writeOutbound(HTTPServerResponsePart.body(.byteBuffer(ByteBuffer())))) From b2595970b99184cb8cc9085c26d5e5c10a14d402 Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Wed, 13 Aug 2025 20:06:16 +0100 Subject: [PATCH 5/6] Add initial configuration --- Sources/NIOHTTP1/HTTPPipelineSetup.swift | 23 ++++++++++++++----- .../HTTPHeaderValidationTests.swift | 4 +++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/Sources/NIOHTTP1/HTTPPipelineSetup.swift b/Sources/NIOHTTP1/HTTPPipelineSetup.swift index b9d417dc270..70170e7b55d 100644 --- a/Sources/NIOHTTP1/HTTPPipelineSetup.swift +++ b/Sources/NIOHTTP1/HTTPPipelineSetup.swift @@ -944,7 +944,8 @@ extension ChannelPipeline.SynchronousOperations { /// - headerValidation: Whether to validate outbound request headers to confirm that they meet /// spec compliance. Defaults to `true`. /// - encoderConfiguration: The configuration for the ``HTTPRequestEncoder``. - /// - headerValidationResponse: Whether to return a response when the header validation fails. + /// - configuration: Confguration for setting up for the pipeline. Provides additional options + /// for configuring the pipeline. /// - Throws: If the pipeline could not be configured. public func configureHTTPServerPipeline( position: ChannelPipeline.SynchronousOperations.Position = .last, @@ -953,7 +954,7 @@ extension ChannelPipeline.SynchronousOperations { withErrorHandling errorHandling: Bool = true, withOutboundHeaderValidation headerValidation: Bool = true, withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init(), - withOutboundHeaderValidationRepsonse headerValidationResponse: Bool + withConfiguration configuration: Configuration ) throws { try self._configureHTTPServerPipeline( position: position, @@ -961,7 +962,7 @@ extension ChannelPipeline.SynchronousOperations { withServerUpgrade: upgrade, withErrorHandling: errorHandling, withOutboundHeaderValidation: headerValidation, - withOutboundHeaderValidationRepsonse: headerValidationResponse, withEncoderConfiguration: encoderConfiguration + configuration: configuration ) } @@ -971,8 +972,8 @@ extension ChannelPipeline.SynchronousOperations { withServerUpgrade upgrade: NIOHTTPServerUpgradeConfiguration? = nil, withErrorHandling errorHandling: Bool = true, withOutboundHeaderValidation headerValidation: Bool = true, - withOutboundHeaderValidationRepsonse headerValidationResponse: Bool = false, - withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init() + withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init(), + configuration: Configuration = .init(), ) throws { self.eventLoop.assertInEventLoop() @@ -986,7 +987,7 @@ extension ChannelPipeline.SynchronousOperations { } if headerValidation { - handlers.append(NIOHTTPResponseHeadersValidator(sendResponseOnInvalidHeader: headerValidationResponse)) + handlers.append(NIOHTTPResponseHeadersValidator(sendResponseOnInvalidHeader: configuration.headerValidationResponse)) } if errorHandling { @@ -1005,4 +1006,14 @@ extension ChannelPipeline.SynchronousOperations { try self.addHandlers(handlers, position: position) } + + /// Configuration for setting up an HTTP client pipeline. + public struct Configuration { + /// Whether or not a response is returned when the header validation fails. + public var headerValidationResponse: Bool + + public init() { + self.headerValidationResponse = false + } + } } diff --git a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift index f08ba2c8a3f..4d0830bff3d 100644 --- a/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift +++ b/Tests/NIOHTTP1Tests/HTTPHeaderValidationTests.swift @@ -638,7 +638,9 @@ final class HTTPHeaderValidationTests: XCTestCase { func testBadRequestResponseIsReturnedIfHeadersInvalidAndConfiguredToDoSo() throws { let channel = EmbeddedChannel() - try channel.pipeline.syncOperations.configureHTTPServerPipeline(withOutboundHeaderValidationRepsonse: true) + var pipelineConfig = ChannelPipeline.SynchronousOperations.Configuration() + pipelineConfig.headerValidationResponse = true + try channel.pipeline.syncOperations.configureHTTPServerPipeline(withConfiguration: pipelineConfig) try channel.primeForResponse() func assertReadHead(from channel: EmbeddedChannel) throws { From f252f9983547d461a78ec1991829e3281312e1ba Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Tue, 26 Aug 2025 17:08:45 +0100 Subject: [PATCH 6/6] Pass configuration direcetly to handler --- Sources/NIOHTTP1/HTTPHeaderValidator.swift | 4 ++-- Sources/NIOHTTP1/HTTPPipelineSetup.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/NIOHTTP1/HTTPHeaderValidator.swift b/Sources/NIOHTTP1/HTTPHeaderValidator.swift index ce39be82753..5f5e20d9388 100644 --- a/Sources/NIOHTTP1/HTTPHeaderValidator.swift +++ b/Sources/NIOHTTP1/HTTPHeaderValidator.swift @@ -78,9 +78,9 @@ public final class NIOHTTPResponseHeadersValidator: ChannelOutboundHandler, Remo self.sendResponseOnInvalidHeader = false } - public init(sendResponseOnInvalidHeader: Bool) { + public init(pipelineConfiguration: ChannelPipeline.SynchronousOperations.Configuration) { self.state = .validating - self.sendResponseOnInvalidHeader = sendResponseOnInvalidHeader + self.sendResponseOnInvalidHeader = pipelineConfiguration.headerValidationResponse } public func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise?) { diff --git a/Sources/NIOHTTP1/HTTPPipelineSetup.swift b/Sources/NIOHTTP1/HTTPPipelineSetup.swift index 70170e7b55d..0fdc45a2a22 100644 --- a/Sources/NIOHTTP1/HTTPPipelineSetup.swift +++ b/Sources/NIOHTTP1/HTTPPipelineSetup.swift @@ -987,7 +987,7 @@ extension ChannelPipeline.SynchronousOperations { } if headerValidation { - handlers.append(NIOHTTPResponseHeadersValidator(sendResponseOnInvalidHeader: configuration.headerValidationResponse)) + handlers.append(NIOHTTPResponseHeadersValidator(pipelineConfiguration: configuration)) } if errorHandling {