fix(security): download installers to file before execution#696
fix(security): download installers to file before execution#696
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced direct remote "curl | sh/bash" pipelines with download-to-temp-file-then-execute flows in multiple scripts, added explicit download error handling and guaranteed temp-dir cleanup, and added a regression test detecting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 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
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
|
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 |
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.
- 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
cv
force-pushed
the
fix/curl-bash-integrity
branch
from
ce4044b to
babfb2e
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
process.exit(1) on line 252 bypasses the finally cleanup. process.exit() in Node.js terminates immediately without executing finally blocks, so if the curl download fails, uninstallDir persists on disk. This is the scenario the second commit was meant to fix per its message ("Move exitWithSpawnResult outside try/finally so process.exit does not bypass tmpdir cleanup") — the fix was applied to exitWithSpawnResult but not to this process.exit(1). Replace with a throw or set a flag and exit after the finally.
Otherwise this looks ready to merge. The security scope is accurately described (partial-download prevention, not supply chain), cleanup is handled correctly in the shell scripts via subshell + trap, and the regression tests are well-constructed.
wscurran
added
CI/CD
Main refactored scripts/install.sh into a thin wrapper delegating to the root install.sh. The PR's NodeSource curl|bash fix for this file is no longer needed since the logic was removed. Taking main's version.
Reviewer guidance — specific risk areas to verify
|
The sign-off was inside the Summary section where contributors would delete it when writing their description. Move it to the bottom after a separator with a CI-required comment so it persists through edits.
process.exit(1) inside the try block bypasses the finally that removes uninstallDir. Use a flag instead so cleanup always runs.
…x/curl-bash-integrity-resolve
|
❌ Brev E2E (full): FAILED on branch
|
|
❌ Brev E2E (full): FAILED on branch |
5 participants
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
Signed-off-by: Aaron Erickson aerickson@nvidia.com