From 982c2b359de189894bbe9de9ebed614da5cb504f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 12:03:18 +0000 Subject: [PATCH 1/2] Initial plan From 20e8f845a84f3dd6f35db25853c8afbc488656ee Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 12:19:07 +0000 Subject: [PATCH 2/2] fix: add sanitizeContent to create_discussion.cjs and create_pull_request.cjs body processing - Adds `sanitizeContent` import and call to `create_discussion.cjs` after `removeDuplicateTitleFromDescription`, matching the pattern used in `create_issue.cjs` and `update_handler_factory.cjs` - Adds `sanitizeContent` import and call to `create_pull_request.cjs` after `removeDuplicateTitleFromDescription`, before the system-generated auto-close keyword is appended - Adds regression tests for @mention neutralization and XPIA channel closure in both handlers (create_discussion_sanitization.test.cjs and create_pull_request_sanitization.test.cjs) Fixes: @mention injection and XPIA channel via hidden markdown link titles in create_discussion and create_pull_request safe-output handlers Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0f3551a7-db46-4881-a4fd-ff2a1dc85ee2 Co-authored-by: szabta89 <1330202+szabta89@users.noreply.github.com> --- actions/setup/js/create_discussion.cjs | 4 + .../create_discussion_sanitization.test.cjs | 171 ++++++++++++++++++ actions/setup/js/create_pull_request.cjs | 4 + .../create_pull_request_sanitization.test.cjs | 144 +++++++++++++++ 4 files changed, 323 insertions(+) create mode 100644 actions/setup/js/create_discussion_sanitization.test.cjs create mode 100644 actions/setup/js/create_pull_request_sanitization.test.cjs diff --git a/actions/setup/js/create_discussion.cjs b/actions/setup/js/create_discussion.cjs index 0366fa4f7f6..8447c066bfc 100644 --- a/actions/setup/js/create_discussion.cjs +++ b/actions/setup/js/create_discussion.cjs @@ -19,6 +19,7 @@ const { ERR_VALIDATION } = require("./error_codes.cjs"); const { createExpirationLine, generateFooterWithExpiration, addExpirationToFooter } = require("./ephemerals.cjs"); const { generateFooterWithMessages } = require("./messages_footer.cjs"); const { generateWorkflowIdMarker, generateWorkflowCallIdMarker, generateCloseKeyMarker, normalizeCloseOlderKey } = require("./generate_footer.cjs"); +const { sanitizeContent } = require("./sanitize_content.cjs"); const { sanitizeLabelContent } = require("./sanitize_label_content.cjs"); const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); @@ -482,6 +483,9 @@ async function main(config = {}) { let processedBody = replaceTemporaryIdReferences(item.body || "", temporaryIdMap, qualifiedItemRepo); processedBody = removeDuplicateTitleFromDescription(title, processedBody); + // Sanitize body content to neutralize @mentions, URLs, and other security risks + processedBody = sanitizeContent(processedBody); + if (!title) { title = item.body || "Discussion"; } diff --git a/actions/setup/js/create_discussion_sanitization.test.cjs b/actions/setup/js/create_discussion_sanitization.test.cjs new file mode 100644 index 00000000000..52c993eeb2d --- /dev/null +++ b/actions/setup/js/create_discussion_sanitization.test.cjs @@ -0,0 +1,171 @@ +// @ts-check +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url); +const { main: createDiscussionMain } = require("./create_discussion.cjs"); + +describe("create_discussion body sanitization", () => { + let mockGithub; + let mockCore; + let mockContext; + let mockExec; + let originalEnv; + + beforeEach(() => { + // Save original environment + originalEnv = { ...process.env }; + + // Mock GitHub API with discussion categories + mockGithub = { + rest: {}, + graphql: vi.fn().mockImplementation((query, variables) => { + // Handle repository query (fetch categories) + if (query.includes("discussionCategories")) { + return Promise.resolve({ + repository: { + id: "R_test123", + discussionCategories: { + nodes: [ + { + id: "DIC_kwDOGFsHUM4BsUn1", + name: "General", + slug: "general", + description: "General discussions", + }, + ], + }, + }, + }); + } + // Handle create discussion mutation — echo back the body for assertion + if (query.includes("createDiscussion")) { + return Promise.resolve({ + createDiscussion: { + discussion: { + id: "D_test456", + number: 42, + title: variables.title, + url: "https://github.com/test-owner/test-repo/discussions/42", + }, + }, + }); + } + return Promise.reject(new Error("Unknown GraphQL query")); + }), + }; + + // Mock Core + mockCore = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + setOutput: vi.fn(), + }; + + // Mock Context + mockContext = { + repo: { owner: "test-owner", repo: "test-repo" }, + runId: 12345, + payload: { + repository: { + html_url: "https://github.com/test-owner/test-repo", + }, + }, + }; + + // Mock Exec + mockExec = { + exec: vi.fn().mockResolvedValue(0), + }; + + // Set globals + global.github = mockGithub; + global.core = mockCore; + global.context = mockContext; + global.exec = mockExec; + + // Set required environment variables + process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"; + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + process.env.GH_AW_WORKFLOW_SOURCE_URL = "https://github.com/owner/repo/blob/main/workflow.md"; + process.env.GITHUB_SERVER_URL = "https://github.com"; + }); + + afterEach(() => { + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); + vi.clearAllMocks(); + }); + + it("should neutralize @mentions in discussion body", async () => { + const handler = await createDiscussionMain({ max: 5, category: "general" }); + const result = await handler( + { + title: "Test Discussion", + body: "This was reported by @malicious-user and CC @another-user.", + }, + {} + ); + + expect(result.success).toBe(true); + + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + const body = createMutationCall[1].body; + // @mentions must be neutralized to backtick form + expect(body).toContain("`@malicious-user`"); + expect(body).toContain("`@another-user`"); + // Raw unescaped @mentions must not appear + expect(body).not.toMatch(/(? { + const handler = await createDiscussionMain({ max: 5, category: "general" }); + const result = await handler( + { + title: "Security Test", + body: 'Click [here](https://example.com "XPIA hidden payload") for details.', + }, + {} + ); + + expect(result.success).toBe(true); + + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + const body = createMutationCall[1].body; + // The hidden title must be moved to visible text (closing the XPIA channel) + // sanitizeContent converts [text](url "hidden") → [text (hidden)](url) + expect(body).toContain("XPIA hidden payload"); + // The payload must no longer be in a hidden markdown link title attribute + expect(body).not.toMatch(/\[.*\]\([^)]*"XPIA hidden payload"[^)]*\)/); + }); + + it("should sanitize body but preserve the footer workflow marker", async () => { + const handler = await createDiscussionMain({ max: 5, category: "general" }); + const result = await handler( + { + title: "Test Discussion", + body: "Please notify @someone about this finding.", + }, + {} + ); + + expect(result.success).toBe(true); + + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + const body = createMutationCall[1].body; + // @mention must be neutralized + expect(body).toContain("`@someone`"); + // System-generated footer marker must still be present + expect(body).toContain("gh-aw-workflow-id"); + }); +}); diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 48e828c81a3..ad4c4687ca6 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -10,6 +10,7 @@ const { pushSignedCommits } = require("./push_signed_commits.cjs"); const { getTrackerID } = require("./get_tracker_id.cjs"); const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs"); const { sanitizeTitle, applyTitlePrefix } = require("./sanitize_title.cjs"); +const { sanitizeContent } = require("./sanitize_content.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { replaceTemporaryIdReferences, replaceTemporaryIdReferencesInPatch, getOrGenerateTemporaryId } = require("./temporary_id.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); @@ -834,6 +835,9 @@ async function main(config = {}) { // Remove duplicate title from description if it starts with a header matching the title processedBody = removeDuplicateTitleFromDescription(title, processedBody); + // Sanitize body content to neutralize @mentions, URLs, and other security risks + processedBody = sanitizeContent(processedBody); + // Auto-add "Fixes #N" closing keyword if triggered from an issue and not already present. // This ensures the triggering issue is auto-closed when the PR is merged. // Agents are instructed to include this but don't reliably do so. diff --git a/actions/setup/js/create_pull_request_sanitization.test.cjs b/actions/setup/js/create_pull_request_sanitization.test.cjs new file mode 100644 index 00000000000..5f0b5c7cb33 --- /dev/null +++ b/actions/setup/js/create_pull_request_sanitization.test.cjs @@ -0,0 +1,144 @@ +// @ts-check +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { createRequire } from "module"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; + +const require = createRequire(import.meta.url); + +describe("create_pull_request - body sanitization", () => { + let tempDir; + let originalEnv; + + beforeEach(() => { + originalEnv = { ...process.env }; + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; + process.env.GITHUB_BASE_REF = "main"; + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-sanitize-test-")); + + global.core = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + setFailed: vi.fn(), + setOutput: vi.fn(), + startGroup: vi.fn(), + endGroup: vi.fn(), + summary: { + addRaw: vi.fn().mockReturnThis(), + write: vi.fn().mockResolvedValue(undefined), + }, + }; + global.github = { + rest: { + pulls: { + create: vi.fn().mockResolvedValue({ data: { number: 1, html_url: "https://github.com/test" } }), + }, + repos: { + get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }), + }, + issues: { + addLabels: vi.fn().mockResolvedValue({}), + }, + }, + graphql: vi.fn(), + }; + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "test-owner", repo: "test-repo" }, + payload: {}, + }; + global.exec = { + exec: vi.fn().mockResolvedValue(0), + getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }), + }; + + // Clear module cache so globals are picked up fresh + delete require.cache[require.resolve("./create_pull_request.cjs")]; + }); + + afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); + + if (tempDir && fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + + delete global.core; + delete global.github; + delete global.context; + delete global.exec; + vi.clearAllMocks(); + }); + + it("should neutralize @mentions in PR body", async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ allow_empty: true }); + + await handler( + { + title: "Test PR", + body: "This PR fixes a bug reported by @malicious-user and reviewed by @another-user.", + }, + {} + ); + + const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0]; + expect(createCall).toBeDefined(); + // @mentions must be neutralized to backtick form + expect(createCall.body).toContain("`@malicious-user`"); + expect(createCall.body).toContain("`@another-user`"); + // Raw unescaped @mentions must not appear + expect(createCall.body).not.toMatch(/(? { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ allow_empty: true }); + + await handler( + { + title: "Security Fix", + body: 'See [the report](https://example.com "XPIA hidden payload") for context.', + }, + {} + ); + + const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0]; + expect(createCall).toBeDefined(); + // The hidden title must be moved to visible text (closing the XPIA channel) + // sanitizeContent converts [text](url "hidden") → [text (hidden)](url) + expect(createCall.body).toContain("XPIA hidden payload"); + // The payload must no longer be in a hidden markdown link title attribute + expect(createCall.body).not.toMatch(/\[.*\]\([^)]*"XPIA hidden payload"[^)]*\)/); + }); + + it("should sanitize body but preserve the footer workflow marker", async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ allow_empty: true }); + + await handler( + { + title: "Test PR", + body: "Please notify @someone about this change.", + }, + {} + ); + + const createCall = global.github.rest.pulls.create.mock.calls[0]?.[0]; + expect(createCall).toBeDefined(); + // @mention must be neutralized + expect(createCall.body).toContain("`@someone`"); + // System-generated footer marker must still be present + expect(createCall.body).toContain("gh-aw-workflow-id"); + }); +});