Skip to content
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

Assert(valid) when ext_proc filter apply header mutations #27547

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,10 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) {
const auto mut_status = MutationUtils::applyHeaderMutations(
response.headers(), headers, false, immediateResponseChecker().checker(),
stats_.rejected_header_mutations_);
ENVOY_BUG(mut_status.ok(), "Immediate response mutations should not fail");
if (!mut_status.ok()) {
ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed with {}",
mut_status.message());
}
}
};

Expand Down
13 changes: 13 additions & 0 deletions source/extensions/filters/http/ext_proc/mutation_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
const Checker& checker,
Counter& rejected_mutations) {
for (const auto& hdr : mutation.remove_headers()) {
if (!Http::HeaderUtility::headerNameIsValid(hdr)) {
ENVOY_LOG(debug, "remove_headers contain invalid character, may not be removed.");
rejected_mutations.inc();
return absl::InvalidArgumentError("Invalid character in remove_headers mutation.");
}
const LowerCaseString remove_header(hdr);
switch (checker.check(CheckOperation::REMOVE, remove_header, "")) {
case CheckResult::OK:
Expand All @@ -62,6 +67,13 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
if (!sh.has_header()) {
continue;
}
if (!Http::HeaderUtility::headerNameIsValid(sh.header().key()) ||
!Http::HeaderUtility::headerValueIsValid(sh.header().value())) {
ENVOY_LOG(debug,
"set_headers contain invalid character in key or value, may not be appended.");
rejected_mutations.inc();
return absl::InvalidArgumentError("Invalid character in set_headers mutation.");
}
const LowerCaseString header_name(sh.header().key());
const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND
Expand All @@ -87,6 +99,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
break;
case CheckResult::FAIL:
ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name);
rejected_mutations.inc();
return absl::InvalidArgumentError(
absl::StrCat("Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1079,6 +1079,21 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediately) {
EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body());
}

TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithInvalidCharacter) {
initializeConfig();
HttpIntegrationTest::initialize();
auto response = sendDownstreamRequest(absl::nullopt);

processAndRespondImmediately(*grpc_upstreams_[0], true, [](ImmediateResponse& immediate) {
immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Unauthorized);
auto* hdr = immediate.mutable_headers()->add_set_headers();
hdr->mutable_header()->set_key("x-failure-reason\n");
hdr->mutable_header()->set_value("testing");
});

verifyDownstreamResponse(*response, 401);
}

// Test the filter using the default configuration by connecting to
// an ext_proc server that responds to the request_headers message
// by sending back an immediate_response message after the
Expand Down
40 changes: 40 additions & 0 deletions test/extensions/filters/http/ext_proc/mutation_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,46 @@ TEST(MutationUtils, TestNonAppendableHeaders) {
EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers));
}

TEST(MutationUtils, TestSetHeaderWithInvalidCharacter) {
Http::TestRequestHeaderMapImpl headers{
{":method", "GET"},
{"host", "localhost:1000"},
};
Checker checker(HeaderMutationRules::default_instance());
Envoy::Stats::MockCounter rejections;
envoy::service::ext_proc::v3::HeaderMutation mutation;
auto* s = mutation.add_set_headers();
// Test header key contains invalid character.
s->mutable_header()->set_key("x-append-this\n");
s->mutable_header()->set_value("value");
EXPECT_CALL(rejections, inc());
EXPECT_FALSE(
MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());

mutation.Clear();
s = mutation.add_set_headers();
// Test header value contains invalid character.
s->mutable_header()->set_key("x-append-this");
s->mutable_header()->set_value("value\r");
EXPECT_CALL(rejections, inc());
EXPECT_FALSE(
MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
}

TEST(MutationUtils, TestRemoveHeaderWithInvalidCharacter) {
Http::TestRequestHeaderMapImpl headers{
{":method", "GET"},
{"host", "localhost:1000"},
};
envoy::service::ext_proc::v3::HeaderMutation mutation;
mutation.add_remove_headers("host\n");
Checker checker(HeaderMutationRules::default_instance());
Envoy::Stats::MockCounter rejections;
EXPECT_CALL(rejections, inc());
EXPECT_FALSE(
MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
}

// Ensure that we actually replace the body
TEST(MutationUtils, TestBodyMutationReplace) {
Buffer::OwnedImpl buf;
Expand Down