Skip to content

Commit

Permalink
Forbid empty :path headers. (#94)
Browse files Browse the repository at this point in the history
Motivation:

RFC 7540 does not allow empty :path pseudo-headers. We should police this.

Modifications:

- Added a check that :path isn't empty.

Result:

Better RFC 7540 validation.
  • Loading branch information
Lukasa authored and weissi committed Apr 12, 2019
1 parent 7acb02b commit b4a9ed3
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 3 deletions.
16 changes: 13 additions & 3 deletions Sources/NIOHTTP2/HPACKHeaders+Validation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,23 @@ extension RequestBlockValidator: HeaderBlockValidator {
// This is a bit awkward.
//
// For now we don't support extended-CONNECT, but when we do we'll need to update the logic here.
guard let pseudoHeaderType = pseudoHeaderType, pseudoHeaderType == .method else {
guard let pseudoHeaderType = pseudoHeaderType else {
// Nothing to do here.
return
}

// This is a method pseudo-header. Check if the value is CONNECT.
self.isConnectRequest = value == "CONNECT"
switch pseudoHeaderType {
case .method:
// This is a method pseudo-header. Check if the value is CONNECT.
self.isConnectRequest = value == "CONNECT"
case .path:
// This is a path pseudo-header. It must not be empty.
if value.utf8.count == 0 {
throw NIOHTTP2Errors.EmptyPathHeader()
}
default:
break
}
}

var allowedPseudoHeaderFields: PseudoHeaders {
Expand Down
5 changes: 5 additions & 0 deletions Sources/NIOHTTP2/HTTP2Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ public enum NIOHTTP2Errors {
public init() { }
}

/// A HTTP/2 header block was received with an empty :path header.
public struct EmptyPathHeader: NIOHTTP2Error {
public init() { }
}

/// A :status header was received with an invalid value.
public struct InvalidStatusValue: NIOHTTP2Error {
public var value: String
Expand Down
2 changes: 2 additions & 0 deletions Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ extension ConnectionStateMachineTests {
("testAllowRequestHeadersWithMissingMethodFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingMethodFieldWhenValidationDisabled),
("testRejectRequestHeadersWithMissingPathField", testRejectRequestHeadersWithMissingPathField),
("testAllowRequestHeadersWithMissingPathFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingPathFieldWhenValidationDisabled),
("testRejectRequestHeadersWithEmptyPathField", testRejectRequestHeadersWithEmptyPathField),
("testAllowRequestHeadersWithEmptyPathFieldWhenValidationDisabled", testAllowRequestHeadersWithEmptyPathFieldWhenValidationDisabled),
("testRejectRequestHeadersWithMissingSchemeField", testRejectRequestHeadersWithMissingSchemeField),
("testAllowRequestHeadersWithMissingSchemeFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingSchemeFieldWhenValidationDisabled),
("testRejectRequestHeadersWithDuplicateMethodField", testRejectRequestHeadersWithDuplicateMethodField),
Expand Down
28 changes: 28 additions & 0 deletions Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1930,6 +1930,34 @@ class ConnectionStateMachineTests: XCTestCase {
assertSucceeds(self.server.receiveHeaders(streamID: streamOne, headers: headers, isEndStreamSet: true))
}

func testRejectRequestHeadersWithEmptyPathField() {
let streamOne = HTTP2StreamID(1)

self.exchangePreamble()

let headers = HPACKHeaders([(":authority", "localhost"), (":scheme", "https"), (":method", "GET"), (":path", ""), ("content-length", "0")])

// Request headers must contain non-empty :path.
assertStreamError(type: .protocolError, self.client.sendHeaders(streamID: streamOne, headers: headers, isEndStreamSet: true))
assertStreamError(type: .protocolError, self.server.receiveHeaders(streamID: streamOne, headers: headers, isEndStreamSet: true))
}

func testAllowRequestHeadersWithEmptyPathFieldWhenValidationDisabled() {
let streamOne = HTTP2StreamID(1)

// Override the setup with validation disabled.
self.server = .init(role: .server, headerBlockValidation: .disabled)
self.client = .init(role: .client, headerBlockValidation: .disabled)

self.exchangePreamble()

let headers = HPACKHeaders([(":authority", "localhost"), (":scheme", "https"), (":method", "GET"), (":path", ""), ("content-length", "0")])

// Request headers must contain non-empty :path.
assertSucceeds(self.client.sendHeaders(streamID: streamOne, headers: headers, isEndStreamSet: true))
assertSucceeds(self.server.receiveHeaders(streamID: streamOne, headers: headers, isEndStreamSet: true))
}

func testRejectRequestHeadersWithMissingSchemeField() {
let streamOne = HTTP2StreamID(1)

Expand Down

0 comments on commit b4a9ed3

Please sign in to comment.