fix(security): stop leaking API keys in process args visible via ps aux

[ericksoa]

Closes #325.

Summary

• Change --credential NVIDIA_API_KEY=${value} to --credential NVIDIA_API_KEY (env-lookup form) so openshell reads the secret from the environment variable internally instead of receiving it as a CLI argument visible in ps aux
• Add 6 regression tests that statically verify no real secrets are passed as literal --credential values

Problem

Any user on the system could run ps aux | grep credential and see the full NVIDIA API key in the process argument list:

openshell provider create --credential NVIDIA_API_KEY=nvapi-abc123...

Fix

openshell's --credential flag supports two forms:

• --credential KEY=VALUE — literal value, visible in ps aux
• --credential KEY — env-var lookup, openshell reads the value from the environment

The fix sets the credential as an environment variable (not visible in ps aux) and passes only the variable name to --credential.

Non-secret dummy values (OPENAI_API_KEY=dummy, OPENAI_API_KEY=ollama) are left in KEY=VALUE form since they are not real secrets.

Files changed

File	Change
bin/lib/onboard.js	NVIDIA_API_KEY=${...} → NVIDIA_API_KEY
nemoclaw/src/commands/onboard.ts	Set process.env[credentialEnv] before exec, pass var name only
nemoclaw-blueprint/orchestrator/runner.py	Set os.environ[target_cred_env] before run_cmd, pass var name only
test/credential-exposure.test.js	6 regression tests verifying no secret leakage

Test plan

• 43/43 root unit tests pass
• 22/22 nemoclaw vitest tests pass
• 6/6 new credential exposure tests pass
• TypeScript compiles cleanly
• ps aux verified: 2,001 lines captured during onboard, zero nvapi- matches
• Provider creation and inference routing work end-to-end
• Dummy/ollama credentials unchanged (OPENAI_API_KEY=dummy, OPENAI_API_KEY=ollama)

Summary by CodeRabbit

• Bug Fixes

  • Improved provider setup to avoid embedding secret credential values in command-line arguments; credentials are now referenced by environment variable names to reduce accidental exposure.

• Tests

  • Added a security regression test suite that scans provider setup code to ensure credential values are not included in CLI arguments and enforces use of environment-variable references.

[coderabbitai]

📝 Walkthrough

Walkthrough

Credentials are moved out of command-line --credential values and passed as environment variable names; the process environment is populated with the secret values before invoking OpenShell provider create/update. A new test scans source files to ensure secrets are not embedded in CLI arguments.

Changes

Cohort / File(s)	Summary
Credential Handling Updates bin/lib/onboard.js, nemoclaw-blueprint/orchestrator/runner.py	Replaced inline KEY=VALUE credential usage in openshell provider create/update with passing the credential key name only (e.g., --credential OPENAI_API_KEY) and setting the corresponding environment variable in-process before invocation.
Security Validation test/credential-exposure.test.js	Added Jest tests that scan target source files for patterns that would embed credential values in --credential CLI arguments (checks for KEY=VALUE, concatenations, and quoted credential tokens containing =).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 I hid the keys where noses can't see,

