From b4a9ed3877a3bce7508b31e64ce8156ff2fa6709 Mon Sep 17 00:00:00 2001
From: Cory Benfield <lukasa@apple.com>
Date: Fri, 12 Apr 2019 14:38:10 +0100
Subject: [PATCH] Forbid empty :path headers. (#94)

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.
---
 .../NIOHTTP2/HPACKHeaders+Validation.swift    | 16 +++++++++--
 Sources/NIOHTTP2/HTTP2Error.swift             |  5 ++++
 .../ConnectionStateMachineTests+XCTest.swift  |  2 ++
 .../ConnectionStateMachineTests.swift         | 28 +++++++++++++++++++
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Sources/NIOHTTP2/HPACKHeaders+Validation.swift b/Sources/NIOHTTP2/HPACKHeaders+Validation.swift
index f798acb8..b0f38750 100644
--- a/Sources/NIOHTTP2/HPACKHeaders+Validation.swift
+++ b/Sources/NIOHTTP2/HPACKHeaders+Validation.swift
@@ -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 {
diff --git a/Sources/NIOHTTP2/HTTP2Error.swift b/Sources/NIOHTTP2/HTTP2Error.swift
index 37fee4fa..6616bfe4 100644
--- a/Sources/NIOHTTP2/HTTP2Error.swift
+++ b/Sources/NIOHTTP2/HTTP2Error.swift
@@ -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
diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift
index 94667d68..03e6f82f 100644
--- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift
+++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift
@@ -91,6 +91,8 @@ extension ConnectionStateMachineTests {
                 ("testAllowRequestHeadersWithMissingMethodFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingMethodFieldWhenValidationDisabled),
                 ("testRejectRequestHeadersWithMissingPathField", testRejectRequestHeadersWithMissingPathField),
                 ("testAllowRequestHeadersWithMissingPathFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingPathFieldWhenValidationDisabled),
+                ("testRejectRequestHeadersWithEmptyPathField", testRejectRequestHeadersWithEmptyPathField),
+                ("testAllowRequestHeadersWithEmptyPathFieldWhenValidationDisabled", testAllowRequestHeadersWithEmptyPathFieldWhenValidationDisabled),
                 ("testRejectRequestHeadersWithMissingSchemeField", testRejectRequestHeadersWithMissingSchemeField),
                 ("testAllowRequestHeadersWithMissingSchemeFieldWhenValidationDisabled", testAllowRequestHeadersWithMissingSchemeFieldWhenValidationDisabled),
                 ("testRejectRequestHeadersWithDuplicateMethodField", testRejectRequestHeadersWithDuplicateMethodField),
diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
index af68843a..e086e542 100644
--- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
+++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
@@ -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)