Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions scripts/nemoclaw-start.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3978,7 +3978,10 @@ if [ "$(id -u)" -ne 0 ]; then
if [ ${#NEMOCLAW_CMD[@]} -gt 0 ]; then
install_messaging_runtime_preloads
verify_messaging_runtime_secret_scans
exec "${NEMOCLAW_CMD[@]}"
_nemoclaw_cmd_rc=0
"${NEMOCLAW_CMD[@]}" || _nemoclaw_cmd_rc=$?
normalize_mutable_config_perms
exit $_nemoclaw_cmd_rc
Comment on lines +3981 to +3984

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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

fi

configure_messaging_channels
Expand Down Expand Up @@ -4144,7 +4147,10 @@ setup_auth_profile_as_sandbox

# If a command was passed (e.g., "openclaw agent ..."), run it as sandbox user
if [ ${#NEMOCLAW_CMD[@]} -gt 0 ]; then
exec "${STEP_DOWN_PREFIX_SANDBOX[@]}" "${NEMOCLAW_CMD[@]}"
_nemoclaw_cmd_rc=0
"${STEP_DOWN_PREFIX_SANDBOX[@]}" "${NEMOCLAW_CMD[@]}" || _nemoclaw_cmd_rc=$?
normalize_mutable_config_perms
exit $_nemoclaw_cmd_rc
fi

# Gateway log: owned by gateway user, world-readable for diagnostics.
Expand Down
193 changes: 193 additions & 0 deletions src/lib/onboard/inference-providers/hermes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

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));
},
}));
Comment on lines +8 to +22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.ts

Also 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


function makeDeps(overrides: Record<string, unknown> = {}) {
return {
runOpenshell: vi.fn(() => ({ status: 0, stdout: "", stderr: "" })),
upsertProvider: vi.fn(),
verifyInferenceRoute: vi.fn(),
verifyOnboardInferenceSmoke: vi.fn(),
isNonInteractive: vi.fn(() => false),
registry: { updateSandbox: vi.fn() },
hermesProviderAuth: {
isHermesProviderRegistered: vi.fn(() => true),
ensureHermesProviderApiKeyCredentials: vi.fn(() => ({})),
ensureHermesProviderOAuthCredentials: vi.fn(() => ({})),
},
getHermesToolGatewayBroker: vi.fn(() => ({
getHermesToolGatewayProviderName: vi.fn(() => "hermes-tool-gateway"),
})),
providerExistsInGateway: vi.fn(() => true),
normalizeHermesAuthMethod: vi.fn(() => "api-key"),
resolveHermesNousApiKey: vi.fn(() => null),
checkHermesProviderStoreReachable: vi.fn(() => ({ ok: true })),
hermesAuthMethodLabel: vi.fn((m: string) => m),
hermesConstants: {
HERMES_NOUS_API_KEY_CREDENTIAL_ENV: "NOUS_API_KEY",
HERMES_AUTH_METHOD_API_KEY: "api-key",
HERMES_AUTH_METHOD_OAUTH: "oauth",
},
requireValue: vi.fn((v: unknown, _msg: string) => v),
redact: vi.fn((s: string) => s),
compactText: vi.fn((s: string) => s),
...overrides,
};
}

describe("setupHermesProviderInference SSRF guard (#6072)", () => {
it("rejects loopback address", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "http://127.0.0.1:8080/v1",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/private or internal/);
});

it("rejects cloud metadata endpoint", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "http://169.254.169.254/latest/meta-data/",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/private or internal/);
});

it("rejects private RFC-1918 range", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "http://10.0.0.1/v1",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/private or internal/);
});

it("rejects localhost hostname", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "http://localhost:11434/v1",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/private or internal/);
});

it("rejects .internal TLD", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "http://my-service.internal/v1",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/private or internal/);
});

it("throws on malformed URL", async () => {
await expect(
setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "not-a-url",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
makeDeps() as never,
),
).rejects.toThrow(/Invalid inference endpoint URL/);
});

