feat: Raise error when problematic use of the if keyword is encountered

Using the if keyword without also using the contains keyword makes the
rule name in the OPA AST an emptry string, which causes conftest to
inadvertently skip over tests leading to inaccurate results.

open-policy-agent/opa#6509

Signed-off-by: James Alseth <[email protected]>

.gitignore

@@ -6,5 +6,9 @@ site
 # IDEs
 .vscode
 .DS*
-
 .idea
+
+# NodeJS files/dirs created when installing bats
+node_modules/
+package.json
+package-lock.json

internal/testing/memfs/memfs.go

@@ -0,0 +1,75 @@
+// Package memfs implements the fs.FS interface.
+package memfs
+
+import (
+	"fmt"
+	"io"
+	"io/fs"
+	"time"
+)
+
+type FS struct {
+	files map[string][]byte
+}
+
+func New(files map[string][]byte) *FS {
+	return &FS{files: files}
+}
+
+func (fs *FS) Open(name string) (fs.File, error) {
+	data, ok := fs.files[name]
+	if !ok {
+		return nil, fmt.Errorf("file not found: %s", name)
+	}
+	return &File{name: name, data: data}, nil
+}
+
+type File struct {
+	name string
+	data []byte
+}
+
+func (f *File) Stat() (fs.FileInfo, error) {
+	return &FileInfo{
+		name: f.name,
+		size: int64(len(f.data)),
+	}, nil
+}
+
+func (f *File) Read(out []byte) (int, error) {
+	n := copy(out, f.data)
+	return n, io.EOF
+}
+
+func (f *File) Close() error {
+	return nil
+}
+
+type FileInfo struct {
+	name string
+	size int64
+}
+
+func (fi *FileInfo) Name() string {
+	return fi.name
+}
+
+func (fi *FileInfo) Size() int64 {
+	return fi.size
+}
+
+func (fi *FileInfo) Mode() fs.FileMode {
+	return fs.FileMode(0644)
+}
+
+func (fi *FileInfo) ModTime() time.Time {
+	return time.Now()
+}
+
+func (fi *FileInfo) IsDir() bool {
+	return false
+}
+
+func (fi *FileInfo) Sys() any {
+	return nil
+}

policy/engine.go

@@ -39,6 +39,11 @@ type compilerOptions struct {
 	capabilities *ast.Capabilities
 }
 
