TT-14891 - adds client ip from XFF by depth by sedkis · Pull Request #7063 · TykTechnologies/tyk

sedkis · 2025-05-20T16:42:30Z

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement, Tests

Description

Add configurable XFF depth for client IP extraction
Update real IP logic to support XFF depth
Extend and add tests for XFF depth scenarios
Document new XFF depth config in code

Changes walkthrough 📝

Relevant files

Enhancement

config.go `Add and document XFFDepth config option` config/config.go Introduced new `XFFDepth` field in `HttpServerOptionsConfig` Documented the security and usage of `XFFDepth`	+8/-0
server.go `Initialize request.Global for config access` gateway/server.go Set up the `request.Global` function to provide gateway config Ensures request package can access runtime config for XFF depth	+5/-0
real_ip.go `Support XFF depth in RealIP extraction` request/real_ip.go Added `Global` config accessor for dynamic config Updated `RealIP` to use XFF depth from config Improved logic to select correct IP from XFF chain Added validation and fallback for invalid IPs	+32/-5

Tests

real_ip_test.go `Add and extend tests for XFF depth logic` request/real_ip_test.go Added tests for various XFF depth scenarios Updated benchmarks to use configurable XFF depth Added edge case and fallback tests for XFF depth Ensured coverage for invalid and mixed-format headers	+151/-0

Need help?
Type /help how to ... in the comments thread for any questions about PR-Agent usage.
Check out the documentation for more information.

Ticket Details

TT-14891


Status	In Code Review
Summary	[Innersource] Dynamic client-IP selection. (XFF Depth Selection Improvement)

Generated at: 2025-11-12 17:18:40

buger · 2025-05-20T16:42:43Z

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status	Open	❌
Allowed Statuses	In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review	✔️

Please ensure your jira story is in one of the allowed statuses

