fix(finding): run Finding.delete() for each finding the merge action deletes

[svader0]

Description

The "delete" option on Merge Findings called .delete() on the queryset. That skips Finding.delete(), so the merge skipped the duplicate-cluster handling, the JIRA sync, the ordered tag removal and the product grading. If a deleted finding was the original of a duplicate cluster, its duplicates kept pointing at a row that no longer existed.

This PR deletes each finding with Finding.delete(), through delete_finding_with_conflict_retry (the same helper the single delete view uses). It passes push_to_jira=None, so JIRA follows the default delete policy.

If the merge target is a duplicate of a finding that the merge deletes, the target is detached from that cluster first. Without this, DD_DUPLICATE_CLUSTER_CASCADE_DELETE=True deletes the target together with the cluster.

Behavior change for users who can already delete: a merge with "delete" now re-grades the product. If the JIRA project keeps issues in sync or pushes all issues, the merge also closes or reassigns the JIRA issue of each deleted finding.

Test results

• New test test_merge_delete_promotes_a_new_original_for_the_duplicate_cluster in unittests/test_bulk_finding_authorization.py. It makes the deleted finding the original of a duplicate, then checks that the duplicate becomes the new original. It fails before the fix and passes after it.
• New test test_merge_delete_keeps_a_target_that_is_a_duplicate_of_a_deleted_finding. It turns on DUPLICATE_CLUSTER_CASCADE_DELETE, makes the merge target a duplicate of the finding being deleted, and checks that the target survives as an original. It fails before the fix and passes after it.
• unittests.test_bulk_finding_authorization, unittests.test_merge_findings_locations and unittests.test_finding_template_merge_endpoints_v3: 18 tests, all pass.
• ruff check . with ruff 0.16.10: all checks pass.

Documentation

No documentation change. The merge form and its options do not change.

Checklist

• Make sure to rebase your PR against the very latest dev.
• Submit all PRs, features and bug fixes alike, against the dev branch.
• Give a meaningful name to your PR, as it may end up being used in the release notes.
• Your code is Ruff compliant (see ruff.toml).
• Your code is python 3.13 compliant.
• If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
• Model changes must include the necessary migrations in the dojo/db_migrations folder.
• Add applicable tests to the unit tests.
• Add the proper label to categorize your PR.

[svader0]

[sc-15583]

[Maffooch]

Adversarial review: fix(finding): run Finding.delete() for each finding the merge action deletes

Verdict: request-changes
Reviewed at: 0b5ff99 Profile: code-change
Repo rules followed: origin/dev .claude/skills/review-pr/SKILL.md (plus the generic review-pr skill)

The fix is right for the default configuration. On dev today, merge with "delete" on a finding that is the original of a duplicate cluster leaves the duplicates pointing at a deleted row. The self-FK is DEFERRABLE INITIALLY DEFERRED (checked in the test DB with pg_constraint), so in production the request fails with an IntegrityError at commit. With this PR the cluster gets a new original and the request succeeds. One narrow regression remains under DD_DUPLICATE_CLUSTER_CASCADE_DELETE=True, described below.

Attempts to break it

1. Regression test on base. Reverse-applied the views.py hunk and ran TestMergeFindingDeleteRequiresStaff. The new test fails on base (persisted duplicate_finding_id=3) and passes on the PR head. No break.
2. Merge target is a duplicate of the finding being deleted (default settings). Made T a duplicate of D, then merged D into T with "delete". PR: T is promoted (duplicate=False), and a forced deferred-constraint check (SET CONSTRAINTS ALL IMMEDIATE, standing in for COMMIT) passes. Base: T keeps duplicate_finding_id pointing at the deleted D and the constraint check fails, which is a 500 in production. No break; the PR fixes this.
3. Same as 2, with DUPLICATE_CLUSTER_CASCADE_DELETE=True. PR: finding_delete cascades D's cluster (dojo/finding/helper.py:661), which deletes the merge target T. Then finding_to_merge_into.notes.add(note) (dojo/finding/ui/views.py:2384) writes a note link to the deleted T, and the constraint check fails on dojo_finding_notes_finding_id_..._fk. In production the cascade delete commits on its own (no ATOMIC_REQUESTS), so the user gets a 500 and loses the finding they chose to keep, with its notes and history. Base: also a 500, but the delete rolls back and T survives. Blocker (introduced by this PR; non-default setting).
4. Original and its duplicate both in "findings to merge", target unrelated. Both get deleted, status 302, deferred FK check passes. Same on base. No break.
5. JIRA sync claim. The PR description says the merge now "runs the normal JIRA sync". It does not. Finding.delete() defaults push_to_jira to the sentinel DELETE_JIRA_SYNC_UNSET from dojo/finding/models.py:52. finding_delete only syncs when push_to_jira is None or isinstance(push_to_jira, bool) (dojo/finding/helper.py:656), so jira_sync_requested is False. I patched _reassign_jira_issue_to_new_original in attempt 2 and it was not called. JIRA close and reassign are skipped, the same as base. No regression, but the description is wrong. Nit.
6. Query count at scale. Merge-delete of N findings into one, CaptureQueriesContext, dedup off: PR 554 queries at N=10 and 2674 at N=50 (about 53 per finding). Base: 204 and 804 (about 15 per finding). Linear, no N+1 beyond the per-finding delete, and the same per-finding loop the bulk delete route already uses (dojo/finding/ui/views.py:2453-2454). Merging 500 findings is roughly 26k queries in one request. Product grading is debounced by schedule_product_grade (dojo/utils.py:1130), so it does not queue one task per finding unless the user's profile forces foreground work. Follow-up, not a blocker.
7. Authorization / cross-product. Unchanged by this PR. The URL needs Product edit (dojo/authorization/url_permissions.py:115), candidate findings are scoped to the product (dojo/finding/ui/views.py:2256-2258), the form's findings_to_merge queryset only accepts those, and each finding to delete needs "delete" (dojo/finding/ui/views.py:2270-2274). Existing tests cover a plain member being refused. Deleting a cluster original does promote and rewrite duplicates in other products the user may not see (the PR's own test does exactly that). Every other delete path does the same. No break.
8. Atomicity. The loop runs without a transaction, and the target is saved (dojo/finding/ui/views.py:2360) before any delete. A failure part way through leaves some findings deleted, the target updated, and no merge note. On base, queryset.delete() was one atomic statement, but the target save was already outside it. Follow-up.
9. Inactive action unchanged. Merge with "inactive" still sets the merged finding inactive (302). No break.

