Skip to content

fix(cli): pin curl protocol for nemoclaw update installer fetch - #9976

Closed
Adityaj0 wants to merge 2 commits into
NVIDIA:mainfrom
Adityaj0:fix/update-curl-proto-pin-9861
Closed

fix(cli): pin curl protocol for nemoclaw update installer fetch#9976
Adityaj0 wants to merge 2 commits into
NVIDIA:mainfrom
Adityaj0:fix/update-curl-proto-pin-9861

Conversation

@Adityaj0

@Adityaj0 Adityaj0 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

nemoclaw update runs curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash without pinning the transfer protocol, so a redirect to a non-HTTPS location can smuggle unauthenticated bytes into the pipeline before curl's TLS check applies. This adds --proto '=https' --proto-redir '=https' to that fetch, the same hardening src/lib/onboard/install-ollama-linux.ts already applies for the identical fetch-shape issue (#9698 / #9703), so the transfer now fails closed on a downgrade redirect instead of piping whatever arrives.

Related Issue

Fixes #9861

Changes

  • Added a curlPipeBashCommand() helper in src/lib/actions/update.ts that builds the installer fetch with --proto '=https' --proto-redir '=https'.
  • NEMOCLAW_UPDATE_COMMAND (the command nemoclaw update actually executes) and the NemoHermes/NemoDeepAgents maintainedUpdateCommand guidance strings now build from that shared helper instead of three separate unpinned literals.
  • Updated src/lib/actions/update.test.ts assertions for the new pinned guidance strings and added a regression test asserting NEMOCLAW_UPDATE_COMMAND carries both proto flags.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: n/a — scripts/prepare-dgx-station-host.sh is not touched by this change
  • Station profile/scenario: n/a
  • Result: n/a
  • Supporting evidence: n/a

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run src/lib/actions/update.test.ts --project cli: 41/41 passed
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Aditya Jain adityaj0714@gmail.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved installer update commands to require secure HTTPS connections for downloads and redirects.
    • Updated Hermes and Deep Agents installer guidance to use HTTPS-only protocols.
    • Ensured pinned update commands retain secure HTTPS enforcement throughout the installation process.

`nemoclaw update` piped `curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash`
without pinning the transfer protocol. `curl -fsSL` follows a redirect to a
non-HTTPS location, so a downgrade redirect could smuggle plaintext bytes into
the pipeline before they reach bash. Add `--proto '=https' --proto-redir
'=https'`, the same hardening already used by
src/lib/onboard/install-ollama-linux.ts for the same fetch-shape issue
(NVIDIA#9698 / NVIDIA#9703).

Fixes NVIDIA#9861

Signed-off-by: Aditya Jain <adityaj0714@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6a87b341-aeb7-4935-9cac-7a4ed2c45004

📥 Commits

Reviewing files that changed from the base of the PR and between 7601ab2 and 7a8e05e.

📒 Files selected for processing (2)
  • src/lib/actions/update.test.ts
  • src/lib/actions/update.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The update commands now use a shared curl builder that permits HTTPS requests and HTTPS redirects only. Hermes and LangChain DeepAgents retain their agent assignments. Tests cover the hardened commands and preserve existing version-ordering behavior.

Changes

Installer transport hardening

Layer / File(s) Summary
Hardened installer command builder
src/lib/actions/update.ts
The shared builder adds HTTPS-only curl options for direct requests and redirects. Maintained Hermes and LangChain DeepAgents commands use the builder.
Installer command validation
src/lib/actions/update.test.ts
Tests require HTTPS for installer commands and validate both request and redirect protocols. The version-ordering test is reformatted without behavior changes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 7a8e0

The update installer now refuses non-HTTPS redirects before piping fetched bytes to the shell; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: cv, rluo8

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes enforce HTTPS for the initial fetch and redirects in all required nemoclaw update commands, satisfying issue #9861.
Out of Scope Changes check ✅ Passed The code and test changes remain focused on HTTPS protocol hardening for nemoclaw update installer fetches.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: enforcing HTTPS for the nemoclaw update installer fetch.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: PR review advisor skipped: OpenShell inference configuration failed or the advisor credential is unavailable

Model lanes

  • GPT-5.6 Terra (primary): Skipped
  • Nemotron 3 Ultra (second opinion): Skipped

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed latest PR commit 7a8e05e. The update command and its generated guidance consistently restrict the installer fetch to HTTPS. The current merge conflict does not reveal a defect in the reviewed patch.

@prekshivyas
prekshivyas enabled auto-merge (squash) August 23, 2026 08:57
@prekshivyas
prekshivyas disabled auto-merge August 23, 2026 08:57
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@prekshivyas

Copy link
Copy Markdown
Collaborator

Closing as superseded by #9862, which merged the same HTTPS protocol pinning for the maintained update commands together with regression coverage. After synchronizing this branch with current main, #9976 has no remaining diff. Thank you @Adityaj0 for identifying and addressing #9861.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nemoclaw update pipes an unpinned-protocol curl into bash

2 participants