Skip to content

Commit c603cd9

Browse files
Maffoochclaude
andauthored
fix(finding): carry locations across a finding merge (#15379)
Merging findings copied the legacy Endpoint m2m onto the destination finding but never copied its location references. With V3_FEATURE_LOCATIONS enabled, findings carry LocationFindingReference rows instead of endpoints, so the destination came out of the merge with none of the merged findings' locations. Add copy_location_references() in dojo/location/utils.py and call it from merge_finding_product alongside the existing endpoint copy, under the same "Add Endpoints" choice. Locations the destination already references are skipped, so its own status and relationship data win and the unique (location, finding) constraint holds. References are copied, not moved, matching how vulnerability IDs already behave. The form label and help text now mention locations as well as endpoints. Fixes #15377 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 00e88f2 commit c603cd9

4 files changed

Lines changed: 210 additions & 4 deletions

File tree

dojo/finding/ui/forms.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,8 @@ class MergeFindings(forms.ModelForm):
124124
append_description = forms.BooleanField(label="Append Description", initial=True, required=False,
125125
help_text="Description in all findings will be appended into the merged finding.")
126126

127-
add_endpoints = forms.BooleanField(label="Add Endpoints", initial=True, required=False,
128-
help_text="Endpoints in all findings will be merged into the merged finding.")
127+
add_endpoints = forms.BooleanField(label="Add Endpoints and Locations", initial=True, required=False,
128+
help_text="Endpoints and locations in all findings will be merged into the merged finding.")
129129

130130
dynamic_raw = forms.BooleanField(label="Dynamic Scanner Raw Requests", initial=True, required=False,
131131
help_text="Dynamic scanner raw requests in all findings will be merged into the merged finding.")

dojo/finding/ui/views.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
from dojo.jira import services as jira_services
7474
from dojo.location.queries import get_authorized_locations
7575
from dojo.location.status import FindingLocationStatus
76+
from dojo.location.utils import copy_location_references
7677
from dojo.models import (
7778
IMPORT_UNTOUCHED_FINDING,
7879
BurpRawRequestResponse,
@@ -2338,12 +2339,14 @@ def merge_finding_product(request, pid):
23382339
):
23392340
finding_references = f"{finding_references}\n{finding.references}"
23402341

2341-
# if checked merge the endpoints
2342+
# if checked merge the endpoints and locations
23422343
if form.cleaned_data["add_endpoints"]:
23432344
with Endpoint.allow_endpoint_init(): # TODO: Delete this after the move to Locations
23442345
finding_to_merge_into.endpoints.add(
23452346
*finding.endpoints.all(),
23462347
)
2348+
if settings.V3_FEATURE_LOCATIONS:
2349+
copy_location_references(finding, finding_to_merge_into)
23472350

23482351
# if checked merge the tags
23492352
if form.cleaned_data["tag_finding"]:

dojo/location/utils.py

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,39 @@
33
from django.core.exceptions import ValidationError
44
from django.db.models import Q
55

6-
from dojo.location.models import AbstractLocation
6+
from dojo.location.models import AbstractLocation, LocationFindingReference
7+
from dojo.models import Finding
78
from dojo.url.models import URL
89
from dojo.url.validators import DEFAULT_PORTS
910

1011
logger = logging.getLogger(__name__)
1112

1213

14+
def copy_location_references(source_finding: Finding, destination_finding: Finding) -> int:
15+
"""
16+
Copy the source finding's location references onto the destination finding.
17+
18+
Used when consolidating findings, so the destination ends up covering every location the
19+
source findings did. Locations the destination already references are skipped: its own
20+
status and relationship data win, and the unique (location, finding) constraint holds.
21+
The source finding keeps its references; they are copied, not moved.
22+
23+
Returns the number of references created.
24+
"""
25+
already_referenced = LocationFindingReference.objects.filter(
26+
finding=destination_finding,
27+
).values_list("location_id", flat=True)
28+
references_to_copy = LocationFindingReference.objects.filter(
29+
finding=source_finding,
30+
).exclude(location_id__in=already_referenced)
31+
32+
copied = 0
33+
for reference in references_to_copy:
34+
reference.copy(destination_finding)
35+
copied += 1
36+
return copied
37+
38+
1339
def save_location(unsaved_location: AbstractLocation) -> AbstractLocation:
1440
# Only support URLs at this time
1541
if isinstance(unsaved_location, URL):
Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
"""
2+
Regression: merging findings copies endpoints onto the destination finding but drops locations.
3+
4+
Reported as issue #15377. ``merge_finding_product`` (``dojo/finding/ui/views.py``) only copied the
5+
legacy ``Endpoint`` m2m when "Add Endpoints" was checked, so under ``V3_FEATURE_LOCATIONS`` the
6+
destination finding came out of the merge with none of the source findings' locations.
7+
"""
8+
9+
import logging
10+
11+
from django.contrib.auth.models import User
12+
from django.test import Client, override_settings
13+
from django.urls import reverse
14+
from django.utils import timezone
15+
from parameterized import parameterized
16+
17+
from dojo.location.models import LocationFindingReference
18+
from dojo.location.status import FindingLocationStatus
19+
from dojo.models import (
20+
Engagement,
21+
Finding,
22+
Product,
23+
Product_Type,
24+
Test,
25+
Test_Type,
26+
UserContactInfo,
27+
)
28+
from dojo.url.models import URL
29+
30+
from .dojo_test_case import DojoTestCase
31+
32+
logger = logging.getLogger(__name__)
33+
34+
35+
@override_settings(V3_FEATURE_LOCATIONS=True, SECURE_SSL_REDIRECT=False)
36+
class TestMergeFindingsLocations(DojoTestCase):
37+
38+
"""Locations of the merged findings must land on the destination finding."""
39+
40+
def setUp(self):
41+
super().setUp()
42+
43+
self.admin = User.objects.create(
44+
username="test_merge_locations_admin",
45+
is_staff=True,
46+
is_superuser=True,
47+
)
48+
UserContactInfo.objects.create(user=self.admin, block_execution=True)
49+
50+
self.ui_client = Client()
51+
self.ui_client.force_login(self.admin)
52+
53+
self.system_settings(enable_jira=False)
54+
self.system_settings(enable_github=False)
55+
56+
now = timezone.now()
57+
test_type = Test_Type.objects.get_or_create(name="Manual Test")[0]
58+
product_type = Product_Type.objects.create(name="Org for merge locations")
59+
self.product = Product.objects.create(
60+
name="Product for merge locations",
61+
description="regression fixture",
62+
prod_type=product_type,
63+
)
64+
engagement = Engagement.objects.create(
65+
name="Engagement for merge locations",
66+
product=self.product,
67+
target_start=now,
68+
target_end=now,
69+
)
70+
self.test = Test.objects.create(
71+
engagement=engagement,
72+
test_type=test_type,
73+
target_start=now,
74+
target_end=now,
75+
)
76+
77+
def _make_finding(self, title):
78+
return Finding.objects.create(
79+
test=self.test,
80+
title=title,
81+
severity="High",
82+
description="regression fixture",
83+
mitigation="n/a",
84+
impact="n/a",
85+
reporter=self.admin,
86+
)
87+
88+
def _add_location(self, finding, host, *, status=FindingLocationStatus.Active):
89+
url = URL(protocol="https", host=host)
90+
url.clean()
91+
url = URL.get_or_create_from_object(url)
92+
LocationFindingReference.objects.create(
93+
location=url.location,
94+
finding=finding,
95+
status=status,
96+
)
97+
return url.location
98+
99+
def _merge(self, destination, sources, *, add_endpoints):
100+
finding_ids = [destination.id, *[f.id for f in sources]]
101+
query = "&".join(f"finding_to_update={fid}" for fid in finding_ids)
102+
payload = {
103+
"finding_to_merge_into": destination.id,
104+
"findings_to_merge": [f.id for f in sources],
105+
"append_description": "on",
106+
"finding_action": "inactive",
107+
}
108+
if add_endpoints:
109+
payload["add_endpoints"] = "on"
110+
return self.ui_client.post(
111+
f"{reverse('merge_finding_product', kwargs={'pid': self.product.id})}?{query}",
112+
payload,
113+
)
114+
115+
@parameterized.expand([(True,), (False,)])
116+
def test_merge_copies_locations_when_requested(self, add_endpoints):
117+
"""Locations follow the "Add Endpoints" choice: copied when checked, left alone when not."""
118+
destination = self._make_finding("Merge locations destination")
119+
source = self._make_finding("Merge locations source")
120+
source_location = self._add_location(source, "merge-source.example.com")
121+
122+
response = self._merge(destination, [source], add_endpoints=add_endpoints)
123+
self.assertEqual(response.status_code, 302)
124+
125+
persisted = list(
126+
LocationFindingReference.objects.filter(finding=destination).values_list(
127+
"location__location_value", flat=True,
128+
),
129+
)
130+
expected = ["https://merge-source.example.com"] if add_endpoints else []
131+
self.assertEqual(
132+
sorted(persisted), expected,
133+
msg=f"add_endpoints={add_endpoints}: expected {expected}, destination has {persisted}",
134+
)
135+
# The source finding keeps its own location reference; locations are copied, not moved.
136+
self.assertTrue(
137+
LocationFindingReference.objects.filter(finding=source, location=source_location).exists(),
138+
msg="the source finding lost its location reference during the merge",
139+
)
140+
141+
def test_merge_keeps_destination_reference_for_shared_location(self):
142+
"""A location on both findings must not raise on the unique (location, finding) constraint."""
143+
destination = self._make_finding("Merge shared location destination")
144+
source = self._make_finding("Merge shared location source")
145+
shared = self._add_location(destination, "merge-shared.example.com")
146+
self._add_location(source, "merge-shared.example.com", status=FindingLocationStatus.Mitigated)
147+
148+
response = self._merge(destination, [source], add_endpoints=True)
149+
self.assertEqual(response.status_code, 302)
150+
151+
references = LocationFindingReference.objects.filter(finding=destination, location=shared)
152+
self.assertEqual(
153+
references.count(), 1,
154+
msg=f"expected a single reference to the shared location, found {references.count()}",
155+
)
156+
self.assertEqual(
157+
references.first().status, FindingLocationStatus.Active,
158+
msg="the destination finding's own status must win over the merged finding's status",
159+
)
160+
161+
def test_merge_copies_locations_from_every_source_finding(self):
162+
destination = self._make_finding("Merge multi destination")
163+
source_a = self._make_finding("Merge multi source A")
164+
source_b = self._make_finding("Merge multi source B")
165+
self._add_location(source_a, "merge-multi-a.example.com")
166+
self._add_location(source_b, "merge-multi-b.example.com")
167+
168+
response = self._merge(destination, [source_a, source_b], add_endpoints=True)
169+
self.assertEqual(response.status_code, 302)
170+
171+
persisted = sorted(
172+
LocationFindingReference.objects.filter(finding=destination).values_list(
173+
"location__location_value", flat=True,
174+
),
175+
)
176+
expected = ["https://merge-multi-a.example.com", "https://merge-multi-b.example.com"]
177+
self.assertEqual(persisted, expected, msg=f"expected {expected}, destination has {persisted}")

0 commit comments

Comments
 (0)