Add support for automatic HTTP error reporting.

Motivation:

Currently the HTTP decoders can throw errors, but they will be ignored
and lead to a simple EOF. That's not ideal: in most cases we should make
a best-effort attempt to send a 4XX error code before we shut the client
down.

Modifications:

Provided a new ChannelHandler that generates 400 errors when the HTTP
decoder fails.
Added a flag to automatically add that handler to the channel pipeline.
Added the handler to the HTTP sample server.
Enabled integration test 12.

Result:

Easier error handling for HTTP servers.

Author: Lukasa

IntegrationTests/tests_01_http/SKIP_test_12_headers_too_large.sh renamed to IntegrationTests/tests_01_http/test_12_headers_too_large.sh

@@ -21,22 +21,21 @@ htdocs=$(get_htdocs "$token")
 touch "$tmp/empty"
 cr=$(echo -e '\r')
 cat > "$tmp/headers_expected" <<EOF
-HTTP/1.0 431 Request Header Fields Too Large$cr
-Content-Length: 0$cr
-Connection: Close$cr
-X-HTTPServer-Error: too many header bytes seen; overflow detected$cr
+HTTP/1.1 400 Bad Request$cr
+content-length: 0$cr
+connection: close$cr
 $cr
 EOF
 echo "FOO BAR" > "$htdocs/some_file.txt"
 # headers have acceptable size
 do_curl "$token" -H "$(python -c 'print "x"*80000'): x" \
-    "http://foobar.com/some_file.txt" > "$tmp/out"
+    "http://foobar.com/fileio/some_file.txt" > "$tmp/out"
 assert_equal_files "$htdocs/some_file.txt" "$tmp/out"
 
 # headers too large
 do_curl "$token" -H "$(python -c 'print "x"*90000'): x" \
     -D "$tmp/headers_actual" \
-    "http://foobar.com/some_file.txt" > "$tmp/out"
+    "http://foobar.com/fileio/some_file.txt" > "$tmp/out"
 assert_equal_files "$tmp/empty" "$tmp/out"
 assert_equal_files "$tmp/headers_expected" "$tmp/headers_actual"
 stop_server "$token"

Sources/NIOHTTP1/HTTPDecoder.swift

@@ -412,6 +412,13 @@ public class HTTPDecoder<HTTPMessageT>: ByteToMessageDecoder, AnyHTTPDecoder {
         ctx.fireChannelReadComplete()
     }
 
