🛡️ Sentinel: Mitigate SSRF in user-supplied content fetching #303
Conversation
Co-authored-by: Laisky <4532436+Laisky@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughAdds SSRF protection for user-supplied content by adding a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPClient
participant Transport
participant DNS
participant Network
Client->>HTTPClient: make request to user URL
HTTPClient->>Transport: prepare connection (DialContext)
Transport->>DNS: resolve host (unless proxy configured)
DNS-->>Transport: return IP(s)
Transport->>Transport: Control: IsInternalIP? / check proxy exemption
alt IP is internal and not exempt
Transport-->>HTTPClient: return error (deny connection)
HTTPClient-->>Client: surface SSRF-block error
else allowed
Transport->>Network: establish TCP connection (or connect to proxy)
Network-->>Transport: connection established
Transport-->>HTTPClient: connection ready
HTTPClient-->>Client: return response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Fix all issues with AI agents
In `@common/client/init.go`:
- Around line 72-76: The fallback branch that constructs
UserContentRequestHTTPClient currently hardcodes Timeout to 30 * time.Second
instead of using the configured value; update the else branch that creates
UserContentRequestHTTPClient (which calls createTransport) to set Timeout to
config.UserContentRequestTimeout (the same config used in the proxy branch) so
the configured timeout is honored when UserContentRequestProxy is empty.
- Around line 38-50: The Control dialer closure currently returns a bare error
from net.SplitHostPort and allows connections when net.ParseIP(host) yields nil;
change it so net.SplitHostPort errors are wrapped using
github.com/Laisky/errors/v2 (e.g., errors.Wrapf(err, "split host: %s", address))
and if net.ParseIP(host) == nil return a wrapped error (e.g.,
errors.Errorf("SSRF protection: could not parse IP from host %q", host)) to
fail-closed, and replace the fmt.Errorf call with errors.Errorf when blocking
internal IPs (referencing dialer.Control, net.SplitHostPort, net.ParseIP, and
network.IsInternalIP).
🧹 Nitpick comments (1)
common/client/init_test.go (1)
32-43: Test relies on implicit default config; consider making it explicit and adding more cases.The test depends on
config.BlockInternalUserContentRequestsdefaulting totrue. Explicitly setting it beforeInit()makes the test self-documenting and resilient to execution order. Also consider adding cases for:
BlockInternalUserContentRequests = false(should succeed/not block)- Private IPs like
10.0.0.1,192.168.1.1- DNS names that resolve to internal IPs (the real SSRF vector via DNS rebinding)
♻️ Proposed improvement
func TestUserContentRequestHTTPClient_SSRF(t *testing.T) { - // Test that UserContentRequestHTTPClient blocks internal IPs - Init() + t.Run("blocks loopback when enabled", func(t *testing.T) { + config.BlockInternalUserContentRequests = true + Init() - // Try to fetch from localhost (which is an internal IP) - // We use a random port that is likely not listening to avoid connection refused - // but the DialControl should block it before it even tries to connect. - _, err := UserContentRequestHTTPClient.Get("http://127.0.0.1:12345") - require.Error(t, err) - require.Contains(t, err.Error(), "SSRF protection") - require.Contains(t, err.Error(), "blocked") + _, err := UserContentRequestHTTPClient.Get("http://127.0.0.1:12345") + require.Error(t, err) + require.Contains(t, err.Error(), "SSRF protection") + require.Contains(t, err.Error(), "blocked") + }) + + t.Run("blocks private IP when enabled", func(t *testing.T) { + config.BlockInternalUserContentRequests = true + Init() + + _, err := UserContentRequestHTTPClient.Get("http://10.0.0.1:12345") + require.Error(t, err) + require.Contains(t, err.Error(), "SSRF protection") + }) + + t.Run("allows internal when disabled", func(t *testing.T) { + config.BlockInternalUserContentRequests = false + Init() + + // Should fail with connection refused, NOT SSRF protection + _, err := UserContentRequestHTTPClient.Get("http://127.0.0.1:12345") + require.Error(t, err) + require.NotContains(t, err.Error(), "SSRF protection") + }) }
There was a problem hiding this comment.
Pull request overview
This PR adds SSRF defenses to the “user-supplied content” outbound HTTP client path, aiming to prevent fetching user-controlled URLs that resolve to internal network addresses (e.g., localhost/private IPs) when downloading assets like images.
Changes:
- Added
network.IsInternalIPhelper for identifying internal/private-style IPs. - Added
config.BlockInternalUserContentRequeststoggle (default enabled) and wired SSRF blocking intoUserContentRequestHTTPClientvia a custom dialer. - Added a unit test asserting localhost access is blocked, plus a Sentinel knowledge-base entry documenting the SSRF mitigation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| common/network/ip.go | Adds IsInternalIP(net.IP) helper used by the SSRF protection logic. |
| common/config/config.go | Introduces BLOCK_INTERNAL_USER_CONTENT_REQUESTS configuration toggle (default true). |
| common/client/init.go | Implements internal-IP blocking by hooking a custom dialer into the user-content HTTP transport. |
| common/client/init_test.go | Adds a test intended to verify SSRF blocking behavior. |
| .jules/sentinel.md | Documents the SSRF vulnerability class and prevention strategy. |
| dialer.Control = func(networkName, address string, c syscall.RawConn) error { | ||
| host, _, err := net.SplitHostPort(address) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ip := net.ParseIP(host) | ||
| if ip != nil && network.IsInternalIP(ip) { | ||
| return fmt.Errorf("SSRF protection: internal IP %s is blocked", ip) | ||
| } |
There was a problem hiding this comment.
The SSRF guard only blocks when the dial target host is a literal IP (net.ParseIP(host) != nil). Requests to hostnames like localhost, *.internal, or DNS rebinding domains will bypass this check because the dialer will resolve them later. Consider resolving host via net.Resolver.LookupIPAddr (and blocking if any resolved IP is internal), or performing the check earlier at the request layer using req.URL.Hostname() (including during redirects).
common/client/init.go
Outdated
| UserContentRequestHTTPClient = &http.Client{ | ||
| Transport: createTransport(nil), | ||
| Transport: createTransport(nil, config.BlockInternalUserContentRequests), | ||
| Timeout: 30 * time.Second, // Set a reasonable default timeout |
There was a problem hiding this comment.
When USER_CONTENT_REQUEST_PROXY is set, the transport dials only the proxy address, so this internal-IP block will (a) not prevent SSRF to internal targets via the proxy, and (b) potentially break legitimate setups where the proxy itself is on localhost/private IP (common for egress proxies). Recommend skipping the block for proxy connections and instead validating the request destination hostname/IPs before issuing the request (and on redirects), or explicitly exempting the configured proxy host from the internal-IP check.
| func TestUserContentRequestHTTPClient_SSRF(t *testing.T) { | ||
| // Test that UserContentRequestHTTPClient blocks internal IPs | ||
| Init() | ||
|
|
||
| // Try to fetch from localhost (which is an internal IP) | ||
| // We use a random port that is likely not listening to avoid connection refused | ||
| // but the DialControl should block it before it even tries to connect. | ||
| _, err := UserContentRequestHTTPClient.Get("http://127.0.0.1:12345") | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "SSRF protection") | ||
| require.Contains(t, err.Error(), "blocked") | ||
| } |
There was a problem hiding this comment.
This test relies on the process environment defaulting BLOCK_INTERNAL_USER_CONTENT_REQUESTS=true; if CI or another test sets it false, this will fail. It also only tests a literal IP, not the hostname path (http://localhost:...) that currently bypasses the new dialer check. Suggest explicitly setting/restoring config.BlockInternalUserContentRequests within the test and adding a localhost case (or other hostname resolving to loopback) so the test matches the intended protection.
This update enhances the SSRF protection for user-supplied content fetching by implementing a fail-closed policy and standardized error handling. - Updated `Control` function to block unparseable IP addresses. - Implemented error wrapping using `github.com/Laisky/errors/v2` per guidelines. - Ensured all security-related errors provide consistent context. - Verified protection with unit tests. This ensures that any address that cannot be explicitly validated as an external IP is blocked, providing more robust protection against SSRF. Co-authored-by: Laisky <4532436+Laisky@users.noreply.github.com>
This change ensures that the `UserContentRequestHTTPClient` uses the configured `UserContentRequestTimeout` in both proxy and non-proxy initialization paths. - Updated `common/client/init.go` to use `config.UserContentRequestTimeout` consistently. - Ensured SSRF protection remains intact. - Verified with unit tests. Co-authored-by: Laisky <4532436+Laisky@users.noreply.github.com>
…S rebinding mitigation This update addresses feedback regarding DNS rebinding and proxy interaction. - SSRF protection now uses `net.Dialer.Control` which intercepts the actual IP addresses after DNS resolution, effectively mitigating DNS rebinding. - Explicitly configured proxies are now exempted from the internal IP check in `dialer.Control` to allow legitimate local egress proxies. - Proxy hostname resolution is performed once at initialization to minimize latency in the dial path. - Unit tests were expanded to cover hostname resolution (localhost), proxy exemptions, and explicit configuration control. - Maintained fail-closed logic and standardized error wrapping. This provides a robust, production-ready SSRF mitigation for fetching user-supplied content. Co-authored-by: Laisky <4532436+Laisky@users.noreply.github.com>
Merging this branch will not change overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@common/client/init.go`:
- Around line 39-48: The code currently appends the raw proxy hostname to
proxyIPs which will never match the resolved IP passed into dialer.Control;
change init logic around proxyURL/proxyHost so you do not add the raw hostname
to proxyIPs, instead perform net.LookupIP(proxyHost) and if it succeeds append
the returned IP strings to proxyIPs, and if it fails either log the lookup error
and return/fail initialization (treat like an invalid proxy URL) so the SSRF
check won't block the proxy; update references in the init routine that populate
proxyIPs (using proxyURL, proxyHost, proxyIPs) and ensure dialer.Control uses
those resolved IP entries.
| var proxyIPs []string | ||
| if proxyURL != nil { | ||
| proxyHost := proxyURL.Hostname() | ||
| proxyIPs = append(proxyIPs, proxyHost) | ||
| if ips, err := net.LookupIP(proxyHost); err == nil { | ||
| for _, ip := range ips { | ||
| proxyIPs = append(proxyIPs, ip.String()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Raw proxy hostname in proxyIPs won't match in Control.
dialer.Control receives the resolved IP (not the original hostname) from the dialer. Line 42 appends the raw proxyHost string (e.g. "my-proxy.internal"), but this will never match the host variable in the Control callback since host is always an IP after resolution. This entry is harmless but dead — only the IPs added on lines 44-45 are effective.
If net.LookupIP fails (line 43), no resolved IPs are added, and the only entry in proxyIPs is the unresolvable hostname — meaning connections to the proxy will be blocked by the SSRF check. Consider logging the lookup failure or treating it as a fatal init error, similar to how an invalid proxy URL is handled on line 90.
Suggested improvement
if proxyURL != nil {
proxyHost := proxyURL.Hostname()
- proxyIPs = append(proxyIPs, proxyHost)
- if ips, err := net.LookupIP(proxyHost); err == nil {
+ // If proxyHost is already an IP, add it directly.
+ if pip := net.ParseIP(proxyHost); pip != nil {
+ proxyIPs = append(proxyIPs, pip.String())
+ } else if ips, err := net.LookupIP(proxyHost); err == nil {
for _, ip := range ips {
proxyIPs = append(proxyIPs, ip.String())
}
+ } else {
+ logger.Logger.Fatal("failed to resolve proxy hostname for SSRF exemption",
+ zap.String("proxy", proxyHost), zap.Error(err))
}
}🤖 Prompt for AI Agents
In `@common/client/init.go` around lines 39 - 48, The code currently appends the
raw proxy hostname to proxyIPs which will never match the resolved IP passed
into dialer.Control; change init logic around proxyURL/proxyHost so you do not
add the raw hostname to proxyIPs, instead perform net.LookupIP(proxyHost) and if
it succeeds append the returned IP strings to proxyIPs, and if it fails either
log the lookup error and return/fail initialization (treat like an invalid proxy
URL) so the SSRF check won't block the proxy; update references in the init
routine that populate proxyIPs (using proxyURL, proxyHost, proxyIPs) and ensure
dialer.Control uses those resolved IP entries.
1 participant
This change implements SSRF protection for the
UserContentRequestHTTPClientby blocking requests to internal network addresses (localhost, private IPs).IsInternalIPtocommon/network/ip.go.common/client/init.goto use a customDialContextwith IP blocking.BlockInternalUserContentRequestsconfiguration tocommon/config/config.go.This prevents attackers from using One API to scan or access internal services via user-supplied URLs (e.g. in multimodal image messages).
PR created automatically by Jules for task 4650579899972121216 started by @Laisky
Summary by CodeRabbit
New Features
Documentation
Tests