Merge pull request #184 from spudro228/bug-fix/183-wrong-proxy-resolving

Bug fix/183 wrong proxy resolving

4 files changed, 105 insertions, 4 deletions

diff --git a/service/http/config.go b/service/http/config.go
index ff15e83e..25be205c 100644
--- a/service/http/config.go
+++ b/service/http/config.go
@@ -189,6 +189,14 @@ func (c *Config) IsTrusted(ip string) bool {
 	return false
 }
 
+func (c *Config) IsValid(ip string) bool {
+	i := net.ParseIP(ip)
+	if i == nil {
+		return false
+	}
+	return true
+}
+
 // Valid validates the configuration.
 func (c *Config) Valid() error {
 	if c.Uploads == nil {
diff --git a/service/http/config_test.go b/service/http/config_test.go
index d8b92247..800c87ce 100644
--- a/service/http/config_test.go
+++ b/service/http/config_test.go
@@ -83,6 +83,43 @@ func Test_Trusted_Subnets(t *testing.T) {
 	assert.False(t, cfg.IsTrusted("127.0.0.0.1"))
 }
 
+func TestConfig_IsValid(t *testing.T) {
+
+	cfg := &Config{
+		Address:        ":8080",
+		MaxRequestSize: 1024,
+		Uploads: &UploadsConfig{
+			Dir:    os.TempDir(),
+			Forbid: []string{".go"},
+		},
+		HTTP2: &HTTP2Config{
+			Enabled: true,
+		},
+		TrustedSubnets: []string{"200.1.0.0/16"},
+		Workers: &roadrunner.ServerConfig{
+			Command: "php tests/client.php echo pipes",
+			Relay:   "pipes",
+			Pool: &roadrunner.Config{
+				NumWorkers:      1,
+				AllocateTimeout: time.Second,
+				DestroyTimeout:  time.Second,
+			},
+		},
+	}
+
+	ip6 := "FE80::0202:B3FF:FE1E:8329"
+	ip4 := "127.0.0.1"
+
+	assert.True(t, cfg.IsValid(ip4))
+	assert.True(t, cfg.IsValid(ip6))
+
+	ip4Invalid := "127.0.0.0.1"
+	ip6Invalid := "FE80::0202::B3FF:FE1E:8329" // Can only use :: once in an address
+
+	assert.False(t, cfg.IsValid(ip4Invalid))
+	assert.False(t, cfg.IsValid(ip6Invalid))
+}
+
 func Test_Trusted_Subnets_Err(t *testing.T) {
 	cfg := &Config{
 		Address:        ":8080",
diff --git a/service/http/handler.go b/service/http/handler.go
index 254f5ca6..19179b72 100644
--- a/service/http/handler.go
+++ b/service/http/handler.go
@@ -152,12 +152,17 @@ func (h *Handler) resolveIP(r *Request) {
 	}
 
 	if r.Header.Get("X-Forwarded-For") != "" {
-		for _, addr := range strings.Split(r.Header.Get("X-Forwarded-For"), ",") {
-			addr = strings.TrimSpace(addr)
-			if h.cfg.IsTrusted(addr) {
+		ips := strings.Split(r.Header.Get("X-Forwarded-For"), ",")
+		ipCount := len(ips)
+
+		for i := ipCount - 1; i >= 0; i-- {
+			addr := strings.TrimSpace(ips[i])
+			if h.cfg.IsValid(addr) {
 				r.RemoteAddr = addr
+				return
 			}
 		}
+
 		return
 	}
 
diff --git a/service/http/handler_test.go b/service/http/handler_test.go
index 95077da6..e29b76ac 100644
--- a/service/http/handler_test.go
+++ b/service/http/handler_test.go
@@ -1345,7 +1345,58 @@ func TestHandler_XForwardedFor(t *testing.T) {
 
 	assert.NoError(t, err)
 	assert.Equal(t, 200, r.StatusCode)
-	assert.Equal(t, "200.0.0.1", body)
+	assert.Equal(t, "101.0.0.1", body)
+
+	body, r, err = getHeader("http://127.0.0.1:8177/", map[string]string{
+		"X-Forwarded-For": "100.0.0.1, 200.0.0.1, 101.0.0.1, invalid",
+	})
+
+	assert.NoError(t, err)
+	assert.Equal(t, 200, r.StatusCode)
+	assert.Equal(t, "101.0.0.1", body)
+}
+
+func TestHandler_XForwardedFor_NotTrustedRemoteIp(t *testing.T) {
+	h := &Handler{
+		cfg: &Config{
+			MaxRequestSize: 1024,
+			Uploads: &UploadsConfig{
+				Dir:    os.TempDir(),
+				Forbid: []string{},
+			},
+			TrustedSubnets: []string{
+				"10.0.0.0/8",
+			},
+		},
+		rr: roadrunner.NewServer(&roadrunner.ServerConfig{
+			Command: "php ../../tests/http/client.php ip pipes",
+			Relay:   "pipes",
+			Pool: &roadrunner.Config{
+				NumWorkers:      1,
+				AllocateTimeout: 10000000,
+				DestroyTimeout:  10000000,
+			},
+		}),
+	}
+
+	h.cfg.parseCIDRs()
+
+	assert.NoError(t, h.rr.Start())
+	defer h.rr.Stop()
+
+	hs := &http.Server{Addr: "127.0.0.1:8177", Handler: h}
+	defer hs.Shutdown(context.Background())
+
+	go func() { hs.ListenAndServe() }()
+	time.Sleep(time.Millisecond * 10)
+
+	body, r, err := getHeader("http://127.0.0.1:8177/", map[string]string{
+		"X-Forwarded-For": "100.0.0.1, 200.0.0.1, invalid, 101.0.0.1",
+	})
+
+	assert.NoError(t, err)
+	assert.Equal(t, 200, r.StatusCode)
+	assert.Equal(t, "127.0.0.1", body)
 }
 
 func BenchmarkHandler_Listen_Echo(b *testing.B) {