diff --git a/dojo/jira_link/helper.py b/dojo/jira_link/helper.py index feff72003ef..e1e874c3205 100644 --- 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,45 @@ 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" (the linked finding should be + # mitigated / risk-accepted / false-positive'd) when the issue's + # statusCategory.key is "done". Jira's statusCategory is the canonical + # closure signal (one of "new", "indeterminate", "done", "undefined"), + # always returned by the API, and does not depend on per-workflow + # resolution-field hygiene. + # + # Relying on statusCategory prevents 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. + # + # The resolution value (when present) is still used downstream to + # classify "done" issues as risk-accepted, false-positive, or the + # default mitigated category (see jira_instance.accepted_resolutions + # and .false_positive_resolutions below). + resolved = 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 index 061ad83d83c..58b738d4068 100644 --- 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/dojo/management/commands/jira_status_reconciliation.py b/dojo/management/commands/jira_status_reconciliation.py index 2b8a649ed9f..215c0fb7a33 100644 --- a/dojo/management/commands/jira_status_reconciliation.py +++ b/dojo/management/commands/jira_status_reconciliation.py @@ -102,6 +102,13 @@ def _reconcile_findings(mode, product_obj, engagement_obj, timestamp, dryrun, me resolution = issue_from_jira.fields.resolution if issue_from_jira.fields.resolution and issue_from_jira.fields.resolution != "None" else None resolution_id = resolution.id if resolution else None resolution_name = resolution.name if resolution else None + # statusCategory.key is the canonical "is this issue done" signal + # that process_resolution_from_jira uses to decide whether to mitigate. + status_category_key = getattr( + getattr(getattr(issue_from_jira.fields, "status", None), "statusCategory", None), + "key", + None, + ) # convert from str to datetime issue_from_jira.fields.updated = parse_datetime(issue_from_jira.fields.updated) @@ -175,7 +182,11 @@ def _reconcile_findings(mode, product_obj, engagement_obj, timestamp, dryrun, me if action == "import_status_from_jira": message_action = "deactivating" if find.active else "reactivating" - status_changed = jira_helper.process_resolution_from_jira(find, resolution_id, resolution_name, assignee_name, issue_from_jira.fields.updated, find.jira_issue) if not dryrun else "dryrun" + status_changed = jira_helper.process_resolution_from_jira( + find, resolution_id, resolution_name, assignee_name, + issue_from_jira.fields.updated, find.jira_issue, + status_category_key=status_category_key, + ) if not dryrun else "dryrun" if status_changed: message = f"{find.jira_issue.jira_key}; {settings.SITE_URL}/finding/{find.id};{find.status()};{resolution_name};{flag1};{flag2};{flag3};{find.jira_issue.jira_change};{issue_from_jira.fields.updated};{find.last_status_update};{issue_from_jira.fields.updated};{find.last_reviewed};{issue_from_jira.fields.updated};{message_action} finding in defectdojo;{status_changed}" messages.append(message) @@ -264,6 +275,13 @@ def _reconcile_finding_groups(mode, product_obj, engagement_obj, timestamp, dryr resolution = issue_from_jira.fields.resolution if issue_from_jira.fields.resolution and issue_from_jira.fields.resolution != "None" else None resolution_id = resolution.id if resolution else None resolution_name = resolution.name if resolution else None + # statusCategory.key is the canonical "is this issue done" signal + # that process_resolution_from_jira uses to decide whether to mitigate. + status_category_key = getattr( + getattr(getattr(issue_from_jira.fields, "status", None), "statusCategory", None), + "key", + None, + ) # convert from str to datetime issue_from_jira.fields.updated = parse_datetime(issue_from_jira.fields.updated) @@ -335,6 +353,7 @@ def _reconcile_finding_groups(mode, product_obj, engagement_obj, timestamp, dryr find, resolution_id, resolution_name, assignee_name, issue_from_jira.fields.updated, finding_group.jira_issue, finding_group=finding_group, + status_category_key=status_category_key, ) else: status_changed = "dryrun" diff --git a/unittests/test_jira_webhook.py b/unittests/test_jira_webhook.py index 8d99a60dac9..1ede1202602 100644 --- 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")