diff --git a/checker/checker_options.h b/checker/checker_options.h index 0ca706088..8c1ca648b 100644 --- a/checker/checker_options.h +++ b/checker/checker_options.h @@ -43,6 +43,12 @@ struct CheckerOptions { // as parsed. bool update_struct_type_names = true; + // Temporary flag to enable type parameter name validation. + // + // When enabled, the TypeCheckerBuilder will validate that type parameter + // names are simple identifiers when declared. + bool enable_type_parameter_name_validation = false; + // Well-known types defined by protobuf are treated specially in CEL, and // generally don't behave like other messages as runtime values. When used as // context declarations, this introduces some ambiguity about the intended @@ -68,6 +74,17 @@ struct CheckerOptions { // If exceeded, the checker will stop processing the ast and return // the current set of issues. int max_error_issues = 20; + + // Maximum amount of nesting allowed for type declarations in function + // signatures and variable declarations. + // + // If exceeded, the TypeCheckerBuilder will report an error when adding the + // declaration. + // + // For untrusted declarations, the caller should set a lower limit to mitigate + // expressions that compound nesting e.g. + // type5(T)->type(type(type(type(type(T)))))); type5(type5(T)) -> type10(T) + int max_type_decl_nesting = 13; }; } // namespace cel diff --git a/checker/internal/BUILD b/checker/internal/BUILD index 6d2254856..05058618d 100644 --- a/checker/internal/BUILD +++ b/checker/internal/BUILD @@ -141,6 +141,7 @@ cc_library( "//common:type_kind", "//common/ast:ast_impl", "//common/ast:expr", + "//internal:lexis", "//internal:status_macros", "//parser:macro", "@com_google_absl//absl/base:no_destructor", @@ -201,7 +202,6 @@ cc_test( ":test_ast_helpers", ":type_checker_impl", "//checker:checker_options", - "//checker:standard_library", "//checker:type_checker", "//checker:validation_result", "//common:decl", @@ -215,6 +215,7 @@ cc_test( "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings:string_view", "@com_google_absl//absl/types:optional", + "@com_google_protobuf//:protobuf", ], ) diff --git a/checker/internal/type_checker_builder_impl.cc b/checker/internal/type_checker_builder_impl.cc index 17a69eba6..ea869056d 100644 --- a/checker/internal/type_checker_builder_impl.cc +++ b/checker/internal/type_checker_builder_impl.cc @@ -37,6 +37,8 @@ #include "common/decl.h" #include "common/type.h" #include "common/type_introspector.h" +#include "common/type_kind.h" +#include "internal/lexis.h" #include "internal/status_macros.h" #include "parser/macro.h" #include "google/protobuf/descriptor.h" @@ -141,6 +143,78 @@ absl::optional FilterDecl(FunctionDecl decl, return filtered; } +absl::Status ValidateType(const Type& t, bool check_type_param_name, + int depth_limit, int remaining_depth) { + if (remaining_depth-- <= 0) { + return absl::InvalidArgumentError( + absl::StrCat("type nesting limit of ", depth_limit, " exceeded")); + } + switch (t.kind()) { + case TypeKind::kTypeParam: { + if (!check_type_param_name) { + return absl::OkStatus(); + } + const TypeParamType& type_param = t.GetTypeParam(); + if (!internal::LexisIsIdentifier(type_param.name())) { + return absl::InvalidArgumentError( + absl::StrCat("type parameter name '", type_param.name(), + "' is not a valid identifier")); + } + return absl::OkStatus(); + } + case TypeKind::kList: { + Type element_type = t.AsList()->GetElement(); + return ValidateType(element_type, check_type_param_name, depth_limit, + remaining_depth); + } + case TypeKind::kMap: { + Type key_type = t.AsMap()->GetKey(); + Type value_type = t.AsMap()->GetValue(); + CEL_RETURN_IF_ERROR(ValidateType(key_type, check_type_param_name, + depth_limit, remaining_depth)); + return ValidateType(value_type, check_type_param_name, depth_limit, + remaining_depth); + } + case TypeKind::kOpaque: { + for (Type type_param : t.AsOpaque()->GetParameters()) { + CEL_RETURN_IF_ERROR(ValidateType(type_param, check_type_param_name, + depth_limit, remaining_depth)); + } + return absl::OkStatus(); + } + case TypeKind::kType: { + for (Type type_param : t.AsType()->GetParameters()) { + CEL_RETURN_IF_ERROR(ValidateType(type_param, check_type_param_name, + depth_limit, remaining_depth)); + } + return absl::OkStatus(); + } + default: + break; + } + return absl::OkStatus(); +} + +absl::Status ValidateFunctionDecl(const FunctionDecl& decl, + bool check_type_param_name, int depth_limit) { + CEL_RETURN_IF_ERROR(CheckStdMacroOverlap(decl)); + for (const auto& ovl : decl.overloads()) { + CEL_RETURN_IF_ERROR(ValidateType(ovl.result(), check_type_param_name, + depth_limit, depth_limit)); + for (const auto& arg : ovl.args()) { + CEL_RETURN_IF_ERROR( + ValidateType(arg, check_type_param_name, depth_limit, depth_limit)); + } + } + return absl::OkStatus(); +} + +absl::Status ValidateVariableDecl(const VariableDecl& decl, + bool check_type_param_name, int depth_limit) { + return ValidateType(decl.type(), check_type_param_name, depth_limit, + depth_limit); +} + } // namespace absl::Status TypeCheckerBuilderImpl::BuildLibraryConfig( @@ -290,12 +364,18 @@ absl::Status TypeCheckerBuilderImpl::AddLibrarySubset( } absl::Status TypeCheckerBuilderImpl::AddVariable(const VariableDecl& decl) { + CEL_RETURN_IF_ERROR( + ValidateVariableDecl(decl, options_.enable_type_parameter_name_validation, + options_.max_type_decl_nesting)); target_config_->variables.push_back({decl, AddSemantic::kInsertIfAbsent}); return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::AddOrReplaceVariable( const VariableDecl& decl) { + CEL_RETURN_IF_ERROR( + ValidateVariableDecl(decl, options_.enable_type_parameter_name_validation, + options_.max_type_decl_nesting)); target_config_->variables.push_back({decl, AddSemantic::kInsertOrReplace}); return absl::OkStatus(); } @@ -327,14 +407,18 @@ absl::Status TypeCheckerBuilderImpl::AddContextDeclaration( } absl::Status TypeCheckerBuilderImpl::AddFunction(const FunctionDecl& decl) { - CEL_RETURN_IF_ERROR(CheckStdMacroOverlap(decl)); + CEL_RETURN_IF_ERROR( + ValidateFunctionDecl(decl, options_.enable_type_parameter_name_validation, + options_.max_type_decl_nesting)); target_config_->functions.push_back( {std::move(decl), AddSemantic::kInsertIfAbsent}); return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::MergeFunction(const FunctionDecl& decl) { - CEL_RETURN_IF_ERROR(CheckStdMacroOverlap(decl)); + CEL_RETURN_IF_ERROR( + ValidateFunctionDecl(decl, options_.enable_type_parameter_name_validation, + options_.max_type_decl_nesting)); target_config_->functions.push_back( {std::move(decl), AddSemantic::kTryMerge}); return absl::OkStatus(); diff --git a/checker/internal/type_checker_builder_impl_test.cc b/checker/internal/type_checker_builder_impl_test.cc index 6d1e3ac5a..6b3e7a890 100644 --- a/checker/internal/type_checker_builder_impl_test.cc +++ b/checker/internal/type_checker_builder_impl_test.cc @@ -23,6 +23,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "checker/checker_options.h" #include "checker/internal/test_ast_helpers.h" #include "checker/type_checker.h" #include "checker/validation_result.h" @@ -33,6 +34,7 @@ #include "common/type_introspector.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" +#include "google/protobuf/arena.h" namespace cel::checker_internal { namespace { @@ -205,6 +207,93 @@ TEST(ContextDeclsTest, ErrorOnOverlappingVariableDeclaration) { "variable 'single_int64' declared multiple times")); } +TEST(ContextDeclsTest, InvalidTypeParamNameVariableValidationDisabled) { + CheckerOptions options; + options.enable_type_parameter_name_validation = false; + TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), + options); + ASSERT_THAT(builder.AddVariable(MakeVariableDecl("x", TypeParamType(""))), + IsOk()); + ASSERT_THAT(builder.AddOrReplaceVariable( + MakeVariableDecl("x", TypeParamType("T% foo"))), + IsOk()); +} + +TEST(ContextDeclsTest, ErrorOnInvalidTypeParamNameVariable) { + CheckerOptions options; + options.enable_type_parameter_name_validation = true; + TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), + options); + ASSERT_THAT(builder.AddVariable(MakeVariableDecl("x", TypeParamType(""))), + StatusIs(absl::StatusCode::kInvalidArgument, + "type parameter name '' is not a valid identifier")); + ASSERT_THAT( + builder.AddOrReplaceVariable( + MakeVariableDecl("x", TypeParamType("T% foo"))), + StatusIs(absl::StatusCode::kInvalidArgument, + "type parameter name 'T% foo' is not a valid identifier")); +} + +TEST(ContextDeclsTest, ErrorOnTooDeepTypeNestingVariable) { + CheckerOptions options; + options.max_type_decl_nesting = 2; + google::protobuf::Arena arena; + + TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), + options); + ASSERT_THAT(builder.AddVariable( + MakeVariableDecl("x", TypeType(&arena, TypeParamType("T")))), + IsOk()); + ASSERT_THAT( + builder.AddOrReplaceVariable(MakeVariableDecl( + "x", TypeType(&arena, TypeType(&arena, TypeParamType("T% foo"))))), + StatusIs(absl::StatusCode::kInvalidArgument, + "type nesting limit of 2 exceeded")); +} + +TEST(ContextDeclsTest, ErrorOnInvalidTypeParamNameFunction) { + CheckerOptions options; + options.enable_type_parameter_name_validation = true; + TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), + options); + google::protobuf::Arena arena; + + ASSERT_OK_AND_ASSIGN( + auto fn_decl, + MakeFunctionDecl( + "type2", + MakeOverloadDecl("type2", TypeType(&arena, TypeParamType("")), + TypeParamType("")))); + ASSERT_THAT(builder.AddFunction(fn_decl), + StatusIs(absl::StatusCode::kInvalidArgument, + "type parameter name '' is not a valid identifier")); +} + +TEST(ContextDeclsTest, ErrorOnTooDeepTypeNestingFunction) { + CheckerOptions options; + options.max_type_decl_nesting = 2; + google::protobuf::Arena arena; + + TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), + options); + ASSERT_OK_AND_ASSIGN( + auto fn_decl, + MakeFunctionDecl( + "add", MakeOverloadDecl("add_int", IntType(), IntType(), IntType()))); + ASSERT_THAT(builder.AddFunction(fn_decl), IsOk()); + + Type list_type = ListType(&arena, ListType(&arena, IntType())); + + ASSERT_OK_AND_ASSIGN( + fn_decl, + MakeFunctionDecl("add", MakeOverloadDecl("add_list_list_int", list_type, + list_type, list_type))); + + ASSERT_THAT(builder.MergeFunction(fn_decl), + StatusIs(absl::StatusCode::kInvalidArgument, + "type nesting limit of 2 exceeded")); +} + TEST(ContextDeclsTest, ReplaceVariable) { TypeCheckerBuilderImpl builder(internal::GetSharedTestingDescriptorPool(), {});