From b09cc24dfc8653916933790fef1b05b5f691e3ee Mon Sep 17 00:00:00 2001 From: Bruno Oliveira da Silva Date: Wed, 14 Oct 2026 16:58:40 -0300 Subject: [PATCH] Add @security reject command to close issues as rejected and reply to reporter Add support for a new @security reject command to handle rejecting security reports, ensuring consistency when rejecting issues across the security queue. When invoked with reasoning, the bot will email the reporter/mailing list, apply the status/rejected label, and close the issue as Not Planned on GitHub. If invoked without reasoning, it must fail with a thumbs down reaction and log the error. Closes #89 AI Disclosure: This commit was authored with the assistance of an AI coding tool. All changes were reviewed and approved by the committer. Signed-off-by: Bruno Oliveira da Silva --- .../org/keycloak/gh/bot/labels/Status.java | 3 +- .../gh/bot/security/SecurityBotCli.java | 2 + .../security/command/MailingListCommand.java | 5 +- .../bot/security/command/RejectCommand.java | 71 +++++ .../gh/bot/security/common/Constants.java | 2 + .../org/keycloak/gh/bot/labels/EnumTest.java | 4 + .../security/command/RejectCommandTest.java | 273 ++++++++++++++++++ 7 files changed, 355 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/keycloak/gh/bot/security/command/RejectCommand.java create mode 100644 src/test/java/org/keycloak/gh/bot/security/command/RejectCommandTest.java diff --git a/src/main/java/org/keycloak/gh/bot/labels/Status.java b/src/main/java/org/keycloak/gh/bot/labels/Status.java index 317484a..dc40058 100644 --- a/src/main/java/org/keycloak/gh/bot/labels/Status.java +++ b/src/main/java/org/keycloak/gh/bot/labels/Status.java @@ -10,7 +10,8 @@ public enum Status { TRIAGE, REOPENED, CVE_REQUESTED, - CVE_ASSIGNED; + CVE_ASSIGNED, + REJECTED; @Override public String toString() { diff --git a/src/main/java/org/keycloak/gh/bot/security/SecurityBotCli.java b/src/main/java/org/keycloak/gh/bot/security/SecurityBotCli.java index dfbf8cd..4efa0f7 100644 --- a/src/main/java/org/keycloak/gh/bot/security/SecurityBotCli.java +++ b/src/main/java/org/keycloak/gh/bot/security/SecurityBotCli.java @@ -4,6 +4,7 @@ import io.quarkiverse.githubapp.command.airline.CliOptions; import io.quarkiverse.githubapp.command.airline.CommandOptions; import org.keycloak.gh.bot.security.command.MailingListCommand; +import org.keycloak.gh.bot.security.command.RejectCommand; import org.keycloak.gh.bot.security.command.SecAlertCommand; import org.keycloak.gh.bot.security.command.UnknownCommand; @@ -12,6 +13,7 @@ defaultCommand = UnknownCommand.class, commands = { MailingListCommand.class, + RejectCommand.class, SecAlertCommand.class, UnknownCommand.class } diff --git a/src/main/java/org/keycloak/gh/bot/security/command/MailingListCommand.java b/src/main/java/org/keycloak/gh/bot/security/command/MailingListCommand.java index 9efb19d..278db9b 100644 --- a/src/main/java/org/keycloak/gh/bot/security/command/MailingListCommand.java +++ b/src/main/java/org/keycloak/gh/bot/security/command/MailingListCommand.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.util.Optional; -import java.util.regex.Pattern; /** * Replies to the keycloak-security mailing list and the original sender when invoked via @security reply. @@ -19,8 +18,6 @@ public class MailingListCommand extends CommandParser implements BotCommand { private static final Logger LOGGER = Logger.getLogger(MailingListCommand.class); - private static final Pattern THREAD_ID_PATTERN = Pattern.compile( - Pattern.quote(Constants.GMAIL_THREAD_ID_PREFIX) + "\\s*([a-f0-9]+)"); @Inject MailSender mailSender; @@ -33,7 +30,7 @@ protected void execute(GHEventPayload.IssueComment payload) throws IOException { Optional msg = extractMessage(payload); if (msg.isEmpty()) return; - Optional threadId = findThreadId(payload, THREAD_ID_PATTERN); + Optional threadId = findThreadId(payload, Constants.GMAIL_THREAD_ID_PATTERN); if (threadId.isEmpty()) { fail(payload, "Gmail Thread ID not found in issue comments. Cannot reply without a linked email thread."); return; diff --git a/src/main/java/org/keycloak/gh/bot/security/command/RejectCommand.java b/src/main/java/org/keycloak/gh/bot/security/command/RejectCommand.java new file mode 100644 index 0000000..854ada4 --- /dev/null +++ b/src/main/java/org/keycloak/gh/bot/security/command/RejectCommand.java @@ -0,0 +1,71 @@ +package org.keycloak.gh.bot.security.command; + +import com.github.rvesse.airline.annotations.Command; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.jboss.logging.Logger; +import org.keycloak.gh.bot.labels.Status; +import org.keycloak.gh.bot.security.common.Constants; +import org.keycloak.gh.bot.security.email.MailSender; +import org.keycloak.gh.bot.utils.Labels; +import org.kohsuke.github.GHEventPayload; +import org.kohsuke.github.GHIssue; +import org.kohsuke.github.GHIssueStateReason; +import org.kohsuke.github.GHLabel; + +import java.io.IOException; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Rejects a security report by notifying the reporter via email, applying the rejected label, and closing the issue. + */ +@Command(name = "reject", description = "Rejects a security report, notifies the reporter, and closes the issue") +public class RejectCommand extends CommandParser implements BotCommand { + + private static final Logger LOGGER = Logger.getLogger(RejectCommand.class); + + @Inject + MailSender mailSender; + + @ConfigProperty(name = "google.group.target") + String targetGroup; + + @Override + protected void execute(GHEventPayload.IssueComment payload) throws IOException { + Optional msg = extractMessage(payload); + if (msg.isEmpty()) return; + + GHIssue issue = payload.getIssue(); + String reasoning = msg.get().body(); + + if (Labels.hasLabel(issue, Labels.SOURCE_EMAIL)) { + Optional threadId = findThreadId(payload, Constants.GMAIL_THREAD_ID_PATTERN); + if (threadId.isEmpty()) { + fail(payload, "Gmail Thread ID not found in issue comments. Cannot reject without notifying the reporter."); + return; + } + + LOGGER.infof("Sending rejection reply to Gmail thread %s for issue #%d", threadId.get(), issue.getNumber()); + + if (!mailSender.sendReply(threadId.get(), reasoning, targetGroup)) { + fail(payload, "Failed to send rejection reply via Gmail API."); + return; + } + } else { + LOGGER.infof("Issue #%d has no %s label. Skipping email notification.", issue.getNumber(), Labels.SOURCE_EMAIL); + } + + Set currentLabels = issue.getLabels().stream().map(GHLabel::getName).collect(Collectors.toSet()); + + issue.addLabels(Status.REJECTED.toLabel()); + + if (currentLabels.contains(Status.TRIAGE.toLabel())) { + issue.removeLabels(Status.TRIAGE.toLabel()); + } + + issue.close(GHIssueStateReason.NOT_PLANNED); + success(payload); + } +} diff --git a/src/main/java/org/keycloak/gh/bot/security/common/Constants.java b/src/main/java/org/keycloak/gh/bot/security/common/Constants.java index 53e89cd..1e34f61 100644 --- a/src/main/java/org/keycloak/gh/bot/security/common/Constants.java +++ b/src/main/java/org/keycloak/gh/bot/security/common/Constants.java @@ -10,6 +10,8 @@ public final class Constants { public static final Pattern GHI_ISSUE_PATTERN = Pattern.compile("#GHI-(\\d{1,9})"); public static final String GMAIL_THREAD_ID_PREFIX = "**Gmail-Thread-ID:**"; + public static final Pattern GMAIL_THREAD_ID_PATTERN = Pattern.compile( + Pattern.quote(GMAIL_THREAD_ID_PREFIX) + "\\s*([a-f0-9]+)"); public static final String SECALERT_THREAD_ID_PREFIX = "**SecAlert-Thread-ID:**"; public static final String CVE_TBD_PREFIX = "[CVE-TBD]"; public static final String ISSUE_DESCRIPTION_TEMPLATE = "_Thread originally started in the keycloak-security mailing list. Replace the content here with a proper CVE description._"; diff --git a/src/test/java/org/keycloak/gh/bot/labels/EnumTest.java b/src/test/java/org/keycloak/gh/bot/labels/EnumTest.java index c88246f..e4eef28 100644 --- a/src/test/java/org/keycloak/gh/bot/labels/EnumTest.java +++ b/src/test/java/org/keycloak/gh/bot/labels/EnumTest.java @@ -12,6 +12,7 @@ public void testToLabel() { Assertions.assertEquals("status/missing-information", Status.MISSING_INFORMATION.toLabel()); Assertions.assertEquals("status/bumped-by-bot", Status.BUMPED_BY_BOT.toLabel()); Assertions.assertEquals("label:status/missing-information", "label:" + Status.MISSING_INFORMATION); + Assertions.assertEquals("status/rejected", Status.REJECTED.toLabel()); Assertions.assertEquals("kind/bug", Kind.BUG.toLabel()); Assertions.assertEquals("label:kind/bug", "label:" + Kind.BUG); } @@ -21,6 +22,7 @@ public void fromLabel() { Assertions.assertEquals(Priority.IMPORTANT, Priority.fromLabel("priority/important")); Assertions.assertEquals(Status.MISSING_INFORMATION, Status.fromLabel("status/missing-information")); Assertions.assertEquals(Status.BUMPED_BY_BOT, Status.fromLabel("status/bumped-by-bot")); + Assertions.assertEquals(Status.REJECTED, Status.fromLabel("status/rejected")); Assertions.assertEquals(Kind.BUG, Kind.fromLabel("kind/bug")); } @@ -28,6 +30,7 @@ public void fromLabel() { public void fromName() { Assertions.assertEquals(Priority.IMPORTANT, Priority.fromPriority("important")); Assertions.assertEquals(Status.MISSING_INFORMATION, Status.fromStatus("missing-information")); + Assertions.assertEquals(Status.REJECTED, Status.fromStatus("rejected")); Assertions.assertEquals(Kind.BUG, Kind.fromKind("bug")); } @@ -44,6 +47,7 @@ public void isInstance() { Assertions.assertFalse(Priority.isInstance("something/important")); Assertions.assertFalse(Priority.isInstance("important")); Assertions.assertTrue(Status.isInstance("status/missing-information")); + Assertions.assertTrue(Status.isInstance("status/rejected")); Assertions.assertFalse(Status.isInstance("something")); } diff --git a/src/test/java/org/keycloak/gh/bot/security/command/RejectCommandTest.java b/src/test/java/org/keycloak/gh/bot/security/command/RejectCommandTest.java new file mode 100644 index 0000000..337d3a3 --- /dev/null +++ b/src/test/java/org/keycloak/gh/bot/security/command/RejectCommandTest.java @@ -0,0 +1,273 @@ +package org.keycloak.gh.bot.security.command; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.keycloak.gh.bot.labels.Status; +import org.keycloak.gh.bot.security.common.Constants; +import org.keycloak.gh.bot.security.email.MailSender; +import org.keycloak.gh.bot.utils.Labels; +import org.kohsuke.github.GHEventPayload; +import org.kohsuke.github.GHIssue; +import org.kohsuke.github.GHIssueComment; +import org.kohsuke.github.GHIssueCommentQueryBuilder; +import org.kohsuke.github.GHIssueStateReason; +import org.kohsuke.github.GHLabel; +import org.kohsuke.github.GHRepository; +import org.kohsuke.github.PagedIterable; +import org.kohsuke.github.PagedIterator; +import org.kohsuke.github.ReactionContent; + +import java.io.IOException; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +class RejectCommandTest { + + private MailSender mailSender; + private RejectCommand command; + private GHEventPayload.IssueComment payload; + private GHIssueComment comment; + private GHIssue issue; + + @BeforeEach + void setUp() throws Exception { + mailSender = mock(MailSender.class); + command = new RejectCommand(); + + setField(command, "mailSender", mailSender); + setField(command, "targetGroup", "keycloak-security@googlegroups.com"); + + payload = mock(GHEventPayload.IssueComment.class); + comment = mock(GHIssueComment.class); + issue = mock(GHIssue.class); + + GHRepository repository = mock(GHRepository.class); + when(repository.getFullName()).thenReturn("keycloak/keycloak-private"); + when(payload.getRepository()).thenReturn(repository); + when(payload.getComment()).thenReturn(comment); + when(comment.getNodeId()).thenReturn("IC_reject123"); + when(payload.getIssue()).thenReturn(issue); + when(issue.getNumber()).thenReturn(99); + } + + @Test + void reject_sendsReplyAndClosesIssue() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nThis does not represent a valid security vulnerability."); + setupIssueLabels(Labels.SOURCE_EMAIL, Status.TRIAGE.toLabel()); + setupIssueComments(Constants.GMAIL_THREAD_ID_PREFIX + " abc123def456"); + when(mailSender.sendReply("abc123def456", "This does not represent a valid security vulnerability.", + "keycloak-security@googlegroups.com")).thenReturn(true); + + command.run(payload); + + verify(mailSender).sendReply("abc123def456", "This does not represent a valid security vulnerability.", + "keycloak-security@googlegroups.com"); + verify(issue).addLabels(Status.REJECTED.toLabel()); + verify(issue).removeLabels(Status.TRIAGE.toLabel()); + verify(issue).close(GHIssueStateReason.NOT_PLANNED); + verify(comment).createReaction(ReactionContent.PLUS_ONE); + } + + @Test + void reject_closesWithoutEmailWhenNotFromEmail() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nNot a security issue."); + setupIssueLabels(Status.TRIAGE.toLabel()); + + command.run(payload); + + verify(mailSender, never()).sendReply(anyString(), anyString(), anyString()); + verify(issue).addLabels(Status.REJECTED.toLabel()); + verify(issue).removeLabels(Status.TRIAGE.toLabel()); + verify(issue).close(GHIssueStateReason.NOT_PLANNED); + verify(comment).createReaction(ReactionContent.PLUS_ONE); + } + + @Test + void reject_skipsRemoveTriageWhenNotPresent() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nNot applicable."); + setupIssueLabels("kind/bug"); + + command.run(payload); + + verify(issue, never()).removeLabels(any(String[].class)); + verify(issue).addLabels(Status.REJECTED.toLabel()); + verify(issue).close(GHIssueStateReason.NOT_PLANNED); + verify(comment).createReaction(ReactionContent.PLUS_ONE); + } + + @Test + void reject_reactsWithThumbsDownWhenNoBody() throws Exception { + when(comment.getBody()).thenReturn("@security reject"); + + command.run(payload); + + verify(comment).createReaction(ReactionContent.MINUS_ONE); + verify(mailSender, never()).sendReply(anyString(), anyString(), anyString()); + verify(issue, never()).addLabels(any(String[].class)); + verify(issue, never()).close(any(GHIssueStateReason.class)); + } + + @Test + void reject_reactsWithThumbsDownWhenEmptyBody() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n "); + + command.run(payload); + + verify(comment).createReaction(ReactionContent.MINUS_ONE); + verify(mailSender, never()).sendReply(anyString(), anyString(), anyString()); + verify(issue, never()).addLabels(any(String[].class)); + verify(issue, never()).close(any(GHIssueStateReason.class)); + } + + @Test + void reject_reactsWithThumbsDownWhenNoThreadId() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nNot a vulnerability."); + setupIssueLabels(Labels.SOURCE_EMAIL, Status.TRIAGE.toLabel()); + setupIssueComments("Some unrelated comment without thread ID"); + + command.run(payload); + + verify(comment).createReaction(ReactionContent.MINUS_ONE); + verify(mailSender, never()).sendReply(anyString(), anyString(), anyString()); + verify(issue, never()).addLabels(any(String[].class)); + verify(issue, never()).close(any(GHIssueStateReason.class)); + } + + @Test + void reject_reactsWithThumbsDownWhenEmailFails() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nRejection reasoning."); + setupIssueLabels(Labels.SOURCE_EMAIL, Status.TRIAGE.toLabel()); + setupIssueComments(Constants.GMAIL_THREAD_ID_PREFIX + " deadbeef42"); + when(mailSender.sendReply("deadbeef42", "Rejection reasoning.", + "keycloak-security@googlegroups.com")).thenReturn(false); + + command.run(payload); + + verify(comment).createReaction(ReactionContent.MINUS_ONE); + verify(issue, never()).addLabels(any(String[].class)); + verify(issue, never()).close(any(GHIssueStateReason.class)); + } + + @Test + void reject_blockedOnUnauthorizedRepository() throws Exception { + GHRepository wrongRepo = mock(GHRepository.class); + when(wrongRepo.getFullName()).thenReturn("keycloak/keycloak-public"); + when(payload.getRepository()).thenReturn(wrongRepo); + when(comment.getBody()).thenReturn("@security reject\n\nRejected."); + + command.run(payload); + + verify(mailSender, never()).sendReply(anyString(), anyString(), anyString()); + verify(issue, never()).addLabels(any(String[].class)); + verify(issue, never()).close(any(GHIssueStateReason.class)); + verify(comment, never()).createReaction(any()); + } + + @Test + void reject_blocksDuplicateWebhook() throws Exception { + when(comment.getBody()).thenReturn("@security reject\n\nDuplicate test."); + setupIssueLabels(Status.TRIAGE.toLabel()); + + CountDownLatch executeBlocked = new CountDownLatch(1); + CountDownLatch secondCallDone = new CountDownLatch(1); + + GHLabel triageLabel = mock(GHLabel.class); + when(triageLabel.getName()).thenReturn(Status.TRIAGE.toLabel()); + + GHIssue slowIssue = mock(GHIssue.class); + when(slowIssue.getNumber()).thenReturn(99); + when(slowIssue.getLabels()).thenReturn(List.of(triageLabel)); + + doAnswer(inv -> { + executeBlocked.countDown(); + secondCallDone.await(5, TimeUnit.SECONDS); + return null; + }).when(slowIssue).addLabels(any(String[].class)); + + GHEventPayload.IssueComment slowPayload = mock(GHEventPayload.IssueComment.class); + GHRepository slowRepo = mock(GHRepository.class); + when(slowRepo.getFullName()).thenReturn("keycloak/keycloak-private"); + when(slowPayload.getRepository()).thenReturn(slowRepo); + when(slowPayload.getComment()).thenReturn(comment); + when(slowPayload.getIssue()).thenReturn(slowIssue); + + RejectCommand secondCommand = new RejectCommand(); + setField(secondCommand, "mailSender", mailSender); + setField(secondCommand, "targetGroup", "keycloak-security@googlegroups.com"); + + AtomicReference firstThreadError = new AtomicReference<>(); + Thread firstThread = new Thread(() -> { + try { command.run(slowPayload); } catch (Exception e) { firstThreadError.set(e); } + }); + firstThread.start(); + + executeBlocked.await(5, TimeUnit.SECONDS); + secondCommand.run(slowPayload); + secondCallDone.countDown(); + firstThread.join(5000); + + assertNull(firstThreadError.get(), "First thread threw an exception"); + verify(slowIssue, times(1)).addLabels(any(String[].class)); + } + + private GHLabel mockLabel(String name) { + GHLabel label = mock(GHLabel.class); + when(label.getName()).thenReturn(name); + return label; + } + + private void setupIssueLabels(String... labelNames) { + List labels = new ArrayList<>(); + for (String name : labelNames) { + labels.add(mockLabel(name)); + } + when(issue.getLabels()).thenReturn(labels); + } + + @SuppressWarnings("unchecked") + private void setupIssueComments(String... commentBodies) throws IOException { + GHIssueCommentQueryBuilder queryBuilder = mock(GHIssueCommentQueryBuilder.class); + when(issue.queryComments()).thenReturn(queryBuilder); + + List comments = new ArrayList<>(); + for (String body : commentBodies) { + GHIssueComment c = mock(GHIssueComment.class); + when(c.getBody()).thenReturn(body); + comments.add(c); + } + + PagedIterable pagedIterable = mock(PagedIterable.class); + when(queryBuilder.list()).thenReturn(pagedIterable); + + when(pagedIterable.iterator()).thenAnswer(inv -> { + PagedIterator pagedIterator = mock(PagedIterator.class); + final int[] index = {0}; + when(pagedIterator.hasNext()).thenAnswer(i -> index[0] < comments.size()); + when(pagedIterator.next()).thenAnswer(i -> comments.get(index[0]++)); + return pagedIterator; + }); + } + + private static void setField(Object target, String fieldName, Object value) throws Exception { + Class clazz = target.getClass(); + while (clazz != null) { + try { + Field field = clazz.getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + return; + } catch (NoSuchFieldException e) { + clazz = clazz.getSuperclass(); + } + } + throw new NoSuchFieldException(fieldName); + } +}