Skip to content

Commit

Permalink
fix: sprintf-arguments-mismatchfalse positive when var used as form…
Browse files Browse the repository at this point in the history
…at (StyraInc#1107)

While this rule likely never will catch all violations, as some are just
too expensive to find for something very unlikely — it should never report
false positives.

Fixes StyraInc#1104

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored and srenatus committed Oct 1, 2024
1 parent dbb4bbd commit 24105f3
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import rego.v1
import data.regal.ast
import data.regal.config
import data.regal.result
import data.regal.util

# METADATA
# description: Missing capability for built-in `sprintf`
Expand All @@ -20,13 +21,24 @@ notices contains result.notice(rego.metadata.chain()) if not "sprintf" in object
# compare it to the number of items in the array (if known), and flag when the numbers
# don't match
report contains violation if {
some fn
ast.function_calls[_][fn].name == "sprintf"
some rule_index, fn
ast.function_calls[rule_index][fn].name == "sprintf"

# this could come either from a term directly (the common case):
# sprintf("%d", [1])
# or a variable (quite uncommon):
# sprintf(format, [1])
# in the latter case, we try to "resolve" the `format` value by checking if
# it was assigned in the scope. we do however only do this one level up, and
# this rule can definitely miss more advanced things, like re-assignemt from
# another variable. tbh, that's a waste of time. what we should make sure is
# to not report anything erroneously.
format_term := _first_arg_value(rule_index, fn.args[0])

fn.args[1].type == "array" # can only check static arrays, not vars

values_in_arr := count(fn.args[1].value)
str_no_escape := replace(fn.args[0].value, "%%", "") # don't include '%%' as it's used to "escape" %
str_no_escape := replace(format_term.value, "%%", "") # don't include '%%' as it's used to "escape" %
values_in_str := strings.count(str_no_escape, "%") - _repeated_explicit_argument_indexes(str_no_escape)

values_in_str != values_in_arr
Expand All @@ -44,3 +56,22 @@ _repeated_explicit_argument_indexes(str) := sum([n |
])

_unique_explicit_arguments(str) := {eai | some eai in regex.find_n(`%\[\d\]`, str, -1)}

_first_arg_value(_, term) := term if term.type == "string"

_first_arg_value(rule_index, term) := found if {
term.type == "var"

trow := util.to_location_object(term.location).row

found := [rhs |
some expr in ast.exprs[to_number(rule_index)]

util.to_location_object(expr.location).row < trow

[lhs, rhs] := ast.assignment_terms(expr)
lhs.type == "var"
lhs.value == term.value
rhs.type == "string"
][0]
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,38 @@ test_fail_different_number_of_values_with_explicit_index if {
}}
}

test_fail_first_arg_is_variable_with_nonmatching_pattern if {
r := rule.report with input as ast.with_rego_v1(`rule if {
s := "%s%s"
sprintf(s, ["foo"])
}`)
r == {{
"category": "bugs",
"description": "Mismatch in `sprintf` arguments count",
"level": "error",
"location": {
"col": 11,
"end": {"col": 21, "row": 7},
"file": "policy.rego",
"row": 7,
"text": "\t\tsprintf(s, [\"foo\"])",
},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/sprintf-arguments-mismatch", "bugs"),
}],
"title": "sprintf-arguments-mismatch",
}}
}

test_success_first_arg_is_variable_with_matching_pattern if {
r := rule.report with input as ast.with_rego_v1(`rule if {
s := "%s"
sprintf(s, ["foo"]) == "foo"
}`)
r == set()
}

test_success_same_number_of_values_with_explicit_index if {
r := rule.report with input as ast.with_rego_v1(`x := sprintf("%[1]s %[1]s %[2]d", [1, 2])`)
r == set()
Expand Down

0 comments on commit 24105f3

Please sign in to comment.