diff --git a/dojo/finding/deduplication.py b/dojo/finding/deduplication.py index bf91b610b6d..3f8e0a24640 100644 --- a/dojo/finding/deduplication.py +++ b/dojo/finding/deduplication.py @@ -1,6 +1,5 @@ import logging from collections.abc import Iterator -from operator import attrgetter import hyperlink from django.conf import settings @@ -544,12 +543,81 @@ def find_candidates_for_reimport_legacy(test, findings, service=None): return existing_by_key +def deduplication_ordering_key(finding): + """ + Stable, content-derived sort key for choosing the canonical "original" + among findings that collide on the deduplication key. + + Independent of parser emission order and DB id, so the winner is + reproducible across re-scans regardless of the order the scanner exports + its findings in. id is the final tiebreak and is only reached when two + findings are identical across every content field in the key (in which + case the choice is immaterial because the findings are interchangeable). + + All fields referenced here are part of ``Finding.DEDUPLICATION_FIELDS``, so + building this key never triggers extra database queries during dedupe. + """ + return ( + finding.hash_code or "", + finding.unique_id_from_tool or "", + finding.file_path or "", + finding.line if finding.line is not None else -1, + finding.title or "", + finding.id or 0, + ) + + +def _candidate_sort_key(candidate, batch_min_id): + """ + Order candidates so the first "older" one encountered is the canonical original. + + Findings that predate the current import batch (id < batch_min_id, or when no + batch context is available) are ranked first and ordered by id, preserving the + historical "pre-existing finding stays the original" behaviour. Findings created + within the current batch are ranked after pre-existing ones and ordered by the + stable content key so the winner does not depend on parser emission order. + + ``batch_min_id`` is an optional marker; when it is not an integer (absent) or the + candidate has no integer id yet (unsaved/preview), the candidate is treated as + pre-existing and ordered by id, so arithmetic is only ever done on real ids. + """ + candidate_id = candidate.id if isinstance(candidate.id, int) else None + if not isinstance(batch_min_id, int) or candidate_id is None or candidate_id < batch_min_id: + return (0, candidate_id or 0) + return (1, deduplication_ordering_key(candidate)) + + +def _prepare_batch_ordering(findings): + """ + Stamp every finding in a dedup batch with the batch's minimum id and return the + batch sorted by the stable content key. + + The stamped ``_dedupe_batch_min_id`` lets ``_is_candidate_older`` tell findings + created within this batch apart from pre-existing candidates loaded from the DB + (which always have smaller ids), so pre-existing findings keep their id-based + ordering while intra-batch ties are broken deterministically by content. + """ + batch_ids = [f.id for f in findings if f.id is not None] + batch_min_id = min(batch_ids) if batch_ids else None + for f in findings: + f._dedupe_batch_min_id = batch_min_id + return sorted(findings, key=deduplication_ordering_key) + + def _is_candidate_older(new_finding, candidate): # Unsaved findings (e.g. preview mode) have no PK — all DB candidates are older by definition if new_finding.pk is None: return True - # Ensure the newer finding is marked as duplicate of the older finding - is_older = candidate.id < new_finding.id + # Findings created within the current import batch all have ids >= batch_min_id + # (auto-increment guarantees pre-existing findings have smaller ids). For those + # we break ties by the stable content key so the "original" is deterministic + # regardless of the order the scanner emitted the findings. Pre-existing + # candidates keep the id ordering so an already-existing finding stays original. + batch_min_id = getattr(new_finding, "_dedupe_batch_min_id", None) + if batch_min_id is not None and candidate.id is not None and candidate.id >= batch_min_id: + is_older = deduplication_ordering_key(candidate) < deduplication_ordering_key(new_finding) + else: + is_older = candidate.id < new_finding.id if not is_older: deduplicationLogger.debug(f"candidate is newer than or equal to new finding: {new_finding.id} and candidate {candidate.id}") return is_older @@ -558,7 +626,11 @@ def _is_candidate_older(new_finding, candidate): def get_matches_from_hash_candidates(new_finding, candidates_by_hash) -> Iterator[Finding]: if new_finding.hash_code is None: return - possible_matches = candidates_by_hash.get(new_finding.hash_code, []) + batch_min_id = getattr(new_finding, "_dedupe_batch_min_id", None) + possible_matches = sorted( + candidates_by_hash.get(new_finding.hash_code, []), + key=lambda c: _candidate_sort_key(c, batch_min_id), + ) deduplicationLogger.debug(f"Finding {new_finding.id}: Found {len(possible_matches)} findings with same hash_code, ids={[(c.id, c.hash_code) for c in possible_matches]}") for candidate in possible_matches: @@ -575,7 +647,11 @@ def get_matches_from_unique_id_candidates(new_finding, candidates_by_uid) -> Ite if new_finding.unique_id_from_tool is None: return - possible_matches = candidates_by_uid.get(new_finding.unique_id_from_tool, []) + batch_min_id = getattr(new_finding, "_dedupe_batch_min_id", None) + possible_matches = sorted( + candidates_by_uid.get(new_finding.unique_id_from_tool, []), + key=lambda c: _candidate_sort_key(c, batch_min_id), + ) deduplicationLogger.debug(f"Finding {new_finding.id}: Found {len(possible_matches)} findings with same unique_id_from_tool, ids={[(c.id, c.unique_id_from_tool) for c in possible_matches]}") for candidate in possible_matches: if not _is_candidate_older(new_finding, candidate): @@ -595,9 +671,10 @@ def get_matches_from_uid_or_hash_candidates(new_finding, candidates_by_uid, cand combined_by_id = {c.id: c for c in uid_list} for c in hash_list: combined_by_id.setdefault(c.id, c) - deduplicationLogger.debug("Finding %s: UID_OR_HASH: combined candidate ids (sorted)=%s", new_finding.id, sorted(combined_by_id.keys())) - for candidate_id in sorted(combined_by_id.keys()): - candidate = combined_by_id[candidate_id] + batch_min_id = getattr(new_finding, "_dedupe_batch_min_id", None) + ordered_candidates = sorted(combined_by_id.values(), key=lambda c: _candidate_sort_key(c, batch_min_id)) + deduplicationLogger.debug("Finding %s: UID_OR_HASH: combined candidate ids (ordered)=%s", new_finding.id, [c.id for c in ordered_candidates]) + for candidate in ordered_candidates: if not _is_candidate_older(new_finding, candidate): continue if is_deduplication_on_engagement_mismatch(new_finding, candidate): @@ -624,6 +701,14 @@ def get_matches_from_legacy_candidates(new_finding, candidates_by_title, candida if getattr(new_finding, "cwe", 0): candidates.extend(candidates_by_cwe.get(new_finding.cwe, [])) + # De-duplicate (a candidate can match on both title and CWE) and walk in + # canonical order so the first "older" candidate is the stable original. + batch_min_id = getattr(new_finding, "_dedupe_batch_min_id", None) + candidates = sorted( + {c.id: c for c in candidates}.values(), + key=lambda c: _candidate_sort_key(c, batch_min_id), + ) + for candidate in candidates: if not _is_candidate_older(new_finding, candidate): continue @@ -813,9 +898,9 @@ def match_batch_of_findings(findings): enabled = System_Settings.objects.get().enable_deduplication if not enabled: return [] - # Only sort by id for saved findings; unsaved findings have no id + # Only stamp/sort for saved findings; unsaved findings have no id or batch context if findings[0].pk is not None: - findings = sorted(findings, key=attrgetter("id")) + findings = _prepare_batch_ordering(findings) test = findings[0].test dedup_alg = test.deduplication_algorithm if dedup_alg == settings.DEDUPE_ALGO_HASH_CODE: @@ -934,8 +1019,10 @@ def dedupe_batch_of_findings(findings, *args, **kwargs): enabled = System_Settings.objects.get().enable_deduplication if enabled: - # sort findings by id to ensure deduplication is deterministic/reproducible - findings = sorted(findings, key=attrgetter("id")) + # Stamp each finding with the batch's minimum id and sort by the stable + # content key so deduplication is deterministic and independent of the + # order the scanner emitted its findings (see _is_candidate_older). + findings = _prepare_batch_ordering(findings) test = findings[0].test dedup_alg = test.deduplication_algorithm diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index e3c6cfbd387..818b936b4d0 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -8,6 +8,7 @@ import dojo.finding.helper as finding_helper from dojo.celery_dispatch import dojo_dispatch_task from dojo.finding.deduplication import ( + deduplication_ordering_key, find_candidates_for_deduplication_hash, find_candidates_for_deduplication_uid_or_hash, find_candidates_for_deduplication_unique_id, @@ -306,7 +307,12 @@ def _process_findings_internal( logger.debug("STEP 1: looping over findings from the reimported report and trying to match them to existing findings") deduplicationLogger.debug(f"Algorithm used for matching new findings to existing findings: {self.deduplication_algorithm}") - # Pre-sanitize and filter by minimum severity to avoid loop control pitfalls + # Pre-sanitize and filter by minimum severity to avoid loop control pitfalls. + # Findings are also fully prepared here (test/service/hash_code) so they can be + # sorted by a stable content key below — this makes the "which duplicate becomes + # the surviving finding" decision deterministic regardless of the order the + # scanner emitted its findings (intra-report duplicates are matched to whichever + # is created first; see add_new_finding_to_candidates). cleaned_findings = [] for raw_finding in parsed_findings or []: sanitized = self.sanitize_severity(raw_finding) @@ -318,8 +324,28 @@ def _process_findings_internal( self.minimum_severity, ) continue + # Some parsers provide "mitigated" field but do not set timezone (because they are probably not available in the report) + # Finding.mitigated is DateTimeField and it requires timezone + if sanitized.mitigated and not sanitized.mitigated.tzinfo: + sanitized.mitigated = sanitized.mitigated.replace(tzinfo=self.now.tzinfo) + # Override the test if needed + if not hasattr(sanitized, "test"): + sanitized.test = self.test + # Set the service supplied at import time + if self.service is not None: + sanitized.service = self.service + self.location_handler.clean_unsaved(sanitized) + # Calculate the hash code to be used to identify duplicates + sanitized.hash_code = self.calculate_unsaved_finding_hash_code(sanitized) + deduplicationLogger.debug(f"unsaved finding's hash_code: {sanitized.hash_code}") cleaned_findings.append(sanitized) + # Sort by the stable content key so intra-report duplicate resolution is + # independent of the scanner's export order. Unsaved findings have no id, so + # the id tiebreak is inert here and byte-identical findings keep their relative + # (immaterial) order via Python's stable sort. + cleaned_findings.sort(key=deduplication_ordering_key) + # Each entry carries the finding's own push_to_jira flag: grouped findings are pushed to # JIRA as a group, so their individual push is suppressed while ungrouped findings in the # same batch must still be pushed. The batch is partitioned by flag at dispatch time @@ -346,22 +372,7 @@ def _process_findings_internal( logger.debug(f"Processing reimport batch {batch_start}-{batch_end} of {len(cleaned_findings)} findings") - # Prepare findings in batch: set test, service, calculate hash codes - for unsaved_finding in unsaved_findings_batch: - # Some parsers provide "mitigated" field but do not set timezone (because they are probably not available in the report) - # Finding.mitigated is DateTimeField and it requires timezone - if unsaved_finding.mitigated and not unsaved_finding.mitigated.tzinfo: - unsaved_finding.mitigated = unsaved_finding.mitigated.replace(tzinfo=self.now.tzinfo) - # Override the test if needed - if not hasattr(unsaved_finding, "test"): - unsaved_finding.test = self.test - # Set the service supplied at import time - if self.service is not None: - unsaved_finding.service = self.service - self.location_handler.clean_unsaved(unsaved_finding) - # Calculate the hash code to be used to identify duplicates - unsaved_finding.hash_code = self.calculate_unsaved_finding_hash_code(unsaved_finding) - deduplicationLogger.debug(f"unsaved finding's hash_code: {unsaved_finding.hash_code}") + # Findings are already prepared (test/service/hash_code) and globally sorted above. # Fetch all candidates for this batch at once (batch candidate finding) candidates_by_hash, candidates_by_uid, candidates_by_key = self.get_reimport_match_candidates_for_batch( @@ -652,13 +663,13 @@ def match_finding_to_candidate_reimport( if candidates_by_hash is None or unsaved_finding.hash_code is None: return [] matches = candidates_by_hash.get(unsaved_finding.hash_code, []) - return sorted(matches, key=lambda f: f.id) + return sorted(matches, key=deduplication_ordering_key) if self.deduplication_algorithm == "unique_id_from_tool": if candidates_by_uid is None or unsaved_finding.unique_id_from_tool is None: return [] matches = candidates_by_uid.get(unsaved_finding.unique_id_from_tool, []) - return sorted(matches, key=lambda f: f.id) + return sorted(matches, key=deduplication_ordering_key) if self.deduplication_algorithm == "unique_id_from_tool_or_hash_code": if candidates_by_hash is None and candidates_by_uid is None: @@ -681,14 +692,14 @@ def match_finding_to_candidate_reimport( matches_by_id[match.id] = match matches = list(matches_by_id.values()) - return sorted(matches, key=lambda f: f.id) + return sorted(matches, key=deduplication_ordering_key) if self.deduplication_algorithm == "legacy": if candidates_by_key is None or not unsaved_finding.title: return [] key = (unsaved_finding.title.lower(), unsaved_finding.severity) matches = candidates_by_key.get(key, []) - return sorted(matches, key=lambda f: f.id) + return sorted(matches, key=deduplication_ordering_key) logger.error(f'Internal error: unexpected deduplication_algorithm: "{self.deduplication_algorithm}"') return [] diff --git a/unittests/scans/fortify/fortify_dedupe_uid_repro_scan1.fpr b/unittests/scans/fortify/fortify_dedupe_uid_repro_scan1.fpr new file mode 100644 index 00000000000..4b05f6e5844 Binary files /dev/null and b/unittests/scans/fortify/fortify_dedupe_uid_repro_scan1.fpr differ diff --git a/unittests/scans/fortify/fortify_dedupe_uid_repro_scan2.fpr b/unittests/scans/fortify/fortify_dedupe_uid_repro_scan2.fpr new file mode 100644 index 00000000000..033a5324bfc Binary files /dev/null and b/unittests/scans/fortify/fortify_dedupe_uid_repro_scan2.fpr differ diff --git a/unittests/test_deduplication_logic.py b/unittests/test_deduplication_logic.py index fd6b5d2847c..33ca9d97ed7 100644 --- a/unittests/test_deduplication_logic.py +++ b/unittests/test_deduplication_logic.py @@ -7,9 +7,16 @@ from crum import impersonate from django.conf import settings from django.core import serializers +from django.test import override_settings from django.utils import timezone -from dojo.finding.deduplication import build_candidate_scope_queryset, set_duplicate +from dojo.finding.deduplication import ( + build_candidate_scope_queryset, + dedupe_batch_of_findings, + deduplication_ordering_key, + get_finding_models_for_deduplication, + set_duplicate, +) from dojo.importers.default_importer import DefaultImporter from dojo.models import ( Development_Environment, @@ -18,10 +25,12 @@ Engagement, Finding, Product, + Product_Type, System_Settings, Test, Test_Import, Test_Import_Finding_Action, + Test_Type, User, copy_model_util, ) @@ -535,6 +544,148 @@ def test_identical_ordering_hash_code(self): # reset for further tests settings.DEDUPE_ALGO_ENDPOINT_FIELDS = dedupe_algo_endpoint_fields + def _make_finding_in_test(self, template_id, title, hash_code): + """ + Create a saved, active finding cloned from template_id with a forced hash_code. + + The hash_code is written directly to the DB (bypassing save()'s recompute) so we + can make two findings with different content collide on the deduplication key. + """ + new, _ = self.copy_and_reset_finding(find_id=template_id) + new.title = title + new.save(dedupe_option=False) + Finding.objects.filter(id=new.id).update(hash_code=hash_code) + new.refresh_from_db() + return new + + def test_dedupe_batch_winner_is_content_stable_hash_code(self): + # Regression for the Fortify import-order dedupe flip: among findings that + # collide on the deduplication key within one import, the surviving + # "original" must be chosen by a stable content key, NOT by whichever was + # created first (lowest DB id / parser emission order). + dedupe_algo_endpoint_fields = settings.DEDUPE_ALGO_ENDPOINT_FIELDS + settings.DEDUPE_ALGO_ENDPOINT_FIELDS = [] + try: + shared_hash = "content_stable_hash_flip_test" + # Created in REVERSE content order: "zzz" first (lower id), "aaa" second + # (higher id). The old behaviour would keep the lower-id "zzz" as original. + finding_z = self._make_finding_in_test(2, "zzz order flip finding", shared_hash) + finding_a = self._make_finding_in_test(2, "aaa order flip finding", shared_hash) + self.assertLess(finding_z.id, finding_a.id) + self.assertEqual(finding_z.test_id, finding_a.test_id) + + batch = get_finding_models_for_deduplication([finding_z.id, finding_a.id]) + dedupe_batch_of_findings(batch) + + finding_a.refresh_from_db() + finding_z.refresh_from_db() + # "aaa" is the canonical original even though it has the HIGHER id. + self.assert_finding(finding_a, duplicate=False) + self.assert_finding(finding_z, duplicate=True, duplicate_finding_id=finding_a.id) + finally: + settings.DEDUPE_ALGO_ENDPOINT_FIELDS = dedupe_algo_endpoint_fields + + def test_dedupe_batch_pre_existing_finding_stays_original(self): + # Backward-compat guard: a finding that already existed before this import + # batch must remain the original regardless of the new finding's content key. + dedupe_algo_endpoint_fields = settings.DEDUPE_ALGO_ENDPOINT_FIELDS + settings.DEDUPE_ALGO_ENDPOINT_FIELDS = [] + try: + shared_hash = "content_stable_hash_preexisting_test" + # Pre-existing finding has a content key that sorts AFTER the new one. + pre_existing = self._make_finding_in_test(2, "zzz pre-existing", shared_hash) + # Dedupe the pre-existing finding alone first (simulates an earlier import). + dedupe_batch_of_findings(get_finding_models_for_deduplication([pre_existing.id])) + + new_finding = self._make_finding_in_test(2, "aaa newer import", shared_hash) + self.assertLess(pre_existing.id, new_finding.id) + + # New batch contains only the new finding; pre-existing is a DB candidate. + dedupe_batch_of_findings(get_finding_models_for_deduplication([new_finding.id])) + + pre_existing.refresh_from_db() + new_finding.refresh_from_db() + # Pre-existing stays original even though "aaa" would sort before "zzz". + self.assert_finding(pre_existing, duplicate=False) + self.assert_finding(new_finding, duplicate=True, duplicate_finding_id=pre_existing.id) + finally: + settings.DEDUPE_ALGO_ENDPOINT_FIELDS = dedupe_algo_endpoint_fields + + def test_deduplication_ordering_key_is_order_independent(self): + # The ordering key must be a pure function of finding content (+ id tiebreak), + # so sorting a set of findings yields the same result regardless of input order. + f_a = Finding.objects.get(id=2) + f_b = Finding.objects.get(id=3) + forward = sorted([f_a, f_b], key=deduplication_ordering_key) + backward = sorted([f_b, f_a], key=deduplication_ordering_key) + self.assertEqual([f.id for f in forward], [f.id for f in backward]) + + def _dedupe_fortify_repro_scan(self, fpr_filename, engagement_name): + """ + Import one corrected Fortify repro .fpr into its own (isolated) engagement + under a shared product, using unique_id_from_tool deduplication, and return + the single surviving non-duplicate finding. + + The two fixtures contain the same pair of findings - which share a + unique_id_from_tool but have different content (different rule/title/line) - + in opposite document order. + """ + from dojo.tools.fortify.parser import FortifyParser # noqa: PLC0415 + + product_type, _ = Product_Type.objects.get_or_create(name="Fortify UID Repro PT") + product, _ = Product.objects.get_or_create( + name="Fortify UID Repro Product", + defaults={"description": "Fortify UID dedupe repro", "prod_type": product_type}, + ) + test_type, _ = Test_Type.objects.get_or_create(name="Fortify Scan") + engagement = Engagement.objects.create( + name=engagement_name, + product=product, + target_start=timezone.now().date(), + target_end=timezone.now().date(), + deduplication_on_engagement=True, # isolate each scan's dedupe scope + ) + test = Test.objects.create( + engagement=engagement, + test_type=test_type, + scan_type="Fortify Scan", + target_start=timezone.now(), + target_end=timezone.now(), + ) + with (get_unit_tests_scans_path("fortify") / fpr_filename).open(encoding="utf-8") as testfile: + parsed_findings = FortifyParser().get_findings(testfile, test) + + admin = User.objects.get(username="admin") + saved = [] + for parsed in parsed_findings: + parsed.test = test + parsed.reporter = admin + parsed.save(dedupe_option=False) # defer dedupe to the batch call below + saved.append(parsed) + + dedupe_batch_of_findings(get_finding_models_for_deduplication([f.id for f in saved])) + + refreshed = [Finding.objects.get(id=f.id) for f in saved] + non_duplicates = [f for f in refreshed if not f.duplicate] + # unique_id_from_tool dedupe collapses the shared-uid pair to one original + self.assertEqual(len(non_duplicates), 1, f"expected exactly one original, got {[(f.id, f.line) for f in refreshed]}") + return non_duplicates[0] + + @override_settings(DEDUPLICATION_ALGORITHM_PER_PARSER={"Fortify Scan": settings.DEDUPE_ALGO_UNIQUE_ID_FROM_TOOL}) + def test_fortify_same_uid_different_content_original_is_order_independent(self): + # Corrected Fortify import-order repro. Two findings share unique_id_from_tool + # but differ in content, provided in opposite order across two .fpr files. + # Under unique_id_from_tool dedupe the surviving "original" must be the same + # (content-canonical) finding regardless of the scanner's export order. + # Pre-fix this flips (lowest-id / first-in-file wins); with the fix it is stable. + original_scan1 = self._dedupe_fortify_repro_scan("fortify_dedupe_uid_repro_scan1.fpr", "fortify-uid-repro-scan1") + original_scan2 = self._dedupe_fortify_repro_scan("fortify_dedupe_uid_repro_scan2.fpr", "fortify-uid-repro-scan2") + + self.assertEqual(original_scan1.unique_id_from_tool, original_scan2.unique_id_from_tool) + # The same content survives regardless of file order (line/title stable). + self.assertEqual(original_scan1.line, original_scan2.line) + self.assertEqual(original_scan1.title, original_scan2.title) + def test_identical_except_title_hash_code(self): # 4 is already a duplicate of 2, let's see what happens if we create an identical finding with different title (and reset status) # expect: NOT marked as duplicate as title is part of hash_code calculation