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

* ASSERT(valid()) when ext_proc filter apply header mutations.

Signed-off-by: Yanjun Xiang <[email protected]>

source/extensions/filters/http/ext_proc/ext_proc.cc

@@ -775,7 +775,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());
+      }
     }
   };
 

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<absl::string_view>(header_name)));
     }

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

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;