fix jira bug

paulOsinski · paulOsinski · commit 568f0cebefb4 · 2026-04-21T16:06:30.000-04:00
diff --git a/dojo/jira_link/helper.py b/dojo/jira_link/helper.py
@@ -1133,7 +1133,23 @@ def failure_to_update_message(message: str, exception: Exception, obj: Any) -> t
         message = f"Failed to fetch fields for {jira_instance.default_issue_type} under project {jira_project.project_key} - {e}"
         return failure_to_update_message(message, e, obj)
 
-    # Update the issue in jira
+    # Update the status in jira FIRST, before applying the other field
+    # updates (priority, description, labels, etc.). This ordering matters
+    # because each call to the Jira REST API fires its own
+    # jira:issue_updated webhook. If we updated fields first, the
+    # pre-transition webhook would see the old (stale) resolution /
+    # statusCategory - for a reopen, the webhook would still report the
+    # issue as resolved and our handler would ricochet the linked finding
+    # back to mitigated before the transition webhook even arrived. By
+    # transitioning first, every webhook that fires during this sync sees
+    # the intended post-transition state.
+    try:
+        push_status_to_jira(obj, jira_instance, jira, issue)
+    except Exception as e:
+        message = f"Failed to update the jira issue status - {e}"
+        return failure_to_update_message(message, e, obj)
+    # Update the rest of the issue fields in jira (summary, description,
+    # priority, labels, due date, etc.) AFTER the transition above.
     try:
         logger.debug("Updating JIRA issue with fields: %s", json.dumps(fields, indent=4))
         issue.update(
@@ -1145,12 +1161,6 @@ def failure_to_update_message(message: str, exception: Exception, obj: Any) -> t
     except Exception as e:
         message = f"Failed to update the jira issue with the following payload: {fields} - {e}"
         return failure_to_update_message(message, e, obj)
-    # Update the status in jira
-    try:
-        push_status_to_jira(obj, jira_instance, jira, issue)
-    except Exception as e:
-        message = f"Failed to update the jira issue status - {e}"
-        return failure_to_update_message(message, e, obj)
     # Upload dojo finding screenshots to Jira
     try:
         findings = [obj]
@@ -1887,11 +1897,46 @@ def escape_for_jira(text):
     return text.replace("|", "%7D")
 
 
-def process_resolution_from_jira(finding, resolution_id, resolution_name, assignee_name, jira_now, jira_issue, finding_group: Finding_Group = None) -> bool:
+def process_resolution_from_jira(
+    finding,
+    resolution_id,
+    resolution_name,
+    assignee_name,
+    jira_now,
+    jira_issue,
+    finding_group: Finding_Group = None,
+    *,
+    status_category_key: str | None = None,
+) -> bool:
     """Processes the resolution field in the JIRA issue and updated the finding in Defect Dojo accordingly"""
     import dojo.risk_acceptance.helper as ra_helper  # noqa: PLC0415 import error
     status_changed = False
-    resolved = resolution_id is not None
+    # A Jira issue is treated as "resolved" (i.e. the linked finding should be
+    # mitigated / risk-accepted / false-positive'd) only when BOTH a non-null
+    # resolution is present AND the issue's statusCategory is "done". Jira's
+    # statusCategory.key is the canonical closure signal (always one of
+    # "new", "indeterminate", "done"), and checking it alongside the
+    # resolution field prevents several real-world bugs:
+    #   1. Workflows where reopen transitions do not clear the resolution -
+    #      the issue ends up in an "open but resolved" state and every
+    #      webhook event for the issue would otherwise mis-mitigate the
+    #      finding.
+    #   2. Workflows that set a default resolution on brand-new issues
+    #      (for example "Unresolved") - a webhook would otherwise mis-
+    #      mitigate the finding as soon as the issue was created.
+    #   3. Ricochets during DefectDojo's own push to Jira: the issue.update()
+    #      call and the subsequent status transition each fire a webhook,
+    #      and the first webhook sees the pre-transition (still-resolved)
+    #      state even though the intended final state is reopened.
+    #
+    # status_category_key is keyword-only and optional to preserve backward
+    # compatibility with any caller that has not yet been updated to extract
+    # it from the webhook payload; when it is None we fall back to the
+    # historical resolution-only behavior.
+    if status_category_key is None:
+        resolved = resolution_id is not None
+    else:
+        resolved = resolution_id is not None and status_category_key == "done"
     jira_instance = get_jira_instance(finding)
 
     if resolved:
diff --git a/dojo/jira_link/views.py b/dojo/jira_link/views.py
@@ -149,11 +149,31 @@ def webhook(request, secret=None):
                 resolution = resolution if resolution and resolution != "None" else None
                 resolution_id = resolution["id"] if resolution else None
                 resolution_name = resolution["name"] if resolution else None
+
+                # Extract the statusCategory.key from the issue status. Jira
+                # returns one of "new" (To Do), "indeterminate" (In Progress)
+                # or "done" (Done). We pass this alongside the resolution into
+                # process_resolution_from_jira so mitigation is only triggered
+                # when the issue is genuinely closed, not just when a
+                # resolution value happens to be present.
+                status = parsed["issue"]["fields"].get("status") or {}
+                status_category = status.get("statusCategory") or {}
+                status_category_key = status_category.get("key")
+
                 jira_now = parse_datetime(parsed["issue"]["fields"]["updated"])
 
                 if findings:
                     for finding in findings:
-                        jira_helper.process_resolution_from_jira(finding, resolution_id, resolution_name, assignee_name, jira_now, jissue, finding_group=jissue.finding_group)
+                        jira_helper.process_resolution_from_jira(
+                            finding,
+                            resolution_id,
+                            resolution_name,
+                            assignee_name,
+                            jira_now,
+                            jissue,
+                            finding_group=jissue.finding_group,
+                            status_category_key=status_category_key,
+                        )
                 # Check for any comment that could have come along with the resolution
                 if (error_response := check_for_and_create_comment(parsed)) is not None:
                     return error_response
diff --git a/unittests/test_jira_webhook.py b/unittests/test_jira_webhook.py
@@ -667,3 +667,124 @@ def test_webhook_issue_updated_extracts_comment(self):
         last_note = finding.notes.order_by("-id").first()
         self.assertIsNotNone(last_note)
         self.assertEqual(last_note.entry, "(Valentijn Scholten (valentijn)): test2")
+
+    # ---- statusCategory guard on the mitigation path ----
+    # The webhook payload's issue.fields.resolution used to be the sole
+    # signal for whether the Jira issue was closed, which caused several
+    # real-world false-mitigations:
+    #   (a) Jira workflows where the Reopen transition does not clear
+    #       resolution - the issue ends up in status=To-Do with a stale
+    #       resolution value, and every webhook mis-mitigated the finding.
+    #   (b) Ricochets during DefectDojo's own push to Jira: when a finding
+    #       is reopened, the issue.update() and the subsequent transition
+    #       fire separate webhooks, and the first one sees the
+    #       pre-transition (still-resolved) state.
+    # The fix: a webhook only mitigates when BOTH resolution is non-null
+    # AND statusCategory.key == "done". These tests lock that in.
+
+    def _update_body_with_status(self, *, status_name, status_category_key):
+        body = json.loads(self.jira_issue_update_template_string)
+        # Target the JIRA_Issue linked to finding 5 (jira_id=2 in the fixture)
+        body["issue"]["id"] = 2
+        body["issue"]["fields"]["status"]["name"] = status_name
+        body["issue"]["fields"]["status"]["statusCategory"]["key"] = status_category_key
+        # The template already carries a non-null resolution ("Cancelled").
+        # That is the interesting state: resolution is set but status may or
+        # may not actually be "done".
+        return body
+
+    def _reset_finding_to_active(self, finding):
+        finding.active = True
+        finding.is_mitigated = False
+        finding.mitigated = None
+        finding.mitigated_by = None
+        finding.false_p = False
+        finding.risk_accepted = False
+        finding.save()
+
+    def test_webhook_update_does_not_mitigate_when_status_category_is_new(self):
+        """
+        Regression: resolution set + statusCategory 'new' must NOT mitigate.
+
+        This is the real-world symptom seen when a Jira Reopen transition
+        forgets to clear the resolution field, or when DefectDojo's own
+        multi-step push to Jira fires a webhook against the pre-transition
+        state during a finding reopen.
+        """
+        self.system_settings(
+            enable_jira=True, enable_jira_web_hook=True,
+            disable_jira_webhook_secret=False,
+            jira_webhook_secret=self.correct_secret,
+        )
+        jira_issue = JIRA_Issue.objects.get(jira_id=2)
+        finding = jira_issue.finding
+        self._reset_finding_to_active(finding)
+
+        body = self._update_body_with_status(
+            status_name="To Do", status_category_key="new",
+        )
+        response = self.client.post(
+            reverse("jira_web_hook_secret", args=(self.correct_secret, )),
+            body,
+            content_type="application/json",
+        )
+        self.assertEqual(200, response.status_code, response.content[:1000])
+
+        finding.refresh_from_db()
+        self.assertTrue(finding.active)
+        self.assertFalse(finding.is_mitigated)
+        self.assertIsNone(finding.mitigated)
+        self.assertIsNone(finding.mitigated_by)
+
+    def test_webhook_update_does_not_mitigate_when_status_category_is_indeterminate(self):
+        """Same guard for issues in the 'In Progress' category."""
+        self.system_settings(
+            enable_jira=True, enable_jira_web_hook=True,
+            disable_jira_webhook_secret=False,
+            jira_webhook_secret=self.correct_secret,
+        )
+        jira_issue = JIRA_Issue.objects.get(jira_id=2)
+        finding = jira_issue.finding
+        self._reset_finding_to_active(finding)
+
+        body = self._update_body_with_status(
+            status_name="In Progress", status_category_key="indeterminate",
+        )
+        response = self.client.post(
+            reverse("jira_web_hook_secret", args=(self.correct_secret, )),
+            body,
+            content_type="application/json",
+        )
+        self.assertEqual(200, response.status_code, response.content[:1000])
+
+        finding.refresh_from_db()
+        self.assertTrue(finding.active)
+        self.assertFalse(finding.is_mitigated)
+
+    def test_webhook_update_mitigates_when_status_category_is_done(self):
+        """Happy path: a genuinely-closed Jira issue still mitigates its finding."""
+        self.system_settings(
+            enable_jira=True, enable_jira_web_hook=True,
+            disable_jira_webhook_secret=False,
+            jira_webhook_secret=self.correct_secret,
+        )
+        jira_issue = JIRA_Issue.objects.get(jira_id=2)
+        finding = jira_issue.finding
+        self._reset_finding_to_active(finding)
+
+        body = self._update_body_with_status(
+            status_name="Done", status_category_key="done",
+        )
+        response = self.client.post(
+            reverse("jira_web_hook_secret", args=(self.correct_secret, )),
+            body,
+            content_type="application/json",
+        )
+        self.assertEqual(200, response.status_code, response.content[:1000])
+
+        finding.refresh_from_db()
+        self.assertFalse(finding.active)
+        self.assertTrue(finding.is_mitigated)
+        self.assertIsNotNone(finding.mitigated)
+        self.assertIsNotNone(finding.mitigated_by)
+        self.assertEqual(finding.mitigated_by.username, "JIRA")
