Skip to content

Commit 66b4cd8

Browse files
CopilotCopilot
authored
fix: apply sanitizeContent to body in create_discussion and create_pull_request handlers (#28053)
1 parent 5291662 commit 66b4cd8

4 files changed

Lines changed: 323 additions & 0 deletions

File tree

‎actions/setup/js/create_discussion.cjs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ const { ERR_VALIDATION } = require("./error_codes.cjs");
1919
const { createExpirationLine, generateFooterWithExpiration, addExpirationToFooter } = require("./ephemerals.cjs");
2020
const { generateFooterWithMessages } = require("./messages_footer.cjs");
2121
const { generateWorkflowIdMarker, generateWorkflowCallIdMarker, generateCloseKeyMarker, normalizeCloseOlderKey } = require("./generate_footer.cjs");
22+
const { sanitizeContent } = require("./sanitize_content.cjs");
2223
const { sanitizeLabelContent } = require("./sanitize_label_content.cjs");
2324
const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs");
2425
const { logStagedPreviewInfo } = require("./staged_preview.cjs");
@@ -482,6 +483,9 @@ async function main(config = {}) {
482483
let processedBody = replaceTemporaryIdReferences(item.body || "", temporaryIdMap, qualifiedItemRepo);
483484
processedBody = removeDuplicateTitleFromDescription(title, processedBody);
484485

486+
// Sanitize body content to neutralize @mentions, URLs, and other security risks
487+
processedBody = sanitizeContent(processedBody);
488+
485489
if (!title) {
486490
title = item.body || "Discussion";
487491
}

‎actions/setup/js/create_discussion_sanitization.test.cjs‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
// @ts-check
2+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
3+
import { createRequire } from "module";
4+
5+
const require = createRequire(import.meta.url);
6+
const { main: createDiscussionMain } = require("./create_discussion.cjs");
7+
8+
describe("create_discussion body sanitization", () => {
9+
let mockGithub;
10+
let mockCore;
11+
let mockContext;
12+
let mockExec;
13+
let originalEnv;
14+
15+
beforeEach(() => {
16+
// Save original environment
17+
originalEnv = { ...process.env };
18+
19+
// Mock GitHub API with discussion categories
20+
mockGithub = {
21+
rest: {},
22+
graphql: vi.fn().mockImplementation((query, variables) => {
23+
// Handle repository query (fetch categories)
24+
if (query.includes("discussionCategories")) {
25+
return Promise.resolve({
26+
repository: {
27+
id: "R_test123",
28+
discussionCategories: {
29+
nodes: [
30+
{
31+
id: "DIC_kwDOGFsHUM4BsUn1",
32+
name: "General",
33+
slug: "general",
34+
description: "General discussions",
35+
},
36+
],
37+
},
38+
},
39+
});
40+
}
41+
// Handle create discussion mutation — echo back the body for assertion
42+
if (query.includes("createDiscussion")) {
43+
return Promise.resolve({
44+
createDiscussion: {
45+
discussion: {
46+
id: "D_test456",
47+
number: 42,
48+
title: variables.title,
49+
url: "https://github.com/test-owner/test-repo/discussions/42",
50+
},
51+
},
52+
});
53+
}
54+
return Promise.reject(new Error("Unknown GraphQL query"));
55+
}),
56+
};
57+
58+
// Mock Core
59+
mockCore = {
60+
info: vi.fn(),
61+
warning: vi.fn(),
62+
error: vi.fn(),
63+
setOutput: vi.fn(),
64+
};
65+
66+
// Mock Context
67+
mockContext = {
68+
repo: { owner: "test-owner", repo: "test-repo" },
69+
runId: 12345,
70+
payload: {
71+
repository: {
72+
html_url: "https://github.com/test-owner/test-repo",
73+
},
74+
},
75+
};
76+
77+
// Mock Exec
78+
mockExec = {
79+
exec: vi.fn().mockResolvedValue(0),
80+
};
81+
82+
// Set globals
83+
global.github = mockGithub;
84+
global.core = mockCore;
85+
global.context = mockContext;
86+
global.exec = mockExec;
87+
88+
// Set required environment variables
89+
process.env.GH_AW_WORKFLOW_NAME = "Test Workflow";
90+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
91+
process.env.GH_AW_WORKFLOW_SOURCE_URL = "https://github.com/owner/repo/blob/main/workflow.md";
92+
process.env.GITHUB_SERVER_URL = "https://github.com";
93+
});
94+
95+
afterEach(() => {
96+
// Restore environment by mutating process.env in place
97+
for (const key of Object.keys(process.env)) {
98+
if (!(key in originalEnv)) {
99+
delete process.env[key];
100+
}
101+
}
102+
Object.assign(process.env, originalEnv);
103+
vi.clearAllMocks();
104+
});
105+
106+
it("should neutralize @mentions in discussion body", async () => {
107+
const handler = await createDiscussionMain({ max: 5, category: "general" });
108+
const result = await handler(
109+
{
110+
title: "Test Discussion",
111+
body: "This was reported by @malicious-user and CC @another-user.",
112+
},
113+
{}
114+
);
115+
116+
expect(result.success).toBe(true);
117+
118+
const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion"));
119+
expect(createMutationCall).toBeDefined();
120+
const body = createMutationCall[1].body;
121+
// @mentions must be neutralized to backtick form
122+
expect(body).toContain("`@malicious-user`");
123+
expect(body).toContain("`@another-user`");
124+
// Raw unescaped @mentions must not appear
125+
expect(body).not.toMatch(/(?<![`])@malicious-user(?![`])/);
126+
expect(body).not.toMatch(/(?<![`])@another-user(?![`])/);
127+
});
128+
129+
it("should expose hidden markdown link title XPIA payloads in discussion body (closing the XPIA channel)", async () => {
130+
const handler = await createDiscussionMain({ max: 5, category: "general" });
131+
const result = await handler(
132+
{
133+
title: "Security Test",
134+
body: 'Click [here](https://example.com "XPIA hidden payload") for details.',
135+
},
136+
{}
137+
);
138+
139+
expect(result.success).toBe(true);
140+
141+
const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion"));
142+
expect(createMutationCall).toBeDefined();
143+
const body = createMutationCall[1].body;
144+
// The hidden title must be moved to visible text (closing the XPIA channel)
145+
// sanitizeContent converts [text](url "hidden") → [text (hidden)](url)
146+
expect(body).toContain("XPIA hidden payload");
147+
// The payload must no longer be in a hidden markdown link title attribute
148+
expect(body).not.toMatch(/\[.*\]\([^)]*"XPIA hidden payload"[^)]*\)/);
149+
});
150+
151+
it("should sanitize body but preserve the footer workflow marker", async () => {
152+
const handler = await createDiscussionMain({ max: 5, category: "general" });
153+
const result = await handler(
154+
{
155+
title: "Test Discussion",
156+
body: "Please notify @someone about this finding.",
157+
},
158+
{}
159+
);
160+
161+
expect(result.success).toBe(true);
162+
163+
const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion"));
164+
expect(createMutationCall).toBeDefined();
165+
const body = createMutationCall[1].body;
166+
// @mention must be neutralized
167+
expect(body).toContain("`@someone`");
168+
// System-generated footer marker must still be present
169+
expect(body).toContain("gh-aw-workflow-id");
170+
});
171+
});

