fix(security): replace process.env spread with allowlist in blueprint runner - #1874
Merged
Conversation
… runner
The runner's provider-create subprocess received the full parent
process.env, leaking secrets like NVIDIA_API_KEY, GITHUB_TOKEN, and
AWS_ACCESS_KEY_ID to child processes. Replace the { ...process.env }
spread with buildSubprocessEnv(), which forwards only an explicit
allowlist of system variables (PATH, HOME, LANG, etc.) and injects
credentials through the existing credEnv overlay.
Adds runtime regression test (runner.test.ts) and static analysis
guard (credential-exposure.test.ts) to prevent reintroduction.
Closes NVBug 6010004
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughReplace full inheritance of host Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Process\n(process.env)
participant Runner as Runner / Onboard / Services
participant Builder as buildSubprocessEnv\n(allowlist)
participant Subproc as Subprocess\n(execa / spawn)
Parent->>Runner: invoke action/start/create
Runner->>Builder: buildSubprocessEnv(optional extra)
Builder->>Builder: filter process.env by allowed names/prefixes
Runner->>Builder: provide explicit credential overlay (extra)
Builder-->>Runner: return filteredEnv + extra
Runner->>Subproc: spawn subprocess with env = filteredEnv+extra
Subproc-->>Runner: subprocess starts (no parent secrets)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
…list Extend the env allowlist to include variables needed in corporate environments: HTTP(S)_PROXY, NO_PROXY, SSL_CERT_*, NODE_EXTRA_CA_CERTS, DOCKER_HOST, KUBECONFIG, SSH_AUTH_SOCK, RUST_LOG/BACKTRACE, and OPENSHELL_*/GRPC_* prefixes. Without these the subprocess would silently fail behind proxies or with custom CA bundles. Adds test assertions for HTTPS_PROXY and OPENSHELL_* passthrough. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/blueprint/runner.test.ts`:
- Around line 433-464: The finally block in the test "does not leak parent
secrets into subprocess env (NVBug 6010004)" unconditionally deletes process.env
keys (MY_API_KEY, GITHUB_TOKEN, AWS_ACCESS_KEY_ID, NVIDIA_API_KEY), which can
erase pre-existing values and pollute other tests; modify the test to capture
the original values before setting them (e.g., save originalMyApiKey =
process.env.MY_API_KEY, etc.), then in the finally restore each env var to its
original value (setting it back if defined, or deleting it only if it was
originally undefined), keeping the rest of the test logic (actionApply,
mockExeca lookup) unchanged so that cleanup safely restores prior state.
In `@test/credential-exposure.test.ts`:
- Around line 42-52: The test currently only scans per-line so a spread of
process.env across multiple lines can evade the check; update the check in
credential-exposure.test.ts to scan the full RUNNER_TS source with a
multiline-capable regex (update spreadRe to use [\s\S]* or the /s flag, e.g.
/env:\s*\{[\s\S]*\.\.\.process\.env/) and run it against the entire src string
instead of line-by-line, and also ensure you ignore matches inside comments by
stripping or ignoring both // and /* */ comments before testing; keep the
expectation that runner.ts must use buildSubprocessEnv() and assert no
violations.
🪄 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 Plus
Run ID: 3ce6ac14-7a36-4a6e-81e5-aec5698008dd
📒 Files selected for processing (3)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.tstest/credential-exposure.test.ts
Move the env var allowlist out of runner.ts into a dedicated shared module (subprocess-env.ts) mirrored in both the CLI and plugin projects. Apply it to all three subprocess spawn sites: - nemoclaw/src/blueprint/runner.ts (provider create) - src/lib/services.ts (cloudflared spawn) - src/lib/onboard.ts (sandbox create — replaces the old blocklist) The allowlist is structured by category (system, temp, locale, proxy, TLS, toolchain) with named prefix groups (LC_, XDG_, OPENSHELL_, GRPC_). The old hand-rolled blocklist in onboard.ts is removed — the allowlist approach is strictly more secure because it rejects unknown/future secrets by default. Static analysis guards in credential-exposure.test.ts now cover all three call sites. Closes NVBug 6010004 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.agents/skills/nemoclaw-user-workspace/references/workspace-files.md (1)
50-50: Tighten duplicated phrasing in the warning reference.Line 50 currently repeats “See … (see …)”. Consider simplifying to one reference to avoid awkward wording.
✍️ Suggested wording
-> See Backup and Restore (see the `nemoclaw-user-workspace` skill) for instructions. +> See **Backup and Restore** in the `nemoclaw-user-workspace` skill for instructions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/nemoclaw-user-workspace/references/workspace-files.md at line 50, The phrase "See Backup and Restore (see the `nemoclaw-user-workspace` skill) for instructions." repeats "see"; update the sentence in workspace-files.md to remove the duplication—e.g., "See Backup and Restore in the `nemoclaw-user-workspace` skill for instructions." Edit the line containing that sentence so it references the `nemoclaw-user-workspace` skill once and retains the same meaning.nemoclaw/src/lib/subprocess-env.ts (1)
17-18: Prevent mirror drift for this security-critical helper.Since this file is intentionally mirrored in another project, add a parity guard (test or generation step) so allowlist changes cannot diverge silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/lib/subprocess-env.ts` around lines 17 - 18, Add an automated parity guard to prevent the mirrored src/lib/subprocess-env.ts from drifting: implement a unit/integration test (or CI generation check) that loads this file and its counterpart in the CLI repo, normalizes whitespace/comments, and asserts their contents (or computed checksum) are identical; name the test something like "subprocess-env parity" and fail the build/CI if the files differ so allowlist changes cannot diverge silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/nemoclaw-user-workspace/SKILL.md:
- Line 3: Fix the frontmatter "description" field in SKILL.md by correcting
typos and improving grammar: replace "Hows" with "How to" and "Whats" with
"What", and rephrase the sentence to read like a single clear clause (for
example: "How to back up and restore OpenClaw workspace files before destructive
operations, what workspace personality and configuration files are, where they
live, and how they persist across sandbox restarts."); update the description
value in the SKILL.md frontmatter accordingly and regenerate the auto-generated
skill file so the docs output matches this corrected text.
- Around line 56-57: Update the warning text for the "nemoclaw <name> destroy"
command to be less absolute about PVC deletion: replace the phrase that says it
"deletes the sandbox and its PVC" with wording that reflects the implementation
path ("deletes the sandbox") and that sandbox deletion may remove associated
workspace storage (e.g., PVCs) and that files can be permanently lost unless
backed up; reference the command name "nemoclaw <name> destroy" and the concept
"sandbox delete" when making the change.
---
Nitpick comments:
In @.agents/skills/nemoclaw-user-workspace/references/workspace-files.md:
- Line 50: The phrase "See Backup and Restore (see the `nemoclaw-user-workspace`
skill) for instructions." repeats "see"; update the sentence in
workspace-files.md to remove the duplication—e.g., "See Backup and Restore in
the `nemoclaw-user-workspace` skill for instructions." Edit the line containing
that sentence so it references the `nemoclaw-user-workspace` skill once and
retains the same meaning.
In `@nemoclaw/src/lib/subprocess-env.ts`:
- Around line 17-18: Add an automated parity guard to prevent the mirrored
src/lib/subprocess-env.ts from drifting: implement a unit/integration test (or
CI generation check) that loads this file and its counterpart in the CLI repo,
normalizes whitespace/comments, and asserts their contents (or computed
checksum) are identical; name the test something like "subprocess-env parity"
and fail the build/CI if the files differ so allowlist changes cannot diverge
silently.
🪄 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 Plus
Run ID: 089fbc98-0abb-40ae-bc0f-890a6e72ed36
📒 Files selected for processing (9)
.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-workspace/SKILL.md.agents/skills/nemoclaw-user-workspace/references/workspace-files.mdnemoclaw/src/blueprint/runner.tsnemoclaw/src/lib/subprocess-env.tssrc/lib/onboard.tssrc/lib/services.tssrc/lib/subprocess-env.tstest/credential-exposure.test.ts
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/nemoclaw-user-reference/references/commands.md
🚧 Files skipped from review as they are similar to previous changes (2)
- nemoclaw/src/blueprint/runner.ts
- test/credential-exposure.test.ts
| --- | ||
| name: "nemoclaw-user-workspace" | ||
| description: "Backs up and restores OpenClaw workspace files before destructive operations. Use when backing up a sandbox, restoring workspace state, or preparing for a destructive operation. Explains what workspace files are, where they live, and how they persist across sandbox restarts. Use when asking about soul.md, identity.md, memory.md, agents.md, or sandbox file persistence." | ||
| description: "Hows to back up and restore OpenClaw workspace files before destructive operations. Whats workspace personality and configuration files are, where they live, and how they persist across sandbox restarts." |
Contributor
There was a problem hiding this comment.
Fix grammar in frontmatter description.
Line 3 has typos (“Hows”, “Whats”) and reads ungrammatically. This is user-facing metadata and should be corrected in the docs source, then regenerated into this skill file.
✍️ Suggested text
-description: "Hows to back up and restore OpenClaw workspace files before destructive operations. Whats workspace personality and configuration files are, where they live, and how they persist across sandbox restarts."
+description: "How to back up and restore OpenClaw workspace files before destructive operations, and what workspace personality/configuration files are, where they live, and how they persist across sandbox restarts."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: "Hows to back up and restore OpenClaw workspace files before destructive operations. Whats workspace personality and configuration files are, where they live, and how they persist across sandbox restarts." | |
| description: "How to back up and restore OpenClaw workspace files before destructive operations, and what workspace personality/configuration files are, where they live, and how they persist across sandbox restarts." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/nemoclaw-user-workspace/SKILL.md at line 3, Fix the
frontmatter "description" field in SKILL.md by correcting typos and improving
grammar: replace "Hows" with "How to" and "Whats" with "What", and rephrase the
sentence to read like a single clear clause (for example: "How to back up and
restore OpenClaw workspace files before destructive operations, what workspace
personality and configuration files are, where they live, and how they persist
across sandbox restarts."); update the description value in the SKILL.md
frontmatter accordingly and regenerate the auto-generated skill file so the docs
output matches this corrected text.
Comment on lines
+56
to
+57
| Running `nemoclaw <name> destroy` **deletes the sandbox and its PVC**. | ||
| All workspace files are permanently lost unless you back them up first. |
Contributor
There was a problem hiding this comment.
Make destroy/PVC wording slightly less absolute.
Line 56 currently asserts direct PVC deletion. The implementation path uses sandbox delete, with storage cleanup as behavior of that operation; consider wording this as associated workspace storage removal for long-term accuracy.
✍️ Suggested wording
-Running `nemoclaw <name> destroy` **deletes the sandbox and its PVC**.
+Running `nemoclaw <name> destroy` **deletes the sandbox and its associated workspace storage**.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Running `nemoclaw <name> destroy` **deletes the sandbox and its PVC**. | |
| All workspace files are permanently lost unless you back them up first. | |
| Running `nemoclaw <name> destroy` **deletes the sandbox and its associated workspace storage**. | |
| All workspace files are permanently lost unless you back them up first. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/nemoclaw-user-workspace/SKILL.md around lines 56 - 57, Update
the warning text for the "nemoclaw <name> destroy" command to be less absolute
about PVC deletion: replace the phrase that says it "deletes the sandbox and its
PVC" with wording that reflects the implementation path ("deletes the sandbox")
and that sandbox deletion may remove associated workspace storage (e.g., PVCs)
and that files can be permanently lost unless backed up; reference the command
name "nemoclaw <name> destroy" and the concept "sandbox delete" when making the
change.
Keep onboard.ts on its battle-tested blocklist to avoid regression risk on the highest-traffic code path. The allowlist is applied only to runner.ts and services.ts where the blast radius is smaller. Also removes all internal bug tracker references from source code comments — those belong in commit messages, not in a public repo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/lib/subprocess-env.ts (1)
17-18: Reduce mirror-drift risk for security-critical logic.The “keep them in sync” comment is helpful, but manual sync can drift over time. Consider a shared source module (or parity test) so CLI/plugin allowlists can’t silently diverge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/lib/subprocess-env.ts` around lines 17 - 18, The comment warns that src/lib/subprocess-env.ts is a mirrored copy and can drift; replace the duplicated logic by extracting the security-critical allowlist logic into a single shared module (e.g., export functions/constants from a new shared package/module used by both subprocess-env.ts and the CLI counterpart) or add an automated parity test that imports both subprocess-env.ts and the CLI mirror to assert equality of exported allowlists/constants; update references in subprocess-env.ts to re-export or import from the shared module and add the parity test to CI to prevent silent divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/lib/subprocess-env.ts`:
- Around line 17-18: The comment warns that src/lib/subprocess-env.ts is a
mirrored copy and can drift; replace the duplicated logic by extracting the
security-critical allowlist logic into a single shared module (e.g., export
functions/constants from a new shared package/module used by both
subprocess-env.ts and the CLI counterpart) or add an automated parity test that
imports both subprocess-env.ts and the CLI mirror to assert equality of exported
allowlists/constants; update references in subprocess-env.ts to re-export or
import from the shared module and add the parity test to CI to prevent silent
divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c2b104f-c816-42ca-ae68-9960bba52f7d
📒 Files selected for processing (5)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/lib/subprocess-env.tssrc/lib/onboard.tssrc/lib/subprocess-env.tstest/credential-exposure.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard.ts
- nemoclaw/src/blueprint/runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/credential-exposure.test.ts
- src/lib/subprocess-env.ts
- Preserve original env var values in test cleanup instead of unconditionally deleting (prevents test pollution) - Use multiline-capable regex for static process.env spread detection so the guard cannot be evaded by splitting across lines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
approved these changes
Apr 14, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Anything to make onboard.ts smaller!
ColinM-sys
pushed a commit
to ColinM-sys/NemoClaw
that referenced
this pull request
Apr 14, 2026
… runner (NVIDIA#1874) ## Summary - Introduce `subprocess-env.ts` — a shared module (mirrored in both CLI and plugin projects) that exports `buildSubprocessEnv()`. Defines an env var allowlist structured by category (system, temp, locale, proxy, TLS, toolchain) with prefix groups (LC_, XDG_, OPENSHELL_, GRPC_). - Apply the allowlist to two subprocess spawn sites: - `nemoclaw/src/blueprint/runner.ts` — provider create (the original bug target) - `src/lib/services.ts` — cloudflared spawn (same vulnerable `{ ...process.env }` pattern) - `src/lib/onboard.ts` — **intentionally left on its existing blocklist** to avoid regression risk on the highest-traffic code path. A TODO marks the future migration. - Static analysis guards in `credential-exposure.test.ts` now cover all three call sites, preventing `...process.env` from creeping back into runner.ts or services.ts - Runtime regression test in `runner.test.ts` asserts secrets are stripped and proxy/openshell vars pass through ## Test plan - [x] `make check` — all hooks pass - [x] `npm test` — all 1528 tests pass - [x] New test: `runner.test.ts` "does not leak parent secrets into subprocess env" - [x] New test: `credential-exposure.test.ts` guards for runner.ts and services.ts - [ ] Manual: verify `nemoclaw apply` configures inference provider correctly - [ ] Manual: verify behind a corporate proxy (HTTPS_PROXY passthrough) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced allowlisted subprocess environment builder so spawned processes receive a filtered env plus explicit credentials. * **Bug Fixes** * Subprocess environment isolation: parent-process secrets are no longer forwarded; only explicit credentials and allowlisted system vars are passed. * **Tests** * Added regression checks to detect full-process env spreading and a test ensuring env filtering and test-env cleanup. * **Documentation** * Clarified destroy warning (includes persistent volume) and updated workspace file, persistence, backup, and "shell" usage guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: ColinM-sys <cmcdonough@50words.com>
ColinM-sys
added a commit
to ColinM-sys/NemoClaw
that referenced
this pull request
Apr 14, 2026
The sandbox creation path passed the host's entire process.env into the sandbox, filtered only by a 12-entry blocklist. Every env var NOT in that list leaked into the sandbox where the AI agent runs with code execution — including GITHUB_TOKEN, AWS_SECRET_ACCESS_KEY, SSH_AUTH_SOCK, KUBECONFIG, NPM_TOKEN, and any CI/CD secrets. Switch to the shared subprocess-env.ts allowlist (introduced in PR NVIDIA#1874) which inverts the default: only known-safe variables (HOME, PATH, TERM, locale, proxy, TLS) are forwarded. Everything else is dropped. Additionally strip KUBECONFIG and SSH_AUTH_SOCK from the sandbox- specific environment — the generic allowlist includes these for host-side processes but the sandbox should never have access to the host's Kubernetes cluster or SSH agent. This was flagged as a TODO at the original site (lines 2702-2705) but left unapplied to the sandbox create path, which is the highest-impact code path because it runs the AI agent. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
3 tasks
cv
pushed a commit
that referenced
this pull request
Apr 16, 2026
#1893) ## Summary - Replace the 12-entry env var blocklist in the sandbox creation path with the shared `subprocess-env.ts` allowlist. The blocklist passed the host's entire `process.env` into the sandbox minus only 12 specific names. The allowlist inverts the default: only known-safe variables are forwarded. - Additionally strip `KUBECONFIG` and `SSH_AUTH_SOCK` from the sandbox environment — the generic allowlist includes these for host-side processes but the sandbox should never have access to the host's Kubernetes cluster or SSH agent. ## Why The sandbox runs an AI agent with code execution. The blocklist only blocked 12 credential names and leaked **everything else** into the sandbox, including: - `GITHUB_TOKEN` (confirmed absent from blocklist) - `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SESSION_TOKEN` - `SSH_AUTH_SOCK` (host SSH agent access) - `KUBECONFIG` (host Kubernetes cluster access) - `NPM_TOKEN`, `DOCKER_PASSWORD` - `DATABASE_URL`, `REDIS_URL`, `MONGO_URI` - Any CI/CD secrets (`GITLAB_TOKEN`, `CIRCLECI_TOKEN`, etc.) A prompt-injected or compromised agent could read these via `process.env` and exfiltrate them through any allowed egress channel. The correct fix — `subprocess-env.ts` — already exists in the codebase (PR #1874) and is applied to `blueprint/runner.ts` and `services.ts`. The sandbox creation path was explicitly left on the old blocklist with a TODO comment (lines 2702-2705). This PR completes that migration for the highest-impact code path. ## What changed - `src/lib/onboard.ts` — import `buildSubprocessEnv` from `subprocess-env.ts`, replace the blocklist + `process.env` filter with `buildSubprocessEnv()`, delete `KUBECONFIG` and `SSH_AUTH_SOCK` from the result. ## Test plan - [x] `npm run build:cli` — compiles cleanly. - [ ] Manual: `nemoclaw onboard` with Ollama — verify sandbox can still reach inference (the allowlist includes `HOME`, `PATH`, `TERM`, proxy vars, and TLS vars which are sufficient). - [ ] Manual: set `GITHUB_TOKEN=test123` in host env, onboard, exec into sandbox, verify `echo $GITHUB_TOKEN` is empty. Signed-off-by: ColinM-sys <cmcdonough@50words.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved sandbox environment isolation so host-level access credentials are no longer forwarded into sandboxes; KUBECONFIG and SSH_AUTH_SOCK are explicitly excluded. * Replaced legacy blocklist handling with a consistent allowlist-based environment snapshot for more predictable and secure sandbox startups. * **Tests** * Updated regression tests to verify the allowlist-based environment behavior and explicit exclusion of host credential variables. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: ColinM-sys <cmcdonough@50words.com> Signed-off-by: Test User <test@example.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
13 tasks
cv
pushed a commit
that referenced
this pull request
Apr 29, 2026
#2616) (#2662) ## Summary On hosts with a system HTTP proxy (Privoxy, corporate CONNECT proxy, Cursor's bundled proxy), NemoClaw spawned the Ollama daemon and its `:11435` auth proxy with `HTTP_PROXY` inherited from the shell but no `NO_PROXY` set. Loopback requests — the auth-proxy health probe, `ollama pull`, and macOS curl's token validation — were tunnelled through the proxy and failed with: ``` HTTP 500 — Internal Privoxy Error Privoxy could not connect to the host: 127.0.0.1:11435 ``` The root cause is two call sites that used `{ ...process.env }` directly instead of the shared `buildSubprocessEnv()` allowlist, combined with `buildSubprocessEnv()` not guarding against a proxy-without-NO_PROXY environment. ## Related Issue Fixes #2616 ## How the fix works | Call site | Before | After | |---|---|---| | `spawnOllamaAuthProxy()` | `{ ...process.env, token, ports }` — full env including secrets | `buildSubprocessEnv({ token, ports })` — allowlisted env + NO_PROXY injected | | `pullOllamaModel()` | `{ ...process.env }` — full env | `buildSubprocessEnv()` — allowlisted env + NO_PROXY injected | | `buildSubprocessEnv()` | forwards `HTTP_PROXY` with no `NO_PROXY` guard | calls `withLocalNoProxy()` which appends `localhost,127.0.0.1` to both `NO_PROXY` and `no_proxy` when any proxy var is present | `withLocalNoProxy()` is additive: it appends only the missing entries so an existing `NO_PROXY=corp.internal` becomes `corp.internal,localhost,127.0.0.1`, preserving outbound proxy functionality for external registries like `registry.ollama.ai`. ## Changes - **`src/lib/subprocess-env.ts`**: add exported `withLocalNoProxy(env)` helper; call it at the end of `buildSubprocessEnv()`. When `HTTP_PROXY`, `HTTPS_PROXY`, `http_proxy`, or `https_proxy` is present, appends `localhost` and `127.0.0.1` to both `NO_PROXY` and `no_proxy` if either entry is missing. - **`nemoclaw/src/lib/subprocess-env.ts`**: mirror of above (kept in sync per existing policy). - **`src/lib/onboard-ollama-proxy.ts`**: add `require("./subprocess-env")` import; switch `spawnOllamaAuthProxy()` from `{ ...process.env, ... }` to `buildSubprocessEnv({ ... })` (also fixes the #1874 secret-leakage risk for this spawn); switch `pullOllamaModel()` from `{ ...process.env }` to `buildSubprocessEnv()`. - **`src/lib/subprocess-env.test.ts`** (new): 11 unit tests covering `withLocalNoProxy` (no proxy → no-op, HTTP_PROXY/HTTPS_PROXY/http_proxy each trigger injection, partial NO_PROXY is extended, full NO_PROXY is unchanged) and `buildSubprocessEnv` integration (injection on proxy set, augmentation of existing NO_PROXY, no injection when no proxy, extra vars preserved). ## Type of Change - [x] 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) ## Verification - [ ] `npx prek run --all-files` passes - [x] `npm test` passes (subprocess-env: 11/11 new tests pass; full suite clean) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] 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) ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed HTTP proxy configuration to ensure localhost and loopback traffic properly bypass proxy routing when proxy environment variables are configured. * **Tests** * Added comprehensive test coverage for proxy bypass functionality in subprocess environment setup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dongni Yang <dongniy@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
NVIDIA#2616) (NVIDIA#2662) ## Summary On hosts with a system HTTP proxy (Privoxy, corporate CONNECT proxy, Cursor's bundled proxy), NemoClaw spawned the Ollama daemon and its `:11435` auth proxy with `HTTP_PROXY` inherited from the shell but no `NO_PROXY` set. Loopback requests — the auth-proxy health probe, `ollama pull`, and macOS curl's token validation — were tunnelled through the proxy and failed with: ``` HTTP 500 — Internal Privoxy Error Privoxy could not connect to the host: 127.0.0.1:11435 ``` The root cause is two call sites that used `{ ...process.env }` directly instead of the shared `buildSubprocessEnv()` allowlist, combined with `buildSubprocessEnv()` not guarding against a proxy-without-NO_PROXY environment. ## Related Issue Fixes NVIDIA#2616 ## How the fix works | Call site | Before | After | |---|---|---| | `spawnOllamaAuthProxy()` | `{ ...process.env, token, ports }` — full env including secrets | `buildSubprocessEnv({ token, ports })` — allowlisted env + NO_PROXY injected | | `pullOllamaModel()` | `{ ...process.env }` — full env | `buildSubprocessEnv()` — allowlisted env + NO_PROXY injected | | `buildSubprocessEnv()` | forwards `HTTP_PROXY` with no `NO_PROXY` guard | calls `withLocalNoProxy()` which appends `localhost,127.0.0.1` to both `NO_PROXY` and `no_proxy` when any proxy var is present | `withLocalNoProxy()` is additive: it appends only the missing entries so an existing `NO_PROXY=corp.internal` becomes `corp.internal,localhost,127.0.0.1`, preserving outbound proxy functionality for external registries like `registry.ollama.ai`. ## Changes - **`src/lib/subprocess-env.ts`**: add exported `withLocalNoProxy(env)` helper; call it at the end of `buildSubprocessEnv()`. When `HTTP_PROXY`, `HTTPS_PROXY`, `http_proxy`, or `https_proxy` is present, appends `localhost` and `127.0.0.1` to both `NO_PROXY` and `no_proxy` if either entry is missing. - **`nemoclaw/src/lib/subprocess-env.ts`**: mirror of above (kept in sync per existing policy). - **`src/lib/onboard-ollama-proxy.ts`**: add `require("./subprocess-env")` import; switch `spawnOllamaAuthProxy()` from `{ ...process.env, ... }` to `buildSubprocessEnv({ ... })` (also fixes the NVIDIA#1874 secret-leakage risk for this spawn); switch `pullOllamaModel()` from `{ ...process.env }` to `buildSubprocessEnv()`. - **`src/lib/subprocess-env.test.ts`** (new): 11 unit tests covering `withLocalNoProxy` (no proxy → no-op, HTTP_PROXY/HTTPS_PROXY/http_proxy each trigger injection, partial NO_PROXY is extended, full NO_PROXY is unchanged) and `buildSubprocessEnv` integration (injection on proxy set, augmentation of existing NO_PROXY, no injection when no proxy, extra vars preserved). ## Type of Change - [x] 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) ## Verification - [ ] `npx prek run --all-files` passes - [x] `npm test` passes (subprocess-env: 11/11 new tests pass; full suite clean) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] 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) ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed HTTP proxy configuration to ensure localhost and loopback traffic properly bypass proxy routing when proxy environment variables are configured. * **Tests** * Added comprehensive test coverage for proxy bypass functionality in subprocess environment setup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dongni Yang <dongniy@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
tantodefi
added a commit
to tantodefi/NemoClaw
that referenced
this pull request
May 14, 2026
Merge upstream/main into tantodefi/NemoClaw fork. Key upstream additions: - TypeScript migration of bin/lib/ -> src/lib/ (all CLI code now TS) - buildSubprocessEnv() security refactor (strips secrets from subprocess env) - Hermes agent (agents/hermes/), OpenClaw agent manifest - Policy tiers (nemoclaw-blueprint/policies/tiers.yaml) - Shields/audit, secret scanner, sandbox-session-state - New e2e test suites (ollama, hermes, network policy, shields) - Windows/WSL e2e support Downstream changes preserved: - GITHUB_TOKEN, PROTON_USERNAME, PROTON_PASSWORD explicit sandbox injection (via buildSubprocessEnv() extra param pattern from NVIDIA#1874) - gbrain.yaml policy preset and GitHub workflow stubs (untracked, popping stash next) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yimoj
added a commit
that referenced
this pull request
Jul 3, 2026
The prebuild ran the local `docker build` under the shared subprocess allowlist minus KUBECONFIG/SSH_AUTH_SOCK. That allowlist already default-denies host secrets (NVIDIA_INFERENCE_API_KEY, GITHUB_TOKEN, AWS_* per #1874), but it still forwarded OPENSHELL_*/GRPC_* control-plane env and RUST_* debug knobs that a `docker build` never consumes. Add dockerBuildSubprocessEnv() to forward only the Docker-build boundary — system, Docker daemon (DOCKER_HOST + the XDG_* the docker CLI reads for its config/credential store), proxy, locale, temp, and TLS CA — and drop the broader host/control-plane vars (resolves PR Review Advisor least-privilege finding on sandbox-prebuild.ts). Subtractive of provably-unused vars, so no behaviour change to the build; covered by a new allowlist unit test. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merged
21 tasks
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
3 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
subprocess-env.ts— a shared module (mirrored in both CLI and plugin projects) that exportsbuildSubprocessEnv(). Defines an env var allowlist structured by category (system, temp, locale, proxy, TLS, toolchain) with prefix groups (LC_, XDG_, OPENSHELL_, GRPC_).nemoclaw/src/blueprint/runner.ts— provider create (the original bug target)src/lib/services.ts— cloudflared spawn (same vulnerable{ ...process.env }pattern)src/lib/onboard.ts— intentionally left on its existing blocklist to avoid regression risk on the highest-traffic code path. A TODO marks the future migration.credential-exposure.test.tsnow cover all three call sites, preventing...process.envfrom creeping back into runner.ts or services.tsrunner.test.tsasserts secrets are stripped and proxy/openshell vars pass throughTest plan
make check— all hooks passnpm test— all 1528 tests passrunner.test.ts"does not leak parent secrets into subprocess env"credential-exposure.test.tsguards for runner.ts and services.tsnemoclaw applyconfigures inference provider correctly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation