Include port in authority computed from absolute-uri (#1484)

Fix the misuse of the UF_* bitfields correctly.
envoyproxy · Aug 22, 2017 · 7899547 · 7899547
1 parent 4e63a02
commit 7899547
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 3 deletions.
diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -416,7 +416,8 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho
     throw CodecProtocolException(
         "http/1.1 protocol error: invalid url in request line, parsed invalid");
   } else {
-    if ((u.field_set & UF_HOST) == UF_HOST && (u.field_set & UF_SCHEMA) == UF_SCHEMA) {
+    if ((u.field_set & (1 << UF_HOST)) == (1 << UF_HOST) &&
+        (u.field_set & (1 << UF_SCHEMA)) == (1 << UF_SCHEMA)) {
       // RFC7230#5.7
       // When a proxy receives a request with an absolute-form of
       // request-target, the proxy MUST ignore the received Host header field
@@ -425,15 +426,21 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho
       // new Host field-value based on the received request-target rather than
       // forward the received Host field-value.
 
+      uint16_t authority_len = u.field_data[UF_HOST].len;
+
+      if ((u.field_set & (1 << UF_PORT)) == (1 << UF_PORT)) {
+        authority_len = authority_len + u.field_data[UF_PORT].len + 1;
+      }
+
       // Insert the host header, this will later be converted to :authority
       std::string new_host(active_request_->request_url_.c_str() + u.field_data[UF_HOST].off,
-                           u.field_data[UF_HOST].len);
+                           authority_len);
 
       headers.insertHost().value(new_host);
 
       // RFC allows the absolute-uri to not end in /, but the absolute path form
       // must start with /
-      if ((u.field_set & UF_PATH) == UF_PATH && u.field_data[UF_PATH].len > 0) {
+      if ((u.field_set & (1 << UF_PATH)) == (1 << UF_PATH) && u.field_data[UF_PATH].len > 0) {
         HeaderString new_path;
         new_path.setCopy(active_request_->request_url_.c_str() + u.field_data[UF_PATH].off,
                          active_request_->request_url_.size() - u.field_data[UF_PATH].off);

diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc
@@ -156,6 +156,14 @@ TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePath2) {
   expectHeadersTest(Protocol::Http11, true, buffer, expected_headers);
 }
 
+TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePathWithPort) {
+  TestHeaderMapImpl expected_headers{
+      {":authority", "www.somewhere.com:4532"}, {":path", "/foo/bar"}, {":method", "GET"}};
+  Buffer::OwnedImpl buffer(
+      "GET http://www.somewhere.com:4532/foo/bar HTTP/1.1\r\nHost: bah\r\n\r\n");
+  expectHeadersTest(Protocol::Http11, true, buffer, expected_headers);
+}
+
 TEST_F(Http1ServerConnectionImplTest, Http11AbsoluteEnabledNoOp) {
   initialize();
 

diff --git a/test/config/integration/server.json b/test/config/integration/server.json
@@ -148,6 +148,17 @@
                 }
               ]
             },
+            {
+              "name": "redirect",
+              "domains": [ "www.namewithport.com:1234" ],
+              "require_ssl": "all",
+              "routes": [
+                {
+                  "prefix": "/",
+                  "cluster": "cluster_1"
+                }
+              ]
+            },
             {
               "name": "integration",
               "domains": [ "*" ],

diff --git a/test/integration/integration.cc b/test/integration/integration.cc
@@ -974,6 +974,36 @@ void BaseIntegrationTest::testAbsolutePath() {
   EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0);
 }
 
+void BaseIntegrationTest::testAbsolutePathWithPort() {
+  Buffer::OwnedImpl buffer("GET http://www.namewithport.com:1234 HTTP/1.1\r\nHost: host\r\n\r\n");
+  std::string response;
+  RawConnectionDriver connection(
+      lookupPort("http_forward"), buffer,
+      [&](Network::ClientConnection& client, const Buffer::Instance& data) -> void {
+        response.append(TestUtility::bufferToString(data));
+        client.close(Network::ConnectionCloseType::NoFlush);
+      },
+      version_);
+
+  connection.run();
+  EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0);
+}
+
+void BaseIntegrationTest::testAbsolutePathWithoutPort() {
+  Buffer::OwnedImpl buffer("GET http://www.namewithport.com HTTP/1.1\r\nHost: host\r\n\r\n");
+  std::string response;
+  RawConnectionDriver connection(
+      lookupPort("http_forward"), buffer,
+      [&](Network::ClientConnection& client, const Buffer::Instance& data) -> void {
+        response.append(TestUtility::bufferToString(data));
+        client.close(Network::ConnectionCloseType::NoFlush);
+      },
+      version_);
+
+  connection.run();
+  EXPECT_TRUE(response.find("HTTP/1.1 404 Not Found\r\n") == 0);
+}
+
 void BaseIntegrationTest::testAllowAbsoluteSameRelative() {
   // Ensure that relative urls behave the same with allow_absolute_url enabled and without
   testEquivalent("GET /foo/bar HTTP/1.1\r\nHost: host\r\n\r\n");

diff --git a/test/integration/integration.h b/test/integration/integration.h
@@ -226,6 +226,8 @@ class BaseIntegrationTest : Logger::Loggable<Logger::Id::testing> {
   void testUpstreamProtocolError();
   void testBadPath();
   void testAbsolutePath();
+  void testAbsolutePathWithPort();
+  void testAbsolutePathWithoutPort();
   void testConnect();
   void testAllowAbsoluteSameRelative();
   // Test that a request returns the same content with both allow_absolute_urls enabled and

diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc
@@ -131,6 +131,10 @@ TEST_P(IntegrationTest, BadPath) { testBadPath(); }
 
 TEST_P(IntegrationTest, AbsolutePath) { testAbsolutePath(); }
 
+TEST_P(IntegrationTest, AbsolutePathWithPort) { testAbsolutePathWithPort(); }
+
+TEST_P(IntegrationTest, AbsolutePathWithoutPort) { testAbsolutePathWithoutPort(); }
+
 TEST_P(IntegrationTest, Connect) { testConnect(); }
 
 TEST_P(IntegrationTest, ValidZeroLengthContent) {