-
Notifications
You must be signed in to change notification settings - Fork 564
Add blocked deny list for assign-to-user and unassign-from-user safe outputs
#16628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
4a1514a
b4cf858
deefded
7102cba
0626e1a
183941f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -483,4 +483,106 @@ describe("assign_to_user (Handler Factory Architecture)", () => { | |
| assignees: ["new-user1"], | ||
| }); | ||
| }); | ||
|
|
||
| describe("blocked patterns", () => { | ||
| it("should filter out blocked users by exact match", async () => { | ||
| const { main } = require("./assign_to_user.cjs"); | ||
| const handler = await main({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good test coverage for blocked patterns. It would also be worth adding a test case that verifies an empty |
||
| max: 10, | ||
| blocked: ["copilot", "admin"], | ||
| }); | ||
|
|
||
| mockGithub.rest.issues.addAssignees.mockResolvedValue({}); | ||
|
|
||
| const message = { | ||
| type: "assign_to_user", | ||
| assignees: ["user1", "copilot", "admin", "user2"], | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.assigneesAdded).toEqual(["user1", "user2"]); | ||
| expect(mockGithub.rest.issues.addAssignees).toHaveBeenCalledWith({ | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| issue_number: 123, | ||
| assignees: ["user1", "user2"], | ||
| }); | ||
| }); | ||
|
|
||
| it("should filter out blocked users by pattern", async () => { | ||
| const { main } = require("./assign_to_user.cjs"); | ||
| const handler = await main({ | ||
| max: 10, | ||
| blocked: ["*[bot]"], | ||
| }); | ||
|
|
||
| mockGithub.rest.issues.addAssignees.mockResolvedValue({}); | ||
|
|
||
| const message = { | ||
| type: "assign_to_user", | ||
| assignees: ["user1", "dependabot[bot]", "github-actions[bot]", "user2"], | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.assigneesAdded).toEqual(["user1", "user2"]); | ||
| expect(mockGithub.rest.issues.addAssignees).toHaveBeenCalledWith({ | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| issue_number: 123, | ||
| assignees: ["user1", "user2"], | ||
| }); | ||
| }); | ||
|
|
||
| it("should combine allowed and blocked filters", async () => { | ||
| const { main } = require("./assign_to_user.cjs"); | ||
| const handler = await main({ | ||
| max: 10, | ||
| allowed: ["user1", "user2", "copilot", "github-actions[bot]"], | ||
| blocked: ["copilot", "*[bot]"], | ||
| }); | ||
|
|
||
| mockGithub.rest.issues.addAssignees.mockResolvedValue({}); | ||
|
|
||
| const message = { | ||
| type: "assign_to_user", | ||
| assignees: ["user1", "user2", "copilot", "github-actions[bot]", "unauthorized"], | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| // Should only include user1 and user2 (allowed and not blocked) | ||
| expect(result.assigneesAdded).toEqual(["user1", "user2"]); | ||
| expect(mockGithub.rest.issues.addAssignees).toHaveBeenCalledWith({ | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| issue_number: 123, | ||
| assignees: ["user1", "user2"], | ||
| }); | ||
| }); | ||
|
|
||
| it("should return success with empty array when all assignees are blocked", async () => { | ||
| const { main } = require("./assign_to_user.cjs"); | ||
| const handler = await main({ | ||
| max: 10, | ||
| blocked: ["*[bot]"], | ||
| }); | ||
|
|
||
| const message = { | ||
| type: "assign_to_user", | ||
| assignees: ["dependabot[bot]", "github-actions[bot]"], | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.assigneesAdded).toEqual([]); | ||
| expect(result.message).toContain("No valid assignees found"); | ||
| expect(mockGithub.rest.issues.addAssignees).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,11 +307,54 @@ function extractAssignees(message) { | |
| return requestedAssignees; | ||
| } | ||
|
|
||
| /** | ||
| * Check if a username matches a blocked pattern | ||
| * Supports exact matching and glob-style patterns (e.g., "*[bot]") | ||
| * @param {string} username - The username to check | ||
| * @param {string} pattern - The pattern to match against (e.g., "copilot", "*[bot]") | ||
| * @returns {boolean} True if username matches the blocked pattern | ||
| */ | ||
| function matchesBlockedPattern(username, pattern) { | ||
|
pelikhan marked this conversation as resolved.
|
||
| if (!username || !pattern) { | ||
| return false; | ||
| } | ||
|
|
||
| // Exact match | ||
| if (username === pattern) { | ||
| return true; | ||
| } | ||
|
|
||
| // Simple glob pattern matching | ||
| // Convert glob pattern to regex: "*[bot]" -> "^.*\[bot\]$" | ||
| const regexPattern = pattern | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot review for security
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored in commit 7102cba. The regex pattern escaping now uses the generic |
||
| .replace(/[.+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars except * | ||
| .replace(/\*/g, ".*"); // Convert * to .* | ||
|
|
||
| const regex = new RegExp(`^${regexPattern}$`, "i"); // Case-insensitive | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot review for security
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored in commit 7102cba. The regex construction is now delegated to |
||
| return regex.test(username); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a username is blocked by any pattern in the blocked list | ||
| * @param {string} username - The username to check | ||
| * @param {string[]|undefined} blockedPatterns - Array of blocked patterns (e.g., ["copilot", "*[bot]"]) | ||
| * @returns {boolean} True if username is blocked | ||
| */ | ||
| function isUsernameBlocked(username, blockedPatterns) { | ||
| if (!blockedPatterns || blockedPatterns.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| return blockedPatterns.some(pattern => matchesBlockedPattern(username, pattern)); | ||
| } | ||
|
|
||
| module.exports = { | ||
| parseAllowedItems, | ||
| parseMaxCount, | ||
| resolveTarget, | ||
| loadCustomSafeOutputJobTypes, | ||
| resolveIssueNumber, | ||
| extractAssignees, | ||
| matchesBlockedPattern, | ||
| isUsernameBlocked, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,6 +245,29 @@ function filterByAllowed(items, allowed) { | |
| return items.filter(item => allowed.includes(item)); | ||
| } | ||
|
|
||
| /** | ||
| * Filter out items matching blocked patterns | ||
| * @param {string[]} items - Items to filter | ||
| * @param {string[]|undefined} blockedPatterns - Blocked patterns list (undefined means no blocking) | ||
| * @returns {string[]} Filtered items with blocked items removed | ||
| */ | ||
| function filterByBlocked(items, blockedPatterns) { | ||
|
||
| if (!blockedPatterns || blockedPatterns.length === 0) { | ||
| return items; | ||
| } | ||
|
|
||
| // Import the pattern matching function | ||
| const { isUsernameBlocked } = require("./safe_output_helpers.cjs"); | ||
|
|
||
| return items.filter(item => { | ||
| const blocked = isUsernameBlocked(item, blockedPatterns); | ||
| if (blocked) { | ||
| core.info(`Filtering out blocked item: ${item}`); | ||
| } | ||
| return !blocked; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Limit items to max count | ||
| * @param {string[]} items - Items to limit | ||
|
|
@@ -260,18 +283,22 @@ function limitToMaxCount(items, maxCount) { | |
| } | ||
|
|
||
| /** | ||
| * Process items through the standard pipeline: filter by allowed, sanitize, dedupe, limit | ||
| * Process items through the standard pipeline: filter by allowed, filter blocked, sanitize, dedupe, limit | ||
| * @param {any[]} rawItems - Raw items array from agent output | ||
| * @param {string[]|undefined} allowed - Allowed items list | ||
| * @param {number} maxCount - Maximum number of items | ||
| * @param {string[]|undefined} blocked - Blocked patterns list (optional) | ||
| * @returns {string[]} Processed items | ||
| */ | ||
| function processItems(rawItems, allowed, maxCount) { | ||
| function processItems(rawItems, allowed, maxCount, blocked = undefined) { | ||
| // Filter by allowed list first | ||
| const filtered = filterByAllowed(rawItems, allowed); | ||
|
|
||
| // Filter out blocked items | ||
| const notBlocked = filterByBlocked(filtered, blocked); | ||
|
|
||
| // Sanitize and deduplicate | ||
| const sanitized = sanitizeItems(filtered); | ||
| const sanitized = sanitizeItems(notBlocked); | ||
|
|
||
| // Limit to max count | ||
| return limitToMaxCount(sanitized, maxCount); | ||
|
|
@@ -281,6 +308,7 @@ module.exports = { | |
| processSafeOutput, | ||
| sanitizeItems, | ||
| filterByAllowed, | ||
| filterByBlocked, | ||
| limitToMaxCount, | ||
| processItems, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ const HANDLER_TYPE = "unassign_from_user"; | |
| async function main(config = {}) { | ||
| // Extract configuration | ||
| const allowedAssignees = config.allowed || []; | ||
| const blockedAssignees = config.blocked || []; | ||
|
||
| const maxCount = config.max || 10; | ||
|
|
||
| // Resolve target repository configuration | ||
|
|
@@ -33,6 +34,9 @@ async function main(config = {}) { | |
| if (allowedAssignees.length > 0) { | ||
| core.info(`Allowed assignees to unassign: ${allowedAssignees.join(", ")}`); | ||
| } | ||
| if (blockedAssignees.length > 0) { | ||
| core.info(`Blocked assignees to unassign: ${blockedAssignees.join(", ")}`); | ||
| } | ||
| core.info(`Default target repository: ${defaultTargetRepo}`); | ||
| if (allowedRepos.size > 0) { | ||
| core.info(`Additional allowed repositories: ${Array.from(allowedRepos).join(", ")}`); | ||
|
|
@@ -78,7 +82,7 @@ async function main(config = {}) { | |
| core.info(`Requested assignees to unassign: ${JSON.stringify(requestedAssignees)}`); | ||
|
|
||
| // Use shared helper to filter, sanitize, dedupe, and limit | ||
| const uniqueAssignees = processItems(requestedAssignees, allowedAssignees, maxCount); | ||
| const uniqueAssignees = processItems(requestedAssignees, allowedAssignees, maxCount, blockedAssignees); | ||
|
|
||
| if (uniqueAssignees.length === 0) { | ||
| core.info("No assignees to remove"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition of
blockedAssigneessupport. Consider adding a comment here explaining thatblockedtakes precedence overallowedto clarify the filtering priority for future readers.