topdown: error on built-in calls with unevaluated operands (#8954)

Fixes: #3680 Built-ins require ground operands. When a Var, Ref, or comprehension reaches one anyway, it returns a generic eval_type_error, which gets collected into builtinErrors and turned into undefined unless strict built-in errors are enabled -- so bugs in OPA surface as "this rule didn't match". #3681 was this shape: a captured function output went untracked in the save set during partial evaluation, and a variable reached count(). Check the plugged operands first and return an internal error naming the offending term. The check runs before the builtin-call timer starts, so the error path needs no stopTimer. The captured output operand is exempt: walk() is legitimately called with a non-ground composite there. The check is shallow -- a type switch plus IsGround, a field read on composites -- because deep-walking every operand would make constant-time built-ins linear; benchmarks are unchanged. A nested but ground term such as [data.foo] is therefore not detected. --------- Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
2026-08-12 19:32:48 -06:00 · 2026-08-11 07:36:11 -05:00
parent 922cf80223
commit 4d7b5d0577
3 changed files with 188 additions and 2 deletions
@@ -6,6 +6,7 @@ package topdown

 import (
 	"errors"
+	"strconv"

 	"github.com/open-policy-agent/opa/v1/ast"
 	"github.com/open-policy-agent/opa/v1/util"
@@ -154,6 +155,18 @@ func mergeConflictErr(loc *ast.Location) error {
 	}
 }

+// unevaluatedOperandErr is returned when a built-in function would have been
+// called with an operand that requires evaluation, which indicates a bug in OPA
+// rather than in the policy being evaluated.
+func unevaluatedOperandErr(loc *ast.Location, name string, pos int, operand *ast.Term) error {
+	return &Error{
+		Code:     InternalErr,
+		Location: loc,
+		Message: "built-in function " + name + " called with operand " + strconv.Itoa(pos) +
+			" that requires evaluation: " + operand.String(),
+	}
+}
+
 func internalErr(loc *ast.Location, msg string) error {
 	return &Error{
 		Code:     InternalErr,
@@ -2071,6 +2071,21 @@ func (e *evalBuiltin) canUseNDBCache(bi *ast.Builtin) bool {
 	return bi.Nondeterministic && e.bctx != nil && e.bctx.NDBuiltinCache != nil
 }

+// operandRequiresEval returns true if a plugged built-in operand still contains
+// terms that must be evaluated. ast.IsConstant answers this exactly, but walks
+// composites, making the check linear in operand size on every built-in call.
+// This stays O(1) -- IsGround is a cached field on composites -- at the cost of
+// missing nested terms that require evaluation but are ground (e.g. [data.foo]).
+func operandRequiresEval(v ast.Value) bool {
+	switch v.(type) {
+	case ast.Var, ast.Ref, ast.Call,
+		*ast.ArrayComprehension, *ast.ObjectComprehension, *ast.SetComprehension:
+		return true
+	}
+
+	return !v.IsGround()
+}
+
 func (e *evalBuiltin) eval(iter unifyIterator) error {

 	operands := make([]*ast.Term, len(e.terms))
@@ -2081,8 +2096,6 @@ func (e *evalBuiltin) eval(iter unifyIterator) error {

 	numDeclArgs := e.bi.Decl.Arity()

-	e.e.instr.startTimer(evalOpBuiltinCall)
-
 	// NOTE(philipc): We sometimes have to drop the very last term off
 	// the args list for cases where a builtin's result is used/assigned,
 	// because the last term will be a generated term, not an actual
@@ -2092,6 +2105,24 @@ func (e *evalBuiltin) eval(iter unifyIterator) error {
 		endIndex--
 	}

+	// Every operand must be ground, except a captured output -- walk() is called
+	// with a non-ground composite there. Void built-ins have none, and Arity()
+	// undercounts the variadic ones (always void), so endIndex can't be used.
+	checkEnd := endIndex
+	if e.bi.Decl.Result() == nil {
+		checkEnd = len(operands)
+	}
+
+	for i, operand := range operands[:checkEnd] {
+		if operandRequiresEval(operand.Value) {
+			// If hit, this is a bug: the compiler hoists arguments that require evaluation.
+			// Fail loudly, as the built-in would return undefined instead leading to unexpected results.
+			return unevaluatedOperandErr(e.e.query[e.e.index].Location, e.bi.Name, i+1, operand)
+		}
+	}
+
+	e.e.instr.startTimer(evalOpBuiltinCall)
+
 	// We skip evaluation of the builtin entirely if the NDBCache is
 	// present, and we have a non-deterministic builtin already cached.
 	if e.canUseNDBCache(e.bi) {
@@ -17,6 +17,7 @@ import (
 	"github.com/open-policy-agent/opa/v1/metrics"
 	"github.com/open-policy-agent/opa/v1/storage"
 	inmem "github.com/open-policy-agent/opa/v1/storage/inmem/test"
+	"github.com/open-policy-agent/opa/v1/types"
 )

 func TestQueryIDFactory(t *testing.T) {
@@ -1668,6 +1669,147 @@ func TestContextErrorHandling(t *testing.T) {
 	}
 }

+func TestEvalBuiltinUnevaluatedOperand(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		note   string
+		body   string
+		expErr string
+	}{
+		{
+			note:   "unbound var",
+			body:   "count(x, y)",
+			expErr: "built-in function count called with operand 1 that requires evaluation: x",
+		},
+		{
+			note:   "ref",
+			body:   "count(data.foo, y)",
+			expErr: "built-in function count called with operand 1 that requires evaluation: data.foo",
+		},
+		{
+			note:   "var nested in composite",
+			body:   "count([x], y)",
+			expErr: "built-in function count called with operand 1 that requires evaluation: [x]",
+		},
+		{
+			note:   "comprehension",
+			body:   "count([i | i = 1], y)",
+			expErr: "built-in function count called with operand 1 that requires evaluation: [i | i = 1]",
+		},
+		{
+			note:   "comprehension nested in composite",
+			body:   "count([[i | i = 1]], y)",
+			expErr: "built-in function count called with operand 1 that requires evaluation: [[i | i = 1]]",
+		},
+		{
+			note:   "unbound var, no captured output",
+			body:   `startswith("foo", x)`,
+			expErr: "built-in function startswith called with operand 2 that requires evaluation: x",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.note, func(t *testing.T) {
+			t.Parallel()
+
+			qrs, err := NewQuery(ast.MustParseBody(tc.body)).Run(t.Context())
+			if err == nil {
+				t.Fatalf("expected error but got results: %v", qrs)
+			}
+
+			var topdownErr *Error
+			if !errors.As(err, &topdownErr) {
+				t.Fatalf("expected *topdown.Error but got %#v", err)
+			}
+
+			if topdownErr.Code != InternalErr {
+				t.Errorf("expected code %v but got %v", InternalErr, topdownErr.Code)
+			}
+
+			if !strings.Contains(topdownErr.Message, tc.expErr) {
+				t.Errorf("expected message %q but got %q", tc.expErr, topdownErr.Message)
+			}
+		})
+	}
+}
+
+func TestEvalBuiltinNonGroundOutputOperand(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		note string
+		body string
+		exp  map[string]string
+	}{
+		{
+			note: "composite output operand",
+			body: `walk({"a": 1}, [["a"], v])`,
+			exp:  map[string]string{"v": "1"},
+		},
+		{
+			note: "wildcard in composite output operand",
+			body: `walk({"a": 1}, [_, v])`,
+			exp:  map[string]string{"v": `{"a": 1}`},
+		},
+		{
+			note: "captured output var",
+			body: "count([1, 2], y)",
+			exp:  map[string]string{"y": "2"},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.note, func(t *testing.T) {
+			t.Parallel()
+
+			qrs, err := NewQuery(ast.MustParseBody(tc.body)).Run(t.Context())
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if len(qrs) == 0 {
+				t.Fatal("expected results but got none")
+			}
+
+			for k, v := range tc.exp {
+				if exp, act := ast.MustParseTerm(v), qrs[0][ast.Var(k)]; !exp.Equal(act) {
+					t.Errorf("expected %v to be %v but got %v", k, exp, act)
+				}
+			}
+		})
+	}
+}
+
+func TestEvalBuiltinUnevaluatedVariadicOperand(t *testing.T) {
+	t.Parallel()
+
+	query := NewQuery(ast.MustParseBody(`test("a", x)`)).WithBuiltins(map[string]*Builtin{
+		"test": {
+			Decl: &ast.Builtin{
+				Name: "test",
+				Decl: types.NewVariadicFunction(types.Args(types.S), types.A, nil),
+			},
+			Func: func(_ BuiltinContext, terms []*ast.Term, _ func(*ast.Term) error) error {
+				t.Fatalf("built-in must not be called, got operands %v", terms)
+				return nil
+			},
+		},
+	})
+
+	_, err := query.Run(t.Context())
+
+	var topdownErr *Error
+	if !errors.As(err, &topdownErr) {
+		t.Fatalf("expected *topdown.Error but got %#v", err)
+	}
+
+	exp := "built-in function test called with operand 2 that requires evaluation: x"
+	if topdownErr.Code != InternalErr || !strings.Contains(topdownErr.Message, exp) {
+		t.Errorf("expected %v %q but got %v %q", InternalErr, exp, topdownErr.Code, topdownErr.Message)
+	}
+}
+
 func TestFmtVarTerm(t *testing.T) {
 	e := &eval{
 		genvarprefix: "foobar",