-
Notifications
You must be signed in to change notification settings - Fork 232
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 all 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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| 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 |
|---|---|---|
| @@ -1,13 +1,30 @@ | ||
| // @ts-check | ||
|
|
||
| /** | ||
| * Internal helper to escape special regex characters in a pattern | ||
| * @param {string} pattern - Pattern to escape | ||
| * @returns {string} - Pattern with special characters escaped | ||
| * @private | ||
| */ | ||
| function escapeRegexChars(pattern) { | ||
| // Escape backslashes first, then dots, then other special chars | ||
| return pattern | ||
| .replace(/\\/g, "\\\\") // Escape backslashes | ||
| .replace(/\./g, "\\.") // Escape dots | ||
| .replace(/[+?^${}()|[\]]/g, "\\$&"); // Escape other special regex chars (except * which is handled separately) | ||
| } | ||
|
|
||
| /** | ||
| * Convert a glob pattern to a RegExp | ||
| * @param {string} pattern - Glob pattern (e.g., "*.json", "metrics/**", "data/**\/*.csv") | ||
| * @param {Object} [options] - Options for pattern conversion | ||
| * @param {boolean} [options.pathMode=true] - If true, * matches non-slash chars; if false, * matches any char | ||
| * @param {boolean} [options.caseSensitive=true] - Whether matching should be case-sensitive | ||
| * @returns {RegExp} - Regular expression that matches the pattern | ||
| * | ||
| * Supports: | ||
| * - * matches any characters except / | ||
| * - ** matches any characters including / | ||
| * - * matches any characters except / (in path mode) or any characters (in simple mode) | ||
| * - ** matches any characters including / (only in path mode) | ||
| * - . is escaped to match literal dots | ||
| * - \ is escaped properly | ||
| * | ||
|
|
@@ -21,17 +38,23 @@ | |
| * regex.test("metrics/data.json"); // true | ||
| * regex.test("metrics/daily/data.json"); // true | ||
| */ | ||
| function globPatternToRegex(pattern) { | ||
| // Convert glob pattern to regex that supports directory wildcards | ||
| // ** matches any path segment (including /) | ||
| // * matches any characters except / | ||
| let regexPattern = pattern | ||
| .replace(/\\/g, "\\\\") // Escape backslashes | ||
| .replace(/\./g, "\\.") // Escape dots | ||
| .replace(/\*\*/g, "<!DOUBLESTAR>") // Temporarily replace ** | ||
| .replace(/\*/g, "[^/]*") // Single * matches non-slash chars | ||
| .replace(/<!DOUBLESTAR>/g, ".*"); // ** matches everything including / | ||
| return new RegExp(`^${regexPattern}$`); | ||
| function globPatternToRegex(pattern, options) { | ||
| const { pathMode = true, caseSensitive = true } = options || {}; | ||
|
|
||
| let regexPattern = escapeRegexChars(pattern); | ||
|
|
||
| if (pathMode) { | ||
| // Path mode: handle ** and * differently | ||
| regexPattern = regexPattern | ||
| .replace(/\*\*/g, "<!DOUBLESTAR>") // Temporarily replace ** | ||
| .replace(/\*/g, "[^/]*") // Single * matches non-slash chars | ||
| .replace(/<!DOUBLESTAR>/g, ".*"); // ** matches everything including / | ||
| } else { | ||
| // Simple mode: * matches any character | ||
| regexPattern = regexPattern.replace(/\*/g, ".*"); | ||
| } | ||
|
|
||
| return new RegExp(`^${regexPattern}$`, caseSensitive ? "" : "i"); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -45,7 +68,11 @@ function globPatternToRegex(pattern) { | |
| * patterns[1].test("file.jsonl"); // true | ||
| */ | ||
| function parseGlobPatterns(fileGlobFilter) { | ||
| return fileGlobFilter.trim().split(/\s+/).filter(Boolean).map(globPatternToRegex); | ||
| return fileGlobFilter | ||
| .trim() | ||
| .split(/\s+/) | ||
| .filter(Boolean) | ||
| .map(pattern => globPatternToRegex(pattern)); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -63,8 +90,64 @@ function matchesGlobPattern(filePath, fileGlobFilter) { | |
| return patterns.some(pattern => pattern.test(filePath)); | ||
| } | ||
|
|
||
| /** | ||
|
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. @copilot deduplicate with globPatternToRegex
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. Deduplicated in commit 0626e1a. The |
||
| * Convert a simple glob pattern to a RegExp (for non-path matching) | ||
| * @param {string} pattern - Glob pattern (e.g., "copilot", "*[bot]") | ||
| * @param {boolean} caseSensitive - Whether matching should be case-sensitive (default: false) | ||
| * @returns {RegExp} - Regular expression that matches the pattern | ||
| * | ||
| * Supports: | ||
| * - * matches any characters (not limited to non-slash like path mode) | ||
| * - Escapes special regex characters except * | ||
| * - Case-insensitive by default | ||
| * | ||
| * @example | ||
| * const regex = simpleGlobToRegex("*[bot]"); | ||
| * regex.test("dependabot[bot]"); // true | ||
| * regex.test("github-actions[bot]"); // true | ||
| * | ||
| * @example | ||
| * const regex = simpleGlobToRegex("copilot"); | ||
| * regex.test("copilot"); // true | ||
| * regex.test("Copilot"); // true (case-insensitive) | ||
| */ | ||
| function simpleGlobToRegex(pattern, caseSensitive = false) { | ||
| return globPatternToRegex(pattern, { pathMode: false, caseSensitive }); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a string matches a simple glob pattern | ||
| * @param {string} str - String to test (e.g., "copilot", "dependabot[bot]") | ||
| * @param {string} pattern - Glob pattern (e.g., "copilot", "*[bot]") | ||
| * @param {boolean} caseSensitive - Whether matching should be case-sensitive (default: false) | ||
| * @returns {boolean} - True if the string matches the pattern | ||
| * | ||
| * @example | ||
| * matchesSimpleGlob("dependabot[bot]", "*[bot]"); // true | ||
| * matchesSimpleGlob("copilot", "copilot"); // true | ||
| * matchesSimpleGlob("Copilot", "copilot"); // true (case-insensitive by default) | ||
| * matchesSimpleGlob("alice", "*[bot]"); // false | ||
| */ | ||
| function matchesSimpleGlob(str, pattern, caseSensitive = false) { | ||
| if (!str || !pattern) { | ||
| return false; | ||
| } | ||
|
|
||
| // Exact match check (case-insensitive by default) | ||
| if (!caseSensitive && str.toLowerCase() === pattern.toLowerCase()) { | ||
| return true; | ||
| } else if (caseSensitive && str === pattern) { | ||
| return true; | ||
| } | ||
|
|
||
| const regex = simpleGlobToRegex(pattern, caseSensitive); | ||
| return regex.test(str); | ||
| } | ||
|
|
||
| module.exports = { | ||
| globPatternToRegex, | ||
| parseGlobPatterns, | ||
| matchesGlobPattern, | ||
| simpleGlobToRegex, | ||
| matchesSimpleGlob, | ||
| }; | ||
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.