Merge pull request from GHSA-3mwq-h3g6-ffhm

* Fix the HTTP1 error handler to close connections when HTTP parse errors occur instead of passing them on.

* Move the error handling for parser errors into HTTPServerRequestDecoder, where it arguably belongs. Don't send 400 bad request for those errors (not required by RFC, difficult to do safely for all cases), fix inbound user event handling in multiple handlers.

Sources/Vapor/HTTP/Server/HTTPServer.swift

@@ -452,73 +452,6 @@ private final class HTTPServerConnection: Sendable {
     }
 }
 
-/// A simple channel handler that catches errors emitted by parsing HTTP requests
-/// and sends 400 Bad Request responses.
-///
-/// This channel handler provides the basic behaviour that the majority of simple HTTP
-/// servers want. This handler does not suppress the parser errors: it allows them to
-/// continue to pass through the pipeline so that other handlers (e.g. logging ones) can
-/// deal with the error.
-/// 
-/// adapted from: https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift
-final class HTTP1ServerErrorHandler: ChannelDuplexHandler, RemovableChannelHandler {
-    typealias InboundIn = Never
-    typealias InboundOut = Never
-    typealias OutboundIn = HTTPServerResponsePart
-    typealias OutboundOut = HTTPServerResponsePart
-    let logger: Logger
-    private var hasUnterminatedResponse: Bool = false
-
-    init(logger: Logger) {
-        self.logger = logger
-    }
-
-    func errorCaught(context: ChannelHandlerContext, error: Error) {
-        if let error = error as? HTTPParserError {
-            self.makeHTTPParserErrorResponse(context: context, error: error)
-        }
-
-        // Now pass the error on in case someone else wants to see it.
-        // In the Vapor ChannelPipeline the connection will eventually 
-        // be closed by the NIOCloseOnErrorHandler
-        context.fireErrorCaught(error)
-    }
-
-    private func makeHTTPParserErrorResponse(context: ChannelHandlerContext, error: HTTPParserError) {
-        // Any HTTPParserError is automatically fatal, and we don't actually need (or want) to
-        // provide that error to the client: we just want to inform them something went wrong
-        // and then close off the pipeline. However, we can only send an
-        // HTTP error response if another response hasn't started yet.
-        //
-        // A side note here: we cannot block or do any delayed work. 
-        // The channel might be closed right after we return from this function.
-        if !self.hasUnterminatedResponse {
-            self.logger.debug("Bad Request - Invalid HTTP: \(error)")
-            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)
-        }
-    }
-
-    public func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
-        let res = self.unwrapOutboundIn(data)
-        switch res {
-        case .head(let head) where head.isInformational:
-            precondition(!self.hasUnterminatedResponse)
-        case .head:
-            precondition(!self.hasUnterminatedResponse)
-            self.hasUnterminatedResponse = true
-        case .body:
-            precondition(self.hasUnterminatedResponse)
-        case .end:
-            precondition(self.hasUnterminatedResponse)
-            self.hasUnterminatedResponse = false
-        }
-        context.write(data, promise: promise)
-    }
-}
-
 extension HTTPResponseHead {
     /// Determines if the head is purely informational. If a head is informational another head will follow this
     /// head eventually.
@@ -608,9 +541,6 @@ extension ChannelPipeline {
             break
         }
 
-        let errorHandler = HTTP1ServerErrorHandler(logger: configuration.logger)
-        handlers.append(errorHandler)
-
         // add NIO -> HTTP response encoder
         let serverResEncoder = HTTPServerResponseEncoder(
             serverHeader: configuration.serverName,

Sources/Vapor/HTTP/Server/HTTPServerHandler.swift

@@ -71,7 +71,7 @@ final class HTTPServerHandler: ChannelInboundHandler, RemovableChannelHandler {
             self.logger.trace("HTTP handler will no longer respect keep-alive")
             self.isShuttingDown = true
         default:
-            self.logger.trace("Unhandled user event: \(event)")
+            context.fireUserInboundEventTriggered(event)
         }
     }
 }

Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift

@@ -133,6 +133,10 @@ final class HTTPServerRequestDecoder: ChannelDuplexHandler, RemovableChannelHand
         default:
             break
         }
+
+        if error is HTTPParserError {
+            self.logger.debug("Invalid HTTP request, will close connection: \(String(reflecting: error))")
+        }
         context.fireErrorCaught(error)
     }
 
@@ -218,7 +222,7 @@ final class HTTPServerRequestDecoder: ChannelDuplexHandler, RemovableChannelHand
                 context.fireUserInboundEventTriggered(event)
             }
         default:
-            self.logger.trace("Unhandled user event: \(event)")
+            context.fireUserInboundEventTriggered(event)
         }
     }
 }

Tests/VaporTests/PipelineTests.swift

@@ -138,7 +138,6 @@ final class PipelineTests: XCTestCase {
             }
         }
         XCTAssertEqual(channel.isActive, false)
-        try XCTAssertContains(channel.readOutbound(as: ByteBuffer.self)?.string, "HTTP/1.1 400 Bad Request")
         try XCTAssertNil(channel.readOutbound(as: ByteBuffer.self)?.string)
     }