Skip to content
Merged
6 changes: 3 additions & 3 deletions cli/linter/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,9 @@
},
"max_response_body_size": {
"type": "integer"
},
"xff_depth": {
"type": "integer"
}
}
},
Expand Down Expand Up @@ -1336,9 +1339,6 @@
"secrets": {
"type": ["array", "null"]
},
"enable_http_profiler": {
"type": "boolean"
},
Comment thread
sedkis marked this conversation as resolved.
"liveness_check": {
"type": ["object", "null"],
"additionalProperties": false,
Expand Down
8 changes: 8 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,14 @@ type HttpServerOptionsConfig struct {
// https://tyk.io/docs/api-management/traffic-transformation/#request-size-limits
MaxRequestBodySize int64 `json:"max_request_body_size"`

// XFFDepth controls which position in the X-Forwarded-For chain to use for determining client IP address.
// A value of 0 means using the first IP (default). this is way the Gateway has calculated the client IP historically,
// the most common case, and will be used when this config is not set.
// However, any non-zero value will use that position from the right in the X-Forwarded-For chain.
// This is a security feature to prevent against IP spoofing attacks, and is recommended to be set to a non-zero value.
// A value of 1 means using the last IP, 2 means second to last, and so on.
XFFDepth int `json:"xff_depth"`

// MaxResponseBodySize configures an upper limit for the size of the response body (payload) in bytes.
//
// This limit is currently applied only if the Response Body Transform middleware is enabled.
Expand Down
5 changes: 5 additions & 0 deletions gateway/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ import (
"github.com/TykTechnologies/tyk/internal/model"
"github.com/TykTechnologies/tyk/internal/netutil"
"github.com/TykTechnologies/tyk/internal/service/newrelic"
"github.com/TykTechnologies/tyk/request"
)

var (
Expand Down Expand Up @@ -400,6 +401,10 @@ func (gw *Gateway) setupGlobals() {
checkup.Run(&gwConfig)

gw.SetConfig(gwConfig)

// Initialize the Global function in the request package to access the gateway config
request.Global = gw.GetConfig

gw.dnsCacheManager = dnscache.NewDnsCacheManager(gwConfig.DnsCache.MultipleIPsHandleStrategy)

if gwConfig.DnsCache.Enabled {
Expand Down
37 changes: 32 additions & 5 deletions request/real_ip.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@
"net/http"
"strings"

"github.com/TykTechnologies/tyk/config"
"github.com/TykTechnologies/tyk/header"
)

var Global func() config.Config

Check failure on line 12 in request/real_ip.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The use of a global function variable `request.Global` to access configuration introduces a hidden dependency and tight coupling between the `request` and `gateway` packages. The `request` package's functionality now implicitly depends on the `gateway` package for initialization, which reduces modularity, reusability, and makes the code harder to test and reason about.
Raw output
Refactor to use dependency injection. Pass the required configuration values (like `xffDepth`) as explicit parameters to the `RealIP` function. This makes dependencies clear, improves testability, and decouples the packages.

Check failure on line 13 in request/real_ip.go

View check run for this annotation

probelabs / Visor: quality

architecture Issue

The use of a global function variable (`request.Global`) to access configuration introduces a hidden dependency, complicates testing, and creates a dependency on initialization order. This makes the code harder to maintain and reason about.
Raw output
Refactor to use explicit dependency injection. The `XFFDepth` value should be passed as an argument to functions that need it, such as `RealIP(r *http.Request, xffDepth int)`. This makes dependencies explicit and removes the global state.
// RealIP takes a request object, and returns the real Client IP address.
func RealIP(r *http.Request) string {

Expand All @@ -22,14 +25,38 @@
}

if fw := r.Header.Get(header.XForwardFor); fw != "" {
// X-Forwarded-For has no port
if i := strings.IndexByte(fw, ','); i >= 0 {
fw = fw[:i]
xffs := strings.Split(fw, ",")

Check warning on line 28 in request/real_ip.go

View check run for this annotation

probelabs / Visor: performance

performance Issue

The `RealIP` function's new implementation for parsing the `X-Forwarded-For` header uses `strings.Split` for all cases. This function allocates a new slice and strings for each IP address in the header on every request. This introduces a performance regression compared to the previous, more efficient implementation which used `strings.IndexByte` for the default case of selecting the first IP. For requests with a long chain of proxies in the `X-Forwarded-For` header, this can lead to a noticeable increase in memory allocations and garbage collector pressure on a critical request path.
Raw output
To mitigate the performance impact, optimize for the default case (`XFFDepth` is 0) by reverting to the more performant logic of using `strings.IndexByte` to find the first comma and then slicing the string. This avoids the overhead of `strings.Split` for the most common configuration and prevents a performance regression. The `strings.Split` logic should only be used when `XFFDepth > 0`.

**Example:**
```go
// ...
depth := 0
if Global != nil {
    depth = Global().HttpServerOptions.XFFDepth
}

if fw := r.Header.Get(header.XForwardFor); fw != "" {
    var ip string
    if depth == 0 {
        // Optimized path for default case
        if i := strings.IndexByte(fw, ','); i >= 0 {
            ip = fw[:i]
        } else {
            ip = fw
        }
    } else {
        // Path for new functionality
        xffs := strings.Split(fw, ",")
        if depth > len(xffs) {
            return ""
        }
        ip = xffs[len(xffs)-depth]
    }

    ip = strings.TrimSpace(ip)
    if parsedIP := net.ParseIP(ip); parsedIP != nil {
        return parsedIP.String()
    }
    // If IP is invalid, fall through to use RemoteAddr
}
// ...
```

// If no IPs, return the first IP in the chain
Comment thread
sedkis marked this conversation as resolved.
Outdated
if len(xffs) == 0 {
return ""
}

Check warning on line 34 in request/real_ip.go

View check run for this annotation

probelabs / Visor: quality

logic Issue

The check `if len(xffs) == 0` is unreachable because `strings.Split()` on a non-empty string always returns a slice with at least one element. The associated comment is also misleading.
Raw output
Remove the dead code block and the misleading comment to improve code clarity.
// Get depth from config, default to 0 (first IP in chain)
depth := 0
if Global != nil {
depth = Global().HttpServerOptions.XFFDepth
}

// If depth exceeds available IPs, return empty
if depth > len(xffs) {
return ""
}

// Choose the appropriate IP based on depth
// depth=0 means first IP (leftmost), depth=1 means last IP, depth=2 means second to last, etc.
var ip string
if depth == 0 {
Comment thread
sedkis marked this conversation as resolved.
ip = strings.TrimSpace(xffs[0])
} else {
ip = strings.TrimSpace(xffs[len(xffs)-depth])
}

Check failure on line 53 in request/real_ip.go

View check run for this annotation

probelabs / Visor: security

security Issue

A negative `XFFDepth` configuration value can cause a panic due to an out-of-bounds slice access when calculating the IP address index from the `X-Forwarded-For` header. This allows a misconfiguration to lead to a denial of service, as any request with an `X-Forwarded-For` header will crash the gateway.
Raw output
Add validation to ensure `XFFDepth` is a non-negative integer before it is used to calculate the slice index. If the value is negative, it should be treated as a misconfiguration, and the system should fall back to a safe default (e.g., 0).

Check failure on line 53 in request/real_ip.go

View check run for this annotation

probelabs / Visor: quality

logic Issue

A negative `XFFDepth` value can cause a runtime panic due to an out-of-bounds slice access in `xffs[len(xffs)-depth]`. The code does not validate against negative inputs.
Raw output
Add a check to handle negative `depth` values, treating them as invalid or falling back to the default behavior (depth 0). Also, consider adding a `minimum: 0` constraint to the `xff_depth` property in `cli/linter/schema.json`.

if fwIP := net.ParseIP(fw); fwIP != nil {
return fwIP.String()
// Validate that the IP is properly formatted before returning it
if parsedIP := net.ParseIP(ip); parsedIP != nil {

Check warning on line 56 in request/real_ip.go

View check run for this annotation

probelabs / Visor: architecture

logic Issue

The function is expected to return a bare IP address. However, when an invalid IP is found in the `X-Forwarded-For` header, the logic falls through to return `r.RemoteAddr`, which includes the port (e.g., "192.168.1.1:8080"). This creates an inconsistency in the function's return value, as other paths return only the IP address.
Raw output
Ensure the fallback logic consistently returns only the IP address. Use `net.SplitHostPort` to parse `r.RemoteAddr` and extract the host part before returning it.
return parsedIP.String()
}
// If IP is invalid, fall through to use RemoteAddr

Check warning on line 59 in request/real_ip.go

View check run for this annotation

probelabs / Visor: security

security Issue

The IP address selected from the `X-Forwarded-For` header is parsed using `net.ParseIP`, which does not handle IP addresses that include a port number. If an upstream proxy includes a port (e.g., '1.2.3.4:5678'), parsing will fail, causing the logic to incorrectly fall back to `RemoteAddr`. This can lead to the bypass of IP-based security policies like rate limiting or access control.
Raw output
Before parsing the IP, strip any potential port number from the string. The logic used for the `X-Real-IP` header in the same file can be reused here for consistency and correctness.
}

// From net/http.Request.RemoteAddr:
Expand Down
190 changes: 184 additions & 6 deletions request/real_ip_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"context"
"net/http"
"testing"

"github.com/TykTechnologies/tyk/config"
)

var ipHeaderTests = []struct {
Expand All @@ -22,11 +24,20 @@
}

func TestRealIP(t *testing.T) {
// Initialize the Global function with a mock config that has XFFDepth set to 0 (first IP in chain)
mockConfig := config.Config{}
mockConfig.HttpServerOptions.XFFDepth = 0
Global = func() config.Config {
return mockConfig
}

for _, test := range ipHeaderTests {
t.Log(test.comment)

r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
t.Fatal(err)
}
r.Header.Set(test.key, test.value)
r.RemoteAddr = test.remoteAddr

Expand All @@ -38,7 +49,10 @@
}

t.Log("Context")
r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
t.Fatal(err)
}

ctx := context.WithValue(r.Context(), "remote_addr", "10.0.0.5")
r = r.WithContext(ctx)
Expand All @@ -47,12 +61,52 @@
if ip != "10.0.0.5" {
t.Errorf("\texpected %s got %s", "10.0.0.5", ip)
}

// Test with XFFDepth = 1 (last IP in chain)
t.Log("XFFDepth=1 (last IP)")
mockConfig.HttpServerOptions.XFFDepth = 1

r, err = http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
t.Fatal(err)
}
r.Header.Set("X-Forwarded-For", "10.0.0.3, 10.0.0.2, 10.0.0.1")

ip = RealIP(r)
if ip != "10.0.0.1" {
t.Errorf("\texpected %s got %s", "10.0.0.1", ip)
}

// Test with XFFDepth = 2 (second to last IP in chain)
t.Log("XFFDepth=2 (second to last IP)")
mockConfig.HttpServerOptions.XFFDepth = 2

r, err = http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
t.Fatal(err)
}
r.Header.Set("X-Forwarded-For", "10.0.0.3, 10.0.0.2, 10.0.0.1")

ip = RealIP(r)
if ip != "10.0.0.2" {
t.Errorf("\texpected %s got %s", "10.0.0.2", ip)
}
}

func BenchmarkRealIP_RemoteAddr(b *testing.B) {
b.ReportAllocs()

r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
// Initialize the Global function for benchmark
mockConfig := config.Config{}
mockConfig.HttpServerOptions.XFFDepth = 0
Global = func() config.Config {
return mockConfig
}

r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
b.Fatal(err)
}
r.RemoteAddr = "10.0.1.4:8081"

for n := 0; n < b.N; n++ {
Expand All @@ -66,7 +120,17 @@
func BenchmarkRealIP_ForwardedFor(b *testing.B) {
b.ReportAllocs()

r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
// Initialize the Global function for benchmark
mockConfig := config.Config{}
mockConfig.HttpServerOptions.XFFDepth = 0
Global = func() config.Config {
return mockConfig
}

r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
b.Fatal(err)
}
r.Header.Set("X-Forwarded-For", "10.0.0.3, 10.0.0.2, 10.0.0.1")

for n := 0; n < b.N; n++ {
Expand All @@ -80,7 +144,17 @@
func BenchmarkRealIP_RealIP(b *testing.B) {
b.ReportAllocs()

r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
// Initialize the Global function for benchmark
mockConfig := config.Config{}
mockConfig.HttpServerOptions.XFFDepth = 0
Global = func() config.Config {
return mockConfig
}

r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
b.Fatal(err)
}
r.Header.Set("X-Real-IP", "10.0.0.1")

for n := 0; n < b.N; n++ {
Expand All @@ -94,7 +168,17 @@
func BenchmarkRealIP_Context(b *testing.B) {
b.ReportAllocs()

r, _ := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
// Initialize the Global function for benchmark
mockConfig := config.Config{}
mockConfig.HttpServerOptions.XFFDepth = 0
Global = func() config.Config {
return mockConfig
}

r, err := http.NewRequest(http.MethodGet, "http://abc.com:8080", nil)
if err != nil {
b.Fatal(err)
}
r.Header.Set("X-Real-IP", "10.0.0.1")
ctx := context.WithValue(r.Context(), "remote_addr", "10.0.0.5")
r = r.WithContext(ctx)
Expand All @@ -106,3 +190,97 @@
}
}
}

func TestXFFDepth(t *testing.T) {
// Define test cases for XFFDepth
testCases := []struct {
name string
xffValue string
depth int
expected string
}{
{
name: "Depth 1 (last IP)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 1,
expected: "13.0.0.1",
},
{
name: "Depth 2 (second to last IP)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 2,
expected: "12.0.0.1",
},
{
name: "Depth 3 (third to last IP)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 3,
expected: "11.0.0.1",
},
{
name: "Depth 4 (fourth to last IP)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 4,
expected: "10.0.0.1",
},
{
name: "Depth 5 (exceeds chain length)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 5,
expected: "",
},
{
name: "Depth 0 (first IP)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: 0,
expected: "10.0.0.1",
},
{
name: "Header with spaces",
xffValue: "10.0.0.1, 11.0.0.1, 12.0.0.1, 13.0.0.1",
depth: 2,

Check warning on line 241 in request/real_ip_test.go

View check run for this annotation

probelabs / Visor: quality

logic Issue

The test suite for `XFFDepth` does not include a test case for a negative `depth` value. This omission means the potential for a runtime panic from invalid configuration is not covered by tests.
Raw output
Add a test case to `TestXFFDepth` to verify the system's behavior when `XFFDepth` is negative, ensuring it handles the invalid configuration gracefully (e.g., by falling back to the default).
expected: "12.0.0.1",
},
{
name: "Header with mixed format",
xffValue: "10.0.0.1,11.0.0.1, 12.0.0.1, 13.0.0.1",
depth: 3,
expected: "11.0.0.1",
},
{
name: "Invalid IP at selected depth",
xffValue: "10.0.0.1,invalid-ip,12.0.0.1,13.0.0.1",
depth: 3,
expected: "192.168.1.1", // Should fall back to RemoteAddr
},
}

// Initialize mock config for tests
mockConfig := config.Config{}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Set the XFFDepth for this test case
mockConfig.HttpServerOptions.XFFDepth = tc.depth
Global = func() config.Config {
return mockConfig
}

// Create request with X-Forwarded-For header
r, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
if err != nil {
t.Fatal(err)
}
r.Header.Set("X-Forwarded-For", tc.xffValue)
r.RemoteAddr = "192.168.1.1:8080" // Fallback IP if XFF processing fails

// Get client IP
ip := RealIP(r)

// Check result
if ip != tc.expected {
t.Errorf("Expected IP %q, got %q", tc.expected, ip)
}
})
}
}
Loading