Skip to content

Commit

Permalink
Some fixes for detecting external refs
Browse files Browse the repository at this point in the history
The following cases are now covered:

```rego
f(_) := r

f(_) := {"r": r}
```
And variations thereof.

Fixes #719

Although it doesn't fix the `func_2` example there, this is about as
much we can do without some major effort.. so I'm not sure it's worth
keeping that issue open as I doubt we'll get there anytime soon.

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert committed May 20, 2024
1 parent 08e4334 commit 400d2c3
Show file tree
Hide file tree
Showing 14 changed files with 35 additions and 7 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ The following rules are currently available:
| style | [default-over-not](https://docs.styra.com/regal/rules/style/default-over-not) | Prefer default assignment over negated condition |
| style | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata) | Detached metadata annotation |
| style | [double-negative](https://docs.styra.com/regal/rules/style/double-negative) | Avoid double negatives |
| style | [external-reference](https://docs.styra.com/regal/rules/style/external-reference) | Reference to input, data or rule ref in function body |
| style | [external-reference](https://docs.styra.com/regal/rules/style/external-reference) | External reference in function |
| style | [file-length](https://docs.styra.com/regal/rules/style/file-length) | Max file length exceeded |
| style | [function-arg-return](https://docs.styra.com/regal/rules/style/function-arg-return) | Function argument used for return value |
| style | [line-length](https://docs.styra.com/regal/rules/style/line-length) | Line too long |
Expand Down
1 change: 1 addition & 0 deletions bundle/regal/ast/ast.rego
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ builtin_functions_called contains name if {
# description: |
# Returns custom functions declared in input policy in the same format as builtin capabilities
function_decls(rules) := {rule_name: decl |
# regal ignore:external-reference
some rule in functions

rule_name := name(rule)
Expand Down
4 changes: 2 additions & 2 deletions bundle/regal/result.rego
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ resource_urls(related_resources, category) := [r |
with_text(location) := {"location": object.union(
location,
{
"file": input.regal.file.name,
"text": input.regal.file.lines[location.row - 1],
"file": input.regal.file.name, # regal ignore:external-reference
"text": input.regal.file.lines[location.row - 1], # regal ignore:external-reference
},
)} if {
location.row
Expand Down
1 change: 1 addition & 0 deletions bundle/regal/rules/bugs/impossible_not_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,5 @@ expected := {
"title": "impossible-not",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
2 changes: 2 additions & 0 deletions bundle/regal/rules/bugs/inconsistent_args_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,10 @@ expected := {
"title": "inconsistent-args",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": loc}) |
some loc in location
} if {
Expand Down
1 change: 1 addition & 0 deletions bundle/regal/rules/custom/one_liner_rule_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -160,4 +160,5 @@ expected := {
"title": "one-liner-rule",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})}
1 change: 1 addition & 0 deletions bundle/regal/rules/custom/prefer_value_in_head_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,5 @@ expected := {
"title": "prefer-value-in-head",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})}
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,10 @@ expected := {
"title": "equals-pattern-matching",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": loc}) |
some loc in location
} if {
Expand Down
2 changes: 2 additions & 0 deletions bundle/regal/rules/idiomatic/use_in_operator_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,10 @@ expected := {
"title": "use-in-operator",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": loc}) |
some loc in location
} if {
Expand Down
7 changes: 4 additions & 3 deletions bundle/regal/rules/style/external_reference.rego
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# METADATA
# description: Reference to input, data or rule ref in function body
# description: External reference in function
package regal.rules.style["external-reference"]

import rego.v1
Expand All @@ -13,10 +13,11 @@ report contains violation if {
some fn in ast.functions

named_args := {arg.value | some arg in fn.head.args; arg.type == "var"}
head_vars := {v.value | some v in ast.find_term_vars(fn.head.value)}

head_vars := {v.value | some v in ast.find_vars(fn.head.value)}
body_vars := {v.value | some v in ast.find_vars(fn.body)}
else_vars := {v.value | some v in ast.find_vars(fn["else"])}
own_vars := (head_vars | body_vars) | else_vars
own_vars := (body_vars | head_vars) | else_vars

# note: parens added by opa fmt 🤦
allowed_refs := (named_args | own_vars) | fn_namespaces
Expand Down
15 changes: 14 additions & 1 deletion bundle/regal/rules/style/external_reference_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ f(x, y) {
r == expected_with_location({"col": 7, "file": "policy.rego", "row": 8, "text": ` y == foo`})
}

test_fail_external_reference_in_head_assignment if {
r := rule.report with input as ast.policy(`f(_) := r`)
with data.internal.combined_config as {"capabilities": capabilities.provided}
r == expected_with_location({"col": 9, "file": "policy.rego", "row": 3, "text": "f(_) := r"})
}

test_fail_external_reference_in_head_terms if {
r := rule.report with input as ast.policy(`f(_) := {"r": r}`)
with data.internal.combined_config as {"capabilities": capabilities.provided}
r == expected_with_location({"col": 15, "file": "policy.rego", "row": 3, "text": "f(_) := {\"r\": r}"})
}

test_success_function_references_no_input_or_data if {
r := rule.report with input as ast.policy(`f(x) { x == true }`)
with data.internal.combined_config as {"capabilities": capabilities.provided}
Expand Down Expand Up @@ -96,7 +108,7 @@ test_success_function_references_external_function_in_expr if {

expected := {
"category": "style",
"description": "Reference to input, data or rule ref in function body",
"description": "External reference in function",
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/external-reference", "style"),
Expand All @@ -105,4 +117,5 @@ expected := {
"level": "error",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
1 change: 1 addition & 0 deletions bundle/regal/rules/style/messy_rule_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,5 @@ expected := {
"title": "messy-rule",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
2 changes: 2 additions & 0 deletions bundle/regal/rules/style/yoda_condition_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ expected := {
"title": "yoda-condition",
}

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)

# regal ignore:external-reference
expected_with_location(location) := {object.union(expected, {"location": loc}) |
some loc in location
} if {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,5 @@ expected := {
"title": "metasyntactic-variable",
}

# regal ignore:external-reference
expected_with_location(location) := object.union(expected, {"location": location})

0 comments on commit 400d2c3

Please sign in to comment.