Fix remove_finding BFLA and add test coverage (PR #14633)

Maffooch · claude · Maffooch · commit fd269085da00 · 2026-04-14T16:33:54.000-06:00
Gate the remove_finding POST branch on edit_mode so only the edit URL
(requiring Risk_Acceptance permission) can process finding removals.
Scope the finding lookup to risk_acceptance.accepted_findings to prevent
cross-product blind enumeration via sequential IDs.

Add 6 security tests covering: edit_mode guard, scoped lookup, cross-product
IDOR, decorator enforcement, and positive regression.

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git 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
@@ -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
@@ -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))
