Skip to content

Preserve allowlisted mentions in add_comment sanitization (@copilot regression) #32683

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.

Sign up for GitHub

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

Merged
pelikhan merged 4 commits into from
May 16, 2026
+191 −26
Merged

Preserve allowlisted mentions in add_comment sanitization (@copilot regression) #32683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0b9a6e
Plan add_comment mention allowlist fix
Copilot May 16, 2026
1450a2b
Pass mentions allowlist into add_comment sanitization
Copilot May 16, 2026
76a1ca4
Refine mention allowlist normalization in add_comment
Copilot May 16, 2026
e7bc146
Address review feedback on mention policy and tests
Copilot May 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 50 additions & 26 deletions actions/setup/js/add_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,9 @@ async function main(config = {}) {
const maxCount = config.max || 20;
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The PR description references a mergeUniqueAliases helper but the implementation inlines the two-loop deduplication instead. The inline approach works correctly, but the logic in add_comment.cjs (lines 562–585) is non-trivial enough that a named, exported helper would:

  1. Enable isolated unit tests for the merge/dedupe logic without spinning up the full handler
  2. Make the call site at line 590 read like the description's pseudocode

Not blocking — just noting the gap between the stated design and what landed.

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.
Expand Down Expand Up @@ -531,46 +534,67 @@ 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
let processedBody = replaceTemporaryIdReferences(message.body || "", temporaryIdMap, itemRepo);

// 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 {
Expand Down
108 changes: 108 additions & 0 deletions actions/setup/js/add_comment.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
};

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new test only exercises @copilot (with the @ prefix), but the normalization in add_comment.cjs also strips leading @ so both "@copilot" and "copilot" should resolve to the same alias. A complementary test case using mentions: { allowed: ["copilot"] } (no @) would lock down the normalization path and prevent a future regression if the .replace(/^@+/, "") logic changes.

it("should preserve `@copilot` mention when allowlist entry omits @ prefix", async () => {
  // ... same setup ...
  const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["copilot"] } }); })()`)
  const result = await handler({ type: "add_comment", body: "`@copilot` review" }, {})
  expect(capturedBody).toContain("`@copilot`")
  expect(capturedBody).not.toContain("`@copilot`")
})

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(capturedBody).toBeDefined();
expect(capturedBody).toContain("@copilot");
expect(capturedBody).not.toContain("`@copilot`");
});

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The deduplication path (where an alias appears in both parentAuthors and mentions.allowed) isn't exercised. If parentAuthors already contains "copilot" and mentions.allowed also lists "@copilot", the merged list should still contain only one @copilot in the sanitized output. A test covering this would lock down the seenAllowedMentionAliases Set logic:

it("should not duplicate alias when it appears in both parentAuthors and mentions.allowed", async () => {
  // payload where PR author is also in mentions.allowed
  mockContext.payload = {
    pull_request: { number: 1, user: { login: "copilot", type: "User" } },
  }
  const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["`@copilot`"] } }); })()`)
  const result = await handler({ type: "add_comment", body: "`@copilot` thanks" }, {})
  // `@copilot` should appear exactly once, not escaped
  expect(capturedBody.match(/`@copilot/g`)).toHaveLength(1)
})

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");

Expand Down
6 changes: 6 additions & 0 deletions actions/setup/js/safe_output_handler_manager.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The type === "add_comment" guard is a targeted fix that works for the immediate regression, but config propagation is now inconsistent: add_comment gets mentions forwarded; other handlers that may grow similar needs in the future won't. An alternative that generalises without requiring per-type patches:

// Merge top-level keys that the handler config doesn't already override
const handlerConfig = { ...config, ...(config[type] || {}) };

This follows the same shallow-merge pattern already used elsewhere and removes the need for the handler-type guard. Worth considering if a second handler ever needs a top-level field.

// preserve the same allowed mention aliases used during collection.
if (type === "add_comment" && handlerConfig.mentions == null && config.mentions != null) {
handlerConfig.mentions = config.mentions;
}
Comment on lines +302 to +306

// Inject shared PR review buffer into handlers that need it
if (PR_REVIEW_HANDLER_TYPES.has(type)) {
handlerConfig._prReviewBuffer = prReviewBuffer;
Expand Down
27 changes: 27 additions & 0 deletions actions/setup/js/safe_output_handler_manager.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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", () => {
Expand Down