Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions .changeset/patch-fix-safe-output-inline-backticks.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 20 additions & 3 deletions actions/setup/js/markdown_code_region_balancer.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@
* @module markdown_code_region_balancer
*/

/**
* Matches a CommonMark fenced-code delimiter line.
* @param {string} line
* @returns {RegExpMatchArray | null}
*/
function matchFenceLine(line) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] matchFenceLine now correctly limits leading indentation to {0,3} spaces per CommonMark spec (the old \s* was over-permissive), but there is no regression test covering the 4-space case. A line indented by 4+ spaces should not be treated as a fence and should be left untouched.

💡 Suggested test
it('should not treat a 4-space indented line as a fence', () => {
  const input = '    ```\ncode\n    ```';
  expect(balancer.isBalanced(input)).toBe(true);
  expect(balancer.balanceCodeRegions(input)).toBe(input);
});

@copilot please address this.

const match = line.match(/^( {0,3})(`{3,}|~{3,})([^`~\s]*)?(.*)$/);
if (!match) {
return null;
}
const infoString = `${match[3] || ""}${match[4] || ""}`;
if (match[2][0] === "`" && infoString.includes("`")) {
return null;
}
return match;
}

/**
* Balance markdown code regions by attempting to fix mismatched fences.
*
Expand Down Expand Up @@ -106,7 +123,7 @@ function balanceCodeRegions(markdown) {
for (let i = 0; i < lines.length; i++) {
if (isInXmlComment(i)) continue;

const fenceMatch = lines[i].match(/^(\s*)(`{3,}|~{3,})([^`~\s]*)?(.*)$/);
const fenceMatch = matchFenceLine(lines[i]);
if (fenceMatch) {
fences.push({
lineIndex: i,
Expand Down Expand Up @@ -340,7 +357,7 @@ function isBalanced(markdown) {
let openingFence = null;

for (const line of lines) {
const fenceMatch = line.match(/^(\s*)(`{3,}|~{3,})([^`~\s]*)?(.*)$/);
const fenceMatch = matchFenceLine(line);

if (fenceMatch) {
const fence = fenceMatch[2];
Expand Down Expand Up @@ -392,7 +409,7 @@ function countCodeRegions(markdown) {
let openingFence = null;

for (const line of lines) {
const fenceMatch = line.match(/^(\s*)(`{3,}|~{3,})([^`~\s]*)?(.*)$/);
const fenceMatch = matchFenceLine(line);

if (fenceMatch) {
const fence = fenceMatch[2];
Expand Down
6 changes: 6 additions & 0 deletions actions/setup/js/markdown_code_region_balancer.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ More content.`;
expect(balancer.balanceCodeRegions(input)).toBe(input);
});

it("should not treat backticks in an info string as a fenced block", () => {
const input = "```x```@octocat";
expect(balancer.balanceCodeRegions(input)).toBe(input);
expect(balancer.isBalanced(input)).toBe(true);
});

it("should not modify properly balanced code blocks", () => {
const input = `# Title

Expand Down
33 changes: 19 additions & 14 deletions actions/setup/js/sanitize_content.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const {
buildAllowedGitHubReferences,
getCurrentRepoSlug,
applyURLSanitizationPolicy,
createRenderSafeCodeSpanWrapper,
neutralizeCommands,
neutralizeGitHubReferences,
removeXmlComments,
Expand Down Expand Up @@ -112,9 +113,6 @@ function sanitizeContent(content, maxLengthOrOptions) {
// removeXmlComments.
sanitized = applyToNonCodeRegions(sanitized, neutralizeMarkdownLinkTitles);

// Neutralize @mentions with selective filtering (custom logic for allowed aliases)
sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase);

// Convert XML tags – skip code blocks and inline code
sanitized = applyToNonCodeRegions(sanitized, convertXmlTags);

Expand All @@ -124,6 +122,10 @@ function sanitizeContent(content, maxLengthOrOptions) {
// Apply truncation limits (shared with core)
sanitized = applyTruncation(sanitized, maxLength);

// Neutralize mentions after truncation so the length boundary cannot split an
// inserted code-span delimiter and reactivate a mention.
sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase);

// Neutralize GitHub references if restrictions are configured
sanitized = neutralizeGitHubReferences(sanitized, allowedGitHubRefs);

Expand Down Expand Up @@ -186,17 +188,20 @@ function sanitizeContent(content, maxLengthOrOptions) {
* @returns {string} Processed string
*/
function neutralizeMentions(s, allowedLowercase) {
return s.replace(/(^|[^\w`])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (_m, p1, p2) => {
// Check if this mention is in the allowed aliases list (case-insensitive)
const isAllowed = allowedLowercase.includes(p2.toLowerCase());
if (isAllowed) {
return `${p1}@${p2}`; // Keep the original mention
}
// Log when a mention is escaped
if (typeof core !== "undefined" && core.info) {
core.info(`Escaped mention: @${p2} (not in allowed list)`);
}
return `${p1}\`@${p2}\``; // Neutralize the mention
const wrapInCodeSpan = createRenderSafeCodeSpanWrapper(s);
return applyToNonCodeRegions(s, (segment, regionBefore = "", regionAfter = "") => {
return segment.replace(/(^|[^\w])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (match, prefix, alias, offset) => {
const isAllowed = allowedLowercase.includes(alias.toLowerCase());
if (isAllowed) {
return `${prefix}@${alias}`;
}
if (typeof core !== "undefined" && core.info) {
core.info(`Escaped mention: @${alias} (not in allowed list)`);
}
const before = prefix || (offset === 0 ? regionBefore : "");
const after = segment[offset + match.length] || (offset + match.length === segment.length ? regionAfter : "");
return `${prefix}${wrapInCodeSpan(`@${alias}`, before, after)}`;
});
});
}
}
Expand Down
82 changes: 81 additions & 1 deletion actions/setup/js/sanitize_content.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ describe("sanitize_content.cjs", () => {
const result = sanitizeContent("/smoke-copilot-sdk run tests");
expect(result).toBe("`/smoke-copilot-sdk` run tests");
});

it("should keep commands neutralized when the body contains attacker backticks", () => {
const result = sanitizeContent("/bot run ` later");
expect(result).toBe("``/bot`` run ` later");
});
});

