From 5dc338815c6ff2105df74860180694a1115e3503 Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Tue, 2 Apr 2019 19:52:57 +0200 Subject: [PATCH 1/3] Description: Added veryfication if path contains query params and append path headers if it has them Risk Level: low Testing: added new test cases Docs Changes: n/a Release Notes: n/a Signed-off-by: Lukasz Dziedziak --- source/common/http/utility.cc | 11 +++++++++-- test/common/http/utility_test.cc | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 50083e96910ba..3f3e601279000 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -57,8 +57,15 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { // RFC allows the absolute-uri to not end in /, but the absolute path form // must start with if ((u.field_set & (1 << UF_PATH)) == (1 << UF_PATH) && u.field_data[UF_PATH].len > 0) { - path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, - u.field_data[UF_PATH].len); + // Check if path contains query parameters and if so add them to path + if ((u.field_set & (1 << UF_QUERY)) == (1 << UF_QUERY) && u.field_data[UF_QUERY].len > 0) { + // u.field_data[UF_PATH].off + u.field_data[UF_PATH].len == u.field_data[UF_QUERY].off - 1 + path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, + u.field_data[UF_PATH].len + 1 + u.field_data[UF_QUERY].len); + } else { + path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, + u.field_data[UF_PATH].len); + } } else { path_ = absl::string_view(kDefaultPath, 1); } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index dc7a29e3fb664..e5072651964c9 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -772,6 +772,21 @@ TEST(Url, ParsingTest) { // Test https scheme ValidateUrl("https://www.host.com", "https", "www.host.com", "/"); + + // Test url with query parameter + ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); + ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + + // Test url with multi-character path and query parameter + ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", + "/path?query=param"); + ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); + + // Test url with multi-character path and more than one query parameter + ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", + "/path?query=param&query2=param2"); + ValidateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", + "/path?query=param&query2=param2"); } } // namespace Http From 3a0c97a11ba104b47b2c72218b3e8d1a68f24ffd Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Wed, 3 Apr 2019 21:20:37 +0200 Subject: [PATCH 2/3] Renamed path_ to path_and_query_params_. Refactored added method. Signed-off-by: Lukasz Dziedziak --- source/common/http/http1/codec_impl.cc | 2 +- source/common/http/utility.cc | 13 +++++-------- source/common/http/utility.h | 6 +++--- source/common/router/router.cc | 2 +- test/common/http/utility_test.cc | 2 +- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 43a3c1ef4cd55..d85e642444788 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -533,7 +533,7 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho // forward the received Host field-value. headers.insertHost().value(std::string(absolute_url.host_and_port())); - headers.insertPath().value(std::string(absolute_url.path())); + headers.insertPath().value(std::string(absolute_url.path_and_query_params())); active_request_->request_url_.clear(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 3f3e601279000..e4980f11bf181 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -57,17 +57,14 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { // RFC allows the absolute-uri to not end in /, but the absolute path form // must start with if ((u.field_set & (1 << UF_PATH)) == (1 << UF_PATH) && u.field_data[UF_PATH].len > 0) { - // Check if path contains query parameters and if so add them to path + uint64_t path_len = u.field_data[UF_PATH].len; if ((u.field_set & (1 << UF_QUERY)) == (1 << UF_QUERY) && u.field_data[UF_QUERY].len > 0) { - // u.field_data[UF_PATH].off + u.field_data[UF_PATH].len == u.field_data[UF_QUERY].off - 1 - path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, - u.field_data[UF_PATH].len + 1 + u.field_data[UF_QUERY].len); - } else { - path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, - u.field_data[UF_PATH].len); + path_len += 1 + u.field_data[UF_QUERY].len; } + path_and_query_params_ = + absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, path_len); } else { - path_ = absl::string_view(kDefaultPath, 1); + path_and_query_params_ = absl::string_view(kDefaultPath, 1); } return true; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 92e8a1e7badc3..5c4526ba9b3d2 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -24,19 +24,19 @@ namespace Utility { /** * Given a fully qualified URL, splits the string_view provided into scheme, - * host and path components. + * host and path with query parameters components. */ class Url { public: bool initialize(absl::string_view absolute_url); absl::string_view scheme() { return scheme_; } absl::string_view host_and_port() { return host_and_port_; } - absl::string_view path() { return path_; } + absl::string_view path_and_query_params() { return path_and_query_params_; } private: absl::string_view scheme_; absl::string_view host_and_port_; - absl::string_view path_; + absl::string_view path_and_query_params_; }; /** diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3d65db36144d5..e2bd72dedd598 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -77,7 +77,7 @@ bool convertRequestHeadersForInternalRedirect(Http::HeaderMap& downstream_header // Replace the original host, scheme and path. downstream_headers.insertScheme().value(std::string(absolute_url.scheme())); downstream_headers.insertHost().value(std::string(absolute_url.host_and_port())); - downstream_headers.insertPath().value(std::string(absolute_url.path())); + downstream_headers.insertPath().value(std::string(absolute_url.path_and_query_params())); return true; } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e5072651964c9..350602b3607a8 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -754,7 +754,7 @@ void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); EXPECT_EQ(url.host_and_port(), expected_host_port); - EXPECT_EQ(url.path(), expected_path); + EXPECT_EQ(url.path_and_query_params(), expected_path); } TEST(Url, ParsingTest) { From 059b14b5d64498768d156dc62bb51056712a346a Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Thu, 4 Apr 2019 19:26:36 +0200 Subject: [PATCH 3/3] added more test cases Signed-off-by: Lukasz Dziedziak --- source/common/http/utility.cc | 5 +++++ test/common/http/utility_test.cc | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e4980f11bf181..7cd9715a66d1d 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -63,6 +63,11 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { } path_and_query_params_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, path_len); + } else if ((u.field_set & (1 << UF_QUERY)) == (1 << UF_QUERY) && u.field_data[UF_QUERY].len > 0) { + // Http parser skips question mark and starts count from first character after ? + // so we need to move left by one + path_and_query_params_ = absl::string_view(absolute_url.data() + u.field_data[UF_QUERY].off - 1, + u.field_data[UF_QUERY].len + 1); } else { path_and_query_params_ = absl::string_view(kDefaultPath, 1); } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 350602b3607a8..6afc92e72886c 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -766,10 +766,22 @@ TEST(Url, ParsingTest) { ValidateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); ValidateUrl("http://www.host.com/", "http", "www.host.com", "/"); + // Test url with "?". + ValidateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/"); + ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/"); + + // Test url with "?" but without slash. + ValidateUrl("http://www.host.com:80?", "http", "www.host.com:80", "/"); + ValidateUrl("http://www.host.com?", "http", "www.host.com", "/"); + // Test url with multi-character path ValidateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); + // Test url with multi-character path and ? at the end + ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path"); + ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path"); + // Test https scheme ValidateUrl("https://www.host.com", "https", "www.host.com", "/"); @@ -777,6 +789,10 @@ TEST(Url, ParsingTest) { ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + // Test url with query parameter but without slash + ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); + ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); + // Test url with multi-character path and query parameter ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", "/path?query=param");