diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index b56f3da39cb1..658d580ad089 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -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()); + } } }; diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 08dbcd6eedf9..6ac7297004ec 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -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: @@ -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 @@ -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(header_name))); } diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 3c488aaa0e27..d137b3e5cda8 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -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 diff --git a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc index 09dc91254794..669dcbd28b60 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -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;