fix(security): disable remote uninstall execution fallback#1476
fix(security): disable remote uninstall execution fallback#1476Saibabu7770 wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Stop executing downloaded uninstall scripts when local uninstall.sh is missing. Require manual review/download instead and add a regression guard to prevent reintroducing remote execution fallback. Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR removes remote uninstall script fallback functionality for security reasons. When no local uninstall script exists, the command now returns an error message with the script URL and instructions for manual download/review, exiting with status 1, instead of automatically downloading and executing a remote script via curl. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runner.test.js`:
- Around line 666-670: The test's regression guard in uninstallBlock (extracted
from "function uninstall(args)") is too narrow: update the assertions to detect
any remote execution fallback, not just the literal spawnSync("bash",
[uninstallScript...]) form. Replace the second not.toMatch with a broader regex
against uninstallBlock that matches spawnSync("bash", ...) when the argument
array contains uninstallScript (for example
/spawnSync\(\s*["']bash["']\s*,\s*\[[^\]]*uninstallScript[^\]]*\]/) and keep the
existing execFileSync("curl") check (or broaden it to
/exec(File)?Sync\(\s*["']curl["']/) so any variant that invokes curl/remote
script is caught; use the uninstallBlock and "function uninstall(args)" context
to locate where to change the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 085c13ad-8930-4fbc-8bf9-ad3f7c245e0a
📒 Files selected for processing (2)
bin/nemoclaw.jstest/runner.test.js
Strengthen the NVIDIA#577 regression check to catch broader curl and bash uninstallScript execution patterns so remote uninstall fallback cannot silently regress under small implementation variations. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/runner.test.js (1)
753-755:⚠️ Potential issue | 🟠 MajorHarden this regression guard; current pattern is too narrow.
The current check only blocks the exact
spawnSync("bash", [uninstallScript...])form, so a remote fallback can be reintroduced with a different variable/signature and still pass. Also, chainedsplit(...)[1]is brittle.Suggested hardening
- const uninstallBlock = src.split("function uninstall(args)")[1].split("function showStatus")[0]; - expect(uninstallBlock).not.toMatch(/execFileSync\("curl"/); - expect(uninstallBlock).not.toMatch(/spawnSync\("bash", \[uninstallScript/); + const start = src.indexOf("function uninstall(args)"); + const end = src.indexOf("function showStatus", start); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const uninstallBlock = src.slice(start, end); + expect(uninstallBlock).not.toMatch(/exec(File)?Sync\(\s*["']curl["']/); + expect(uninstallBlock).not.toMatch(/spawnSync\(\s*["']bash["']\s*,\s*\[\s*(?!localScript\b)/); expect(uninstallBlock).toContain("Remote uninstall fallback is disabled for security.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 753 - 755, The current test extracts uninstallBlock via brittle chained splits and only looks for exact patterns execFileSync("curl" and spawnSync("bash", [uninstallScript), which is too narrow; update the test to robustly extract the uninstall function body (e.g., match the function uninstall(...) { ... } block with a single regex like /function uninstall\([^)]*\)\s*{[\s\S]*?}/) into the uninstallBlock variable and change the assertions to reject broader remote-execution patterns such as any exec/execFile/spawn/spawnSync/child_process usage invoking curl|wget|http(s) URLs or invoking bash/sh with any argument that contains "uninstallScript" or an http(s) URL; keep using uninstall and showStatus as anchors to locate the function body and replace the two strict expect(...).not.toMatch(...) checks with regexes that cover these wider cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/runner.test.js`:
- Around line 753-755: The current test extracts uninstallBlock via brittle
chained splits and only looks for exact patterns execFileSync("curl" and
spawnSync("bash", [uninstallScript), which is too narrow; update the test to
robustly extract the uninstall function body (e.g., match the function
uninstall(...) { ... } block with a single regex like /function
uninstall\([^)]*\)\s*{[\s\S]*?}/) into the uninstallBlock variable and change
the assertions to reject broader remote-execution patterns such as any
exec/execFile/spawn/spawnSync/child_process usage invoking curl|wget|http(s)
URLs or invoking bash/sh with any argument that contains "uninstallScript" or an
http(s) URL; keep using uninstall and showStatus as anchors to locate the
function body and replace the two strict expect(...).not.toMatch(...) checks
with regexes that cover these wider cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a93459c-ae87-4ae5-9f0d-2140cf93ebb7
📒 Files selected for processing (2)
bin/nemoclaw.jstest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
wscurran
added
security
|
✨ Thanks for submitting this fix, which proposes a way to disable remote uninstall script execution fallback, improving security by requiring manual review. |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Requesting changes: this PR has merge conflicts due to the TS migration (#1673) and the uninstall handler extraction (#1549). The uninstall function was moved out of bin/nemoclaw.js and test/runner.test.js was migrated to TypeScript, so both files this PR touches no longer exist in their original form. The fix itself is correct and wanted, but needs to be re-applied to the new TypeScript locations rather than conflict-resolved. Marking as request-changes to prevent accidental merge.
Merge remote-tracking branch 'upstream/main' into fix/uninstall-fallback-no-curl-pipe-577. Resolve bin/nemoclaw.js to the dist bootstrap entrypoint. Remove curl/bash remote uninstall execution from src/lib/uninstall-command.ts (NVIDIA#577). Update uninstall-command tests, runner regression guard, and help text. Made-with: Cursor
4 participants
Stop executing downloaded uninstall scripts when local uninstall.sh is missing. Require manual review/download instead and add a regression guard to prevent reintroducing remote execution fallback.
Summary
Related Issue
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Your Name your-email@example.com
Summary by CodeRabbit