feat(#123): add IP-level SSRF defense to Gitea client and action #129
@@ -135,12 +135,24 @@ func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error
|
||||
return nil, fmt.Errorf("safeDialContext: blocked: %q resolves to private/reserved IP %s", host, a.IP)
|
||||
}
|
||||
}
|
||||
// Dial the first resolved IP directly to avoid a second lookup.
|
||||
// Try each resolved IP in order, returning the first successful connection.
|
||||
// Fallback is important when a hostname resolves to multiple IPs and the first
|
||||
|
|
||||
// is temporarily unreachable. All IPs were already validated above, so dialing
|
||||
// any of them is safe.
|
||||
//
|
||||
// Timeout: 10s per the design (PLAN.md); the outer http.Client has a 30s
|
||||
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `safeDialContext` creates a new `net.Dialer{}` on every call with no timeout configured. The outer `http.Client` has a 30s timeout, but the dial itself has no deadline beyond the context. In the original code (pre-PR), there was no DialContext either, so this is not a regression. However, the PLAN.md design snippet shows `d := &net.Dialer{Timeout: 10 * time.Second}` with an explicit timeout, but the implementation uses `&net.Dialer{}` (no timeout). This deviation from the design is minor since the context propagates cancellation, but it could allow a slow TCP connect to consume the full 30s.
[MAJOR] Safe dialer does not protect against proxy-assisted SSRF. newSafeHTTPClient clones http.DefaultTransport (retaining ProxyFromEnvironment). When a proxy is configured, DialContext connects to the proxy (which passes the Authorization header through), and the target host/IP is not validated by safeDialContext. An attacker could point baseURL to a private/reserved IP (e.g., 169.254.169.254) and rely on an environment proxy to reach it, bypassing the IP block. **[MAJOR]** Safe dialer does not protect against proxy-assisted SSRF. newSafeHTTPClient clones http.DefaultTransport (retaining ProxyFromEnvironment). When a proxy is configured, DialContext connects to the proxy (which passes the Authorization header through), and the target host/IP is not validated by safeDialContext. An attacker could point baseURL to a private/reserved IP (e.g., 169.254.169.254) and rely on an environment proxy to reach it, bypassing the IP block.
|
||||
// total timeout, but the dial itself needs an independent bound so a slow
|
||||
// TCP connect does not consume the full 30s without cancellation.
|
||||
// total timeout, but the per-dial timeout ensures a slow TCP connect on one IP
|
||||
// doesn't consume the budget needed to try others.
|
||||
d := &net.Dialer{Timeout: 10 * time.Second}
|
||||
return d.DialContext(ctx, network, net.JoinHostPort(addrs[0].IP.String(), port))
|
||||
var lastErr error
|
||||
for _, a := range addrs {
|
||||
conn, err := d.DialContext(ctx, network, net.JoinHostPort(a.IP.String(), port))
|
||||
if err == nil {
|
||||
return conn, nil
|
||||
}
|
||||
lastErr = err
|
||||
|
sonnet-review-bot
commented
[MINOR] safeDialContext creates a new net.Dialer on every call (one per HTTP connection). This is fine for correctness but allocates a struct per dial. A package-level or transport-level Dialer would be more efficient, though for this workload (non-hot-path API calls) the impact is negligible. **[MINOR]** safeDialContext creates a new net.Dialer on every call (one per HTTP connection). This is fine for correctness but allocates a struct per dial. A package-level or transport-level Dialer would be more efficient, though for this workload (non-hot-path API calls) the impact is negligible.
|
||||
}
|
||||
return nil, fmt.Errorf("safeDialContext: all %d addresses for %q failed, last error: %w", len(addrs), host, lastErr)
|
||||
|
sonnet-review-bot
commented
[MINOR] safeDialContext creates a new net.Dialer on every call (line 155: **[MINOR]** safeDialContext creates a new net.Dialer on every call (line 155: `d := &net.Dialer{Timeout: 10 * time.Second}`). While not a correctness issue, for a function called on every HTTP connection this allocates a new struct each time. The dialer could be a package-level variable or embedded in a struct to avoid repeated allocation.
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] In **[MINOR]** In `safeDialContext`, the comment says 'we dial the first resolved IP directly to narrow DNS rebinding window' but the implementation iterates through ALL resolved IPs with fallback. The code is actually correct (and better than the comment implies — it tries each IP in order), but the comment in the doc is slightly misleading since it says 'Dials first resolved IP directly' (visible in the PR description too). The comment at line 157 inside the function ('Try each resolved IP in order...') is accurate but the package-level doc comment should be updated to match.
|
||||
// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext
|
||||
|
||||
[MAJOR] newSafeHTTPClient constructs a bare http.Transport with only DialContext set. This drops ProxyFromEnvironment and standard transport timeouts (IdleConnTimeout, TLSHandshakeTimeout, ExpectContinueTimeout, MaxIdleConns, ForceAttemptHTTP2) that http.DefaultTransport provides. This can break proxyed environments and lead to suboptimal or leaking connection pools. Prefer cloning http.DefaultTransport and overriding DialContext.
[MAJOR] By not setting Proxy: http.ProxyFromEnvironment on the custom transport, proxy environment variables (HTTP_PROXY/HTTPS_PROXY/NO_PROXY) are ignored. Previous behavior (using default transport) respected proxies. This is a behavioral regression that can prevent the client from working behind corporate proxies.