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#11795dio merged 19 commits intoenvoyproxy:masterfrom dio:url-decode
Conversation
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>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
dio
changed the title
|
/azp run envoy-presubmit |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
dio
changed the title
source/common/http/utility.cc
Outdated
| } | ||
|
|
||
| start++; | ||
| return parseParameters(PercentEncoding::decode(StringUtil::subspan(url, start, url.size())), 0); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
dio
changed the title
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
source/common/http/utility.cc
Outdated
| const auto param_value = StringUtil::subspan(data, start + equal + 1, end); | ||
| params.emplace(StringUtil::subspan(data, start, start + equal), | ||
| StringUtil::subspan(data, start + equal + 1, end)); | ||
| decode_param_value ? PercentEncoding::decode(param_value) : param_value); |
There was a problem hiding this comment.
Should decode param names as well?
source/common/http/utility.cc
Outdated
| } | ||
|
|
||
| Utility::QueryParams Utility::parseQueryString(absl::string_view url) { | ||
| Utility::QueryParams Utility::parseQueryString(absl::string_view url, bool decode_param_value) { |
There was a problem hiding this comment.
Sorry, but I still think the separate methods was easier to read.
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
source/common/router/config_impl.cc
Outdated
| if (!config_query_parameters_.empty()) { | ||
| Http::Utility::QueryParams query_parameters = | ||
| Http::Utility::parseQueryString(headers.getPathValue()); | ||
| Http::Utility::parseAndDecodeQueryString(headers.getPathValue()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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>
2 participants
Commit Message: This introduces
parseAndDecodeQueryStringmethod 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