From 36f3655bc7d39bdae555131d094ab8025a7ac4f7 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Thu, 4 Jun 2026 08:19:46 +0200 Subject: [PATCH 1/5] feature: configurable trusted headers for proxy_ip_parser 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. --- config.go | 3 + doc.go | 6 +- plugin.go | 148 +++++++++++++++++++++------- tests/configs/.rr-http-headers.yaml | 37 +++++++ tests/php_test_files/http/ip.php | 11 +++ tests/proxy_ip_parser_test.go | 98 ++++++++++++++++++ trusted_test.go | 62 +++++++++++- 7 files changed, 325 insertions(+), 40 deletions(-) create mode 100644 tests/configs/.rr-http-headers.yaml create mode 100644 tests/php_test_files/http/ip.php diff --git a/config.go b/config.go index b366c46..505d146 100644 --- a/config.go +++ b/config.go @@ -3,4 +3,7 @@ package proxy type Config struct { // TrustedSubnets declare IP subnets which are allowed to set ip using X-Real-Ip and X-Forwarded-For TrustedSubnets []string `mapstructure:"trusted_subnets"` + // TrustedHeaders is the ordered allowlist of headers consulted to resolve the + // real client IP. When empty, the built-in default order is used. + TrustedHeaders []string `mapstructure:"trusted_headers"` } diff --git a/doc.go b/doc.go index fd089b3..d0b8207 100644 --- a/doc.go +++ b/doc.go @@ -1,5 +1,9 @@ // Package proxy provides an HTTP middleware plugin for RoadRunner that resolves // the real client IP address from proxy headers (X-Forwarded-For, X-Real-Ip, -// True-Client-Ip, CF-Connecting-Ip, Forwarded) when requests arrive through +// True-Client-Ip, Cf-Connecting-Ip, Forwarded) when requests arrive through // trusted subnets. +// +// The headers consulted are configurable via http.trusted_headers: an ordered +// allowlist where the first non-empty match wins. When it is unset, the headers +// above are used in that default order. package proxy diff --git a/plugin.go b/plugin.go index b1a652e..651692b 100644 --- a/plugin.go +++ b/plugin.go @@ -18,13 +18,14 @@ import ( ) const ( - name string = "proxy_ip_parser" - configKey string = "http.trusted_subnets" - xff string = "X-Forwarded-For" - xrip string = "X-Real-Ip" - tcip string = "True-Client-Ip" - cfip string = "Cf-Connecting-Ip" - forwarded string = "Forwarded" + name string = "proxy_ip_parser" + configKey string = "http.trusted_subnets" + headersKey string = "http.trusted_headers" + xff string = "X-Forwarded-For" + xrip string = "X-Real-Ip" + tcip string = "True-Client-Ip" + cfip string = "Cf-Connecting-Ip" + forwarded string = "Forwarded" ) var forwardedRegex = regexp.MustCompile(`(?i)(?:for=)([^(;|,| )]+)`) @@ -41,10 +42,11 @@ type Configurer interface { } type Plugin struct { - cfg *Config - log *slog.Logger - trusted []*net.IPNet - prop propagation.TextMapPropagator + cfg *Config + log *slog.Logger + trusted []*net.IPNet + resolvers []resolver + prop propagation.TextMapPropagator } func (p *Plugin) Init(cfg Configurer, l Logger) error { @@ -77,6 +79,13 @@ func (p *Plugin) Init(cfg Configurer, l Logger) error { p.trusted[i] = ipNet } + if cfg.Has(headersKey) { + if err := cfg.UnmarshalKey(headersKey, &p.cfg.TrustedHeaders); err != nil { + return errors.E(op, err) + } + } + p.resolvers = buildResolvers(p.cfg.TrustedHeaders) + return nil } @@ -129,38 +138,101 @@ func (p *Plugin) Name() string { return name } -// get real ip passing multiple proxy -func (p *Plugin) resolveIP(headers http.Header) string { - // new Forwarded header - // https://datatracker.ietf.org/doc/html/rfc7239 - if fwd := headers.Get(forwarded); fwd != "" { - if get := forwardedRegex.FindStringSubmatch(fwd); len(get) > 1 { - // IPv6 -> It is important to note that an IPv6 address and any nodename with - // node-port specified MUST be quoted - // we should trim the " - return strings.Trim(get[1], `"`) +// resolver extracts a candidate client IP from a single header value. +type resolver struct { + name string // canonical header name, e.g. "X-Forwarded-For" + parse func(string) string +} + +// defaultResolvers returns the built-in resolution chain used when +// http.trusted_headers is not configured. True-Client-Ip and Cf-Connecting-Ip +// (CloudFlare) are checked last, matching their historical priority. +func defaultResolvers() []resolver { + return []resolver{ + {forwarded, parseForwarded}, + {xff, parseXFF}, + {xrip, parseVerbatim}, + {tcip, parseVerbatim}, + {cfip, parseVerbatim}, + } +} + +// buildResolvers turns the configured header allowlist into an ordered resolver +// chain. Entries are trimmed and canonicalized, blanks and duplicates dropped. +// An empty allowlist falls back to the default chain. +func buildResolvers(headers []string) []resolver { + resolvers := make([]resolver, 0, len(headers)) + seen := make(map[string]struct{}, len(headers)) + + for i := range headers { + h := strings.TrimSpace(headers[i]) + if h == "" { + continue + } + + canon := http.CanonicalHeaderKey(h) + if _, ok := seen[canon]; ok { + continue } - // XFF parse - } else if fwd := headers.Get(xff); fwd != "" { - // take the first address; Cut returns the whole string when no comma is present - before, _, _ := strings.Cut(fwd, ",") - return before - // next -> X-Real-Ip - } else if fwd := headers.Get(xrip); fwd != "" { - return fwd + + seen[canon] = struct{}{} + resolvers = append(resolvers, resolver{canon, parserFor(canon)}) + } + + if len(resolvers) == 0 { + return defaultResolvers() } - // The logic here is the following: - // CloudFlare headers - // True-Client-IP is a general CF header in which copied information from X-Real-Ip in CF. - // CF-Connecting-IP is an Enterprise feature and we check it last in order. - // This operations are near O(1) because Headers struct are the map type -> type MIMEHeader map[string][]string - if fwd := headers.Get(tcip); fwd != "" { - return fwd + return resolvers +} + +// parserFor selects the value parser for a canonical header name. The two +// structured headers keep dedicated parsers; everything else (including custom +// headers) is taken verbatim. +func parserFor(canon string) func(string) string { + switch canon { + case forwarded: + return parseForwarded + case xff: + return parseXFF + default: + return parseVerbatim + } +} + +// parseForwarded extracts the "for=" target from an RFC 7239 Forwarded header. +// https://datatracker.ietf.org/doc/html/rfc7239 +func parseForwarded(v string) string { + if m := forwardedRegex.FindStringSubmatch(v); len(m) > 1 { + // An IPv6 address (and any node-port) MUST be quoted, so trim the quotes. + return strings.Trim(m[1], `"`) } - if fwd := headers.Get(cfip); fwd != "" { - return fwd + return "" +} + +// parseXFF takes the left-most address from an X-Forwarded-For list. +func parseXFF(v string) string { + // Cut returns the whole string when no comma is present. + before, _, _ := strings.Cut(v, ",") + return before +} + +// parseVerbatim returns the header value unchanged (X-Real-Ip, True-Client-Ip, +// Cf-Connecting-Ip and custom headers carry a single address). +func parseVerbatim(v string) string { + return v +} + +// resolveIP returns the first non-empty client IP parsed from the configured +// (or default) header chain. +func (p *Plugin) resolveIP(headers http.Header) string { + for i := range p.resolvers { + if raw := headers.Get(p.resolvers[i].name); raw != "" { + if ip := p.resolvers[i].parse(raw); ip != "" { + return ip + } + } } return "" diff --git a/tests/configs/.rr-http-headers.yaml b/tests/configs/.rr-http-headers.yaml new file mode 100644 index 0000000..14a01d7 --- /dev/null +++ b/tests/configs/.rr-http-headers.yaml @@ -0,0 +1,37 @@ +version: '3' + +rpc: + listen: tcp://127.0.0.1:6001 + +server: + command: "php php_test_files/http/client.php ip pipes" + relay: "pipes" + relay_timeout: "20s" + +http: + address: 127.0.0.1:12411 + max_request_size: 1024 + middleware: [ "proxy_ip_parser" ] + uploads: + forbid: [ ".php", ".exe", ".bat" ] + trusted_subnets: + [ + "10.0.0.0/8", + "127.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "::1/128", + "fc00::/7", + "fe80::/10" + ] + # only X-Real-Ip is trusted; X-Forwarded-* and the rest are ignored + trusted_headers: [ "X-Real-Ip" ] + pool: + num_workers: 2 + max_jobs: 0 + allocate_timeout: 60s + destroy_timeout: 60s + +logs: + mode: development + level: error diff --git a/tests/php_test_files/http/ip.php b/tests/php_test_files/http/ip.php new file mode 100644 index 0000000..49eb928 --- /dev/null +++ b/tests/php_test_files/http/ip.php @@ -0,0 +1,11 @@ +getBody()->write($req->getServerParams()['REMOTE_ADDR']); + + return $resp; +} diff --git a/tests/proxy_ip_parser_test.go b/tests/proxy_ip_parser_test.go index 6767276..e9dc52a 100644 --- a/tests/proxy_ip_parser_test.go +++ b/tests/proxy_ip_parser_test.go @@ -1,6 +1,7 @@ package proxy_ip_parser //nolint:stylecheck import ( + "io" "log/slog" "net/http" "os" @@ -233,3 +234,100 @@ func TestForwarded(t *testing.T) { stopCh <- struct{}{} wg.Wait() } + +// TestTrustedHeadersAllowlist verifies that, with trusted_headers: [ X-Real-Ip ], +// only X-Real-Ip is honored when resolving the client IP and X-Forwarded-For is +// ignored. The ip worker echoes REMOTE_ADDR so we can assert the resolved value. +func TestTrustedHeadersAllowlist(t *testing.T) { + cont := endure.New(slog.LevelDebug) + + cfg := &config.Plugin{ + Version: "2024.2.0", + Path: "configs/.rr-http-headers.yaml", + } + + err := cont.RegisterAll( + cfg, + &logger.Plugin{}, + &server.Plugin{}, + &ipparser.Plugin{}, + &httpPlugin.Plugin{}, + ) + assert.NoError(t, err) + + err = cont.Init() + if err != nil { + t.Fatal(err) + } + + ch, err := cont.Serve() + assert.NoError(t, err) + + sig := make(chan os.Signal, 1) + signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM) + + wg := &sync.WaitGroup{} + + stopCh := make(chan struct{}, 1) + + wg.Go(func() { + for { + select { + case e := <-ch: + assert.Fail(t, "error", e.Error.Error()) + err = cont.Stop() + if err != nil { + assert.FailNow(t, "error", err.Error()) + } + case <-sig: + err = cont.Stop() + if err != nil { + assert.FailNow(t, "error", err.Error()) + } + return + case <-stopCh: + // timeout + err = cont.Stop() + if err != nil { + assert.FailNow(t, "error", err.Error()) + } + return + } + } + }) + + time.Sleep(time.Second * 2) + + // X-Real-Ip is on the allowlist -> trusted, becomes REMOTE_ADDR. + req, err := http.NewRequest("GET", "http://127.0.0.1:12411", nil) + assert.NoError(t, err) + req.Header.Set("X-Real-Ip", "5.6.7.8") + + r, err := http.DefaultClient.Do(req) + assert.NoError(t, err) + assert.Equal(t, 200, r.StatusCode) + + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + assert.Equal(t, "5.6.7.8", string(body)) + assert.NoError(t, r.Body.Close()) + + // --- + + // X-Forwarded-For is NOT on the allowlist -> ignored, REMOTE_ADDR stays the peer. + req, err = http.NewRequest("GET", "http://127.0.0.1:12411", nil) + assert.NoError(t, err) + req.Header.Set("X-Forwarded-For", "5.6.7.8") + + r, err = http.DefaultClient.Do(req) + assert.NoError(t, err) + assert.Equal(t, 200, r.StatusCode) + + body, err = io.ReadAll(r.Body) + assert.NoError(t, err) + assert.Equal(t, "127.0.0.1", string(body)) + assert.NoError(t, r.Body.Close()) + + stopCh <- struct{}{} + wg.Wait() +} diff --git a/trusted_test.go b/trusted_test.go index 6ee735e..9ae2ea5 100644 --- a/trusted_test.go +++ b/trusted_test.go @@ -34,7 +34,7 @@ func TestIP(t *testing.T) { {forwarded, `for="workstation.local",for=198.51.100.17`, "workstation.local"}, // Hostname } - tr := &Plugin{} + tr := &Plugin{resolvers: defaultResolvers()} for _, v := range headers { req := &http.Request{ Header: http.Header{ @@ -48,6 +48,66 @@ func TestIP(t *testing.T) { } } +// header builds an http.Header from key/value pairs. +func header(kv ...string) http.Header { + h := http.Header{} + for i := 0; i+1 < len(kv); i += 2 { + h.Set(kv[i], kv[i+1]) + } + return h +} + +func resolverNames(rs []resolver) []string { + out := make([]string, len(rs)) + for i := range rs { + out[i] = rs[i].name + } + return out +} + +// An allowlist consults only the configured headers; unlisted ones are ignored. +func TestResolveIPAllowlistIgnoresUnlisted(t *testing.T) { + p := &Plugin{resolvers: buildResolvers([]string{xrip})} + // X-Forwarded-For is present but not on the allowlist, so it is ignored. + require.Equal(t, "9.9.9.9", p.resolveIP(header(xff, "8.8.8.8", xrip, "9.9.9.9"))) +} + +// The first header in the allowlist that yields a value wins. +func TestResolveIPOrderWins(t *testing.T) { + h := header(xff, "1.1.1.1", xrip, "2.2.2.2") + + xripFirst := &Plugin{resolvers: buildResolvers([]string{xrip, xff})} + require.Equal(t, "2.2.2.2", xripFirst.resolveIP(h)) + + xffFirst := &Plugin{resolvers: buildResolvers([]string{xff, xrip})} + require.Equal(t, "1.1.1.1", xffFirst.resolveIP(h)) +} + +// Custom (non-built-in) headers are taken verbatim. +func TestResolveIPCustomHeaderVerbatim(t *testing.T) { + p := &Plugin{resolvers: buildResolvers([]string{"X-Client-Ip"})} + require.Equal(t, "3.3.3.3", p.resolveIP(header("X-Client-Ip", "3.3.3.3"))) +} + +// A present but unparseable header falls through to the next resolver. The old +// if/else-if chain short-circuited here; the ordered chain keeps looking. +func TestResolveIPFallThroughOnParseFailure(t *testing.T) { + p := &Plugin{resolvers: buildResolvers([]string{forwarded, xrip})} + // "Forwarded" with no for= directive yields nothing, so X-Real-Ip is used. + require.Equal(t, "8.8.8.8", p.resolveIP(header(forwarded, "by=203.0.113.43;proto=https", xrip, "8.8.8.8"))) +} + +func TestBuildResolvers(t *testing.T) { + // trim, canonicalize, dedup, skip blanks + rs := buildResolvers([]string{" x-real-ip ", "X-Real-Ip", "", "Cf-Connecting-Ip"}) + require.Equal(t, []string{xrip, cfip}, resolverNames(rs)) + + // empty / all-blank input falls back to the default chain + def := resolverNames(defaultResolvers()) + require.Equal(t, def, resolverNames(buildResolvers(nil))) + require.Equal(t, def, resolverNames(buildResolvers([]string{"", " "}))) +} + func TestCidrsInRange(t *testing.T) { ipAddr := "62.76.33.22/22" From 04a806dcc842a34b7575843ce6adbcc46da733b3 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Thu, 4 Jun 2026 08:20:10 +0200 Subject: [PATCH 2/5] fix(test): initialize propagator in otel middleware tests 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. --- plugin_otel_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugin_otel_test.go b/plugin_otel_test.go index 61bfea4..32744c3 100644 --- a/plugin_otel_test.go +++ b/plugin_otel_test.go @@ -10,6 +10,7 @@ import ( rrcontext "github.com/roadrunner-server/context" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/propagation" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" ) @@ -22,7 +23,7 @@ func TestMiddlewareSpanEndsBeforeNextHandler(t *testing.T) { _, ipNet, err := net.ParseCIDR("127.0.0.0/8") require.NoError(t, err) - p := &Plugin{trusted: []*net.IPNet{ipNet}} + p := &Plugin{trusted: []*net.IPNet{ipNet}, prop: propagation.TraceContext{}} // "next" handler that creates its own span to mark when downstream starts next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -78,7 +79,7 @@ func TestMiddlewareSpanEndsOnError(t *testing.T) { tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) t.Cleanup(func() { _ = tp.Shutdown(t.Context()) }) - p := &Plugin{} + p := &Plugin{prop: propagation.TraceContext{}} nextCalled := false next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { From 949ba04da5ea0963ae0b54b685732a045bc1d4c3 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Thu, 4 Jun 2026 08:20:10 +0200 Subject: [PATCH 3/5] chore(deps): bump test module dependencies --- tests/go.mod | 5 ++--- tests/go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/go.mod b/tests/go.mod index 865092c..ab2f203 100644 --- a/tests/go.mod +++ b/tests/go.mod @@ -33,7 +33,7 @@ require ( github.com/joho/godotenv v1.5.1 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect github.com/libdns/libdns v1.1.1 // indirect - github.com/mattn/go-colorable v0.1.14 // indirect + github.com/mattn/go-colorable v0.1.15 // indirect github.com/mattn/go-isatty v0.0.22 // indirect github.com/mholt/acmez v1.2.0 // indirect github.com/mholt/acmez/v3 v3.1.6 // indirect @@ -43,7 +43,7 @@ require ( github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect - github.com/prometheus/common v0.67.5 // indirect + github.com/prometheus/common v0.68.1 // indirect github.com/prometheus/procfs v0.20.1 // indirect github.com/quic-go/qpack v0.6.0 // indirect github.com/quic-go/quic-go v0.59.1 // indirect @@ -76,7 +76,6 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.28.0 // indirect go.uber.org/zap/exp v0.3.0 // indirect - go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.52.0 // indirect golang.org/x/mod v0.36.0 // indirect diff --git a/tests/go.sum b/tests/go.sum index 1f21fba..51b0054 100644 --- a/tests/go.sum +++ b/tests/go.sum @@ -50,8 +50,8 @@ github.com/letsencrypt/pebble/v2 v2.10.0 h1:Wq6gYXlsY6ubqI3hhxsTzdyotvfdjFBxuwYq github.com/letsencrypt/pebble/v2 v2.10.0/go.mod h1:Sk8cmUIPcIdv2nINo+9PB4L+ZBhzY+F9A1a/h/xmWiQ= github.com/libdns/libdns v1.1.1 h1:wPrHrXILoSHKWJKGd0EiAVmiJbFShguILTg9leS/P/U= github.com/libdns/libdns v1.1.1/go.mod h1:4Bj9+5CQiNMVGf87wjX4CY3HQJypUHRuLvlsfsZqLWQ= -github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= -github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= +github.com/mattn/go-colorable v0.1.15 h1:+u9SLTRGnXv73cEsnsmoZBom+dMU88B2M0aDcWy0/jY= +github.com/mattn/go-colorable v0.1.15/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= github.com/mattn/go-isatty v0.0.22 h1:j8l17JJ9i6VGPUFUYoTUKPSgKe/83EYU2zBC7YNKMw4= github.com/mattn/go-isatty v0.0.22/go.mod h1:ZXfXG4SQHsB/w3ZeOYbR0PrPwLy+n6xiMrJlRFqopa4= github.com/mholt/acmez v1.2.0 h1:1hhLxSgY5FvH5HCnGUuwbKY2VQVo8IU7rxXKSnZ7F30= @@ -70,8 +70,8 @@ github.com/prometheus/client_golang v1.23.2 h1:Je96obch5RDVy3FDMndoUsjAhG5Edi49h github.com/prometheus/client_golang v1.23.2/go.mod h1:Tb1a6LWHB3/SPIzCoaDXI4I8UHKeFTEQ1YCr+0Gyqmg= github.com/prometheus/client_model v0.6.2 h1:oBsgwpGs7iVziMvrGhE53c/GrLUsZdHnqNwqPLxwZyk= github.com/prometheus/client_model v0.6.2/go.mod h1:y3m2F6Gdpfy6Ut/GBsUqTWZqCUvMVzSfMLjcu6wAwpE= -github.com/prometheus/common v0.67.5 h1:pIgK94WWlQt1WLwAC5j2ynLaBRDiinoAb86HZHTUGI4= -github.com/prometheus/common v0.67.5/go.mod h1:SjE/0MzDEEAyrdr5Gqc6G+sXI67maCxzaT3A2+HqjUw= +github.com/prometheus/common v0.68.1 h1:omjRRl4QP4komogpXuhfeOiisQg7xdy8VM1UY+pStaY= +github.com/prometheus/common v0.68.1/go.mod h1:ZzL3f6u94qUxh9p+tJTrF+FvBS1XXbbRAZCQkytAL0Y= github.com/prometheus/procfs v0.20.1 h1:XwbrGOIplXW/AU3YhIhLODXMJYyC1isLFfYCsTEycfc= github.com/prometheus/procfs v0.20.1/go.mod h1:o9EMBZGRyvDrSPH1RqdxhojkuXstoe4UlK79eF5TGGo= github.com/quic-go/qpack v0.6.0 h1:g7W+BMYynC1LbYLSqRt8PBg5Tgwxn214ZZR34VIOjz8= From aa8c95831a33891355f5ba9cb70217517e9c47c8 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Thu, 4 Jun 2026 08:26:51 +0200 Subject: [PATCH 4/5] refactor: tidy resolver chain after review - 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 --- plugin.go | 24 ++++++++++++------------ trusted_test.go | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/plugin.go b/plugin.go index 651692b..f8ca326 100644 --- a/plugin.go +++ b/plugin.go @@ -146,15 +146,15 @@ type resolver struct { // defaultResolvers returns the built-in resolution chain used when // http.trusted_headers is not configured. True-Client-Ip and Cf-Connecting-Ip -// (CloudFlare) are checked last, matching their historical priority. +// (CloudFlare) are checked last, matching their historical priority. The parser +// for each header comes from parserFor, the single source of that mapping. func defaultResolvers() []resolver { - return []resolver{ - {forwarded, parseForwarded}, - {xff, parseXFF}, - {xrip, parseVerbatim}, - {tcip, parseVerbatim}, - {cfip, parseVerbatim}, + chain := []string{forwarded, xff, xrip, tcip, cfip} + resolvers := make([]resolver, len(chain)) + for i, h := range chain { + resolvers[i] = resolver{h, parserFor(h)} } + return resolvers } // buildResolvers turns the configured header allowlist into an ordered resolver @@ -164,8 +164,8 @@ func buildResolvers(headers []string) []resolver { resolvers := make([]resolver, 0, len(headers)) seen := make(map[string]struct{}, len(headers)) - for i := range headers { - h := strings.TrimSpace(headers[i]) + for _, hdr := range headers { + h := strings.TrimSpace(hdr) if h == "" { continue } @@ -227,9 +227,9 @@ func parseVerbatim(v string) string { // resolveIP returns the first non-empty client IP parsed from the configured // (or default) header chain. func (p *Plugin) resolveIP(headers http.Header) string { - for i := range p.resolvers { - if raw := headers.Get(p.resolvers[i].name); raw != "" { - if ip := p.resolvers[i].parse(raw); ip != "" { + for _, r := range p.resolvers { + if raw := headers.Get(r.name); raw != "" { + if ip := r.parse(raw); ip != "" { return ip } } diff --git a/trusted_test.go b/trusted_test.go index 9ae2ea5..a9c1494 100644 --- a/trusted_test.go +++ b/trusted_test.go @@ -59,8 +59,8 @@ func header(kv ...string) http.Header { func resolverNames(rs []resolver) []string { out := make([]string, len(rs)) - for i := range rs { - out[i] = rs[i].name + for i, r := range rs { + out[i] = r.name } return out } From 70a7242e5125b62c1bfe4c1c7c0fdba4fdb38b99 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Thu, 4 Jun 2026 08:32:27 +0200 Subject: [PATCH 5/5] fix: correct default header order in doc and harden integration test - doc.go: list default headers in actual precedence (Forwarded first) - tests: require.NoError before dereferencing the HTTP response --- doc.go | 4 ++-- tests/proxy_ip_parser_test.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/doc.go b/doc.go index d0b8207..52cd22e 100644 --- a/doc.go +++ b/doc.go @@ -1,6 +1,6 @@ // Package proxy provides an HTTP middleware plugin for RoadRunner that resolves -// the real client IP address from proxy headers (X-Forwarded-For, X-Real-Ip, -// True-Client-Ip, Cf-Connecting-Ip, Forwarded) when requests arrive through +// the real client IP address from proxy headers (Forwarded, X-Forwarded-For, +// X-Real-Ip, True-Client-Ip, Cf-Connecting-Ip) when requests arrive through // trusted subnets. // // The headers consulted are configurable via http.trusted_headers: an ordered diff --git a/tests/proxy_ip_parser_test.go b/tests/proxy_ip_parser_test.go index e9dc52a..295b166 100644 --- a/tests/proxy_ip_parser_test.go +++ b/tests/proxy_ip_parser_test.go @@ -18,6 +18,7 @@ import ( ipparser "github.com/roadrunner-server/proxy_ip_parser/v6" "github.com/roadrunner-server/server/v6" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestXFF(t *testing.T) { @@ -304,11 +305,11 @@ func TestTrustedHeadersAllowlist(t *testing.T) { req.Header.Set("X-Real-Ip", "5.6.7.8") r, err := http.DefaultClient.Do(req) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 200, r.StatusCode) body, err := io.ReadAll(r.Body) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, "5.6.7.8", string(body)) assert.NoError(t, r.Body.Close()) @@ -320,11 +321,11 @@ func TestTrustedHeadersAllowlist(t *testing.T) { req.Header.Set("X-Forwarded-For", "5.6.7.8") r, err = http.DefaultClient.Do(req) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 200, r.StatusCode) body, err = io.ReadAll(r.Body) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, "127.0.0.1", string(body)) assert.NoError(t, r.Body.Close())