-
Notifications
You must be signed in to change notification settings - Fork 107
Fix command injection and artipacked vulnerabilities in unbloat-docs workflow #2925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
Copilot
AI
Nov 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} })()`); |
Copilot
AI
Nov 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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])) | ||
| ); |
Copilot
AI
Nov 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.