feature: configurable trusted headers for proxy_ip_parser#120
Conversation
Add the http.trusted_headers config key: an ordered allowlist of headers consulted to resolve the real client IP, first non-empty match wins. Listing only X-Real-Ip ignores X-Forwarded-* etc.; custom headers are supported. When unset, the built-in default order is used, so existing configs are unaffected.
The otel middleware tests built Plugin without prop, so the OTel branch nil-dereferenced p.prop.Inject. Init always sets it in production; set a propagator in the tests too. CI runs only the tests/ module, so these main-module tests were never exercised.
|
Warning Review limit reached
More reviews will be available in 48 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a configurable ordered header allowlist ( ChangesTrusted Headers Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring which proxy IP-forwarding headers are trusted (and in what order) when resolving the real client IP, while preserving the previous default resolution chain when the new setting is unset. It also updates unit/integration tests (including an OTel test fix) and bumps a couple of transitive dependencies in the tests module.
Changes:
- Add
http.trusted_headersallowlist support, with ordered resolution and fall-through on parse failures. - Expand unit + integration test coverage for allowlist behavior; add a PHP worker that echoes
REMOTE_ADDRfor integration assertions. - Fix OTel middleware tests to initialize the propagator and update
testsmodule dependencies.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| trusted_test.go | Adds unit tests for allowlist ordering, custom headers, and parse-failure fall-through. |
| tests/proxy_ip_parser_test.go | Adds an integration test asserting allowlist behavior end-to-end via REMOTE_ADDR. |
| tests/php_test_files/http/ip.php | Adds a PHP handler to echo REMOTE_ADDR for integration testing. |
| tests/go.sum | Updates transitive dependency checksums for the tests module. |
| tests/go.mod | Bumps two indirect deps and drops an unused indirect YAML v2 dep in the tests module. |
| tests/configs/.rr-http-headers.yaml | Adds a dedicated RR config enabling http.trusted_headers for integration testing. |
| plugin.go | Implements resolver chain abstraction and configurable trusted header allowlist. |
| plugin_otel_test.go | Fixes OTel middleware tests by ensuring a non-nil propagator is set. |
| doc.go | Documents the new http.trusted_headers configuration behavior. |
| config.go | Adds TrustedHeaders config field mapped from trusted_headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc.go`:
- Around line 1-8: Doc default header order is inconsistent with the runtime
resolver in plugin.go where Forwarded is checked first; update the header list
in doc.go to mirror the actual default precedence used by the resolver (place
Forwarded first, then X-Forwarded-For, X-Real-Ip, True-Client-Ip,
Cf-Connecting-Ip) and verify the text still references http.trusted_headers as
the configurable allowlist.
In `@tests/proxy_ip_parser_test.go`:
- Around line 306-313: Replace non-fatal assertions that guard dereferencing the
HTTP response with fatal ones: change assert.NoError(t, err) to
require.NoError(t, err) for the http.DefaultClient.Do(req) call (the variables r
and err) so we don't access r.Body when r is nil, and likewise change the
assert.NoError(t, err) after io.ReadAll(r.Body) to require.NoError(t, err);
apply the same replacements in the similar block covering lines 322-329 to
harden those tests as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc10a97b-26bf-4734-8df6-6d647e813978
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
config.godoc.goplugin.goplugin_otel_test.gotests/configs/.rr-http-headers.yamltests/go.modtests/php_test_files/http/ip.phptests/proxy_ip_parser_test.gotrusted_test.go
- range over values instead of indices (resolveIP, buildResolvers, test helper) - derive the default chain via parserFor so the header->parser mapping has a single source of truth
- doc.go: list default headers in actual precedence (Forwarded first) - tests: require.NoError before dereferencing the HTTP response
Adds a configurable allowlist of IP-forwarding headers for resolving the real client IP.
http.trusted_headers: an ordered allowlist, first non-empty match wins. Listing onlyX-Real-IpignoresX-Forwarded-*; custom headers are supported (taken verbatim).Forwarded,X-Forwarded-For,X-Real-Ip,True-Client-Ip,Cf-Connecting-Ip), so existing configs are unaffected.Forwardedwithout afor=directive) now falls through to the next header instead of short-circuiting.Also includes a fix for the OTel middleware unit tests (they constructed
Pluginwithout a propagator → nil panic; these main-module tests are not run in CI) and a bump of two transitive test-module deps.The integration job is red for a pre-existing, org-wide reason (the PHP worker SDK predates
pool/v2-beta / PHP 8.5), unrelated to this change; the feature is covered by unit tests.closes roadrunner-server/roadrunner#1515
Summary by CodeRabbit
New Features
trusted_headerssetting to specify which headers the proxy IP parser middleware consults (and in what order) when resolving the real client IP. Defaults to built-in header priorities when unset.Documentation