diff --git a/.gitignore b/.gitignore index 2c534d8803..a8da11a32a 100644 --- a/.gitignore +++ b/.gitignore @@ -6,5 +6,9 @@ site # IDEs .vscode .DS* - .idea + +# NodeJS files/dirs created when installing bats +node_modules/ +package.json +package-lock.json diff --git a/internal/testing/memfs/memfs.go b/internal/testing/memfs/memfs.go new file mode 100644 index 0000000000..dab0d2eb02 --- /dev/null +++ b/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 +} diff --git a/policy/engine.go b/policy/engine.go index 47d18ecb5a..a19c216a29 100644 --- a/policy/engine.go +++ b/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) { diff --git a/policy/engine_test.go b/policy/engine_test.go index 0c0e778f22..b98e00ef78 100644 --- a/policy/engine_test.go +++ b/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) + } + }) + } +} diff --git a/tests/problematic-if/data.yaml b/tests/problematic-if/data.yaml new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/problematic-if/policy/invalid.rego b/tests/problematic-if/policy/invalid.rego new file mode 100644 index 0000000000..2d04e6d935 --- /dev/null +++ b/tests/problematic-if/policy/invalid.rego @@ -0,0 +1,7 @@ +package main + +import future.keywords.if + +deny[msg] if { + msg := "foo" +} diff --git a/tests/problematic-if/policy/valid.rego b/tests/problematic-if/policy/valid.rego new file mode 100644 index 0000000000..c36142cff3 --- /dev/null +++ b/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" +} diff --git a/tests/problematic-if/test.bats b/tests/problematic-if/test.bats new file mode 100644 index 0000000000..3d5923a41d --- /dev/null +++ b/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" ]] +}