fix(security): align SSRF denylists and reject endpoint userinfo - #7764
fix(security): align SSRF denylists and reject endpoint userinfo#7764Ayush7614 wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughExpanded SSRF blocklists with additional IPv4/IPv6 prefixes and metadata hostnames, added parity coverage, and changed endpoint validation to reject URLs containing credentials. ChangesSSRF protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoclaw/src/blueprint/ssrf.test.ts`:
- Around line 409-415: Add username-only and password-only credential URL cases
to the validateEndpointUrl tests, alongside the existing userinfo case, and
assert each rejection message excludes the supplied credential values while
retaining the credentials rejection expectation.
🪄 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: f02291e7-4980-4623-aa73-8ff027afbfb7
📒 Files selected for processing (7)
nemoclaw-blueprint/private-networks.yamlnemoclaw/src/blueprint/private-networks.tsnemoclaw/src/blueprint/ssrf.test.tsnemoclaw/src/blueprint/ssrf.tssrc/lib/private-networks.tstest/mcp-url-target.test.tstest/package-contract/ssrf-parity.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Addressed the CodeRabbit / PR Review Advisor warning: added username-only and password-only userinfo cases, and asserted rejection messages do not echo the supplied secret values. |
Extend private-networks.yaml to the special-purpose ranges MCP already blocks, and reject credentialed endpoint URLs in plugin SSRF validation so inference and MCP share one host-safety contract. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
Reject both credential halves and assert error messages do not echo the supplied secret values. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
64b2731 to
b24b12d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@nemoclaw-blueprint/private-networks.yaml`:
- Around line 115-116: The IPv6 block list is missing the non-forwardable Dummy
IPv6 Prefix 100:0:0:1::/64. Add this prefix to the YAML-backed BlockList
alongside the existing special-purpose IPv6 entries, and add matching boundary
test vectors in ssrf-parity.test.ts covering addresses inside and outside the
prefix.
🪄 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: df2b6880-d07a-4eb6-9139-2a64dca37e26
📒 Files selected for processing (7)
nemoclaw-blueprint/private-networks.yamlnemoclaw/src/blueprint/private-networks.tsnemoclaw/src/blueprint/ssrf.test.tsnemoclaw/src/blueprint/ssrf.tssrc/lib/private-networks.tstest/mcp-url-target.test.tstest/package-contract/ssrf-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- nemoclaw/src/blueprint/private-networks.ts
- nemoclaw/src/blueprint/ssrf.ts
- src/lib/private-networks.ts
- nemoclaw/src/blueprint/ssrf.test.ts
- test/mcp-url-target.test.ts
b24b12d to
cd058dc
Compare
…ject Add IANA 100:0:0:1::/64 to the shared SSRF denylist and MCP target filter, and confirm credentialed endpoint URLs fail before DNS lookup. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
wscurran
added
area: security
|
✨ Thanks for the PR. This aligns SSRF denylists across inference, MCP, and plugin validation and rejects credentialed endpoint URLs. Maintainers will review the denylist extensions, userinfo rejection, and test coverage. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit a7802373c. The SSRF changes fail closed before DNS resolution, do not echo URL credentials, align the YAML-backed CLI/plugin rules with MCP special-purpose ranges, and include boundary, parity, and credential-redaction tests. Security categories: PASS for secrets, input validation, authentication/authorization, dependencies, error handling/logging, cryptography, configuration, security tests, and system security. I found no vulnerability in the changed validation logic.
Contributor compliance blocks this PR. GitHub reports commits 280840632, cd058dcf9, and a7802373c as unsigned rather than Verified. Maintainers must not repair contributor signature history. Replace the branch with compliant Verified commits; if published history cannot be replaced, open a new branch and PR. The replacement must also include a documentation-writer receipt for its final commit and current required CI/E2E evidence. I am not updating this branch from main because that would not correct the three noncompliant commits.
|
The merge gate cannot pass because three commits do not appear as Verified in GitHub. Repository policy requires every contributor commit to appear as Verified. Maintainers cannot amend, sign, or force-push contributor history. Please replace the branch history with contributor-created, verified commits. After that update, maintainers can rerun the required gates and continue review. |
|
Closing in favor of #8321. The replacement preserves Ayush7614 as the author of all three contributor commits and reproduces the same source-and-test patch content, confirmed by the matching stable patch ID The replacement is assigned to @apurvvkumaria and retains the |
4 participants
Summary
Inference SSRF used
private-networks.yamlwhile MCP used a harder-coded denylist, so special-purpose IPs and themetadatahostname that MCP already rejects could still pass endpoint validation. Plugin SSRF also accepted URL userinfo that CLI onboard andinference setreject. This change aligns the shared YAML denylist with MCP and rejects credentialed endpoint URLs in plugin validation.Changes
nemoclaw-blueprint/private-networks.yamlwith MCP-parity blocks: AS112 / AMT / 6to4 anycast IPv4 ranges,::/96, broader2001::/23, AS112 / documentation / SRv6 / site-local IPv6 ranges, and the reservedmetadataname.nemoclaw/src/blueprint/ssrf.tswith the same credentials-in-URL contract used by onboard and inference set.isPrivateHostnameparity coverage intest/mcp-url-target.test.ts.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/pages changed. Change hardens existing private-network and endpoint URL validation; users already cannot point inference at private or credentialed endpoints under CLI paths.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project package-contract test/package-contract/ssrf-parity.test.ts→ 214 passed;npx vitest run test/mcp-url-target.test.ts→ 32 passed;npx vitest run --project plugin nemoclaw/src/blueprint/ssrf.test.ts→ 101 passed; CLI and plugin builds for parity artifacts passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Ayush7614 ayushknj3@gmail.com
Summary by CodeRabbit
metadata/instance.metadata) and ensuredmetadata.example.comremains allowed.