From ce2009482481d7becd409007653052f65ce4edf2 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Sat, 7 Mar 2026 22:21:05 +0100 Subject: [PATCH 1/3] fix: endpoints not removable from finding via Edit Finding form Two bugs introduced in the locations refactor (PR #14198): 1. FindingForm did not set `endpoints.initial` for the non-V3 path, because `endpoints` is in Meta.exclude and Django won't auto-populate excluded fields from the model instance. Added explicit initial assignment so existing endpoints are pre-selected in the edit form. 2. add_locations() merged the submitted endpoint selection with the pre-existing set (| finding.endpoints.all()), making it impossible to remove an endpoint by deselecting it. Removed the union with the existing set so the submitted selection replaces the current one. Adds unit tests covering add, keep, remove, and switch scenarios for both add_locations() directly and the EditFinding view. --- dojo/finding/helper.py | 3 +- dojo/forms.py | 2 + unittests/test_edit_finding_endpoints.py | 242 +++++++++++++++++++++++ 3 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 unittests/test_edit_finding_endpoints.py diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 4e6b8caace4..12c4ab58a9c 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -771,10 +771,9 @@ def add_locations(finding, form): added_endpoints = save_endpoints_to_add(form.endpoints_to_add_list, finding.test.engagement.product) endpoint_ids = [endpoint.id for endpoint in added_endpoints] - # Merge form endpoints with existing endpoints (don't replace) form_endpoints = form.cleaned_data.get("endpoints", Endpoint.objects.none()) new_endpoints = Endpoint.objects.filter(id__in=endpoint_ids) - finding.endpoints.set(form_endpoints | new_endpoints | finding.endpoints.all()) + finding.endpoints.set(form_endpoints | new_endpoints) for endpoint in finding.endpoints.all(): _eps, _created = Endpoint_Status.objects.get_or_create( diff --git a/dojo/forms.py b/dojo/forms.py index f731a32671e..f8c1b5f760a 100644 --- a/dojo/forms.py +++ b/dojo/forms.py @@ -1582,6 +1582,8 @@ def __init__(self, *args, **kwargs): else: # TODO: Delete this after the move to Locations self.fields["endpoints"].queryset = Endpoint.objects.filter(product=self.instance.test.engagement.product) + if self.instance and self.instance.pk: + self.fields["endpoints"].initial = self.instance.endpoints.all() self.fields["mitigated_by"].queryset = get_authorized_users(Permissions.Finding_Edit) diff --git a/unittests/test_edit_finding_endpoints.py b/unittests/test_edit_finding_endpoints.py new file mode 100644 index 00000000000..2d09c967dd0 --- /dev/null +++ b/unittests/test_edit_finding_endpoints.py @@ -0,0 +1,242 @@ +""" +Tests for endpoint add/remove behaviour on the Edit Finding view. + +Covers two bugs introduced in the locations refactor (PR #14198): +1. Existing endpoints were not pre-selected in the edit form (Meta.exclude + prevents Django from auto-populating the custom field from the instance). +2. Removing a selected endpoint had no effect because add_locations() always + unioned the submitted selection with the pre-existing endpoints. +""" + +import logging +from datetime import date +from types import SimpleNamespace + +from django.test import TestCase, override_settings +from django.urls import reverse +from django.utils.timezone import now + +from dojo.finding.helper import add_locations +from dojo.models import ( + Endpoint, + Endpoint_Status, + Engagement, + Finding, + Product, + Product_Type, + Test, + Test_Type, + User, +) + +logger = logging.getLogger(__name__) + + +def _make_form(endpoints_qs, date_value=None): + """Return a minimal form-like object accepted by add_locations().""" + return SimpleNamespace( + endpoints_to_add_list=[], + cleaned_data={ + "endpoints": endpoints_qs, + "date": date_value or date.today(), + }, + ) + + +@override_settings(V3_FEATURE_LOCATIONS=False) +class TestAddLocationsEndpoints(TestCase): + + """Unit tests for finding.helper.add_locations() in non-V3 (Endpoint) mode.""" + + def setUp(self): + product_type = Product_Type.objects.create(name="PT") + self.product = Product.objects.create(name="P", prod_type=product_type) + engagement = Engagement.objects.create( + name="E", product=self.product, target_start=now(), target_end=now(), + ) + test_type = Test_Type.objects.create(name="TT") + self.test = Test.objects.create( + engagement=engagement, test_type=test_type, + target_start=now(), target_end=now(), + ) + user = User.objects.create_user(username="u1", password="pass") # noqa: S106 + self.finding = Finding.objects.create( + title="F", severity="High", test=self.test, reporter=user, + ) + self.ep1 = Endpoint.objects.create(host="host1.example.com", product=self.product) + self.ep2 = Endpoint.objects.create(host="host2.example.com", product=self.product) + + def test_add_endpoint(self): + """Submitting an endpoint that is not yet on the finding adds it.""" + form = _make_form(Endpoint.objects.filter(pk=self.ep1.pk)) + add_locations(self.finding, form) + self.assertIn(self.ep1, self.finding.endpoints.all()) + + def test_keep_existing_endpoint(self): + """Re-submitting an already-associated endpoint keeps it.""" + self.finding.endpoints.add(self.ep1) + + form = _make_form(Endpoint.objects.filter(pk=self.ep1.pk)) + add_locations(self.finding, form) + + self.assertIn(self.ep1, self.finding.endpoints.all()) + + def test_remove_endpoint(self): + """Submitting an empty selection removes the previously-associated endpoint.""" + self.finding.endpoints.add(self.ep1) + + form = _make_form(Endpoint.objects.none()) + add_locations(self.finding, form) + + self.assertNotIn(self.ep1, self.finding.endpoints.all()) + + def test_switch_endpoint(self): + """Deselecting one endpoint and selecting another replaces it.""" + self.finding.endpoints.add(self.ep1) + + form = _make_form(Endpoint.objects.filter(pk=self.ep2.pk)) + add_locations(self.finding, form) + + self.assertNotIn(self.ep1, self.finding.endpoints.all()) + self.assertIn(self.ep2, self.finding.endpoints.all()) + + def test_endpoint_status_created_on_add(self): + """An Endpoint_Status record is created when an endpoint is added.""" + form = _make_form(Endpoint.objects.filter(pk=self.ep1.pk)) + add_locations(self.finding, form) + + self.assertTrue( + Endpoint_Status.objects.filter(finding=self.finding, endpoint=self.ep1).exists(), + ) + + +@override_settings(V3_FEATURE_LOCATIONS=False) +class TestEditFindingEndpointView(TestCase): + + """View-level tests for EditFinding endpoint handling.""" + + def _minimal_post_data(self, **overrides): + data = { + "title": self.finding.title, + "date": "2024-01-01", + "severity": "High", + "description": "Test description", + "active": "on", + "verified": "", + "false_p": "", + "duplicate": "", + "out_of_scope": "", + "endpoints": [], + "endpoints_to_add": "", + "vulnerability_ids": "", + "references": "", + "mitigation": "", + "impact": "", + "steps_to_reproduce": "", + "severity_justification": "", + } + data.update(overrides) + return data + + def setUp(self): + self.user = User.objects.create_user( + username="tester", password="pass", # noqa: S106 + is_staff=True, is_superuser=True, + ) + self.client.force_login(self.user) + product_type = Product_Type.objects.create(name="PT") + self.product = Product.objects.create(name="P", prod_type=product_type) + engagement = Engagement.objects.create( + name="E", product=self.product, target_start=now(), target_end=now(), + ) + test_type = Test_Type.objects.create(name="TT") + self.test_obj = Test.objects.create( + engagement=engagement, test_type=test_type, + target_start=now(), target_end=now(), + ) + self.finding = Finding.objects.create( + title="Endpoint Test Finding", + severity="High", + test=self.test_obj, + reporter=self.user, + ) + self.endpoint = Endpoint.objects.create( + host="vuln.example.com", product=self.product, + ) + self.url = reverse("edit_finding", args=[self.finding.id]) + + def test_get_preselects_existing_endpoints(self): + """GET edit form has existing endpoints pre-selected as initial values.""" + self.finding.endpoints.add(self.endpoint) + + response = self.client.get(self.url) + + self.assertEqual(response.status_code, 200) + initial = list(response.context["form"].fields["endpoints"].initial) + self.assertIn(self.endpoint, initial) + + def test_get_preselects_multiple_endpoints(self): + """GET edit form pre-selects all associated endpoints, not just the first.""" + endpoint2 = Endpoint.objects.create(host="vuln2.example.com", product=self.product) + self.finding.endpoints.add(self.endpoint) + self.finding.endpoints.add(endpoint2) + + response = self.client.get(self.url) + + self.assertEqual(response.status_code, 200) + initial = list(response.context["form"].fields["endpoints"].initial) + self.assertIn(self.endpoint, initial) + self.assertIn(endpoint2, initial) + + def test_get_no_endpoints_when_none_associated(self): + """GET edit form initial is empty when no endpoints are associated.""" + response = self.client.get(self.url) + + self.assertEqual(response.status_code, 200) + initial = list(response.context["form"].fields["endpoints"].initial) + self.assertEqual(initial, []) + + def test_post_removes_deselected_endpoint(self): + """POST with empty endpoints list removes the previously-associated endpoint.""" + self.finding.endpoints.add(self.endpoint) + + response = self.client.post(self.url, self._minimal_post_data()) + + self.assertIn(response.status_code, [200, 302]) + self.finding.refresh_from_db() + self.assertNotIn(self.endpoint, self.finding.endpoints.all()) + + def test_post_removes_endpoint_status_on_remove(self): + """POST that removes an endpoint also cleans up its Endpoint_Status record.""" + self.finding.endpoints.add(self.endpoint) + + self.client.post(self.url, self._minimal_post_data()) + + self.assertFalse( + Endpoint_Status.objects.filter(finding=self.finding, endpoint=self.endpoint).exists(), + ) + + def test_post_keeps_selected_endpoint(self): + """POST with an endpoint still selected keeps it on the finding.""" + self.finding.endpoints.add(self.endpoint) + + data = self._minimal_post_data(endpoints=[self.endpoint.pk]) + response = self.client.post(self.url, data) + + self.assertIn(response.status_code, [200, 302]) + self.finding.refresh_from_db() + self.assertIn(self.endpoint, self.finding.endpoints.all()) + + def test_post_keeps_all_selected_endpoints(self): + """POST that keeps all endpoints selected preserves all of them.""" + endpoint2 = Endpoint.objects.create(host="vuln2.example.com", product=self.product) + self.finding.endpoints.add(self.endpoint) + self.finding.endpoints.add(endpoint2) + + data = self._minimal_post_data(endpoints=[self.endpoint.pk, endpoint2.pk]) + response = self.client.post(self.url, data) + + self.assertIn(response.status_code, [200, 302]) + self.finding.refresh_from_db() + self.assertIn(self.endpoint, self.finding.endpoints.all()) + self.assertIn(endpoint2, self.finding.endpoints.all()) From a46a7d63531e4410454206535a2795b0f96eccd4 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Sat, 7 Mar 2026 23:16:42 +0100 Subject: [PATCH 2/3] fix: restore union behaviour in add_locations() for non-edit paths The previous fix applied replace semantics unconditionally, breaking the add_finding_from_template flow which copies template endpoints onto the finding before calling add_locations(). Replace is now opt-in via a keyword argument (replace=False by default). Only EditFinding passes replace=True; all other callers (add from template, promote to finding, add finding) keep the original union behaviour so that pre-populated endpoints are not wiped by an empty form submission. Unit tests updated to pass replace=True when testing the remove/replace scenarios that are specific to the EditFinding path. --- dojo/finding/helper.py | 7 +++++-- dojo/finding/views.py | 4 ++-- unittests/test_edit_finding_endpoints.py | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 12c4ab58a9c..b390ff17be6 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -765,7 +765,7 @@ def removeLoop(finding_id, counter): removeLoop(f.id, counter - 1) -def add_locations(finding, form): +def add_locations(finding, form, *, replace=False): # TODO: Delete this after the move to Locations if not settings.V3_FEATURE_LOCATIONS: added_endpoints = save_endpoints_to_add(form.endpoints_to_add_list, finding.test.engagement.product) @@ -773,7 +773,10 @@ def add_locations(finding, form): form_endpoints = form.cleaned_data.get("endpoints", Endpoint.objects.none()) new_endpoints = Endpoint.objects.filter(id__in=endpoint_ids) - finding.endpoints.set(form_endpoints | new_endpoints) + if replace: + finding.endpoints.set(form_endpoints | new_endpoints) + else: + finding.endpoints.set(form_endpoints | new_endpoints | finding.endpoints.all()) for endpoint in finding.endpoints.all(): _eps, _created = Endpoint_Status.objects.get_or_create( diff --git a/dojo/finding/views.py b/dojo/finding/views.py index 1be56f7e891..b361e402d96 100644 --- a/dojo/finding/views.py +++ b/dojo/finding/views.py @@ -935,8 +935,8 @@ def process_finding_form(self, request: HttpRequest, finding: Finding, context: ra_helper.simple_risk_accept(request.user, new_finding, perform_save=False) elif new_finding.risk_accepted: ra_helper.risk_unaccept(request.user, new_finding, perform_save=False) - # Save and add new locations - associated_locations = finding_helper.add_locations(new_finding, context["form"]) + # Save and add new locations; replace=True so deselected endpoints are removed + associated_locations = finding_helper.add_locations(new_finding, context["form"], replace=True) # Remove unrelated endpoints if settings.V3_FEATURE_LOCATIONS: for ref in new_finding.locations.all(): diff --git a/unittests/test_edit_finding_endpoints.py b/unittests/test_edit_finding_endpoints.py index 2d09c967dd0..faeef7a1d3e 100644 --- a/unittests/test_edit_finding_endpoints.py +++ b/unittests/test_edit_finding_endpoints.py @@ -86,7 +86,7 @@ def test_remove_endpoint(self): self.finding.endpoints.add(self.ep1) form = _make_form(Endpoint.objects.none()) - add_locations(self.finding, form) + add_locations(self.finding, form, replace=True) self.assertNotIn(self.ep1, self.finding.endpoints.all()) @@ -95,7 +95,7 @@ def test_switch_endpoint(self): self.finding.endpoints.add(self.ep1) form = _make_form(Endpoint.objects.filter(pk=self.ep2.pk)) - add_locations(self.finding, form) + add_locations(self.finding, form, replace=True) self.assertNotIn(self.ep1, self.finding.endpoints.all()) self.assertIn(self.ep2, self.finding.endpoints.all()) From cec9d2e9dcf8a055ece20872f1ed0837fe5b3a22 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Sat, 7 Mar 2026 23:19:45 +0100 Subject: [PATCH 3/3] fix(tests): add required Product description to avoid V3 tagulous validation error --- unittests/test_edit_finding_endpoints.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unittests/test_edit_finding_endpoints.py b/unittests/test_edit_finding_endpoints.py index faeef7a1d3e..33037b178b5 100644 --- a/unittests/test_edit_finding_endpoints.py +++ b/unittests/test_edit_finding_endpoints.py @@ -50,7 +50,7 @@ class TestAddLocationsEndpoints(TestCase): def setUp(self): product_type = Product_Type.objects.create(name="PT") - self.product = Product.objects.create(name="P", prod_type=product_type) + self.product = Product.objects.create(name="P", prod_type=product_type, description="Test product") engagement = Engagement.objects.create( name="E", product=self.product, target_start=now(), target_end=now(), ) @@ -145,7 +145,7 @@ def setUp(self): ) self.client.force_login(self.user) product_type = Product_Type.objects.create(name="PT") - self.product = Product.objects.create(name="P", prod_type=product_type) + self.product = Product.objects.create(name="P", prod_type=product_type, description="Test product") engagement = Engagement.objects.create( name="E", product=self.product, target_start=now(), target_end=now(), )