github-actions · 2025-05-20T16:43:21Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Edge Case Handling The logic for selecting the client IP from the X-Forwarded-For header based on XFFDepth may return an empty string if the depth exceeds the number of IPs in the header. This could result in unexpected behavior downstream. Ensure that this is the intended and safe fallback, and that all usages of RealIP properly handle an empty return value. if fw := r.Header.Get(header.XForwardFor); fw != "" { xffs := strings.Split(fw, ",") // If no IPs, return the first IP in the chain if len(xffs) == 0 { return "" } // 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 { ip = strings.TrimSpace(xffs[0]) } else { ip = strings.TrimSpace(xffs[len(xffs)-depth]) } // 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 } Global Config Access Pattern The use of a global function variable (`Global`) to access configuration introduces a hidden dependency and may complicate testing or future refactoring. Consider if this approach is consistent with the rest of the codebase and whether it could introduce concurrency or initialization issues. var Global func() config.Config

github-actions · 2025-05-20T16:43:33Z

API Changes

--- prev.txt	2025-11-12 17:19:36.671171359 +0000
+++ current.txt	2025-11-12 17:19:27.025137028 +0000
@@ -6838,6 +6838,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.
+	// 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.
@@ -12953,6 +12961,10 @@
 package request // import "github.com/TykTechnologies/tyk/request"
 
 
+VARIABLES
+
+var Global func() config.Config
+
 FUNCTIONS
 
 func RealIP(r *http.Request) string

github-actions · 2025-05-20T16:43:39Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent out-of-bounds slice access The check for `depth > len(xffs)` is incorrect and may cause a runtime panic when `depth == len(xffs)`. Since slice indices are zero-based, accessing `xffs[len(xffs)-depth]` when `depth == len(xffs)` will result in an out-of-bounds error. Change the condition to `depth > 0 && depth > len(xffs)` to prevent this. request/real_ip.go [42-44] -if depth > len(xffs) { +if depth > 0 && depth > len(xffs) { return "" } Suggestion importance[1-10]: 8 __ Why: This suggestion addresses a potential runtime panic due to an out-of-bounds slice access when `depth == len(xffs)`. The fix is accurate and important for preventing crashes in production.	Medium
General	Ensure fallback returns only IP part The expected value for fallback should be the IP part of `RemoteAddr`, not the full address with port. Ensure the test expects only the IP portion, or update the fallback logic in `RealIP` to strip the port. request/real_ip_test.go [227-231] { 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 + expected: "192.168.1.1", // Should fall back to RemoteAddr (IP only) }, Suggestion importance[1-10]: 7 __ Why: The suggestion ensures that the test expects only the IP portion (not the port) when falling back to `RemoteAddr`, which matches the intended behavior and prevents false negatives in testing.	Medium
General	Correct misleading comment for clarity The comment and logic are inconsistent: if `len(xffs) == 0`, there are no IPs to return, so returning an empty string is correct, but the comment should not say "return the first IP in the chain." Update the comment to accurately reflect the logic. request/real_ip.go [31-34] -// If no IPs, return the first IP in the chain +// If no IPs, return empty string if len(xffs) == 0 { return "" } Suggestion importance[1-10]: 5 __ Why: The suggestion improves code clarity by correcting a misleading comment, which helps maintainability but does not affect functionality.	Low

…t-ip

probelabs · 2025-10-31T14:24:23Z

🔍 Code Analysis Results

This PR introduces a configurable XFFDepth option, allowing administrators to specify which IP address in the X-Forwarded-For (XFF) header should be treated as the client IP. This enhances security by mitigating IP spoofing risks in environments with one or more trusted proxies.

Files Changed Analysis

The changes are spread across 5 files, with a total of 243 additions and 14 deletions. The most significant change is in request/real_ip_test.go (+196 lines), which adds a comprehensive test suite for the new logic. This indicates a strong focus on correctness and reliability. The core logic change in request/real_ip.go is concise, with the remaining changes enabling the new configuration.

Architecture & Impact Assessment

What this PR accomplishes:
This PR hardens the gateway's client IP detection mechanism. By allowing configuration of which IP to trust in the X-Forwarded-For chain, it enables administrators to prevent clients from spoofing their IP address by simply adding a fake IP to the header.

Key technical changes introduced:

New Configuration: A new xff_depth field is added to the http_server_options configuration block.
IP Selection Logic: The request.RealIP function is updated to parse the XFF header and select an IP based on xff_depth. A value of 0 (default) preserves the old behavior (leftmost IP), while a positive value n selects the nth IP from the right.
Dependency Injection: A global function request.Global is introduced to provide the gateway's configuration to the request package. This is a notable architectural choice to avoid a large refactoring of function signatures that use RealIP.

Affected system components:
The change impacts all system components that rely on request.RealIP for the client's IP address. This includes:

Security: IP-based access control, such as the IP blacklisting middleware (mw_ip_blacklist.go).
Analytics & Logging: Access logs and general request logging (log_helpers.go, accesslog/record.go).
Rate Limiting & Quotas: Policies that are scoped by IP address.
Instrumentation: Metrics that record the client IP (instrumentation_handlers.go).

Client IP Selection Flow:

graph TD
    A[Incoming Request] --> B{Check Context for 'remote_addr'};
    B -- Yes --> C[Use Context IP];
    B -- No --> D{Check X-Real-IP Header};
    D -- Yes --> E[Parse IP];
    E -- Valid --> F[Use X-Real-IP];
    E -- Invalid --> G{Check X-Forwarded-For Header};
    D -- No --> G;
    G -- No --> H[Use RemoteAddr];
    G -- Yes --> I[Split XFF Header into IP list];
    I --> J{Get XFFDepth from Config};
    J -- depth <= 0 --> K[Select first IP];
    J -- depth > 0 --> L[Select nth IP from right];
    K --> M{Is IP valid?};
    L --> M;
    M -- Yes --> N[Use Selected IP];
    M -- No --> H;
    C --> Z[End];
    F --> Z;
    H --> Z;
    N --> Z;

Scope Discovery & Context Expansion

The impact of this change is system-wide, as request.RealIP is the centralized function for determining client IP addresses. The introduction of request.Global couples the low-level request package with the gateway's configuration lifecycle. While this is a pragmatic solution to avoid passing configuration through many layers of middleware, it introduces a global state dependency that reviewers should be aware of, as it can make testing and reasoning about the code more complex. The extensive test suite in request/real_ip_test.go is crucial for mitigating the risks associated with this change.

Metadata

Review Effort: 3 / 5
Primary Label: enhancement

Powered by Visor from Probelabs

Last updated: 2025-11-12T17:21:50.428Z | Triggered by: synchronize | Commit: a3d051d

💡 TIP: You can chat with Visor using /visor ask <your question>

probelabs · 2025-10-31T14:24:24Z

🔍 Code Analysis Results

Security Issues (1)

Severity	Location	Issue
🟡 Warning	`request/real_ip.go:42-44`	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. 💡 Suggestion 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.

Architecture Issues (1)

Severity	Location	Issue
🟠 Error	`request/real_ip.go:12`	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. 💡 Suggestion 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.

Performance Issues (1)

Severity Location Issue

🟡 Warning

request/real_ip.go:29-49

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.

💡 Suggestion

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.

🔧 Suggested Fix

if fw := r.Header.Get(header.XForwardFor); fw != "" {
    var depth int
    if Global != nil {
        depth = Global().HttpServerOptions.XFFDepth
    }
var ip string
if depth == 0 {
    // Optimized path for default case, avoids strings.Split
    if i := strings.IndexByte(fw, &#39;,&#39;); i &gt;= 0 {
        ip = strings.TrimSpace(fw[:i])
    } else {
        ip = strings.TrimSpace(fw)
    }
} else {
    // Slower path for non-default depth
    xffs := strings.Split(fw, &#34;,&#34;)
    if depth &lt; 0 || depth &gt; len(xffs) {
        return &#34;&#34;
    }
    ip = strings.TrimSpace(xffs[len(xffs)-depth])
}

// 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

}

Quality Issues (3)

Severity	Location	Issue
🟢 Info	`cli/linter/schema.json:1339-1341`	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. 💡 Suggestion 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.
🟡 Warning	`request/real_ip.go:13`	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. 💡 Suggestion 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.
🟡 Warning	`request/real_ip.go:50-51`	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. 💡 Suggestion 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.

Dependency Issues (2)

Severity	Location	Issue
🟠 Error	`config/config.go:577`	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. 💡 Suggestion 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.
🟠 Error	`config/config.go:577`	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. 💡 Suggestion Update the `tyk-charts` repository to add `http_server_options.xff_depth` to the gateway's `values.yaml` and corresponding configuration templates.

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.

Powered by Visor from Probelabs

Last updated: 2025-11-12T17:21:51.815Z | Triggered by: synchronize | Commit: a3d051d

💡 TIP: You can chat with Visor using /visor ask <your question>

…t-ip

…into TT-14891/dynamic-xff-client-ip

github-actions · 2025-11-11T14:18:01Z

🎯 Recommended Merge Targets

Based on JIRA ticket TT-14891: [Innersource] Dynamic client-IP selection. (XFF Depth Selection Improvement)

Fix Version: Tyk 5.11.0

⚠️ Warning: Expected release branches not found in repository

Required:

master - No matching release branches found. Fix will be included in future releases.

📋 Workflow

Merge this PR to master first

sonarqubecloud · 2025-11-12T17:29:53Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

adds client ip from XFF by depth

2e86ea0

buger added the Story label May 20, 2025

buger added the Tyk Product Development label May 20, 2025

github-actions Bot added the Review effort 3/5 label May 20, 2025

sedkis added 2 commits October 30, 2025 10:19

Merge remote-tracking branch 'origin' into TT-14891/dynamic-xff-clien…

05c4637

…t-ip

Fix lint

50cd2b3

Merge branch 'master' into TT-14891/dynamic-xff-client-ip

7f06a82

sedkis changed the title ~~adds client ip from XFF by depth~~ TT-14891 - adds client ip from XFF by depth Oct 31, 2025

sedkis added 3 commits November 3, 2025 10:33

sonar qube wants to catch errors

3006422

Merge remote-tracking branch 'origin' into TT-14891/dynamic-xff-clien…

22ca1e1

…t-ip

Merge remote-tracking branch 'origin/TT-14891/dynamic-xff-client-ip' …

2c0ec8a

…into TT-14891/dynamic-xff-client-ip

sedkis enabled auto-merge (squash) November 4, 2025 14:00

mativm02 requested changes Nov 7, 2025

View reviewed changes

Comment thread cli/linter/schema.json

Comment thread request/real_ip.go

Comment thread request/real_ip.go Outdated

catch the negative depth invalid config.

5785956

sedkis added 2 commits November 11, 2025 09:27

Add the negative depth case

38609d7

consolidate IF checks

af5e796

mativm02 approved these changes Nov 11, 2025

View reviewed changes

Merge branch 'master' into TT-14891/dynamic-xff-client-ip

a3d051d

sedkis merged commit abdff24 into master Nov 12, 2025
47 of 49 checks passed

sedkis deleted the TT-14891/dynamic-xff-client-ip branch November 12, 2025 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TT-14891 - adds client ip from XFF by depth#7063

TT-14891 - adds client ip from XFF by depth#7063
sedkis merged 11 commits intomasterfrom
TT-14891/dynamic-xff-client-ip

sedkis commented May 20, 2025 •

edited by github-actions Bot

Loading

Uh oh!

buger commented May 20, 2025

Uh oh!

github-actions Bot commented May 20, 2025

Uh oh!

github-actions Bot commented May 20, 2025 •

edited

Loading

Uh oh!

github-actions Bot commented May 20, 2025

Uh oh!

probelabs Bot commented Oct 31, 2025 •

edited

Loading

Uh oh!

probelabs Bot commented Oct 31, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 11, 2025 •

edited

Loading

Uh oh!

sonarqubecloud Bot commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sedkis commented May 20, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Ticket Details

Uh oh!

buger commented May 20, 2025

Uh oh!

github-actions Bot commented May 20, 2025

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 20, 2025

PR Code Suggestions ✨

Uh oh!

probelabs Bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Code Analysis Results

Files Changed Analysis

Architecture & Impact Assessment

Scope Discovery & Context Expansion

Uh oh!

probelabs Bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Code Analysis Results

Security Issues (1)

Architecture Issues (1)

Performance Issues (1)

Quality Issues (3)

Dependency Issues (2)

✅ Connectivity Check Passed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎯 Recommended Merge Targets

Fix Version: Tyk 5.11.0

📋 Workflow

Uh oh!

sonarqubecloud Bot commented Nov 12, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sedkis commented May 20, 2025 •

edited by github-actions Bot

Loading

github-actions Bot commented May 20, 2025 •

edited

Loading

probelabs Bot commented Oct 31, 2025 •

edited

Loading

probelabs Bot commented Oct 31, 2025 •

edited

Loading

github-actions Bot commented Nov 11, 2025 •

edited

Loading