Skip to content

Check for unexpected target (function call receiver) expressions in builtin operator handlers. #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2025
Merged

Check for unexpected target (function call receiver) expressions in builtin operator handlers. #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions eval/compiler/flat_expr_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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()) {
Expand All @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
124 changes: 124 additions & 0 deletions eval/compiler/flat_expr_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'"));
Expand Down