diff --git a/dojo/engagement/views.py b/dojo/engagement/views.py index 1b94ace21f4..c955439814f 100644 --- a/dojo/engagement/views.py +++ b/dojo/engagement/views.py @@ -1442,9 +1442,10 @@ def view_edit_risk_acceptance(request, eid, raid, *, edit_mode=False): "Since you are not the note's author, it was not deleted.", extra_tags="alert-danger") - if "remove_finding" in request.POST: + if edit_mode and "remove_finding" in request.POST: finding = get_object_or_404( - Finding, pk=request.POST["remove_finding_id"]) + risk_acceptance.accepted_findings, + pk=request.POST["remove_finding_id"]) ra_helper.remove_finding_from_risk_acceptance(request.user, risk_acceptance, finding) diff --git a/unittests/test_permissions_audit.py b/unittests/test_permissions_audit.py index 8904c6270be..26ef3fb78c8 100644 --- a/unittests/test_permissions_audit.py +++ b/unittests/test_permissions_audit.py @@ -12,6 +12,7 @@ 8. Questionnaire cross-engagement IDOR (H1 #3571957) 9. Finding Templates exposure via find_template_to_apply (H1 #3577363) 10. Jira Epic BFLA - Reader cannot trigger update_jira_epic (H1 #3577193) +11. Risk Acceptance remove_finding: edit_mode guard + scoped finding lookup (PR #14633) """ import datetime @@ -701,6 +702,266 @@ def test_view_risk_acceptance_same_engagement(self): self.assertEqual(response.status_code, 200) +class TestRiskAcceptanceRemoveFindingGuard(DojoTestCase): + + """ + PR #14633: view_edit_risk_acceptance must: + 1. Only process 'remove_finding' when edit_mode is True (Writer+ via edit URL). + 2. Scope the finding lookup to risk_acceptance.accepted_findings (not global Finding). + + Prevents a Reader from removing findings via the view URL, and prevents + cross-product blind enumeration of finding IDs. + """ + + @classmethod + def setUpTestData(cls): + cls.reader_role = Role.objects.get(name="Reader") + cls.owner_role = Role.objects.get(name="Owner") + + # ── Product A ──────────────────────────────────────────────── + cls.product_type_a = Product_Type.objects.create( + name="RA Remove Guard Test PT A", + ) + cls.product_a = Product.objects.create( + name="RA Remove Guard Product A", + description="Test", + prod_type=cls.product_type_a, + enable_full_risk_acceptance=True, + ) + + # ── Product B (for cross-product IDOR test) ───────────────── + cls.product_type_b = Product_Type.objects.create( + name="RA Remove Guard Test PT B", + ) + cls.product_b = Product.objects.create( + name="RA Remove Guard Product B", + description="Test", + prod_type=cls.product_type_b, + enable_full_risk_acceptance=True, + ) + + # ── Users ──────────────────────────────────────────────────── + cls.reader_user_a = Dojo_User.objects.create_user( + username="ra_remove_reader_a", + password="testTEST1234!@#$", # noqa: S106 + is_active=True, + ) + cls.owner_user_a = Dojo_User.objects.create_user( + username="ra_remove_owner_a", + password="testTEST1234!@#$", # noqa: S106 + is_active=True, + ) + cls.owner_user_b = Dojo_User.objects.create_user( + username="ra_remove_owner_b", + password="testTEST1234!@#$", # noqa: S106 + is_active=True, + ) + + # ── Role assignments ───────────────────────────────────────── + Product_Member.objects.create( + product=cls.product_a, + user=cls.reader_user_a, + role=cls.reader_role, + ) + Product_Member.objects.create( + product=cls.product_a, + user=cls.owner_user_a, + role=cls.owner_role, + ) + Product_Member.objects.create( + product=cls.product_b, + user=cls.owner_user_b, + role=cls.owner_role, + ) + + # ── Product A: engagement, test, findings, risk acceptance ─── + cls.engagement_a = Engagement.objects.create( + name="RA Remove Guard Engagement A", + product=cls.product_a, + target_start=datetime.date(2024, 1, 1), + target_end=datetime.date(2024, 12, 31), + ) + test_type, _ = Test_Type.objects.get_or_create( + name="Manual Code Review", + ) + cls.test_a = Test.objects.create( + engagement=cls.engagement_a, + test_type=test_type, + target_start=timezone.now(), + target_end=timezone.now(), + ) + + # Finding that IS in the risk acceptance + cls.finding_a = Finding.objects.create( + title="RA Remove Guard Finding A", + test=cls.test_a, + severity="High", + numerical_severity="S1", + active=False, + risk_accepted=True, + reporter=cls.owner_user_a, + ) + + # Finding in same engagement but NOT in the risk acceptance + cls.finding_a_extra = Finding.objects.create( + title="RA Remove Guard Finding A Extra", + test=cls.test_a, + severity="Medium", + numerical_severity="S2", + active=True, + risk_accepted=False, + reporter=cls.owner_user_a, + ) + + cls.risk_acceptance_a = Risk_Acceptance.objects.create( + name="RA Remove Guard RA A", + owner=cls.owner_user_a, + ) + cls.risk_acceptance_a.accepted_findings.add(cls.finding_a) + cls.engagement_a.risk_acceptance.add(cls.risk_acceptance_a) + + # ── Product B: engagement, test, finding, risk acceptance ──── + cls.engagement_b = Engagement.objects.create( + name="RA Remove Guard Engagement B", + product=cls.product_b, + target_start=datetime.date(2024, 1, 1), + target_end=datetime.date(2024, 12, 31), + ) + cls.test_b = Test.objects.create( + engagement=cls.engagement_b, + test_type=test_type, + target_start=timezone.now(), + target_end=timezone.now(), + ) + cls.finding_b = Finding.objects.create( + title="RA Remove Guard Finding B", + test=cls.test_b, + severity="High", + numerical_severity="S1", + active=False, + risk_accepted=True, + reporter=cls.owner_user_b, + ) + + cls.risk_acceptance_b = Risk_Acceptance.objects.create( + name="RA Remove Guard RA B", + owner=cls.owner_user_b, + ) + cls.risk_acceptance_b.accepted_findings.add(cls.finding_b) + cls.engagement_b.risk_acceptance.add(cls.risk_acceptance_b) + + def _login(self, username): + client = Client() + client.login( + username=username, + password="testTEST1234!@#$", # noqa: S106 + ) + return client + + def _remove_finding_data(self, finding_id): + return { + "remove_finding": "Remove", + "remove_finding_id": finding_id, + } + + # ── Test 1: edit_mode guard (BFLA) ─────────────────────────────── + + def test_reader_cannot_remove_finding_via_view_url(self): + """Reader POSTing remove_finding to view URL must be silently ignored.""" + client = self._login("ra_remove_reader_a") + url = reverse("view_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + response = client.post(url, self._remove_finding_data(self.finding_a.id)) + # View still redirects (302) because errors=False, but finding is untouched + self.assertEqual(response.status_code, 302) + self.finding_a.refresh_from_db() + self.assertFalse(self.finding_a.active) + self.assertTrue(self.finding_a.risk_accepted) + self.assertTrue( + self.risk_acceptance_a.accepted_findings + .filter(pk=self.finding_a.pk) + .exists(), + ) + + # ── Test 2: positive regression (edit URL works) ───────────────── + + def test_owner_can_remove_finding_via_edit_url(self): + """Owner POSTing remove_finding to edit URL must succeed.""" + client = self._login("ra_remove_owner_a") + url = reverse("edit_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + response = client.post(url, self._remove_finding_data(self.finding_a.id)) + self.assertEqual(response.status_code, 302) + self.finding_a.refresh_from_db() + self.assertTrue(self.finding_a.active) + self.assertFalse(self.finding_a.risk_accepted) + self.assertFalse( + self.risk_acceptance_a.accepted_findings + .filter(pk=self.finding_a.pk) + .exists(), + ) + + # ── Test 3: scoped lookup (finding not in this RA) ─────────────── + + def test_finding_not_in_risk_acceptance_returns_404(self): + """Supplying a finding ID not in the RA's accepted_findings must 404.""" + client = self._login("ra_remove_owner_a") + url = reverse("edit_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + # finding_a_extra exists in the same engagement but is NOT accepted + response = client.post( + url, self._remove_finding_data(self.finding_a_extra.id), + ) + self.assertEqual(response.status_code, 404) + + # ── Test 4: cross-product IDOR ─────────────────────────────────── + + def test_cross_product_finding_id_rejected(self): + """Finding from Product B cannot be removed via Product A's RA.""" + client = self._login("ra_remove_owner_a") + url = reverse("edit_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + response = client.post( + url, self._remove_finding_data(self.finding_b.id), + ) + self.assertEqual(response.status_code, 404) + # Product B's finding must remain untouched + self.finding_b.refresh_from_db() + self.assertFalse(self.finding_b.active) + self.assertTrue(self.finding_b.risk_accepted) + + # ── Test 5: Reader blocked by decorator on edit URL ────────────── + + def test_reader_blocked_on_edit_url_by_decorator(self): + """Reader lacks Risk_Acceptance permission — edit URL itself is denied.""" + client = self._login("ra_remove_reader_a") + url = reverse("edit_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + response = client.post(url, self._remove_finding_data(self.finding_a.id)) + # PermissionDenied raised; custom handler403 returns 400 (DD bug) + self.assertIn(response.status_code, [400, 403]) + # Finding must remain untouched + self.finding_a.refresh_from_db() + self.assertFalse(self.finding_a.active) + self.assertTrue(self.finding_a.risk_accepted) + + # ── Test 6: nonexistent finding ID ─────────────────────────────── + + def test_nonexistent_finding_id_returns_404(self): + """A bogus finding ID must produce 404 from the scoped lookup.""" + client = self._login("ra_remove_owner_a") + url = reverse("edit_risk_acceptance", args=( + self.engagement_a.id, self.risk_acceptance_a.id, + )) + response = client.post(url, self._remove_finding_data(999999)) + self.assertEqual(response.status_code, 404) + + class TestEngagementPresetsCrossProductIDOR(DojoTestCase): """ diff --git a/unittests/test_risk_acceptance.py b/unittests/test_risk_acceptance.py index c0613bd49ee..628624b6db1 100644 --- a/unittests/test_risk_acceptance.py +++ b/unittests/test_risk_acceptance.py @@ -111,7 +111,7 @@ def test_remove_findings_from_risk_acceptance_findings_active(self): data = copy.copy(self.data_remove_finding_from_ra) data["remove_finding_id"] = 2 ra = Risk_Acceptance.objects.last() - response = self.client.post(reverse("view_risk_acceptance", args=(1, ra.id)), data) + response = self.client.post(reverse("edit_risk_acceptance", args=(1, ra.id)), data) self.assertEqual(302, response.status_code, response.content[:1000]) self.assert_all_active_not_risk_accepted(Finding.objects.filter(id=2)) self.assert_all_inactive_risk_accepted(Finding.objects.filter(id=3))