From eeba5c9d59a56873ceaa880b4a23f95966f2e587 Mon Sep 17 00:00:00 2001 From: Phil Ewels Date: Thu, 20 Aug 2026 12:38:14 +0200 Subject: [PATCH] Add /nf-core security summary command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @core-team can now get an on-demand summary of every currently-open GitHub security advisory across nf-core repos (live data, not the daily digest's cached state), grouped by repo. Posted publicly wherever the command is run (channel or DM), not ephemerally, so the whole team sees the result — mirrors /nf-core proposals review's delivery pattern including the DM fallback when the bot isn't a member of the channel. Co-Authored-By: Claude Sonnet 5 --- README.md | 5 +- docs/commands.md | 40 +++++++- docs/security-advisories.md | 10 ++ src/nf_core_bot/commands/help.py | 18 ++++ src/nf_core_bot/commands/router.py | 52 +++++++++- src/nf_core_bot/commands/security/__init__.py | 0 src/nf_core_bot/commands/security/summary.py | 67 +++++++++++++ src/nf_core_bot/security/advisories.py | 30 ++++++ tests/test_security_advisories.py | 61 ++++++++++++ tests/test_security_command.py | 98 +++++++++++++++++++ 10 files changed, 373 insertions(+), 8 deletions(-) create mode 100644 src/nf_core_bot/commands/security/__init__.py create mode 100644 src/nf_core_bot/commands/security/summary.py create mode 100644 tests/test_security_command.py diff --git a/README.md b/README.md index 8700dc3..e8c30e4 100644 --- a/README.md +++ b/README.md @@ -171,8 +171,9 @@ needed for creation or status changes. The bot polls GitHub once a day for new or changed security advisories (including Triage/draft reports) across every nf-core repo and posts them to a -private Slack channel for the core team to review — see -[docs/security-advisories.md](docs/security-advisories.md). +private Slack channel for the core team to review. `@core-team` members can also +run `/nf-core security summary` any time for an on-demand summary of all open +advisories — see [docs/security-advisories.md](docs/security-advisories.md). ## Development diff --git a/docs/commands.md b/docs/commands.md index 4ebfa4d..c36ee9a 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -495,13 +495,43 @@ when the schedule changes. --- +## Security Commands + +These commands surface GitHub security advisory data. All require `@core-team` +Slack user group membership. See +[security-advisories.md](security-advisories.md) for the daily automated digest +that runs alongside these. + +### `security help` + +``` +/nf-core security help +``` + +Show security command help. + +### `security summary` + +``` +/nf-core security summary +``` + +Fetches live data from GitHub (not the cached digest state) and posts a summary +of every open advisory (`triage`, `draft`, or `published` — excludes +`closed`/`withdrawn`) across all nf-core repos, grouped by repo. Posted +**visibly** in the conversation where you run it (channel or DM), not +ephemerally, so anyone there can see the result. Falls back to DMing you if the +bot can't post where the command was run (e.g. a channel it isn't a member of). + +--- + ## Permission Model -| Level | Who | Access | -| ------------------ | -------------------------------------------- | -------------------------------------------------------------------------------------- | -| **User** | Everyone | `/hackathon list`, `/hackathon register/edit/cancel`, `/hackathon sites` | -| **Site organiser** | Per-hackathon, per-site (stored in DynamoDB) | `/hackathon export` | -| **Admin** | `@core-team` Slack user group members | All commands including `/hackathon admin *`, `/nf-core github *`, `/nf-core on-call *` | +| Level | Who | Access | +| ------------------ | -------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | +| **User** | Everyone | `/hackathon list`, `/hackathon register/edit/cancel`, `/hackathon sites` | +| **Site organiser** | Per-hackathon, per-site (stored in DynamoDB) | `/hackathon export` | +| **Admin** | `@core-team` Slack user group members | All commands including `/hackathon admin *`, `/nf-core github *`, `/nf-core on-call *`, `/nf-core security *` | Admin membership is checked via `usergroups.users.list` and cached for 5 minutes. diff --git a/docs/security-advisories.md b/docs/security-advisories.md index bee2242..f74b75c 100644 --- a/docs/security-advisories.md +++ b/docs/security-advisories.md @@ -34,6 +34,14 @@ flowchart TD Runs as a background `asyncio.Task` launched from `app.py`, following the same pattern as the on-call scheduler (`scheduler/oncall_jobs.py`). +## On-demand summary + +`/nf-core security summary` (`@core-team` only) fetches live data from GitHub +and posts a summary of every currently-open advisory across all repos, grouped +by repo — independent of the daily digest's new/changed diffing. Posted visibly +wherever the command is run (channel or DM), not ephemerally. See +[commands.md](commands.md#security-commands). + ## Configuration (env vars) | Var | Default | Notes | @@ -69,3 +77,5 @@ belongs to. state (`ADVISORY#{repo}` / `{ghsa_id}`) - `src/nf_core_bot/security/advisories.py` — the scheduler loop, diffing logic, and Slack posting (digest + failure notices) +- `src/nf_core_bot/commands/security/summary.py` — the `security summary` slash + command handler diff --git a/src/nf_core_bot/commands/help.py b/src/nf_core_bot/commands/help.py index fb5ba76..a23db54 100644 --- a/src/nf_core_bot/commands/help.py +++ b/src/nf_core_bot/commands/help.py @@ -48,6 +48,10 @@ ("on-call calendar me", "Get a subscribable iCal URL for your on-call weeks", "admin"), ] +SECURITY_COMMANDS: list[tuple[str, str, str]] = [ + ("security summary", "Summarise all open GitHub security advisories across nf-core repos", "admin"), +] + GENERAL_COMMANDS: list[tuple[str, str, str]] = [ ("help", "Show this help message", "all"), ( @@ -57,6 +61,7 @@ ), ("on-call help", "Show on-call rotation commands (@core-team only)", "admin"), ("github help", "Show GitHub commands (@core-team only)", "admin"), + ("security help", "Show security commands (@core-team only)", "admin"), ] @@ -174,3 +179,16 @@ async def handle_oncall_help( sections.append(_format_commands(ONCALL_COMMANDS)) await respond("\n".join(sections), response_type="ephemeral") + + +async def handle_security_help( + respond: Respond, +) -> None: + """Security help: ``/nf-core security help``. + + Only visible to ``@core-team`` members (permission checked in router). + """ + sections = ["*Security commands* (`@core-team` only)\n"] + sections.append(_format_commands(SECURITY_COMMANDS)) + + await respond("\n".join(sections), response_type="ephemeral") diff --git a/src/nf_core_bot/commands/router.py b/src/nf_core_bot/commands/router.py index d718fdf..39d5470 100644 --- a/src/nf_core_bot/commands/router.py +++ b/src/nf_core_bot/commands/router.py @@ -30,7 +30,13 @@ handle_edit, handle_register, ) -from nf_core_bot.commands.help import handle_github_help, handle_hackathon_help, handle_help, handle_oncall_help +from nf_core_bot.commands.help import ( + handle_github_help, + handle_hackathon_help, + handle_help, + handle_oncall_help, + handle_security_help, +) from nf_core_bot.commands.oncall.calendar_cmd import handle_oncall_calendar from nf_core_bot.commands.oncall.list_cmd import handle_oncall_list from nf_core_bot.commands.oncall.me import handle_oncall_me @@ -39,6 +45,7 @@ from nf_core_bot.commands.oncall.switch import handle_oncall_switch from nf_core_bot.commands.oncall.unavailable import handle_oncall_unavailable from nf_core_bot.commands.proposals.review import handle_proposals_review +from nf_core_bot.commands.security.summary import handle_security_summary from nf_core_bot.permissions.checks import is_core_team, is_core_team_or_maintainers if TYPE_CHECKING: @@ -108,6 +115,11 @@ async def dispatch( await _route_proposals(ack, respond, client, user_id, command, rest) return + # ── Security commands ───────────────────────────────────────────── + if sub == "security": + await _route_security(ack, respond, client, user_id, command, rest) + return + # ── Unknown ────────────────────────────────────────────────────── await ack() await respond( @@ -348,3 +360,41 @@ async def _route_proposals( f"Unknown proposals command: `{sub}`. {_PROPOSALS_HELP}", response_type="ephemeral", ) + + +# ── Security dispatch ──────────────────────────────────────────────── + + +async def _route_security( + ack: Ack, + respond: Respond, + client: AsyncWebClient, + user_id: str, + command: dict[str, str], + tokens: list[str], +) -> None: + """Dispatch ``/nf-core security [args…]``. + + Restricted to ``@core-team`` members. + """ + await ack() + + if not await is_core_team(client, user_id): + await respond( + "Sorry, security commands are restricted to `@core-team` members.", + response_type="ephemeral", + ) + return + + if not tokens or tokens[0].lower() == "help": + await handle_security_help(respond) + return + + sub = tokens[0].lower() + if sub == "summary": + await handle_security_summary(respond, client, user_id, command) + else: + await respond( + f"Unknown security command: `{sub}`. Run `/nf-core security help` for options.", + response_type="ephemeral", + ) diff --git a/src/nf_core_bot/commands/security/__init__.py b/src/nf_core_bot/commands/security/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/src/nf_core_bot/commands/security/summary.py b/src/nf_core_bot/commands/security/summary.py new file mode 100644 index 0000000..0847ccb --- /dev/null +++ b/src/nf_core_bot/commands/security/summary.py @@ -0,0 +1,67 @@ +"""``/nf-core security summary`` — summarise open GitHub security advisories. + +Restricted to ``@core-team`` (checked by the router). Posts **publicly** in +the conversation where the command was run — a channel or DM — so the whole +team can see the result, not just the caller, mirroring +``commands/proposals/review.py``'s delivery pattern. +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +from slack_sdk.errors import SlackApiError + +from nf_core_bot.checks.github import list_all_org_advisories +from nf_core_bot.security.advisories import format_advisories_summary + +if TYPE_CHECKING: + from slack_bolt.context.respond.async_respond import AsyncRespond as Respond + from slack_sdk.web.async_client import AsyncWebClient + +logger = logging.getLogger(__name__) + +# Posting where the command was run can fail when the bot isn't a member there +# (a personal self-DM, a channel it hasn't joined) — fall back to a DM instead. +_DM_FALLBACK_ERRORS = frozenset({"channel_not_found", "not_in_channel", "is_archived"}) + + +async def handle_security_summary( + respond: Respond, + client: AsyncWebClient, + user_id: str, + command: dict[str, str], +) -> None: + """Fetch live advisory data from GitHub and post a public summary.""" + try: + advisories = await list_all_org_advisories() + except Exception: + logger.exception("Failed to fetch org security advisories for /nf-core security summary") + await respond( + "Could not fetch security advisories from GitHub — check the bot's token/permissions.", + response_type="ephemeral", + ) + return + + text = format_advisories_summary(advisories) + channel_id = command.get("channel_id", "") + + try: + await client.chat_postMessage(channel=channel_id, text=text) + except SlackApiError as e: + if e.response.get("error") not in _DM_FALLBACK_ERRORS: + raise + try: + dm = (await client.conversations_open(users=[user_id]))["channel"]["id"] + await client.chat_postMessage(channel=dm, text=text) + await respond( + ":inbox_tray: Posted the summary to our DM — I couldn't post where you ran the command.", + response_type="ephemeral", + ) + except SlackApiError: + logger.exception("Failed to deliver security summary for caller %s", user_id) + await respond( + "Sorry — I couldn't deliver the summary. Try running the command in a channel I'm a member of.", + response_type="ephemeral", + ) diff --git a/src/nf_core_bot/security/advisories.py b/src/nf_core_bot/security/advisories.py index fb72e44..91a84c8 100644 --- a/src/nf_core_bot/security/advisories.py +++ b/src/nf_core_bot/security/advisories.py @@ -105,6 +105,36 @@ async def _post_digest(client: AsyncWebClient, advisories: list[SecurityAdvisory logger.exception("Failed to post security advisory %s to Slack", adv.ghsa_id) +# Advisory states that are resolved/done, so are excluded from "open". +_CLOSED_STATES = frozenset({"closed", "withdrawn"}) + + +def format_advisories_summary(advisories: list[SecurityAdvisory]) -> str: + """Render a summary of all open advisories, grouped by repo, for ``/nf-core security summary``. + + "Open" excludes ``closed``/``withdrawn`` states — ``triage``, ``draft`` and + ``published`` all still need attention. Summaries are attacker-influenced + (see ``_escape_mrkdwn``), same as the daily digest. + """ + open_advisories = [a for a in advisories if a.state not in _CLOSED_STATES] + if not open_advisories: + return ":white_check_mark: No open security advisories across nf-core repos." + + by_repo: dict[str, list[SecurityAdvisory]] = {} + for adv in open_advisories: + by_repo.setdefault(adv.repo, []).append(adv) + + count = len(open_advisories) + lines = [f":closed_lock_with_key: *{count} open security advisor{'y' if count == 1 else 'ies'} across nf-core:*"] + for repo in sorted(by_repo): + lines.append(f"\n*{_escape_mrkdwn(repo)}*") + for adv in by_repo[repo]: + severity = f", {adv.severity}" if adv.severity else "" + summary = _escape_mrkdwn(adv.summary) + lines.append(f"• <{adv.url}|{summary}> (`{adv.state}`{severity}) — `{adv.ghsa_id}`") + return "\n".join(lines) + + async def _notify(client: AsyncWebClient, message: str) -> None: """Post a check-failure notice to the security channel — never fail silently.""" channel = await resolve_channel_id(client, config.SECURITY_CHANNEL or "security") diff --git a/tests/test_security_advisories.py b/tests/test_security_advisories.py index ad0a793..af99852 100644 --- a/tests/test_security_advisories.py +++ b/tests/test_security_advisories.py @@ -236,3 +236,64 @@ async def test_no_post_when_nothing_flagged(self, _mock_advisories, _mock_ch, dd await check_security_advisories(client) client.chat_postMessage.assert_not_awaited() + + +# ── security.advisories: format_advisories_summary ───────────────────── + + +class TestFormatAdvisoriesSummary: + def test_no_open_advisories(self) -> None: + from nf_core_bot.security.advisories import format_advisories_summary + + assert "No open security advisories" in format_advisories_summary([]) + + def test_excludes_closed_and_withdrawn(self) -> None: + from nf_core_bot.security.advisories import format_advisories_summary + + advisories = [ + SecurityAdvisory("rnaseq", "GHSA-a", "fixed one", "closed", "high", "2026-08-01", "https://x"), + SecurityAdvisory("rnaseq", "GHSA-b", "withdrawn one", "withdrawn", "high", "2026-08-01", "https://x"), + ] + summary = format_advisories_summary(advisories) + assert "No open security advisories" in summary + + def test_groups_by_repo_and_includes_open_states(self) -> None: + from nf_core_bot.security.advisories import format_advisories_summary + + advisories = [ + SecurityAdvisory("rnaseq", "GHSA-a", "triage issue", "triage", "high", "2026-08-01", "https://x/a"), + SecurityAdvisory("sarek", "GHSA-b", "published issue", "published", None, "2026-08-02", "https://x/b"), + SecurityAdvisory("rnaseq", "GHSA-c", "old closed", "closed", "low", "2026-08-01", "https://x/c"), + ] + summary = format_advisories_summary(advisories) + + assert "2 open security advisories" in summary + assert summary.index("*rnaseq*") < summary.index("*sarek*") + assert "" in summary + assert "`triage`" in summary and "high" in summary + assert "" in summary + assert "GHSA-c" not in summary + + def test_singular_count(self) -> None: + from nf_core_bot.security.advisories import format_advisories_summary + + advisories = [SecurityAdvisory("rnaseq", "GHSA-a", "leak", "triage", "high", "2026-08-01", "https://x")] + assert "1 open security advisory " in format_advisories_summary(advisories) + + def test_escapes_mrkdwn_in_summary_and_repo(self) -> None: + from nf_core_bot.security.advisories import format_advisories_summary + + advisories = [ + SecurityAdvisory( + "rnaseq", + "GHSA-a", + "leak> ", + "triage", + "high", + "2026-08-01", + "https://x", + ) + ] + summary = format_advisories_summary(advisories) + assert "" not in summary + assert "leak> <https://evil.example.com|click>" in summary diff --git a/tests/test_security_command.py b/tests/test_security_command.py new file mode 100644 index 0000000..8468423 --- /dev/null +++ b/tests/test_security_command.py @@ -0,0 +1,98 @@ +"""Tests for the /nf-core security summary slash command + router gate.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +from slack_sdk.errors import SlackApiError + +from nf_core_bot.commands import router +from nf_core_bot.commands.security import summary as cmd + + +class TestHandler: + async def test_posts_summary_where_command_was_run(self) -> None: + respond = AsyncMock() + client = AsyncMock() + with ( + patch.object(cmd, "list_all_org_advisories", new=AsyncMock(return_value=[])), + patch.object(cmd, "format_advisories_summary", return_value="SUMMARY TEXT"), + ): + await cmd.handle_security_summary(respond, client, "U1", {"channel_id": "C1"}) + + client.chat_postMessage.assert_awaited_once_with(channel="C1", text="SUMMARY TEXT") + respond.assert_not_awaited() + + async def test_fetch_failure_reports_ephemerally(self) -> None: + respond = AsyncMock() + client = AsyncMock() + with patch.object(cmd, "list_all_org_advisories", new=AsyncMock(side_effect=RuntimeError("403"))): + await cmd.handle_security_summary(respond, client, "U1", {"channel_id": "C1"}) + + client.chat_postMessage.assert_not_awaited() + respond.assert_awaited_once() + assert "Could not fetch" in respond.await_args.args[0] + assert respond.await_args.kwargs.get("response_type") == "ephemeral" + + async def test_falls_back_to_dm_when_not_in_channel(self) -> None: + respond = AsyncMock() + client = AsyncMock() + client.conversations_open = AsyncMock(return_value={"channel": {"id": "D9"}}) + err = SlackApiError("nope", {"error": "channel_not_found"}) + client.chat_postMessage = AsyncMock(side_effect=[err, {"ok": True}]) + with ( + patch.object(cmd, "list_all_org_advisories", new=AsyncMock(return_value=[])), + patch.object(cmd, "format_advisories_summary", return_value="SUMMARY TEXT"), + ): + await cmd.handle_security_summary(respond, client, "U1", {"channel_id": "C_selfdm"}) + + assert client.chat_postMessage.await_count == 2 + assert client.chat_postMessage.await_args_list[1].kwargs["channel"] == "D9" + assert respond.await_args.kwargs.get("response_type") == "ephemeral" + + async def test_reraises_unexpected_slack_errors(self) -> None: + respond = AsyncMock() + client = AsyncMock() + err = SlackApiError("nope", {"error": "some_other_error"}) + client.chat_postMessage = AsyncMock(side_effect=err) + with ( + patch.object(cmd, "list_all_org_advisories", new=AsyncMock(return_value=[])), + patch.object(cmd, "format_advisories_summary", return_value="SUMMARY TEXT"), + ): + try: + await cmd.handle_security_summary(respond, client, "U1", {"channel_id": "C1"}) + except SlackApiError: + pass + else: + raise AssertionError("expected SlackApiError to propagate") + + +class TestRouterGate: + async def test_non_member_denied(self) -> None: + ack, respond = AsyncMock(), AsyncMock() + with patch.object(router, "is_core_team", new=AsyncMock(return_value=False)): + await router._route_security(ack, respond, AsyncMock(), "U1", {"channel_id": "C1"}, ["summary"]) + ack.assert_awaited_once() + assert "restricted" in respond.await_args.args[0] + + async def test_member_dispatched(self) -> None: + ack, respond = AsyncMock(), AsyncMock() + handler = AsyncMock() + with ( + patch.object(router, "is_core_team", new=AsyncMock(return_value=True)), + patch.object(router, "handle_security_summary", handler), + ): + await router._route_security(ack, respond, AsyncMock(), "U1", {"channel_id": "C1"}, ["summary"]) + handler.assert_awaited_once() + + async def test_help(self) -> None: + ack, respond = AsyncMock(), AsyncMock() + with patch.object(router, "is_core_team", new=AsyncMock(return_value=True)): + await router._route_security(ack, respond, AsyncMock(), "U1", {"channel_id": "C1"}, []) + assert "security summary" in respond.await_args.args[0] + + async def test_unknown_subcommand(self) -> None: + ack, respond = AsyncMock(), AsyncMock() + with patch.object(router, "is_core_team", new=AsyncMock(return_value=True)): + await router._route_security(ack, respond, AsyncMock(), "U1", {"channel_id": "C1"}, ["bogus"]) + assert "Unknown security command" in respond.await_args.args[0]