-
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]: Fine grained path normalization controls implementation #15450
Changes from 29 commits
d8e1b04
effbb99
579c5b1
12cb83a
33e999f
d4330f3
2de9ab6
1540cdb
b45e089
93d74e1
7614247
0e08d96
0e6bca7
e9bbff2
91aff85
044d820
996b786
d39afbe
95137da
184359c
c2810d9
3d37a0f
e24a7e7
50a227e
544755a
e9a483d
b40bebb
0d7f899
afb4b6d
ee8b9a3
4786f15
ba7f746
43b95f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -461,6 +461,8 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl<RequestHeaderMap>, | |
INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) | ||
INLINE_REQ_RESP_STRING_HEADERS(DEFINE_INLINE_HEADER_STRING_FUNCS) | ||
INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) | ||
absl::string_view getForwardingPath() override { return forwarding_path_; } | ||
absl::string_view getFilterPath() override { return filter_path_; } | ||
|
||
protected: | ||
// NOTE: Because inline_headers_ is a variable size member, it must be the last member in the | ||
|
@@ -478,9 +480,11 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl<RequestHeaderMap>, | |
INLINE_REQ_RESP_STRING_HEADERS(DEFINE_HEADER_HANDLE) | ||
INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_HEADER_HANDLE) | ||
}; | ||
|
||
void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } | ||
void setFilterPath(absl::string_view path) override { filter_path_ = path; } | ||
std::string forwarding_path_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the intended use for these two be documented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
std::string filter_path_; | ||
using HeaderHandles = ConstSingleton<HeaderHandleValues>; | ||
|
||
RequestHeaderMapImpl() { clearInline(); } | ||
|
||
HeaderEntryImpl* inline_headers_[]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
#include "common/http/path_utility.h" | ||
|
||
#include "envoy/common/exception.h" | ||
|
||
#include "common/common/logger.h" | ||
#include "common/http/legacy_path_canonicalizer.h" | ||
#include "common/runtime/runtime_features.h" | ||
|
@@ -35,16 +37,56 @@ absl::optional<std::string> canonicalizePath(absl::string_view original_path) { | |
bool PathUtil::canonicalPath(RequestHeaderMap& headers) { | ||
ASSERT(headers.Path()); | ||
const auto original_path = headers.getPathValue(); | ||
// canonicalPath is supposed to apply on path component in URL instead of :path header | ||
const absl::optional<std::string> normalized_path = PathTransformer::rfcNormalize(original_path); | ||
if (!normalized_path.has_value()) { | ||
return false; | ||
} | ||
headers.setPath(normalized_path.value()); | ||
return true; | ||
} | ||
|
||
void PathUtil::mergeSlashes(RequestHeaderMap& headers) { | ||
ASSERT(headers.Path()); | ||
const auto original_path = headers.getPathValue(); | ||
const absl::optional<std::string> normalized = | ||
PathTransformer::mergeSlashes(original_path).value(); | ||
if (normalized.has_value()) { | ||
headers.setPath(normalized.value()); | ||
} | ||
} | ||
|
||
absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) { | ||
absl::string_view ret = path; | ||
// Trim query parameters and/or fragment if present. | ||
size_t offset = ret.find_first_of("?#"); | ||
if (offset != absl::string_view::npos) { | ||
ret.remove_suffix(ret.length() - offset); | ||
} | ||
return ret; | ||
} | ||
|
||
absl::optional<std::string> PathTransformer::mergeSlashes(absl::string_view original_path) { | ||
const absl::string_view::size_type query_start = original_path.find('?'); | ||
const absl::string_view path = original_path.substr(0, query_start); | ||
const absl::string_view query = absl::ClippedSubstr(original_path, query_start); | ||
if (path.find("//") == absl::string_view::npos) { | ||
return std::string(original_path); | ||
} | ||
const absl::string_view path_prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); | ||
const absl::string_view path_suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); | ||
return absl::StrCat(path_prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), | ||
path_suffix, query); | ||
} | ||
|
||
absl::optional<std::string> PathTransformer::rfcNormalize(absl::string_view original_path) { | ||
const auto query_pos = original_path.find('?'); | ||
auto normalized_path_opt = canonicalizePath( | ||
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
query_pos == original_path.npos | ||
? original_path | ||
: absl::string_view(original_path.data(), query_pos) // '?' is not included | ||
); | ||
|
||
if (!normalized_path_opt.has_value()) { | ||
return false; | ||
return {}; | ||
} | ||
auto& normalized_path = normalized_path_opt.value(); | ||
const absl::string_view query_suffix = | ||
|
@@ -54,35 +96,41 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { | |
if (!query_suffix.empty()) { | ||
normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); | ||
} | ||
headers.setPath(normalized_path); | ||
return true; | ||
return normalized_path; | ||
} | ||
|
||
void PathUtil::mergeSlashes(RequestHeaderMap& headers) { | ||
ASSERT(headers.Path()); | ||
const auto original_path = headers.getPathValue(); | ||
// Only operate on path component in URL. | ||
const absl::string_view::size_type query_start = original_path.find('?'); | ||
const absl::string_view path = original_path.substr(0, query_start); | ||
const absl::string_view query = absl::ClippedSubstr(original_path, query_start); | ||
if (path.find("//") == absl::string_view::npos) { | ||
return; | ||
PathTransformer::PathTransformer( | ||
envoy::type::http::v3::PathTransformation const& path_transformation) { | ||
const auto& operations = path_transformation.operations(); | ||
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
std::vector<uint64_t> operation_hashes; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depending on the length, it might be better to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be overkill to use a hash set here since we expect a very small number of transformation. |
||
for (auto const& operation : operations) { | ||
uint64_t operation_hash = MessageUtil::hash(operation); | ||
if (find(operation_hashes.begin(), operation_hashes.end(), operation_hash) != | ||
operation_hashes.end()) { | ||
// Currently we only have RFC normalization and merge slashes, don't expect duplicates for | ||
// these transformations. | ||
throw EnvoyException("Duplicate path transformation"); | ||
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if (operation.has_normalize_path_rfc_3986()) { | ||
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
transformations_.emplace_back(PathTransformer::rfcNormalize); | ||
} else if (operation.has_merge_slashes()) { | ||
transformations_.emplace_back(PathTransformer::mergeSlashes); | ||
} | ||
operation_hashes.push_back(operation_hash); | ||
} | ||
const absl::string_view path_prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); | ||
const absl::string_view path_suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); | ||
headers.setPath(absl::StrCat(path_prefix, | ||
absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), | ||
path_suffix, query)); | ||
} | ||
|
||
absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) { | ||
absl::string_view ret = path; | ||
// Trim query parameters and/or fragment if present. | ||
size_t offset = ret.find_first_of("?#"); | ||
if (offset != absl::string_view::npos) { | ||
ret.remove_suffix(ret.length() - offset); | ||
absl::optional<std::string> PathTransformer::transform(absl::string_view original) const { | ||
absl::optional<std::string> path_string = std::string(original); | ||
absl::string_view path_string_view = original; | ||
for (Transformation const& transformation : transformations_) { | ||
path_string = transformation(path_string_view); | ||
if (!path_string.has_value()) { | ||
return {}; | ||
} | ||
path_string_view = path_string.value(); | ||
} | ||
return ret; | ||
return path_string; | ||
} | ||
|
||
} // namespace Http | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
#pragma once | ||
|
||
#include <list> | ||
|
||
#include "envoy/http/header_map.h" | ||
#include "envoy/type/http/v3/path_transformation.pb.h" | ||
|
||
#include "common/protobuf/protobuf.h" | ||
#include "common/protobuf/utility.h" | ||
|
||
#include "absl/strings/string_view.h" | ||
|
||
|
@@ -24,5 +30,24 @@ class PathUtil { | |
static absl::string_view removeQueryAndFragment(const absl::string_view path); | ||
}; | ||
|
||
class PathTransformer { | ||
chaoqin-li1123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public: | ||
PathTransformer(envoy::type::http::v3::PathTransformation const& path_transformation); | ||
|
||
// Take a string_view as argument and return an optional string. | ||
// The optional will be null if the transformation fail. | ||
absl::optional<std::string> transform(const absl::string_view original_path) const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: could you mention that this will execute all configured transformations, or something along those lines? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, will add comments. |
||
|
||
static absl::optional<std::string> mergeSlashes(absl::string_view original_path); | ||
|
||
static absl::optional<std::string> rfcNormalize(absl::string_view original_path); | ||
|
||
private: | ||
using Transformation = std::function<absl::optional<std::string>(absl::string_view)>; | ||
// A sequence of transformations specified by path_transformation.operations() | ||
// Transformations will be applied to a path string in order in transform(). | ||
std::list<Transformation> transformations_; | ||
}; | ||
|
||
} // namespace Http | ||
} // namespace Envoy |
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.
Please add comments explaining what these methods are for.
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.
Thanks, will fix.