refactor: extract SSRF-safe dial logic into core/network package and replace duplicate implementations in fetch.go and skills_serving.go - #5246
Conversation
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesSSRF protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SSRFSafeDialContext
participant Resolver
participant netDialer
Caller->>SSRFSafeDialContext: dial host and port
SSRFSafeDialContext->>Resolver: resolve hostname
Resolver-->>SSRFSafeDialContext: resolved IP addresses
SSRFSafeDialContext->>SSRFSafeDialContext: validate every IP with IsPublicIP
SSRFSafeDialContext->>netDialer: dial first validated IP and port
netDialer-->>Caller: connection or dial error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 4/5The standalone transport build needs dependency alignment before merging.
Important Files Changed
Reviews (1): Last reviewed commit: "refactor: share SSRF-safe outbound diale..." | Re-trigger Greptile |
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 `@core/network/ssrf.go`:
- Around line 62-68: Update embeddedIPv4 and its callers to accept the active
operator-configured NAT64/Pref64 prefixes, extract the embedded IPv4 address for
those prefixes, and apply the existing SSRF validation to that address before
allowing the request. Preserve handling for 6to4 and the standard NAT64
prefixes, and add a regression test proving a custom prefix targeting a
private/IMDS IPv4 address is rejected.
🪄 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: a0a9eef1-bf00-4ab9-80c8-08e6dc53d4a4
📒 Files selected for processing (5)
core/network/ssrf.gocore/network/ssrf_test.gocore/providers/utils/fetch.gocore/providers/utils/fetch_test.gotransports/bifrost-http/handlers/skills_serving.go
💤 Files with no reviewable changes (1)
- core/providers/utils/fetch_test.go
Merge activity
|
…d replace duplicate implementations in `fetch.go` and `skills_serving.go` (#5246) ## Summary The SSRF protection logic (IP classification and safe dial context) was duplicated across `core/providers/utils/fetch.go` and `transports/bifrost-http/handlers/skills_serving.go`, with the skills handler using a weaker `isPrivateIP` check that missed CGNAT, IPv6 transition addresses, site-local IPv6, broadcast, and interface-local multicast. This PR extracts a single, hardened implementation into `core/network` and replaces both call sites with it. ## Changes - Added `core/network/ssrf.go` with `IsPublicIP` and `SSRFSafeDialContext` as the canonical SSRF-safe dialing primitives. `IsPublicIP` blocks loopback, RFC 1918, CGNAT (RFC 6598), link-local, site-local (RFC 3879), multicast, broadcast, unspecified, and IPv4 addresses smuggled inside IPv6 transition representations (6to4 `2002::/16`, NAT64 `64:ff9b::/96`, NAT64 local-use `64:ff9b:1::/48`). `SSRFSafeDialContext` resolves the host, validates every returned IP against `IsPublicIP`, and dials the first validated IP directly — eliminating the DNS-rebinding TOCTOU window. - Removed the private `isPublicIP`/`embeddedIPv4` functions from `fetch.go` and the weaker `ssrfSafeDialContext`/`isPrivateIP` functions from `skills_serving.go`; both now call `network.SSRFSafeDialContext`. - Moved tests from `core/providers/utils/fetch_test.go` into `core/network/ssrf_test.go`, expanded them to cover the additional blocked ranges (CGNAT, broadcast, interface-local multicast, IPv6 site-local), and added dial-path tests for DNS error propagation, empty resolution, missing port, re-resolution per dial, and the DNS-rebinding TOCTOU guard. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/network/... ./core/providers/utils/... ./transports/bifrost-http/... ``` The `TestSSRFSafeDialContextBlocksLoopbackLiteral` test exercises the exported constructor end-to-end against the real resolver and will fail if the dial gate is not applied. All other tests use a `fakeResolver` and require no network access. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The skills handler previously used `isPrivateIP`, which did not block CGNAT addresses (used as pod IPs on EKS), IPv6 site-local (`fec0::/10`), broadcast (`255.255.255.255`), interface-local multicast, or IPv4 addresses wrapped in 6to4/NAT64 IPv6 representations. An admin-controlled `source_url` or a user-controlled fetch URL could have reached internal infrastructure through any of those gaps. The consolidated `IsPublicIP` closes all of them. The DNS-rebinding TOCTOU fix (dial the already-resolved IP directly) was present in `fetch.go` but absent in the skills handler; it is now enforced in both paths via the shared implementation. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…d replace duplicate implementations in `fetch.go` and `skills_serving.go` (#5246) ## Summary The SSRF protection logic (IP classification and safe dial context) was duplicated across `core/providers/utils/fetch.go` and `transports/bifrost-http/handlers/skills_serving.go`, with the skills handler using a weaker `isPrivateIP` check that missed CGNAT, IPv6 transition addresses, site-local IPv6, broadcast, and interface-local multicast. This PR extracts a single, hardened implementation into `core/network` and replaces both call sites with it. ## Changes - Added `core/network/ssrf.go` with `IsPublicIP` and `SSRFSafeDialContext` as the canonical SSRF-safe dialing primitives. `IsPublicIP` blocks loopback, RFC 1918, CGNAT (RFC 6598), link-local, site-local (RFC 3879), multicast, broadcast, unspecified, and IPv4 addresses smuggled inside IPv6 transition representations (6to4 `2002::/16`, NAT64 `64:ff9b::/96`, NAT64 local-use `64:ff9b:1::/48`). `SSRFSafeDialContext` resolves the host, validates every returned IP against `IsPublicIP`, and dials the first validated IP directly — eliminating the DNS-rebinding TOCTOU window. - Removed the private `isPublicIP`/`embeddedIPv4` functions from `fetch.go` and the weaker `ssrfSafeDialContext`/`isPrivateIP` functions from `skills_serving.go`; both now call `network.SSRFSafeDialContext`. - Moved tests from `core/providers/utils/fetch_test.go` into `core/network/ssrf_test.go`, expanded them to cover the additional blocked ranges (CGNAT, broadcast, interface-local multicast, IPv6 site-local), and added dial-path tests for DNS error propagation, empty resolution, missing port, re-resolution per dial, and the DNS-rebinding TOCTOU guard. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/network/... ./core/providers/utils/... ./transports/bifrost-http/... ``` The `TestSSRFSafeDialContextBlocksLoopbackLiteral` test exercises the exported constructor end-to-end against the real resolver and will fail if the dial gate is not applied. All other tests use a `fakeResolver` and require no network access. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The skills handler previously used `isPrivateIP`, which did not block CGNAT addresses (used as pod IPs on EKS), IPv6 site-local (`fec0::/10`), broadcast (`255.255.255.255`), interface-local multicast, or IPv4 addresses wrapped in 6to4/NAT64 IPv6 representations. An admin-controlled `source_url` or a user-controlled fetch URL could have reached internal infrastructure through any of those gaps. The consolidated `IsPublicIP` closes all of them. The DNS-rebinding TOCTOU fix (dial the already-resolved IP directly) was present in `fetch.go` but absent in the skills handler; it is now enforced in both paths via the shared implementation. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
The SSRF protection logic (IP classification and safe dial context) was duplicated across
core/providers/utils/fetch.goandtransports/bifrost-http/handlers/skills_serving.go, with the skills handler using a weakerisPrivateIPcheck that missed CGNAT, IPv6 transition addresses, site-local IPv6, broadcast, and interface-local multicast. This PR extracts a single, hardened implementation intocore/networkand replaces both call sites with it.Changes
core/network/ssrf.gowithIsPublicIPandSSRFSafeDialContextas the canonical SSRF-safe dialing primitives.IsPublicIPblocks loopback, RFC 1918, CGNAT (RFC 6598), link-local, site-local (RFC 3879), multicast, broadcast, unspecified, and IPv4 addresses smuggled inside IPv6 transition representations (6to42002::/16, NAT6464:ff9b::/96, NAT64 local-use64:ff9b:1::/48).SSRFSafeDialContextresolves the host, validates every returned IP againstIsPublicIP, and dials the first validated IP directly — eliminating the DNS-rebinding TOCTOU window.isPublicIP/embeddedIPv4functions fromfetch.goand the weakerssrfSafeDialContext/isPrivateIPfunctions fromskills_serving.go; both now callnetwork.SSRFSafeDialContext.core/providers/utils/fetch_test.gointocore/network/ssrf_test.go, expanded them to cover the additional blocked ranges (CGNAT, broadcast, interface-local multicast, IPv6 site-local), and added dial-path tests for DNS error propagation, empty resolution, missing port, re-resolution per dial, and the DNS-rebinding TOCTOU guard.Type of change
Affected areas
How to test
go test ./core/network/... ./core/providers/utils/... ./transports/bifrost-http/...The
TestSSRFSafeDialContextBlocksLoopbackLiteraltest exercises the exported constructor end-to-end against the real resolver and will fail if the dial gate is not applied. All other tests use afakeResolverand require no network access.Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The skills handler previously used
isPrivateIP, which did not block CGNAT addresses (used as pod IPs on EKS), IPv6 site-local (fec0::/10), broadcast (255.255.255.255), interface-local multicast, or IPv4 addresses wrapped in 6to4/NAT64 IPv6 representations. An admin-controlledsource_urlor a user-controlled fetch URL could have reached internal infrastructure through any of those gaps. The consolidatedIsPublicIPcloses all of them. The DNS-rebinding TOCTOU fix (dial the already-resolved IP directly) was present infetch.gobut absent in the skills handler; it is now enforced in both paths via the shared implementation.Checklist
docs/contributing/README.mdand followed the guidelines