Skip to content

Commit

Permalink
Fix missing MatchErrors due to hash collisions (#4297)
Browse files Browse the repository at this point in the history
Setting the filename for match_error has been removed because this will
be done on MatchError instance creation via the __post_init__ method.
  • Loading branch information
cavcrosby authored and ssbarnea committed Sep 19, 2024
1 parent 571711f commit e5f7841
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 20 deletions.
3 changes: 3 additions & 0 deletions examples/playbooks/vars/rule_var_naming_fails_files/bar.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
CamelCaseIsBad: bar
ALL_CAPS_ARE_BAD_TOO: bar
3 changes: 3 additions & 0 deletions examples/playbooks/vars/rule_var_naming_fails_files/foo.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
CamelCaseIsBad: foo
ALL_CAPS_ARE_BAD_TOO: foo
55 changes: 43 additions & 12 deletions src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,15 @@ def get_var_naming_matcherror(
ident: str,
*,
prefix: Prefix | None = None,
file: Lintable,
) -> MatchError | None:
"""Return a MatchError if the variable name is not valid, otherwise None."""
if not isinstance(ident, str): # pragma: no cover
return MatchError(
tag="var-naming[non-string]",
message="Variables names must be strings.",
rule=self,
lintable=file,
)

if ident in ANNOTATION_KEYS or ident in self.allowed_special_names:
Expand All @@ -136,27 +138,31 @@ def get_var_naming_matcherror(
tag="var-naming[non-ascii]",
message=f"Variables names must be ASCII. ({ident})",
rule=self,
lintable=file,
)

if keyword.iskeyword(ident):
return MatchError(
tag="var-naming[no-keyword]",
message=f"Variables names must not be Python keywords. ({ident})",
rule=self,
lintable=file,
)

if ident in self.reserved_names:
return MatchError(
tag="var-naming[no-reserved]",
message=f"Variables names must not be Ansible reserved names. ({ident})",
rule=self,
lintable=file,
)

if ident in self.read_only_names:
return MatchError(
tag="var-naming[read-only]",
message=f"This special variable is read-only. ({ident})",
rule=self,
lintable=file,
)

# We want to allow use of jinja2 templating for variable names
Expand All @@ -165,6 +171,7 @@ def get_var_naming_matcherror(
tag="var-naming[no-jinja]",
message="Variables names must not contain jinja2 templating.",
rule=self,
lintable=file,
)

if not bool(self.re_pattern.match(ident)) and (
Expand All @@ -174,6 +181,7 @@ def get_var_naming_matcherror(
tag="var-naming[pattern]",
message=f"Variables names should match {self.re_pattern_str} regex. ({ident})",
rule=self,
lintable=file,
)

if (
Expand All @@ -186,6 +194,7 @@ def get_var_naming_matcherror(
tag="var-naming[no-role-prefix]",
message=f"Variables names from within roles should use {prefix.value}_ as a prefix.",
rule=self,
lintable=file,
)
return None

Expand All @@ -199,9 +208,8 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
# If the Play uses the 'vars' section to set variables
our_vars = data.get("vars", {})
for key in our_vars:
match_error = self.get_var_naming_matcherror(key)
match_error = self.get_var_naming_matcherror(key, file=file)
if match_error:
match_error.filename = str(file.path)
match_error.lineno = (
key.ansible_pos[1]
if isinstance(key, AnsibleUnicode)
Expand All @@ -219,9 +227,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
match_error.lineno = (
key.ansible_pos[1]
Expand All @@ -235,9 +243,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
match_error.lineno = (
key.ansible_pos[1]
Expand Down Expand Up @@ -266,7 +274,6 @@ def matchtask(
"""Return matches for task based variables."""
results = []
prefix = Prefix()
filename = "" if file is None else str(file.path)
if file and file.parent and file.parent.kind == "role":
prefix = Prefix(file.parent.path.name)
ansible_module = task["action"]["__ansible_module__"]
Expand All @@ -283,9 +290,9 @@ def matchtask(
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.filename = filename
match_error.lineno = our_vars[LINE_NUMBER_KEY]
match_error.message += f" (vars: {key})"
results.append(match_error)
Expand All @@ -298,9 +305,12 @@ def matchtask(
and x != "cacheable",
task["action"].keys(),
):
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.filename = filename
match_error.lineno = task["action"][LINE_NUMBER_KEY]
match_error.message += f" (set_fact: {key})"
results.append(match_error)
Expand All @@ -311,10 +321,10 @@ def matchtask(
match_error = self.get_var_naming_matcherror(
registered_var,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.message += f" (register: {registered_var})"
match_error.filename = filename
match_error.lineno = task[LINE_NUMBER_KEY]
results.append(match_error)

Expand All @@ -325,15 +335,17 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
results: list[MatchError] = []
raw_results: list[MatchError] = []
meta_data: dict[AnsibleUnicode, Any] = {}
filename = "" if file is None else str(file.path)

if str(file.kind) == "vars" and file.data:
meta_data = parse_yaml_from_file(str(file.path))
for key in meta_data:
prefix = Prefix(file.role) if file.role else Prefix()
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = filename
match_error.lineno = key.ansible_pos[1]
match_error.message += f" (vars: {key})"
raw_results.append(match_error)
Expand Down Expand Up @@ -413,6 +425,25 @@ def test_invalid_var_name_varsfile(
assert result.tag == expected_errors[idx][0]
assert result.lineno == expected_errors[idx][1]

def test_invalid_vars_diff_files(
default_rules_collection: RulesCollection,
) -> None:
"""Test rule matches."""
results = Runner(
Lintable("examples/playbooks/vars/rule_var_naming_fails_files"),
rules=default_rules_collection,
).run()
expected_errors = (
("var-naming[pattern]", 2),
("var-naming[pattern]", 3),
("var-naming[pattern]", 2),
("var-naming[pattern]", 3),
)
assert len(results) == len(expected_errors)
for idx, result in enumerate(results):
assert result.tag == expected_errors[idx][0]
assert result.lineno == expected_errors[idx][1]

def test_var_naming_with_role_prefix(
default_rules_collection: RulesCollection,
) -> None:
Expand Down
13 changes: 6 additions & 7 deletions src/ansiblelint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ def taskshandlers_children(
raise MatchError(
message="A malformed block was encountered while loading a block.",
rule=RuntimeErrorRule(),
lintable=lintable,
)
for task_handler in v:
# ignore empty tasks, `-`
Expand Down Expand Up @@ -408,23 +409,21 @@ def _validate_task_handler_action_for_role(self, th_action: dict[str, Any]) -> N
"""Verify that the task handler action is valid for role include."""
module = th_action["__ansible_module__"]

lintable = Lintable(
self.rules.options.lintables[0] if self.rules.options.lintables else ".",
)
if "name" not in th_action:
raise MatchError(
message=f"Failed to find required 'name' key in {module!s}",
rule=self.rules.rules[0],
lintable=Lintable(
(
self.rules.options.lintables[0]
if self.rules.options.lintables
else "."
),
),
lintable=lintable,
)

if not isinstance(th_action["name"], str):
raise MatchError(
message=f"Value assigned to 'name' key on '{module!s}' is not a string.",
rule=self.rules.rules[1],
lintable=lintable,
)

def roles_children(
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ setenv =
PRE_COMMIT_COLOR = always
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests. (tox-extra)
PYTEST_REQPASS = 893
PYTEST_REQPASS = 894
FORCE_COLOR = 1
pre: PIP_PRE = 1
allowlist_externals =
Expand Down

0 comments on commit e5f7841

Please sign in to comment.