http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

dio · 2020-06-29T13:17:33Z

Commit Message: This introduces parseAndDecodeQueryString method to parse the URL into percent-decoded query params.

Additional comments: Especially for fixing the Prometheus handler decoding problem as mentioned in #10926.

Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Fixes #10926

Signed-off-by: Dhi Aurrahman dio@tetrate.io

This makes sure we do URL decoding when parsing query string. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-06-30T04:28:39Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-30T04:28:47Z

Azure Pipelines successfully started running 1 pipeline(s).

source/server/admin/stats_handler.cc

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

zuercher · 2020-07-07T17:42:40Z

source/common/http/utility.cc

+  }
+
+  start++;
+  return parseParameters(PercentEncoding::decode(StringUtil::subspan(url, start, url.size())), 0);


Oh, I apologize. I read this code incorrectly the first time. I thought we could just implement this method as:

return PercentEncoding::decode(parseQueryString(url))

I see now that's percent-decoding the params and then parsing them.

Doesn't this have a bug? If I pass query params with a percent-encoded & (e.g. x=a%26b) then decoding before splitting parameters will split in the wrong location, yes?

You're right! Will fix this while waiting for the chance to use GURL (#11670) to arrive.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

zuercher · 2020-07-08T17:38:23Z

source/common/http/utility.cc

      params.emplace(StringUtil::subspan(data, start, start + equal),
-                     StringUtil::subspan(data, start + equal + 1, end));
+                     decode_param_value ? PercentEncoding::decode(param_value) : param_value);


Should decode param names as well?

zuercher · 2020-07-08T17:40:22Z

source/common/http/utility.cc

@@ -264,22 +264,23 @@ std::string Utility::createSslRedirectPath(const RequestHeaderMap& headers) {
  return fmt::format("https://{}{}", headers.getHostValue(), headers.getPathValue());
 }

-Utility::QueryParams Utility::parseQueryString(absl::string_view url) {
+Utility::QueryParams Utility::parseQueryString(absl::string_view url, bool decode_param_value) {


Sorry, but I still think the separate methods was easier to read.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

zuercher

Ok, last thing.

zuercher · 2020-07-09T19:07:43Z

source/common/router/config_impl.cc

@@ -490,7 +490,7 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers,
  matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_);
  if (!config_query_parameters_.empty()) {
    Http::Utility::QueryParams query_parameters =
-        Http::Utility::parseQueryString(headers.getPathValue());
+        Http::Utility::parseAndDecodeQueryString(headers.getPathValue());


I think we can make this the default, but I think we probably need a runtime flag to disable this for people who are matching values with percent encoding enabled.

Same applies to the other filters. I think it's ok to change the behavior in the admin handlers.

@zuercher I'll revert the change to places outside admin handler for now and do runtime flagging in different PR. WDYT?

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

…oyproxy#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 envoyproxy#10926. Risk Level: Low Testing: Unit Fixes envoyproxy#10926 Signed-off-by: Dhi Aurrahman <dio@tetrate.io> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

dio added 3 commits June 29, 2020 20:15

utility: Do URL decoding when parseQueryString

6a7a956

This makes sure we do URL decoding when parsing query string. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Add one more case

28b4d37

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Introduce do_percent_decoding flag

10e849c

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio changed the title ~~http, utility: Do URL decoding when parsing query string~~ http, utility: Optionally do URL decoding when parsing query string Jun 29, 2020

mattklein123 assigned zuercher Jun 30, 2020

dio added 3 commits July 1, 2020 06:26

Merge remote-tracking branch 'upstream/master'

0914ff6

Merge remote-tracking branch 'upstream/master'

1ff6b15

Merge remote-tracking branch 'upstream/master'

3c47492

zuercher reviewed Jul 4, 2020

View reviewed changes

source/server/admin/stats_handler.cc Outdated Show resolved Hide resolved

dio added 2 commits July 5, 2020 13:12

Introduce Utility::parseAndDecodeQueryString

a098ee0

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

fb30a22

dio changed the title ~~http, utility: Optionally do URL decoding when parsing query string~~ http, utility: Introduce Utility::parseAndDecodeQueryString Jul 5, 2020

zuercher requested changes Jul 7, 2020

View reviewed changes

dio added 2 commits July 8, 2020 06:57

Do decoding only for param values

9ad3a40

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Add flag

ce9d248

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio changed the title ~~http, utility: Introduce Utility::parseAndDecodeQueryString~~ http, utility: Allow to optionally decode the parsed param value when parsing query string Jul 8, 2020

dio added 4 commits July 8, 2020 08:04

Decode by default

451d0c9

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

WIP on defaulting to true

e570e9c

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Exclude udpa parsing

347f3e5

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Missing param

c41bb5e

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

zuercher reviewed Jul 8, 2020

View reviewed changes

dio added 2 commits July 9, 2020 07:47

parseAndDecodeQueryString

9da88a1

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

b37bac6

dio requested review from lizan and mattklein123 as code owners July 9, 2020 00:48

zuercher reviewed Jul 9, 2020

View reviewed changes

dio added 2 commits July 10, 2020 05:48

Merge remote-tracking branch 'upstream/master'

3db0a57

Revernt to non decoding for other than admin

3dd1564

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

581bc5c

zuercher approved these changes Jul 10, 2020

View reviewed changes

dio merged commit cb03985 into envoyproxy:master Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

dio commented Jun 29, 2020 •

edited

Loading

dio commented Jun 30, 2020

azure-pipelines bot commented Jun 30, 2020

zuercher Jul 7, 2020

dio Jul 7, 2020

zuercher Jul 8, 2020

zuercher Jul 8, 2020

zuercher left a comment

zuercher Jul 9, 2020

dio Jul 9, 2020

http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

Conversation

dio commented Jun 29, 2020 • edited Loading

dio commented Jun 30, 2020

azure-pipelines bot commented Jun 30, 2020

zuercher Jul 7, 2020

Choose a reason for hiding this comment

dio Jul 7, 2020

Choose a reason for hiding this comment

zuercher Jul 8, 2020

Choose a reason for hiding this comment

zuercher Jul 8, 2020

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

zuercher Jul 9, 2020

Choose a reason for hiding this comment

dio Jul 9, 2020

Choose a reason for hiding this comment

dio commented Jun 29, 2020 •

edited

Loading