From bb0b9581b5f8b11bdf2b885d16f5b202351d3a66 Mon Sep 17 00:00:00 2001 From: Justin King Date: Wed, 17 Jun 2026 15:48:14 -0700 Subject: [PATCH] Refine iteration variable validation in CEL macros. This change introduces stricter validation for iteration variables in standard CEL macros like `all`, `exists`, `map`, and `filter`. Iteration variables must now be simple identifiers, disallowing names that start with a dot (e.g., `.x`) and preventing the use of the internal accumulator variable `__result__`. The validation logic is now shared between the standard macros and the comprehensions extension. PiperOrigin-RevId: 933966053 --- .../CelComprehensionsExtensions.java | 9 ++- .../java/dev/cel/parser/CelStandardMacro.java | 51 +++++++++++---- .../parser/CelParserParameterizedTest.java | 10 +++ .../src/test/resources/parser_errors.baseline | 64 ++++++++++++++++++- 4 files changed, 117 insertions(+), 17 deletions(-) diff --git a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java index 3bf47c4a6..14968099c 100644 --- a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java +++ b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java @@ -479,9 +479,8 @@ private static Optional transformMapEntryMacro( private static CelExpr validatedIterationVariable( CelMacroExprFactory exprFactory, CelExpr argument) { - CelExpr arg = checkNotNull(argument); - if (arg.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { + if (!isSimpleIdentifier(arg)) { return reportArgumentError(exprFactory, arg); } else if (arg.exprKind().ident().name().equals("__result__")) { return reportAccumulatorOverwriteError(exprFactory, arg); @@ -490,6 +489,12 @@ private static CelExpr validatedIterationVariable( } } + private static boolean isSimpleIdentifier(CelExpr expr) { + return expr.getKind() == CelExpr.ExprKind.Kind.IDENT + && !expr.ident().name().isEmpty() + && !expr.ident().name().startsWith("."); + } + private static CelExpr reportArgumentError(CelMacroExprFactory exprFactory, CelExpr argument) { return exprFactory.reportError( CelIssue.formatError( diff --git a/parser/src/main/java/dev/cel/parser/CelStandardMacro.java b/parser/src/main/java/dev/cel/parser/CelStandardMacro.java index 20d30bc17..275159569 100644 --- a/parser/src/main/java/dev/cel/parser/CelStandardMacro.java +++ b/parser/src/main/java/dev/cel/parser/CelStandardMacro.java @@ -122,9 +122,9 @@ private static Optional expandAllMacro( checkNotNull(exprFactory); checkNotNull(target); checkArgument(arguments.size() == 2); - CelExpr arg0 = checkNotNull(arguments.get(0)); + CelExpr arg0 = validatedIterationVariable(exprFactory, arguments.get(0)); if (arg0.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { - return Optional.of(reportArgumentError(exprFactory, arg0)); + return Optional.of(arg0); } CelExpr arg1 = checkNotNull(arguments.get(1)); CelExpr accuInit = exprFactory.newBoolLiteral(true); @@ -155,9 +155,9 @@ private static Optional expandExistsMacro( checkNotNull(exprFactory); checkNotNull(target); checkArgument(arguments.size() == 2); - CelExpr arg0 = checkNotNull(arguments.get(0)); + CelExpr arg0 = validatedIterationVariable(exprFactory, arguments.get(0)); if (arg0.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { - return Optional.of(reportArgumentError(exprFactory, arg0)); + return Optional.of(arg0); } CelExpr arg1 = checkNotNull(arguments.get(1)); CelExpr accuInit = exprFactory.newBoolLiteral(false); @@ -190,9 +190,9 @@ private static Optional expandExistsOneMacro( checkNotNull(exprFactory); checkNotNull(target); checkArgument(arguments.size() == 2); - CelExpr arg0 = checkNotNull(arguments.get(0)); + CelExpr arg0 = validatedIterationVariable(exprFactory, arguments.get(0)); if (arg0.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { - return Optional.of(reportArgumentError(exprFactory, arg0)); + return Optional.of(arg0); } CelExpr arg1 = checkNotNull(arguments.get(1)); CelExpr accuInit = exprFactory.newIntLiteral(0); @@ -228,12 +228,9 @@ private static Optional expandMapMacro( checkNotNull(exprFactory); checkNotNull(target); checkArgument(arguments.size() == 2 || arguments.size() == 3); - CelExpr arg0 = checkNotNull(arguments.get(0)); + CelExpr arg0 = validatedIterationVariable(exprFactory, arguments.get(0)); if (arg0.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { - return Optional.of( - exprFactory.reportError( - CelIssue.formatError( - exprFactory.getSourceLocation(arg0), "argument is not an identifier"))); + return Optional.of(arg0); } CelExpr arg1; CelExpr arg2; @@ -276,9 +273,9 @@ private static Optional expandFilterMacro( checkNotNull(exprFactory); checkNotNull(target); checkArgument(arguments.size() == 2); - CelExpr arg0 = checkNotNull(arguments.get(0)); + CelExpr arg0 = validatedIterationVariable(exprFactory, arguments.get(0)); if (arg0.exprKind().getKind() != CelExpr.ExprKind.Kind.IDENT) { - return Optional.of(reportArgumentError(exprFactory, arg0)); + return Optional.of(arg0); } CelExpr arg1 = checkNotNull(arguments.get(1)); CelExpr accuInit = exprFactory.newList(); @@ -305,9 +302,37 @@ private static Optional expandFilterMacro( exprFactory.newIdentifier(exprFactory.getAccumulatorVarName()))); } + private static CelExpr validatedIterationVariable( + CelMacroExprFactory exprFactory, CelExpr argument) { + CelExpr arg = checkNotNull(argument); + if (!isSimpleIdentifier(arg)) { + return reportArgumentError(exprFactory, arg); + } else if (arg.exprKind().ident().name().equals("__result__")) { + return reportAccumulatorOverwriteError(exprFactory, arg); + } else { + return arg; + } + } + + private static boolean isSimpleIdentifier(CelExpr expr) { + return expr.getKind() == CelExpr.ExprKind.Kind.IDENT + && !expr.ident().name().isEmpty() + && !expr.ident().name().startsWith("."); + } + private static CelExpr reportArgumentError(CelMacroExprFactory exprFactory, CelExpr argument) { return exprFactory.reportError( CelIssue.formatError( exprFactory.getSourceLocation(argument), "The argument must be a simple name")); } + + private static CelExpr reportAccumulatorOverwriteError( + CelMacroExprFactory exprFactory, CelExpr argument) { + return exprFactory.reportError( + CelIssue.formatError( + exprFactory.getSourceLocation(argument), + String.format( + "The iteration variable %s overwrites accumulator variable", + argument.ident().name()))); + } } diff --git a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java index b7474041d..019cea520 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java @@ -245,6 +245,16 @@ public void parser_errors() { runTest(PARSER, "1 + $"); runTest(PARSER, "1.all(2, 3)"); runTest(PARSER, "1.exists(2, 3)"); + runTest(PARSER, "[].all(__result__, x)"); + runTest(PARSER, "[].exists(__result__, x)"); + runTest(PARSER, "[].exists_one(__result__, x)"); + runTest(PARSER, "[].map(__result__, x, x)"); + runTest(PARSER, "[].filter(__result__, x)"); + runTest(PARSER, "[].all(.x, x)"); + runTest(PARSER, "[].exists(.x, x)"); + runTest(PARSER, "[].exists_one(.x, x)"); + runTest(PARSER, "[].map(.x, x, x)"); + runTest(PARSER, "[].filter(.x, x)"); runTest(PARSER, "1 + +"); runTest(PARSER, "\"\\xFh\""); runTest(PARSER, "\"\\a\\b\\f\\n\\r\\t\\v\\'\\\"\\\\\\? Illegal escape \\>\""); diff --git a/parser/src/test/resources/parser_errors.baseline b/parser/src/test/resources/parser_errors.baseline index 998bbd487..bb4ab3ed3 100644 --- a/parser/src/test/resources/parser_errors.baseline +++ b/parser/src/test/resources/parser_errors.baseline @@ -52,6 +52,66 @@ E: ERROR: :1:10: The argument must be a simple name | 1.exists(2, 3) | .........^ +I: [].all(__result__, x) +=====> +E: ERROR: :1:8: The iteration variable __result__ overwrites accumulator variable + | [].all(__result__, x) + | .......^ + +I: [].exists(__result__, x) +=====> +E: ERROR: :1:11: The iteration variable __result__ overwrites accumulator variable + | [].exists(__result__, x) + | ..........^ + +I: [].exists_one(__result__, x) +=====> +E: ERROR: :1:15: The iteration variable __result__ overwrites accumulator variable + | [].exists_one(__result__, x) + | ..............^ + +I: [].map(__result__, x, x) +=====> +E: ERROR: :1:8: The iteration variable __result__ overwrites accumulator variable + | [].map(__result__, x, x) + | .......^ + +I: [].filter(__result__, x) +=====> +E: ERROR: :1:11: The iteration variable __result__ overwrites accumulator variable + | [].filter(__result__, x) + | ..........^ + +I: [].all(.x, x) +=====> +E: ERROR: :1:9: The argument must be a simple name + | [].all(.x, x) + | ........^ + +I: [].exists(.x, x) +=====> +E: ERROR: :1:12: The argument must be a simple name + | [].exists(.x, x) + | ...........^ + +I: [].exists_one(.x, x) +=====> +E: ERROR: :1:16: The argument must be a simple name + | [].exists_one(.x, x) + | ...............^ + +I: [].map(.x, x, x) +=====> +E: ERROR: :1:9: The argument must be a simple name + | [].map(.x, x, x) + | ........^ + +I: [].filter(.x, x) +=====> +E: ERROR: :1:12: The argument must be a simple name + | [].filter(.x, x) + | ...........^ + I: 1 + + =====> E: ERROR: :1:5: mismatched input '+' expecting {'[', '{', '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, NUM_UINT, STRING, BYTES, IDENTIFIER} @@ -219,7 +279,7 @@ I: [1, 2, 3].map(var, var * var) E: ERROR: :1:15: reserved identifier: var | [1, 2, 3].map(var, var * var) | ..............^ -ERROR: :1:15: argument is not an identifier +ERROR: :1:15: The argument must be a simple name | [1, 2, 3].map(var, var * var) | ..............^ ERROR: :1:20: reserved identifier: var @@ -362,4 +422,4 @@ ERROR: :1:6: unsupported syntax '`' | .....^ ERROR: :1:9: missing ')' at '' | has(.`.` - | ........^ + | ........^ \ No newline at end of file