Skip to content

Commit d583fa1

Browse files
authored
refactor(security): share private-network boundary (NVIDIA#9445)
<!-- markdownlint-disable MD041 --> ## Summary Share private-network policy parsing and address matching between the CLI and blueprint packages. Package-local loading, path resolution, and caching stay unchanged while the duplicated security logic moves behind one generated CommonJS boundary. ## Related Issue Fixes NVIDIA#8291 ## Changes - Add `nemoclaw/src/shared/private-networks-boundary.cts` as the single parser and matcher implementation used by both packages. - Keep each package's existing policy-file resolution, cache behavior, and package-specific helpers in its local wrapper. - Build and resolve the shared boundary in both package and Vitest configurations. - Update the package-contract test to exercise the generated boundary and both package loaders by behavior. A direct change to either package alone would leave the other copy free to drift; the 235-case package-contract suite protects the shared consumer boundary. - Remove more duplicated code than the shared module adds: 246 insertions and 258 deletions. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: [Focused security review of commit `f84d33115a87bca9c1405f0feb454307473cac3a` passed with no actionable findings](NVIDIA#9445 (review)). - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station preparation changes. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project package-contract test/package-contract/ssrf-parity.test.ts test/package-contract/openshell-policy-boundary.test.ts` (235 passed); plugin SSRF suites (146 passed); adjacent CLI/integration SSRF suites (77 passed) - [x] Applicable broad gate passed — This is a bounded internal refactor rather than a repo-wide runtime or test-harness change. Both package builds, both package typechecks, `npm run lint`, and the normal commit/push hooks passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run 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) --- Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved private-network validation with clearer source and entry-level errors. * Improved matching for private IP addresses, hostnames, subdomains, bracketed hostnames, and trailing-dot forms. * Enforced canonical hostname formats while accepting valid terminal-dot names. * Ensured reserved names and private-network checks behave consistently across application components. * **Refactor** * Centralized private-network parsing and matching for more consistent results across supported interfaces. * **Tests** * Expanded coverage for CIDR matching, hostname handling, validation, and cross-component behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com>
1 parent 39e7926 commit d583fa1

8 files changed

Lines changed: 265 additions & 258 deletions

File tree

nemoclaw/src/blueprint/private-networks.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,22 @@ describe("private-networks loader", () => {
275275
expect(() => getNetworkEntries()).toThrow(/missing or empty 'name'/);
276276
});
277277

278+
it.each([" localhost", "localhost ", "."])("rejects non-canonical name %j", (name) => {
279+
seedYaml(
280+
"/blueprint/private-networks.yaml",
281+
`ipv4: []\nipv6: []\nnames:\n - name: ${JSON.stringify(name)}\n purpose: malformed\n`,
282+
);
283+
expect(() => getNetworkEntries()).toThrow(/'name' must be canonical/);
284+
});
285+
286+
it("accepts a canonical name with a terminal dot", () => {
287+
seedYaml(
288+
"/blueprint/private-networks.yaml",
289+
"ipv4: []\nipv6: []\nnames:\n - name: localhost.\n purpose: canonical FQDN\n",
290+
);
291+
expect(isPrivateHostname("localhost")).toBe(true);
292+
});
293+
278294
it("rejects a names entry with empty purpose", () => {
279295
seedYaml(
280296
"/blueprint/private-networks.yaml",

nemoclaw/src/blueprint/private-networks.ts

Lines changed: 36 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,8 @@
44
// Private-network block list for SSRF validation. Loads the canonical
55
// CIDR set from nemoclaw-blueprint/private-networks.yaml and builds a
66
// node:net BlockList on first use, then memoises until the YAML file
7-
// source or stats (mtime/size) change. The CLI has an equivalent module
8-
// at src/lib/private-networks.ts; the parity test at test/ssrf-parity.test.ts
9-
// verifies both produce identical results.
7+
// source or stats (mtime/size) change. Pure parsing and matching live in
8+
// the shared private-network boundary.
109
//
1110
// Path resolution mirrors loadBlueprint() in runner.ts: honour
1211
// NEMOCLAW_BLUEPRINT_PATH when set, otherwise try the dev-checkout
@@ -16,37 +15,37 @@
1615
// cwd-located blueprint is required at runtime.
1716

1817
import { existsSync, readFileSync, statSync } from "node:fs";
19-
import { BlockList, isIP } from "node:net";
2018
import { dirname, join } from "node:path";
2119
import { fileURLToPath } from "node:url";
2220

23-
import YAML from "yaml";
24-
25-
export interface NetworkEntry {
26-
address: string;
27-
prefix: number;
28-
purpose: string;
29-
}
30-
31-
export interface NameEntry {
32-
name: string;
33-
purpose: string;
34-
}
35-
36-
export interface NetworkDocument {
37-
ipv4: NetworkEntry[];
38-
ipv6: NetworkEntry[];
39-
names: NameEntry[];
40-
}
21+
import * as importedPrivateNetworkBoundary from "../shared/private-networks-boundary.cjs";
22+
import type {
23+
NetworkDocument,
24+
PrivateNetworkMatcher,
25+
} from "../shared/private-networks-boundary.cjs";
26+
27+
export type {
28+
NameEntry,
29+
NetworkDocument,
30+
NetworkEntry,
31+
} from "../shared/private-networks-boundary.cjs";
32+
33+
// The generated module exposes named CommonJS exports. Source-mode tsx maps
34+
// the .cjs specifier to .cts and exposes the same module as its default.
35+
const sourceOrGeneratedPrivateNetworkBoundary =
36+
importedPrivateNetworkBoundary as typeof importedPrivateNetworkBoundary & {
37+
default?: typeof importedPrivateNetworkBoundary;
38+
};
39+
const { createPrivateNetworkMatcher, parsePrivateNetworkDocument } =
40+
sourceOrGeneratedPrivateNetworkBoundary.default ?? sourceOrGeneratedPrivateNetworkBoundary;
4141

4242
interface LoadedNetworks {
4343
source: string;
4444
mtimeMs: number;
4545
size: number;
4646
checkedAtMs: number;
4747
networks: NetworkDocument;
48-
blockList: BlockList;
49-
normalisedNames: string[];
48+
matcher: PrivateNetworkMatcher;
5049
}
5150

5251
// Keep hot SSRF checks in memory while still letting long-running plugin
@@ -74,79 +73,6 @@ function resolveBlueprintPath(): string {
7473
return ".";
7574
}
7675

77-
function validateNetworkEntry(
78-
entry: unknown,
79-
family: "ipv4" | "ipv6",
80-
index: number,
81-
source: string,
82-
): NetworkEntry {
83-
const where = `${source}: ${family}[${String(index)}]`;
84-
if (typeof entry !== "object" || entry === null) {
85-
throw new Error(`${where}: expected an object`);
86-
}
87-
const record = entry as Record<string, unknown>;
88-
const address = record.address;
89-
const prefix = record.prefix;
90-
const purpose = record.purpose;
91-
if (typeof address !== "string" || address.length === 0) {
92-
throw new Error(`${where}: missing or empty 'address'`);
93-
}
94-
const expectedFamily = family === "ipv4" ? 4 : 6;
95-
if (isIP(address) !== expectedFamily) {
96-
throw new Error(
97-
`${where}: 'address' must be a valid ${family} literal, got ${JSON.stringify(address)}`,
98-
);
99-
}
100-
const maxPrefix = family === "ipv4" ? 32 : 128;
101-
if (typeof prefix !== "number" || !Number.isInteger(prefix) || prefix < 0 || prefix > maxPrefix) {
102-
throw new Error(
103-
`${where}: 'prefix' must be an integer in [0, ${String(maxPrefix)}], got ${JSON.stringify(prefix)}`,
104-
);
105-
}
106-
if (typeof purpose !== "string" || purpose.trim().length === 0) {
107-
throw new Error(
108-
`${where}: 'purpose' must be a non-empty string so reviewers can judge the block`,
109-
);
110-
}
111-
return { address, prefix, purpose };
112-
}
113-
114-
function validateNameEntry(entry: unknown, index: number, source: string): NameEntry {
115-
const where = `${source}: names[${String(index)}]`;
116-
if (typeof entry !== "object" || entry === null) {
117-
throw new Error(`${where}: expected an object`);
118-
}
119-
const record = entry as Record<string, unknown>;
120-
const name = record.name;
121-
const purpose = record.purpose;
122-
if (typeof name !== "string" || name.length === 0) {
123-
throw new Error(`${where}: missing or empty 'name'`);
124-
}
125-
if (typeof purpose !== "string" || purpose.trim().length === 0) {
126-
throw new Error(
127-
`${where}: 'purpose' must be a non-empty string so reviewers can judge the block`,
128-
);
129-
}
130-
return { name, purpose };
131-
}
132-
133-
function parseDocument(raw: string, source: string): NetworkDocument {
134-
const parsed = YAML.parse(raw) as Record<string, unknown> | null;
135-
if (
136-
!parsed ||
137-
!Array.isArray(parsed.ipv4) ||
138-
!Array.isArray(parsed.ipv6) ||
139-
!Array.isArray(parsed.names)
140-
) {
141-
throw new Error(`${source}: expected top-level 'ipv4', 'ipv6', and 'names' arrays`);
142-
}
143-
return {
144-
ipv4: parsed.ipv4.map((entry, i) => validateNetworkEntry(entry, "ipv4", i, source)),
145-
ipv6: parsed.ipv6.map((entry, i) => validateNetworkEntry(entry, "ipv6", i, source)),
146-
names: parsed.names.map((entry, i) => validateNameEntry(entry, i, source)),
147-
};
148-
}
149-
15076
function isNodeEnoent(err: unknown): boolean {
15177
return err instanceof Error && "code" in err && err.code === "ENOENT";
15278
}
@@ -179,23 +105,20 @@ function load(): LoadedNetworks {
179105
cached.checkedAtMs = now;
180106
return cached;
181107
}
182-
const networks = parseDocument(readPrivateNetworksFile(source), source);
183-
const blockList = new BlockList();
184-
for (const { address, prefix } of networks.ipv4) blockList.addSubnet(address, prefix, "ipv4");
185-
for (const { address, prefix } of networks.ipv6) blockList.addSubnet(address, prefix, "ipv6");
186-
const normalisedNames = networks.names.map((e) => e.name.replace(/\.$/, "").toLowerCase());
187-
cached = { source, mtimeMs, size, checkedAtMs: now, networks, blockList, normalisedNames };
108+
const networks = parsePrivateNetworkDocument(readPrivateNetworksFile(source), source);
109+
cached = {
110+
source,
111+
mtimeMs,
112+
size,
113+
checkedAtMs: now,
114+
networks,
115+
matcher: createPrivateNetworkMatcher(networks),
116+
};
188117
return cached;
189118
}
190119

191-
function isPrivateIpInBlockList(address: string, blockList: BlockList): boolean {
192-
const family = isIP(address);
193-
if (family === 0) return false;
194-
return blockList.check(address, family === 6 ? "ipv6" : "ipv4");
195-
}
196-
197-
export function getPrivateNetworks(): BlockList {
198-
return load().blockList;
120+
export function getPrivateNetworks(): PrivateNetworkMatcher["blockList"] {
121+
return load().matcher.blockList;
199122
}
200123

201124
export function getNetworkEntries(): NetworkDocument {
@@ -223,7 +146,7 @@ export function resetCache(): void {
223146
* because BlockList does not extract embedded IPv4 from those forms.
224147
*/
225148
export function isPrivateIp(address: string): boolean {
226-
return isPrivateIpInBlockList(address, getPrivateNetworks());
149+
return load().matcher.isPrivateIp(address);
227150
}
228151

229152
/**
@@ -239,15 +162,5 @@ export function isPrivateIp(address: string): boolean {
239162
* the narrower isPrivateIp.
240163
*/
241164
export function isPrivateHostname(hostname: string): boolean {
242-
// Strip URL IPv6 brackets before any check. Brackets are only legal
243-
// in URL syntax around IPv6 literals, so stripping them is safe for
244-
// both the name-level and IP-literal checks below.
245-
const stripped =
246-
hostname.startsWith("[") && hostname.endsWith("]") ? hostname.slice(1, -1) : hostname;
247-
const normalised = stripped.replace(/\.$/, "").toLowerCase();
248-
const { blockList, normalisedNames } = load();
249-
for (const reserved of normalisedNames) {
250-
if (normalised === reserved || normalised.endsWith(`.${reserved}`)) return true;
251-
}
252-
return isPrivateIpInBlockList(normalised, blockList);
165+
return load().matcher.isPrivateHostname(hostname);
253166
}
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
import { BlockList, isIP } from "node:net";
5+
6+
import YAML from "yaml";
7+
8+
export interface NetworkEntry {
9+
address: string;
10+
prefix: number;
11+
purpose: string;
12+
}
13+
14+
export interface NameEntry {
15+
name: string;
16+
purpose: string;
17+
}
18+
19+
export interface NetworkDocument {
20+
ipv4: NetworkEntry[];
21+
ipv6: NetworkEntry[];
22+
names: NameEntry[];
23+
}
24+
25+
export interface PrivateNetworkMatcher {
26+
blockList: BlockList;
27+
isPrivateIp(address: string): boolean;
28+
isPrivateHostname(hostname: string): boolean;
29+
}
30+
31+
function validateNetworkEntry(
32+
entry: unknown,
33+
family: "ipv4" | "ipv6",
34+
index: number,
35+
source: string,
36+
): NetworkEntry {
37+
const where = `${source}: ${family}[${String(index)}]`;
38+
if (typeof entry !== "object" || entry === null) {
39+
throw new Error(`${where}: expected an object`);
40+
}
41+
const record = entry as Record<string, unknown>;
42+
const { address, prefix, purpose } = record;
43+
if (typeof address !== "string" || address.length === 0) {
44+
throw new Error(`${where}: missing or empty 'address'`);
45+
}
46+
const expectedFamily = family === "ipv4" ? 4 : 6;
47+
if (isIP(address) !== expectedFamily) {
48+
throw new Error(
49+
`${where}: 'address' must be a valid ${family} literal, got ${JSON.stringify(address)}`,
50+
);
51+
}
52+
const maxPrefix = family === "ipv4" ? 32 : 128;
53+
if (typeof prefix !== "number" || !Number.isInteger(prefix) || prefix < 0 || prefix > maxPrefix) {
54+
throw new Error(
55+
`${where}: 'prefix' must be an integer in [0, ${String(maxPrefix)}], got ${JSON.stringify(prefix)}`,
56+
);
57+
}
58+
if (typeof purpose !== "string" || purpose.trim().length === 0) {
59+
throw new Error(
60+
`${where}: 'purpose' must be a non-empty string so reviewers can judge the block`,
61+
);
62+
}
63+
return { address, prefix, purpose };
64+
}
65+
66+
function validateNameEntry(entry: unknown, index: number, source: string): NameEntry {
67+
const where = `${source}: names[${String(index)}]`;
68+
if (typeof entry !== "object" || entry === null) {
69+
throw new Error(`${where}: expected an object`);
70+
}
71+
const record = entry as Record<string, unknown>;
72+
const { name, purpose } = record;
73+
if (typeof name !== "string" || name.length === 0) {
74+
throw new Error(`${where}: missing or empty 'name'`);
75+
}
76+
if (name !== name.trim() || name.replace(/\.$/, "").length === 0) {
77+
throw new Error(`${where}: 'name' must be canonical and contain no surrounding whitespace`);
78+
}
79+
if (typeof purpose !== "string" || purpose.trim().length === 0) {
80+
throw new Error(
81+
`${where}: 'purpose' must be a non-empty string so reviewers can judge the block`,
82+
);
83+
}
84+
return { name, purpose };
85+
}
86+
87+
export function parsePrivateNetworkDocument(raw: string, source: string): NetworkDocument {
88+
const parsed = YAML.parse(raw) as Record<string, unknown> | null;
89+
if (
90+
!parsed ||
91+
!Array.isArray(parsed.ipv4) ||
92+
!Array.isArray(parsed.ipv6) ||
93+
!Array.isArray(parsed.names)
94+
) {
95+
throw new Error(`${source}: expected top-level 'ipv4', 'ipv6', and 'names' arrays`);
96+
}
97+
return {
98+
ipv4: parsed.ipv4.map((entry, index) => validateNetworkEntry(entry, "ipv4", index, source)),
99+
ipv6: parsed.ipv6.map((entry, index) => validateNetworkEntry(entry, "ipv6", index, source)),
100+
names: parsed.names.map((entry, index) => validateNameEntry(entry, index, source)),
101+
};
102+
}
103+
104+
export function createPrivateNetworkMatcher(networks: NetworkDocument): PrivateNetworkMatcher {
105+
const blockList = new BlockList();
106+
for (const { address, prefix } of networks.ipv4) blockList.addSubnet(address, prefix, "ipv4");
107+
for (const { address, prefix } of networks.ipv6) blockList.addSubnet(address, prefix, "ipv6");
108+
const normalisedNames = networks.names.map((entry) =>
109+
entry.name.replace(/\.$/, "").toLowerCase(),
110+
);
111+
112+
const isPrivateIp = (address: string): boolean => {
113+
const family = isIP(address);
114+
if (family === 0) return false;
115+
return blockList.check(address, family === 6 ? "ipv6" : "ipv4");
116+
};
117+
118+
return {
119+
blockList,
120+
isPrivateIp,
121+
isPrivateHostname(hostname: string): boolean {
122+
const stripped =
123+
hostname.startsWith("[") && hostname.endsWith("]") ? hostname.slice(1, -1) : hostname;
124+
const normalised = stripped.replace(/\.$/, "").toLowerCase();
125+
for (const reserved of normalisedNames) {
126+
if (normalised === reserved || normalised.endsWith(`.${reserved}`)) return true;
127+
}
128+
return isPrivateIp(normalised);
129+
},
130+
};
131+
}

nemoclaw/tsconfig.shared.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"src/shared/banner-boundary.cts",
99
"src/shared/credential-filter-boundary.cts",
1010
"src/shared/openshell-policy-boundary.cts",
11+
"src/shared/private-networks-boundary.cts",
1112
"src/shared/sandbox-name.cts",
1213
"src/shared/snapshot-sanitizer-boundary.cts"
1314
],

nemoclaw/vitest.project.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ const canonicalOpenShellPolicyBoundary = path.resolve(
1313
import.meta.dirname,
1414
"src/shared/openshell-policy-boundary.cts",
1515
);
16+
const canonicalPrivateNetworksBoundary = path.resolve(
17+
import.meta.dirname,
18+
"src/shared/private-networks-boundary.cts",
19+
);
1620
const canonicalSandboxName = path.resolve(import.meta.dirname, "src/shared/sandbox-name.cts");
1721
const canonicalSnapshotSanitizerBoundary = path.resolve(
1822
import.meta.dirname,
@@ -60,6 +64,10 @@ const pluginVitestProjectOptions = {
6064
find: /^.*openshell-policy-boundary\.cjs$/,
6165
replacement: canonicalOpenShellPolicyBoundary,
6266
},
67+
{
68+
find: /^.*private-networks-boundary\.cjs$/,
69+
replacement: canonicalPrivateNetworksBoundary,
70+
},
6371
{
6472
find: /^.*sandbox-name\.cjs$/,
6573
replacement: canonicalSandboxName,

0 commit comments

Comments
 (0)