‎actions/setup/js/create_pull_request.cjs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const { pushSignedCommits } = require("./push_signed_commits.cjs");
1010
const { getTrackerID } = require("./get_tracker_id.cjs");
1111
const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs");
1212
const { sanitizeTitle, applyTitlePrefix } = require("./sanitize_title.cjs");
13+
const { sanitizeContent } = require("./sanitize_content.cjs");
1314
const { getErrorMessage } = require("./error_helpers.cjs");
1415
const { replaceTemporaryIdReferences, replaceTemporaryIdReferencesInPatch, getOrGenerateTemporaryId } = require("./temporary_id.cjs");
1516
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
@@ -851,6 +852,9 @@ async function main(config = {}) {
851852
// Remove duplicate title from description if it starts with a header matching the title
852853
processedBody = removeDuplicateTitleFromDescription(title, processedBody);
853854

855+
// Sanitize body content to neutralize @mentions, URLs, and other security risks
856+
processedBody = sanitizeContent(processedBody);
857+
854858
// Auto-add "Fixes #N" closing keyword if triggered from an issue and not already present.
855859
// This ensures the triggering issue is auto-closed when the PR is merged.
856860
// Agents are instructed to include this but don't reliably do so.

‎actions/setup/js/create_pull_request_sanitization.test.cjs‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
// @ts-check
2+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
3+
import { createRequire } from "module";
4+
import * as fs from "fs";
5+
import * as path from "path";
6+
import * as os from "os";
7+
8+
const require = createRequire(import.meta.url);
9+
10+
describe("create_pull_request - body sanitization", () => {
11+
let tempDir;
12+
let originalEnv;
13+
14+
beforeEach(() => {
15+
originalEnv = { ...process.env };
16+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
17+
process.env.GITHUB_REPOSITORY = "test-owner/test-repo";
18+
process.env.GITHUB_BASE_REF = "main";
19+
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-sanitize-test-"));
20+
21+
global.core = {
22+
info: vi.fn(),
23+
warning: vi.fn(),
24+
error: vi.fn(),
25+
debug: vi.fn(),
26+
setFailed: vi.fn(),
27+
setOutput: vi.fn(),
28+
startGroup: vi.fn(),
29+
endGroup: vi.fn(),
30+
summary: {
31+
addRaw: vi.fn().mockReturnThis(),
32+
write: vi.fn().mockResolvedValue(undefined),
33+
},
34+
};
35+
global.github = {
36+
rest: {
37+
pulls: {
38+
create: vi.fn().mockResolvedValue({ data: { number: 1, html_url: "https://github.com/test" } }),
39+
},
40+
repos: {
41+
get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }),
42+
},
43+
issues: {
44+
addLabels: vi.fn().mockResolvedValue({}),
45+
},
46+
},
47+
graphql: vi.fn(),
48+
};
49+
global.context = {
50+
eventName: "workflow_dispatch",
51+
repo: { owner: "test-owner", repo: "test-repo" },
52+
payload: {},
53+
};
54+
global.exec = {
55+
exec: vi.fn().mockResolvedValue(0),
56+
getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }),
57+
};
58+
59+
// Clear module cache so globals are picked up fresh
60+
delete require.cache[require.resolve("./create_pull_request.cjs")];
61+
});
62+
63+
afterEach(() => {
64+
for (const key of Object.keys(process.env)) {
65+
if (!(key in originalEnv)) {
66+
delete process.env[key];
67+
}
68+
}
69+
Object.assign(process.env, originalEnv);
70+
71+
if (tempDir && fs.existsSync(tempDir)) {
72+
fs.rmSync(tempDir, { recursive: true, force: true });
73+
}
74+
75+
delete global.core;
76+
delete global.github;
77+
delete global.context;
78+
delete global.exec;
79+
vi.clearAllMocks();
80+
});
81+
82+
it("should neutralize @mentions in PR body", async () => {
83+
const { main } = require("./create_pull_request.cjs");
84+
const handler = await main({ allow_empty: true });
85+
86+
await handler(
87+
{
88+
title: "Test PR",
89+
body: "This PR fixes a bug reported by @malicious-user and reviewed by @another-user.",
90+
},
91+
{}
92+
);
93+
94+
const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0];
95+
expect(createCall).toBeDefined();
96+
// @mentions must be neutralized to backtick form
97+
expect(createCall.body).toContain("`@malicious-user`");
98+
expect(createCall.body).toContain("`@another-user`");
99+
// Raw unescaped @mentions must not appear
100+
expect(createCall.body).not.toMatch(/(?<![`])@malicious-user(?![`])/);
101+
expect(createCall.body).not.toMatch(/(?<![`])@another-user(?![`])/);
102+
});
103+
104+
it("should expose hidden markdown link title XPIA payloads in PR body (closing the XPIA channel)", async () => {
105+
const { main } = require("./create_pull_request.cjs");
106+
const handler = await main({ allow_empty: true });
107+
108+
await handler(
109+
{
110+
title: "Security Fix",
111+
body: 'See [the report](https://example.com "XPIA hidden payload") for context.',
112+
},
113+
{}
114+
);
115+
116+
const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0];
117+
expect(createCall).toBeDefined();
118+
// The hidden title must be moved to visible text (closing the XPIA channel)
119+
// sanitizeContent converts [text](url "hidden") → [text (hidden)](url)
120+
expect(createCall.body).toContain("XPIA hidden payload");
121+
// The payload must no longer be in a hidden markdown link title attribute
122+
expect(createCall.body).not.toMatch(/\[.*\]\([^)]*"XPIA hidden payload"[^)]*\)/);
123+
});
124+
125+
it("should sanitize body but preserve the footer workflow marker", async () => {
126+
const { main } = require("./create_pull_request.cjs");
127+
const handler = await main({ allow_empty: true });
128+
129+
await handler(
130+
{
131+
title: "Test PR",
132+
body: "Please notify @someone about this change.",
133+
},
134+
{}
135+
);
136+
137+
const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0];
138+
expect(createCall).toBeDefined();
139+
// @mention must be neutralized
140+
expect(createCall.body).toContain("`@someone`");
141+
// System-generated footer marker must still be present
142+
expect(createCall.body).toContain("gh-aw-workflow-id");
143+
});
144+
});

0 commit comments

Comments
 (0)