Variable and nested rule limits (#989)

* Variable and nested rule limits
* Update the config version to be a string and address feedback

policy/compiler.go

@@ -33,6 +33,11 @@ type CompiledRule struct {
 	matches   []*CompiledMatch
 }
 
+// SourceID returns the source metadata identifier associated with the compiled rule.
+func (r *CompiledRule) SourceID() int64 {
+	return r.ID().ID
+}
+
 // ID returns the expression id associated with the rule.
 func (r *CompiledRule) ID() *ValueString {
 	return r.id
@@ -50,11 +55,17 @@ func (r *CompiledRule) Matches() []*CompiledMatch {
 
 // CompiledVariable represents the variable name, expression, and associated type-check declaration.
 type CompiledVariable struct {
+	id      int64
 	name    string
 	expr    *cel.Ast
 	varDecl *decls.VariableDecl
 }
 
+// SourceID returns the source metadata identifier associated with the variable.
+func (v *CompiledVariable) SourceID() int64 {
+	return v.id
+}
+
 // Name returns the variable name.
 func (v *CompiledVariable) Name() string {
 	return v.name
@@ -110,12 +121,28 @@ func (o *OutputValue) Expr() *cel.Ast {
 	return o.expr
 }
 
+// CompilerOption specifies a functional option to be applied to new RuleComposer instances.
+type CompilerOption func(*compiler) error
+
+// MaxNestedExpressions limits the number of variable and nested rule expressions during compilation.
+//
+// Defaults to 100 if not set.
+func MaxNestedExpressions(limit int) CompilerOption {
+	return func(c *compiler) error {
+		if limit <= 0 {
+			return fmt.Errorf("nested expression limit must be non-negative, non-zero value: %d", limit)
+		}
+		c.maxNestedExpressions = limit
+		return nil
+	}
+}
+
 // Compile combines the policy compilation and composition steps into a single call.
 //
 // This generates a single CEL AST from a collection of policy expressions associated with a
 // CEL environment.
-func Compile(env *cel.Env, p *Policy) (*cel.Ast, *cel.Issues) {
-	rule, iss := CompileRule(env, p)
+func Compile(env *cel.Env, p *Policy, opts ...CompilerOption) (*cel.Ast, *cel.Issues) {
+	rule, iss := CompileRule(env, p, opts...)
 	if iss.Err() != nil {
 		return nil, iss
 	}
@@ -126,14 +153,21 @@ func Compile(env *cel.Env, p *Policy) (*cel.Ast, *cel.Issues) {
 // CompileRule creates a compiled rules from the policy which contains a set of compiled variables and
 // match statements. The compiled rule defines an expression graph, which can be composed into a single
 // expression via the RuleComposer.Compose method.
-func CompileRule(env *cel.Env, p *Policy) (*CompiledRule, *cel.Issues) {
+func CompileRule(env *cel.Env, p *Policy, opts ...CompilerOption) (*CompiledRule, *cel.Issues) {
 	c := &compiler{
-		env:  env,
-		info: p.SourceInfo(),
-		src:  p.Source(),
+		env:                  env,
+		info:                 p.SourceInfo(),
+		src:                  p.Source(),
+		maxNestedExpressions: defaultMaxNestedExpressions,
 	}
 	errs := common.NewErrors(c.src)
 	iss := cel.NewIssuesWithSourceInfo(errs, c.info)
+	for _, o := range opts {
+		if err := o(c); err != nil {
+			iss.ReportErrorAtID(p.Name().ID, "error configuring compiler option: %s", err)
+			return nil, iss
+		}
+	}
 	rule, ruleIss := c.compileRule(p.Rule(), c.env, iss)
 	iss = iss.Append(ruleIss)
 	return rule, iss
@@ -143,6 +177,9 @@ type compiler struct {
 	env  *cel.Env
 	info *ast.SourceInfo
 	src  *Source
+
+	maxNestedExpressions int
+	nestedCount          int
 }
 
 func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*CompiledRule, *cel.Issues) {
@@ -170,12 +207,22 @@ func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*Com
 		if err != nil {
 			iss.ReportErrorAtID(v.Expression().ID, "invalid variable declaration")
 		}
-		compiledVars[i] = &CompiledVariable{
+		compiledVar := &CompiledVariable{
+			id:      v.name.ID,
 			name:    v.name.Value,
 			expr:    varAST,
 			varDecl: varDecl,
 		}
+		compiledVars[i] = compiledVar
+
+		// Increment the nesting count post-compile.
+		c.nestedCount++
+		if c.nestedCount == c.maxNestedExpressions+1 {
+			iss.ReportErrorAtID(compiledVar.SourceID(), "variable exceeds nested expression limit")
+		}
 	}
+
+	// Compile the set of match conditions under the rule.
 	compiledMatches := []*CompiledMatch{}
 	for _, m := range r.Matches() {
 		condSrc := c.relSource(m.Condition())
@@ -208,6 +255,12 @@ func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*Com
 				cond:       condAST,
 				nestedRule: nestedRule,
 			})
+
+			// Increment the nesting count post-compile.
+			c.nestedCount++
+			if c.nestedCount == c.maxNestedExpressions+1 {
+				iss.ReportErrorAtID(nestedRule.SourceID(), "rule exceeds nested expression limit")
+			}
 		}
 	}
 	return &CompiledRule{
@@ -232,4 +285,6 @@ func (c *compiler) relSource(pstr ValueString) *RelativeSource {
 const (
 	// Consider making the variables namespace configurable.
 	variablePrefix = "variables"
+
+	defaultMaxNestedExpressions = 100
 )

policy/compiler_test.go

@@ -33,7 +33,7 @@ func TestCompile(t *testing.T) {
 
 func TestCompileError(t *testing.T) {
 	for _, tst := range policyErrorTests {
-		_, _, iss := compile(t, tst.name, []ParserOption{}, []cel.EnvOption{})
+		_, _, iss := compile(t, tst.name, []ParserOption{}, []cel.EnvOption{}, tst.compilerOpts)
 		if iss.Err() == nil {
 			t.Fatalf("compile(%s) did not error, wanted %s", tst.name, tst.err)
 		}
@@ -61,15 +61,16 @@ func newRunner(t testing.TB, name, expr string, parseOpts []ParserOption, opts .
 }
 
 type runner struct {
-	name      string
-	envOpts   []cel.EnvOption
-	parseOpts []ParserOption
-	env       *cel.Env
-	expr      string
-	prg       cel.Program
+	name         string
+	envOpts      []cel.EnvOption
+	parseOpts    []ParserOption
+	compilerOpts []CompilerOption
+	env          *cel.Env
+	expr         string
+	prg          cel.Program
 }
 
-func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.EnvOption) (*cel.Env, *cel.Ast, *cel.Issues) {
+func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.EnvOption, compilerOpts []CompilerOption) (*cel.Env, *cel.Ast, *cel.Issues) {
 	config := readPolicyConfig(t, fmt.Sprintf("testdata/%s/config.yaml", name))
 	srcFile := readPolicy(t, fmt.Sprintf("testdata/%s/policy.yaml", name))
 	parser, err := NewParser(parseOpts...)
@@ -84,6 +85,7 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.
 		t.Errorf("policy name is %v, wanted %q", policy.name, name)
 	}
 	env, err := cel.NewEnv(
+		cel.DefaultUTCTimeZone(true),
 		cel.OptionalTypes(),
 		cel.EnableMacroCallTracking(),
 		cel.ExtendedValidations())
@@ -104,12 +106,12 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.
 	if err != nil {
 		t.Fatalf("env.Extend() with config options %v, failed: %v", config, err)
 	}
-	ast, iss := Compile(env, policy)
+	ast, iss := Compile(env, policy, compilerOpts...)
 	return env, ast, iss
 }
 
 func (r *runner) setup(t testing.TB) {
-	env, ast, iss := compile(t, r.name, r.parseOpts, r.envOpts)
+	env, ast, iss := compile(t, r.name, r.parseOpts, r.envOpts, r.compilerOpts)
 	if iss.Err() != nil {
 		t.Fatalf("Compile() failed: %v", iss.Err())
 	}

policy/composer.go

@@ -24,7 +24,10 @@ import (
 // NewRuleComposer creates a rule composer which stitches together rules within a policy into
 // a single CEL expression.
 func NewRuleComposer(env *cel.Env, p *Policy) *RuleComposer {
-	return &RuleComposer{env: env, p: p}
+	return &RuleComposer{
+		env: env,
+		p:   p,
+	}
 }
 
 // RuleComposer optimizes a set of expressions into a single expression.
@@ -41,22 +44,26 @@ func (c *RuleComposer) Compose(r *CompiledRule) (*cel.Ast, *cel.Issues) {
 }
 
 type ruleComposerImpl struct {
-	rule *CompiledRule
+	rule                     *CompiledRule
+	maxNestedExpressionLimit int
 }
 
 // Optimize implements an AST optimizer for CEL which composes an expression graph into a single
 // expression value.
 func (opt *ruleComposerImpl) Optimize(ctx *cel.OptimizerContext, a *ast.AST) *ast.AST {
 	// The input to optimize is a dummy expression which is completely replaced according
 	// to the configuration of the rule composition graph.
-	ruleExpr, _ := optimizeRule(ctx, opt.rule)
+	ruleExpr, _ := opt.optimizeRule(ctx, opt.rule)
 	return ctx.NewAST(ruleExpr)
 }
 
-func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) {
+func (opt *ruleComposerImpl) optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) {
 	matchExpr := ctx.NewCall("optional.none")
 	matches := r.Matches()
+	vars := r.Variables()
 	optionalResult := true
+
+	// Build the rule subgraph.
 	for i := len(matches) - 1; i >= 0; i-- {
 		m := matches[i]
 		cond := ctx.CopyASTAndMetadata(m.Condition().NativeRep())
@@ -78,7 +85,7 @@ func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) {
 				matchExpr)
 			continue
 		}
-		nestedRule, nestedOptional := optimizeRule(ctx, m.NestedRule())
+		nestedRule, nestedOptional := opt.optimizeRule(ctx, m.NestedRule())
 		if optionalResult && !nestedOptional {
 			nestedRule = ctx.NewCall("optional.of", nestedRule)
 		}
@@ -90,10 +97,19 @@ func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) {
 			ctx.ReportErrorAtID(nestedRule.ID(), "subrule early terminates policy")
 			continue
 		}
-		matchExpr = ctx.NewMemberCall("or", nestedRule, matchExpr)
+		if triviallyTrue {
+			matchExpr = ctx.NewMemberCall("or", nestedRule, matchExpr)
+		} else {
+			matchExpr = ctx.NewCall(
+				operators.Conditional,
+				cond,
+				nestedRule,
+				matchExpr,
+			)
+		}
 	}
 
-	vars := r.Variables()
+	// Bind variables in reverse order to declaration on top of rule-subgraph.
 	for i := len(vars) - 1; i >= 0; i-- {
 		v := vars[i]
 		varAST := ctx.CopyASTAndMetadata(v.Expr().NativeRep())

policy/config.go

@@ -16,6 +16,8 @@ package policy
 
 import (
 	"fmt"
+	"math"
+	"strconv"
 
 	"github.com/google/cel-go/cel"
 	"github.com/google/cel-go/ext"
@@ -74,7 +76,7 @@ type ExtensionResolver interface {
 // ExtensionConfig represents a YAML serializable definition of a versioned extension library reference.
 type ExtensionConfig struct {
 	Name    string `yaml:"name"`
-	Version int    `yaml:"version"`
+	Version string `yaml:"version"`
 	ExtensionResolver
 }
 
@@ -87,7 +89,18 @@ func (ec *ExtensionConfig) AsEnvOption(baseEnv *cel.Env) (cel.EnvOption, error)
 	if !found {
 		return nil, fmt.Errorf("unrecognized extension: %s", ec.Name)
 	}
-	return fac(uint32(ec.Version)), nil
+	// If the version is 'latest', set the version value to the max uint.
+	if ec.Version == "latest" {
+		return fac(math.MaxUint32), nil
+	}
+	if ec.Version == "" {
+		return fac(0), nil
+	}
+	ver, err := strconv.ParseUint(ec.Version, 10, 32)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing uint version: %w", err)
+	}
+	return fac(uint32(ver)), nil
 }
 
 // VariableDecl represents a YAML serializable CEL variable declaration.

policy/conformance.go

@@ -36,6 +36,7 @@ type TestCase struct {
 	Output string               `yaml:"output"`
 }
 
+// TestInput represents an input literal value or expression.
 type TestInput struct {
 	Value any    `yaml:"value"`
 	Expr  string `yaml:"expr"`

policy/helper_test.go

@@ -112,12 +112,33 @@ var (
 								return types.String("ir")
 							}
 						}))),
-			}},
+			},
+		},
+		{
+			name: "limits",
+			expr: `
+	cel.bind(variables.greeting, "hello",
+	cel.bind(variables.farewell, "goodbye",
+	cel.bind(variables.person, "me",
+	cel.bind(variables.message_fmt, "%s, %s",
+	  (now.getHours() >= 20)
+	  ? cel.bind(variables.message, variables.message_fmt.format([variables.farewell, variables.person]),
+	    (now.getHours() < 21)
+		  ? optional.of(variables.message + "!")
+		  : ((now.getHours() < 22)
+		    ? optional.of(variables.message + "!!")
+			: ((now.getHours() < 24)
+			  ? optional.of(variables.message + "!!!")
+			  : optional.none())))
+	   : optional.of(variables.message_fmt.format([variables.greeting, variables.person]))
+	 ))))`,
+		},
 	}
 
 	policyErrorTests = []struct {
-		name string
-		err  string
+		name         string
+		err          string
+		compilerOpts []CompilerOption
 	}{
 		{
 			name: "errors",
@@ -140,6 +161,21 @@ ERROR: testdata/errors/policy.yaml:34:67: undeclared reference to 'format' (in c
  |         "invalid values provided on one or more labels: %s".format([variables.invalid])
  | ..................................................................^`,
 		},
+		{
+			name: "limits",
+			err: `ERROR: testdata/limits/policy.yaml:22:14: variable exceeds nested expression limit
+ |     - name: "person"
+ | .............^`,
+			compilerOpts: []CompilerOption{MaxNestedExpressions(2)},
+		},
+
+		{
+			name: "limits",
+			err: `ERROR: testdata/limits/policy.yaml:30:14: rule exceeds nested expression limit
+ |         id: "farewells"
+ | .............^`,
+			compilerOpts: []CompilerOption{MaxNestedExpressions(5)},
+		},
 	}
 )
 

policy/testdata/errors/config.yaml

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-name: "labels"
+name: "errors"
 extensions:
   - name: "lists"
   - name: "sets"