From 8c41ec75db3a10b07fbf2414614468c03cb9272f Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Thu, 19 Dec 2024 11:55:42 -0600 Subject: [PATCH] httpservice: improve validation of proxied URLs (#29600) --- server/public/go.mod | 2 +- server/public/shared/httpservice/client.go | 21 +++- .../public/shared/httpservice/client_test.go | 103 ++++++++++++++++++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/server/public/go.mod b/server/public/go.mod index df606b13d3..81b453b708 100644 --- a/server/public/go.mod +++ b/server/public/go.mod @@ -28,6 +28,7 @@ require ( github.com/tinylib/msgp v1.2.0 github.com/vmihailenco/msgpack/v5 v5.4.1 golang.org/x/crypto v0.25.0 + golang.org/x/net v0.27.0 golang.org/x/oauth2 v0.21.0 golang.org/x/text v0.16.0 golang.org/x/tools v0.23.0 @@ -62,7 +63,6 @@ require ( github.com/wiggin77/srslog v1.0.1 // indirect github.com/xtgo/uuid v0.0.0-20140804021211-a0b114877d4c // indirect golang.org/x/mod v0.19.0 // indirect - golang.org/x/net v0.27.0 // indirect golang.org/x/sync v0.7.0 // indirect golang.org/x/sys v0.22.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240722135656-d784300faade // indirect diff --git a/server/public/shared/httpservice/client.go b/server/public/shared/httpservice/client.go index f55393fdbe..39b7ef3d16 100644 --- a/server/public/shared/httpservice/client.go +++ b/server/public/shared/httpservice/client.go @@ -7,9 +7,14 @@ import ( "context" "crypto/tls" "errors" + "fmt" "net" "net/http" + "net/netip" + "net/url" "time" + + "golang.org/x/net/http/httpproxy" ) const ( @@ -155,6 +160,20 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo } } +func getProxyFn() func(r *http.Request) (*url.URL, error) { + proxyFromEnvFn := httpproxy.FromEnvironment().ProxyFunc() + return func(r *http.Request) (*url.URL, error) { + // TODO: Consider removing this code once MM-61938 is fixed upstream. + if r.URL != nil { + if addr, err := netip.ParseAddr(r.URL.Hostname()); err == nil && addr.Is6() && addr.Zone() != "" { + return nil, fmt.Errorf("invalid IPv6 address in URL: %q", addr.String()) + } + } + + return proxyFromEnvFn(r.URL) + } +} + func NewTransport(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) *MattermostTransport { dialContext := (&net.Dialer{ Timeout: ConnectTimeout, @@ -167,7 +186,7 @@ func NewTransport(enableInsecureConnections bool, allowHost func(host string) bo return &MattermostTransport{ &http.Transport{ - Proxy: http.ProxyFromEnvironment, + Proxy: getProxyFn(), DialContext: dialContext, MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second, diff --git a/server/public/shared/httpservice/client_test.go b/server/public/shared/httpservice/client_test.go index a65ce5f77d..709fc5f5e7 100644 --- a/server/public/shared/httpservice/client_test.go +++ b/server/public/shared/httpservice/client_test.go @@ -110,6 +110,17 @@ func TestHTTPClientWithProxy(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, "proxy", string(body)) + + t.Run("invalid IPv6 address in proxied URL", func(t *testing.T) { + c := NewHTTPClient(NewTransport(true, nil, nil)) + + t.Setenv("HTTP_PROXY", "http://proxy.example.org") + t.Setenv("HTTPS_PROXY", "https://proxy.example.org") + t.Setenv("NO_PROXY", ".example.com") + + _, err := c.Get("http://[fe80::8e87:5021:6f6c:605e%25eth0]") + require.EqualError(t, err, `Get "http://[fe80::8e87:5021:6f6c:605e%25eth0]": invalid IPv6 address in URL: "fe80::8e87:5021:6f6c:605e%eth0"`) + }) } func createProxyServer() *httptest.Server { @@ -261,3 +272,95 @@ func TestSplitHostnames(t *testing.T) { hostnames = strings.FieldsFunc(config, splitFields) require.Equal(t, []string{"127.0.0.1", "localhost", "192.168.1.0"}, hostnames) } + +func TestGetProxyFn(t *testing.T) { + t.Setenv("HTTP_PROXY", "http://proxy.example.org") + t.Setenv("HTTPS_PROXY", "https://proxy.example.org") + t.Setenv("NO_PROXY", ".example.com") + + for _, tc := range []struct { + name string + input string + output string + err string + }{ + { + name: "empty", + }, + { + name: "no proxy", + input: "http://test.example.com", + }, + { + name: "localhost", + input: "http://localhost", + }, + { + name: "hostname", + input: "http://example.org", + output: "http://proxy.example.org", + }, + { + name: "hostname with port", + input: "http://example.org:4545", + output: "http://proxy.example.org", + }, + { + name: "https", + input: "https://example.org", + output: "https://proxy.example.org", + }, + { + name: "ipv4", + input: "http://10.0.0.45", + output: "http://proxy.example.org", + }, + { + name: "ipv4 with port", + input: "http://10.0.0.45:4545", + output: "http://proxy.example.org", + }, + { + name: "ipv6", + input: "http://[fe80::8e87:5021:6f6c:605e]", + output: "http://proxy.example.org", + }, + { + name: "ipv6 with port", + input: "http://[fe80::8e87:5021:6f6c:605e]:4545", + output: "http://proxy.example.org", + }, + { + name: "ipv6 with zone", + input: "http://[fe80::8e87:5021:6f6c:605e%25eth0]", + output: "http://proxy.example.org", + err: `invalid IPv6 address in URL: "fe80::8e87:5021:6f6c:605e%eth0"`, + }, + { + name: "ipv6 with zone and port", + input: "http://[fe80::8e87:5021:6f6c:605e%25eth0]:4545", + output: "http://proxy.example.org", + err: `invalid IPv6 address in URL: "fe80::8e87:5021:6f6c:605e%eth0"`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + inURL, err := url.Parse(tc.input) + require.NoError(t, err) + outURL, err := getProxyFn()(&http.Request{ + URL: inURL, + }) + if tc.err != "" { // error case + require.EqualError(t, err, tc.err) + return + } + + require.NoError(t, err) + if tc.output == "" { // not proxied case + require.Nil(t, outURL) + } else { // proxied case + require.NotNil(t, outURL) + require.Equal(t, tc.output, outURL.String()) + } + }) + } +}