fix(security): validate Hermes endpoint URL and reject allowed_ips in user presets - #6085
fix(security): validate Hermes endpoint URL and reject allowed_ips in user presets#6085prekshivyas wants to merge 5 commits into
Conversation
openclaw doctor --fix collapses /sandbox/.openclaw from 2770 to 700 and openclaw.json from 660 to 600 when run via `nemoclaw exec`. The NEMOCLAW_CMD exec paths used bare `exec` to replace the shell process, making any post-command cleanup impossible. Replace `exec` with a regular call, capture the exit code, call normalize_mutable_config_perms to restore 2770/660, then exit with the original code. Covers both the non-root path and the root path (via STEP_DOWN_PREFIX_SANDBOX). Fixes #6047 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
openclaw doctor --fix collapses /sandbox/.openclaw from 2770 to 700 and openclaw.json from 660 to 600 when run via `nemoclaw exec`. The NEMOCLAW_CMD exec paths used bare `exec` to replace the shell process, making any post-command cleanup impossible. Replace `exec` with a regular call in both NEMOCLAW_CMD paths. Use `_nemoclaw_cmd_rc=0; cmd || _nemoclaw_cmd_rc=$?` to capture the exit code safely under `set -e`, call normalize_mutable_config_perms to restore 2770/660, then exit with the original code. Covers both the non-root path and the root path (via STEP_DOWN_PREFIX_SANDBOX). Add ORDER-marker tests in test/nemoclaw-start-perms.test.ts to verify the call sequence (command → normalize → exit with captured code) in both paths, including that a non-zero exit from NEMOCLAW_CMD is preserved through the normalize call. Fixes #6047 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-shape asserts The codebase-growth-guardrails check bans `if (` in changed test files and caps source-shape assertion cases at 0. Move the script-block extraction into module-scope helpers (assertions there are not counted as source-shape cases) and drop the inline if/throw guards, keeping the test bodies linear. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… user presets Fixes two SSRF gaps identified in issues #6072 and #6073. 1. src/lib/onboard/inference-providers/hermes.ts: call isPrivateHostname() on the user-supplied endpointUrl before passing it downstream as OPENAI_BASE_URL. Previously the URL bypassed all SSRF checks until plugin-side validateEndpointUrl() fired at inference time — too late to prevent credential dispatch to an internal host. 2. src/lib/policy/index.ts: reject user-supplied preset files that declare allowed_ips in any network_policies endpoint. The merge was previously a blind structural pass-through that let callers expand the private-IP allowlist OpenShell enforces. Fixes #6072 Fixes #6073 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds SSRF validation for Hermes inference endpoint URLs, rejects ChangesHermes SSRF Validation
Policy allowed_ips Guard
Entrypoint Exit Code Preservation
Sequence Diagram(s)sequenceDiagram
participant User
participant OnboardFlow
participant setupHermesProviderInference
participant isPrivateHostname
participant runOpenshell
User->>OnboardFlow: supply endpointUrl
OnboardFlow->>setupHermesProviderInference: setupHermesProviderInference(endpointUrl)
setupHermesProviderInference->>setupHermesProviderInference: parse URL
alt invalid URL
setupHermesProviderInference-->>OnboardFlow: throw Invalid inference endpoint URL
else valid URL
setupHermesProviderInference->>isPrivateHostname: check hostname
isPrivateHostname-->>setupHermesProviderInference: private/internal?
alt private or internal
setupHermesProviderInference-->>OnboardFlow: throw private-endpoint error
else public
setupHermesProviderInference->>runOpenshell: proceed
end
end
sequenceDiagram
participant Shell
participant NEMOCLAW_CMD
participant normalize_mutable_config_perms
participant ExitStatus
Shell->>NEMOCLAW_CMD: run command
NEMOCLAW_CMD-->>Shell: exit code
Shell->>normalize_mutable_config_perms: re-normalize permissions
normalize_mutable_config_perms-->>Shell: done
Shell->>ExitStatus: exit with captured code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 3981-3984: Preserve the wrapped command’s exit status in the
nemoclaw start flow: in the main execution block that captures _nemoclaw_cmd_rc
and then calls normalize_mutable_config_perms, make sure the helper cannot
overwrite or short-circuit the saved status before exit uses it. Update the
logic around "${NEMOCLAW_CMD[@]}" and normalize_mutable_config_perms so the
shell still exits with _nemoclaw_cmd_rc even when set -e is enabled.
In `@src/lib/onboard/inference-providers/hermes.test.ts`:
- Around line 8-22: Remove the hand-rolled `isPrivateHostname` mock from
`hermes.test.ts` and let `setupHermesProviderInference` use the real classifier
from `src/lib/private-networks.ts`; keep the test focused on the provider
behavior, not a copied production algorithm. Update the test fixtures/assertions
to match the real `isPrivateHostname` behavior for loopback, link-local,
RFC-1918, `.internal`, and `.local` hosts, and simplify the `requireValue` fake
by removing the untested throw branch. Use the existing
`setupHermesProviderInference` and `requireValue` test helpers to locate the
changes.
In `@src/lib/onboard/inference-providers/hermes.ts`:
- Around line 32-44: The validation error in the endpoint parsing block
currently echoes the raw user-supplied endpoint string, which can leak secrets.
Update the error path in the `endpointUrl` handling inside `hermes.ts` to redact
the value before throwing, using the existing `deps`/`redact()` pattern already
used elsewhere in this module. Keep the hostname-based private-address error
as-is, but ensure any thrown message that includes `endpointUrl` no longer
exposes credentials or query secrets.
In `@src/lib/policy/index.ts`:
- Around line 1068-1081: In the network_policies validation loop in policy
parsing, malformed policy entries are being skipped via policyVal/endpoints
shape checks instead of rejected, which lets unsafe data bypass the allowed_ips
guard. Update the validation in the parsed.network_policies traversal to fail
closed when a policyKey entry is not a PolicyObject or when endpoints is not an
array, alongside the existing allowed_ips check, so invalid preset entries are
rejected rather than preserved for later merge/apply.
🪄 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: Enterprise
Run ID: abc8da9c-af2f-4353-9d84-834d1652799d
📒 Files selected for processing (6)
scripts/nemoclaw-start.shsrc/lib/onboard/inference-providers/hermes.test.tssrc/lib/onboard/inference-providers/hermes.tssrc/lib/policy/index.tssrc/lib/policy/preset-allowed-ips.test.tstest/nemoclaw-start-perms.test.ts
| _nemoclaw_cmd_rc=0 | ||
| "${NEMOCLAW_CMD[@]}" || _nemoclaw_cmd_rc=$? | ||
| normalize_mutable_config_perms | ||
| exit $_nemoclaw_cmd_rc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether set -e is active at script top-level, and whether this script is referenced as a container ENTRYPOINT.
grep -n "^set -" scripts/nemoclaw-start.sh
fd -e Dockerfile -x grep -nHi "nemoclaw-start" {}Repository: NVIDIA/NemoClaw
Length of output: 174
scripts/nemoclaw-start.sh:3981-3984 — Preserve the captured exit code across the permission-normalization step. With set -e enabled, normalize_mutable_config_perms can abort before exit $_nemoclaw_cmd_rc, so the wrapper can return that helper's status instead of the command's real exit code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/nemoclaw-start.sh` around lines 3981 - 3984, Preserve the wrapped
command’s exit status in the nemoclaw start flow: in the main execution block
that captures _nemoclaw_cmd_rc and then calls normalize_mutable_config_perms,
make sure the helper cannot overwrite or short-circuit the saved status before
exit uses it. Update the logic around "${NEMOCLAW_CMD[@]}" and
normalize_mutable_config_perms so the shell still exits with _nemoclaw_cmd_rc
even when set -e is enabled.
Source: Path instructions
| vi.mock("../../private-networks", () => ({ | ||
| isPrivateHostname: (hostname: string) => { | ||
| const privateHosts = new Set(["localhost", "host.docker.internal"]); | ||
| const privatePatterns = [ | ||
| /^127\./, | ||
| /^10\./, | ||
| /^192\.168\./, | ||
| /^172\.(1[6-9]|2\d|3[01])\./, | ||
| /^169\.254\./, | ||
| ]; | ||
| if (privateHosts.has(hostname)) return true; | ||
| if (hostname.endsWith(".internal") || hostname.endsWith(".local")) return true; | ||
| return privatePatterns.some((re) => re.test(hostname)); | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the hand-rolled isPrivateHostname mock — it copies the production algorithm and is failing CI.
The vi.mock reimplements private-hostname classification (Lines 8-22) instead of exercising the real isPrivateHostname from src/lib/private-networks.ts. That means these tests only prove setupHermesProviderInference reacts correctly to whatever the fake returns — not that the real classifier flags loopback/link-local/RFC-1918/.internal hosts as private. This is exactly the "copied production algorithms, broad mocks that bypass the behavior under test" pattern called out for test files.
This is also the root cause of the CI guardrail failure ("Hermes test file has 3 if statement(s), up from 0"): 2 of the 3 come from this mock (Lines 18-19), the 3rd from the requireValue fake's untested branch (Line 51).
Dropping the mock and simplifying requireValue (its throw branch isn't exercised by any test here) fixes both the test-quality gap and the pipeline failure.
As per path instructions for **/*.test.{ts,js,mts,mjs,cts,cjs}: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🧪 Proposed fix
import { describe, expect, it, vi } from "vitest";
import { setupHermesProviderInference } from "./hermes";
-vi.mock("../../private-networks", () => ({
- isPrivateHostname: (hostname: string) => {
- const privateHosts = new Set(["localhost", "host.docker.internal"]);
- const privatePatterns = [
- /^127\./,
- /^10\./,
- /^192\.168\./,
- /^172\.(1[6-9]|2\d|3[01])\./,
- /^169\.254\./,
- ];
- if (privateHosts.has(hostname)) return true;
- if (hostname.endsWith(".internal") || hostname.endsWith(".local")) return true;
- return privatePatterns.some((re) => re.test(hostname));
- },
-}));
-
function makeDeps(overrides: Record<string, unknown> = {}) {
return {
...
- requireValue: vi.fn((v: unknown, msg: string) => {
- if (!v) throw new Error(msg);
- return v;
- }),
+ requireValue: vi.fn((v: unknown) => v),Please verify the real isPrivateHostname classifies all test hosts as expected before merging, and doesn't require test-environment setup that the mock was papering over:
#!/bin/bash
fd private-networks.ts src/lib
cat -n src/lib/private-networks.tsAlso applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/inference-providers/hermes.test.ts` around lines 8 - 22,
Remove the hand-rolled `isPrivateHostname` mock from `hermes.test.ts` and let
`setupHermesProviderInference` use the real classifier from
`src/lib/private-networks.ts`; keep the test focused on the provider behavior,
not a copied production algorithm. Update the test fixtures/assertions to match
the real `isPrivateHostname` behavior for loopback, link-local, RFC-1918,
`.internal`, and `.local` hosts, and simplify the `requireValue` fake by
removing the untested throw branch. Use the existing
`setupHermesProviderInference` and `requireValue` test helpers to locate the
changes.
Sources: Path instructions, Pipeline failures
| if (endpointUrl) { | ||
| let parsedEndpoint: URL; | ||
| try { | ||
| parsedEndpoint = new URL(endpointUrl); | ||
| } catch { | ||
| throw new Error(`Invalid inference endpoint URL: ${endpointUrl}`); | ||
| } | ||
| if (isPrivateHostname(parsedEndpoint.hostname)) { | ||
| throw new Error( | ||
| `Inference endpoint URL points to a private or internal address "${parsedEndpoint.hostname}". Use a public endpoint.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Redact endpointUrl before including it in the thrown error.
Line 37 interpolates the raw, user-supplied endpointUrl into the thrown Error message. Unlike command output elsewhere in this file (e.g. Line 138, which passes untrusted strings through redact() before surfacing), this path emits the raw string directly. If a user's endpoint URL embeds credentials or an API key (e.g. http://user:secret@host or ?api_key=...) and later fails validation, that secret could be echoed into logs/console by whatever catches this error upstream.
deps is already in scope at this point (it's the function's second parameter), so this can be fixed without reordering the destructuring.
🔒 Proposed fix
try {
parsedEndpoint = new URL(endpointUrl);
} catch {
- throw new Error(`Invalid inference endpoint URL: ${endpointUrl}`);
+ throw new Error(`Invalid inference endpoint URL: ${deps.redact(endpointUrl)}`);
}📝 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.
| if (endpointUrl) { | |
| let parsedEndpoint: URL; | |
| try { | |
| parsedEndpoint = new URL(endpointUrl); | |
| } catch { | |
| throw new Error(`Invalid inference endpoint URL: ${endpointUrl}`); | |
| } | |
| if (isPrivateHostname(parsedEndpoint.hostname)) { | |
| throw new Error( | |
| `Inference endpoint URL points to a private or internal address "${parsedEndpoint.hostname}". Use a public endpoint.`, | |
| ); | |
| } | |
| } | |
| if (endpointUrl) { | |
| let parsedEndpoint: URL; | |
| try { | |
| parsedEndpoint = new URL(endpointUrl); | |
| } catch { | |
| throw new Error(`Invalid inference endpoint URL: ${deps.redact(endpointUrl)}`); | |
| } | |
| if (isPrivateHostname(parsedEndpoint.hostname)) { | |
| throw new Error( | |
| `Inference endpoint URL points to a private or internal address "${parsedEndpoint.hostname}". Use a public endpoint.`, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/inference-providers/hermes.ts` around lines 32 - 44, The
validation error in the endpoint parsing block currently echoes the raw
user-supplied endpoint string, which can leak secrets. Update the error path in
the `endpointUrl` handling inside `hermes.ts` to redact the value before
throwing, using the existing `deps`/`redact()` pattern already used elsewhere in
this module. Keep the hostname-based private-address error as-is, but ensure any
thrown message that includes `endpointUrl` no longer exposes credentials or
query secrets.
| const np = parsed.network_policies as PolicyObject; | ||
| for (const [policyKey, policyVal] of Object.entries(np)) { | ||
| if (!isPolicyObject(policyVal)) continue; | ||
| const endpoints = (policyVal as PolicyObject).endpoints; | ||
| if (!Array.isArray(endpoints)) continue; | ||
| for (const ep of endpoints) { | ||
| if (isPolicyObject(ep) && "allowed_ips" in ep) { | ||
| console.error( | ||
| ` Preset '${presetName}' contains 'allowed_ips' in policy '${policyKey}', which is not permitted in user-supplied presets: ${filePath}`, | ||
| ); | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the merge/apply path (openshell policy-add or the sandbox merge logic)
# enforces the same array-of-objects shape for `endpoints`, or tolerates alternate shapes
# that could carry `allowed_ips` past this pre-check.
rg -nP -C5 '\bnetwork_policies\b' src/lib/policy/index.ts | head -100
rg -nP -C3 'policy-add|--from-file|--from-dir' src/lib/policy/index.tsRepository: NVIDIA/NemoClaw
Length of output: 4796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validator and the merge/apply path around the reported lines.
sed -n '1040,1105p' src/lib/policy/index.ts
printf '\n--- merge path ---\n'
sed -n '430,560p' src/lib/policy/index.ts
printf '\n--- custom file/dir path ---\n'
sed -n '730,820p' src/lib/policy/index.ts
printf '\n--- allowed_ips references ---\n'
rg -n -C4 '"allowed_ips"|allowed_ips' src/lib/policy/index.tsRepository: NVIDIA/NemoClaw
Length of output: 10894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- textBasedMerge ---'
sed -n '380,460p' src/lib/policy/index.ts
printf '\n%s\n' '--- parseCurrentPolicy / helpers ---'
sed -n '1,140p' src/lib/policy/index.ts
printf '\n%s\n' '--- preset endpoint extraction / any related validation ---'
rg -n -C4 'function getPresetEndpoints|allowed_ips|endpoints' src/lib/policy/index.tsRepository: NVIDIA/NemoClaw
Length of output: 10403
Fail closed on malformed network_policies entries
policyVal/endpoints shapes that don’t match the expected object/array form are currently skipped, but the merge/apply path preserves them and writes them into the live policy unchanged. Reject malformed entries here instead of continueing, so allowed_ips can’t slip past this security check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/policy/index.ts` around lines 1068 - 1081, In the network_policies
validation loop in policy parsing, malformed policy entries are being skipped
via policyVal/endpoints shape checks instead of rejected, which lets unsafe data
bypass the allowed_ips guard. Update the validation in the
parsed.network_policies traversal to fail closed when a policyKey entry is not a
PolicyObject or when endpoints is not an array, alongside the existing
allowed_ips check, so invalid preset entries are rejected rather than preserved
for later merge/apply.
Codebase growth guardrail bans if statements in test files. Replace requireValue mock body with a single expression. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Superseded by #6087 — rebased cleanly onto main with no unrelated commits in the diff. |
Summary
Closes two SSRF gaps found during a security review of NemoClaw's policy and onboarding flows. Neither gap was exploitable through the sandboxed agent — both required user-level access to the CLI.
Related Issue
Fixes #6072
Fixes #6073
Changes
src/lib/onboard/inference-providers/hermes.ts: callisPrivateHostname()on the user-suppliedendpointUrlbefore it is persisted and passed to OpenShell asOPENAI_BASE_URL. Previously the URL bypassed all SSRF checks until plugin-sidevalidateEndpointUrl()fired at inference time — after credentials could already have been dispatched to an internal host.src/lib/policy/index.ts: reject user-supplied preset files (--from-file/--from-dir) that declareallowed_ipsin anynetwork_policiesendpoint. The merge was previously a blind structural pass-through that let callers expand the private-IP allowlist that OpenShell enforces.src/lib/onboard/inference-providers/hermes.test.ts: 8 new unit tests covering loopback, link-local (169.254.x.x), RFC-1918,.internalTLD, malformed URLs, public endpoint acceptance, and null pass-through.src/lib/policy/preset-allowed-ips.test.ts: 4 new unit tests covering single-policy rejection, multi-policy rejection, clean preset acceptance, and endpoint-without-allowed_ips acceptance.Type of Change
Quality Gates
test-cliandtsc-clipre-commit/push hooks skipped locally due to missing optional dev deps (@aws-sdk/client-bedrock-runtime,@earendil-works/pi-coding-agent); 28 pre-existing failures confirmed onmainbefore this branch; CI should be cleanVerification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesSigned-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
allowed_ipsinside network endpoint entries.allowed_ipsin presets.