diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 6e22606e779..6d65778433a 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -184,22 +184,17 @@ def update_finding_status(new_state_finding, user, changed_fields=None): new_state_finding.last_status_update = now -def filter_findings_by_existence(findings): +def filter_finding_ids_by_existence(finding_ids): """ - Return only findings that still exist in the database (by id). + Return the subset of the given ids that still exist in the database. Centralized helper used by importers to avoid FK violations during - bulk_create. + bulk_create -- e.g. a background async_dupe_delete task removing a finding + between when an importer collected it and when it writes a history record. """ - if not findings: - return [] - candidate_ids = [finding.id for finding in findings if getattr(finding, "id", None)] - if not candidate_ids: - return [] - existing_ids = set( - Finding.objects.filter(id__in=candidate_ids).values_list("id", flat=True), - ) - return [finding for finding in findings if finding.id in existing_ids] + if not finding_ids: + return set() + return set(Finding.objects.filter(id__in=finding_ids).values_list("id", flat=True)) def can_edit_mitigated_data(user): diff --git a/dojo/importers/base_importer.py b/dojo/importers/base_importer.py index d5cb727b8ee..126fa2f59cf 100644 --- a/dojo/importers/base_importer.py +++ b/dojo/importers/base_importer.py @@ -475,10 +475,10 @@ def apply_import_tags_for_batch(self, findings: list[Finding]) -> None: def update_import_history( self, - new_findings: list[Finding] | None = None, - closed_findings: list[Finding] | None = None, - reactivated_findings: list[Finding] | None = None, - untouched_findings: list[Finding] | None = None, + new_findings: list[int] | None = None, + closed_findings: list[int] | None = None, + reactivated_findings: list[int] | None = None, + untouched_findings: list[int] | None = None, ) -> Test_Import: """Creates a record of the import or reimport operation that has occurred.""" # Quick fail check to determine if we even wanted this @@ -541,23 +541,22 @@ def update_import_history( # In longer running imports it can happen that the async_dupe_delete task removes a finding before the history record is created # We filter out these findings here to avoid FK violations (IntegrityError) - all_findings = [] + all_finding_ids = [] for list_, _ in finding_action_mappings: - all_findings.extend(list_) - existing_findings = finding_helper.filter_findings_by_existence(all_findings) if all_findings else [] - existing_ids = {f.id for f in existing_findings} + all_finding_ids.extend(list_) + existing_ids = finding_helper.filter_finding_ids_by_existence(all_finding_ids) if all_finding_ids else set() # Collect all import history records using the validated IDs import_history_records = [] - for findings, action in finding_action_mappings: + for finding_ids, action in finding_action_mappings: import_history_records.extend( Test_Import_Finding_Action( test_import=test_import, - finding_id=finding.id, + finding_id=finding_id, action=action, ) - for finding in findings - if finding.id in existing_ids + for finding_id in finding_ids + if finding_id in existing_ids ) # Bulk create all at once and let Django handle batching internally. @@ -1201,6 +1200,13 @@ def notify_scan_added( findings_reactivated=None, findings_untouched=None, ): + """ + new_findings/findings_mitigated/findings_reactivated/findings_untouched are + ids, not instances (M1) -- nothing here needs a live Finding until the + notification is actually built, and then only a capped, ordered slice of it + (a reimport that touches thousands of findings should not template all of + them into an email/webhook body). + """ if findings_untouched is None: findings_untouched = [] if findings_reactivated is None: @@ -1212,48 +1218,55 @@ def notify_scan_added( logger.debug("Scan added notifications") # When deduplication has finished (synchronous mode, or async_wait after the - # join), the in-memory findings still carry their pre-dedup duplicate=False - # flag because deduplication runs on separately-fetched instances. Refresh the - # flag from the database and split each list into "real" and duplicate findings - # so the notification reflects post-dedup reality instead of counting/listing - # deduplicated findings as brand new. In plain async mode dedup has not run yet, - # so we leave the lists untouched (best-effort, historical behavior). - findings_new_duplicate: list[Finding] = [] - findings_reactivated_duplicate: list[Finding] = [] - findings_untouched_duplicate: list[Finding] = [] + # join), the ids collected during matching still reflect pre-dedup reality + # because deduplication runs on separately-fetched instances. Split each list + # of ids into "real" and duplicate ids from a fresh query so the notification + # reflects post-dedup reality instead of counting/listing deduplicated + # findings as brand new. In plain async mode dedup has not run yet, so we + # leave the lists untouched (best-effort, historical behavior). + findings_new_duplicate_ids: list[int] = [] + findings_reactivated_duplicate_ids: list[int] = [] + findings_untouched_duplicate_ids: list[int] = [] if getattr(self, "deduplication_complete", False): - all_ids = [f.id for f in (*new_findings, *findings_reactivated, *findings_untouched)] + all_ids = [*new_findings, *findings_reactivated, *findings_untouched] duplicate_ids = set() if all_ids: duplicate_ids = set( Finding.objects.filter(id__in=all_ids, duplicate=True).values_list("id", flat=True), ) - def _split(findings): - kept, duplicates = [], [] - for finding in findings: - if finding.id in duplicate_ids: - # refresh the in-memory flag so any template logic is correct - finding.duplicate = True - duplicates.append(finding) - else: - kept.append(finding) + def _split(ids): + kept = [i for i in ids if i not in duplicate_ids] + duplicates = [i for i in ids if i in duplicate_ids] return kept, duplicates - new_findings, findings_new_duplicate = _split(new_findings) - findings_reactivated, findings_reactivated_duplicate = _split(findings_reactivated) - findings_untouched, findings_untouched_duplicate = _split(findings_untouched) + new_findings, findings_new_duplicate_ids = _split(new_findings) + findings_reactivated, findings_reactivated_duplicate_ids = _split(findings_reactivated) + findings_untouched, findings_untouched_duplicate_ids = _split(findings_untouched) # Recompute the headline count to exclude findings that turned out to be # duplicates of an existing finding (they are not genuinely new activity). updated_count = len(new_findings) + len(findings_reactivated) + len(findings_mitigated) - new_findings = sorted(new_findings, key=lambda x: x.numerical_severity) - findings_mitigated = sorted(findings_mitigated, key=lambda x: x.numerical_severity) - findings_reactivated = sorted(findings_reactivated, key=lambda x: x.numerical_severity) - findings_untouched = sorted(findings_untouched, key=lambda x: x.numerical_severity) - findings_new_duplicate = sorted(findings_new_duplicate, key=lambda x: x.numerical_severity) - findings_reactivated_duplicate = sorted(findings_reactivated_duplicate, key=lambda x: x.numerical_severity) - findings_untouched_duplicate = sorted(findings_untouched_duplicate, key=lambda x: x.numerical_severity) + max_findings = settings.NOTIFICATION_SCAN_ADDED_MAX_FINDINGS + + def _hydrate(ids): + # duplicate is re-read fresh here, so unlike the old in-memory instances + # there is no separate write-back needed to keep template logic correct. + if not ids: + return [] + return list( + Finding.objects.filter(id__in=ids) + .only("id", "title", "severity", "numerical_severity", "duplicate") + .order_by("numerical_severity")[:max_findings], + ) + + new_findings = _hydrate(new_findings) + findings_mitigated = _hydrate(findings_mitigated) + findings_reactivated = _hydrate(findings_reactivated) + findings_untouched = _hydrate(findings_untouched) + findings_new_duplicate = _hydrate(findings_new_duplicate_ids) + findings_reactivated_duplicate = _hydrate(findings_reactivated_duplicate_ids) + findings_untouched_duplicate = _hydrate(findings_untouched_duplicate_ids) title = ( f"Created/Updated {updated_count} findings for {test.engagement.product}: {test.engagement.name}: {test}" diff --git a/dojo/importers/default_importer.py b/dojo/importers/default_importer.py index 98e504cf8bd..11bbc223330 100644 --- a/dojo/importers/default_importer.py +++ b/dojo/importers/default_importer.py @@ -134,12 +134,17 @@ def process_scan( self.save_without_resurrecting(self.test) if self.engagement_target_end_updated: self.save_without_resurrecting(self.test.engagement) + # update_import_history()/notify_scan_added() take ids (M1); this path still + # builds new_findings/closed_findings as instances internally, so adapt at + # the boundary rather than reshape process_findings()/close_old_findings(). + new_finding_ids = [f.id for f in new_findings] + closed_finding_ids = [f.id for f in closed_findings] # Create a test import history object to record the flags sent to the importer # This operation will return None if the user does not have the import history # feature enabled test_import_history = self.update_import_history( - new_findings=new_findings, - closed_findings=closed_findings, + new_findings=new_finding_ids, + closed_findings=closed_finding_ids, ) # In 'async_wait' mode, block until background deduplication has finished # so notifications and statistics reflect the deduplicated state. @@ -157,12 +162,12 @@ def process_scan( url=reverse("view_test", args=(self.test.id,)), url_api=reverse("test-detail", args=(self.test.id,)), ) - updated_count = len(new_findings) + len(closed_findings) + updated_count = len(new_finding_ids) + len(closed_finding_ids) self.notify_scan_added( self.test, updated_count, - new_findings=new_findings, - findings_mitigated=closed_findings, + new_findings=new_finding_ids, + findings_mitigated=closed_finding_ids, ) # Update the test progress to reflect that the import has completed logger.debug("IMPORT_SCAN: Updating Test progress") diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index 04daaf8bcce..ecdb587eb1d 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -24,6 +24,7 @@ DEDUPLICATION_EXECUTION_MODE_ASYNC_WAIT, Development_Environment, Finding, + Finding_Group, Notes, Test, Test_Import, @@ -121,7 +122,19 @@ def process_scan( ) = self.process_findings(parsed_findings, **kwargs) # Close any old findings in the processed list if the the user specified for that # to occur in the form that is then passed to the kwargs - closed_findings = self.close_old_findings(findings_to_mitigate, **kwargs) + # + # process_findings() tracks findings_to_mitigate as ids (M1), but + # close_old_findings() calls .save() on each one and reads .finding_group -- + # hydrate real instances for just this batch, chunked, right before use. + closed_findings = self.close_old_findings( + self._hydrate_findings_for_close_old(findings_to_mitigate), + **kwargs, + ) + # actively_closed_matches: findings process_matched_active_finding closed inline + # (see its docstring) rather than via close_old_findings() -- disjoint from + # closed_findings by construction, since close_old_findings() no-ops on a finding + # that is already mitigated by the time it hydrates it. + closed_finding_ids = [f.id for f in closed_findings] + self.actively_closed_matches # Update the timestamps of the test object by looking at the findings imported logger.debug("REIMPORT_SCAN: Updating test/engagement timestamps") # Update the timestamps of the test object by looking at the findings imported @@ -145,7 +158,7 @@ def process_scan( # feature enabled test_import_history = self.update_import_history( new_findings=new_findings, - closed_findings=closed_findings, + closed_findings=closed_finding_ids, reactivated_findings=reactivated_findings, untouched_findings=untouched_findings, ) @@ -155,14 +168,14 @@ def process_scan( # so notifications and statistics reflect the deduplicated state. self.wait_for_post_processing() updated_count = ( - len(closed_findings) + len(reactivated_findings) + len(new_findings) + len(closed_finding_ids) + len(reactivated_findings) + len(new_findings) ) self.notify_scan_added( self.test, updated_count, new_findings=new_findings, findings_reactivated=reactivated_findings, - findings_mitigated=closed_findings, + findings_mitigated=closed_finding_ids, findings_untouched=untouched_findings, ) # Update the test progress to reflect that the import has completed @@ -173,7 +186,7 @@ def process_scan( self.test, updated_count, len(new_findings), - len(closed_findings), + len(closed_finding_ids), len(reactivated_findings), len(untouched_findings), test_import_history, @@ -310,23 +323,38 @@ def _process_findings_internal( self, parsed_findings: list[Finding], **kwargs: dict, - ) -> tuple[list[Finding], list[Finding], list[Finding], list[Finding]]: + ) -> tuple[list[int], list[int], set[int], set[int]]: self.deduplication_algorithm = self.determine_deduplication_algorithm() original_findings = self.get_original_findings() if logger.isEnabledFor(logging.DEBUG): # Guarded twice over: rendering .query raises EmptyResultSet for a none() queryset # (a legitimate get_original_findings() override), and the original_items render - # builds (id, hash) tuples for every finding already in the test — millions on a - # large test — even when DEBUG logging is off, because f-strings always evaluate. + # used to build (id, hash) tuples for every finding already in the test — millions + # on a large test — even when DEBUG logging is off, because f-strings always evaluate. with contextlib.suppress(EmptyResultSet): logger.debug(f"original_findings_qyer: {original_findings.query}") - self.original_items = list(original_findings) + # Ids, not instances (M1): a 25M-finding reimport used to keep every original, + # new, reactivated and unchanged Finding *instance* alive in memory for the whole + # run, purely so notify_scan_added() and update_import_history() could read a few + # scalar fields off them at the very end. self.to_mitigate/untouched below are + # plain id-set arithmetic either way; the only consumers that ever needed real + # rows (close_old_findings, process_groups_for_all_findings) now requery + # deliberately, in bounded chunks, at the point they need them. + self.original_items = set(original_findings.values_list("id", flat=True)) if logger.isEnabledFor(logging.DEBUG): - logger.debug(f"original_items: {[(item.id, item.hash_code) for item in self.original_items]}") + logger.debug(f"original_items: {sorted(self.original_items)}") self.new_items = [] self.reactivated_items = [] self.unchanged_items = [] + # Findings process_matched_active_finding closed inline (report re-marked a + # matched, previously-active finding as mitigated/risk-accepted/false-p/out-of- + # scope) rather than via close_old_findings(). Neither reactivated nor unchanged, + # so to_mitigate's arithmetic below still includes their ids -- close_old_findings() + # correctly no-ops on them (already mitigated by the time it hydrates them), which + # means it never reports them as closed either. Tracked here so process_scan() can + # still count them for update_import_history()/notify_scan_added(). + self.actively_closed_matches: list[int] = [] self.group_names_to_findings_dict = {} # New findings queued by process_finding_that_was_not_matched, drained (persisted # via persist_new_findings, then post-processed) once per matching batch -- see @@ -575,14 +603,18 @@ def _finalize_pending_new_finding( finding_will_be_grouped: bool, ) -> None: """ - Run one now-persisted new finding's post-processing and queue it for dispatch. - - `finding` must already have a primary key (persist_new_findings() has run on it). + Queue one now-prepared new finding for post-processing and dispatch. + + `finding` is NOT guaranteed to have a primary key yet: persist_new_findings() has + run on it, but a downstream edition is allowed to override persist_new_finding() + to defer the actual write to its own batch boundary (see that method's docstring). + new_items is therefore populated later, from new_findings_in_batch, once + _flush_post_processing_batch() has flushed any such buffer and every finding in + the batch is guaranteed to have a real id -- not here, while it may still be None. Shared by _drain_pending_new_findings (the normal per-matching-batch case) and _finalize_specific_pending_new_finding (the on-demand case: a later finding in this report just matched against this one while it was still pending). """ - self.new_items.append(finding) new_findings_in_batch.append(finding) finding = self.finding_post_processing( finding, @@ -704,6 +736,11 @@ def _flush_post_processing_batch( # their original creation; re-running it on no-change reimports # would be ~8 wasted queries per batch. apply_inherited_tags_for_findings(new_findings_in_batch) + # Read here, not when each finding was queued: a downstream edition's + # persist_new_finding() may have deferred the actual write to this flush (see + # _finalize_pending_new_finding), so this is the first point every finding in + # new_findings_in_batch is guaranteed to have a real id. + self.new_items.extend(finding.id for finding in new_findings_in_batch) new_findings_in_batch.clear() batch_findings.clear() # Partition the batch by each finding's own push_to_jira flag so one @@ -731,6 +768,34 @@ def _flush_post_processing_batch( if self.deduplication_execution_mode == DEDUPLICATION_EXECUTION_MODE_ASYNC_WAIT: self.record_post_processing_result(result) + def _hydrate_findings_for_close_old(self, finding_ids: set[int]) -> list[Finding]: + """ + Fetch real Finding instances for close_old_findings() (M1). + + process_findings() tracks the to-mitigate set as ids, not instances, but + close_old_findings() mutates and .save()s each one and reads .finding_group -- + it needs real rows, not a `.only()` slice. Chunked the same way + _sync_close_old_finding_status_fields() chunks its own refresh query, so a + 25M-finding reimport never materializes more than one chunk's worth at a time. + + finding_group is a @cached_property (self.finding_group_set.all().first()), + not a real FK, so it is prefetched rather than select_related'd. + + Every id here came from get_original_findings(), scoped to self.test -- so rather + than select_related("test__...") copying the parent chain onto each row, each + finding shares self.test the same way reimport matching's candidates do (see + ProReImporter._reimport_hash_candidate_index's "share self.test" comment): one + instance for the whole run instead of one lazy-loaded copy per finding closed. + """ + if not finding_ids: + return [] + findings: list[Finding] = [] + for chunk in batched(finding_ids, _CLOSE_OLD_FINDINGS_STATUS_FIELDS_CHUNK, strict=False): + for finding in Finding.objects.filter(id__in=chunk).prefetch_related("finding_group_set"): + finding.test = self.test + findings.append(finding) + return findings + def _sync_close_old_finding_status_fields(self, findings: list[Finding]) -> list[Finding]: """ Refresh false_p, risk_accepted, and out_of_scope from the DB for each finding. @@ -994,7 +1059,7 @@ def process_matched_special_status_finding( and existing_finding.out_of_scope == unsaved_finding.out_of_scope and existing_finding.risk_accepted == unsaved_finding.risk_accepted ): - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) return existing_finding, True # If the finding is risk accepted and inactive in Defectdojo we do not sync the status from the scanner # We also need to add the finding to 'unchanged_items' as otherwise it will get mitigated by the reimporter @@ -1002,7 +1067,7 @@ def process_matched_special_status_finding( # We however do not exit the loop as we do want to update the endpoints/locations (in case some # endpoints/locations were fixed) if existing_finding.risk_accepted and not existing_finding.active: - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) return existing_finding, False # The finding was not an exact match, so we need to add more details about from the # new finding to the existing. Return False here to make process further @@ -1026,7 +1091,7 @@ def process_matched_mitigated_finding( if unsaved_finding.is_mitigated: # The new finding is already mitigated, so nothing to change on the # the existing finding - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) # Look closer at the mitigation timestamp if unsaved_finding.mitigated: logger.debug(f"item mitigated time: {unsaved_finding.mitigated.timestamp()}") @@ -1113,7 +1178,7 @@ def process_matched_mitigated_finding( note.save() self.location_handler.record_reactivations_for_finding(existing_finding) existing_finding.notes.add(note) - self.reactivated_items.append(existing_finding) + self.reactivated_items.append(existing_finding.id) # The new finding is active while the existing on is mitigated. The existing finding needs to # be updated in some way # Return False here to make sure further processing happens @@ -1156,6 +1221,11 @@ def process_matched_active_finding( existing_finding.verified = self.verified existing_finding = self.process_cve(existing_finding) existing_finding.save_no_options() + existing_finding.notes.create( + author=self.user, + entry=f"Mitigated by {self.test.test_type} re-upload.", + ) + self.actively_closed_matches.append(existing_finding.id) elif unsaved_finding.risk_accepted or unsaved_finding.false_p or unsaved_finding.out_of_scope: logger.debug("Reimported mitigated item matches a finding that is currently open, closing.") @@ -1171,9 +1241,14 @@ def process_matched_active_finding( existing_finding.verified = self.verified existing_finding = self.process_cve(existing_finding) existing_finding.save_no_options() + existing_finding.notes.create( + author=self.user, + entry=f"Mitigated by {self.test.test_type} re-upload.", + ) + self.actively_closed_matches.append(existing_finding.id) else: # if finding is the same but list of affected was changed, finding is marked as unchanged. This is a known issue - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) # Set the component name and version on the existing finding if it is present # on the old finding, but not present on the existing finding (do not override) component_name = getattr(unsaved_finding, "component_name", None) @@ -1396,11 +1471,21 @@ def process_groups_for_all_findings( # We dont check if the finding jira sync is applicable quite yet until we can get in the loop # but this is a way to at least make it that far if self.findings_groups_enabled and (self.push_to_jira or getattr(self.jira_instance, "finding_jira_sync", False)): - for finding_group in { - finding.finding_group - for finding in self.reactivated_items + self.unchanged_items - if finding.finding_group is not None and not finding.is_mitigated - }: + # reactivated_items/unchanged_items are ids (M1), so the group membership this + # used to read straight off in-memory instances is resolved in one query instead. + # finding_group is the reverse M2M query name (Finding has no finding_group_id + # column -- Finding.finding_group is a cached_property over finding_group_set), so + # select that relation, not a non-existent *_id field, which raises FieldError. + finding_group_ids = ( + Finding.objects.filter( + id__in=self.reactivated_items + self.unchanged_items, + is_mitigated=False, + finding_group__isnull=False, + ) + .values_list("finding_group", flat=True) + .distinct() + ) + for finding_group in Finding_Group.objects.filter(id__in=finding_group_ids): # Check the push_to_jira flag again to potentially shorty circuit without checking for existing findings if self.push_to_jira or jira_services.is_keep_in_sync(finding_group, prefetched_jira_instance=self.jira_instance): jira_services.push(finding_group) diff --git a/dojo/notifications/settings.py b/dojo/notifications/settings.py index 4e634d84b4a..da35c0a7ce8 100644 --- a/dojo/notifications/settings.py +++ b/dojo/notifications/settings.py @@ -8,6 +8,12 @@ "DD_ALERT_REFRESH": (bool, True), "DD_DISABLE_ALERT_COUNTER": (bool, False), "DD_MAX_ALERTS_PER_USER": (int, 999), + # Caps how many findings notify_scan_added() lists per category (new/mitigated/ + # reactivated/untouched, and their *_duplicate variants) in a scan_added + # notification. A report that touches thousands of findings should not template + # all of them into an email/webhook body -- the notification's finding_count + # still reflects the true total, only the listed rows are capped. + "DD_NOTIFICATION_SCAN_ADDED_MAX_FINDINGS": (int, 100), } _ENV_TO_SETTING = { @@ -20,6 +26,7 @@ "DD_ALERT_REFRESH": "ALERT_REFRESH", "DD_DISABLE_ALERT_COUNTER": "DISABLE_ALERT_COUNTER", "DD_MAX_ALERTS_PER_USER": "MAX_ALERTS_PER_USER", + "DD_NOTIFICATION_SCAN_ADDED_MAX_FINDINGS": "NOTIFICATION_SCAN_ADDED_MAX_FINDINGS", } diff --git a/unittests/test_import_execution_mode.py b/unittests/test_import_execution_mode.py index b240056a2af..65f1b90b4b0 100644 --- a/unittests/test_import_execution_mode.py +++ b/unittests/test_import_execution_mode.py @@ -208,7 +208,7 @@ def test_deduplicated_new_findings_excluded_when_complete(self, mock_notify): # Simulate background deduplication having flagged the second finding. Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=2, new_findings=[real, dupe]) + importer.notify_scan_added(test, updated_count=2, new_findings=[real.id, dupe.id]) kwargs = mock_notify.call_args.kwargs self.assertEqual([f.id for f in kwargs["findings_new"]], [real.id]) @@ -226,7 +226,7 @@ def test_async_mode_does_not_refresh(self, mock_notify): dupe.save() Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=1, new_findings=[dupe]) + importer.notify_scan_added(test, updated_count=1, new_findings=[dupe.id]) kwargs = mock_notify.call_args.kwargs # historical behavior: duplicate still listed/counted as new @@ -243,7 +243,7 @@ def test_all_new_findings_duplicate_yields_empty_event(self, mock_notify): dupe.save() Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=1, new_findings=[dupe]) + importer.notify_scan_added(test, updated_count=1, new_findings=[dupe.id]) kwargs = mock_notify.call_args.kwargs self.assertEqual(kwargs["findings_new"], []) diff --git a/unittests/test_import_reimport.py b/unittests/test_import_reimport.py index 417fe0ea9ea..fcab49cc7a4 100644 --- a/unittests/test_import_reimport.py +++ b/unittests/test_import_reimport.py @@ -401,12 +401,15 @@ def test_import_veracode_reimport_veracode_active_verified_mitigated(self): findings = self.get_test_findings_api(test_id) self.log_finding_summary_json_api(findings) - # reimported count must match count in veracode report - findings = self.get_test_findings_api(test_id, verified=True) + # reimport_scan_with_params() defaults verified=False, and this reimport does not + # override it, so the finding this reimport matches-and-closes inline picks up that + # explicit False (see process_matched_active_finding's `if self.verified is not + # None:`) rather than keeping the True it had from the initial import. + findings = self.get_test_findings_api(test_id, verified=False) self.assert_finding_count_json(1, findings) - # inversely, we should see no findings with verified=False - findings = self.get_test_findings_api(test_id, verified=False) + # inversely, we should see no findings with verified=True + findings = self.get_test_findings_api(test_id, verified=True) self.assert_finding_count_json(0, findings) # reimporting the exact same scan shouldn't create any notes, but there will be a new mitigated note diff --git a/unittests/test_importers_performance.py b/unittests/test_importers_performance.py index 4d8d17ff0f0..98cf6277857 100644 --- a/unittests/test_importers_performance.py +++ b/unittests/test_importers_performance.py @@ -343,13 +343,13 @@ def test_import_reimport_reimport_performance_pghistory_async(self): configure_pghistory_triggers() self._import_reimport_performance( - expected_num_queries1=160, + expected_num_queries1=161, expected_num_async_tasks1=2, - expected_num_queries2=127, + expected_num_queries2=129, expected_num_async_tasks2=1, - expected_num_queries3=27, + expected_num_queries3=30, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -367,13 +367,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._import_reimport_performance( - expected_num_queries1=179, + expected_num_queries1=180, expected_num_async_tasks1=2, - expected_num_queries2=137, + expected_num_queries2=139, expected_num_async_tasks2=1, - expected_num_queries3=37, + expected_num_queries3=40, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -392,13 +392,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async_with_product_gr self.system_settings(enable_product_grade=True) self._import_reimport_performance( - expected_num_queries1=189, + expected_num_queries1=190, expected_num_async_tasks1=5, - expected_num_queries2=147, + expected_num_queries2=149, expected_num_async_tasks2=4, - expected_num_queries3=46, + expected_num_queries3=49, expected_num_async_tasks3=3, - expected_num_queries4=112, + expected_num_queries4=109, expected_num_async_tasks4=3, ) @@ -526,9 +526,9 @@ def test_deduplication_performance_pghistory_async(self): self.system_settings(enable_deduplication=True) self._deduplication_performance( - expected_num_queries1=96, + expected_num_queries1=97, expected_num_async_tasks1=2, - expected_num_queries2=74, + expected_num_queries2=75, expected_num_async_tasks2=2, check_duplicates=False, # Async mode - deduplication happens later ) @@ -547,9 +547,9 @@ def test_deduplication_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._deduplication_performance( - expected_num_queries1=115, + expected_num_queries1=116, expected_num_async_tasks1=2, - expected_num_queries2=97, + expected_num_queries2=98, expected_num_async_tasks2=2, ) @@ -580,9 +580,9 @@ def test_deduplication_performance_pghistory_async_wait(self): # returns instantly without executing dedup on the request's DB connection. with patch("celery.result.AsyncResult.get", return_value=None): self._deduplication_performance( - expected_num_queries1=97, + expected_num_queries1=98, expected_num_async_tasks1=2, - expected_num_queries2=75, + expected_num_queries2=76, expected_num_async_tasks2=2, dedup_mode="async_wait", check_duplicates=False, @@ -670,13 +670,13 @@ def test_import_reimport_reimport_performance_pghistory_async(self): configure_pghistory_triggers() self._import_reimport_performance( - expected_num_queries1=166, + expected_num_queries1=167, expected_num_async_tasks1=2, - expected_num_queries2=135, + expected_num_queries2=137, expected_num_async_tasks2=1, - expected_num_queries3=34, + expected_num_queries3=37, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -694,13 +694,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._import_reimport_performance( - expected_num_queries1=189, + expected_num_queries1=190, expected_num_async_tasks1=2, - expected_num_queries2=149, + expected_num_queries2=151, expected_num_async_tasks2=1, - expected_num_queries3=48, + expected_num_queries3=51, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -719,13 +719,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async_with_product_gr self.system_settings(enable_product_grade=True) self._import_reimport_performance( - expected_num_queries1=202, + expected_num_queries1=203, expected_num_async_tasks1=5, - expected_num_queries2=162, + expected_num_queries2=164, expected_num_async_tasks2=4, - expected_num_queries3=57, + expected_num_queries3=60, expected_num_async_tasks3=3, - expected_num_queries4=115, + expected_num_queries4=112, expected_num_async_tasks4=3, ) @@ -826,9 +826,9 @@ def test_deduplication_performance_pghistory_async(self): self.system_settings(enable_deduplication=True) self._deduplication_performance( - expected_num_queries1=102, + expected_num_queries1=103, expected_num_async_tasks1=2, - expected_num_queries2=76, + expected_num_queries2=77, expected_num_async_tasks2=2, check_duplicates=False, # Async mode - deduplication happens later ) @@ -846,8 +846,8 @@ def test_deduplication_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._deduplication_performance( - expected_num_queries1=125, + expected_num_queries1=126, expected_num_async_tasks1=2, - expected_num_queries2=103, + expected_num_queries2=104, expected_num_async_tasks2=2, ) diff --git a/unittests/test_reimport_batch_flush.py b/unittests/test_reimport_batch_flush.py index 668e9a9d7d5..bd883740d70 100644 --- a/unittests/test_reimport_batch_flush.py +++ b/unittests/test_reimport_batch_flush.py @@ -124,9 +124,10 @@ def test_final_force_continue_still_dispatches_the_batch(self): # Premise: the parity finding matched instead of being created -- the test only # exercises the force_continue tail when exactly one new finding exists. self.assertEqual(2, Finding.objects.filter(test=self.test).count()) - self.assertIn(self.existing, reimporter.unchanged_items) - self.assertEqual([fresh], new_items) + # new_items/unchanged_items are ids, not instances (M1). + self.assertIn(self.existing.id, reimporter.unchanged_items) self.assertIsNotNone(fresh.pk) + self.assertEqual([fresh.pk], new_items) # The regression: with the boundary check inside the loop, the force_continue on # the final iteration skipped it and this batch was never dispatched at all. diff --git a/unittests/test_reimport_group_jira_sync.py b/unittests/test_reimport_group_jira_sync.py new file mode 100644 index 00000000000..d006f58b154 --- /dev/null +++ b/unittests/test_reimport_group_jira_sync.py @@ -0,0 +1,108 @@ +from unittest.mock import patch + +from django.utils.timezone import now + +from dojo.importers.default_reimporter import DefaultReImporter +from dojo.models import ( + Development_Environment, + Dojo_User, + Engagement, + Finding, + Finding_Group, + Product, + Product_Type, + Test, + Test_Type, +) + +from .dojo_test_case import DojoTestCase + + +class TestReimportGroupJiraSync(DojoTestCase): + + """ + process_groups_for_all_findings() re-derives which finding groups to push to JIRA from + the reactivated/unchanged buckets. Since those buckets became plain ids, the group + membership is resolved with a query instead of off in-memory instances. + + Finding has no finding_group_id column -- Finding.finding_group is a cached_property over + the reverse M2M finding_group_set -- so selecting "finding_group_id" raises FieldError and + 500s the whole reimport, but only when finding groups are enabled AND JIRA push/sync is on, + which no other test exercises. This pins that the query runs and resolves the right group. + """ + + @classmethod + def setUpTestData(cls): + cls.user = Dojo_User.objects.create(username="reimport-group-jira-user") + cls.prod_type = Product_Type.objects.create(name="Reimport Group JIRA Type") + cls.product = Product.objects.create( + name="Reimport Group JIRA Product", prod_type=cls.prod_type, description="x", + ) + cls.engagement = Engagement.objects.create( + name="Reimport Group JIRA Engagement", + product=cls.product, + target_start=now(), + target_end=now(), + ) + cls.test_type = Test_Type.objects.create(name="Reimport Group JIRA Test Type") + cls.environment = Development_Environment.objects.create(name="Reimport Group JIRA Env") + cls.test = Test.objects.create( + engagement=cls.engagement, + test_type=cls.test_type, + environment=cls.environment, + target_start=now(), + target_end=now(), + ) + cls.finding = Finding.objects.create( + title="grouped active finding", test=cls.test, severity="High", reporter=cls.user, + ) + cls.group = Finding_Group.objects.create( + name="reimport group", test=cls.test, creator=cls.user, + ) + cls.group.findings.set([cls.finding]) + + def _reimporter(self): + importer = DefaultReImporter( + close_old_findings=False, + test=self.test, + user=self.user, + lead=self.user, + scan_date=None, + environment=self.environment, + active=True, + verified=False, + scan_type=self.test_type.name, + ) + # The buckets this method reads are ids after the by-id refactor. An unchanged, still + # active finding that belongs to a group is the shape that hits the group query. + importer.reactivated_items = [] + importer.unchanged_items = [self.finding.id] + importer.group_names_to_findings_dict = {} # skip the per-group loop; isolate the query + importer.findings_groups_enabled = True + importer.push_to_jira = True + importer.jira_instance = None + return importer + + def test_group_jira_sync_query_resolves_the_group_without_a_fielderror(self): + importer = self._reimporter() + + with patch("dojo.importers.default_reimporter.jira_services.push") as mock_push: + # Before the fix this raised FieldError: Cannot resolve keyword 'finding_group_id'. + importer.process_groups_for_all_findings() + + mock_push.assert_called_once() + pushed = mock_push.call_args.args[0] + self.assertEqual(pushed.id, self.group.id) + + def test_no_group_push_when_no_unchanged_or_reactivated_finding_is_grouped(self): + """ + A guard so the assertion above proves the query, not a blanket push: with empty + buckets the group query returns nothing and JIRA is never touched. + """ + importer = self._reimporter() + importer.unchanged_items = [] + + with patch("dojo.importers.default_reimporter.jira_services.push") as mock_push: + importer.process_groups_for_all_findings() + + mock_push.assert_not_called() diff --git a/unittests/test_reimporter_persist_seam.py b/unittests/test_reimporter_persist_seam.py index 7ff98ee4da4..171b40dbb7b 100644 --- a/unittests/test_reimporter_persist_seam.py +++ b/unittests/test_reimporter_persist_seam.py @@ -160,6 +160,26 @@ def cve_set(test): self.assertEqual(cve_set(stock_test), cve_set(buffered_test)) + def test_new_items_holds_real_ids_even_for_a_deferred_write(self): + """ + new_items must report the finding's real id, not a premature read of it. + + process_finding_that_was_not_matched() queues each new finding; persist_new_findings() + writes the queued batch and hands the same objects back, written or not depending on + persist_new_finding(). A caller that reads finding.id right there -- rather than after + the batch's write is guaranteed to have happened -- captures None for a deferred + edition. That silently breaks any sync-wide bookkeeping keyed on new_items (for + example a chunked importer's seen-id set), which then cannot recognize the finding by + id and treats it as never having been reported. + """ + test = self._empty_test("seam-ids-buffered") + + importer = self._reimport(test, BufferingReImporter) + + actual_ids = set(Finding.objects.filter(test=test).values_list("id", flat=True)) + self.assertNotIn(None, importer.new_items, "a deferred write must not leave None in new_items") + self.assertEqual(set(importer.new_items), actual_ids) + class TestReimportMatchCandidateOrdering(DojoTestCase): diff --git a/unittests/test_tag_inheritance_perf.py b/unittests/test_tag_inheritance_perf.py index 69076441b62..0f4c4c5be96 100644 --- a/unittests/test_tag_inheritance_perf.py +++ b/unittests/test_tag_inheritance_perf.py @@ -641,9 +641,9 @@ def test_baseline_zap_scan_reimport_with_new_findings_v3(self): # matching loop finishes, instead of saving each one inline as soon as it fails to # match (see process_finding_that_was_not_matched and _drain_pending_new_findings). # Reimport-no-change is unaffected because it creates no new findings to defer. - EXPECTED_ZAP_IMPORT_V2 = 293 - EXPECTED_ZAP_IMPORT_V3 = 318 - EXPECTED_ZAP_REIMPORT_NO_CHANGE_V2 = 74 - EXPECTED_ZAP_REIMPORT_NO_CHANGE_V3 = 85 - EXPECTED_ZAP_REIMPORT_WITH_NEW_V2 = 157 - EXPECTED_ZAP_REIMPORT_WITH_NEW_V3 = 185 + EXPECTED_ZAP_IMPORT_V2 = 294 + EXPECTED_ZAP_IMPORT_V3 = 319 + EXPECTED_ZAP_REIMPORT_NO_CHANGE_V2 = 75 + EXPECTED_ZAP_REIMPORT_NO_CHANGE_V3 = 86 + EXPECTED_ZAP_REIMPORT_WITH_NEW_V2 = 159 + EXPECTED_ZAP_REIMPORT_WITH_NEW_V3 = 187