diff --git a/bundle/regal/result/result.rego b/bundle/regal/result/result.rego index a57f4c49..86a153df 100644 --- a/bundle/regal/result/result.rego +++ b/bundle/regal/result/result.rego @@ -196,7 +196,7 @@ _with_text(loc_obj) := loc if { # METADATA # description: | # returns a "normalized" location object from the location value found in the AST. -# new code should most often use one of the ranged_ location functions instea, as +# new code should most often use one of the ranged_ location functions instead, as # that will also include an `"end"` location attribute # scope: document location(x) := _with_text(util.to_location_object(x.location)) diff --git a/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check.rego b/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check.rego index 743fb5d9..eeb79dde 100644 --- a/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check.rego +++ b/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check.rego @@ -29,6 +29,27 @@ report contains violation if { violation := result.fail(rego.metadata.chain(), result.ranged_from_ref(expr.terms.value)) } +# METADATA +# description: | +# check for redundant existence checks of function args in function bodies +# note: this only scans "top level" expressions in the function body, and not +# e.g. those nested inside of comprehensions, every bodies, etc.. while this +# would certainly be possible, the cost does not justify the benefit, as it's +# quite unlikely that existence checks are found there +report contains violation if { + some func in ast.functions + some expr in func.body + + expr.terms.type == "var" + + some arg in func.head.args + + arg.type == "var" + arg.value == expr.terms.value + + violation := result.fail(rego.metadata.chain(), result.location(expr.terms)) +} + # METADATA # description: check for redundant existence checks in rule head assignment report contains violation if { diff --git a/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check_test.rego b/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check_test.rego index ec075727..a13fe08c 100644 --- a/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check_test.rego +++ b/bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check_test.rego @@ -66,3 +66,22 @@ test_fail_redundant_existence_check_head_assignment_of_ref if { "title": "redundant-existence-check", }} } + +test_fail_redundant_existence_check_function_arg if { + module := ast.with_rego_v1(` + fun(foo) if { + foo + }`) + r := rule.report with input as module + r == {{ + "category": "bugs", + "description": "Redundant existence check", + "level": "error", + "location": {"col": 3, "end": {"col": 6, "row": 7}, "file": "policy.rego", "row": 7, "text": "\t\tfoo"}, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/redundant-existence-check", "bugs"), + }], + "title": "redundant-existence-check", + }} +} diff --git a/docs/rules/bugs/redundant-existence-check.md b/docs/rules/bugs/redundant-existence-check.md index d317da86..3d51a55b 100644 --- a/docs/rules/bugs/redundant-existence-check.md +++ b/docs/rules/bugs/redundant-existence-check.md @@ -14,6 +14,11 @@ employee if { input.user.email endswith(input.user.email, "@acmecorp.com") } + +is_admin(user) if { + user + "admin" in user.roles +} ``` **Prefer** @@ -29,6 +34,10 @@ employee if { # alternatively employee if endswith(input.user.email, "@acmecorp.com") + +is_admin(user) if { + "admin" in user.roles +} ``` ## Rationale @@ -42,6 +51,29 @@ variable. used later in the rule, it won't be flagged. While the existence check _could_ be redundant even in that case, it could also be used to avoid making some expensive computation, an `http.send` call, or whatnot. +## Exceptions + +Function arguments where a boolean value is expected will be flagged as redundant existence checks, even though the +intent was to check the boolean condition. + +```rego +report(user, is_admin) if { + is_admin + + # more conditions +} +``` + +For these cases, prefer to be explicit about what the assertion is checking: + +```rego +report(user, is_admin) if { + is_admin == false # or true, != false, etc. + + # more conditions +} +``` + ## Configuration Options This linter rule provides the following configuration options: diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index 133ebd0c..29c1386d 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -699,23 +699,19 @@ func (l Linter) paramsToRulesConfig() map[string]any { } func (l Linter) prepareRegoArgs(query ast.Body) ([]func(*rego.Rego), error) { - var regoArgs []func(*rego.Rego) - - roots := []string{"eval"} - dataBundle := bundle.Bundle{ Data: l.paramsToRulesConfig(), - Manifest: bundle.Manifest{Roots: &roots}, + Manifest: bundle.Manifest{Roots: &[]string{"eval"}}, } - regoArgs = append(regoArgs, + regoArgs := []func(*rego.Rego){ rego.StoreReadAST(true), rego.Metrics(l.metrics), rego.ParsedQuery(query), rego.ParsedBundle("regal_eval_params", &dataBundle), rego.Function2(builtins.RegalParseModuleMeta, builtins.RegalParseModule), rego.Function1(builtins.RegalLastMeta, builtins.RegalLast), - ) + } if l.debugMode && l.printHook == nil { l.printHook = topdown.NewPrintHook(os.Stderr)