Make UHV consistent with existing decoding of slashes in the URL :path

Signed-off-by: Yan Avlasov <yavlasov@google.com>

envoy/http/header_validator_errors.h

@@ -21,6 +21,7 @@ struct UhvResponseCodeDetailValues {
   const std::string InvalidPseudoHeader = "uhv.invalid_pseudo_header";
   const std::string InvalidHostDeprecatedUserInfo = "uhv.invalid_host_deprecated_user_info";
   const std::string FragmentInUrlPath = "uhv.fragment_in_url_path";
+  const std::string EscapedSlashesInPath = "uhv.escaped_slashes_in_url_path";
 };
 
 using UhvResponseCodeDetail = ConstSingleton<UhvResponseCodeDetailValues>;
@@ -34,5 +35,11 @@ struct Http1ResponseCodeDetailValues {
 
 using Http1ResponseCodeDetail = ConstSingleton<Http1ResponseCodeDetailValues>;
 
+struct PathNormalizerResponseCodeDetailValues {
+  const std::string RedirectNormalized = "uhv.path_normalization_redirect";
+};
+
+using PathNormalizerResponseCodeDetail = ConstSingleton<PathNormalizerResponseCodeDetailValues>;
+
 } // namespace Http
 } // namespace Envoy

source/common/http/conn_manager_impl.cc

@@ -1007,10 +1007,11 @@ bool ConnectionManagerImpl::ActiveStream::validateHeaders() {
       } else {
         sendLocalReply(response_code, "", modify_headers, grpc_status, failure_details);
         // Pre UHV HCM did not respect stream_error_on_invalid_http_message
-        // and only sent 400 when :path contained fragment
-        // TODO(#28555): make this error respect the stream_error_on_invalid_http_message
+        // and only sent 400 when :path contained fragment or encoded slashes
+        // TODO(#28555): make these errors respect the stream_error_on_invalid_http_message
         if (!response_encoder_->streamErrorOnInvalidHttpMessage() &&
-            failure_details != UhvResponseCodeDetail::get().FragmentInUrlPath) {
+            failure_details != UhvResponseCodeDetail::get().FragmentInUrlPath &&
+            failure_details != UhvResponseCodeDetail::get().EscapedSlashesInPath) {
           connection_manager_.handleCodecError(failure_details);
         }
       }

source/extensions/filters/network/http_connection_manager/config.cc

@@ -126,10 +126,10 @@ envoy::extensions::filters::network::http_connection_manager::v3::HttpConnection
       KEEP_UNCHANGED;
 }
 
-Http::HeaderValidatorFactoryPtr createHeaderValidatorFactory(
-    [[maybe_unused]] const envoy::extensions::filters::network::http_connection_manager::v3::
-        HttpConnectionManager& config,
-    [[maybe_unused]] Server::Configuration::ServerFactoryContext& server_context) {
+Http::HeaderValidatorFactoryPtr
+createHeaderValidatorFactory([[maybe_unused]] const envoy::extensions::filters::network::
+                                 http_connection_manager::v3::HttpConnectionManager& config,
+                             [[maybe_unused]] Server::Configuration::FactoryContext& context) {
 
   Http::HeaderValidatorFactoryPtr header_validator_factory;
 #ifdef ENVOY_ENABLE_UHV
@@ -148,7 +148,7 @@ Http::HeaderValidatorFactoryPtr createHeaderValidatorFactory(
         static_cast<
             ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig::
                 UriPathNormalizationOptions::PathWithEscapedSlashesAction>(
-            config.path_with_escaped_slashes_action()));
+            getPathWithEscapedSlashesAction(config, context)));
     uhv_config.mutable_http1_protocol_options()->set_allow_chunked_length(
         config.http_protocol_options().allow_chunked_length());
     uhv_config.set_headers_with_underscores_action(
@@ -172,8 +172,8 @@ Http::HeaderValidatorFactoryPtr createHeaderValidatorFactory(
         fmt::format("Header validator extension not found: '{}'", header_validator_config.name()));
   }
 
-  header_validator_factory =
-      factory->createFromProto(header_validator_config.typed_config(), server_context);
+  header_validator_factory = factory->createFromProto(header_validator_config.typed_config(),
+                                                      context.getServerFactoryContext());
   if (!header_validator_factory) {
     throw EnvoyException(fmt::format("Header validator extension could not be created: '{}'",
                                      header_validator_config.name()));
@@ -381,8 +381,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
                                ? std::make_unique<HttpConnectionManagerProto::ProxyStatusConfig>(
                                      config.proxy_status_config())
                                : nullptr),