+    public func decodeLast(ctx: ChannelHandlerContext, buffer: inout ByteBuffer) throws -> DecodingState {
+        // This handler parses data eagerly, so re-parsing for decodeLast is not useful.
+        // TODO(cory): We should actually add EOF handling here, because EOF can be meaningful in HTTP.
+        // See https://github.com/apple/swift-nio/issues/254
+        return .needMoreData
+    }
+
     public func errorCaught(ctx: ChannelHandlerContext, error: Error) {
         ctx.fireErrorCaught(error)
         if error is HTTPParserError {

Sources/NIOHTTP1/HTTPPipelineSetup.swift

@@ -89,10 +89,14 @@ public extension ChannelPipeline {
     ///         provided should be a tuple of an array of `HTTPProtocolUpgrader` 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 `false` for
+    ///         backward-compatibility reasons.
     /// - returns: An `EventLoopFuture` that will fire when the pipeline is configured.
     public func configureHTTPServerPipeline(first: Bool = false,
                                             withPipeliningAssistance pipelining: Bool = true,
-                                            withServerUpgrade upgrade: HTTPUpgradeConfiguration? = nil) -> EventLoopFuture<Void> {
+                                            withServerUpgrade upgrade: HTTPUpgradeConfiguration? = nil,
+                                            withErrorHandling errorHandling: Bool = false) -> EventLoopFuture<Void> {
         let responseEncoder = HTTPResponseEncoder()
         let requestDecoder = HTTPRequestDecoder()
 
@@ -102,6 +106,10 @@ public extension ChannelPipeline {
             handlers.append(HTTPServerPipelineHandler())
         }
 
+        if errorHandling {
+            handlers.append(HTTPServerProtocolErrorHandler())
+        }
+
         if let (upgraders, completionHandler) = upgrade {
             let upgrader = HTTPServerUpgradeHandler(upgraders: upgraders,
                                                     httpEncoder: responseEncoder,

Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift

@@ -0,0 +1,49 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftNIO open source project
+//
+// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+import NIO
+
+/// 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.
+public final class HTTPServerProtocolErrorHandler: ChannelInboundHandler {
+    public typealias InboundIn = HTTPServerRequestPart
+    public typealias InboundOut = HTTPServerRequestPart
+    public typealias OutboundOut = HTTPServerResponsePart
+
+    public func errorCaught(ctx: ChannelHandlerContext, error: Error) {
+        guard error is HTTPParserError else {
+            ctx.fireErrorCaught(error)
+            return
+        }
+
+        // Any HTTPParserError is automatically fatal, and we don't actually need (or want) to
+        // provide that error to the client: we just want to tell it that it screwed up and then
+        // let the rest of the pipeline shut the door in its face.
+        //
+        // A side note here: we cannot block or do any delayed work. ByteToMessageDecoder is going
+        // to come along and close the channel right after we return from this function.
+        let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
+        let head = HTTPResponseHead(version: .init(major: 1, minor: 1), status: .badRequest, headers: headers)
+        ctx.write(self.wrapOutboundOut(.head(head)), promise: nil)
+        ctx.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
+
+        // Now pass the error on in case someone else wants to see it.
+        ctx.fireErrorCaught(error)
+    }
+}

Sources/NIOHTTP1Server/main.swift

@@ -471,7 +471,7 @@ let bootstrap = ServerBootstrap(group: group)
 
     // Set the handlers that are applied to the accepted Channels
     .childChannelInitializer { channel in
-        channel.pipeline.configureHTTPServerPipeline().then {
+        channel.pipeline.configureHTTPServerPipeline(withErrorHandling: true).then {
             channel.pipeline.add(handler: HTTPHandler(fileIO: fileIO, htdocsPath: htdocs))
         }
     }

Tests/LinuxMain.swift

@@ -58,6 +58,7 @@ import XCTest
          testCase(HTTPResponseEncoderTests.allTests),
          testCase(HTTPServerClientTest.allTests),
          testCase(HTTPServerPipelineHandlerTest.allTests),
+         testCase(HTTPServerProtocolErrorHandlerTest.allTests),
          testCase(HTTPTest.allTests),
          testCase(HTTPUpgradeTestCase.allTests),
          testCase(HappyEyeballsTest.allTests),

Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest+XCTest.swift

@@ -0,0 +1,34 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftNIO open source project
+//
+// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+//
+// HTTPServerProtocolErrorHandlerTest+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 HTTPServerProtocolErrorHandlerTest {
+
+   static var allTests : [(String, (HTTPServerProtocolErrorHandlerTest) -> () throws -> Void)] {
+      return [
+                ("testHandlesBasicErrors", testHandlesBasicErrors),
+                ("testIgnoresNonParserErrors", testIgnoresNonParserErrors),
+           ]
+   }
+}
+

Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest.swift

@@ -0,0 +1,69 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftNIO open source project
+//
+// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+import XCTest
+import NIO
+import NIOHTTP1
+
+class HTTPServerProtocolErrorHandlerTest: XCTestCase {
+    func testHandlesBasicErrors() throws {
+        let channel = EmbeddedChannel()
+        XCTAssertNoThrow(try channel.pipeline.configureHTTPServerPipeline(withErrorHandling: true).wait())
+
+        var buffer = channel.allocator.buffer(capacity: 1024)
+        buffer.write(staticString: "GET / HTTP/1.1\r\nContent-Length: -4\r\n\r\n")
+        do {
+            try channel.writeInbound(buffer)
+        } catch HTTPParserError.invalidContentLength {
+            // This error is expected
+        }
+        (channel.eventLoop as! EmbeddedEventLoop).run()
+
+        // The channel should be closed at this stage.
+        XCTAssertNoThrow(try channel.closeFuture.wait())
+
+        // We expect exactly one ByteBuffer in the output.
+        guard case .some(.byteBuffer(var written)) = channel.readOutbound() else {
+            XCTFail("No writes")
+            return
+        }
+
+        XCTAssertNil(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"])
+    }
+
+    func testIgnoresNonParserErrors() throws {
+        enum DummyError: Error {
+            case error
+        }
+        let channel = EmbeddedChannel()
+        XCTAssertNoThrow(try channel.pipeline.configureHTTPServerPipeline(withErrorHandling: true).wait())
+
+        channel.pipeline.fireErrorCaught(DummyError.error)
+        do {
+            try channel.throwIfErrorCaught()
+            XCTFail("No error caught")
+        } catch DummyError.error {
+            // ok
+        } catch {
+            XCTFail("Unexpected error: \(error)")
+        }
+
+        XCTAssertNoThrow(try channel.finish())
+    }
+}

Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift

@@ -138,7 +138,7 @@ private func setUpTestWithAutoremoval(pipelining: Bool = false,
     return (group, serverChannel, clientChannel, try connectedServerChannelFuture.wait())
 }
 
-private func assertResponseIs(response: String, expectedResponseLine: String, expectedResponseHeaders: [String]) {
+internal func assertResponseIs(response: String, expectedResponseLine: String, expectedResponseHeaders: [String]) {
     var lines = response.split(separator: "\r\n", omittingEmptySubsequences: false).map { String($0) }
 
     // We never expect a response body here. This means we need the last two entries to be empty strings.