Blocking

• dojo/finding/ui/views.py:2377-2378 With DD_DUPLICATE_CLUSTER_CASCADE_DELETE=True, if the merge target is a duplicate of a finding being deleted, Finding.delete() cascades and deletes the target. Then line 2384 raises an IntegrityError (500). The target's deletion is already committed by then. Suggested fix: before the loop, detach the target from any cluster it shares with the findings being deleted, for example Finding.objects.filter(pk=finding_to_merge_into.pk, duplicate_finding__in=findings_to_merge).update(duplicate=False, duplicate_finding=None). Or refuse that merge. Add a test with @override_settings(DUPLICATE_CLUSTER_CASCADE_DELETE=True).

Nice to have

• Fix the PR description: JIRA close and reassign do not run on this path (attempt 5). If they should, pass push_to_jira explicitly (None for the default policy), as the single and bulk delete views do.
• Consider delete_finding_with_conflict_retry (dojo/finding/helper.py) for each delete, as DeleteFinding does (dojo/finding/ui/views.py:1093), so a concurrent import does not turn the merge into a 500.
• Follow-up: wrap the merge in transaction.atomic(), or use bulk_delete_findings, to cut the roughly 53 queries per finding and avoid partial merges.

Verified hands-on

• PR head in a local stack (uwsgi + postgres + valkey), source bind-mounted: unittests.test_bulk_finding_authorization, unittests.test_merge_findings_locations and unittests.test_finding_template_merge_endpoints_v3 ran 17 tests, OK.
• Attempts 1 to 6 and 9 ran as a scratch test module through the merge view (Django test client), on the PR head and on the head with the views.py hunk reversed.
• ruff check (0.16.10) on both changed files: all checks passed. scripts/check_migration_leaves.py: single leaf.
• Deferred self-FK confirmed in Postgres: dojo_finding_duplicate_finding_id_9ff391c5_fk_dojo_finding_id ... DEFERRABLE INITIALLY DEFERRED.

Checked only statically

• Authorization paths (attempt 7) and atomicity (attempt 8).
• Commit-time behavior was emulated with SET CONSTRAINTS ALL IMMEDIATE inside the test transaction, not with real commits.
• Required CI on 0b5ff99: ruff-linting and Unit Tests Complete green, as are all other reported checks. The head is behind origin/dev (022e5f4); the merge queue will retest.

Still needs verification

• A real JIRA-linked finding through this path (no JIRA instance available).
• v3_feature_locations=true locally (CI ran both REST variants green).

[svader0]

Thanks for the pass. Fixed in 594be67:

• Blocker: the merge now takes the target out of any duplicate group it shares with the findings being deleted, before the deletes run. New test test_merge_delete_keeps_a_target_that_is_a_duplicate_of_a_deleted_finding covers it with DUPLICATE_CLUSTER_CASCADE_DELETE=True. It fails on the old head and passes now.
• JIRA: you were right, the sync did not run. The merge now passes push_to_jira=None, so it follows the default delete policy. I fixed the description to match.
• Each delete now goes through delete_finding_with_conflict_retry.
• Left as follow-ups: transaction.atomic() around the merge, and the query count per finding.

Merge test files: 18 tests OK. ruff check . (0.16.10): clean.

[dryrunsecurity]

This pull request contains multiple low-severity findings where user 'svader0' modified sensitive codepaths in the dojo directory without being on the allowed authors list. These changes affect various modules including findings, importers, tasks, and database migrations, but are not blocking.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/finding/models.py (drs_dc3f815e)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/finding/models.py' matches configured sensitive codepath pattern 'dojo/finding/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/base_importer.py (drs_36f385f1)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/importers/base_importer.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/default_importer.py (drs_e193a0a5)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/importers/default_importer.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/default_reimporter.py (drs_2a13f064)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/importers/default_reimporter.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/import_lock.py (drs_720ec318)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/importers/import_lock.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/tasks.py (drs_21a25c01)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/tasks.py' matches configured sensitive codepath pattern 'dojo/tasks.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/db_migrations/0299_finding_test_numsev_index.py (drs_60403234)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/db_migrations/0299_finding_test_numsev_index.py' matches configured sensitive codepath pattern 'dojo/db_migrations/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/db_migrations/0300_finding_upper_title_index.py (drs_d769f394)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/db_migrations/0300_finding_upper_title_index.py' matches configured sensitive codepath pattern 'dojo/db_migrations/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/finding/deduplication.py (drs_f67a9b65)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/finding/deduplication.py' matches configured sensitive codepath pattern 'dojo/finding/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/finding/helper.py (drs_07012841)

Vulnerability	Configured Sensitive Codepath Modified by Non-Allowed Author
Description	File 'dojo/finding/helper.py' matches configured sensitive codepath pattern 'dojo/finding/*.py' and was modified by 'svader0' (commit 35b2779) who is not in the allowed authors list.

---

Comment to provide feedback on these findings.

Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]

Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing

All finding details can be found in the DryRun Security Dashboard.