test(security): add SSRF validation edge-case coverage#898
test(security): add SSRF validation edge-case coverage#898areporeporepo wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
- CIDR boundary precision for all private ranges - IPv6 edge cases: link-local, multicast, fd00::/8 - IPv4-mapped IPv6 boundary tests - DNS rebinding: mixed public+private A records - URL parsing: data: URI, query params, userinfo Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdded test cases expanding SSRF protections: precise IPv4 CIDR boundary checks, IPv6 edge cases including IPv4-mapped addresses, DNS rebinding simulations with mixed resolved records, and URL parsing edge-case tests (reject Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds expanded SSRF-related unit test coverage for IP classification and endpoint URL validation in the NemoClaw blueprint SSRF guardrails.
Changes:
- Add CIDR boundary tests for IPv4 private ranges (10/8, 172.16/12, 169.254/16, 192.168/16 boundaries).
- Add IPv6 edge-case tests (ULA boundaries and IPv4-mapped IPv6 cases).
- Add
validateEndpointUrltests for DNS rebinding (mixed A/AAAA results) and URL parsing cases (data: URI, query/fragment, userinfo).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // link-local and multicast are NOT in PRIVATE_NETWORKS — verify they're treated as public | ||
| ["fe80::1", false], | ||
| ["ff02::1", false], | ||
| // zero address | ||
| ["::0", false], |
There was a problem hiding this comment.
The IPv6 expectations here treat link-local (fe80::/10), multicast (ff00::/8), and the unspecified address (::/128) as “public”. For SSRF protection these ranges are non-globally-routable and are commonly treated as internal/reserved, so allowing them can enable access to local interfaces/services. Consider updating isPrivateIp/PRIVATE_NETWORKS to block these reserved ranges and adjust these tests to expect rejection (private=true) accordingly.
| // link-local and multicast are NOT in PRIVATE_NETWORKS — verify they're treated as public | |
| ["fe80::1", false], | |
| ["ff02::1", false], | |
| // zero address | |
| ["::0", false], | |
| // link-local, multicast, and unspecified are treated as private/internal for SSRF protection | |
| ["fe80::1", true], | |
| ["ff02::1", true], | |
| // zero address | |
| ["::0", true], |
| // fd00::/8 boundaries | ||
| ["fcff::1", false], // fc00::/8 is NOT protected, only fd00::/8 | ||
| ["fd00::0", true], // first address in fd00::/8 | ||
| ["fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", true], // last address in fd00::/8 | ||
| ["fe00::1", false], // just above fd00::/8 |
There was a problem hiding this comment.
This test asserts fc00::/8 (via fcff::1) is allowed, but RFC 4193 Unique Local Addresses are fc00::/7 (covering both fc00::/8 and fd00::/8) and are typically considered private/internal for SSRF defenses. Consider expanding the IPv6 private range to fc00::/7 and updating this expectation to true (and adding a boundary test for fbff:: vs fc00:: if you want precision similar to the IPv4 cases).
| // fd00::/8 boundaries | |
| ["fcff::1", false], // fc00::/8 is NOT protected, only fd00::/8 | |
| ["fd00::0", true], // first address in fd00::/8 | |
| ["fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", true], // last address in fd00::/8 | |
| ["fe00::1", false], // just above fd00::/8 | |
| // fc00::/7 (RFC 4193 Unique Local Addresses) boundaries | |
| ["fbff::1", false], // just below fc00::/7 | |
| ["fcff::1", true], // within fc00::/7 ULA range | |
| ["fd00::0", true], // first address in fd00::/8 (within fc00::/7) | |
| ["fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", true], // last address in fc00::/7 ULA range | |
| ["fe00::1", false], // just above fc00::/7 |
| await expect(validateEndpointUrl(url)).resolves.toBe(url); | ||
| }); | ||
|
|
||
| it("allows URL with basic auth in hostname", async () => { |
There was a problem hiding this comment.
Test name is misleading: https://user:pass@api.example.com/v1 is userinfo/basic auth in the URL (not “in hostname”). Renaming the test description to reflect userinfo will make it clearer what behavior is being validated.
| it("allows URL with basic auth in hostname", async () => { | |
| it("allows URL with userinfo/basic auth", async () => { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@nemoclaw/src/blueprint/ssrf.test.ts`:
- Around line 203-213: The test's it.each array in ssrf.test.ts currently marks
link-local and multicast IPv6 (e.g., "fe80::1", "ff02::1") and the fc00::/7
boundary example as allowed (false); update those expectations to true so they
are treated as protected by SSRF checks. Locate the it.each block in
ssrf.test.ts and change the boolean for "fe80::1", "ff02::1" and the fc00::/7
example entry (e.g., "fcff::1" or whichever fc00::/7 test row exists) from false
to true to reflect that these scopes are not public.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52f24342-45f1-4ceb-9568-81b1029079cf
📒 Files selected for processing (1)
nemoclaw/src/blueprint/ssrf.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nemoclaw/src/blueprint/ssrf.test.ts (1)
204-206:⚠️ Potential issue | 🔴 CriticalDo not codify link-local/multicast IPv6 as public in SSRF tests.
Line 205 and Line 206 currently assert
fe80::1andff02::1as non-private. That bakes in a security gap rather than guarding against it.🔧 Suggested expectation update
- ["fe80::1", false], - ["ff02::1", false], + ["fe80::1", true], // link-local should be blocked + ["ff02::1", true], // multicast should be blocked🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/ssrf.test.ts` around lines 204 - 206, The test currently asserts ["fe80::1", false] and ["ff02::1", false] as non-private (public); update those expectations to treat link-local and multicast IPv6 as private/blocked by changing both entries to ["fe80::1", true] and ["ff02::1", true] in the ssrf test data (the array containing those tuples) and add a brief inline comment noting they are link-local/multicast addresses so the SSRF check treats them as non-public.
🤖 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/ssrf.test.ts`:
- Around line 210-213: Tests expect the full ULA range (fc00::/7) but the
implementation's PRIVATE_NETWORKS only contains cidr6("fd00::", 8); open
nemoclaw/src/blueprint/ssrf.ts and add cidr6("fc00::", 7) to the
PRIVATE_NETWORKS array (next to the existing cidr6("fd00::", 8)), ensuring the
same type/format and any import/usage of cidr6 remains correct so the ssrf check
(PRIVATE_NETWORKS) blocks the full ULA range that the tests assert.
---
Duplicate comments:
In `@nemoclaw/src/blueprint/ssrf.test.ts`:
- Around line 204-206: The test currently asserts ["fe80::1", false] and
["ff02::1", false] as non-private (public); update those expectations to treat
link-local and multicast IPv6 as private/blocked by changing both entries to
["fe80::1", true] and ["ff02::1", true] in the ssrf test data (the array
containing those tuples) and add a brief inline comment noting they are
link-local/multicast addresses so the SSRF check treats them as non-public.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f28560f3-ae44-4653-a235-0c891b4eb86c
📒 Files selected for processing (1)
nemoclaw/src/blueprint/ssrf.test.ts
| ["fc00::1", true], // first address in fc00::/7 (ULA) | ||
| ["fcff::1", true], // still within fc00::/7 | ||
| ["fd00::0", true], // first address in fd00::/8 | ||
| ["fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", true], // last address in fd00::/8 |
There was a problem hiding this comment.
fc00::/7 expectations currently conflict with implementation.
Line 210–Line 213 expect fc00::/7 + fd00::/8 to be private, but nemoclaw/src/blueprint/ssrf.ts currently defines only cidr6("fd00::", 8) in PRIVATE_NETWORKS. This will fail unless implementation is updated in the same PR.
🔧 If intent is to block full ULA, update implementation
- cidr6("fd00::", 8),
+ cidr6("fc00::", 7),📝 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.
| ["fc00::1", true], // first address in fc00::/7 (ULA) | |
| ["fcff::1", true], // still within fc00::/7 | |
| ["fd00::0", true], // first address in fd00::/8 | |
| ["fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", true], // last address in fd00::/8 | |
| cidr6("fc00::", 7), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/blueprint/ssrf.test.ts` around lines 210 - 213, Tests expect the
full ULA range (fc00::/7) but the implementation's PRIVATE_NETWORKS only
contains cidr6("fd00::", 8); open nemoclaw/src/blueprint/ssrf.ts and add
cidr6("fc00::", 7) to the PRIVATE_NETWORKS array (next to the existing
cidr6("fd00::", 8)), ensuring the same type/format and any import/usage of cidr6
remains correct so the ssrf check (PRIVATE_NETWORKS) blocks the full ULA range
that the tests assert.
Adds 31 new test cases to
ssrf.test.tscovering:Summary by CodeRabbit