Skip to content

Commit 06f4f0a

Browse files
CopilotCopilot
authored
SEC-004: Add content sanitization to safe-output handlers (#15807)
1 parent 56e1ce9 commit 06f4f0a

30 files changed

+288
-49
lines changed

‎.changeset/patch-sanitize-safe-output-handlers.md‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎actions/setup/js/add_comment.cjs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ const { resolveTarget } = require("./safe_output_helpers.cjs");
1414
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
1515
const { getMissingInfoSections } = require("./missing_messages_helper.cjs");
1616
const { getMessages } = require("./messages_core.cjs");
17+
const { sanitizeContent } = require("./sanitize_content.cjs");
1718

1819
/** @type {string} Safe output type handled by this module */
1920
const HANDLER_TYPE = "add_comment";
@@ -463,6 +464,9 @@ async function main(config = {}) {
463464
// Replace temporary ID references in body
464465
let processedBody = replaceTemporaryIdReferences(item.body || "", temporaryIdMap, itemRepo);
465466

467+
// Sanitize content to prevent injection attacks
468+
processedBody = sanitizeContent(processedBody);
469+
466470
// Enforce max limits before processing (validates user-provided content)
467471
try {
468472
enforceCommentLimits(processedBody);

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

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,6 +1098,122 @@ describe("add_comment", () => {
10981098
expect(capturedBody).not.toContain("aw_test02");
10991099
});
11001100
});
1101+
1102+
describe("sanitization preserves markers", () => {
1103+
it("should preserve tracker ID markers after sanitization", async () => {
1104+
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");
1105+
1106+
// Setup environment
1107+
process.env.GH_AW_WORKFLOW_NAME = "Test Workflow";
1108+
process.env.GH_AW_TRACKER_ID = "test-tracker-123";
1109+
1110+
let capturedBody = null;
1111+
mockGithub.rest.issues.createComment = async params => {
1112+
capturedBody = params.body;
1113+
return {
1114+
data: {
1115+
id: 12345,
1116+
html_url: "https://github.com/owner/repo/issues/42#issuecomment-12345",
1117+
},
1118+
};
1119+
};
1120+
1121+
// Execute the handler
1122+
const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);
1123+
1124+
const message = {
1125+
type: "add_comment",
1126+
body: "User content with <script>alert('xss')</script> attempt",
1127+
};
1128+
1129+
const result = await handler(message, {});
1130+
1131+
expect(result.success).toBe(true);
1132+
expect(capturedBody).toBeDefined();
1133+
// Verify tracker ID is present (not removed by sanitization)
1134+
expect(capturedBody).toContain("<!-- gh-aw-tracker-id: test-tracker-123 -->");
1135+
// Verify script tags were sanitized (converted to safe format)
1136+
expect(capturedBody).not.toContain("<script>");
1137+
expect(capturedBody).toContain("(script)"); // Tags converted to parentheses
1138+
1139+
delete process.env.GH_AW_WORKFLOW_NAME;
1140+
delete process.env.GH_AW_TRACKER_ID;
1141+
});
1142+
1143+
it("should preserve workflow footer after sanitization", async () => {
1144+
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");
1145+
1146+
// Setup environment
1147+
process.env.GH_AW_WORKFLOW_NAME = "Security Test Workflow";
1148+
1149+
let capturedBody = null;
1150+
mockGithub.rest.issues.createComment = async params => {
1151+
capturedBody = params.body;
1152+
return {
1153+
data: {
1154+
id: 12345,
1155+
html_url: "https://github.com/owner/repo/issues/42#issuecomment-12345",
1156+
},
1157+
};
1158+
};
1159+
1160+
// Execute the handler
1161+
const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);
1162+
1163+
const message = {
1164+
type: "add_comment",
1165+
body: "User content <!-- malicious comment -->",
1166+
};
1167+
1168+
const result = await handler(message, {});
1169+
1170+
expect(result.success).toBe(true);
1171+
expect(capturedBody).toBeDefined();
1172+
// Verify AI footer is present (not removed by sanitization)
1173+
expect(capturedBody).toContain("AI generated by");
1174+
expect(capturedBody).toContain("Security Test Workflow");
1175+
// Verify malicious comment in user content was removed by sanitization
1176+
expect(capturedBody).not.toContain("<!-- malicious comment -->");
1177+
1178+
delete process.env.GH_AW_WORKFLOW_NAME;
1179+
});
1180+
1181+
it("should sanitize user content but preserve system markers", async () => {
1182+
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");
1183+
1184+
let capturedBody = null;
1185+
mockGithub.rest.issues.createComment = async params => {
1186+
capturedBody = params.body;
1187+
return {
1188+
data: {
1189+
id: 12345,
1190+
html_url: "https://github.com/owner/repo/issues/42#issuecomment-12345",
1191+
},
1192+
};
1193+
};
1194+
1195+
// Execute the handler
1196+
const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);
1197+
1198+
const message = {
1199+
type: "add_comment",
1200+
body: "User says: @badactor please <!-- inject this --> [phishing](http://evil.com)",
1201+
};
1202+
1203+
const result = await handler(message, {});
1204+
1205+
expect(result.success).toBe(true);
1206+
expect(capturedBody).toBeDefined();
1207+
1208+
// User content should be sanitized
1209+
expect(capturedBody).toContain("`@badactor`"); // Mention neutralized
1210+
expect(capturedBody).not.toContain("<!-- inject this -->"); // Comment removed
1211+
expect(capturedBody).toContain("(evil.com/redacted)"); // HTTP URL redacted
1212+
1213+
// But footer should still be present with proper markdown
1214+
expect(capturedBody).toContain("> AI generated by");
1215+
});
1216+
});
11011217
});
11021218

