fix(security): download installers to file before execution#696
Open
fix(security): download installers to file before execution#696
Conversation
Replace all curl-pipe-to-shell patterns with download-to-tempfile: - install.sh: Ollama installer (2 locations) - scripts/install.sh: NodeSource setup - scripts/brev-setup.sh: NodeSource setup - bin/nemoclaw.js: remote uninstall fallback Each uses mktemp -d, downloads to file, executes, cleans up. Prevents partial-download execution and allows inspection. SHA-256 pinning isn't practical for rolling-release upstream URLs. 4 regression tests verify no curl-pipe-to-shell in any of these files. Closes #574, #576, #577, #583.
📝 WalkthroughWalkthroughReplaced inline Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (1)
bin/nemoclaw.js (1)
243-248: Pin the fallback uninstall script to a versioned ref.This path still executes
REMOTE_UNINSTALL_URL, which points atrefs/heads/main. Download-to-file avoids partial execution, but it also means an older client can end up running whatever uninstall script happens to be onmainthat day. Using the installed tag/commit or a release asset would make this fallback deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 243 - 248, The fallback currently downloads REMOTE_UNINSTALL_URL (which points at refs/heads/main); change the download to a deterministic, versioned URL by constructing or substituting a pinned ref (e.g., a RELEASE_TAG, package version, or the installed commit) before calling execFileSync—update the variable used by execFileSync (REMOTE_UNINSTALL_URL) or create a new pinnedUninstallUrl and use that when fetching into uninstallScript, keeping the same execFileSync/ spawnSync flow with uninstallDir, uninstallScript, and args so the fallback always retrieves a specific tag/commit or release asset instead of main.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 246-255: The try/finally currently calls execFileSync and
exitWithSpawnResult inside the try so process.exit prevents the finally cleanup
and download errors leak stack traces and uninstallDir; refactor by isolating
the download: wrap the execFileSync( "curl", [...REMOTE_UNINSTALL_URL...,
uninstallScript]) call in its own try-catch that logs a clean user-friendly
error and exits (or throws) on failure, leave the spawnSync(...) invocation and
its result handling separate, move exitWithSpawnResult(result) to after the
try/finally that performs fs.rmSync(uninstallDir, { recursive: true, force: true
}) so cleanup always runs, and keep references to execFileSync, spawnSync,
exitWithSpawnResult, uninstallDir, REMOTE_UNINSTALL_URL, and uninstallScript to
locate and modify the code.
In `@scripts/brev-setup.sh`:
- Around line 41-44: The temporary directory created in tmpdir=$(mktemp -d) may
not be removed if curl or sudo -E bash "$tmpdir/setup_node.sh" fails; add a
cleanup trap that removes "$tmpdir" on exit/failure (e.g., trap 'rm -rf
"$tmpdir"' EXIT) immediately after creating tmpdir, and ensure tmpdir is
referenced in that trap so rm -rf "$tmpdir" always runs regardless of errors in
the subsequent curl or sudo -E bash commands; keep the existing rm -rf "$tmpdir"
for explicit cleanup after successful execution.
In `@scripts/install.sh`:
- Around line 235-238: The temporary directory created with tmpdir=$(mktemp -d)
can be leaked if curl or sudo -E bash fails; add a cleanup trap immediately
after creating tmpdir (e.g., trap 'rm -rf "$tmpdir"' EXIT) so rm -rf "$tmpdir"
runs on both success and failure, and ensure the trap is removed or left in
place as appropriate after cleanup; update the block containing tmpdir, mktemp
-d, curl -fsSL ... -o "$tmpdir"/setup_node.sh, and sudo -E bash
"$tmpdir"/setup_node.sh to rely on the trap for robust cleanup.
In `@test/runner.test.js`:
- Around line 213-245: The current per-line checks (isViolation, and the inline
filter in the bin/nemoclaw.js test) miss multiline curl-pipe-to-shell cases;
update the tests ("install.sh does not pipe curl to shell",
"scripts/install.sh...", "scripts/brev-setup.sh...", and "bin/nemoclaw.js does
not pipe curl to shell") to detect line-wrapped pipes by normalizing input first
(e.g., join logical shell continuations or remove trailing backslashes and join
adjacent lines, or collapse newline + indentation into a single space) before
applying the curl→shell regex, or alternatively run a multiline regex against
the whole file content to catch patterns like "curl ...
|\\s*\\n\\s*(sh|bash|sudo)"; apply this change where isViolation is defined and
where the inline filter is used so multiline piped shells are flagged.
---
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 243-248: The fallback currently downloads REMOTE_UNINSTALL_URL
(which points at refs/heads/main); change the download to a deterministic,
versioned URL by constructing or substituting a pinned ref (e.g., a RELEASE_TAG,
package version, or the installed commit) before calling execFileSync—update the
variable used by execFileSync (REMOTE_UNINSTALL_URL) or create a new
pinnedUninstallUrl and use that when fetching into uninstallScript, keeping the
same execFileSync/ spawnSync flow with uninstallDir, uninstallScript, and args
so the fallback always retrieves a specific tag/commit or release asset instead
of main.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc10a599-bd1b-4857-b393-5f3242266022
📒 Files selected for processing (5)
bin/nemoclaw.jsinstall.shscripts/brev-setup.shscripts/install.shtest/runner.test.js
- Move exitWithSpawnResult outside try/finally in nemoclaw.js so process.exit does not bypass tmpdir cleanup - Wrap download in inner try/catch for user-friendly error on failure - Use subshell + trap EXIT for tmpdir cleanup in all shell scripts so set -euo pipefail exits don't leak temp directories - Normalize test input (join continuations, collapse multiline pipes) to catch line-wrapped curl-pipe-to-shell regressions - Tighten violation regex to match sh/bash/sudo bash but not sudo gpg
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/nemoclaw.js (1)
247-253:⚠️ Potential issue | 🟠 MajorMove failure exit out of the
tryblock to guarantee temp-dir cleanup.At Line 252,
process.exit(1)is called inside the outertry; this can terminate before thefinallyat Line 259-260 runs, leavinguninstallDirbehind on curl failures.Suggested fix
const uninstallScript = path.join(uninstallDir, "uninstall.sh"); let result; + let downloadFailed = false; try { try { execFileSync("curl", ["-fsSL", REMOTE_UNINSTALL_URL, "-o", uninstallScript], { stdio: "inherit" }); } catch { console.error(` Failed to download uninstall script from ${REMOTE_UNINSTALL_URL}`); - process.exit(1); + downloadFailed = true; } - result = spawnSync("bash", [uninstallScript, ...args], { - stdio: "inherit", - cwd: ROOT, - env: process.env, - }); + if (!downloadFailed) { + result = spawnSync("bash", [uninstallScript, ...args], { + stdio: "inherit", + cwd: ROOT, + env: process.env, + }); + } } finally { fs.rmSync(uninstallDir, { recursive: true, force: true }); } + if (downloadFailed) process.exit(1); exitWithSpawnResult(result);#!/bin/bash set -euo pipefail # Inspect current control flow around the changed block. cat -n bin/nemoclaw.js | sed -n '240,265p' # Verify there is a process.exit call inside a try/finally structure. ast-grep --lang javascript --pattern $'try { $$$ process.exit($_); $$$ } finally { $$$ }' bin/nemoclaw.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 247 - 253, The current code calls process.exit(1) inside the try block which can bypass the finally that cleans up uninstallDir; change the flow so the try/finally always runs and exit happens after finally: remove the process.exit(1) from inside the try, instead set a local exitCode variable (e.g., let exitCode = 0) and in the inner catch handling execFileSync/REMOTE_UNINSTALL_URL/uninstallScript set exitCode = 1 (and log the error), then after the try/finally block check exitCode and call process.exit(exitCode); this guarantees uninstallDir cleanup while preserving the failure exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/nemoclaw.js`:
- Around line 247-253: The current code calls process.exit(1) inside the try
block which can bypass the finally that cleans up uninstallDir; change the flow
so the try/finally always runs and exit happens after finally: remove the
process.exit(1) from inside the try, instead set a local exitCode variable
(e.g., let exitCode = 0) and in the inner catch handling
execFileSync/REMOTE_UNINSTALL_URL/uninstallScript set exitCode = 1 (and log the
error), then after the try/finally block check exitCode and call
process.exit(exitCode); this guarantees uninstallDir cleanup while preserving
the failure exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb8d548b-de0d-4c8d-92fc-024c101462c9
📒 Files selected for processing (5)
bin/nemoclaw.jsinstall.shscripts/brev-setup.shscripts/install.shtest/runner.test.js
✅ Files skipped from review due to trivial changes (2)
- scripts/install.sh
- test/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/brev-setup.sh
- install.sh
Contributor
|
Very nice. Curious about your thoughts on the NodeSource paths specifically: curl -fsSL https://nvidia.github.io/libnvidia-container/gpgkey \
| sudo gpg --dearmor -o /usr/share/keyrings/nvidia-container-toolkit-keyring.gpgThe same pattern works for NodeSource — add the GPG key + apt source directly instead of downloading and running their setup script: curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key \
| sudo gpg --dearmor -o /usr/share/keyrings/nodesource.gpg
echo "deb [signed-by=/usr/share/keyrings/nodesource.gpg] https://deb.nodesource.com/node_22.x nodistro main" \
| sudo tee /etc/apt/sources.list.d/nodesource.list > /dev/null
sudo apt-get update -qq && sudo apt-get install -y -qq nodejs |
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
2 participants
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.
Summary
Replace all
curl | bash/curl | sudo bashpatterns with download-to-tempfile-then-execute across the codebase.Closes #574, #576, #577, #583.
install.shscripts/install.shsetup_22.xscripts/brev-setup.shsetup_22.xbin/nemoclaw.jsEach location now uses
mktemp -d, downloads the script to a file, executes from the file, and cleans up. SHA-256 pinning isn't practical for rolling-release upstream URLs, but download-then-execute prevents partial-download execution and allows inspection.Test plan
curl | sh/bash/sudopatterns in any of the affected filesnemoclaw onboardwith Ollama provider installs correctlynemoclaw uninstallfalls back to remote script correctlySummary by CodeRabbit
Bug Fixes
Tests