Names in the shell, secrets with me.
No more peeking from ps in the night,
Env holds the treasure, snug and tight. 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The code changes implement the exact requirement from #325: passing credentials via environment variables using --credential KEY instead of --credential KEY=VALUE.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing credential exposure by refactoring how credentials are passed to openshell and adding regression tests.
Title check	✅ Passed	The title clearly summarizes the main security fix: preventing API key exposure in process arguments visible via ps aux. It directly reflects the primary objective of the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-325-credential-ps-aux

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/credential-exposure.test.js`:
- Around line 56-65: The current credential detection uses a loose regex and a
prefix-based allowlist check which can miss unquoted KEY=VALUE args and allow
bypasses; update the regex used to produce credMatch so it reliably captures
unquoted and quoted KEY=VALUE forms (handle optional quotes/backticks and
optional surrounding braces) and normalize the captured value into fullValue
without dropping characters that could alter the token, then replace the
allowlist check (ALLOWED_CREDENTIAL_VALUES.some(...) with startsWith) with an
exact normalized comparison (normalize both allowed entries and fullValue and
use ===) so allowlisted items match only exactly and not by prefix; refer to the
variables/expressions credMatch, key, fullValue and ALLOWED_CREDENTIAL_VALUES
when making the changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c3ea473-9e61-465e-bad9-c40b62056432

📥 Commits

Reviewing files that changed from the base of the PR and between 1e23347 and bf2529d.

📒 Files selected for processing (4)

• bin/lib/onboard.js
• nemoclaw-blueprint/orchestrator/runner.py
• nemoclaw/src/commands/onboard.ts
• test/credential-exposure.test.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Harden credential literal detection to avoid false negatives.

The scanner can miss/bypass unsafe literals because allowlist matching is prefix-based and the regex can skip unquoted KEY=VALUE args. Tighten both checks so leaks can’t pass silently.

🔧 Proposed fix

-        const credMatch = line.match(/--credential[",\s]+["'`]?([A-Z_]+)=(.+?)["'`]/);
+        const credMatch = line.match(/--credential[",\s]+["'`]?([A-Z_]+)=([^"'`\s,]+)/);
         if (!credMatch) continue;

         const key = credMatch[1];
-        const fullValue = `${key}=${credMatch[2].replace(/["'`}\s]/g, "")}`;
+        const fullValue = `${key}=${credMatch[2].trim()}`;

         // Check if this is in the allowlist
-        const isAllowed = ALLOWED_CREDENTIAL_VALUES.some((allowed) =>
-          fullValue.startsWith(allowed.replace(/["']/g, ""))
-        );
+        const isAllowed = ALLOWED_CREDENTIAL_VALUES.includes(fullValue);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/credential-exposure.test.js` around lines 56 - 65, The current
credential detection uses a loose regex and a prefix-based allowlist check which
can miss unquoted KEY=VALUE args and allow bypasses; update the regex used to
produce credMatch so it reliably captures unquoted and quoted KEY=VALUE forms
(handle optional quotes/backticks and optional surrounding braces) and normalize
the captured value into fullValue without dropping characters that could alter
the token, then replace the allowlist check (ALLOWED_CREDENTIAL_VALUES.some(...)
with startsWith) with an exact normalized comparison (normalize both allowed
entries and fullValue and use ===) so allowlisted items match only exactly and
not by prefix; refer to the variables/expressions credMatch, key, fullValue and
ALLOWED_CREDENTIAL_VALUES when making the changes.

[coderabbitai]

🧹 Nitpick comments (2)

test/credential-exposure.test.js (2)

29-31: Consider documenting regex coverage limitations.

The current regexes are tailored to the patterns in use:

• JS_EXPOSURE_RE catches quoted KEY= patterns
• JS_CREDENTIAL_CONCAT_RE catches process.env interpolation
• PY_EXPOSURE_RE catches f-string brace patterns

These work for the current code, but an unquoted pattern like --credential KEY=VALUE (no quotes) would slip through. Since the tested files currently use quoted forms, this is acceptable—but a brief comment noting the assumed patterns would help future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/credential-exposure.test.js` around lines 29 - 31, Add a short comment
above the regex definitions (JS_EXPOSURE_RE, JS_CREDENTIAL_CONCAT_RE,
PY_EXPOSURE_RE) documenting their coverage and limitations: note they assume
quoted JS credential patterns (KEY= inside quotes), process.env interpolation
for concatenation, and Python f-string brace usage, and explicitly call out that
unquoted forms like "--credential KEY=VALUE" will not be detected; this helps
future maintainers understand the assumptions without changing the tests.

---

38-43: Unused variable i in filter callback.

The line index parameter i is declared but never used.

🧹 Proposed fix

     const violations = lines.filter(
-      (line, i) =>
+      (line) =>
         (JS_EXPOSURE_RE.test(line) || JS_CREDENTIAL_CONCAT_RE.test(line)) &&
         // Allow comments that describe the old pattern
         !line.trimStart().startsWith("//"),
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/credential-exposure.test.js` around lines 38 - 43, The filter callback
for computing violations declares an unused second parameter `i`; remove it (or
replace it with `_`) from the parameter list of the arrow function used to build
`violations` so the linter won't flag an unused variable—update the filter
invocation that references JS_EXPOSURE_RE and JS_CREDENTIAL_CONCAT_RE
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/credential-exposure.test.js`:
- Around line 29-31: Add a short comment above the regex definitions
(JS_EXPOSURE_RE, JS_CREDENTIAL_CONCAT_RE, PY_EXPOSURE_RE) documenting their
coverage and limitations: note they assume quoted JS credential patterns (KEY=
inside quotes), process.env interpolation for concatenation, and Python f-string
brace usage, and explicitly call out that unquoted forms like "--credential
KEY=VALUE" will not be detected; this helps future maintainers understand the
assumptions without changing the tests.
- Around line 38-43: The filter callback for computing violations declares an
unused second parameter `i`; remove it (or replace it with `_`) from the
parameter list of the arrow function used to build `violations` so the linter
won't flag an unused variable—update the filter invocation that references
JS_EXPOSURE_RE and JS_CREDENTIAL_CONCAT_RE accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 53c58c50-e643-4f8c-a0f9-63dee202b491

📥 Commits

Reviewing files that changed from the base of the PR and between bf2529d and 9936c52.

📒 Files selected for processing (3)

• bin/lib/onboard.js
• nemoclaw-blueprint/orchestrator/runner.py
• test/credential-exposure.test.js