diff --git a/bundle/regal/rules/bugs/bugs.rego b/bundle/regal/rules/bugs/bugs.rego index 479a729f..0927438c 100644 --- a/bundle/regal/rules/bugs/bugs.rego +++ b/bundle/regal/rules/bugs/bugs.rego @@ -120,6 +120,34 @@ report contains violation if { violation := result.fail(rego.metadata.rule(), result.location(expr.terms[0])) } +# METADATA +# title: not-equals-in-loop +# description: Use of != in loop +# related_resources: +# - description: documentation +# ref: $baseUrl/$category/not-equals-in-loop +# custom: +# category: bugs +report contains violation if { + config.for_rule(rego.metadata.rule()).level != "ignore" + + some rule in input.rules + some expr in rule.body + + expr.terms[0].type == "ref" + expr.terms[0].value[0].type == "var" + expr.terms[0].value[0].value == "neq" + + some neq_term in array.slice(expr.terms, 1, count(expr.terms)) + neq_term.type == "ref" + + some i + neq_term.value[i].type == "var" + startswith(neq_term.value[i].value, "$") + + violation := result.fail(rego.metadata.rule(), result.location(expr.terms[0])) +} + # regal ignore:external-reference illegal_value_ref(value) if not value in _rule_names diff --git a/bundle/regal/rules/bugs/bugs_test.rego b/bundle/regal/rules/bugs/bugs_test.rego index 8cb1f0c7..d617cfe5 100644 --- a/bundle/regal/rules/bugs/bugs_test.rego +++ b/bundle/regal/rules/bugs/bugs_test.rego @@ -113,6 +113,52 @@ test_success_return_value_assigned if { report(`allow { x := indexof("s", "s") }`) == set() } +test_fail_neq_in_loop if { + r := report(`deny { + "admin" != input.user.groups[_] + input.user.groups[_] != "admin" + }`) + r == { + { + "category": "bugs", + "description": "Use of != in loop", + "level": "error", + "location": {"col": 11, "file": "policy.rego", "row": 9, "text": "\t\t\"admin\" != input.user.groups[_]"}, + "related_resources": [{ + "description": "documentation", + "ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md", + }], + "title": "not-equals-in-loop", + }, + { + "category": "bugs", + "description": "Use of != in loop", + "level": "error", + "location": {"col": 24, "file": "policy.rego", "row": 10, "text": "\t\tinput.user.groups[_] != \"admin\""}, + "related_resources": [{ + "description": "documentation", + "ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md", + }], + "title": "not-equals-in-loop", + }, + } +} + +test_fail_neq_in_loop_one_liner if { + r := report(`deny if "admin" != input.user.groups[_]`) + r == {{ + "category": "bugs", + "description": "Use of != in loop", + "level": "error", + "location": {"col": 17, "file": "policy.rego", "row": 8, "text": "deny if \"admin\" != input.user.groups[_]"}, + "related_resources": [{ + "description": "documentation", + "ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md", + }], + "title": "not-equals-in-loop", + }} +} + report(snippet) := report if { # regal ignore:external-reference report := bugs.report with input as ast.with_future_keywords(snippet) diff --git a/docs/rules/bugs/not-equals-in-loop.md b/docs/rules/bugs/not-equals-in-loop.md new file mode 100644 index 00000000..e46b8b66 --- /dev/null +++ b/docs/rules/bugs/not-equals-in-loop.md @@ -0,0 +1,69 @@ +# not-equals-in-loop + +**Summary**: Use of != in loop + +**Category**: Bugs + +**Avoid** +```rego +package policy + +import future.keywords.if + +deny if { + "admin" != input.user.roles[_] +} +``` + +**Prefer** +```rego +package policy + +import future.keywords.if +import future.keywords.in + +deny if { + not "admin" in input.user.roles +} + +# Or as a one-liner +deny if not "admin" in input.user.roles +``` + +## Rationale + +Likely one of the most common mistakes in Rego is to use `!=` in a loop thinking it means "not in". It took some years +for the `in` keyword to be added to Rego, so perhaps it's not surprising that this mistake is a common one even to this +day. If it doesn't mean "not in", what does it mean? + +```rego +package policy + +import future.keywords.if + +deny if { + "admin" != input.user.roles[_] +} +``` + +The body of the `deny` rule above roughly translates to "for any item in `input.user.roles`, return true if the item is +not `admin`". This is almost never what the policy author intended. What the policy author likely intended was +"deny if `admin` is not in `input.user.roles`". The above policy would thus **not** deny a user with the roles +`["user", "admin"]` since the first item in the array is not "admin". This is almost never what the policy author +intended. + +**Note**: This linter rule currently only checks for `!=` in a non-nested comparison where iteration happens on either +side of the comparison in the same expression. This will be improved in time. Another limitation is that this rule +currently only checks for wildcard iteration (`[_]`). + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + bugs: + not-equals-in-loop: + # one of "error", "warning", "ignore" + level: error +```