11031219
describe("enforceCommentLimits", () => {

‎actions/setup/js/add_reaction_and_edit_comment.cjs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
const { getRunStartedMessage } = require("./messages_run_status.cjs");
55
const { getErrorMessage } = require("./error_helpers.cjs");
66
const { generateWorkflowIdMarker } = require("./generate_footer.cjs");
7+
const { sanitizeContent } = require("./sanitize_content.cjs");
78

89
async function main() {
910
// Read inputs from environment variables
@@ -328,18 +329,20 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName) {
328329
eventType: eventTypeDescription,
329330
});
330331

331-
// Add workflow-id and tracker-id markers for hide-older-comments feature
332-
const workflowId = process.env.GITHUB_WORKFLOW || "";
333-
const trackerId = process.env.GH_AW_TRACKER_ID || "";
334-
335-
let commentBody = workflowLinkText;
332+
// Sanitize the workflow link text to prevent injection attacks (defense in depth for custom message templates)
333+
// This must happen BEFORE adding workflow markers to preserve them
334+
let commentBody = sanitizeContent(workflowLinkText);
336335

337336
// Add lock notice if lock-for-agent is enabled for issues or issue_comment
338337
const lockForAgent = process.env.GH_AW_LOCK_FOR_AGENT === "true";
339338
if (lockForAgent && (eventName === "issues" || eventName === "issue_comment")) {
340339
commentBody += "\n\n🔒 This issue has been locked while the workflow is running to prevent concurrent modifications.";
341340
}
342341

342+
// Add workflow-id and tracker-id markers for hide-older-comments feature
343+
const workflowId = process.env.GITHUB_WORKFLOW || "";
344+
const trackerId = process.env.GH_AW_TRACKER_ID || "";
345+
343346
// Add workflow-id marker if available
344347
if (workflowId) {
345348
commentBody += `\n\n${generateWorkflowIdMarker(workflowId)}`;

‎actions/setup/js/add_workflow_run_comment.cjs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
const { getRunStartedMessage } = require("./messages_run_status.cjs");
55
const { getErrorMessage } = require("./error_helpers.cjs");
66
const { generateWorkflowIdMarker } = require("./generate_footer.cjs");
7+
const { sanitizeContent } = require("./sanitize_content.cjs");
78

89
/**
910
* Add a comment with a workflow run link to the triggering item.
@@ -140,18 +141,20 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName) {
140141
eventType: eventTypeDescription,
141142
});
142143

143-
// Add workflow-id and tracker-id markers for hide-older-comments feature
144-
const workflowId = process.env.GITHUB_WORKFLOW || "";
145-
const trackerId = process.env.GH_AW_TRACKER_ID || "";
146-
147-
let commentBody = workflowLinkText;
144+
// Sanitize the workflow link text to prevent injection attacks (defense in depth for custom message templates)
145+
// This must happen BEFORE adding workflow markers to preserve them
146+
let commentBody = sanitizeContent(workflowLinkText);
148147

149148
// Add lock notice if lock-for-agent is enabled for issues or issue_comment
150149
const lockForAgent = process.env.GH_AW_LOCK_FOR_AGENT === "true";
151150
if (lockForAgent && (eventName === "issues" || eventName === "issue_comment")) {
152151
commentBody += "\n\n🔒 This issue has been locked while the workflow is running to prevent concurrent modifications.";
153152
}
154153

154+
// Add workflow-id and tracker-id markers for hide-older-comments feature
155+
const workflowId = process.env.GITHUB_WORKFLOW || "";
156+
const trackerId = process.env.GH_AW_TRACKER_ID || "";
157+
155158
// Add workflow-id marker if available
156159
if (workflowId) {
157160
commentBody += `\n\n${generateWorkflowIdMarker(workflowId)}`;

‎actions/setup/js/check_workflow_recompile_needed.cjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ async function main() {
103103
const footer = getFooterWorkflowRecompileCommentMessage(ctx);
104104
const xmlMarker = generateXMLMarker(workflowName, runUrl);
105105

106+
// Sanitize the message text but not the footer/marker which are system-generated
106107
const commentBody = `Workflows are still out of sync.\n\n---\n${footer}\n\n${xmlMarker}`;
107108

108109
await github.rest.issues.createComment({
@@ -157,6 +158,7 @@ async function main() {
157158
// Use custom footer template if configured, with XML marker for traceability
158159
const footer = getFooterWorkflowRecompileMessage(ctx);
159160
const xmlMarker = generateXMLMarker(workflowName, runUrl);
161+
// Note: issueBody is built from a template render, no user content to sanitize
160162
issueBody += "\n\n---\n" + footer + "\n\n" + xmlMarker + "\n";
161163

162164
try {

‎actions/setup/js/close_discussion.cjs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77

88
const { getErrorMessage } = require("./error_helpers.cjs");
9+
const { sanitizeContent } = require("./sanitize_content.cjs");
910

1011
/**
1112
* Get discussion details using GraphQL with pagination for labels
@@ -264,7 +265,8 @@ async function main(config = {}) {
264265
// Add comment if body is provided
265266
let commentUrl;
266267
if (item.body) {
267-
const comment = await addDiscussionComment(github, discussion.id, item.body);
268+
const sanitizedBody = sanitizeContent(item.body);
269+
const comment = await addDiscussionComment(github, discussion.id, sanitizedBody);
268270
core.info(`Added comment to discussion #${discussionNumber}: ${comment.url}`);
269271
commentUrl = comment.url;
270272
}

‎actions/setup/js/close_entity_helpers.cjs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const { generateFooterWithMessages } = require("./messages_footer.cjs");
66
const { getTrackerID } = require("./get_tracker_id.cjs");
77
const { getRepositoryUrl } = require("./get_repository_url.cjs");
88
const { getErrorMessage } = require("./error_helpers.cjs");
9+
const { sanitizeContent } = require("./sanitize_content.cjs");
910

1011
/**
1112
* @typedef {'issue' | 'pull_request'} EntityType
@@ -57,7 +58,10 @@ function buildCommentBody(body, triggeringIssueNumber, triggeringPRNumber) {
5758
const workflowSourceURL = process.env.GH_AW_WORKFLOW_SOURCE_URL || "";
5859
const runUrl = buildRunUrl();
5960

60-
return body.trim() + getTrackerID("markdown") + generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, triggeringIssueNumber, triggeringPRNumber, undefined);
61+
// Sanitize the body content to prevent injection attacks
62+
const sanitizedBody = sanitizeContent(body);
63+
64+
return sanitizedBody.trim() + getTrackerID("markdown") + generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, triggeringIssueNumber, triggeringPRNumber, undefined);
6165
}
6266

6367
/**

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import { describe, it, expect, beforeEach, vi } from "vitest";
2+
import fs from "fs";
3+
import path from "path";
24
const mockCore = { debug: vi.fn(), info: vi.fn(), warning: vi.fn(), error: vi.fn(), setFailed: vi.fn(), setOutput: vi.fn(), summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue() } },
35
mockContext = { eventName: "issues", runId: 12345, repo: { owner: "testowner", repo: "testrepo" }, payload: { issue: { number: 42 }, pull_request: { number: 100 }, repository: { html_url: "https://github.com/testowner/testrepo" } } };
46
((global.core = mockCore), (global.context = mockContext));
@@ -183,5 +185,57 @@ describe("close_entity_helpers", () => {
183185
it("should have correct URL path", () => {
184186
expect(PULL_REQUEST_CONFIG.urlPath).toBe("pull");
185187
}));
188+
}),
189+
describe("sanitization preserves markers", () => {
190+
it("should sanitize user content while preserving system markers in buildCommentBody", () => {
191+
// Import sanitizeContent to test the actual behavior
192+
const { sanitizeContent } = require("./sanitize_content.cjs");
193+
const { getTrackerID } = require("./get_tracker_id.cjs");
194+
const { generateFooterWithMessages } = require("./messages_footer.cjs");
195+
196+
// Set up environment for tracker ID and footer
197+
process.env.GH_AW_WORKFLOW_NAME = "Test Workflow";
198+
process.env.GH_AW_WORKFLOW_SOURCE = "test.md";
199+
process.env.GH_AW_WORKFLOW_SOURCE_URL = "https://github.com/test/repo";
200+
process.env.GH_AW_TRACKER_ID = "test-tracker-456";
201+
202+
// Simulate what buildCommentBody does
203+
const userContent = "User comment with <script>xss</script> and <!-- evil comment -->";
204+
const sanitizedBody = sanitizeContent(userContent);
205+
const trackerID = getTrackerID("markdown");
206+
const footer = generateFooterWithMessages("Test Workflow", "https://github.com/test/repo/actions/runs/123", "test.md", "https://github.com/test/repo", 42, undefined, undefined);
207+
const result = sanitizedBody.trim() + trackerID + footer;
208+
209+
// User content should be sanitized (tags converted)
210+
expect(result).not.toContain("<script>");
211+
expect(result).toContain("(script)"); // Tags converted to parentheses
212+
expect(result).not.toContain("<!-- evil comment -->");
213+
214+
// System markers should be preserved
215+
expect(result).toContain("<!-- gh-aw-tracker-id: test-tracker-456 -->");
216+
expect(result).toContain("Generated by"); // Footer contains this text
217+
218+
// Clean up
219+
delete process.env.GH_AW_WORKFLOW_NAME;
220+
delete process.env.GH_AW_WORKFLOW_SOURCE;
221+
delete process.env.GH_AW_WORKFLOW_SOURCE_URL;
222+
delete process.env.GH_AW_TRACKER_ID;
223+
});
224+
225+
it("should preserve footer markers when user content has malicious XML comments", () => {
226+
const { sanitizeContent } = require("./sanitize_content.cjs");
227+
228+
const userContent = "Text with <!-- malicious --> comment";
229+
const sanitized = sanitizeContent(userContent);
230+
231+
// User's malicious comment should be removed
232+
expect(sanitized).not.toContain("<!-- malicious -->");
233+
234+
// Now add system marker after sanitization
235+
const withMarker = sanitized + "\n\n<!-- gh-aw-workflow-id: test -->";
236+
237+
// System marker should still be present
238+
expect(withMarker).toContain("<!-- gh-aw-workflow-id: test -->");
239+
});
186240
}));
187241
});

‎actions/setup/js/close_expired_discussions.cjs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
const { executeExpiredEntityCleanup } = require("./expired_entity_main_flow.cjs");
55
const { generateExpiredEntityFooter } = require("./generate_footer.cjs");
6+
const { sanitizeContent } = require("./sanitize_content.cjs");
67

78
/**
89
* Add comment to a GitHub Discussion using GraphQL
@@ -22,7 +23,7 @@ async function addDiscussionComment(github, discussionId, message) {
2223
}
2324
}
2425
}`,
25-
{ dId: discussionId, body: message }
26+
{ dId: discussionId, body: sanitizeContent(message) }
2627
);
2728

2829
return result.addDiscussionComment.comment;

0 commit comments

Comments
 (0)