diff --git a/README.md b/README.md index 75fd0d9d..dc61683b 100644 --- a/README.md +++ b/README.md @@ -232,6 +232,7 @@ The following rules are currently available: | imports | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports) | Prefer importing packages over rules | | imports | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias) | Redundant alias | | imports | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import) | Redundant import of data | +| imports | [unresolved-import](https://docs.styra.com/regal/rules/imports/unresolved-import) | Unresolved import | | imports | [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1) | Use `import rego.v1` | | performance | [with-outside-test-context](https://docs.styra.com/regal/rules/performance/with-outside-test-context) | `with` used outside test context | | style | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix) | Avoid `get_` and `list_` prefix for rules and functions | diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index 3dcee985..07169ee1 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/bundle/regal/config/provided/data.yaml @@ -81,6 +81,8 @@ rules: level: error redundant-data-import: level: error + unresolved-import: + level: error use-rego-v1: level: error performance: diff --git a/bundle/regal/rules/imports/unresolved_import.rego b/bundle/regal/rules/imports/unresolved_import.rego new file mode 100644 index 00000000..297af966 --- /dev/null +++ b/bundle/regal/rules/imports/unresolved_import.rego @@ -0,0 +1,142 @@ +# METADATA +# description: Unresolved import +package regal.rules.imports["unresolved-import"] + +import rego.v1 + +import data.regal.config +import data.regal.result +import data.regal.util + +aggregate contains entry if { + package_path := [part.value | some i, part in input["package"].path; i > 0] + + imports_with_location := [imp | + some _import in input.imports + + _import.path.value[0].value == "data" + len := count(_import.path.value) + len > 1 + path := [part.value | some part in array.slice(_import.path.value, 1, len)] + + # Special case for custom rules, where we don't want to flag e.g. `import data.regal.ast` + # as unknown, even though it's not a package included in evaluation. + not custom_regal_package_and_import(package_path, path) + + imp := object.union(result.location(_import), {"path": path}) + ] + + exported_refs := {package_path} | {ref | + some rule in input.rules + + # locations will only contribute to each item in the set being unique, + # which we don't want here — we only care for distinct ref paths + some ref in to_paths(package_path, rule.head.ref) + } + + entry := result.aggregate(rego.metadata.chain(), { + "imports": imports_with_location, + "exported_refs": exported_refs, + }) +} + +# METADATA +# schemas: +# - input: schema.regal.aggregate +aggregate_report contains violation if { + all_known_refs := {path | + some entry in input.aggregate + + some path in entry.aggregate_data.exported_refs + } + + all_imports := {imp | + some entry in input.aggregate + some imp in entry.aggregate_data.imports + } + + some imp in all_imports + not imp.path in (all_known_refs | except_imports) + + # cheap operation failed — need to check wildcards here to account + # for map generating / general ref head rules + not wildcard_match(imp.path, all_known_refs, except_imports) + + violation := result.fail(rego.metadata.chain(), result.location(imp)) +} + +custom_regal_package_and_import(pkg_path, path) if { + pkg_path[0] == "custom" + pkg_path[1] == "regal" + path[0] == "regal" +} + +# the package part will always be included exported refs +# but if we have a rule like foo.bar.baz +# we'll want to include both foo.bar and foo.bar.baz +to_paths(pkg_path, ref) := [to_path(pkg_path, ref)] if count(ref) < 3 + +to_paths(pkg_path, ref) := paths if { + count(ref) > 2 + + paths := [path | + some p in util.all_paths(ref) + path := to_path(pkg_path, p) + ] +} + +to_path(pkg_path, ref) := array.concat(pkg_path, [str | + some i, part in ref + str := to_string(i, part) +]) + +to_string(0, part) := part.value + +to_string(i, part) := part.value if { + i > 0 + part.type == "string" +} + +to_string(i, part) := "**" if { + i > 0 + part.type == "var" +} + +all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))] + +except_imports contains exception if { + cfg := config.for_rule("imports", "unresolved-import") + + some str in cfg["except-imports"] + exception := trim_data(split(str, ".")) +} + +trim_data(path) := array.slice(path, 1, count(path)) if path[0] == "data" + +trim_data(path) := path if path[0] != "data" + +wildcard_match(imp_path, all_known_refs, except_imports) if { + except_imports_wildcards := {path | + some except in except_imports + path := concat(".", except) + contains(path, "*") + } + + all_known_refs_wildcards := {path | + some ref in all_known_refs + path := concat(".", ref) + contains(path, "*") + } + + all_wildcard_paths := except_imports_wildcards | all_known_refs_wildcards + + some path in all_wildcard_paths + + # note that we are quite forgiving here, as we'll match the + # shortest path component containing a wildcard at the end.. + # we may want to make this more strict later, but as this is + # a new rule with a potentially high impact, let's start like + # this and then decide if we want to be more strict later, and + # perhaps offer that as a "strict" option + glob.match(path, [], concat(".", imp_path)) +} diff --git a/bundle/regal/rules/imports/unresolved_import_test.rego b/bundle/regal/rules/imports/unresolved_import_test.rego new file mode 100644 index 00000000..d41b3503 --- /dev/null +++ b/bundle/regal/rules/imports/unresolved_import_test.rego @@ -0,0 +1,124 @@ +package regal.rules.imports["unresolved-import_test"] + +import rego.v1 + +import data.regal.config + +import data.regal.rules.imports["unresolved-import"] as rule + +test_fail_identifies_unresolved_imports if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar + import data.bar.x + import data.bar.nope + import data.nope + + x := 1 + `) + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + import data.foo + import data.foo.x + + x := 1 + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == { + with_location({"file": "p1.rego", "row": 5, "col": 2, "text": "\timport data.nope"}), + with_location({"file": "p1.rego", "row": 4, "col": 2, "text": "\timport data.bar.nope"}), + } +} + +test_success_no_unresolved_imports if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar.x + + x := 1 + `) + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + import data.foo.x + + x := 1 + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +test_success_unresolved_imports_are_excepted if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar.x + import data.bar.excepted + + x := 1 + `) + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + x := 1 + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + with config.for_rule as {"level": "error", "except-imports": ["data.bar.excepted"]} + r == set() +} + +test_success_resolved_import_in_middle_of_explicit_paths if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar.x.y + `) + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + + x.y.z := 1 + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +test_success_map_rule_may_resolve_so_allow if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar.x.y + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + import rego.v1 + + x[y] := z if { + some y in input.ys + z := {"foo": y + 1} + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +test_success_general_ref_head_rule_may_resolve_so_allow if { + agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo + import data.bar.x.foo.z.bar + `) + + agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar + import rego.v1 + + x[y].z[foo] := z if { + some y in input.ys + z := {"foo": y + 1} + } + `) + + r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)} + r == set() +} + +with_location(location) := { + "category": "imports", + "description": "Unresolved import", + "level": "error", + "location": location, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/unresolved-import", "imports"), + }], + "title": "unresolved-import", +} diff --git a/bundle/regal/rules/style/prefer_some_in_iteration.rego b/bundle/regal/rules/style/prefer_some_in_iteration.rego index 534264c0..a5101752 100644 --- a/bundle/regal/rules/style/prefer_some_in_iteration.rego +++ b/bundle/regal/rules/style/prefer_some_in_iteration.rego @@ -7,6 +7,7 @@ import rego.v1 import data.regal.ast import data.regal.config import data.regal.result +import data.regal.util cfg := config.for_rule("style", "prefer-some-in-iteration") @@ -59,11 +60,9 @@ filter_top_level_ref(rule) := rule.body if { rule.head.value.type == "ref" } else := rule -all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))] - # don't recommend `some .. in` if iteration occurs inside of arrays, objects, or sets invalid_some_context(rule, path) if { - some p in all_paths(path) + some p in util.all_paths(path) node := object.get(rule, p, []) @@ -76,7 +75,7 @@ invalid_some_context(rule, path) if { # not _directly_ replaceable by `some .. in`, so we'll leave it # be here invalid_some_context(rule, path) if { - some p in all_paths(path) + some p in util.all_paths(path) node := object.get(rule, p, []) diff --git a/bundle/regal/util/util.rego b/bundle/regal/util/util.rego index 4bf29364..b4e5debe 100644 --- a/bundle/regal/util/util.rego +++ b/bundle/regal/util/util.rego @@ -18,3 +18,9 @@ find_duplicates(arr) := {indices | count(indices) > 1 } + +# METADATA +# description: | +# returns an array of arrays built from all parts of the provided path array, +# so e.g. [1, 2, 3] would return [[1], [1, 2], [1, 2, 3]] +all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))] diff --git a/docs/rules/imports/unresolved-import.md b/docs/rules/imports/unresolved-import.md new file mode 100644 index 00000000..84d9981e --- /dev/null +++ b/docs/rules/imports/unresolved-import.md @@ -0,0 +1,64 @@ +# unresolved-import + +**Summary**: Unresolved import + +**Category**: Imports + +**Avoid** + +Imports that can't be resolved. + +## Rationale + +OPA does no compile time checks to ensure that references in imports _resolve_ to anything, and unresolved references at +runtime are simply **undefined**. This is not a bug in OPA, but a necessary feature to allow for dynamic loading of data +and policy at runtime. The fact that it's not a bug does however not mean that it can't be +[a problem](https://github.com/open-policy-agent/opa/issues/491)! A simple typo, a refactoring, or a mistake, could +easily lead to an an import being unresolved, and as such undefined at runtime. + +This rule takes a stricter approach to imports, and will have Regal try to resolve them by scanning all the policies it +is provided for **packages**, **rules** and **functions** that may resolve the import. Note that Regal does not scan any +_data_ files. If no reference is found, the rule will flag it as unresolved. + +Since unresolved imports may be perfectly valid — for example when an import points to data — this rule provides an +option in its configuration to except certain paths from being checked. These paths may even contain a wildcard suffix +to indicate that any path past the wildcard (e.g. `data.users.*`) should be ignored. It is also possible to use a +regular [ignore directive](https://docs.styra.com/regal#inline-ignore-directives): + +```rego +package example + +# this is provided as data! +# regal ignore:unresolved-import +import data.users +``` + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + imports: + unresolved-import: + # one of "error", "warning", "ignore" + level: error + # list of paths that should be ignored + # these may be paths to data, or rules that may + # not be present at the time of linting + except-imports: + - data.identity.users + - data.permissions.* +``` + +## Related Resources + +- OPA Docs: [Imports](https://www.openpolicyagent.org/docs/latest/policy-language/#imports) +- OPA Docs: [Collaboration Using Import](https://www.openpolicyagent.org/docs/latest/faq/#collaboration-using-import) +- OPA Issues: [Missing import should create error](https://github.com/open-policy-agent/opa/issues/491) + +## Community + +If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules, +or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community +[Slack](https://communityinviter.com/apps/styracommunity/signup)!