fix(cli): restrict the maintained update fetch to HTTPS - #9862
Conversation
`nemoclaw update` runs `curl -fsSL <installer> | bash`, and `curl -fsSL` follows an HTTPS-to-HTTP downgrade redirect, so plaintext-fetched bytes reach `bash`. Add `--proto '=https' --proto-redir '=https'` so the transfer fails closed instead, matching the installer fetch in `src/lib/onboard/install-ollama-linux.ts`. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe update action now shares HTTPS-only curl restrictions across default and branded installer commands. Tests verify command guidance, maintained installer execution, and unchanged leading-zero version handling. ChangesUpdate download hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The update command now restricts both initial requests and redirects to HTTPS before piping content to bash; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
There was a problem hiding this comment.
The executed update pipeline correctly restricts the initial transfer and redirects to HTTPS, but the user-visible maintained update commands still bypass that control. updateBranding() constructs weaker normal, Hermes, and Deep Agents Code commands, and printStatus() presents them as the maintained path. A user copying one can follow an HTTPS-to-HTTP redirect and pipe plaintext-fetched bytes into Bash.
Derive all reported commands from the same HTTPS-restricted curl command, adding only the branded NEMOCLAW_AGENT=... assignment. Add normal, Hermes, and Deep Agents Code --check output assertions for both --proto '=https' and --proto-redir '=https' plus the expected agent assignment. This is protocol restriction under normal Web PKI, not certificate or artifact pinning; no new redirect-host allowlist is required in this focused fix.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Review and Security Follow-UpAddressed the current change request in the latest PR revision. The normal NemoClaw, NemoHermes, and NemoDeepAgents status guidance now derives from the same HTTPS-restricted installer fetch used by the executed update path. Tests cover both protocol flags and the expected agent assignment for every displayed variant. Security review: PASS. The change strengthens initial-transfer and redirect transport requirements under normal Web PKI. It does not add user-controlled command construction, credential handling, authorization changes, dependencies, persistent state, or concurrency behavior. Validation:
The current diff is 67 additions and 27 deletions across two files, so it is not a large increase. Fresh repository checks and repository-routed human re-review remain required before merge. |
The blocking finding is addressed at the latest PR commit. CI remains a separate approval gate.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Current Main SynchronizationThe contributor branch now includes current Validation after synchronization:
The push started fresh standard checks. I did not request a workflow rerun. Merge remains deferred until every required check passes and an independent repository review approves the current revision. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
<!-- markdownlint-disable MD041 --> ## Summary Complete the v0.0.114 documentation for user-visible behavior that the cumulative post-merge workflow missed. The update covers managed-image onboarding, managed vLLM GPU selection, messaging provider lifecycle, paused channel status, Deep Agents tool discovery, Portable lifecycle timing, HTTPS-only updates, and current Hermes runtime architecture. ## Changes - Complete the v0.0.114 changelog for merged PRs #9323, #9862, #9913, #9964, #10021, #10025, #10026, #10031, #10047, and #10052. - Document managed vLLM GPU selection, resume constraints, and GPU-specific preflight behavior. - Document exact endpointless messaging-provider validation and stopped Hermes Discord provider retention across rebuild. - Document the paused detailed channel-status JSON contract and Portable lifecycle timing output. - Correct the Hermes managed-startup architecture description and Deep Agents loaded MCP tool discovery behavior. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This PR updates public documentation to match already tested source behavior and adds no runtime code. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: An independent documentation review checked credential custody, provider reuse, stopped-channel effects, pairing claim boundaries, GPU selection, variant routing, and recovery guidance against current source and tests. The first review's blockers were corrected, and the final review is recorded in the authoring evidence. - [ ] 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: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: This documentation-only change does not modify `scripts/prepare-dgx-station-host.sh`. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — documentation-only change; targeted runtime tests are not applicable - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not run; the PR changes documentation only - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) — completed with 0 errors and 2 existing Fern warnings hidden by default - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — no new pages --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Select managed vLLM GPUs by index or UUID, with selections preserved when resuming setup. - View detailed Portable recovery timing and action results. - Discover late-loaded managed tools through progressive tool search. - **Bug Fixes** - Improved sandbox rebuild handling for stopped messaging channels. - Strengthened provider validation, pairing checks, recovery handoffs, and duplicate tool detection. - Added safer managed-image onboarding and approval-flow handling. - Update downloads and redirects now require HTTPS. - **Documentation** - Expanded guidance for onboarding, vLLM configuration, messaging channels, recovery, architecture, and CLI commands. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
nemoclaw updatepreviously allowed its HTTPS installer request to follow a redirect to a non-HTTPS location. The maintained fetch now requires HTTPS for the initial request and every redirect before any bytes are piped tobash.Related Issue
Fixes #9861
Changes
curl --proto '=https' --proto-redir '=https'to the maintained update fetch.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/update.test.tspassed 41 tests after synchronizing currentmain.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Udaya Tejas udayatejas2004@gmail.com
Summary by CodeRabbit
Security Enhancements
Improvements
--checkoutput now displays the maintained update command.