describe("@mention neutralization", () => {
Expand Down Expand Up @@ -139,6 +144,42 @@ describe("sanitize_content.cjs", () => {
expect(result).toBe("Hello `@user_name_test`");
});

it("should use a distinct delimiter for mentions after unmatched backticks", () => {
expect(sanitizeContent("note ` @octocat done")).toBe("note ` ``@octocat`` done");
});

it("should preserve mentions already contained by matched attacker backticks", () => {
expect(sanitizeContent("start ` mid @octocat end ` tail")).toBe("start ` mid @octocat end ` tail");
});

it("should choose a delimiter length absent from the complete input", () => {
expect(sanitizeContent("one ` two `` @octocat done")).toBe("one ` two `` ```@octocat``` done");
});

it("should neutralize mentions adjacent to unmatched backticks", () => {
expect(sanitizeContent("`@octocat")).toBe("` ``@octocat``");
expect(sanitizeContent("``@octocat`")).toBe("`` ```@octocat``` `");
expect(sanitizeContent("@octocat`")).toBe("``@octocat`` `");
});

it("should preserve mentions inside matched code spans", () => {
expect(sanitizeContent("`@octocat`")).toBe("`@octocat`");
});

it("should separate neutralized mentions from adjacent matched code spans", () => {
expect(sanitizeContent("`x`@octocat @other")).toBe("`x` ``@octocat`` ``@other``");
expect(sanitizeContent("@octocat`x`")).toBe("``@octocat`` `x`");
});

it("should not treat inline code with trailing prose as a fenced block", () => {
expect(sanitizeContent("```x```@octocat")).toBe("```x``` `@octocat`");
});

it("should neutralize a mention after truncating its alias", () => {
expect(sanitizeContent("123456 @octocat", 10)).toBe("123456 `@oc`\n[Content truncated due to length]");
expect(sanitizeContent("123456 @octocat", { maxLength: 10, allowedAliases: ["author"] })).toBe("123456 `@oc`\n[Content truncated due to length]");
});

it("should neutralize @mentions with underscores and hyphens", () => {
const result = sanitizeContent("Hello @user-name_test");
expect(result).toBe("Hello `@user-name_test`");
Expand Down Expand Up @@ -1755,6 +1796,21 @@ describe("sanitize_content.cjs", () => {
// The 12th entry (11th unquoted) is wrapped
expect(result).toContain("`fixes #12`");
});

it("should keep excess bot triggers neutralized when preceded by attacker backticks", () => {
const result = sanitizeContent("prefix ` fixes #1", { maxBotMentions: 0 });
expect(result).toBe("prefix ` ``fixes #1``");
});

it("should neutralize excess bot triggers adjacent to unmatched backticks", () => {
expect(sanitizeContent("`fixes #1", { maxBotMentions: 0 })).toBe("` ``fixes #1``");
expect(sanitizeContent("fixes #1`", { maxBotMentions: 0 })).toBe("``fixes #1`` `");
});

it("should separate excess bot triggers from adjacent matched code spans", () => {
expect(sanitizeContent("`x`fixes #1", { maxBotMentions: 0 })).toBe("`x` ``fixes #1``");
expect(sanitizeContent("```x```fixes #1", { maxBotMentions: 0 })).toBe("```x``` `fixes #1`");
});
});

describe("GitHub reference neutralization", () => {
Expand Down Expand Up @@ -1782,6 +1838,30 @@ describe("sanitize_content.cjs", () => {
expect(result).toBe("See issue #123 and `other/repo#456`");
});

it("should keep restricted references neutralized when preceded by attacker backticks", () => {
process.env.GITHUB_REPOSITORY = "myorg/myrepo";
process.env.GH_AW_ALLOWED_GITHUB_REFS = "repo";

const result = sanitizeContent("see ` other/repo#1337 now");
expect(result).toBe("see ` ``other/repo#1337`` now");
});

it("should neutralize restricted references adjacent to unmatched backticks", () => {
process.env.GITHUB_REPOSITORY = "myorg/myrepo";
process.env.GH_AW_ALLOWED_GITHUB_REFS = "repo";

expect(sanitizeContent("`other/repo#1337")).toBe("` ``other/repo#1337``");
expect(sanitizeContent("other/repo#1337`")).toBe("``other/repo#1337`` `");
});

it("should separate restricted references from adjacent matched code spans", () => {
process.env.GITHUB_REPOSITORY = "myorg/myrepo";
process.env.GH_AW_ALLOWED_GITHUB_REFS = "repo";

expect(sanitizeContent("`x`other/repo#1337")).toBe("`x` ``other/repo#1337``");
expect(sanitizeContent("```x```other/repo#1337")).toBe("```x``` `other/repo#1337`");
});

it("should allow current repo references with 'repo' keyword", () => {
process.env.GITHUB_REPOSITORY = "myorg/myrepo";
process.env.GH_AW_ALLOWED_GITHUB_REFS = "repo";
Expand Down Expand Up @@ -2111,7 +2191,7 @@ describe("sanitize_content.cjs", () => {

it("should handle nested backticks", () => {
const result = sanitizeContent("Already `@user` and @other");
expect(result).toBe("Already `@user` and `@other`");
expect(result).toBe("Already `@user` and ``@other``");
});
});

Expand Down
Loading
Loading