diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index da6076f75689a..bee04a98cabe0 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -233,14 +233,26 @@ Utility::QueryParams Utility::parseQueryString(absl::string_view url) { } start++; - return parseParameters(url, start); + return parseParameters(url, start, /*decode_params=*/false); +} + +Utility::QueryParams Utility::parseAndDecodeQueryString(absl::string_view url) { + size_t start = url.find('?'); + if (start == std::string::npos) { + QueryParams params; + return params; + } + + start++; + return parseParameters(url, start, /*decode_params=*/true); } Utility::QueryParams Utility::parseFromBody(absl::string_view body) { - return parseParameters(body, 0); + return parseParameters(body, 0, /*decode_params=*/true); } -Utility::QueryParams Utility::parseParameters(absl::string_view data, size_t start) { +Utility::QueryParams Utility::parseParameters(absl::string_view data, size_t start, + bool decode_params) { QueryParams params; while (start < data.size()) { @@ -252,8 +264,10 @@ Utility::QueryParams Utility::parseParameters(absl::string_view data, size_t sta const size_t equal = param.find('='); if (equal != std::string::npos) { - params.emplace(StringUtil::subspan(data, start, start + equal), - StringUtil::subspan(data, start + equal + 1, end)); + const auto param_name = StringUtil::subspan(data, start, start + equal); + const auto param_value = StringUtil::subspan(data, start + equal + 1, end); + params.emplace(decode_params ? PercentEncoding::decode(param_name) : param_name, + decode_params ? PercentEncoding::decode(param_value) : param_value); } else { params.emplace(StringUtil::subspan(data, start, end), ""); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 69778024e8a7e..4fef6cc233270 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -176,6 +176,13 @@ std::string createSslRedirectPath(const RequestHeaderMap& headers); */ QueryParams parseQueryString(absl::string_view url); +/** + * Parse a URL into query parameters. + * @param url supplies the url to parse. + * @return QueryParams the parsed and percent-decoded parameters, if any. + */ +QueryParams parseAndDecodeQueryString(absl::string_view url); + /** * Parse a a request body into query parameters. * @param body supplies the body to parse. @@ -187,9 +194,11 @@ QueryParams parseFromBody(absl::string_view body); * Parse query parameters from a URL or body. * @param data supplies the data to parse. * @param start supplies the offset within the data. + * @param decode_params supplies the flag whether to percent-decode the parsed parameters (both name + * and value). Set to false to keep the parameters encoded. * @return QueryParams the parsed parameters, if any. */ -QueryParams parseParameters(absl::string_view data, size_t start); +QueryParams parseParameters(absl::string_view data, size_t start, bool decode_params); /** * Finds the start of the query string in a path diff --git a/source/extensions/filters/http/jwt_authn/extractor.cc b/source/extensions/filters/http/jwt_authn/extractor.cc index 6e02093c0749a..338187e6a1392 100644 --- a/source/extensions/filters/http/jwt_authn/extractor.cc +++ b/source/extensions/filters/http/jwt_authn/extractor.cc @@ -208,7 +208,7 @@ ExtractorImpl::extract(const Http::RequestHeaderMap& headers) const { } // Check query parameter locations. - const auto& params = Http::Utility::parseQueryString(headers.getPathValue()); + const auto& params = Http::Utility::parseAndDecodeQueryString(headers.getPathValue()); for (const auto& location_it : param_locations_) { const auto& param_key = location_it.first; const auto& location_spec = location_it.second; diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index dd52e2807ffb7..5b3d3b3c4255b 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -442,7 +442,7 @@ void AdminImpl::writeClustersAsText(Buffer::Instance& response) { Http::Code AdminImpl::handlerClusters(absl::string_view url, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&) { - Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); + Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); const auto format_value = Utility::formatParam(query_params); if (format_value.has_value() && format_value.value() == "json") { @@ -622,7 +622,7 @@ ProtobufTypes::MessagePtr AdminImpl::dumpEndpointConfigs() const { Http::Code AdminImpl::handlerConfigDump(absl::string_view url, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&) const { - Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); + Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); const auto resource = resourceParam(query_params); const auto mask = maskParam(query_params); const bool include_eds = shouldIncludeEdsInDump(query_params); diff --git a/source/server/admin/profiling_handler.cc b/source/server/admin/profiling_handler.cc index 121daeb9976b1..243b8292a0af6 100644 --- a/source/server/admin/profiling_handler.cc +++ b/source/server/admin/profiling_handler.cc @@ -11,7 +11,7 @@ ProfilingHandler::ProfilingHandler(const std::string& profile_path) : profile_pa Http::Code ProfilingHandler::handlerCpuProfiler(absl::string_view url, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream&) { - Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); + Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); if (query_params.size() != 1 || query_params.begin()->first != "enable" || (query_params.begin()->second != "y" && query_params.begin()->second != "n")) { response.add("?enable=\n"); @@ -40,7 +40,7 @@ Http::Code ProfilingHandler::handlerHeapProfiler(absl::string_view url, Http::Re return Http::Code::NotImplemented; } - Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); + Http::Utility::QueryParams query_params = Http::Utility::parseAndDecodeQueryString(url); if (query_params.size() != 1 || query_params.begin()->first != "enable" || (query_params.begin()->second != "y" && query_params.begin()->second != "n")) { response.add("?enable=\n"); diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index d2ee6dd848132..869427f694b84 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -18,7 +18,7 @@ RuntimeHandler::RuntimeHandler(Server::Instance& server) : HandlerContextBase(se Http::Code RuntimeHandler::handlerRuntime(absl::string_view url, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + const Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); // TODO(jsedgwick): Use proto to structure this output instead of arbitrary JSON. @@ -80,7 +80,7 @@ Http::Code RuntimeHandler::handlerRuntime(absl::string_view url, Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream& admin_stream) { - Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); if (params.empty()) { // Check if the params are in the request's body. if (admin_stream.getRequestBody() != nullptr && diff --git a/source/server/admin/stats_handler.cc b/source/server/admin/stats_handler.cc index 3ec4702b2c5a0..753774f59dc97 100644 --- a/source/server/admin/stats_handler.cc +++ b/source/server/admin/stats_handler.cc @@ -72,7 +72,7 @@ Http::Code StatsHandler::handlerStats(absl::string_view url, Http::ResponseHeaderMap& response_headers, Buffer::Instance& response, AdminStream& admin_stream) { Http::Code rc = Http::Code::OK; - const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + const Http::Utility::QueryParams params = Http::Utility::parseAndDecodeQueryString(url); const bool used_only = params.find("usedonly") != params.end(); absl::optional regex; @@ -140,7 +140,8 @@ Http::Code StatsHandler::handlerStats(absl::string_view url, Http::Code StatsHandler::handlerPrometheusStats(absl::string_view path_and_query, Http::ResponseHeaderMap&, Buffer::Instance& response, AdminStream&) { - const Http::Utility::QueryParams params = Http::Utility::parseQueryString(path_and_query); + const Http::Utility::QueryParams params = + Http::Utility::parseAndDecodeQueryString(path_and_query); const bool used_only = params.find("usedonly") != params.end(); absl::optional regex; if (!Utility::filterParam(params, response, regex)) { diff --git a/test/common/http/utility_fuzz_test.cc b/test/common/http/utility_fuzz_test.cc index 18d5c0c4c388a..2b665893f50f3 100644 --- a/test/common/http/utility_fuzz_test.cc +++ b/test/common/http/utility_fuzz_test.cc @@ -18,6 +18,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::UtilityTestCase& input) { } switch (input.utility_selector_case()) { case test::common::http::UtilityTestCase::kParseQueryString: { + // TODO(dio): Add the case when using parseAndDecodeQueryString(). Http::Utility::parseQueryString(input.parse_query_string()); break; } @@ -57,7 +58,9 @@ DEFINE_PROTO_FUZZER(const test::common::http::UtilityTestCase& input) { } case test::common::http::UtilityTestCase::kParseParameters: { const auto& parse_parameters = input.parse_parameters(); - Http::Utility::parseParameters(parse_parameters.data(), parse_parameters.start()); + // TODO(dio): Add a case when doing parse_parameters with decode_params flag true. + Http::Utility::parseParameters(parse_parameters.data(), parse_parameters.start(), + /*decode_params*/ false); break; } case test::common::http::UtilityTestCase::kFindQueryString: { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 4a42d7d8fd9eb..47c12303d0b6d 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -29,16 +29,65 @@ namespace Http { TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); + EXPECT_EQ(Utility::QueryParams(), Utility::parseAndDecodeQueryString("/hello")); + EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello?")); + EXPECT_EQ(Utility::QueryParams(), Utility::parseAndDecodeQueryString("/hello?")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), Utility::parseQueryString("/hello?hello")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), + Utility::parseAndDecodeQueryString("/hello?hello")); + EXPECT_EQ(Utility::QueryParams({{"hello", "world"}}), Utility::parseQueryString("/hello?hello=world")); + EXPECT_EQ(Utility::QueryParams({{"hello", "world"}}), + Utility::parseAndDecodeQueryString("/hello?hello=world")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), Utility::parseQueryString("/hello?hello=")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), + Utility::parseAndDecodeQueryString("/hello?hello=")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), Utility::parseQueryString("/hello?hello=&")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}}), + Utility::parseAndDecodeQueryString("/hello?hello=&")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}, {"hello2", "world2"}}), Utility::parseQueryString("/hello?hello=&hello2=world2")); + EXPECT_EQ(Utility::QueryParams({{"hello", ""}, {"hello2", "world2"}}), + Utility::parseAndDecodeQueryString("/hello?hello=&hello2=world2")); + EXPECT_EQ(Utility::QueryParams({{"name", "admin"}, {"level", "trace"}}), Utility::parseQueryString("/logging?name=admin&level=trace")); + EXPECT_EQ(Utility::QueryParams({{"name", "admin"}, {"level", "trace"}}), + Utility::parseAndDecodeQueryString("/logging?name=admin&level=trace")); + + EXPECT_EQ(Utility::QueryParams({{"param_value_has_encoded_ampersand", "a%26b"}}), + Utility::parseQueryString("/hello?param_value_has_encoded_ampersand=a%26b")); + EXPECT_EQ(Utility::QueryParams({{"param_value_has_encoded_ampersand", "a&b"}}), + Utility::parseAndDecodeQueryString("/hello?param_value_has_encoded_ampersand=a%26b")); + + EXPECT_EQ(Utility::QueryParams({{"params_has_encoded_%26", "a%26b"}, {"ok", "1"}}), + Utility::parseQueryString("/hello?params_has_encoded_%26=a%26b&ok=1")); + EXPECT_EQ(Utility::QueryParams({{"params_has_encoded_&", "a&b"}, {"ok", "1"}}), + Utility::parseAndDecodeQueryString("/hello?params_has_encoded_%26=a%26b&ok=1")); + + // A sample of request path with query strings by Prometheus: + // https://github.com/envoyproxy/envoy/issues/10926#issuecomment-651085261. + EXPECT_EQ( + Utility::QueryParams( + {{"filter", + "%28cluster.upstream_%28rq_total%7Crq_time_sum%7Crq_time_count%7Crq_time_" + "bucket%7Crq_xx%7Crq_complete%7Crq_active%7Ccx_active%29%29%7C%28server.version%29"}}), + Utility::parseQueryString( + "/stats?filter=%28cluster.upstream_%28rq_total%7Crq_time_sum%7Crq_time_count%7Crq_time_" + "bucket%7Crq_xx%7Crq_complete%7Crq_active%7Ccx_active%29%29%7C%28server.version%29")); + EXPECT_EQ( + Utility::QueryParams( + {{"filter", "(cluster.upstream_(rq_total|rq_time_sum|rq_time_count|rq_time_bucket|rq_xx|" + "rq_complete|rq_active|cx_active))|(server.version)"}}), + Utility::parseAndDecodeQueryString( + "/stats?filter=%28cluster.upstream_%28rq_total%7Crq_time_sum%7Crq_time_count%7Crq_time_" + "bucket%7Crq_xx%7Crq_complete%7Crq_active%7Ccx_active%29%29%7C%28server.version%29")); } TEST(HttpUtility, getResponseStatus) { @@ -1212,7 +1261,19 @@ TEST(PercentEncoding, EncodeDecode) { validatePercentEncodingEncodeDecode("_-ok-_", "_-ok-_"); } -TEST(PercentEncoding, Trailing) { +TEST(PercentEncoding, Decoding) { + EXPECT_EQ(Utility::PercentEncoding::decode("a%26b"), "a&b"); + EXPECT_EQ(Utility::PercentEncoding::decode("hello%20world"), "hello world"); + EXPECT_EQ(Utility::PercentEncoding::decode("upstream%7Cdownstream"), "upstream|downstream"); + EXPECT_EQ( + Utility::PercentEncoding::decode( + "filter=%28cluster.upstream_%28rq_total%7Crq_time_sum%7Crq_time_count%7Crq_time_bucket%" + "7Crq_xx%7Crq_complete%7Crq_active%7Ccx_active%29%29%7C%28server.version%29"), + "filter=(cluster.upstream_(rq_total|rq_time_sum|rq_time_count|rq_time_bucket|rq_xx|rq_" + "complete|rq_active|cx_active))|(server.version)"); +} + +TEST(PercentEncoding, DecodingWithTrailingInput) { EXPECT_EQ(Utility::PercentEncoding::decode("too%20lar%20"), "too lar "); EXPECT_EQ(Utility::PercentEncoding::decode("too%20larg%e"), "too larg%e"); EXPECT_EQ(Utility::PercentEncoding::decode("too%20large%"), "too large%");