From 934c6728ee5276dc51bd746a2e1f0d1b846e5c02 Mon Sep 17 00:00:00 2001 From: claw Date: Thu, 14 May 2026 04:49:21 -0700 Subject: [PATCH] fix(#123): address review feedback on SSRF defense - 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. --- .gitea/actions/review/action.yml | 12 ++++++-- gitea/client.go | 9 ++++-- gitea/client_test.go | 50 ++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 0edd547..1f6d201 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -201,13 +201,17 @@ runs: printf '%s\n' \ 'import socket,ipaddress,sys,os' \ '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' \ 'try: rs=socket.getaddrinfo(h,None)' \ '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)' \ 'for _,_,_,_,(a,*_) in rs:' \ ' 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:' \ ' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \ > /tmp/_ssrf_check.py @@ -344,13 +348,17 @@ runs: printf '%s\n' \ 'import socket,ipaddress,sys,os' \ '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' \ 'try: rs=socket.getaddrinfo(h,None)' \ '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)' \ 'for _,_,_,_,(a,*_) in rs:' \ ' 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:' \ ' print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \ > /tmp/_ssrf_check_install.py diff --git a/gitea/client.go b/gitea/client.go index 6639753..e1e4bf4 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -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 // 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 { - transport := &http.Transport{ - DialContext: safeDialContext, - } + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.DialContext = safeDialContext return &http.Client{ Timeout: 30 * time.Second, Transport: transport, diff --git a/gitea/client_test.go b/gitea/client_test.go index 0363084..8de33ab 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -1369,3 +1369,53 @@ func TestSetHTTPClient_NilRestoresSafeTransport(t *testing.T) { 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") + } +}