test(security): add SSRF validation edge-case coverage#898
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>
📝 WalkthroughWalkthroughUpdated SSRF protections: expanded test coverage for IPv4/IPv6 boundaries, DNS rebinding, IPv4-mapped IPv6, and URL parsing; and adjusted IPv6 private-network CIDRs used by Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
nemoclaw/src/blueprint/ssrf.test.ts
Outdated
| // 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], |
nemoclaw/src/blueprint/ssrf.test.ts
Outdated
| // 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 |
nemoclaw/src/blueprint/ssrf.test.ts
Outdated
| 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
Address review feedback on PR NVIDIA#898: - Expand ULA range from fd00::/8 to fc00::/7 (RFC 4193) in PRIVATE_NETWORKS to block the full Unique Local Address space - Add fe80::/10 (link-local), ff00::/8 (multicast), and ::/128 (unspecified) to PRIVATE_NETWORKS as non-globally-routable - Update test expectations: these scopes are now correctly treated as private/internal for SSRF protection - Add fc00::/7 boundary precision tests (fbff::1 vs fc00::1) - Rename misleading test description to 'userinfo/basic auth' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Anh Quang Nguyen <29374105+aprprprr@users.noreply.github.com>
wscurran
added
security
|
✨ Thanks for submitting this PR with a detailed summary, it may improve test coverage for security features and proposes a fix to improve SSRF validation. |
|
Tests pass locally (75/75). The expanded IPv6 CIDR coverage and boundary precision tests look solid. Good catch on link-local and multicast gaps. |
|
Hey @areporeporepo! The only failing check is DCO (Developer Certificate of Origin) meaning your commits need a Signed-off-by line. You can fix it with:
|
Signed-off-by: Anh Quang Nguyen <anh.nqqq@icloud.com>
Adds 31 new test cases to
ssrf.test.tscovering:Signed-off-by: Anh Quang Nguyen anh.nqqq@icloud.com
Summary by CodeRabbit
Tests
Bug Fixes