it("accepts a public HTTPS endpoint", async () => {
const deps = makeDeps();
await setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: "https://integrate.api.nvidia.com/v1",
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
deps as never,
);
expect(deps.runOpenshell).toHaveBeenCalled();
});

it("skips SSRF check when endpointUrl is null", async () => {
const deps = makeDeps();
await setupHermesProviderInference(
{
sandboxName: "alpha",
model: "m",
provider: "p",
endpointUrl: null,
credentialEnv: null,
hermesAuthMethod: null,
hermesToolGateways: [],
},
deps as never,
);
expect(deps.runOpenshell).toHaveBeenCalled();
});
});
14 changes: 14 additions & 0 deletions src/lib/onboard/inference-providers/hermes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// Extracted verbatim from onboard.setupInference (#767).

import type { HermesAuthMethod } from "../hermes-auth";
import { isPrivateHostname } from "../../private-networks";
import type { HermesDeps, SetupInferenceResult } from "./types";

export async function setupHermesProviderInference(
Expand All @@ -28,6 +29,19 @@ export async function setupHermesProviderInference(
hermesAuthMethod,
hermesToolGateways,
} = args;
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.`,
);
}
}
Comment on lines +32 to +44

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

Suggested change
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 {
runOpenshell,
upsertProvider: _upsertProvider, // intentionally unused; matches inline branch
Expand Down
14 changes: 14 additions & 0 deletions src/lib/policy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,20 @@ function loadPresetFromFile(filePath: string): { presetName: string; content: st
console.error(` Preset missing network_policies section: ${filePath}`);
return null;
}
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;
}
}
}
Comment on lines +1068 to +1081

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.ts

Repository: 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.ts

Repository: 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.ts

Repository: 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.

const builtin = listPresets().map((p) => p.name);
if (builtin.includes(presetName)) {
console.error(
Expand Down
106 changes: 106 additions & 0 deletions src/lib/policy/preset-allowed-ips.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";

import { afterEach, beforeEach, describe, expect, it } from "vitest";

import { loadPresetFromFile } from ".";

let tempDir: string;

function writePreset(name: string, body: string): string {
const file = path.join(tempDir, `${name}.yaml`);
fs.writeFileSync(file, body);
return file;
}

beforeEach(() => {
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "preset-ssrf-test-"));
});

afterEach(() => {
fs.rmSync(tempDir, { recursive: true, force: true });
});

describe("loadPresetFromFile allowed_ips guard (#6073)", () => {
it("rejects a preset whose endpoint declares allowed_ips", () => {
const file = writePreset(
"evil-preset",
`\
preset:
name: evil-preset
description: sneaky
network_policies:
evil:
endpoints:
- host: 10.200.0.2
port: 18789
allowed_ips:
- 10.0.0.0/8
`,
);
expect(loadPresetFromFile(file)).toBeNull();
});

it("rejects when allowed_ips appears in a second policy entry", () => {
const file = writePreset(
"evil-preset-2",
`\
preset:
name: evil-preset-2
description: sneaky second policy
network_policies:
legit:
endpoints:
- host: api.example.com
port: 443
evil:
endpoints:
- host: 192.168.1.1
port: 8080
allowed_ips:
- 192.168.0.0/16
`,
);
expect(loadPresetFromFile(file)).toBeNull();
});

it("accepts a valid preset with no allowed_ips", () => {
const file = writePreset(
"good-preset",
`\
preset:
name: good-preset
description: clean
network_policies:
api:
endpoints:
- host: api.example.com
port: 443
`,
);
expect(loadPresetFromFile(file)).toMatchObject({ presetName: "good-preset" });
});

it("accepts endpoints that omit allowed_ips entirely", () => {
const file = writePreset(
"no-ips-preset",
`\
preset:
name: no-ips-preset
description: plain endpoints only
network_policies:
cdn:
endpoints:
- host: cdn.example.com
port: 443
- host: assets.example.com
port: 443
`,
);
expect(loadPresetFromFile(file)).toMatchObject({ presetName: "no-ips-preset" });
});
});
Loading
Loading