fix(security): disable remote uninstall execution fallback#1476
Open
Saibabu7770 wants to merge 3 commits intoNVIDIA:mainfrom
Open
fix(security): disable remote uninstall execution fallback#1476Saibabu7770 wants to merge 3 commits intoNVIDIA:mainfrom
Saibabu7770 wants to merge 3 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
Contributor
📝 WalkthroughWalkthroughRemoved automatic remote uninstall fallback from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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}
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Tests