Tidy: Complete TrustedProxies feature (#2887)

README.md

@@ -2130,11 +2130,17 @@ Gin lets you specify which headers to hold the real client IP (if any),
 as well as specifying which proxies (or direct clients) you trust to
 specify one of these headers.
 
-The `TrustedProxies` slice on your `gin.Engine` specifes network addresses or
-network CIDRs from where clients which their request headers related to client
+Use function `SetTrustedProxies()` on your `gin.Engine` to specify network addresses
+or network CIDRs from where clients which their request headers related to client
 IP can be trusted. They can be IPv4 addresses, IPv4 CIDRs, IPv6 addresses or
 IPv6 CIDRs.
 
+**Attention:** Gin trust all proxies by default if you don't specify a trusted 
+proxy using the function above, **this is NOT safe**. At the same time, if you don't
+use any proxy, you can disable this feature by using `Engine.SetTrustedProxies(nil)`,
+then `Context.ClientIP()` will return the remote address directly to avoid some
+unnecessary computation.
+
 ```go
 import (
 	"fmt"
@@ -2145,7 +2151,7 @@ import (
 func main() {
 
 	router := gin.Default()
-	router.TrustedProxies = []string{"192.168.1.2"}
+	router.SetTrustedProxies([]string{"192.168.1.2"})
 
 	router.GET("/", func(c *gin.Context) {
 		// If the client is 192.168.1.2, use the X-Forwarded-For

context.go

@@ -756,7 +756,7 @@ func (c *Context) ClientIP() string {
 // RemoteIP parses the IP from Request.RemoteAddr, normalizes and returns the IP (without the port).
 // It also checks if the remoteIP is a trusted proxy or not.
 // In order to perform this validation, it will see if the IP is contained within at least one of the CIDR blocks
-// defined in Engine.TrustedProxies
+// defined by Engine.SetTrustedProxies()
 func (c *Context) RemoteIP() (net.IP, bool) {
 	ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr))
 	if err != nil {

context_test.go

@@ -1429,6 +1429,10 @@ func TestContextClientIP(t *testing.T) {
 	c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"}
 	assert.Equal(t, "40.40.40.40", c.ClientIP())
 
+	// Disabled TrustedProxies feature
+	_ = c.engine.SetTrustedProxies(nil)
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
 	// Last proxy is trusted, but the RemoteAddr is not
 	c.engine.TrustedProxies = []string{"30.30.30.30"}
 	resetTrustedCIDRs(c)

gin.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"os"
 	"path"
+	"reflect"
 	"strings"
 	"sync"
 
@@ -25,7 +26,9 @@ var (
 	default405Body = []byte("405 method not allowed")
 )
 
-var defaultAppEngine bool
+var defaultPlatform string
+
+var defaultTrustedCIDRs = []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}} // 0.0.0.0/0
 
 // HandlerFunc defines the handler used by gin middleware as return value.
 type HandlerFunc func(*Context)
@@ -92,26 +95,12 @@ type Engine struct {
 	// List of headers used to obtain the client IP when
 	// `(*gin.Engine).ForwardedByClientIP` is `true` and
 	// `(*gin.Context).Request.RemoteAddr` is matched by at least one of the
-	// network origins of `(*gin.Engine).TrustedProxies`.
+	// network origins of list defined by `(*gin.Engine).SetTrustedProxies()`.
 	RemoteIPHeaders []string
 
-	// List of network origins (IPv4 addresses, IPv4 CIDRs, IPv6 addresses or
-	// IPv6 CIDRs) from which to trust request's headers that contain
-	// alternative client IP when `(*gin.Engine).ForwardedByClientIP` is
-	// `true`.
-	TrustedProxies []string
-
-	// #726 #755 If enabled, it will trust some headers starting with
-	// 'X-AppEngine...' for better integration with that PaaS.
-	AppEngine bool
-
-	// If enabled, the url.RawPath will be used to find parameters.
-	UseRawPath bool
-
-	// If true, the path value will be unescaped.
-	// If UseRawPath is false (by default), the UnescapePathValues effectively is true,
-	// as url.Path gonna be used, which is already unescaped.
-	UnescapePathValues bool
+	// If set to a constant of value gin.Platform*, trusts the headers set by
+	// that platform, for example to determine the client IP
+	TrustedPlatform string
 
 	// Value of 'maxMemory' param that is given to http.Request's ParseMultipartForm
 	// method call.
@@ -132,6 +121,7 @@ type Engine struct {
 	pool             sync.Pool
 	trees            methodTrees
 	maxParams        uint16
+	trustedProxies   []string
 	trustedCIDRs     []*net.IPNet
 }
 
@@ -159,16 +149,16 @@ func New() *Engine {
 		HandleMethodNotAllowed: false,
 		ForwardedByClientIP:    true,
 		RemoteIPHeaders:        []string{"X-Forwarded-For", "X-Real-IP"},
-		TrustedProxies:         []string{"0.0.0.0/0"},
-		AppEngine:              defaultAppEngine,
+		TrustedPlatform:        defaultPlatform,
 		UseRawPath:             false,
 		RemoveExtraSlash:       false,
 		UnescapePathValues:     true,
 		MaxMultipartMemory:     defaultMultipartMemory,
 		trees:                  make(methodTrees, 0, 9),
 		delims:                 render.Delims{Left: "{{", Right: "}}"},
 		secureJSONPrefix:       "while(1);",
-		trustedCIDRs:           []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}},
+		trustedProxies:         []string{"0.0.0.0/0"},
+		trustedCIDRs:           defaultTrustedCIDRs,
 	}
 	engine.RouterGroup.engine = engine
 	engine.pool.New = func() interface{} {
@@ -327,24 +317,24 @@ func iterate(path, method string, routes RoutesInfo, root *node) RoutesInfo {
 func (engine *Engine) Run(addr ...string) (err error) {
 	defer func() { debugPrintError(err) }()
 
-	trustedCIDRs, err := engine.prepareTrustedCIDRs()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
-	engine.trustedCIDRs = trustedCIDRs
+
 	address := resolveAddress(addr)
 	debugPrint("Listening and serving HTTP on %s\n", address)
 	err = http.ListenAndServe(address, engine)
 	return
 }
 
 func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) {
-	if engine.TrustedProxies == nil {
+	if engine.trustedProxies == nil {
 		return nil, nil
 	}
 
-	cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies))
-	for _, trustedProxy := range engine.TrustedProxies {
+	cidr := make([]*net.IPNet, 0, len(engine.trustedProxies))
+	for _, trustedProxy := range engine.trustedProxies {
 		if !strings.Contains(trustedProxy, "/") {
 			ip := parseIP(trustedProxy)
 			if ip == nil {
@@ -367,6 +357,31 @@ func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) {
 	return cidr, nil
 }
 
+// SetTrustedProxies set a list of network origins (IPv4 addresses,
+// IPv4 CIDRs, IPv6 addresses or IPv6 CIDRs) from which to trust
+// request's headers that contain alternative client IP when
+// `(*gin.Engine).ForwardedByClientIP` is `true`. `TrustedProxies`
+// feature is enabled by default, and it also trusts all proxies
+// by default. If you want to disable this feature, use
+// Engine.SetTrustedProxies(nil), then Context.ClientIP() will
+// return the remote address directly.
+func (engine *Engine) SetTrustedProxies(trustedProxies []string) error {
+	engine.trustedProxies = trustedProxies
+	return engine.parseTrustedProxies()
+}
+
+// isUnsafeTrustedProxies compares Engine.trustedCIDRs and defaultTrustedCIDRs, it's not safe if equal (returns true)
+func (engine *Engine) isUnsafeTrustedProxies() bool {
+	return reflect.DeepEqual(engine.trustedCIDRs, defaultTrustedCIDRs)
+}
+
+// parseTrustedProxies parse Engine.trustedProxies to Engine.trustedCIDRs
+func (engine *Engine) parseTrustedProxies() error {
+	trustedCIDRs, err := engine.prepareTrustedCIDRs()
+	engine.trustedCIDRs = trustedCIDRs
+	return err
+}
+
 // parseIP parse a string representation of an IP and returns a net.IP with the
 // minimum byte representation or nil if input is invalid.
 func parseIP(ip string) net.IP {
@@ -388,6 +403,11 @@ func (engine *Engine) RunTLS(addr, certFile, keyFile string) (err error) {
 	debugPrint("Listening and serving HTTPS on %s\n", addr)
 	defer func() { debugPrintError(err) }()
 
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
+	}
+
 	err = http.ListenAndServeTLS(addr, certFile, keyFile, engine)
 	return
 }
@@ -399,6 +419,11 @@ func (engine *Engine) RunUnix(file string) (err error) {
 	debugPrint("Listening and serving HTTP on unix:/%s", file)
 	defer func() { debugPrintError(err) }()
 
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
+	}
+
 	listener, err := net.Listen("unix", file)
 	if err != nil {
 		return
@@ -417,6 +442,11 @@ func (engine *Engine) RunFd(fd int) (err error) {
 	debugPrint("Listening and serving HTTP on fd@%d", fd)
 	defer func() { debugPrintError(err) }()
 
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
+	}
+
 	f := os.NewFile(uintptr(fd), fmt.Sprintf("fd@%d", fd))
 	listener, err := net.FileListener(f)
 	if err != nil {
@@ -432,6 +462,12 @@ func (engine *Engine) RunFd(fd int) (err error) {
 func (engine *Engine) RunListener(listener net.Listener) (err error) {
 	debugPrint("Listening and serving HTTP on listener what's bind with address@%s", listener.Addr())
 	defer func() { debugPrintError(err) }()
+
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
+	}
+
 	err = http.Serve(listener, engine)
 	return
 }

gin_integration_test.go

@@ -76,13 +76,81 @@ func TestRunEmpty(t *testing.T) {
 	testRequest(t, "http://localhost:8080/example")
 }
 
-func TestTrustedCIDRsForRun(t *testing.T) {
+func TestBadTrustedCIDRs(t *testing.T) {
+	router := New()
+	assert.Error(t, router.SetTrustedProxies([]string{"hello/world"}))
+}
+
+/* legacy tests
+func TestBadTrustedCIDRsForRun(t *testing.T) {
 	os.Setenv("PORT", "")
 	router := New()
 	router.TrustedProxies = []string{"hello/world"}
 	assert.Error(t, router.Run(":8080"))
 }
 
+func TestBadTrustedCIDRsForRunUnix(t *testing.T) {
+	router := New()
+	router.TrustedProxies = []string{"hello/world"}
+
+	unixTestSocket := filepath.Join(os.TempDir(), "unix_unit_test")
+
+	defer os.Remove(unixTestSocket)
+
+	go func() {
+		router.GET("/example", func(c *Context) { c.String(http.StatusOK, "it worked") })
+		assert.Error(t, router.RunUnix(unixTestSocket))
+	}()
+	// have to wait for the goroutine to start and run the server
+	// otherwise the main thread will complete
+	time.Sleep(5 * time.Millisecond)
+}
+
+func TestBadTrustedCIDRsForRunFd(t *testing.T) {
+	router := New()
+	router.TrustedProxies = []string{"hello/world"}
+
+	addr, err := net.ResolveTCPAddr("tcp", "localhost:0")
+	assert.NoError(t, err)
+	listener, err := net.ListenTCP("tcp", addr)
+	assert.NoError(t, err)
+	socketFile, err := listener.File()
+	assert.NoError(t, err)
+
+	go func() {
+		router.GET("/example", func(c *Context) { c.String(http.StatusOK, "it worked") })
+		assert.Error(t, router.RunFd(int(socketFile.Fd())))
+	}()
+	// have to wait for the goroutine to start and run the server
+	// otherwise the main thread will complete
+	time.Sleep(5 * time.Millisecond)
+}
+
+func TestBadTrustedCIDRsForRunListener(t *testing.T) {
+	router := New()
+	router.TrustedProxies = []string{"hello/world"}
+
+	addr, err := net.ResolveTCPAddr("tcp", "localhost:0")
+	assert.NoError(t, err)
+	listener, err := net.ListenTCP("tcp", addr)
+	assert.NoError(t, err)
+	go func() {
+		router.GET("/example", func(c *Context) { c.String(http.StatusOK, "it worked") })
+		assert.Error(t, router.RunListener(listener))
+	}()
+	// have to wait for the goroutine to start and run the server
+	// otherwise the main thread will complete
+	time.Sleep(5 * time.Millisecond)
+}
+
+func TestBadTrustedCIDRsForRunTLS(t *testing.T) {
+	os.Setenv("PORT", "")
+	router := New()
+	router.TrustedProxies = []string{"hello/world"}
+	assert.Error(t, router.RunTLS(":8080", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem"))
+}
+*/
+
 func TestRunTLS(t *testing.T) {
 	router := New()
 	go func() {