fix(security): stop passing NVIDIA_API_KEY into sandbox and command lines#675
Open
fix(security): stop passing NVIDIA_API_KEY into sandbox and command lines#675
Conversation
…ines The OpenShell gateway proxies inference requests and injects stored credentials server-side (proxy.rs strips client auth headers, backend.rs re-authenticates upstream). The raw key was never needed inside the sandbox but was passed via env args, setup.sh, walkthrough commands, and the setupSpark sudo call — exposing it in ps aux, /proc/pid/cmdline, docker inspect, and k3s audit logs. Changes: - Remove NVIDIA_API_KEY from openshell sandbox create env args - Use env-name-only credential form in setup.sh - Remove key from walkthrough.sh tmux/connect commands - Remove unnecessary key + ensureApiKey() from setupSpark - Clear key from process.env after setupInference handoff - Add 6 regression tests for credential exposure Does NOT fix /proc/pid/environ (kernel snapshot is immutable after exec — requires file-based credential loading in OpenShell). Messaging tokens left in sandbox env pending #617 merge. Closes #429.
📝 WalkthroughWalkthroughThese changes relocate NVIDIA API key handling from client-side environment injection to server-side gateway credential injection. The API key is removed from sandbox and child process environments across multiple scripts and binaries, while an explicit cleanup step deletes the key from the parent process. New tests validate that the key is not exposed in command invocations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 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 `@scripts/walkthrough.sh`:
- Around line 46-49: Remove the hard failure that requires NVIDIA_API_KEY by
deleting or changing the conditional block that checks [ -n
"${NVIDIA_API_KEY:-}" ] and exits (the NVIDIA_API_KEY guard); instead, either
remove the check entirely or replace it with a non-fatal warning (echo a message
if empty but do not call exit 1) so the walkthrough can run without that secret
in the host environment; update any references to NVIDIA_API_KEY in the script
to handle an empty value safely where actually needed.
In `@test/runner.test.js`:
- Around line 263-274: The current test "walkthrough.sh does not embed
NVIDIA_API_KEY in tmux or sandbox commands" only scans tmux/openshell lines
(cmdLines) so other exposures can slip through; update the test to scan the full
script content (src) — or at least all non-comment/non-echo executable lines —
for the string "NVIDIA_API_KEY" and assert no matches, replacing the filtered
cmdLines logic with a broader filter over src.split("\n") (reusing variables
like src, fs, path) so any direct env usage anywhere in walkthrough.sh fails the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10e304b6-c6bb-4666-9a41-7b37e1097a57
📒 Files selected for processing (5)
bin/lib/onboard.jsbin/nemoclaw.jsscripts/setup.shscripts/walkthrough.shtest/runner.test.js
Comment on lines
+46
to
+49
| [ -n "${NVIDIA_API_KEY:-}" ] || { | ||
| echo "NVIDIA_API_KEY required" | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
Remove hard NVIDIA_API_KEY runtime requirement from walkthrough.
The walkthrough no longer injects this key into sandbox commands, so failing fast here unnecessarily keeps the secret in host process environments.
🔧 Proposed fix
-[ -n "${NVIDIA_API_KEY:-}" ] || {
- echo "NVIDIA_API_KEY required"
- exit 1
-}
+# NVIDIA_API_KEY is not required at walkthrough runtime.
+# Credentials are resolved server-side by the OpenShell gateway provider config.📝 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
| [ -n "${NVIDIA_API_KEY:-}" ] || { | |
| echo "NVIDIA_API_KEY required" | |
| exit 1 | |
| } | |
| # NVIDIA_API_KEY is not required at walkthrough runtime. | |
| # Credentials are resolved server-side by the OpenShell gateway provider config. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/walkthrough.sh` around lines 46 - 49, Remove the hard failure that
requires NVIDIA_API_KEY by deleting or changing the conditional block that
checks [ -n "${NVIDIA_API_KEY:-}" ] and exits (the NVIDIA_API_KEY guard);
instead, either remove the check entirely or replace it with a non-fatal warning
(echo a message if empty but do not call exit 1) so the walkthrough can run
without that secret in the host environment; update any references to
NVIDIA_API_KEY in the script to handle an empty value safely where actually
needed.
Comment on lines
+263
to
+274
| it("walkthrough.sh does not embed NVIDIA_API_KEY in tmux or sandbox commands", () => { | ||
| const fs = require("fs"); | ||
| const src = fs.readFileSync(path.join(__dirname, "..", "scripts", "walkthrough.sh"), "utf-8"); | ||
| // Check only executable lines (tmux spawn, openshell connect) — not comments/docs | ||
| const cmdLines = src.split("\n").filter( | ||
| (l) => !l.trim().startsWith("#") && !l.trim().startsWith("echo") && | ||
| (l.includes("tmux") || l.includes("openshell sandbox connect")) | ||
| ); | ||
| for (const line of cmdLines) { | ||
| expect(line.includes("NVIDIA_API_KEY")).toBe(false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Credential regression guard is too narrow for walkthrough script.
This check only inspects tmux/connect command lines, so a direct env dependency/exposure path can slip through undetected.
🧪 Proposed test hardening
it("walkthrough.sh does not embed NVIDIA_API_KEY in tmux or sandbox commands", () => {
const fs = require("fs");
const src = fs.readFileSync(path.join(__dirname, "..", "scripts", "walkthrough.sh"), "utf-8");
+ // Guard against runtime env preconditions that force secret presence.
+ expect(src.includes('[ -n "${NVIDIA_API_KEY:-}" ]')).toBe(false);
// Check only executable lines (tmux spawn, openshell connect) — not comments/docs
const cmdLines = src.split("\n").filter(
(l) => !l.trim().startsWith("#") && !l.trim().startsWith("echo") &&
(l.includes("tmux") || l.includes("openshell sandbox connect"))
);
for (const line of cmdLines) {
expect(line.includes("NVIDIA_API_KEY")).toBe(false);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runner.test.js` around lines 263 - 274, The current test "walkthrough.sh
does not embed NVIDIA_API_KEY in tmux or sandbox commands" only scans
tmux/openshell lines (cmdLines) so other exposures can slip through; update the
test to scan the full script content (src) — or at least all
non-comment/non-echo executable lines — for the string "NVIDIA_API_KEY" and
assert no matches, replacing the filtered cmdLines logic with a broader filter
over src.split("\n") (reusing variables like src, fs, path) so any direct env
usage anywhere in walkthrough.sh fails the test.
cv
approved these changes
Mar 23, 2026
ericksoa
added a commit
that referenced
this pull request
Mar 23, 2026
Main switched to --credential "OPENAI_API_KEY" (env-lookup form from #675). We set the proxy token in the environment so openshell reads it via the env-name-only pattern while still using our random per-instance token instead of the static "ollama" dummy value.
cv
approved these changes
Mar 23, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
LGTM — important security fix. The gateway already injects credentials server-side, so the sandbox never needed the raw key. Good documentation of the /proc/pid/environ limitation and solid regression tests.
16 tasks
5 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
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
Closes #429. Lands independently of #617.
The OpenShell gateway proxies inference and injects stored credentials server-side — the raw NVIDIA_API_KEY was never needed inside the sandbox but was passed via env args,
setup.sh, walkthrough commands, and thesetupSparksudo call, exposing it inps aux,/proc/pid/cmdline,docker inspect, and k3s audit logs.NVIDIA_API_KEYfromopenshell sandbox createenv args (onboard.js,setup.sh)--credential NVIDIA_API_KEYform insetup.sh(same pattern as fix(security): stop leaking API keys in process args visible via ps aux #330)walkthrough.shtmux/connect commandsensureApiKey()fromsetupSpark(script never reads it)process.envaftersetupInferencehandoffWhat this does NOT fix
/proc/pid/environof the nemoclaw process itself — kernel snapshot is immutable after exec.delete process.envonly prevents child process inheritance. True fix requires file-based credential loading (OpenShell provider model change).--credential NVIDIA_API_KEY=<value>inonboard.jssetupInference— that's fix(security): stop leaking API keys in process args visible via ps aux #330's scope.Why it works
Verified in OpenShell source:
proxy.rs:1068— gateway strips allAuthorization/X-Api-Keyheaders from sandbox requestsbackend.rs:101— gateway re-authenticates upstream using stored provider keygrpc.rs:3446— inference provider credentials are NOT injected into sandbox envnemoclaw-start.sh:19—write_auth_profile()gracefully no-ops if key is absentTest plan
nemoclaw onboardcompletes, inference works through gateway without key in sandboxSummary by CodeRabbit
Tests
Chores