-      header_validator_factory_(
-          createHeaderValidatorFactory(config, context.getServerFactoryContext())),
+      header_validator_factory_(createHeaderValidatorFactory(config, context)),
       append_x_forwarded_port_(config.append_x_forwarded_port()),
       add_proxy_protocol_connection_state_(
           PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, add_proxy_protocol_connection_state, true)) {

source/extensions/http/header_validators/envoy_default/header_validator.cc

@@ -4,6 +4,7 @@
 
 #include "envoy/http/header_validator_errors.h"
 
+#include "source/common/http/path_utility.h"
 #include "source/common/runtime/runtime_features.h"
 #include "source/extensions/http/header_validators/envoy_default/character_tables.h"
 
@@ -46,6 +47,7 @@ constexpr std::array<uint32_t, 8> kPathHeaderCharTableWithBackSlashAllowed = {
 
 using ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig;
 using ::Envoy::Http::HeaderString;
+using ::Envoy::Http::PathUtil;
 using ::Envoy::Http::Protocol;
 using ::Envoy::Http::testCharInTable;
 using ::Envoy::Http::UhvResponseCodeDetail;
@@ -584,6 +586,37 @@ void HeaderValidator::sanitizePathWithFragment(::Envoy::Http::RequestHeaderMap&
   }
 }
 
+PathNormalizer::PathNormalizationResult
+HeaderValidator::sanitizeEncodedSlashes(::Envoy::Http::RequestHeaderMap& header_map) {
+  if (!header_map.Path()) {
+    return PathNormalizer::PathNormalizationResult::success();
+  }
+  const auto escaped_slashes_action =
+      config_.uri_path_normalization_options().path_with_escaped_slashes_action();
+
+  if (escaped_slashes_action !=
+      HeaderValidatorConfig::UriPathNormalizationOptions::KEEP_UNCHANGED) {
+    // When path normalization is enabled decoding of slashes is done as part of the normalization
+    // function for performance.
+    auto escaped_slashes_result = PathUtil::unescapeSlashes(header_map);
+    if (escaped_slashes_result == PathUtil::UnescapeSlashesResult::FoundAndUnescaped) {
+      if (escaped_slashes_action ==
+          HeaderValidatorConfig::UriPathNormalizationOptions::REJECT_REQUEST) {
+        return {PathNormalizer::PathNormalizationResult::Action::Reject,
+                UhvResponseCodeDetail::get().EscapedSlashesInPath};
+      } else if (escaped_slashes_action ==
+                 HeaderValidatorConfig::UriPathNormalizationOptions::UNESCAPE_AND_REDIRECT) {
+        return {PathNormalizer::PathNormalizationResult::Action::Redirect,
+                ::Envoy::Http::PathNormalizerResponseCodeDetail::get().RedirectNormalized};
+      } else {
+        ASSERT(escaped_slashes_action ==
+               HeaderValidatorConfig::UriPathNormalizationOptions::UNESCAPE_AND_FORWARD);
+      }
+    }
+  }
+  return PathNormalizer::PathNormalizationResult::success();
+}
+
 } // namespace EnvoyDefault
 } // namespace HeaderValidators
 } // namespace Http

source/extensions/http/header_validators/envoy_default/header_validator.h

@@ -169,6 +169,12 @@ class HeaderValidator {
    */
   void sanitizePathWithFragment(::Envoy::Http::RequestHeaderMap& header_map);
 
+  /**
+   * Decode percent-encoded slash characters based on configuration.
+   */
+  PathNormalizer::PathNormalizationResult
+  sanitizeEncodedSlashes(::Envoy::Http::RequestHeaderMap& header_map);
+
   const envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig
       config_;
   ::Envoy::Http::Protocol protocol_;

source/extensions/http/header_validators/envoy_default/header_validator_factory.cc

@@ -12,8 +12,22 @@ namespace EnvoyDefault {
 using ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig;
 using ::Envoy::Http::Protocol;
 
+namespace {
+
+HeaderValidatorConfig setHeaderValidatorConfigDefaults(const HeaderValidatorConfig& config) {
+  HeaderValidatorConfig new_config(config);
+  if (new_config.uri_path_normalization_options().path_with_escaped_slashes_action() ==
+      HeaderValidatorConfig::UriPathNormalizationOptions::IMPLEMENTATION_SPECIFIC_DEFAULT) {
+    new_config.mutable_uri_path_normalization_options()->set_path_with_escaped_slashes_action(
+        HeaderValidatorConfig::UriPathNormalizationOptions::KEEP_UNCHANGED);
+  }
+  return new_config;
+}
+
+} // namespace
+
 HeaderValidatorFactory::HeaderValidatorFactory(const HeaderValidatorConfig& config)
-    : config_(config) {}
+    : config_(setHeaderValidatorConfigDefaults(config)) {}
 
 ::Envoy::Http::ServerHeaderValidatorPtr
 HeaderValidatorFactory::createServerHeaderValidator(Protocol protocol,

source/extensions/http/header_validators/envoy_default/header_validator_factory.h

@@ -23,6 +23,11 @@ class HeaderValidatorFactory : public ::Envoy::Http::HeaderValidatorFactory {
   createClientHeaderValidator(::Envoy::Http::Protocol protocol,
                               ::Envoy::Http::HeaderValidatorStats& stats) override;
 
+  const envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig&
+  getConfigurationForTestsOnly() const {
+    return config_;
+  }
+
 private:
   const envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig
       config_;

source/extensions/http/header_validators/envoy_default/http1_header_validator.cc

@@ -313,6 +313,13 @@ ServerHttp1HeaderValidator::transformRequestHeaders(::Envoy::Http::RequestHeader
     if (!path_result.ok()) {
       return path_result;
     }
+  } else {
+    // Path normalization includes sanitization of encoded slashes for performance reasons.
+    // If normalization is disabled, sanitize encoded slashes here
+    auto result = sanitizeEncodedSlashes(header_map);
+    if (!result.ok()) {
+      return result;
+    }
   }
   return ::Envoy::Http::ServerHeaderValidator::RequestHeadersTransformationResult::success();
 }

source/extensions/http/header_validators/envoy_default/http2_header_validator.cc

@@ -434,6 +434,13 @@ ServerHttp2HeaderValidator::transformRequestHeaders(::Envoy::Http::RequestHeader
     if (!path_result.ok()) {
       return path_result;
     }
+  } else {
+    // Path normalization includes sanitization of encoded slashes for performance reasons.
+    // If normalization is disabled, sanitize encoded slashes here
+    auto result = sanitizeEncodedSlashes(header_map);
+    if (!result.ok()) {
+      return result;
+    }
   }
 
   // Transform H/2 extended CONNECT to H/1 UPGRADE, so that request processing always observes H/1

source/extensions/http/header_validators/envoy_default/path_normalizer.cc

@@ -19,16 +19,11 @@ using ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderVal
 using ::envoy::extensions::http::header_validators::envoy_default::v3::
     HeaderValidatorConfig_UriPathNormalizationOptions;
 using ::Envoy::Http::HeaderUtility;
+using ::Envoy::Http::PathNormalizerResponseCodeDetail;
 using ::Envoy::Http::RequestHeaderMap;
 using ::Envoy::Http::testCharInTable;
 using ::Envoy::Http::UhvResponseCodeDetail;
 
-struct PathNormalizerResponseCodeDetailValues {
-  const std::string RedirectNormalized = "uhv.path_noramlization_redirect";
-};
-
-using PathNormalizerResponseCodeDetail = ConstSingleton<PathNormalizerResponseCodeDetailValues>;
-
 PathNormalizer::PathNormalizer(const HeaderValidatorConfig& config) : config_(config) {}
 
 PathNormalizer::DecodedOctet
@@ -92,7 +87,7 @@ PathNormalizer::normalizeAndDecodeOctet(std::string::iterator iter,
     return {PercentDecodeResult::Decoded, ch};
   }
 
-  if (ch == '/') {
+  if (ch == '/' || ch == '\\') {
     // We decoded a slash character and how we handle it depends on the active configuration.
     switch (config_.uri_path_normalization_options().path_with_escaped_slashes_action()) {
     case HeaderValidatorConfig_UriPathNormalizationOptions::IMPLEMENTATION_SPECIFIC_DEFAULT:

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -2808,11 +2808,11 @@ TEST_F(HttpConnectionManagerConfigTest, PathWithEscapedSlashesActionDefault) {
 
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               featureEnabled(_, An<const envoy::type::v3::FractionalPercent&>()))
-      .WillOnce(Return(true));
+      .WillRepeatedly(Return(true));
   EXPECT_CALL(context_.runtime_loader_.snapshot_, getInteger(_, _)).Times(AnyNumber());
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               getInteger("http_connection_manager.path_with_escaped_slashes_action", 0))
-      .WillOnce(Return(0));
+      .WillRepeatedly(Return(0));
   HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_,
                                      date_provider_, route_config_provider_manager_,
                                      scoped_routes_config_provider_manager_, tracer_manager_,
@@ -2840,7 +2840,7 @@ TEST_F(HttpConnectionManagerConfigTest, PathWithEscapedSlashesActionDefaultOverr
   EXPECT_CALL(context_.runtime_loader_.snapshot_, getInteger(_, _)).Times(AnyNumber());
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               getInteger("http_connection_manager.path_with_escaped_slashes_action", 0))
-      .WillOnce(Return(3));
+      .WillRepeatedly(Return(3));
   HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_,
                                      date_provider_, route_config_provider_manager_,
                                      scoped_routes_config_provider_manager_, tracer_manager_,
@@ -2852,7 +2852,7 @@ TEST_F(HttpConnectionManagerConfigTest, PathWithEscapedSlashesActionDefaultOverr
   // Check the UNESCAPE_AND_FORWARD override to mollify coverage check
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               getInteger("http_connection_manager.path_with_escaped_slashes_action", 0))
-      .WillOnce(Return(4));
+      .WillRepeatedly(Return(4));
   HttpConnectionManagerConfig config1(parseHttpConnectionManagerFromYaml(yaml_string), context_,
                                       date_provider_, route_config_provider_manager_,
                                       scoped_routes_config_provider_manager_, tracer_manager_,
@@ -2878,7 +2878,7 @@ TEST_F(HttpConnectionManagerConfigTest,
 
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               featureEnabled(_, An<const envoy::type::v3::FractionalPercent&>()))
-      .WillOnce(Return(true));
+      .WillRepeatedly(Return(true));
   EXPECT_CALL(context_.runtime_loader_.snapshot_, getInteger(_, _)).Times(AnyNumber());
   // When configuration value is not the IMPLEMENTATION_SPECIFIC_DEFAULT the runtime override should
   // not even be considered.
@@ -2913,7 +2913,7 @@ TEST_F(HttpConnectionManagerConfigTest, PathWithEscapedSlashesActionDefaultOverr
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               featureEnabled("http_connection_manager.path_with_escaped_slashes_action_enabled",
                              An<const envoy::type::v3::FractionalPercent&>()))
-      .WillOnce(Return(false));
+      .WillRepeatedly(Return(false));
   EXPECT_CALL(context_.runtime_loader_.snapshot_, getInteger(_, _)).Times(AnyNumber());
   EXPECT_CALL(context_.runtime_loader_.snapshot_,
               getInteger("http_connection_manager.path_with_escaped_slashes_action", 0))
@@ -3084,7 +3084,7 @@ TEST_F(HttpConnectionManagerConfigTest, DefaultHeaderValidatorConfig) {
   EXPECT_TRUE(proto_config.uri_path_normalization_options().skip_merging_slashes());
   EXPECT_EQ(proto_config.uri_path_normalization_options().path_with_escaped_slashes_action(),
             ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig::
-                UriPathNormalizationOptions::IMPLEMENTATION_SPECIFIC_DEFAULT);
+                UriPathNormalizationOptions::KEEP_UNCHANGED);
   EXPECT_FALSE(proto_config.http1_protocol_options().allow_chunked_length());
 #else
   // If UHV is disabled, config should be accepted and factory should be nullptr
@@ -3117,9 +3117,9 @@ TEST_F(HttpConnectionManagerConfigTest, TranslateLegacyConfigToDefaultHeaderVali
       proto_config;
   DefaultHeaderValidatorFactoryConfigOverride factory(proto_config);
   Registry::InjectFactory<Http::HeaderValidatorFactoryConfig> registration(factory);
-  EXPECT_CALL(context_.runtime_loader_.snapshot_, featureEnabled(_, An<uint64_t>()))
-      .WillRepeatedly(Invoke(&context_.runtime_loader_.snapshot_,
-                             &Runtime::MockSnapshot::featureEnabledDefault));
+  EXPECT_CALL(context_.runtime_loader_.snapshot_,
+              featureEnabled(_, An<const envoy::type::v3::FractionalPercent&>()))
+      .WillRepeatedly(Return(true));
   EXPECT_CALL(context_.runtime_loader_.snapshot_, getInteger(_, _)).Times(AnyNumber());
   HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_,
                                      date_provider_, route_config_provider_manager_,

test/extensions/http/header_validators/envoy_default/config_test.cc

@@ -2,6 +2,7 @@
 #include "envoy/registry/registry.h"
 
 #include "source/extensions/http/header_validators/envoy_default/config.h"
+#include "source/extensions/http/header_validators/envoy_default/header_validator_factory.h"
 
 #include "test/mocks/server/instance.h"
 #include "test/test_common/utility.h"
@@ -14,6 +15,8 @@ namespace Http {
 namespace HeaderValidators {
 namespace EnvoyDefault {
 
+using ::envoy::extensions::http::header_validators::envoy_default::v3::HeaderValidatorConfig;
+
 TEST(EnvoyDefaultUhvFactoryTest, Basic) {
   auto* factory = Registry::FactoryRegistry<Envoy::Http::HeaderValidatorFactoryConfig>::getFactory(
       "envoy.http.header_validators.envoy_default");
@@ -33,6 +36,33 @@ TEST(EnvoyDefaultUhvFactoryTest, Basic) {
   EXPECT_NE(factory->createFromProto(typed_config.typed_config(), server_context), nullptr);
 }
 
+TEST(EnvoyDefaultUhvFactoryTest, PathWithEscapedSlashesDefaultValue) {
+  auto* factory = Registry::FactoryRegistry<Envoy::Http::HeaderValidatorFactoryConfig>::getFactory(
+      "envoy.http.header_validators.envoy_default");
+  ASSERT_NE(factory, nullptr);
+
+  envoy::config::core::v3::TypedExtensionConfig typed_config;
+  const std::string yaml = R"EOF(
+    name: envoy.http.header_validators.envoy_default
+    typed_config:
+        "@type": type.googleapis.com/envoy.extensions.http.header_validators.envoy_default.v3.HeaderValidatorConfig
+)EOF";
+  TestUtility::loadFromYaml(yaml, typed_config);
+
+  ::testing::NiceMock<Server::Configuration::MockServerFactoryContext> server_context;
+  auto uhv_factory = factory->createFromProto(typed_config.typed_config(), server_context);
+  // Cast to the expected type to examine configuration values
+  auto* uhv_factory_ptr = static_cast<
+      ::Envoy::Extensions::Http::HeaderValidators::EnvoyDefault::HeaderValidatorFactory*>(
+      uhv_factory.get());
+  // Expect that the IMPLEMENTATION_SPECIFIC_DEFAULT (the default enum value) was replaced with the
+  // KEEP_UNCHANGED
+  EXPECT_EQ(uhv_factory_ptr->getConfigurationForTestsOnly()
+                .uri_path_normalization_options()
+                .path_with_escaped_slashes_action(),
+            HeaderValidatorConfig::UriPathNormalizationOptions::KEEP_UNCHANGED);
+}
+
 } // namespace EnvoyDefault
 } // namespace HeaderValidators
 } // namespace Http

test/extensions/http/header_validators/envoy_default/header_validator_test.h

@@ -62,6 +62,24 @@ class HeaderValidatorTest {
   static constexpr absl::string_view fragment_in_path_allowed = R"EOF(
     strip_fragment_from_path: true
     )EOF";
+
+  static constexpr absl::string_view no_path_normalization_no_decoding_slashes = R"EOF(
+    uri_path_normalization_options:
+      skip_path_normalization: true
+      path_with_escaped_slashes_action: KEEP_UNCHANGED
+    )EOF";
+
+  static constexpr absl::string_view decode_slashes_and_forward = R"EOF(
+    uri_path_normalization_options:
+      skip_path_normalization: true
+      path_with_escaped_slashes_action: UNESCAPE_AND_FORWARD
+    )EOF";
+
+  static constexpr absl::string_view reject_encoded_slashes = R"EOF(
+    uri_path_normalization_options:
+      skip_path_normalization: true
+      path_with_escaped_slashes_action: REJECT_REQUEST
+    )EOF";
 };
 
 } // namespace EnvoyDefault