Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 5 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
12 changes: 12 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,18 @@ 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 treat a four-space-indented fence as a fenced block", () => {
const input = " ```\n@octocat\n ```";
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(/(^|[^A-Za-z0-9])@([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