Fix command injection and artipacked vulnerabilities in unbloat-docs workflow#2925
Fix command injection and artipacked vulnerabilities in unbloat-docs workflow#2925
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot compile unbloat-docs agentic workflow with --zizmor to make sure the artipack is fixed. |
…checkout Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit 79d636d. The artipacked vulnerability was caused by the custom checkout step missing |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a security vulnerability related to command injection in git commit operations and adds a security hardening measure for GitHub Actions credential handling.
Key Changes:
- Removed extra quotes around commit messages in
exec.execcalls to prevent shell command injection vulnerabilities - Added comprehensive test coverage for the vulnerability fix
- Added
persist-credentials: falseto checkout step in the unbloat-docs workflow to prevent credential exposure
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/upload_assets.cjs | Fixed command injection vulnerability by removing quotes around commit message parameter |
| pkg/workflow/js/upload_assets.test.cjs | Added test file with security-focused tests for the commit message fix and branch name normalization |
| .github/workflows/unbloat-docs.md | Added persist-credentials: false to checkout step for security hardening |
| .github/workflows/unbloat-docs.lock.yml | Compiled YAML reflecting both the security fix and credential persistence change |
| .github/workflows/technical-doc-writer.lock.yml | Compiled YAML with the security fix applied |
| .github/workflows/python-data-charts.lock.yml | Compiled YAML with the security fix applied |
| .github/workflows/poem-bot.lock.yml | Compiled YAML with the security fix applied |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; | ||
|
|
||
| // Read the script content | ||
| const scriptPath = path.join(process.cwd(), "upload_assets.cjs"); |
There was a problem hiding this comment.
The test is looking for upload_assets.cjs in process.cwd() but the actual file is in pkg/workflow/js/upload_assets.cjs. This path resolution will fail unless the test is run from the pkg/workflow/js/ directory. Use path.join(__dirname, 'upload_assets.cjs') or path.join(process.cwd(), 'pkg/workflow/js/upload_assets.cjs') instead.
| const scriptPath = path.join(process.cwd(), "upload_assets.cjs"); | |
| const scriptPath = path.join(__dirname, "upload_assets.cjs"); |
| const executeScript = async () => { | ||
| global.core = mockCore; | ||
| global.exec = mockExec; | ||
| return await eval(`(async () => { ${uploadAssetsScript} })()`); |
There was a problem hiding this comment.
Using eval() to execute script content is dangerous and can lead to code injection vulnerabilities. Consider using a safer approach such as dynamically importing the module or restructuring the script to be testable without eval. If eval is necessary, add a clear security comment explaining why and ensure the script content is never derived from user input.
| console.log( | ||
| "All exec calls:", | ||
| allCalls.map(c => (Array.isArray(c[1]) ? `${c[0]} ${c[1].join(" ")}` : c[0])) | ||
| ); |
There was a problem hiding this comment.
Debug console.log statements should not be committed to production code. According to the coding guidelines (CodingGuidelineID: 1000003), use core.debug() for logging in GitHub Actions scripts, or remove this debugging code entirely before merging.
|
@copilot fix tests |
…dd eval safety comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Head branch was pushed to by a user without write access
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fix command injection and artipacked vulnerabilities in unbloat-docs workflow
This PR fixes two security vulnerabilities in the unbloat-docs workflow: a command injection vulnerability in
upload_assets.cjsand an artipacked vulnerability in the workflow configuration.Summary of Changes
Changes
1. Command Injection Fix (upload_assets.cjs)
pkg/workflow/js/upload_assets.cjs: Removed redundant quote wrapping in git commit command
pkg/workflow/js/upload_assets.test.cjs: Added test coverage for commit message formatting and branch name normalization
The
@actions/execlibrary's array form automatically escapes arguments. Extra quotes bypass this protection and could allow injection if commit message content were attacker-controlled. This pattern now matches other git commands in the codebase (e.g.,checkout_pr_branch.cjs).2. Artipacked Vulnerability Fix (unbloat-docs.md)
persist-credentials: falseto the custom checkout stepThe custom checkout step was missing
persist-credentials: false, which could allow GitHub credentials to persist in the.gitdirectory and potentially be exposed through artifacts. This fix prevents the GITHUB_TOKEN from being persisted in.git/config.3. Test Improvements (based on code review)
__dirnameinstead ofprocess.cwd()for reliabilityconsole.logstatement from test codeeval()in test contextVerification
Files Changed
pkg/workflow/js/upload_assets.cjs- Fixed vulnerable git commit call (1 line changed)pkg/workflow/js/upload_assets.test.cjs- Added test coverage and addressed code review feedback.github/workflows/unbloat-docs.md- Added persist-credentials: false to checkout (3 lines added).github/workflows/unbloat-docs.lock.yml- Recompiled with both fixesOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.