diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 1800ee91b5bf..6aa3eba61a19 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -619,7 +619,8 @@ message RouteMatch { // match. The router will check the query string from the ``path`` header // against all the specified query parameters. If the number of specified // query parameters is nonzero, they all must match the ``path`` header's - // query string for a match to occur. + // query string for a match to occur. In the event query parameters are + // repeated, only the first value for each key will be considered. // // .. note:: // diff --git a/envoy/http/query_params.h b/envoy/http/query_params.h index eac6741fc28d..ca1ea2d2a07e 100644 --- a/envoy/http/query_params.h +++ b/envoy/http/query_params.h @@ -6,6 +6,7 @@ #include "absl/container/btree_map.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "header_map.h" namespace Envoy { @@ -29,6 +30,7 @@ class QueryParamsMulti { void overwrite(absl::string_view key, absl::string_view value); std::string toString(); std::string replaceQueryString(const HeaderString& path); + absl::optional getFirstValue(absl::string_view key) const; const absl::btree_map>& data() { return data_; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index bd9ce8323bd1..ecb5a83a8234 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -32,6 +32,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "quiche/http2/adapter/http2_protocol.h" namespace Envoy { @@ -609,6 +610,15 @@ void Utility::QueryParamsMulti::overwrite(absl::string_view key, absl::string_vi this->data_[key] = std::vector{std::string(value)}; } +absl::optional Utility::QueryParamsMulti::getFirstValue(absl::string_view key) const { + auto it = this->data_.find(key); + if (it == this->data_.end()) { + return std::nullopt; + } + + return absl::optional{it->second.at(0)}; +} + absl::string_view Utility::findQueryStringStart(const HeaderString& path) { absl::string_view path_str = path.getStringView(); size_t query_offset = path_str.find('?'); diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index d760155d7c51..f742bbcce907 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -787,8 +787,8 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers, return false; } if (!config_query_parameters_.empty()) { - Http::Utility::QueryParams query_parameters = - Http::Utility::parseQueryString(headers.getPathValue()); + auto query_parameters = + Http::Utility::QueryParamsMulti::parseQueryString(headers.getPathValue()); matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_); if (!matches) { return false; diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 70362166294b..8a7ff350e1fd 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -38,16 +38,19 @@ ConfigUtility::QueryParameterMatcher::QueryParameterMatcher( : name_(config.name()), matcher_(maybeCreateStringMatcher(config)) {} bool ConfigUtility::QueryParameterMatcher::matches( - const Http::Utility::QueryParams& request_query_params) const { - auto query_param = request_query_params.find(name_); - if (query_param == request_query_params.end()) { + const Http::Utility::QueryParamsMulti& request_query_params) const { + // This preserves the legacy behavior of ignoring all but the first value for a given key + auto data = request_query_params.getFirstValue(name_); + if (!data.has_value()) { return false; - } else if (!matcher_.has_value()) { - // Present match. + } + + if (!matcher_.has_value()) { + // Present check return true; - } else { - return matcher_.value().match(query_param->second); } + + return matcher_.value().match(data.value()); } Upstream::ResourcePriority @@ -63,7 +66,7 @@ ConfigUtility::parsePriority(const envoy::config::core::v3::RoutingPriority& pri } bool ConfigUtility::matchQueryParams( - const Http::Utility::QueryParams& query_params, + const Http::Utility::QueryParamsMulti& query_params, const std::vector& config_query_params) { for (const auto& config_query_param : config_query_params) { if (!config_query_param->matches(query_params)) { diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index 4895f103de3a..d5773941eebe 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -39,7 +39,7 @@ class ConfigUtility { * @param request_query_params supplies the parsed query parameters from a request. * @return bool true if a match for this QueryParameterMatcher exists in request_query_params. */ - bool matches(const Http::Utility::QueryParams& request_query_params) const; + bool matches(const Http::Utility::QueryParamsMulti& request_query_params) const; private: const std::string name_; @@ -62,7 +62,7 @@ class ConfigUtility { * @return bool true if all the query params (and values) in the config_params are found in the * query_params */ - static bool matchQueryParams(const Http::Utility::QueryParams& query_params, + static bool matchQueryParams(const Http::Utility::QueryParamsMulti& query_params, const std::vector& config_query_params); /** diff --git a/source/common/router/router_ratelimit.cc b/source/common/router/router_ratelimit.cc index 0d75d283fe9d..03c5b9a86ba1 100644 --- a/source/common/router/router_ratelimit.cc +++ b/source/common/router/router_ratelimit.cc @@ -257,8 +257,8 @@ QueryParameterValueMatchAction::QueryParameterValueMatchAction( bool QueryParameterValueMatchAction::populateDescriptor( RateLimit::DescriptorEntry& descriptor_entry, const std::string&, const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo&) const { - Http::Utility::QueryParams query_parameters = - Http::Utility::parseAndDecodeQueryString(headers.getPathValue()); + Http::Utility::QueryParamsMulti query_parameters = + Http::Utility::QueryParamsMulti::parseAndDecodeQueryString(headers.getPathValue()); if (expect_match_ == ConfigUtility::matchQueryParams(query_parameters, action_query_parameters_)) { descriptor_entry = {descriptor_key_, descriptor_value_}; diff --git a/source/extensions/filters/http/jwt_authn/matcher.cc b/source/extensions/filters/http/jwt_authn/matcher.cc index 8f1c6ca70993..a95474ca5e09 100644 --- a/source/extensions/filters/http/jwt_authn/matcher.cc +++ b/source/extensions/filters/http/jwt_authn/matcher.cc @@ -42,8 +42,8 @@ class BaseMatcherImpl : public Matcher, public Logger::Loggable matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_); if (!config_query_parameters_.empty()) { - Http::Utility::QueryParams query_parameters = - Http::Utility::parseQueryString(headers.getPathValue()); + Http::Utility::QueryParamsMulti query_parameters = + Http::Utility::QueryParamsMulti::parseQueryString(headers.getPathValue()); matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_); } return matches; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 70d87052dab8..573ed4e7be6f 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -298,6 +298,9 @@ TEST(HttpUtility, testQueryParamModification) { EXPECT_EQ(params.toString(), "?a=1&a=2&b=3&c=4"); params.overwrite("b", "foo"); EXPECT_EQ(params.toString(), "?a=1&a=2&b=foo&c=4"); + EXPECT_EQ(params.getFirstValue("a").value(), "1"); + EXPECT_EQ(params.getFirstValue("b").value(), "foo"); + EXPECT_FALSE(params.getFirstValue("d").has_value()); params.remove("b"); EXPECT_EQ(params.toString(), "?a=1&a=2&c=4"); params.overwrite("a", "bar"); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 549700fd2615..eda9b850b860 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2704,6 +2704,20 @@ TEST_F(RouteMatcherTest, QueryParamMatchedRouting) { config.route(headers, 0)->routeEntry()->clusterName()); } + { // Repeated parameter - match should only track the first, and match + Http::TestRequestHeaderMapImpl headers = + genHeaders("example.com", "/?debug3=foo&debug3=bar", "GET"); + EXPECT_EQ("local_service_with_string_match_query_parameter", + config.route(headers, 0)->routeEntry()->clusterName()); + } + + { // Repeated parameter - match should only track the first, and not match + Http::TestRequestHeaderMapImpl headers = + genHeaders("example.com", "/?debug3=bar&debug3=foo", "GET"); + EXPECT_EQ("local_service_without_query_parameters", + config.route(headers, 0)->routeEntry()->clusterName()); + } + { Http::TestRequestHeaderMapImpl headers = genHeaders("example.com", "/?debug=2", "GET"); EXPECT_EQ("local_service_with_valueless_query_parameter",