From 4eee1edc88652687584fa9b8ca8e3739f954a88c Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Fri, 4 Apr 2025 12:10:49 -0700 Subject: [PATCH] Check for unexpected target (function call receiver) expressions in builtin operator handlers. PiperOrigin-RevId: 744027012 --- eval/compiler/flat_expr_builder.cc | 23 ++++- eval/compiler/flat_expr_builder_test.cc | 124 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 3 deletions(-) diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 3f0d40a82..bf2eb581a 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -1962,6 +1962,11 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleIndex( const cel::Expr& expr, const cel::CallExpr& call_expr) { ABSL_DCHECK(call_expr.function() == cel::builtin::kIndex); auto depth = RecursionEligible(); + if (!ValidateOrError( + call_expr.args().size() == 2 && !call_expr.has_target(), + "unexpected number of args for builtin index operator")) { + return CallHandlerResult::kIntercepted; + } if (depth.has_value()) { auto args = ExtractRecursiveDependencies(); @@ -1984,6 +1989,12 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleIndex( FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNot( const cel::Expr& expr, const cel::CallExpr& call_expr) { ABSL_DCHECK(call_expr.function() == cel::builtin::kNot); + + if (!ValidateOrError(call_expr.args().size() == 1 && !call_expr.has_target(), + "unexpected number of args for builtin not operator")) { + return CallHandlerResult::kIntercepted; + } + auto depth = RecursionEligible(); if (depth.has_value()) { @@ -2005,6 +2016,12 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNotStrictlyFalse( const cel::Expr& expr, const cel::CallExpr& call_expr) { auto depth = RecursionEligible(); + if (!ValidateOrError(call_expr.args().size() == 1 && !call_expr.has_target(), + "unexpected number of args for builtin " + "not_strictly_false operator")) { + return CallHandlerResult::kIntercepted; + } + if (depth.has_value()) { auto args = ExtractRecursiveDependencies(); if (args.size() != 1) { @@ -2026,7 +2043,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleBlock( const cel::Expr& expr, const cel::CallExpr& call_expr) { ABSL_DCHECK(call_expr.function() == kBlock); if (!block_.has_value() || block_->expr != &expr || - call_expr.args().size() != 2) { + call_expr.args().size() != 2 || call_expr.has_target()) { SetProgressStatusError( absl::InvalidArgumentError("unexpected call to internal cel.@block")); return CallHandlerResult::kIntercepted; @@ -2102,7 +2119,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleListAppend( FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleHeterogeneousEquality( const cel::Expr& expr, const cel::CallExpr& call, bool inequality) { if (!ValidateOrError( - call.args().size() == 2, + call.args().size() == 2 && !call.has_target(), "unexpected number of args for builtin equality operator")) { return CallHandlerResult::kIntercepted; } @@ -2128,7 +2145,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleHeterogeneousEquality( FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, const cel::CallExpr& call) { - if (!ValidateOrError(call.args().size() == 2, + if (!ValidateOrError(call.args().size() == 2 && !call.has_target(), "unexpected number of args for builtin 'in' operator")) { return CallHandlerResult::kIntercepted; } diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 8020d940c..5f20b5a55 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -1916,6 +1916,130 @@ TEST(FlatExprBuilderTest, FastEquality) { EXPECT_FALSE(result.BoolOrDie()); } +TEST(FlatExprBuilderTest, FastEqualityFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("'foo' == 'bar'")); + parsed_expr.mutable_expr() + ->mutable_call_expr() + ->mutable_target() + ->mutable_const_expr() + ->set_string_value("foo"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr( + "unexpected number of args for builtin equality operator"))); +} + +TEST(FlatExprBuilderTest, FastInequalityFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("'foo' != 'bar'")); + parsed_expr.mutable_expr() + ->mutable_call_expr() + ->mutable_target() + ->mutable_const_expr() + ->set_string_value("foo"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr( + "unexpected number of args for builtin equality operator"))); +} + +TEST(FlatExprBuilderTest, FastInFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("a in b")); + parsed_expr.mutable_expr() + ->mutable_call_expr() + ->mutable_target() + ->mutable_const_expr() + ->set_string_value("foo"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs( + absl::StatusCode::kInvalidArgument, + HasSubstr("unexpected number of args for builtin 'in' operator"))); +} + +TEST(FlatExprBuilderTest, IndexFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("a[b]")); + parsed_expr.mutable_expr() + ->mutable_call_expr() + ->mutable_target() + ->mutable_const_expr() + ->set_string_value("foo"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs( + absl::StatusCode::kInvalidArgument, + HasSubstr("unexpected number of args for builtin index operator"))); +} + +TEST(FlatExprBuilderTest, NotFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("!a")); + parsed_expr.mutable_expr() + ->mutable_call_expr() + ->mutable_target() + ->mutable_const_expr() + ->set_string_value("foo"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs( + absl::StatusCode::kInvalidArgument, + HasSubstr("unexpected number of args for builtin not operator"))); +} + +TEST(FlatExprBuilderTest, NotStrictlyFalseFiltersBadCalls) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("!a")); + auto* call = parsed_expr.mutable_expr()->mutable_call_expr(); + call->mutable_target()->mutable_const_expr()->set_string_value("foo"); + call->set_function("@not_strictly_false"); + cel::RuntimeOptions options; + options.enable_fast_builtins = true; + InterpreterOptions legacy_options; + legacy_options.enable_fast_builtins = true; + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry(), legacy_options), + IsOk()); + ASSERT_THAT( + builder.CreateExpression(&parsed_expr.expr(), &parsed_expr.source_info()), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("unexpected number of args for builtin " + "not_strictly_false operator"))); +} + TEST(FlatExprBuilderTest, FastEqualityDisabledWithCustomEquality) { TestMessage message; ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("1 == b'\001'"));