Sanitize subprocess output in run_validate_workflows.cjs (SEC-004) by Copilot · Pull Request #25971 · github/gh-aw

Copilot · 2026-04-13T02:18:37Z

run_validate_workflows.cjs embeds raw gh aw compile --validate stdout/stderr into GitHub issue and comment bodies without sanitization. Malformed tool output could escape the code fence or inject unexpected Markdown/HTML.

Changes

Import the shared sanitizeContent helper (used by 30+ other handlers) and apply it to truncatedOutput before embedding in both the comment body (existing issue path) and issue body (new issue path)
Add tests verifying @mention neutralization in both code paths

const { sanitizeContent } = require("./sanitize_content.cjs");

const truncatedOutput = combinedOutput.substring(0, 50000) + ...;
const sanitizedOutput = sanitizeContent(truncatedOutput);
// sanitizedOutput used in template instead of truncatedOutput

… (SEC-004) Import and apply the shared sanitizeContent helper to truncatedOutput before embedding it in GitHub issue/comment bodies in run_validate_workflows.cjs. This prevents untrusted subprocess output from escaping code fences or injecting unexpected Markdown/HTML. Add tests verifying @mentions in output are neutralized in both the new-issue and existing-issue-comment code paths. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b1e91242-6a52-4b62-a671-b50030ed6caf Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot

Pull request overview

This PR aims to prevent Markdown/HTML injection when run_validate_workflows.cjs posts gh aw compile --validate output into GitHub issue/comment bodies by sanitizing the embedded output.

Changes:

Import and apply the shared sanitizeContent helper to the truncated validator output before embedding it in issue/comment templates.
Add Vitest coverage to ensure @mentions in output are neutralized in both “new issue” and “existing issue comment” paths.

Show a summary per file

File	Description
actions/setup/js/run_validate_workflows.cjs	Sanitizes truncated subprocess output before embedding in issue/comment Markdown.
actions/setup/js/run_validate_workflows.test.cjs	Adds tests asserting `@mention` neutralization in both posting paths.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 2/2 changed files
Comments generated: 3

Copilot · 2026-04-13T02:32:47Z

 <details>
 <summary>Validation output</summary>

 \`\`\`
-${truncatedOutput}
+${sanitizedOutput}
 \`\`\`


The validation output is embedded inside a triple-backtick fenced code block. If the subprocess output itself contains a bare fence line like "```" (or "~~~"), it can prematurely close the outer fence and escape the intended code region. sanitizeContent() balances markdown fences but does not neutralize/escape fence delimiters, so this is still exploitable for Markdown injection. Consider switching the wrapper to a longer fence length (the repo uses 6 backticks for untrusted content; see `actions/setup/js/log_parser_shared.cjs:1006-1013`) or otherwise ensure fence sequences in the output cannot terminate the wrapper fence.

Copilot · 2026-04-13T02:32:48Z

      const truncatedOutput = combinedOutput.substring(0, 50000) + (combinedOutput.length > 50000 ? "\n\n... (output truncated)" : "");
+      const sanitizedOutput = sanitizeContent(truncatedOutput);


sanitizeContent(truncatedOutput) is called without a maxLength, and sanitization can expand the string (e.g., wrapping many @mentions in backticks, balancing fences). That means the final issue/comment body can exceed GitHub's size limits even though you pre-truncate to 50k. To keep the post size bounded, pass an explicit maxLength to sanitizeContent (e.g., 50k) and/or perform truncation after sanitization so the sanitized string is what gets size-limited.

Suggested change

const truncatedOutput = combinedOutput.substring(0, 50000) + (combinedOutput.length > 50000 ? "\n\n... (output truncated)" : "");

const sanitizedOutput = sanitizeContent(truncatedOutput);

const maxOutputLength = 50000;

const truncatedOutput = combinedOutput.substring(0, maxOutputLength) + (combinedOutput.length > maxOutputLength ? "\n\n... (output truncated)" : "");

const sanitizedOutput = sanitizeContent(truncatedOutput, maxOutputLength);

Copilot · 2026-04-13T02:32:48Z

