fix(security): share private-network block list between CLI and plugin#2324
Open
laitingsheng wants to merge 9 commits intomainfrom
Open
fix(security): share private-network block list between CLI and plugin#2324laitingsheng wants to merge 9 commits intomainfrom
laitingsheng wants to merge 9 commits intomainfrom
Conversation
Replace the weak prefix-based isPrivateIp in src/lib/sandbox-config.ts with a node:net BlockList built from a new canonical source under nemoclaw-blueprint/private-networks.yaml. The plugin's nemoclaw/src/blueprint/ssrf.ts now loads the same data, so the drift that allowed #2300 to exist cannot recur. Coverage added: CGNAT, IETF protocol assignments (incl. DS-Lite), IPv4 documentation ranges, IPv4 multicast and reserved-for-future-use (with 255.255.255.255 limited broadcast), and the IPv6 translation prefixes NAT64 well-known, NAT64 local-use, Teredo, and 6to4. IPv4-mapped IPv6 (::ffff:x.x.x.x) is handled by BlockList's cross-family auto-match; no custom extraction needed. Adds a 'names' section to the YAML for reserved private/internal name-level matches — localhost (RFC 6761), local (RFC 6762 mDNS), and internal (ICANN-reserved 2024 private-use TLD). Matching is case-insensitive and trailing-dot-normalised, covering the CodeRabbit finding on #2305 about *.localhost and FQDN-form variants. Every YAML entry requires a non-empty purpose field so blocks ship with a human-reviewable rationale rather than a bare CIDR or bare name. The new test/ssrf-parity.test.ts guards against drift: schema checks plus per-CIDR boundary vectors (start, end, two middles, one below start, one above end) asserting CLI and plugin isPrivateIp agree. The plugin-side private-networks.test.ts covers path resolution and schema-validation error branches. Fixes #2300 Supersedes and closes #2305 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a canonical SSRF-deny YAML, runtime loaders in blueprint and CLI/library, strict schema validation, memoized node:net BlockList and normalized reserved-name list, new exported accessors (getNetworkEntries/getPrivateNetworks/isPrivateIp/isPrivateHostname/resetCache), and extensive tests and parity checks. (28 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant SSRF_Module as "ssrf.ts"
participant PrivateNetworks as "private-networks(.ts)"
participant FileSystem as "private-networks.yaml"
participant BlockList as "node:net BlockList"
Client->>SSRF_Module: validate endpoint / hostname
SSRF_Module->>PrivateNetworks: isPrivateHostname(hostname)
PrivateNetworks->>FileSystem: resolve & read YAML (env/dev/cwd) [cached]
FileSystem-->>PrivateNetworks: YAML document
PrivateNetworks->>PrivateNetworks: validate schema & build memoized entries
PrivateNetworks->>BlockList: construct/add CIDRs
BlockList-->>PrivateNetworks: membership checks
PrivateNetworks-->>SSRF_Module: boolean (private or not)
SSRF_Module-->>Client: validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/sandbox-config.ts (1)
324-336:⚠️ Potential issue | 🟠 MajorRun
validateUrlValue()for every string, not only lower-casehttp(s)prefixes.The guard on Lines 325-327 lets valid URLs like
HTTP://127.0.0.1,Https://localhost, or whitespace-padded inputs skip SSRF validation entirely.validateUrlValue()already no-ops for non-URLs, so this prefix check only creates a bypass.Suggested fix
- if ( - typeof parsedValue === "string" && - (parsedValue.startsWith("http://") || parsedValue.startsWith("https://")) - ) { + if (typeof parsedValue === "string") { try { validateUrlValue(parsedValue); } catch (err: unknown) { const message = err instanceof Error ? err.message : String(err); console.error(` URL validation failed: ${message}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-config.ts` around lines 324 - 336, The current guard only runs validateUrlValue when parsedValue starts with lower-case "http://" or "https://", letting mixed-case or whitespace-padded URLs bypass SSRF checks; change the logic so that for any typeof parsedValue === "string" you call validateUrlValue (after trimming the string) inside the existing try/catch. Specifically, replace the startsWith checks with a simple typeof parsedValue === "string" condition, call validateUrlValue(parsedValue.trim()), and keep the existing error handling that logs the message and exits; reference parsedValue and validateUrlValue to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/blueprint/private-networks.ts`:
- Around line 49-56: resolveBlueprintPath() currently returns the devGuess
directory if it exists even when the required private-networks.yaml isn't
present, causing load() to fail; update resolveBlueprintPath to only return
devGuess when both the directory exists and the file join(devGuess,
"private-networks.yaml") exists (use existsSync), otherwise fall back to the
current default ("."); refer to the resolveBlueprintPath function in
private-networks.ts and the subsequent load() call so the check ensures the YAML
is present before returning the dev checkout path.
In `@test/ssrf-parity.test.ts`:
- Around line 36-38: The parity guard currently omits validating
names[*].purpose: update the NetworkHelper API returned by getNetworkEntries()
to include the names array (e.g., add a names: NetworkEntryName[] or similar
shape) and then extend the assertion loop that iterates ipv4/ipv6 to also
iterate each entry in names and assert that name.purpose is present and
non-empty; update any type references and the test assertions that use
NetworkHelper/getNetworkEntries() and isPrivateHostname to ensure
names[*].purpose is checked (mirror the ipv4/ipv6 validation logic).
---
Outside diff comments:
In `@src/lib/sandbox-config.ts`:
- Around line 324-336: The current guard only runs validateUrlValue when
parsedValue starts with lower-case "http://" or "https://", letting mixed-case
or whitespace-padded URLs bypass SSRF checks; change the logic so that for any
typeof parsedValue === "string" you call validateUrlValue (after trimming the
string) inside the existing try/catch. Specifically, replace the startsWith
checks with a simple typeof parsedValue === "string" condition, call
validateUrlValue(parsedValue.trim()), and keep the existing error handling that
logs the message and exits; reference parsedValue and validateUrlValue to locate
the code to change.
🪄 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: Pro Plus
Run ID: f152327f-d496-4851-a373-fd651a6f27fb
📒 Files selected for processing (8)
nemoclaw-blueprint/private-networks.yamlnemoclaw/src/blueprint/private-networks.test.tsnemoclaw/src/blueprint/private-networks.tsnemoclaw/src/blueprint/ssrf.test.tsnemoclaw/src/blueprint/ssrf.tssrc/lib/private-networks.tssrc/lib/sandbox-config.tstest/ssrf-parity.test.ts
* private-networks.ts resolveBlueprintPath: check for the
private-networks.yaml file specifically instead of just the
nemoclaw-blueprint directory. A stale checkout with the directory
but no yaml now falls through to cwd instead of failing load().
* ssrf-parity.test.ts: extend the non-empty-purpose loop and the
duplicate-entry check to cover the names array, so a blank
names[*].purpose or a duplicate name can no longer slip through.
* sandbox-config.ts validateUrlValue guard: drop the lowercase
startsWith("http://"|"https://") pre-filter, which let mixed-case
schemes (HTTP://127.0.0.1) and whitespace-padded URLs bypass
validation. Trim the string and hand every string to
validateUrlValue, which already no-ops for non-URL input.
Plus a matching dev-guess-fallback test in the plugin's
private-networks.test.ts.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
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/ssrf-parity.test.ts`:
- Around line 83-90: The duplicate-check currently builds the name keys from
doc.names using e.name.toLowerCase() but doesn't remove a trailing dot, so names
like "localhost" and "localhost." slip past; update the key creation for names
in the keys array to normalize exactly like runtime by lowercasing and stripping
a single trailing dot (e.g., transform e.name via lowercasing then remove a
trailing '.' before building the `name:${...}` entry), leaving the IPv4/IPv6 key
logic unchanged.
🪄 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: Pro Plus
Run ID: 5fd826a5-fa2e-4110-94d9-f52a6c724704
📒 Files selected for processing (4)
nemoclaw/src/blueprint/private-networks.test.tsnemoclaw/src/blueprint/private-networks.tssrc/lib/sandbox-config.tstest/ssrf-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- nemoclaw/src/blueprint/private-networks.test.ts
- src/lib/sandbox-config.ts
- nemoclaw/src/blueprint/private-networks.ts
Match the runtime name-normalisation in isPrivateHostname, so the duplicate-entry guard catches a name that appears with and without the trailing FQDN dot (e.g. localhost vs localhost.). Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ssrf-parity.test.ts (2)
46-47: Add clearer failure diagnostics for missing build artifacts.
Directrequire()from dist paths fails with a generic module error; wrapping loads with a small helper gives contributors an immediate build hint and reduces triage time.Suggested change
const require = createRequire(import.meta.url); @@ -const cliHelper = require("../dist/lib/private-networks") as NetworkHelper; -const pluginHelper = require("../nemoclaw/dist/blueprint/private-networks.js") as NetworkHelper; +function loadHelper(path: string, hint: string): NetworkHelper { + try { + return require(path) as NetworkHelper; + } catch { + throw new Error(`Failed to load '${path}'. ${hint}`); + } +} + +const cliHelper = loadHelper("../dist/lib/private-networks", "Run `npm run build:cli` first."); +const pluginHelper = loadHelper( + "../nemoclaw/dist/blueprint/private-networks.js", + "Run `npm run build` in `nemoclaw/` first.", +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ssrf-parity.test.ts` around lines 46 - 47, The test currently does direct requires for dist artifacts (cliHelper and pluginHelper) which surface generic module-not-found errors; create and use a small loader helper (e.g., loadDist or requireWithBuildHint) that attempts require(path) and on failure throws or logs a clear message telling the contributor to run the build (include the original error message for diagnostics), then replace the direct requires of "../dist/lib/private-networks" and "../nemoclaw/dist/blueprint/private-networks.js" with calls to that helper so failures show the actionable build hint and original error details.
105-274: Consider deriving CIDR boundary vectors fromgetNetworkEntries()to prevent stale coverage.
The current matrix is strong, but manual vectors can drift when the YAML changes. Generating vectors from parsed CIDRs keeps boundary coverage automatically aligned with the canonical block list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ssrf-parity.test.ts` around lines 105 - 274, The hardcoded vectors array in test/ssrf-parity.test.ts can become stale; instead generate boundary test vectors from the canonical CIDRs returned by getNetworkEntries(): iterate getNetworkEntries() (the same function that parses the YAML), and for each NetworkEntry produce the start, end, quarter, three-quarter and optional before/after addresses (handling IPv4 vs IPv6 appropriately) and replace the static vectors variable with this generated list so tests stay in sync with the source CIDR list; update any test consumer of vectors to accept the generated format and ensure ordering/labels remain stable.
🤖 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/ssrf-parity.test.ts`:
- Around line 46-47: The test currently does direct requires for dist artifacts
(cliHelper and pluginHelper) which surface generic module-not-found errors;
create and use a small loader helper (e.g., loadDist or requireWithBuildHint)
that attempts require(path) and on failure throws or logs a clear message
telling the contributor to run the build (include the original error message for
diagnostics), then replace the direct requires of "../dist/lib/private-networks"
and "../nemoclaw/dist/blueprint/private-networks.js" with calls to that helper
so failures show the actionable build hint and original error details.
- Around line 105-274: The hardcoded vectors array in test/ssrf-parity.test.ts
can become stale; instead generate boundary test vectors from the canonical
CIDRs returned by getNetworkEntries(): iterate getNetworkEntries() (the same
function that parses the YAML), and for each NetworkEntry produce the start,
end, quarter, three-quarter and optional before/after addresses (handling IPv4
vs IPv6 appropriately) and replace the static vectors variable with this
generated list so tests stay in sync with the source CIDR list; update any test
consumer of vectors to accept the generated format and ensure ordering/labels
remain stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3e45b52-2da3-4534-bfa6-d86fb4b856ad
📒 Files selected for processing (1)
test/ssrf-parity.test.ts
wscurran
added
bug
Contributor
|
✨ Thanks for submitting this issue that identifies a bug with the private network block lists and proposes a fix. Related open PRs: Related open issues: |
ericksoa
requested changes
Apr 23, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Nice work on this one, Tinson. The architecture is sound — single YAML source of truth with a parity test is exactly the right approach, and the coverage expansion (CGNAT, translation prefixes, reserved TLDs) is substantial.
One small change request, then a few items for follow-up.
Requested change
Add prefix-length upper-bound validation. Both loaders validate prefix >= 0 but never check prefix <= 32 (IPv4) or prefix <= 128 (IPv6). A typo like prefix: 33 on an IPv4 entry would be silently passed to BlockList.addSubnet with undefined behavior.
In both src/lib/private-networks.ts and nemoclaw/src/blueprint/private-networks.ts, the validateNetworkEntry function should include:
const maxPrefix = family === "ipv4" ? 32 : 128;
if (typeof prefix !== "number" || !Number.isInteger(prefix) || prefix < 0 || prefix > maxPrefix) {Follow-up items (non-blocking)
-
Code duplication between CLI and plugin loaders — the two
private-networks.tsfiles share ~90% identical code. Any future logic change (like this prefix bound) must be applied in both. Worth considering whether a shared internal package or build-time copy could reduce the maintenance surface. -
Parity test depends on pre-built artifacts —
test/ssrf-parity.test.tsimports fromdist/. A developer who forgets to build first gets a confusingMODULE_NOT_FOUND. A guard with a descriptive error message would improve DX. -
readFileSyncerror on missing YAML — a missing file produces a rawENOENT. A one-line existence check before the read with a hint aboutNEMOCLAW_BLUEPRINT_PATHwould help. -
192.0.1.0/24gap —192.0.0.0/24and192.0.2.0/24are both blocked but the/24in between is not. If intentional, a comment in the YAML noting the gap would be helpful.
Contributor
Author
I will try to address the rest. |
Contributor
Author
Regarding 1, since there is a boundary for plugin and CLI, I will hold the changes since it needs more complex refactorings, and I think it is already out of the scope of this PR. But I totally agree that we need to deduplicate the code to avoid any possibile mistakes even though we have parity tests. This could be beneficial for 2, but I would just improve it a bit according to your suggestions for this PR. |
- validateNetworkEntry now enforces prefix ∈ [0, 32] for IPv4 and [0, 128] for IPv6. Previously an out-of-range value like prefix: 33 was passed through to BlockList.addSubnet with undefined behaviour. Applied symmetrically to the CLI loader (src/lib/private-networks.ts) and the plugin loader (nemoclaw/src/blueprint/private-networks.ts). - Both loaders now existsSync-precheck the resolved YAML path and throw a message naming the path and NEMOCLAW_BLUEPRINT_PATH, instead of a raw ENOENT from readFileSync. - test/ssrf-parity.test.ts wraps its two dist/ require() calls in a helper that catches MODULE_NOT_FOUND and points at the relevant build command, replacing the confusing generic module error when a developer forgets to build first. - nemoclaw-blueprint/private-networks.yaml gains a comment explaining that the gap at 192.0.1.0/24 is intentional — that /24 is globally routable APNIC space, not an IANA special-purpose assignment like the /24 blocks either side of it. Tests extended with coverage for negative, fractional, too-big IPv4 (>32), and too-big IPv6 (>128) prefixes, plus a missing-YAML error message assertion. The CLI-plugin loader duplication item from the review is tracked as a follow-up — the right shape is either a pure- functions core with two thin wrappers or a full collapse, and the decision belongs in its own PR. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…x/2300-ssrf-blocklist Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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 `@src/lib/private-networks.ts`:
- Around line 103-111: The error thrown in load() referencing
NEMOCLAW_BLUEPRINT_PATH is misleading because this module uses ROOT (from
./runner) and resolves the blueprint at build/compile time; update the message
thrown when NETWORKS_FILE is missing to explain that the file is sought relative
to the compiled project root (ROOT) and suggest ensuring the blueprint is
present in the repository/checkout or adjusting the build/checkout location, not
setting an environment variable; locate the load() function and replace the
incorrect env-var guidance with wording that mentions ROOT or the compiled
project root and how to place the nemoclaw-blueprint/ directory so NETWORKS_FILE
can be found.
In `@test/ssrf-parity.test.ts`:
- Around line 46-59: The catch block in loadHelper uses the undefined NodeJS
type and discards the original error; change the error typing to unknown (e.g.,
const err = error as unknown as { code?: string } or narrow from unknown) and
check err.code for "MODULE_NOT_FOUND", and when throwing the new Error include
the original error as the cause (using new Error(message, { cause: error })) so
the stack context is preserved; update references in loadHelper, modulePath,
buildHint, and err accordingly.
🪄 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: 159b5b7d-176b-4365-8e1f-67ac5d300c7e
📒 Files selected for processing (5)
nemoclaw-blueprint/private-networks.yamlnemoclaw/src/blueprint/private-networks.test.tsnemoclaw/src/blueprint/private-networks.tssrc/lib/private-networks.tstest/ssrf-parity.test.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw-blueprint/private-networks.yaml
- src/lib/private-networks.ts: The ENOENT error added in 4efbc76 suggested setting NEMOCLAW_BLUEPRINT_PATH as a remedy, but the CLI loader resolves its path via ROOT (path.resolve(__dirname, "..", "..")) and does not consult that env var — only the plugin loader does. Replace the suggestion with guidance that matches how the CLI actually resolves the file. - test/ssrf-parity.test.ts: The loadHelper introduced in 4efbc76 used `NodeJS.ErrnoException` (flagged by eslint no-undef in ESM context) and discarded the original error (flagged by preserve-caught-error). Narrow the type to `{ code?: unknown }` and preserve the underlying cause via `{ cause: error }`. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/private-networks.ts (1)
45-67: Validate the IP family during the schema pass.
addressis only checked for non-empty string here, so a malformed literal or an IPv6 entry underipv4would fail later inload()and lose the...ipv4[i]/...ipv6[i]context you already built. ValidatingisIP(address)againstfamilyhere keeps YAML errors precise.Suggested change
function validateNetworkEntry(entry: unknown, family: "ipv4" | "ipv6", index: number): NetworkEntry { const where = `${NETWORKS_FILE}: ${family}[${String(index)}]`; if (typeof entry !== "object" || entry === null) { throw new Error(`${where}: expected an object`); } const record = entry as Record<string, unknown>; const address = record.address; const prefix = record.prefix; const purpose = record.purpose; if (typeof address !== "string" || address.length === 0) { throw new Error(`${where}: missing or empty 'address'`); } + const expectedFamily = family === "ipv4" ? 4 : 6; + if (isIP(address) !== expectedFamily) { + throw new Error(`${where}: 'address' must be a valid ${family} literal`); + } const maxPrefix = family === "ipv4" ? 32 : 128; if (typeof prefix !== "number" || !Number.isInteger(prefix) || prefix < 0 || prefix > maxPrefix) { throw new Error( `${where}: 'prefix' must be an integer in [0, ${String(maxPrefix)}], got ${JSON.stringify(prefix)}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/private-networks.ts` around lines 45 - 67, validateNetworkEntry currently only checks that address is a non-empty string but not that it matches the declared family; update validateNetworkEntry to call the IP validation helper (e.g., isIP or equivalent) to assert that address is a valid IPv4 when family === "ipv4" and a valid IPv6 when family === "ipv6", and if the check fails throw a descriptive Error including the where variable and the family (so messages like `${where}: address '${address}' is not a valid ${family} address`); keep existing checks for prefix and purpose and return the same NetworkEntry shape so later load() no longer has to re-associate family-specific context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/private-networks.ts`:
- Around line 45-67: validateNetworkEntry currently only checks that address is
a non-empty string but not that it matches the declared family; update
validateNetworkEntry to call the IP validation helper (e.g., isIP or equivalent)
to assert that address is a valid IPv4 when family === "ipv4" and a valid IPv6
when family === "ipv6", and if the check fails throw a descriptive Error
including the where variable and the family (so messages like `${where}: address
'${address}' is not a valid ${family} address`); keep existing checks for prefix
and purpose and return the same NetworkEntry shape so later load() no longer has
to re-associate family-specific context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e85c81e-85e8-494f-b9c4-4fd90e5050fd
📒 Files selected for processing (2)
src/lib/private-networks.tstest/ssrf-parity.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/ssrf-parity.test.ts
Both loaders now reject an entry whose address doesn't parse as the declared family — an IPv6 literal under ipv4 (or vice versa), or a malformed string. Previously typeof/length was the only guard, so a typo like placing "::1" under ipv4 silently passed validation and later surfaced as a confusing runtime error from BlockList.addSubnet with no ipv4[i] / ipv6[i] location context. Applied symmetrically to src/lib/private-networks.ts and nemoclaw/src/blueprint/private-networks.ts so the parity test continues to hold. Tests added for all three rejection paths: IPv6-under-ipv4, IPv4-under-ipv6, and a non-IP-literal string. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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
3 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
Replace the weak prefix-based isPrivateIp in src/lib/sandbox-config.ts with a node:net BlockList built from a new canonical source under nemoclaw-blueprint/private-networks.yaml. The plugin's nemoclaw/src/blueprint/ssrf.ts now loads the same data, so the drift that allowed #2300 to exist cannot recur.
Related Issue
Fixes #2300
Supersedes and closes #2305
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Refactor
Tests