http, utility: Add utility for parsing and decoding query string (#11795

) This introduces `parseAndDecodeQueryString` method to parse the URL into percent-decoded query params. Especially for fixing the Prometheus handler decoding problem as mentioned in #10926. Risk Level: Low Testing: Unit Fixes #10926 Signed-off-by: Dhi Aurrahman <[email protected]>
envoyproxy · Jul 10, 2020 · cb03985 · cb03985
1 parent e712d9d
commit cb03985
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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=<y|n>\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=<y|n>\n");

diff --git 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
@@ -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<std::regex> 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<std::regex> 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
@@ -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
@@ -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%");