diff --git a/actions/setup/js/add_comment.cjs b/actions/setup/js/add_comment.cjs index 4761de52fd..48a550a866 100644 --- a/actions/setup/js/add_comment.cjs +++ b/actions/setup/js/add_comment.cjs @@ -362,6 +362,9 @@ async function main(config = {}) { const maxCount = config.max || 20; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const includeFooter = parseBoolTemplatable(config.footer, true); + const mentionsDisabled = config.mentions === false || config.mentions?.enabled === false; + const configuredMentionAliases = + !mentionsDisabled && Array.isArray(config.mentions?.allowed) ? config.mentions.allowed.map(alias => (typeof alias === "string" ? alias.trim().replace(/^@+/, "") : "")).filter(alias => alias.length > 0) : []; // Create an authenticated GitHub client. Uses config["github-token"] when set // (for cross-repository operations), otherwise falls back to the step-level github. @@ -531,38 +534,59 @@ async function main(config = {}) { // allowed aliases would neutralize those preserved mentions. We re-add the parent entity // author so the second sanitization pass does not accidentally strip them. const parentAuthors = []; - if (!isDiscussion) { - if (explicitItemNumber !== undefined) { - // Explicit item_number/issue_number: fetch the issue/PR to get its author - try { - const { data: issueData } = await githubClient.rest.issues.get({ - owner: repoParts.owner, - repo: repoParts.repo, - issue_number: itemNumber, - }); - if (issueData.user?.login && !isPayloadUserBot(issueData.user)) { - parentAuthors.push(issueData.user.login); + if (!mentionsDisabled) { + if (!isDiscussion) { + if (explicitItemNumber !== undefined) { + // Explicit item_number/issue_number: fetch the issue/PR to get its author + try { + const { data: issueData } = await githubClient.rest.issues.get({ + owner: repoParts.owner, + repo: repoParts.repo, + issue_number: itemNumber, + }); + if (issueData.user?.login && !isPayloadUserBot(issueData.user)) { + parentAuthors.push(issueData.user.login); + } + } catch (err) { + core.info(`Could not fetch parent issue/PR author for mention allowlist: ${getErrorMessage(err)}`); + } + } else { + // Triggering context: use the issue/PR author from the event payload + if (context.payload?.issue?.user?.login && !isPayloadUserBot(context.payload.issue.user)) { + parentAuthors.push(context.payload.issue.user.login); + } + if (context.payload?.pull_request?.user?.login && !isPayloadUserBot(context.payload.pull_request.user)) { + parentAuthors.push(context.payload.pull_request.user.login); } - } catch (err) { - core.info(`Could not fetch parent issue/PR author for mention allowlist: ${getErrorMessage(err)}`); } } else { - // Triggering context: use the issue/PR author from the event payload - if (context.payload?.issue?.user?.login && !isPayloadUserBot(context.payload.issue.user)) { - parentAuthors.push(context.payload.issue.user.login); - } - if (context.payload?.pull_request?.user?.login && !isPayloadUserBot(context.payload.pull_request.user)) { - parentAuthors.push(context.payload.pull_request.user.login); + // Discussion: use the discussion author from the event payload + if (context.payload?.discussion?.user?.login && !isPayloadUserBot(context.payload.discussion.user)) { + parentAuthors.push(context.payload.discussion.user.login); } } - } else { - // Discussion: use the discussion author from the event payload - if (context.payload?.discussion?.user?.login && !isPayloadUserBot(context.payload.discussion.user)) { - parentAuthors.push(context.payload.discussion.user.login); + } + const allowedMentionAliases = []; + const seenAllowedMentionAliases = new Set(); + for (const alias of parentAuthors) { + const key = alias.toLowerCase(); + if (seenAllowedMentionAliases.has(key)) { + continue; } + seenAllowedMentionAliases.add(key); + allowedMentionAliases.push(alias); } - if (parentAuthors.length > 0) { - core.info(`[MENTIONS] Allowing parent entity authors in comment: ${parentAuthors.join(", ")}`); + for (const alias of configuredMentionAliases) { + const key = alias.toLowerCase(); + if (seenAllowedMentionAliases.has(key)) { + continue; + } + seenAllowedMentionAliases.add(key); + allowedMentionAliases.push(alias); + } + + if (allowedMentionAliases.length > 0) { + core.info(`[MENTIONS] Allowing aliases in comment: ${allowedMentionAliases.join(", ")}`); } // Replace temporary ID references in body @@ -570,7 +594,7 @@ async function main(config = {}) { // Sanitize content to prevent injection attacks, allowing parent issue/PR/discussion authors // so they can be @mentioned in the generated comment. - processedBody = sanitizeContent(processedBody, { allowedAliases: parentAuthors }); + processedBody = sanitizeContent(processedBody, { allowedAliases: allowedMentionAliases }); // Enforce max limits before processing (validates user-provided content) try { diff --git a/actions/setup/js/add_comment.test.cjs b/actions/setup/js/add_comment.test.cjs index da9530e509..79bdc7ae36 100644 --- a/actions/setup/js/add_comment.test.cjs +++ b/actions/setup/js/add_comment.test.cjs @@ -2431,6 +2431,114 @@ describe("add_comment", () => { expect(capturedBody).toContain("`@copilot`"); }); + it("should preserve @copilot mention when mentions allowlist includes copilot", async () => { + const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); + + mockContext.payload = { + pull_request: { + number: 8535, + user: { login: "PRAuthor", type: "User" }, + }, + }; + + let capturedBody = null; + mockGithub.rest.issues.createComment = async params => { + capturedBody = params.body; + return { + data: { + id: 12345, + html_url: "https://github.com/owner/repo/issues/8535#issuecomment-12345", + }, + }; + }; + + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["@copilot"] } }); })()`); + + const message = { + type: "add_comment", + body: "@copilot review all comments", + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(capturedBody).toBeDefined(); + expect(capturedBody).toContain("@copilot"); + expect(capturedBody).not.toContain("`@copilot`"); + }); + + it("should escape all mentions when mentions.enabled is false", async () => { + const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); + + mockContext.payload = { + pull_request: { + number: 8535, + user: { login: "PRAuthor", type: "User" }, + }, + }; + + let capturedBody = null; + mockGithub.rest.issues.createComment = async params => { + capturedBody = params.body; + return { + data: { + id: 12345, + html_url: "https://github.com/owner/repo/issues/8535#issuecomment-12345", + }, + }; + }; + + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { enabled: false, allowed: ["@copilot"] } }); })()`); + + const message = { + type: "add_comment", + body: "@copilot ping @PRAuthor", + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(capturedBody).toBeDefined(); + expect(capturedBody).toContain("`@copilot`"); + expect(capturedBody).toContain("`@PRAuthor`"); + }); + + it("should escape all mentions when mentions is false", async () => { + const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); + + mockContext.payload = { + pull_request: { + number: 8535, + user: { login: "PRAuthor", type: "User" }, + }, + }; + + let capturedBody = null; + mockGithub.rest.issues.createComment = async params => { + capturedBody = params.body; + return { + data: { + id: 12345, + html_url: "https://github.com/owner/repo/issues/8535#issuecomment-12345", + }, + }; + }; + + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: false }); })()`); + + const message = { + type: "add_comment", + body: "@copilot ping @PRAuthor", + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(capturedBody).toBeDefined(); + expect(capturedBody).toContain("`@copilot`"); + expect(capturedBody).toContain("`@PRAuthor`"); + }); + it("should fetch and preserve issue author for explicit item_number", async () => { const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index b15d92fa9c..c2c00939c3 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -299,6 +299,12 @@ async function loadHandlers(config, prReviewBuffer) { // Call the factory function with config to get the message handler const handlerConfig = { ...(config[type] || {}) }; + // Pass top-level mentions policy through to add_comment so the handler can + // preserve the same allowed mention aliases used during collection. + if (type === "add_comment" && handlerConfig.mentions == null && config.mentions != null) { + handlerConfig.mentions = config.mentions; + } + // Inject shared PR review buffer into handlers that need it if (PR_REVIEW_HANDLER_TYPES.has(type)) { handlerConfig._prReviewBuffer = prReviewBuffer; diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index f81ace5ba8..d8f7db930b 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -2,8 +2,11 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import fs from "fs"; +import { createRequire } from "module"; import { loadConfig, loadHandlers, processMessages, buildCommentMemoryMessagesFromFiles } from "./safe_output_handler_manager.cjs"; +const require = createRequire(import.meta.url); + describe("Safe Output Handler Manager", () => { beforeEach(() => { // Mock global core @@ -115,6 +118,30 @@ describe("Safe Output Handler Manager", () => { // This test documents the expected behavior for validation expect(true).toBe(true); }); + + it("should pass top-level mentions config into add_comment handler config", async () => { + const addCommentModule = require("./add_comment.cjs"); + const addCommentMainSpy = vi.spyOn(addCommentModule, "main").mockImplementation(async () => async () => ({ success: true })); + + try { + const mentionsConfig = { enabled: true, allowed: ["@copilot"] }; + const handlers = await loadHandlers({ + add_comment: { max: 1 }, + mentions: mentionsConfig, + }); + + expect(handlers.has("add_comment")).toBe(true); + expect(addCommentMainSpy).toHaveBeenCalledTimes(1); + expect(addCommentMainSpy).toHaveBeenCalledWith( + expect.objectContaining({ + max: 1, + mentions: mentionsConfig, + }) + ); + } finally { + addCommentMainSpy.mockRestore(); + } + }); }); describe("loadHandlers - path traversal sanitization", () => {