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 @@ -1333,12 +1336,9 @@
}
}
},
"secrets": {
"type": ["array", "null"]
},

Check notice on line 1341 in cli/linter/schema.json

View check run for this annotation

probelabs / Visor: quality

style Issue

The removal of the `enable_http_profiler` property from the JSON schema appears to be unrelated to the primary purpose of this pull request, which is to add support for `xff_depth`. Including unrelated changes in a PR makes the change history harder to follow and reviews more complex.
Raw output
It is recommended to submit unrelated changes, such as configuration cleanup, in a separate pull request with a clear title and description explaining the change.
"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 @@ -571,6 +571,14 @@
// 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.

Check failure on line 577 in config/config.go

View check run for this annotation

probelabs / Visor: dependency

architecture Issue

The new configuration option `http_server_options.xff_depth` is not exposed in the `tyk-operator` CRDs. Users managing Tyk Gateway configurations via the operator will not be able to set this value.
Raw output
Update the `tyk-operator` repository to expose the `xff_depth` setting in the appropriate CRD (e.g., `TykGateway`) and ensure it's propagated to the gateway's configuration.

Check failure on line 577 in config/config.go

View check run for this annotation

probelabs / Visor: dependency

architecture Issue

The new configuration option `http_server_options.xff_depth` is not exposed in the `tyk-charts` Helm chart. Users deploying Tyk via Helm will be unable to configure this security feature.
Raw output
Update the `tyk-charts` repository to add `http_server_options.xff_depth` to the gateway's `values.yaml` and corresponding configuration templates.
// 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
36 changes: 31 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 provide configuration to the `RealIP` function introduces a hidden dependency and tight coupling between the `request` and `gateway` packages. This pattern makes the `request` package difficult to use and test in isolation, as it relies on an external package (`gateway`) to initialize its state correctly. Global variables can also lead to issues with initialization order and race conditions in concurrent applications.
Raw output
To improve decoupling and make dependencies explicit, pass the required configuration via the request's context. A top-level middleware can inject the configuration into the `http.Request` context, and `request.RealIP` can then retrieve it from there. This approach avoids global state, aligns with idiomatic Go practices for handling request-scoped data, and makes the function's dependencies clear.

Check warning 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 `Global` to access configuration introduces a hidden dependency from the `request` package to the `gateway` package for initialization. This can complicate testing, introduce potential race conditions if not initialized properly, and make the code harder to reason about. While it is managed in tests within this PR, this pattern can be detrimental to long-term maintainability.
Raw output
Consider using dependency injection to pass the configuration to the `RealIP` function or the component that uses it. For example, `RealIP(r *http.Request, conf config.Config) string`. This makes dependencies explicit and improves testability.
// RealIP takes a request object, and returns the real Client IP address.
func RealIP(r *http.Request) string {

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

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, ",")

// Get depth from config, default to 0 (first IP in chain)
var depth int
if Global != nil {
depth = Global().HttpServerOptions.XFFDepth
}

// The following check for invalid depth configs.
// It's more secure to return empty if depth is invalid.
// Defaulting to an IP from the request would be
// burying a configuration failure.
if depth < 0 || depth > len(xffs) {
return ""
}

// Choose the appropriate IP based on depth

Check warning on line 44 in request/real_ip.go

View check run for this annotation

probelabs / Visor: security

security Issue

The function returns an empty string `""` when the configured `XFFDepth` is invalid for a given request's `X-Forwarded-For` header (e.g., depth is negative or exceeds the number of proxies). This "fail-safe" behavior is intentional to expose misconfigurations, but it introduces a new return value that bypasses the `RemoteAddr` fallback. If downstream components like rate limiters or logging are not designed to handle an empty IP address, this could lead to unexpected behavior. For example, a rate limiter might group all such requests into a single bucket, creating a potential denial-of-service vector where one client can exhaust the quota for many.
Raw output
Verify that all consumers of the `RealIP` function are robust against receiving an empty string. At a minimum, a warning should be logged when this condition occurs to aid operators in diagnosing configuration issues that could affect security policies.
// depth=0 means first IP (leftmost), depth=1 means last IP, depth=2 means second to last, etc.
// Negative depth is invalid and treated same as 0/unset.
var ip string
if depth == 0 {
ip = strings.TrimSpace(xffs[0])

Check warning on line 49 in request/real_ip.go

View check run for this annotation

probelabs / Visor: performance

performance Issue

The `RealIP` function unconditionally uses `strings.Split` on the `X-Forwarded-For` header. This introduces a performance regression for the default and most common configuration (`XFFDepth: 0`) compared to the previous implementation, which was more efficient. The unnecessary memory allocations on every request for the default path can increase garbage collection pressure in a high-traffic gateway.
Raw output
Optimize the function by creating a fast path for the default case (`depth == 0`) that avoids `strings.Split`. The original logic of finding the first comma and slicing the string should be restored for this common path. The `strings.Split` operation should only be performed when a non-zero `depth` is configured.
} else {
ip = strings.TrimSpace(xffs[len(xffs)-depth])

Check warning on line 51 in request/real_ip.go

View check run for this annotation

probelabs / Visor: quality

documentation Issue

The comment `// Negative depth is invalid and treated same as 0/unset.` is incorrect. The code handles negative depth by returning an empty string, whereas a depth of 0 (the default for unset) results in selecting the first IP from the `X-Forwarded-For` header. This discrepancy can mislead developers and cause incorrect assumptions about the function's behavior.
Raw output
Update the comment to accurately reflect the implementation. For example: `// Negative depth is invalid and will cause an empty string to be returned.` A similar misleading comment exists in `request/real_ip_test.go` at line 199 and should also be corrected.
}

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 {
return parsedIP.String()
}
// If IP is invalid, fall through to use RemoteAddr
}

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

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

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

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 @@ func TestRealIP(t *testing.T) {
}

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 @@ func TestRealIP(t *testing.T) {
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_RemoteAddr(b *testing.B) {
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_ForwardedFor(b *testing.B) {
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_RealIP(b *testing.B) {
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,109 @@ func BenchmarkRealIP_Context(b *testing.B) {
}
}
}

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: "Depth -5 (Negative Depth uses same as NO depth)",
xffValue: "10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1",
depth: -5,
expected: "",
},
{
name: "Header with spaces",
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: "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: "Empty header",
xffValue: "",
depth: 0,
expected: "192.168.1.1", // Should fall back to RemoteAddr
},
{
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