From 5754512bbbf7bbd0c262fd17845a879e494d7cac Mon Sep 17 00:00:00 2001 From: Dan D'Avella Date: Wed, 4 Dec 2024 17:12:21 -0500 Subject: [PATCH] Update fixed findings metadata to align with CodeTF spec (#941) * Update Change.findings => fixedFindings to conform to spec * Add fixedFindings to ChangeSet per spec * Fix logic for gathering fixed findings --- .../codemods/imported_call_modifier.py | 2 +- src/codemodder/codemods/libcst_transformer.py | 2 +- src/codemodder/codemods/regex_transformer.py | 4 ++-- src/codemodder/codemods/test/utils.py | 2 +- src/codemodder/codemods/xml_transformer.py | 2 +- src/codemodder/codetf.py | 7 ++++-- src/codemodder/file_context.py | 22 +++++++++-------- .../utils/update_finding_metadata.py | 2 +- src/core_codemods/file_resource_leak.py | 2 +- .../test_avoid_insecure_deserialization.py | 24 +++++++++---------- .../semgrep/test_django_secure_set_cookie.py | 6 ++--- .../test_semgrep_sql_parametrization.py | 4 +++- .../test_sonar_django_json_response_type.py | 8 +++---- .../test_sonar_django_receiver_on_top.py | 4 ++-- .../sonar/test_sonar_fix_assert_tuple.py | 6 ++--- ...sonar_remove_assertion_in_pytest_raises.py | 2 +- tests/test_update_finding_metadata.py | 16 ++++++------- 17 files changed, 61 insertions(+), 54 deletions(-) diff --git a/src/codemodder/codemods/imported_call_modifier.py b/src/codemodder/codemods/imported_call_modifier.py index 4ce2f045..3a768e7d 100644 --- a/src/codemodder/codemods/imported_call_modifier.py +++ b/src/codemodder/codemods/imported_call_modifier.py @@ -81,7 +81,7 @@ def leave_Call(self, original_node: cst.Call, updated_node: cst.Call): Change( lineNumber=line_number, description=self.change_description, - findings=self.file_context.get_findings_for_location( + fixedFindings=self.file_context.get_findings_for_location( line_number ), ) diff --git a/src/codemodder/codemods/libcst_transformer.py b/src/codemodder/codemods/libcst_transformer.py index ef4b6e9c..0bc3fe85 100644 --- a/src/codemodder/codemods/libcst_transformer.py +++ b/src/codemodder/codemods/libcst_transformer.py @@ -126,7 +126,7 @@ def report_change_for_line( Change( lineNumber=line_number, description=description or self.change_description, - findings=findings + fixedFindings=findings or self.file_context.get_findings_for_location(line_number), ) ) diff --git a/src/codemodder/codemods/regex_transformer.py b/src/codemodder/codemods/regex_transformer.py index efc1c477..f19a2956 100644 --- a/src/codemodder/codemods/regex_transformer.py +++ b/src/codemodder/codemods/regex_transformer.py @@ -40,7 +40,7 @@ def _apply(self, original_lines, file_context, results): Change( lineNumber=lineno + 1, description=self.change_description, - findings=file_context.get_findings_for_location(lineno), + fixedFindings=file_context.get_findings_for_location(lineno), ) ) return changes, updated_lines @@ -110,7 +110,7 @@ def _apply(self, original_lines, file_context, results): Change( lineNumber=lineno + 1, description=self.change_description, - findings=file_context.get_findings_for_location(lineno), + fixedFindings=file_context.get_findings_for_location(lineno), ) ) diff --git a/src/codemodder/codemods/test/utils.py b/src/codemodder/codemods/test/utils.py index 78fb9045..3ccdbdd7 100644 --- a/src/codemodder/codemods/test/utils.py +++ b/src/codemodder/codemods/test/utils.py @@ -224,5 +224,5 @@ def run_and_assert( def assert_findings(self, changes: list[Change]): assert all( - x.findings for x in changes + x.fixedFindings for x in changes ), f"Expected all changes to have findings: {changes}" diff --git a/src/codemodder/codemods/xml_transformer.py b/src/codemodder/codemods/xml_transformer.py index 11a94b7a..d18b4c90 100644 --- a/src/codemodder/codemods/xml_transformer.py +++ b/src/codemodder/codemods/xml_transformer.py @@ -96,7 +96,7 @@ def add_change(self, line): Change( lineNumber=line, description=self.change_description or None, - findings=self.file_context.get_findings_for_location(line), + fixedFindings=self.file_context.get_findings_for_location(line), ) ) diff --git a/src/codemodder/codetf.py b/src/codemodder/codetf.py index ecee2b18..75fc195e 100644 --- a/src/codemodder/codetf.py +++ b/src/codemodder/codetf.py @@ -62,7 +62,7 @@ class Change(BaseModel): diffSide: DiffSide = DiffSide.RIGHT properties: Optional[dict] = None packageActions: Optional[list[PackageAction]] = None - findings: Optional[list[Finding]] = None + fixedFindings: Optional[list[Finding]] = None @model_validator(mode="after") def validate_lineNumber(self): @@ -83,7 +83,7 @@ def with_findings(self, findings: list[Finding] | None) -> Change: diffSide=self.diffSide, properties=self.properties, packageActions=self.packageActions, - findings=findings, + fixedFindings=findings, ) @@ -108,6 +108,8 @@ class ChangeSet(BaseModel): ai: Optional[AIMetadata] = None strategy: Optional[Strategy] = None provisional: Optional[bool] = False + # For fixed findings that are not associated with a specific change + fixedFindings: Optional[list[Finding]] = None def with_changes(self, changes: list[Change]) -> ChangeSet: return ChangeSet( @@ -117,6 +119,7 @@ def with_changes(self, changes: list[Change]) -> ChangeSet: ai=self.ai, strategy=self.strategy, provisional=self.provisional, + fixedFindings=self.fixedFindings, ) diff --git a/src/codemodder/file_context.py b/src/codemodder/file_context.py index 5aaec3c0..a10ba0bb 100644 --- a/src/codemodder/file_context.py +++ b/src/codemodder/file_context.py @@ -50,20 +50,22 @@ def add_unfixed_findings( ] ) - def get_findings_for_location(self, line_number: int): + def get_findings_for_location(self, line_number: int) -> list[Finding]: return [ result.finding for result in (self.results or []) - if any( - location.start.line <= line_number <= location.end.line - for location in result.locations - ) - or any( - location.start.line <= line_number <= location.end.line - for codeflow in result.codeflows - for location in codeflow + if result.finding is not None + and ( + any( + location.start.line <= line_number <= location.end.line + for location in result.locations + ) + or any( + location.start.line <= line_number <= location.end.line + for codeflow in result.codeflows + for location in codeflow + ) ) - and result.finding is not None ] def match_findings(self, line_numbers): diff --git a/src/codemodder/utils/update_finding_metadata.py b/src/codemodder/utils/update_finding_metadata.py index 6dda95de..5360fb6f 100644 --- a/src/codemodder/utils/update_finding_metadata.py +++ b/src/codemodder/utils/update_finding_metadata.py @@ -27,7 +27,7 @@ def update_finding_metadata( if finding.rule.id in tool_rule_map else finding ) - for finding in change.findings or [] + for finding in change.fixedFindings or [] ] or None ) diff --git a/src/core_codemods/file_resource_leak.py b/src/core_codemods/file_resource_leak.py index 89fab3ec..8ce6c3e0 100644 --- a/src/core_codemods/file_resource_leak.py +++ b/src/core_codemods/file_resource_leak.py @@ -229,7 +229,7 @@ def _handle_block( Change( lineNumber=line_number, description=FileResourceLeakTransformer.change_description, - findings=self.file_context.get_findings_for_location( + fixedFindings=self.file_context.get_findings_for_location( line_number ), ) diff --git a/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py b/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py index 16931f0f..76cd9944 100644 --- a/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py +++ b/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py @@ -46,9 +46,9 @@ def test_yaml_load(self, tmpdir): ) assert changes is not None - assert changes[0].changes[0].findings is not None - assert changes[0].changes[0].findings[0].id == "1" - assert changes[0].changes[0].findings[0].rule.id == RULE_ID + assert changes[0].changes[0].fixedFindings is not None + assert changes[0].changes[0].fixedFindings[0].id == "1" + assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID @mock.patch("codemodder.codemods.api.FileContext.add_dependency") def test_pickle_load(self, adds_dependency, tmpdir): @@ -80,9 +80,9 @@ def test_pickle_load(self, adds_dependency, tmpdir): adds_dependency.assert_called_once_with(Fickling) assert changes is not None - assert changes[0].changes[0].findings is not None - assert changes[0].changes[0].findings[0].id == "2" - assert changes[0].changes[0].findings[0].rule.id == RULE_ID + assert changes[0].changes[0].fixedFindings is not None + assert changes[0].changes[0].fixedFindings[0].id == "2" + assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID @mock.patch("codemodder.codemods.api.FileContext.add_dependency") def test_pickle_and_yaml(self, adds_dependency, tmpdir): @@ -128,12 +128,12 @@ def test_pickle_and_yaml(self, adds_dependency, tmpdir): adds_dependency.assert_called_once_with(Fickling) assert changes is not None - assert changes[0].changes[0].findings is not None - assert changes[0].changes[0].findings[0].id == "4" - assert changes[0].changes[0].findings[0].rule.id == RULE_ID - assert changes[0].changes[1].findings is not None - assert changes[0].changes[1].findings[0].id == "3" - assert changes[0].changes[1].findings[0].rule.id == RULE_ID + assert changes[0].changes[0].fixedFindings is not None + assert changes[0].changes[0].fixedFindings[0].id == "4" + assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID + assert changes[0].changes[1].fixedFindings is not None + assert changes[0].changes[1].fixedFindings[0].id == "3" + assert changes[0].changes[1].fixedFindings[0].rule.id == RULE_ID @mock.patch("codemodder.codemods.api.FileContext.add_dependency") def test_pickle_loads(self, adds_dependency, tmpdir): diff --git a/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py b/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py index 525b21c0..ff5ee628 100644 --- a/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py +++ b/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py @@ -37,9 +37,9 @@ def test_simple(self, tmpdir): ) assert changes is not None - assert changes[0].changes[0].findings is not None - assert changes[0].changes[0].findings[0].id == "1" + assert changes[0].changes[0].fixedFindings is not None + assert changes[0].changes[0].fixedFindings[0].id == "1" assert ( - changes[0].changes[0].findings[0].rule.id + changes[0].changes[0].fixedFindings[0].rule.id == "python.django.security.audit.secure-cookies.django-secure-set-cookie" ) diff --git a/tests/codemods/semgrep/test_semgrep_sql_parametrization.py b/tests/codemods/semgrep/test_semgrep_sql_parametrization.py index 0b8a9111..0d79cff7 100644 --- a/tests/codemods/semgrep/test_semgrep_sql_parametrization.py +++ b/tests/codemods/semgrep/test_semgrep_sql_parametrization.py @@ -195,4 +195,6 @@ def f(): expexted_output, results=json.dumps(results), ) - assert len(changes[0].changes[0].findings) == len(results["runs"][0]["results"]) + assert len(changes[0].changes[0].fixedFindings) == len( + results["runs"][0]["results"] + ) diff --git a/tests/codemods/sonar/test_sonar_django_json_response_type.py b/tests/codemods/sonar/test_sonar_django_json_response_type.py index 3a5f56bc..6b392490 100644 --- a/tests/codemods/sonar/test_sonar_django_json_response_type.py +++ b/tests/codemods/sonar/test_sonar_django_json_response_type.py @@ -50,7 +50,7 @@ def foo(request): tmpdir, input_code, expected, results=json.dumps(issues) ) assert changes is not None - assert changes[0].changes[0].findings is not None - assert changes[0].changes[0].findings[0].id == rule_id - assert changes[0].changes[0].findings[0].rule.id == rule_id - assert changes[0].changes[0].findings[0].rule.name == rule_id + assert changes[0].changes[0].fixedFindings is not None + assert changes[0].changes[0].fixedFindings[0].id == rule_id + assert changes[0].changes[0].fixedFindings[0].rule.id == rule_id + assert changes[0].changes[0].fixedFindings[0].rule.name == rule_id diff --git a/tests/codemods/sonar/test_sonar_django_receiver_on_top.py b/tests/codemods/sonar/test_sonar_django_receiver_on_top.py index 513906ce..77d2c013 100644 --- a/tests/codemods/sonar/test_sonar_django_receiver_on_top.py +++ b/tests/codemods/sonar/test_sonar_django_receiver_on_top.py @@ -13,8 +13,8 @@ def test_name(self): def assert_findings(self, changes): # For now we can only link the finding to the line with the receiver decorator - assert changes[0].findings - assert not changes[1].findings + assert changes[0].fixedFindings + assert not changes[1].fixedFindings def test_simple(self, tmpdir): input_code = """ diff --git a/tests/codemods/sonar/test_sonar_fix_assert_tuple.py b/tests/codemods/sonar/test_sonar_fix_assert_tuple.py index 3d371d13..573634cc 100644 --- a/tests/codemods/sonar/test_sonar_fix_assert_tuple.py +++ b/tests/codemods/sonar/test_sonar_fix_assert_tuple.py @@ -13,9 +13,9 @@ def test_name(self): def assert_findings(self, changes): # For now we can only link the finding to the first line changed - assert changes[0].findings - assert not changes[1].findings - assert not changes[2].findings + assert changes[0].fixedFindings + assert not changes[1].fixedFindings + assert not changes[2].fixedFindings def test_simple(self, tmpdir): input_code = """ diff --git a/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py b/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py index ca9eae0a..24dcdd19 100644 --- a/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py +++ b/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py @@ -14,7 +14,7 @@ def test_name(self): assert self.codemod.name == "remove-assertion-in-pytest-raises" def assert_findings(self, changes): - assert not all(x.findings for x in changes) + assert not all(x.fixedFindings for x in changes) def test_simple(self, tmpdir): input_code = """ diff --git a/tests/test_update_finding_metadata.py b/tests/test_update_finding_metadata.py index 5f9a762f..5dc43169 100644 --- a/tests/test_update_finding_metadata.py +++ b/tests/test_update_finding_metadata.py @@ -12,14 +12,14 @@ def test_update_finding_metdata(): Change( lineNumber=1, description="foo", - findings=[ + fixedFindings=[ Finding(id="rule_id", rule=Rule(id="rule_id", name="other_name")) ], ), Change( lineNumber=2, description="bar", - findings=[ + fixedFindings=[ Finding(id="other_id", rule=Rule(id="other_id", name="other_name")) ], ), @@ -34,10 +34,10 @@ def test_update_finding_metdata(): tool_rules=[tool_rule], changesets=[changeset] ) - assert new_changesets[0].changes[0].findings - assert new_changesets[0].changes[0].findings[0].rule.name == "rule_name" - assert new_changesets[0].changes[0].findings[0].rule.url == "rule_url" - assert new_changesets[0].changes[1].findings - assert new_changesets[0].changes[1].findings[0].rule.name == "other_name" - assert new_changesets[0].changes[1].findings[0].rule.url is None + assert new_changesets[0].changes[0].fixedFindings + assert new_changesets[0].changes[0].fixedFindings[0].rule.name == "rule_name" + assert new_changesets[0].changes[0].fixedFindings[0].rule.url == "rule_url" + assert new_changesets[0].changes[1].fixedFindings + assert new_changesets[0].changes[1].fixedFindings[0].rule.name == "other_name" + assert new_changesets[0].changes[1].fixedFindings[0].rule.url is None assert new_changesets[0].changes[2] == changeset.changes[2]