-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http, utility: Allow to optionally decode the parsed param value when parsing query string #11795
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>
/azp run envoy-presubmit |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should decode param names as well?
source/common/http/utility.cc
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I still think the separate methods was easier to read.
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, last thing.
source/common/router/config_impl.cc
Outdated
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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>
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