diff --git a/IntegrationTests/tests_01_http/SKIP_test_12_headers_too_large.sh b/IntegrationTests/tests_01_http/test_12_headers_too_large.sh similarity index 61% rename from IntegrationTests/tests_01_http/SKIP_test_12_headers_too_large.sh rename to IntegrationTests/tests_01_http/test_12_headers_too_large.sh index 888a484539c..1d0d59df6a0 100644 --- a/IntegrationTests/tests_01_http/SKIP_test_12_headers_too_large.sh +++ b/IntegrationTests/tests_01_http/test_12_headers_too_large.sh @@ -21,22 +21,35 @@ htdocs=$(get_htdocs "$token") touch "$tmp/empty" cr=$(echo -e '\r') cat > "$tmp/headers_expected" < "$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" + +if ! grep -q 'HTTP/1.1 400 Bad Request' "$tmp/headers_actual"; then + fail "couldn't find status line in response" +fi +if ! grep -q 'content-length: 0' "$tmp/headers_actual"; then + fail "couldn't find content-length in response" +fi +if ! grep -q 'connection: close' "$tmp/headers_actual"; then + fail "couldn't find connection: close in response" +fi + +linecount=$(wc "$tmp/headers_actual") +if [ $linecount -ne 4 ]; then + fail "overlong response" +fi stop_server "$token" diff --git a/Sources/NIOHTTP1/HTTPDecoder.swift b/Sources/NIOHTTP1/HTTPDecoder.swift index c68735db37c..3296e3a907d 100644 --- a/Sources/NIOHTTP1/HTTPDecoder.swift +++ b/Sources/NIOHTTP1/HTTPDecoder.swift @@ -416,6 +416,13 @@ public class HTTPDecoder: 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 { diff --git a/Sources/NIOHTTP1/HTTPPipelineSetup.swift b/Sources/NIOHTTP1/HTTPPipelineSetup.swift index 9ca0d24d295..4779194af65 100644 --- a/Sources/NIOHTTP1/HTTPPipelineSetup.swift +++ b/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 { + withServerUpgrade upgrade: HTTPUpgradeConfiguration? = nil, + withErrorHandling errorHandling: Bool = false) -> EventLoopFuture { 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, diff --git a/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift b/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift new file mode 100644 index 00000000000..77557ef3f66 --- /dev/null +++ b/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) + } +} diff --git a/Sources/NIOHTTP1Server/main.swift b/Sources/NIOHTTP1Server/main.swift index 82873327c78..dd64ef5e088 100644 --- a/Sources/NIOHTTP1Server/main.swift +++ b/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)) } } diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index f40aadad947..dd5f3137408 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -59,6 +59,7 @@ import XCTest testCase(HTTPResponseEncoderTests.allTests), testCase(HTTPServerClientTest.allTests), testCase(HTTPServerPipelineHandlerTest.allTests), + testCase(HTTPServerProtocolErrorHandlerTest.allTests), testCase(HTTPTest.allTests), testCase(HTTPUpgradeTestCase.allTests), testCase(HappyEyeballsTest.allTests), diff --git a/Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest+XCTest.swift b/Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest+XCTest.swift new file mode 100644 index 00000000000..f64d75f1cc4 --- /dev/null +++ b/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), + ] + } +} + diff --git a/Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest.swift b/Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest.swift new file mode 100644 index 00000000000..420bc346c72 --- /dev/null +++ b/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()) + } +} diff --git a/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift b/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift index 5ef3f870466..efa5dc5dad6 100644 --- a/Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift +++ b/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.