+  it("should sanitize output containing @mentions in new issue body", async () => {
+    mockExec.exec.mockImplementation(async (_cmd, _args, options) => {
+      if (options?.listeners?.stderr) {
+        options.listeners.stderr(Buffer.from("Error: @malicious-user triggered a warning\n"));
+      }
+      return 1;
+    });
+
+    mockGithub.rest.search.issuesAndPullRequests.mockResolvedValue({
+      data: { total_count: 0, items: [] },
+    });
+
+    mockGithub.rest.issues.create.mockResolvedValue({
+      data: { number: 45, html_url: "https://github.com/testowner/testrepo/issues/45" },
+    });
+
+    const { main } = await import("./run_validate_workflows.cjs");
+    await main();
+
+    const createCall = mockGithub.rest.issues.create.mock.calls[0][0];
+    // sanitizeContent should neutralize @mentions so they don't ping users
+    expect(createCall.body).not.toMatch(/@malicious-user(?!`)/);
+  });


The new tests only assert that @mentions are neutralized, but the PR description calls out code-fence escaping/Markdown injection as the main risk. Add a regression test where the tool output contains a bare fence line (e.g., "```\n...") and verify the created issue/comment body still keeps the output confined (for example by using a longer wrapper fence or escaping fence delimiters).

github-actions · 2026-04-13T02:33:25Z

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	2
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	2 (100%)
Duplicate test clusters	0
Test inflation detected	⚠️ Yes (47 test lines / 5 prod lines = 9.4x — see note below)
🚨 Coding-guideline violations	0

Test Classification Details

📋 Per-test classification (2 tests)

Test	File	Classification	Issues Detected
`should sanitize output containing` @mentions `in new issue body`	`actions/setup/js/run_validate_workflows.test.cjs`	✅ Design	None — verifies observable security behavior in new issue body
`should sanitize output containing` @mentions `in comment body`	`actions/setup/js/run_validate_workflows.test.cjs`	✅ Design	None — verifies observable security behavior in existing issue comment

Test Analysis

Both new tests target the SEC-004 sanitization behavior added in run_validate_workflows.cjs. They correctly simulate command output containing @malicious-user and assert that the constructed issue/comment body does not contain unsanitized @mention text — verifying the behavioral contract that subprocess output cannot trigger GitHub notifications.

Mocking assessment: The tests mock global.core, global.exec, global.github, and global.context — all GitHub Actions runtime globals. This is legitimate external-I/O mocking; the business logic under test (sanitization of subprocess content) runs un-mocked. ✅

Error coverage: Both tests drive an error path (exec returns exit code 1) which exercises the code paths that produce issues/comments. Full credit. ✅

Coverage gap noted: The tests cover @mention neutralization but do not cover other possible injection vectors (e.g., #issue-ref, backtick injection, or HTML in subprocess output). Depending on what sanitizeContent actually neutralizes, additional tests for those patterns could strengthen the contract. This is a suggestion only, not a blocker.

Inflation Note

The 9.4x test-to-production ratio (47 test lines / 5 production lines) technically triggers the inflation flag, but this is contextually justified: the production change is minimal (calling an already-existing sanitizeContent helper in two places), while proper security coverage requires end-to-end tests for both affected code paths (new issue creation and comment-on-existing-issue). The tests are not inflated in spirit.

Language Support

Tests analyzed:

🐹 Go (*_test.go): 0 tests
🟨 JavaScript (*.test.cjs): 2 tests (vitest)

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both new tests enforce the behavioral security contract that subprocess output is sanitized before being written to GitHub issues/comments.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

Assert on observable outputs, return values, or state changes
Cover error paths and boundary conditions
Would catch a behavioral regression if deleted
Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

Assert on internal function calls (mocking internals)
Only test the happy path with typical inputs
Break during legitimate refactoring even when behavior is correct
Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 547.5K · ◷

github-actions

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both new tests enforce the behavioral security contract for SEC-004 sanitization.

Initial plan

f027b96

Copilot AI assigned Copilot and pelikhan Apr 13, 2026

Copilot started work on behalf of pelikhan April 13, 2026 02:18 View session

Copilot AI linked an issue Apr 13, 2026 that may be closed by this pull request

[Safe Outputs Conformance] SEC-004: run_validate_workflows.cjs writes untrusted command output to issue body without sanitization #25942

Closed

Copilot AI changed the title ~~[WIP] Fix untrusted command output sanitization in issue body~~ Sanitize subprocess output in run_validate_workflows.cjs (SEC-004) Apr 13, 2026

Copilot AI requested a review from pelikhan April 13, 2026 02:27

Copilot finished work on behalf of pelikhan April 13, 2026 02:27

pelikhan approved these changes Apr 13, 2026

View reviewed changes

pelikhan marked this pull request as ready for review April 13, 2026 02:28

Copilot AI review requested due to automatic review settings April 13, 2026 02:28

pelikhan merged commit 40e9f46 into main Apr 13, 2026
55 checks passed

pelikhan deleted the copilot/sec-004-sanitize-command-output branch April 13, 2026 02:28

Copilot started reviewing on behalf of pelikhan April 13, 2026 02:29 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

github-actions bot mentioned this pull request Apr 13, 2026

[aw] No-Op Runs #25214

Open

github-actions bot approved these changes Apr 13, 2026

View reviewed changes

github-actions bot mentioned this pull request Apr 13, 2026

[blog] Weekly blog post – 2026-04-13 #26033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize subprocess output in run_validate_workflows.cjs (SEC-004)#25971

Sanitize subprocess output in run_validate_workflows.cjs (SEC-004)#25971
pelikhan merged 2 commits intomainfrom
copilot/sec-004-sanitize-command-output

Copilot AI commented Apr 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

github-actions bot commented Apr 13, 2026

Uh oh!

github-actions bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		const truncatedOutput = combinedOutput.substring(0, 50000) + (combinedOutput.length > 50000 ? "\n\n... (output truncated)" : "");
		const sanitizedOutput = sanitizeContent(truncatedOutput);

-      const truncatedOutput = combinedOutput.substring(0, 50000) + (combinedOutput.length > 50000 ? "\n\n... (output truncated)" : "");
-      const sanitizedOutput = sanitizeContent(truncatedOutput);
+      const maxOutputLength = 50000;
+      const truncatedOutput = combinedOutput.substring(0, maxOutputLength) + (combinedOutput.length > maxOutputLength ? "\n\n... (output truncated)" : "");
+      const sanitizedOutput = sanitizeContent(truncatedOutput, maxOutputLength);

Conversation

Copilot AI commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 13, 2026

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

Test Classification Details

Test Analysis

Inflation Note

Language Support

Verdict

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Apr 13, 2026 •

edited

Loading