fix(security): migrate docker shell strings to argv arrays in agent-onboard#2075
fix(security): migrate docker shell strings to argv arrays in agent-onboard#2075Sanjays2402 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
…nboard Convert 2 of 3 shell-string run() calls in agent-onboard.ts to argv arrays, eliminating shell injection surface for docker image inspect and docker build commands. The third call (openshell sandbox connect with stdin redirection) retains shell form as it requires < redirection and the openshell binary path is not directly accessible in the OnboardContext. Partial fix for NVIDIA#1889
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes refactor Docker command execution in the agent onboarding setup from shell-constructed strings to argv-based calls. Two Docker commands—image inspection and build—are converted for improved safety and explicit stdio control. A clarifying comment is added explaining why the sandbox connect command must remain shell-based. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✨ Thanks for submitting this PR that proposes a security fix for the agent-onboard by migrating docker shell strings to argv arrays, which could help prevent security issues with the agent-onboard. Possibly related open issues: |
|
Hey @Sanjays2402 — thanks for picking this up! Appreciate you diving into the argv migration tracked in #1889. Heads up that this work was already covered by #1915 (merged Apr 20), which converted all three Timeline for reference:
Looks like the branches crossed in flight. Can you confirm whether everything you intended here is covered by #1915? If so, I'll go ahead and close this out. There are still a few remaining |
Summary\n\nPartial migration of shell-string
run()calls to argv arrays insrc/lib/agent-onboard.ts, as tracked in #1889.\n\n## Changes\n\nConverts 2 of 3 shell-string calls to structurally safe argv arrays:\n-docker image inspect— no longer uses shell interpolation\n-docker build— no longer uses shell interpolation\n\nThe third call (openshell sandbox connectwith stdin<redirection) retains shell form because it requires stdin redirection and the openshell binary path is not directly accessible from theOnboardContext. A full migration of this callsite requires exposinggetOpenshellBinary()orrunOpenshell()in the context.\n\nPartial fix for #1889Summary by CodeRabbit
Release Notes
This release contains internal maintenance and code improvements with no user-facing changes. Updates to underlying infrastructure components enhance reliability and code quality without affecting end-user functionality or features.