+var (
+	warningRegex = regexp.MustCompile("^warn(_[a-zA-Z0-9]+)*$")
+	failureRegex = regexp.MustCompile("^(deny|violation)(_[a-zA-Z0-9]+)*$")
+)
+
 func newCompilerOptions(strict bool, capabilities string) (compilerOptions, error) {
 	c := ast.CapabilitiesForThisVersion()
 	if capabilities != "" {
@@ -81,6 +86,10 @@ func Load(policyPaths []string, c compilerOptions) (*Engine, error) {
 		return nil, fmt.Errorf("get compiler: %w", compiler.Errors)
 	}
 
+	if err := problematicIf(modules); err != nil {
+		return nil, fmt.Errorf("rule is using 'if' keyword without 'contains' keyword: %w", err)
+	}
+
 	policyContents := make(map[string]string, len(modules))
 	for path, module := range policies.Modules {
 		path = filepath.Clean(path)
@@ -90,7 +99,7 @@ func Load(policyPaths []string, c compilerOptions) (*Engine, error) {
 	}
 
 	engine := Engine{
-		modules:  policies.ParsedModules(),
+		modules:  modules,
 		compiler: compiler,
 		policies: policyContents,
 	}
@@ -107,7 +116,6 @@ func LoadWithData(policyPaths []string, dataPaths []string, capabilities string,
 
 	engine := &Engine{}
 	if len(policyPaths) > 0 {
-		var err error
 		engine, err = Load(policyPaths, compilerOptions)
 		if err != nil {
 			return nil, fmt.Errorf("loading policies: %w", err)
@@ -519,15 +527,32 @@ func (e *Engine) query(ctx context.Context, input interface{}, query string) (ou
 }
 
 func isWarning(rule string) bool {
-	warningRegex := regexp.MustCompile("^warn(_[a-zA-Z0-9]+)*$")
 	return warningRegex.MatchString(rule)
 }
 
 func isFailure(rule string) bool {
-	failureRegex := regexp.MustCompile("^(deny|violation)(_[a-zA-Z0-9]+)*$")
 	return failureRegex.MatchString(rule)
 }
 
+func problematicIf(modules map[string]*ast.Module) error {
+	// https://github.com/open-policy-agent/opa/issues/6509
+	for _, module := range modules {
+		for _, rule := range module.Rules {
+			if rule.Head == nil || rule.Head.Value == nil || len(rule.Head.Reference) == 0 {
+				continue
+			}
+			refName := rule.Head.Reference[0].Value.String()
+			if isFailure(refName) || isWarning(refName) {
+				// Value being "true" here indicates usage of "if" without "contains".
+				if rule.Head.Value.String() == "true" {
+					return fmt.Errorf("rule in %s at line %d", module.Package.Loc().File, rule.Head.Location.Row)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 func contains(collection []string, item string) bool {
 	for _, value := range collection {
 		if strings.EqualFold(value, item) {

policy/engine_test.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"testing"
 
+	"github.com/open-policy-agent/conftest/internal/testing/memfs"
 	"github.com/open-policy-agent/conftest/parser"
 	"github.com/open-policy-agent/opa/ast"
+	"github.com/open-policy-agent/opa/loader"
 )
 
 func TestException(t *testing.T) {
@@ -313,3 +315,52 @@ deny[{"msg": msg}] {
 		})
 	}
 }
+
+func TestProblematicIf(t *testing.T) {
+	testCases := []struct {
+		desc    string
+		body    string
+		wantErr bool
+	}{
+		{
+			desc: "No rules",
+			body: "",
+		},
+		{
+			desc: "Rule not using if statement",
+			body: "deny[msg] {\n 1 == 1\nmsg := \"foo\"\n}\n",
+		},
+		{
+			desc: "Unrelated rule using problematic if",
+			body: "import future.keywords.if\nunrelated[msg] if {\n 1 == 1\nmsg := \"foo\"\n}\n",
+		},
+		{
+			desc:    "Rule using if without contains",
+			body:    "import future.keywords.if\ndeny[msg] if {\n 1 == 1\nmsg := \"foo\"\n}\n",
+			wantErr: true,
+		},
+		{
+			desc: "Rule using if with contains",
+			body: "import future.keywords.if\nimport future.keywords.contains\ndeny contains msg if {\n 1 == 1\nmsg := \"foo\"\n}\n",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			files := map[string][]byte{
+				"policy.rego": []byte("package main\n\n" + tc.body),
+			}
+			fs := memfs.New(files)
+			l := loader.NewFileLoader().WithFS(fs)
+
+			pols, err := l.All([]string{"policy.rego"})
+			if err != nil {
+				t.Fatalf("Load policies: %v", err)
+			}
+			err = problematicIf(pols.ParsedModules())
+			if gotErr := err != nil; gotErr != tc.wantErr {
+				t.Errorf("problematicIf = %v, want %v", gotErr, tc.wantErr)
+			}
+		})
+	}
+}

tests/problematic-if/policy/invalid.rego

@@ -0,0 +1,7 @@
+package main
+
+import future.keywords.if
+
+deny[msg] if {
+    msg := "foo"
+}

tests/problematic-if/policy/valid.rego

@@ -0,0 +1,8 @@
+package main
+
+import future.keywords.contains
+import future.keywords.if
+
+deny contains msg if {
+    msg := "foo"
+}

tests/problematic-if/test.bats

@@ -0,0 +1,17 @@
+#!/usr/bin/env bats
+
+@test "Test works as expected using contains and if" {
+  run $CONFTEST test --policy=policy/valid.rego data.yaml
+
+  [ "$status" -eq 1 ]
+  echo $output
+  [[ "$output" =~ "1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions" ]]
+}
+
+@test "Error is raised when if is used without contains" {
+  run $CONFTEST test --policy=policy/invalid.rego data.yaml
+
+  [ "$status" -eq 1 ]
+  echo $output
+  [[ "$output" =~ "'if' keyword without 'contains' keyword" ]]
+}