Skip to content

fix: close shell injection in upload_assets.cjs (incomplete fix from d07e64c3) - #17736

Merged
pelikhan merged 2 commits into
mainfrom
copilot/fix-incomplete-security-fix
Feb 22, 2026
Merged

fix: close shell injection in upload_assets.cjs (incomplete fix from d07e64c3)#17736
pelikhan merged 2 commits into
mainfrom
copilot/fix-incomplete-security-fix

Conversation

CopilotAI commented Feb 22, 2026

Copy link
Copy Markdown
Contributor

Commit d07e64c3 converted all exec.exec() template-string calls to the safe array form — except line 156, leaving a shell injection vector via unsanitized targetFileName sourced from agent output JSON.

Change

// Before — shell injection via targetFileName (quotes, semicolons, backticks)awaitexec.exec(`git add "${targetFileName}"`);// After — argument passed literally, no shell interpolationawaitexec.exec("git",["add",targetFileName]);

Consistent with every other exec.exec call in the file post-d07e64c3.

Original prompt

This section details on the original issue you should resolve

<issue_title>🚨 [SECURITY] Security Red Team Findings - 2026-02-22 (Weekly Full Scan)</issue_title>
<issue_description>Weekly security red team scan completed on 2026-02-22 (Sunday full scan). 1 confirmed finding requiring action: an incomplete security fix in upload_assets.cjs. 2 low-risk mitigated patterns and 1 test-only accepted-risk pattern also documented.

Scan Summary

MetricValue
Scan ModeWEEKLY FULL (Sunday)
Techniquefull-comprehensive (all 6 techniques)
Files Analyzed250 (216 .cjs + 34 .sh)
Confirmed Findings1 (Medium)
Mitigated/Low Risk2
Accepted Risk1 (test-only)
Malicious Patterns0
Secret Exfiltration0
Backdoors0

Finding 1 — INCOMPLETE SECURITY FIX (Medium)

Location: actions/setup/js/upload_assets.cjs:156

Description: The security fix commit d07e64c3 ("fix: supply chain and shell injection security findings", Feb 20 2026) converted all template-string exec.exec() calls in upload_assets.cjs to the safe array form — except line 156, which still uses a template string with an unsanitized value:

// Line 156 — NOT fixed by d07e64c3awaitexec.exec(`git add "\$\{targetFileName}"`);

All other exec.exec calls in the same file were correctly updated:

awaitexec.exec("git",["rev-parse","--verify",`origin/\$\{normalizedBranchName}`]);awaitexec.exec("git",["checkout","-B",normalizedBranchName, ...]);awaitexec.exec("git",["push","origin",normalizedBranchName]);

targetFileName is sourced from agent output JSON (loadAgentOutput()result.itemsasset.targetFileName) with no path or character sanitization applied before the exec call. A targetFileName containing " could break out of quoting; one containing ;, &&, or backticks could permit command injection.

Forensics Analysis:

  • Line 156 introduced in commit 17ca20b8 by Copilot on 2025-12-23 ("Remove inline mode and externalize all scripts via setup action")
  • Partial fix attempted in commit d07e64c3 by Copilot on 2026-02-20 ("fix: supply chain and shell injection security findings") — but this line was missed
  • No further fix commits since then

Remediation Task:

  • Task 1 — Convert line 156 of actions/setup/js/upload_assets.cjs to use the safe array form:
    // Change from:awaitexec.exec(`git add "\$\{targetFileName}"`);// Change to:awaitexec.exec("git",["add",targetFileName]);
    This is consistent with the existing pattern used in the rest of the file after commit d07e64c3.

Finding 2 — DYNAMIC EXEC (Low / Mitigated)

Location: actions/setup/js/push_to_pull_request_branch.cjs:312 and actions/setup/js/create_pull_request.cjs:568

Description: exec.exec(\git am ${patchFilePath}`)uses template string interpolation. However,patchFilePathis **internally generated** ingenerate_git_patch.cjs` via:

functiongetPatchPath(branchName){constsanitized=sanitizeBranchNameForPatch(branchName);return`/tmp/gh-aw/aw-\$\{sanitized}.patch`;}// sanitizeBranchNameForPatch removes: / \ : * ? " < > |// Result is always: /tmp/gh-aw/aw-(alphanumeric-dash-chars).patch

The sanitization constrains the path to characters safe for shell interpolation. No actionable fix required, but converting to array form would be defensive best practice.

Forensics: Introduced in commit b58fd691 by Copilot on 2026-02-21.

Recommendation (non-urgent):

  • Consider converting to exec.exec("git", ["am", patchFilePath]) for consistency with the safe pattern used elsewhere in the codebase.

Finding 3 — SHELL EVAL IN TEST HELPER (Negligible / Accepted Risk)

Location: actions/setup/sh/clean_git_credentials_test.sh:34

Description: eval "\$\{condition}" is used in a test-only assert() helper function. All current callers pass hard-coded string literals. No external input reaches this eval.

Forensics: Introduced in commit 45f92f64 by Copilot on 2026-02-19 ("Recursively clean git credentials from all checkouts in workspace and /tmp/").

Recommendation: Low priority. Could be refactored to use a safer assertion mechanism if the test file evolves.


Techniques Executed (Full Comprehensive)

Technique Results Detail
TechniqueStatusNotes
Pattern AnalysisComplete1 confirmed finding (upload_assets), 2 mitigated
AST InspectionNo findingsNo suspicious function names, no unusual exports
Entropy AnalysisNo findingsNo obfuscated/encoded strings
Network AnalysisNo findingsvalidate_secrets.cjs makes expected API validation calls; all other URLs are docs refs or GitHub domains
Behavioral AnalysisNo findingsDate logic is timestamps/expiry guards only; no time bombs; no persistence
Dependency Audi...

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…pload_assets.cjs
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix incomplete security fix in upload_assets.cjsfix: close shell injection in upload_assets.cjs (incomplete fix from d07e64c3)Feb 22, 2026
@pelikhan
pelikhan marked this pull request as ready for review February 22, 2026 15:04
CopilotAI review requested due to automatic review settings February 22, 2026 15:05
@pelikhan
pelikhan merged commit 0eb518a into mainFeb 22, 2026
114 checks passed
@pelikhan
pelikhan deleted the copilot/fix-incomplete-security-fix branch February 22, 2026 15:05

CopilotAI left a comment

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.

Pull request overview

This pull request fixes an incomplete security patch from commit d07e64c that addressed shell injection vulnerabilities in upload_assets.cjs. While that commit converted most exec.exec() calls from template strings to the safe array form, line 156 was inadvertently missed, leaving a shell injection vector through the unsanitized targetFileName parameter.

Changes:

  • Converts line 156 in upload_assets.cjs from template string form await exec.exec(\git add "${targetFileName}"`)to safe array formawait exec.exec("git", ["add", targetFileName])`

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🚨 [SECURITY] Security Red Team Findings - 2026-02-22 (Weekly Full Scan)

3 participants

@pelikhan