Add validations for types in TypeChecker declarations

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

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",
     ],
 )
 

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<FunctionDecl> 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();

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(),
                                  {});