fix(#123): address review feedback on SSRF defense
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m14s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m24s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m14s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m24s
- Clone http.DefaultTransport instead of bare &http.Transport{} to preserve
ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, connection
pooling, and HTTP/2 support (fixes transport regression).
- Add IPv6-mapped IPv4 normalization in action.yml Python SSRF checks to
prevent bypass via ::ffff:10.0.0.1 style AAAA records.
- Reject URLs with user-info (user:pass@host) in action.yml Python checks
to match validate-url subcommand behavior.
- Add test verifying DefaultTransport settings are preserved.
This commit is contained in:
@@ -201,13 +201,17 @@ runs:
|
|||||||
printf '%s\n' \
|
printf '%s\n' \
|
||||||
'import socket,ipaddress,sys,os' \
|
'import socket,ipaddress,sys,os' \
|
||||||
'from urllib.parse import urlparse' \
|
'from urllib.parse import urlparse' \
|
||||||
'u=os.environ["CHECK_URL"]; h=urlparse(u).hostname' \
|
'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \
|
||||||
|
'if parsed.username or parsed.password:' \
|
||||||
|
' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \
|
||||||
|
'h=parsed.hostname' \
|
||||||
'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
|
'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
|
||||||
'try: rs=socket.getaddrinfo(h,None)' \
|
'try: rs=socket.getaddrinfo(h,None)' \
|
||||||
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
||||||
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
||||||
'for _,_,_,_,(a,*_) in rs:' \
|
'for _,_,_,_,(a,*_) in rs:' \
|
||||||
' ip=ipaddress.ip_address(a)' \
|
' ip=ipaddress.ip_address(a)' \
|
||||||
|
' if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
|
||||||
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
||||||
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
||||||
> /tmp/_ssrf_check.py
|
> /tmp/_ssrf_check.py
|
||||||
@@ -344,13 +348,17 @@ runs:
|
|||||||
printf '%s\n' \
|
printf '%s\n' \
|
||||||
'import socket,ipaddress,sys,os' \
|
'import socket,ipaddress,sys,os' \
|
||||||
'from urllib.parse import urlparse' \
|
'from urllib.parse import urlparse' \
|
||||||
'u=os.environ["CHECK_URL"]; h=urlparse(u).hostname' \
|
'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \
|
||||||
|
'if parsed.username or parsed.password:' \
|
||||||
|
' print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \
|
||||||
|
'h=parsed.hostname' \
|
||||||
'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
|
'(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
|
||||||
'try: rs=socket.getaddrinfo(h,None)' \
|
'try: rs=socket.getaddrinfo(h,None)' \
|
||||||
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
|
||||||
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
|
||||||
'for _,_,_,_,(a,*_) in rs:' \
|
'for _,_,_,_,(a,*_) in rs:' \
|
||||||
' ip=ipaddress.ip_address(a)' \
|
' ip=ipaddress.ip_address(a)' \
|
||||||
|
' if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
|
||||||
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
' if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved:' \
|
||||||
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
|
||||||
> /tmp/_ssrf_check_install.py
|
> /tmp/_ssrf_check_install.py
|
||||||
|
|||||||
+6
-3
@@ -157,10 +157,13 @@ func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error
|
|||||||
|
|
||||||
// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext
|
// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext
|
||||||
// transport and the cross-host redirect rejection policy.
|
// transport and the cross-host redirect rejection policy.
|
||||||
|
//
|
||||||
|
// We clone http.DefaultTransport to preserve its production-ready defaults
|
||||||
|
// (ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, connection
|
||||||
|
// pooling, HTTP/2 support) and override only DialContext with safeDialContext.
|
||||||
func newSafeHTTPClient() *http.Client {
|
func newSafeHTTPClient() *http.Client {
|
||||||
transport := &http.Transport{
|
transport := http.DefaultTransport.(*http.Transport).Clone()
|
||||||
DialContext: safeDialContext,
|
transport.DialContext = safeDialContext
|
||||||
}
|
|
||||||
return &http.Client{
|
return &http.Client{
|
||||||
Timeout: 30 * time.Second,
|
Timeout: 30 * time.Second,
|
||||||
Transport: transport,
|
Transport: transport,
|
||||||
|
|||||||
@@ -1369,3 +1369,53 @@ func TestSetHTTPClient_NilRestoresSafeTransport(t *testing.T) {
|
|||||||
t.Fatal("expected DialContext to be restored after SetHTTPClient(nil)")
|
t.Fatal("expected DialContext to be restored after SetHTTPClient(nil)")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestNewSafeHTTPClient_PreservesDefaultTransportSettings verifies that
|
||||||
|
// newSafeHTTPClient clones http.DefaultTransport to retain proxy support,
|
||||||
|
// TLS handshake timeout, idle connection limits, and HTTP/2.
|
||||||
|
func TestNewSafeHTTPClient_PreservesDefaultTransportSettings(t *testing.T) {
|
||||||
|
c := NewClient("https://gitea.example.com", "token")
|
||||||
|
transport, ok := c.http.Transport.(*http.Transport)
|
||||||
|
if !ok {
|
||||||
|
t.Fatalf("expected *http.Transport, got %T", c.http.Transport)
|
||||||
|
}
|
||||||
|
|
||||||
|
defaults := http.DefaultTransport.(*http.Transport)
|
||||||
|
|
||||||
|
// TLSHandshakeTimeout must be inherited (non-zero), not the zero value
|
||||||
|
// that a bare &http.Transport{} would have.
|
||||||
|
if transport.TLSHandshakeTimeout == 0 {
|
||||||
|
t.Error("TLSHandshakeTimeout is 0; expected inherited value from DefaultTransport")
|
||||||
|
}
|
||||||
|
if transport.TLSHandshakeTimeout != defaults.TLSHandshakeTimeout {
|
||||||
|
t.Errorf("TLSHandshakeTimeout = %v, want %v", transport.TLSHandshakeTimeout, defaults.TLSHandshakeTimeout)
|
||||||
|
}
|
||||||
|
|
||||||
|
// IdleConnTimeout must be inherited.
|
||||||
|
if transport.IdleConnTimeout == 0 {
|
||||||
|
t.Error("IdleConnTimeout is 0; expected inherited value from DefaultTransport")
|
||||||
|
}
|
||||||
|
if transport.IdleConnTimeout != defaults.IdleConnTimeout {
|
||||||
|
t.Errorf("IdleConnTimeout = %v, want %v", transport.IdleConnTimeout, defaults.IdleConnTimeout)
|
||||||
|
}
|
||||||
|
|
||||||
|
// MaxIdleConns must be inherited.
|
||||||
|
if transport.MaxIdleConns == 0 {
|
||||||
|
t.Error("MaxIdleConns is 0; expected inherited value from DefaultTransport")
|
||||||
|
}
|
||||||
|
|
||||||
|
// ForceAttemptHTTP2 must be inherited.
|
||||||
|
if !transport.ForceAttemptHTTP2 {
|
||||||
|
t.Error("ForceAttemptHTTP2 is false; expected true from DefaultTransport")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Proxy must be set (ProxyFromEnvironment).
|
||||||
|
if transport.Proxy == nil {
|
||||||
|
t.Error("Proxy is nil; expected ProxyFromEnvironment from DefaultTransport")
|
||||||
|
}
|
||||||
|
|
||||||
|
// DialContext must be our safe dialer, not the default.
|
||||||
|
if transport.DialContext == nil {
|
||||||
|
t.Error("DialContext is nil; expected safeDialContext")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user