diff --git a/docs/security/security.md b/docs/security/security.md index 046364b..8dc17ec 100644 --- a/docs/security/security.md +++ b/docs/security/security.md @@ -449,10 +449,11 @@ Even with your own Issue fingerprint, you want GitHub alerts to remain stable: ## Current implementation status -As of 2026-02, `promote_alerts.py` implements the fingerprint-based sync loop described above: +As of 2026-03, `promote_alerts.py` implements the fingerprint-based sync loop described above: - Matches issues strictly by `secmeta.fingerprint` (from the alert message `Alert hash: ...`) - Ensures a parent issue per `rule_id` (`secmeta.type=parent`) and links child issues under the parent using GitHub sub-issues +- Closes an open parent issue when all known child issues for the same `rule_id` are already closed - Writes/updates `secmeta` on child issues, including `gh_alert_numbers`, `first_seen`, `last_seen`, `last_seen_commit`, and occurrence tracking - Reopens a closed matching Issue when an alert is open again - Adds `[sec-event]` comments only for meaningful events (reopen, new occurrence) diff --git a/src/security/collect_alert.py b/src/security/collect_alert.py index 32cc5a4..63d7f75 100644 --- a/src/security/collect_alert.py +++ b/src/security/collect_alert.py @@ -59,13 +59,15 @@ def _snake_case(name: str) -> str: return name.strip().lower().replace(" ", "_") -def _help_value(rule_help: str, name: str) -> str | None: +def _help_value(rule_help: str | None, name: str) -> str | None: """Extract a value from ``**Name:** value`` markup in the rule help text.""" + if not rule_help: + return None m = re.search(rf"\*\*{re.escape(name)}:\*\*\s*([^\n\r]+)", rule_help, re.IGNORECASE) return m.group(1) if m else None -def _parse_rule_details(rule_help: str) -> dict[str, str | None]: +def _parse_rule_details(rule_help: str | None) -> dict[str, str | None]: """Extract known rule detail fields from ``**Key:** value`` markup in rule help.""" return {_snake_case(key): _help_value(rule_help, key) for key in RULE_DETAIL_KEYS} @@ -118,7 +120,7 @@ def _normalise_alert(alert: dict) -> dict: instance = alert.get("most_recent_instance") or {} location = instance.get("location") or {} message_text = (instance.get("message") or {}).get("text", "") - rule_help = rule.get("help") or "" + rule_help = rule.get("help", "") return { "metadata": { diff --git a/src/security/utils/alert_parser.py b/src/security/utils/alert_parser.py index 4cbedbf..f593296 100644 --- a/src/security/utils/alert_parser.py +++ b/src/security/utils/alert_parser.py @@ -101,7 +101,7 @@ def load_open_alerts_from_file(path: str) -> LoadedAlerts: alerts = data.get("alerts", []) logging.info(f"Loaded {len(alerts)} alerts from {path} (repo={repo_full})") - open_alerts = [a for a in alerts if str((a.get("metadata") or {}).get("state") or "").lower() == "open"] + open_alerts = [a for a in alerts if str(a.get("metadata", {}).get("state", "")).lower() == "open"] logging.info(f"Found {len(open_alerts)} open alerts") open_by_number: dict[int, Alert] = {} diff --git a/src/security/utils/issue_sync.py b/src/security/utils/issue_sync.py index 87330b1..21429b9 100644 --- a/src/security/utils/issue_sync.py +++ b/src/security/utils/issue_sync.py @@ -71,13 +71,13 @@ def build_issue_index(issues: dict[int, Issue]) -> IssueIndex: for issue in issues.values(): secmeta = load_secmeta(issue.body) - secmeta_type = (secmeta.get("type") or "").strip().lower() + secmeta_type = secmeta.get("type", "").strip().lower() if secmeta_type == SECMETA_TYPE_PARENT: - rule_id = (secmeta.get("rule_id") or "").strip() + rule_id = secmeta.get("rule_id", "").strip() if rule_id: parent_by_rule_id.setdefault(rule_id, issue) - fp = (secmeta.get("fingerprint") or "").strip() or (secmeta.get("alert_hash") or "").strip() + fp = secmeta.get("fingerprint", "").strip() or secmeta.get("alert_hash", "").strip() if fp and secmeta_type != SECMETA_TYPE_PARENT: by_fingerprint.setdefault(fp, issue) @@ -146,6 +146,61 @@ def maybe_reopen_parent_issue( ) +def _close_resolved_parent_issues( + issues: dict[int, Issue], + index: IssueIndex, + *, + dry_run: bool, +) -> None: + """Close open parent issues whose known child issues are all closed.""" + child_issues_by_rule_id: dict[str, list[Issue]] = {} + + for issue in issues.values(): + secmeta = load_secmeta(issue.body) + if secmeta.get("type", "").strip().lower() != SECMETA_TYPE_CHILD: + continue + + rule_id = secmeta.get("rule_id", "").strip() + if not rule_id: + continue + + child_issues_by_rule_id.setdefault(rule_id, []).append(issue) + + for rule_id, parent_issue in index.parent_by_rule_id.items(): + if parent_issue.state.lower() == "closed": + continue + + child_issues = child_issues_by_rule_id.get(rule_id, []) + if not child_issues: + continue + + if any(child_issue.state.lower() != "closed" for child_issue in child_issues): + continue + + parent_secmeta = load_secmeta(parent_issue.body) + repo = parent_secmeta.get("repo", "").strip() + if not repo and child_issues: + repo = load_secmeta(child_issues[0].body).get("repo", "").strip() + if not repo: + logging.debug(f"Skip closing parent issue #{parent_issue.number}: no repo in secmeta") + continue + + if dry_run: + logging.info( + f"DRY-RUN: would close parent issue #{parent_issue.number} (rule_id={rule_id}) " + f"because all {len(child_issues)} child issue(s) are closed" + ) + parent_issue.state = "closed" + continue + + if gh_issue_edit_state(repo, parent_issue.number, "closed"): + logging.info( + f"Closed parent issue #{parent_issue.number} (rule_id={rule_id}) " + f"because all {len(child_issues)} child issue(s) are closed" + ) + parent_issue.state = "closed" + + def ensure_parent_issue( alert: Alert, issues: dict[int, Issue], @@ -508,7 +563,7 @@ def _merge_child_secmeta( if str(ctx.alert_number) not in existing_alerts: existing_alerts.append(str(ctx.alert_number)) - last_occ_fp = secmeta.get("last_occurrence_fp") or "" + last_occ_fp = secmeta.get("last_occurrence_fp", "") occurrence_count = int(secmeta.get("occurrence_count") or "0" or 0) new_occurrence = bool(ctx.occurrence_fp and ctx.occurrence_fp != last_occ_fp) if occurrence_count <= 0: @@ -880,5 +935,6 @@ def sync_alerts_and_issues( priority_sync.flush() _label_orphan_issues(alerts, index, dry_run=dry_run) + _close_resolved_parent_issues(issues, index, dry_run=dry_run) return SyncResult(notifications=notifications, severity_changes=severity_changes) diff --git a/src/security/utils/models.py b/src/security/utils/models.py index 1cd6cb8..c495db7 100644 --- a/src/security/utils/models.py +++ b/src/security/utils/models.py @@ -143,7 +143,7 @@ def from_dict(cls, d: dict[str, Any], *, repo: str = "") -> "Alert": metadata=AlertMetadata(**{k: v for k, v in md.items() if k in AlertMetadata.__dataclass_fields__}), alert_details=AlertDetails(**{k: v for k, v in ad.items() if k in AlertDetails.__dataclass_fields__}), rule_details=RuleDetails(**{k: v for k, v in rd.items() if k in RuleDetails.__dataclass_fields__}), - repo=repo or str(d.get("_repo") or ""), + repo=repo or str(d.get("_repo", "")), ) diff --git a/src/shared/github_issues.py b/src/shared/github_issues.py index 8edf70a..00a5604 100644 --- a/src/shared/github_issues.py +++ b/src/shared/github_issues.py @@ -118,9 +118,9 @@ def gh_issue_list_by_label(repo: str, label: str) -> dict[int, Issue]: label_names = [str(lbl.get("name") or lbl) if isinstance(lbl, dict) else str(lbl) for lbl in raw_labels] issues[number] = Issue( number=number, - state=str(obj.get("state") or ""), - title=str(obj.get("title") or ""), - body=str(obj.get("body") or ""), + state=str(obj.get("state", "")), + title=str(obj.get("title", "")), + body=str(obj.get("body", "")), labels=label_names, ) diff --git a/src/shared/github_projects.py b/src/shared/github_projects.py index 0195379..2a2ec4a 100644 --- a/src/shared/github_projects.py +++ b/src/shared/github_projects.py @@ -233,7 +233,7 @@ def _prefetch_items(self) -> None: if item_id and content_id: self._content_to_item[content_id] = item_id fv = node.get("fieldValueByName") or {} - current_opt = fv.get("optionId") or "" + current_opt = fv.get("optionId", "") if item_id: self._item_current_option[item_id] = current_opt total += len(nodes) diff --git a/tests/security/utils/test_issue_sync.py b/tests/security/utils/test_issue_sync.py index 9a502c5..5fa7422 100644 --- a/tests/security/utils/test_issue_sync.py +++ b/tests/security/utils/test_issue_sync.py @@ -26,6 +26,7 @@ from shared.models import Issue from utils.issue_sync import ( _append_notification, + _close_resolved_parent_issues, _comment_child_event, _flush_parent_body_updates, _handle_existing_child_issue, @@ -811,6 +812,50 @@ def test_label_orphan_no_repo_in_secmeta() -> None: _label_orphan_issues({}, index, dry_run=False) +# ===================================================================== +# _close_resolved_parent_issues +# ===================================================================== + + +def test_close_resolved_parent_issue(mocker: MockerFixture) -> None: + """Closes an open parent when all known children are closed.""" + mock_edit = mocker.patch("utils.issue_sync.gh_issue_edit_state", return_value=True) + parent = _issue_with_secmeta(10, { + "type": "parent", "rule_id": "R1", "repo": "org/repo", + }) + child_one = _issue_with_secmeta(11, { + "type": "child", "rule_id": "R1", "fingerprint": "fp1", "repo": "org/repo", + }, state="closed") + child_two = _issue_with_secmeta(12, { + "type": "child", "rule_id": "R1", "fingerprint": "fp2", "repo": "org/repo", + }, state="closed") + issues = {10: parent, 11: child_one, 12: child_two} + index = build_issue_index(issues) + + _close_resolved_parent_issues(issues, index, dry_run=False) + + mock_edit.assert_called_once_with("org/repo", 10, "closed") + assert parent.state == "closed" + + +def test_close_resolved_parent_skips_open_child(mocker: MockerFixture) -> None: + """Leaves the parent open when any child is still open.""" + mock_edit = mocker.patch("utils.issue_sync.gh_issue_edit_state", return_value=True) + parent = _issue_with_secmeta(10, { + "type": "parent", "rule_id": "R1", "repo": "org/repo", + }) + child = _issue_with_secmeta(11, { + "type": "child", "rule_id": "R1", "fingerprint": "fp1", "repo": "org/repo", + }, state="open") + issues = {10: parent, 11: child} + index = build_issue_index(issues) + + _close_resolved_parent_issues(issues, index, dry_run=False) + + mock_edit.assert_not_called() + assert parent.state == "open" + + # ===================================================================== # ensure_issue (end-to-end orchestration per alert) # ===================================================================== @@ -947,6 +992,24 @@ def test_sync_severity_change_detected(sast_alert: Alert) -> None: assert result.severity_changes[0].new_severity == "high" +def test_sync_closes_parent_when_all_children_closed(mocker: MockerFixture) -> None: + """Closes a parent during sync when its children are already closed.""" + mock_edit = mocker.patch("utils.issue_sync.gh_issue_edit_state", return_value=True) + parent = _issue_with_secmeta(10, { + "type": "parent", "rule_id": "R1", "repo": "org/repo", + }) + child = _issue_with_secmeta(11, { + "type": "child", "rule_id": "R1", "fingerprint": "fp1", "repo": "org/repo", + }, state="closed") + issues = {10: parent, 11: child} + + result = sync_alerts_and_issues({}, issues, dry_run=False) + + assert result.notifications == [] + mock_edit.assert_called_once_with("org/repo", 10, "closed") + assert parent.state == "closed" + + # ===================================================================== # _init_priority_sync # =====================================================================