Skip to content

Commit

Permalink
Add support for automatic HTTP error reporting.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Lukasa committed Apr 3, 2018
1 parent 2a684fc commit 265104a
Show file tree
Hide file tree
Showing 9 changed files with 166 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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"
7 changes: 7 additions & 0 deletions Sources/NIOHTTP1/HTTPDecoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 9 additions & 1 deletion Sources/NIOHTTP1/HTTPPipelineSetup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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,
Expand Down
49 changes: 49 additions & 0 deletions Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift
Original file line number Diff line number Diff line change
@@ -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)
}
}
2 changes: 1 addition & 1 deletion Sources/NIOHTTP1Server/main.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
Expand Down
1 change: 1 addition & 0 deletions Tests/LinuxMain.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
@@ -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),
]
}
}

59 changes: 59 additions & 0 deletions Tests/NIOHTTP1Tests/HTTPServerProtocolErrorHandlerTest.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
//===----------------------------------------------------------------------===//
//
// 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)
}
}
2 changes: 1 addition & 1 deletion Tests/NIOHTTP1Tests/HTTPUpgradeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 265104a

Please # to comment.