release: v7.15.5 (#3553)

* update to release version v7.15.5

* Merge commit from fork

Signed-off-by: Jan Larwig <jan@larwig.com>
Co-authored-by: blakeroberts-wk <blake.roberts@workiva.com>

* Merge commit from fork

Signed-off-by: Jan Larwig <jan@larwig.com>

* Merge commit from fork

* fix: validate trusted IP proxy headers

Respect trusted proxy boundaries before using real-client-IP headers for authentication bypass decisions and safely traverse X-Forwarded-For chains.

Signed-off-by: Jan Larwig <jan@larwig.com>

* fix: trusted-ip header bypass

Signed-off-by: Jan Larwig <jan@larwig.com>

* docs: add changelog entry

Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>

---------

Signed-off-by: Jan Larwig <jan@larwig.com>
Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>

* docs: changelog for v7.15.5

Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>

* docs: update order of owners for prow

Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>

* ci: make the linter happy again

Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>

---------

Signed-off-by: Jan Larwig <jan@larwig.com>
Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jan Larwig <jan@larwig.com>
Co-authored-by: blakeroberts-wk <blake.roberts@workiva.com>
Co-authored-by: Jan Larwig <jan.larwig@digits.schwarz>

pkg/apis/options/options.go

@@ -119,9 +119,9 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet := pflag.NewFlagSet("oauth2-proxy", pflag.ExitOnError)
 
 	flagSet.Bool("reverse-proxy", false, "are we running behind a reverse proxy, controls whether headers like X-Real-Ip are accepted")
-	flagSet.String("real-client-ip-header", "X-Real-IP", "Header used to determine the real IP of the client (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP)")
-	flagSet.StringSlice("trusted-proxy-ip", []string{}, "list of IPs or CIDR ranges that are allowed to set X-Forwarded-* headers when --reverse-proxy is enabled. Defaults to trusting all IPs for backwards compatibility; configure this to your reverse proxy addresses to prevent header spoofing.")
-	flagSet.StringSlice("trusted-ip", []string{}, "list of IPs or CIDR ranges to allow to bypass authentication. WARNING: trusting by IP has inherent security flaws, read the configuration documentation for more information.")
+	flagSet.String("real-client-ip-header", "X-Real-IP", "Header used to determine the real IP of the client when the direct caller is a trusted proxy (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP)")
+	flagSet.StringSlice("trusted-proxy-ip", []string{}, "list of direct proxy IPs or CIDR ranges allowed to set X-Forwarded-* headers when --reverse-proxy is enabled, including every trusted proxy hop in X-Forwarded-For. Defaults to trusting all IPs for backwards compatibility.")
+	flagSet.StringSlice("trusted-ip", []string{}, "list of IPs or CIDR ranges to allow to bypass authentication. WARNING: trusting by IP has inherent security flaws; with --reverse-proxy, configure --trusted-proxy-ip and read the configuration documentation.")
 	flagSet.Bool("force-https", false, "force HTTPS redirect for HTTP requests")
 	flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"")
 	flagSet.Bool("relative-redirect-url", false, "allow relative OAuth Redirect URL.")

pkg/ip/realclientip.go

@@ -49,18 +49,58 @@ func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, err
 	if commaIndex := strings.IndexRune(ipStr, ','); commaIndex != -1 {
 		ipStr = ipStr[:commaIndex]
 	}
-	ipStr = strings.TrimSpace(ipStr)
 
+	return parseClientIP(ipStr, p.header)
+}
+
+// GetClientIPFromTrustedProxy obtains the client IP from a header supplied by
+// a caller that has already been verified as a trusted proxy.
+func GetClientIPFromTrustedProxy(p ipapi.RealClientIPParser, req *http.Request, trustedProxies *NetSet) (net.IP, error) {
+	if p == nil {
+		return nil, fmt.Errorf("real client IP parser is required")
+	}
+
+	xffParser, ok := p.(*xForwardedForClientIPParser)
+	if !ok || xffParser.header != http.CanonicalHeaderKey("X-Forwarded-For") {
+		return p.GetRealClientIP(req.Header)
+	}
+
+	if trustedProxies == nil {
+		return nil, fmt.Errorf("trusted proxy list is required to parse X-Forwarded-For")
+	}
+
+	xff := strings.Join(req.Header.Values(xffParser.header), ",")
+	if xff == "" {
+		return nil, nil
+	}
+
+	chain := strings.Split(xff, ",")
+	for i := len(chain) - 1; i >= 0; i-- {
+		clientIP, err := parseClientIP(chain[i], xffParser.header)
+		if err != nil {
+			return nil, err
+		}
+
+		if i == 0 || !trustedProxies.Has(clientIP) {
+			return clientIP, nil
+		}
+	}
+
+	return nil, nil
+}
+
+func parseClientIP(ipStr string, header string) (net.IP, error) {
+	ipStr = strings.TrimSpace(ipStr)
 	if ipHost, _, err := net.SplitHostPort(ipStr); err == nil {
 		ipStr = ipHost
 	}
 
-	ip := net.ParseIP(ipStr)
-	if ip == nil {
-		return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(p.header))
+	clientIP := net.ParseIP(ipStr)
+	if clientIP == nil {
+		return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(header))
 	}
 
-	return ip, nil
+	return clientIP, nil
 }
 
 // GetClientIP obtains the perceived end-user IP address from headers if p != nil else from req.RemoteAddr.
@@ -90,11 +130,13 @@ func getRemoteIP(req *http.Request) (net.IP, error) {
 	//revive:enable:indent-error-flow
 }
 
-// GetClientString obtains the human readable string of the remote IP and optionally the real client IP if available
-func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool) (s string) {
+// GetClientString obtains the human readable string of the remote IP and optionally
+// the real client IP. Callers must pass a nil parser unless the peer is trusted.
+// Missing or invalid client headers leave only the transport IP in the output.
+func GetClientString(p ipapi.RealClientIPParser, req *http.Request, trustedProxies *NetSet, full bool) (s string) {
 	var realClientIPStr string
 	if p != nil {
-		if realClientIP, err := p.GetRealClientIP(req.Header); err == nil && realClientIP != nil {
+		if realClientIP, err := GetClientIPFromTrustedProxy(p, req, trustedProxies); err == nil && realClientIP != nil {
 			realClientIPStr = realClientIP.String()
 		}
 	}

pkg/ip/realclientip_test.go

@@ -8,6 +8,7 @@ import (
 
 	ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGetRealClientIPParser(t *testing.T) {
@@ -106,6 +107,93 @@ func TestXForwardedForClientIPParserIgnoresOthers(t *testing.T) {
 	assert.Equal(t, ip, net.ParseIP(expectedIPString))
 }
 
+func TestGetClientIPFromTrustedProxy(t *testing.T) {
+	parser, err := GetRealClientIPParser("X-Forwarded-For")
+	require.NoError(t, err)
+	trustedProxies, err := ParseNetSet([]string{"192.0.2.0/24", "2001:db8:1::/48"})
+	require.NoError(t, err)
+
+	tests := []struct {
+		name         string
+		headerValues []string
+		expectedIP   string
+		errString    string
+	}{
+		{"Ignores spoofed leftmost entry", []string{"10.0.0.5, 198.51.100.23"}, "198.51.100.23", ""},
+		{"Walks past trusted intermediate proxies", []string{"198.51.100.23, 192.0.2.20, 192.0.2.21"}, "198.51.100.23", ""},
+		{"Uses leftmost entry when entire chain is trusted", []string{"192.0.2.30, 192.0.2.20"}, "192.0.2.30", ""},
+		{"Repeated fields preserve chain order", []string{"10.0.0.5", "198.51.100.23, 192.0.2.20", "192.0.2.21"}, "198.51.100.23", ""},
+		{"Whitespace and IPv4 port", []string{" 198.51.100.23:1234 ,\t192.0.2.20:443 "}, "198.51.100.23", ""},
+		{"IPv6 client and proxies", []string{"2001:db8:2::23, 2001:db8:1::20"}, "2001:db8:2::23", ""},
+		{"IPv6 port and mixed chain", []string{" [2001:db8:2::23]:1234 , [2001:db8:1::20]:443, 192.0.2.20"}, "2001:db8:2::23", ""},
+		{"Stops at first untrusted hop", []string{"10.0.0.5, 198.51.100.22, 198.51.100.23, 192.0.2.20"}, "198.51.100.23", ""},
+		{"Ignores malformed entry beyond boundary", []string{"invalid, 198.51.100.23, 192.0.2.20"}, "198.51.100.23", ""},
+		{"Missing header", nil, "", ""},
+		{"Empty header", []string{""}, "", ""},
+		{"Whitespace only", []string{" \t"}, "", "unable to parse ip () from X-Forwarded-For header"},
+		{"Malformed rightmost hop", []string{"10.0.0.5, invalid"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"},
+		{"Malformed relevant intermediate hop", []string{"10.0.0.5, invalid, 192.0.2.20"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"},
+		{"Malformed leftmost in otherwise trusted chain", []string{"invalid, 192.0.2.20"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"},
+		{"Empty intermediate hop", []string{"10.0.0.5,,192.0.2.20"}, "", "unable to parse ip () from X-Forwarded-For header"},
+		{"Empty last field", []string{"10.0.0.5", ""}, "", "unable to parse ip () from X-Forwarded-For header"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			req := &http.Request{Header: http.Header{}}
+			for _, value := range test.headerValues {
+				req.Header.Add("X-Forwarded-For", value)
+			}
+
+			clientIP, err := GetClientIPFromTrustedProxy(parser, req, trustedProxies)
+			if test.errString != "" {
+				assert.EqualError(t, err, test.errString)
+			} else {
+				assert.NoError(t, err)
+			}
+			assert.Equal(t, net.ParseIP(test.expectedIP), clientIP)
+		})
+	}
+}
+
+func TestGetClientIPFromTrustedProxyConfiguration(t *testing.T) {
+	xff, err := GetRealClientIPParser("X-Forwarded-For")
+	require.NoError(t, err)
+	realIP, err := GetRealClientIPParser("X-Real-IP")
+	require.NoError(t, err)
+	trustAll, err := ParseNetSet([]string{"0.0.0.0/0", "::/0"})
+	require.NoError(t, err)
+
+	tests := []struct {
+		name           string
+		parser         ipapi.RealClientIPParser
+		trustedProxies *NetSet
+		expectedIP     string
+		errString      string
+	}{
+		{"Nil parser", nil, trustAll, "", "real client IP parser is required"},
+		{"Nil trusted proxies", xff, nil, "", "trusted proxy list is required to parse X-Forwarded-For"},
+		{"Empty trusted proxies", xff, NewNetSet(), "2001:db8::23", ""},
+		{"Trust all retains leftmost compatibility", xff, trustAll, "10.0.0.5", ""},
+		{"Single value header", realIP, trustAll, "198.51.100.23", ""},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			req := &http.Request{Header: http.Header{
+				"X-Forwarded-For": {"10.0.0.5, 2001:db8::23"},
+				"X-Real-Ip":       {"198.51.100.23"},
+			}}
+			clientIP, err := GetClientIPFromTrustedProxy(test.parser, req, test.trustedProxies)
+			if test.errString != "" {
+				assert.EqualError(t, err, test.errString)
+			} else {
+				assert.NoError(t, err)
+			}
+			assert.Equal(t, net.ParseIP(test.expectedIP), clientIP)
+		})
+	}
+}
+
 func TestGetRemoteIP(t *testing.T) {
 	tests := []struct {
 		remoteAddr string
@@ -147,6 +235,8 @@ func TestGetRemoteIP(t *testing.T) {
 
 func TestGetClientString(t *testing.T) {
 	p := &xForwardedForClientIPParser{header: http.CanonicalHeaderKey("X-Forwarded-For")}
+	trustedProxies, err := ParseNetSet([]string{"192.0.2.0/24", "2001:db8:1::/48"})
+	require.NoError(t, err)
 
 	tests := []struct {
 		parser             ipapi.RealClientIPParser
@@ -155,7 +245,7 @@ func TestGetClientString(t *testing.T) {
 		expectedClient     string
 		expectedClientFull string
 	}{
-		// Should fail quietly, only printing warnings to the log
+		// Preserve transport-only output when no client address is available.
 		{nil, "", "", "", ""},
 		// Unix domain socket — no IP available
 		{nil, "@", "", "", ""},
@@ -164,6 +254,13 @@ func TestGetClientString(t *testing.T) {
 		{nil, "10.254.244.165:62750", "", "10.254.244.165", "10.254.244.165"},
 		// Parser is nil, the contents of X-Forwarded-For should be ignored in all cases.
 		{nil, "[2001:470:26:307:a5a1:1177:2ae3:e9c3]:48290", "127.0.0.1", "2001:470:26:307:a5a1:1177:2ae3:e9c3", "2001:470:26:307:a5a1:1177:2ae3:e9c3"},
+		{p, "192.0.2.10:443", "10.0.0.5, 198.51.100.23, 192.0.2.20", "198.51.100.23", "192.0.2.10 (198.51.100.23)"},
+		{p, "[2001:db8:1::10]:443", "[2001:db8:2::23]:1234, 2001:db8:1::20", "2001:db8:2::23", "2001:db8:1::10 (2001:db8:2::23)"},
+		{p, "192.0.2.10:443", "10.0.0.5, invalid, 192.0.2.20", "192.0.2.10", "192.0.2.10"},
+		{p, "192.0.2.10:443", "invalid, 198.51.100.23", "198.51.100.23", "192.0.2.10 (198.51.100.23)"},
+		{p, "192.0.2.10:443", "192.0.2.30, 192.0.2.20", "192.0.2.30", "192.0.2.10 (192.0.2.30)"},
+		{p, "@", "198.51.100.23, 192.0.2.20", "198.51.100.23", " (198.51.100.23)"},
+		{p, "@", "invalid", "", ""},
 	}
 
 	for _, test := range tests {
@@ -174,10 +271,10 @@ func TestGetClientString(t *testing.T) {
 			RemoteAddr: test.remoteAddr,
 		}
 
-		client := GetClientString(test.parser, req, false)
+		client := GetClientString(test.parser, req, trustedProxies, false)
 		assert.Equal(t, test.expectedClient, client)
 
-		clientFull := GetClientString(test.parser, req, true)
+		clientFull := GetClientString(test.parser, req, trustedProxies, true)
 		assert.Equal(t, test.expectedClientFull, clientFull)
 	}
 }

pkg/requests/util/util.go

@@ -1,6 +1,7 @@
 package util
 
 import (
+	"errors"
 	"net/http"
 	"net/url"
 	"strings"
@@ -45,35 +46,35 @@ func GetRequestURI(req *http.Request) string {
 	return uri
 }
 
-// GetRequestPath returns the request URI or X-Forwarded-Uri if present and the
-// request came from a trusted reverse proxy but always strips the query
-// parameters and fragment suffixes and only returns the pure path.
-func GetRequestPath(req *http.Request) string {
-	uri := stripRequestFragment(GetRequestURI(req))
-
-	// Parse URI and return only the path component
-	if parsedURL, err := url.Parse(uri); err == nil {
-		return stripRequestFragment(parsedURL.Path)
+// GetRequestPath returns a decoded path suitable for skip-auth matching, using
+// X-Forwarded-Uri only for a trusted reverse proxy. An error means the path must
+// not grant an authentication exemption. It does not modify the request URL.
+func GetRequestPath(req *http.Request) (string, error) {
+	uri := GetRequestURI(req)
+	if !strings.HasPrefix(uri, "/") || strings.ContainsAny(uri, "# \t\r\n") {
+		return "", errors.New("request target is not an unambiguous origin-form URI")
 	}
 
-	// Fallback: strip query parameters manually
-	return stripRequestQuery(uri)
-}
-
-func stripRequestFragment(uri string) string {
-	if idx := strings.Index(uri, "#"); idx != -1 {
-		return uri[:idx]
+	// Unlike url.Parse, ParseRequestURI keeps a leading // in the path.
+	parsedURL, err := url.ParseRequestURI(uri)
+	if err != nil {
+		return "", errors.New("invalid request target")
 	}
-
-	return uri
-}
-
-func stripRequestQuery(uri string) string {
-	if idx := strings.Index(uri, "?"); idx != -1 {
-		return uri[:idx]
+	requestPath := parsedURL.Path
+	if strings.ContainsAny(requestPath, ";\\#?") || strings.Contains(requestPath, "//") {
+		return "", errors.New("request path contains ambiguous separators")
 	}
-
-	return uri
+	for _, char := range requestPath {
+		if char < 0x20 || char == 0x7f {
+			return "", errors.New("request path contains a control character")
+		}
+	}
+	for _, segment := range strings.Split(requestPath, "/") {
+		if segment == "." || segment == ".." {
+			return "", errors.New("request path contains a dot segment")
+		}
+	}
+	return requestPath, nil
 }
 
 // CanTrustForwardedHeaders determines if forwarded headers should be processed

pkg/requests/util/util_test.go

@@ -143,6 +143,93 @@ var _ = Describe("Util Suite", func() {
 	})
 
 	Context("GetRequestPath", func() {
+		DescribeTable("preserves unambiguous paths without changing the request",
+			func(target, expected string) {
+				for _, forwarded := range []bool{false, true} {
+					req = httptest.NewRequest(http.MethodGet, target, nil)
+					if forwarded {
+						req = httptest.NewRequest(http.MethodGet, "/oauth2/auth", nil)
+						req.RemoteAddr = "127.0.0.1:4180"
+						req = middleware.AddRequestScope(req, &middleware.RequestScope{
+							ReverseProxy: true, TrustedProxies: trustedProxies,
+						})
+						req.Header.Set("X-Forwarded-Uri", target)
+					}
+					originalURL, originalTarget := *req.URL, req.RequestURI
+					requestPath, err := util.GetRequestPath(req)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(requestPath).To(Equal(expected))
+					Expect(*req.URL).To(Equal(originalURL))
+					Expect(req.RequestURI).To(Equal(originalTarget))
+				}
+			},
+			Entry("root", "/", "/"),
+			Entry("public path", "/public/file", "/public/file"),
+			Entry("protected path", "/protected/file", "/protected/file"),
+			Entry("trailing slash", "/public/", "/public/"),
+			Entry("query ignored", "/public/file?next=/../protected;v=1&tag=%23", "/public/file"),
+			Entry("invalid query escape does not affect the path", "/public/file?q=%zz", "/public/file"),
+			Entry("escaped space", "/public/a%20b", "/public/a b"),
+			Entry("escaped letter", "/public/%66ile", "/public/file"),
+			Entry("escaped Unicode", "/public/caf%C3%A9", "/public/caf\u00e9"),
+			Entry("escaped slash", "/public%2ffile", "/public/file"),
+			Entry("literal plus", "/public/a+b", "/public/a+b"),
+			Entry("escaped plus", "/public/a%2bb", "/public/a+b"),
+			Entry("escaped percent", "/public/100%25", "/public/100%"),
+			Entry("dot within a segment", "/public/file.txt", "/public/file.txt"),
+			Entry("multiple dots are not a parent segment", "/public/...", "/public/..."),
+		)
+
+		DescribeTable("declines ambiguous or invalid original request targets",
+			func(target string) {
+				req.RemoteAddr = "127.0.0.1:4180"
+				req = middleware.AddRequestScope(req, &middleware.RequestScope{
+					ReverseProxy: true, TrustedProxies: trustedProxies,
+				})
+				req.Header.Set("X-Forwarded-Uri", target)
+				requestPath, err := util.GetRequestPath(req)
+				Expect(err).To(HaveOccurred())
+				Expect(requestPath).To(BeEmpty())
+				Expect(err.Error()).NotTo(ContainSubstring("sensitive-value"))
+			},
+			Entry("literal parent segment", "/public/../protected?token=sensitive-value"),
+			Entry("encoded parent segment", "/public/%2e%2E/protected"),
+			Entry("encoded slash exposes parent segment", "/public%2f..%2fprotected"),
+			Entry("current segment", "/public/./file"),
+			Entry("terminal parent segment", "/public/.."),
+			Entry("terminal current segment", "/public/."),
+			Entry("leading double slash", "//protected/public"),
+			Entry("repeated slash", "/public//file"),
+			Entry("encoded repeated slash", "/public/%2ffile"),
+			Entry("matrix parent segment", "/public/..;/protected"),
+			Entry("encoded matrix parent segment", "/public/%2e%2e%3b/protected"),
+			Entry("valid matrix parameter", "/public/file;version=1"),
+			Entry("matrix parameter containing a slash", "/protected;/public"),
+			Entry("invalid escape", "/public/%zz?token=sensitive-value"),
+			Entry("incomplete escape", "/public/%"),
+			Entry("relative target", "public/file"),
+			Entry("absolute target", "https://example.com/public/file"),
+			Entry("authority target", "example.com:443"),
+			Entry("asterisk target", "*"),
+			Entry("literal fragment", "/public#sensitive-value"),
+			Entry("encoded fragment", "/public%23sensitive-value"),
+			Entry("encoded question mark", "/public%3fsensitive-value"),
+			Entry("backslash", "/public\\..\\protected"),
+			Entry("encoded backslash", "/public%5c..%5cprotected"),
+			Entry("encoded control character", "/public/%00"),
+			Entry("encoded delete character", "/public/%7f"),
+			Entry("unescaped space", "/public/a b"),
+		)
+
+		It("ignores a forged forwarded URI from an untrusted peer", func() {
+			req.RemoteAddr = "192.0.2.10:4180"
+			req = middleware.AddRequestScope(req, &middleware.RequestScope{
+				ReverseProxy: true, TrustedProxies: trustedProxies,
+			})
+			req.Header.Set("X-Forwarded-Uri", "/public")
+			Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams))
+		})
+
 		Context("trusted forwarded headers are disabled", func() {
 			BeforeEach(func() {
 				req = middleware.AddRequestScope(req, &middleware.RequestScope{})
@@ -152,21 +239,25 @@ var _ = Describe("Util Suite", func() {
 				Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams))
 			})
 
-			It("drops fragment content from a parsed request path", func() {
+			It("declines fragment content in a parsed request path", func() {
 				// Simulate net/http ParseRequestURI preserving '#' in URL.Path.
 				req.URL.Path = "/foo/secret#/bar"
 				req.URL.RawPath = "/foo/secret%23/bar"
-				Expect(util.GetRequestPath(req)).To(Equal("/foo/secret"))
+				requestPath, err := util.GetRequestPath(req)
+				Expect(err).To(HaveOccurred())
+				Expect(requestPath).To(BeEmpty())
 			})
 
-			It("drops fragment-like suffixes from encoded number signs", func() {
+			It("declines encoded number signs", func() {
 				req = httptest.NewRequest(
 					http.MethodGet,
 					fmt.Sprintf("%s://%s/foo/secret%%23/bar?query=param", proto, host),
 					nil,
 				)
 				req = middleware.AddRequestScope(req, &middleware.RequestScope{})
-				Expect(util.GetRequestPath(req)).To(Equal("/foo/secret"))
+				requestPath, err := util.GetRequestPath(req)
+				Expect(err).To(HaveOccurred())
+				Expect(requestPath).To(BeEmpty())
 			})
 
 			It("ignores X-Forwarded-Uri and returns the URI (without query params)", func() {
@@ -193,9 +284,11 @@ var _ = Describe("Util Suite", func() {
 				Expect(util.GetRequestPath(req)).To(Equal("/some/other/path"))
 			})
 
-			It("drops fragment-like suffixes from the X-Forwarded-Uri", func() {
+			It("declines fragment-like suffixes from the X-Forwarded-Uri", func() {
 				req.Header.Add("X-Forwarded-Uri", "/foo/secret%23/bar?query=param")
-				Expect(util.GetRequestPath(req)).To(Equal("/foo/secret"))
+				requestPath, err := util.GetRequestPath(req)
+				Expect(err).To(HaveOccurred())
+				Expect(requestPath).To(BeEmpty())
 			})
 		})
 	})

pkg/validation/allowlist.go

@@ -20,7 +20,7 @@ func validateAllowlists(o *options.Options) []string {
 	msgs = append(msgs, validateTrustedIPs(o)...)
 
 	if len(o.TrustedIPs) > 0 && o.ReverseProxy {
-		_, err := fmt.Fprintln(os.Stderr, "WARNING: mixing --trusted-ip with --reverse-proxy is a potential security vulnerability. An attacker can inject a trusted IP into an X-Real-IP or X-Forwarded-For header if they aren't properly protected outside of oauth2-proxy")
+		_, err := fmt.Fprintln(os.Stderr, "WARNING: --trusted-ip bypasses authentication. When using it with --reverse-proxy, configure --trusted-proxy-ip and ensure the proxy overwrites the selected real-client-IP header (or safely appends X-Forwarded-For).")
 		if err != nil {
 			panic(err)
 		}

pkg/validation/options.go

@@ -8,6 +8,7 @@ import (
 	"net/url"
 	"strings"
 
+	middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/hmacauth"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
@@ -89,7 +90,15 @@ func Validate(o *options.Options) error {
 
 		// Allow the logger to get client IPs
 		logger.SetGetClientFunc(func(r *http.Request) string {
-			return ip.GetClientString(o.GetRealClientIPParser(), r, false)
+			parser := o.GetRealClientIPParser()
+			var trustedProxies *ip.NetSet
+			scope := middlewareapi.GetRequestScope(r)
+			if scope != nil && scope.CanTrustForwardedHeaders(r) {
+				trustedProxies = scope.TrustedProxies
+			} else {
+				parser = nil
+			}
+			return ip.GetClientString(parser, r, trustedProxies, false)
 		})
 	}
 

pkg/validation/options_test.go

@@ -1,16 +1,24 @@
 package validation
 
 import (
+	"bytes"
 	"crypto"
+	"net/http"
+	"net/http/httptest"
 	"net/url"
 	"os"
 	"strings"
 	"testing"
 	"time"
 
+	middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr"
+	"github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -224,6 +232,65 @@ func TestRealClientIPHeader(t *testing.T) {
 	assert.Nil(t, o.GetRealClientIPParser())
 }
 
+func TestRealClientIPLogging(t *testing.T) {
+	t.Cleanup(func() {
+		logger.SetOutput(ginkgo.GinkgoWriter)
+		logger.SetAuthTemplate(logger.DefaultAuthLoggingFormat)
+		logger.SetReqTemplate(logger.DefaultRequestLoggingFormat)
+		logger.SetGetClientFunc(func(r *http.Request) string { return r.RemoteAddr })
+	})
+	tests := []struct {
+		name         string
+		remoteAddr   string
+		headerValues []string
+		reverseProxy bool
+		nilParser    bool
+		expected     string
+	}{
+		{"Untrusted peer", "198.51.100.23:1234", []string{"10.0.0.5"}, true, false, "198.51.100.23"},
+		{"Appending trusted proxy", "192.0.2.10:1234", []string{"10.0.0.5, 198.51.100.23"}, true, false, "198.51.100.23"},
+		{"Repeated fields and multiple proxies", "192.0.2.10:1234", []string{"10.0.0.5", "198.51.100.23, 192.0.2.20", "192.0.2.21"}, true, false, "198.51.100.23"},
+		{"Legitimate client", "192.0.2.10:1234", []string{"10.0.0.5, 192.0.2.20"}, true, false, "10.0.0.5"},
+		{"Missing header", "192.0.2.10:1234", nil, true, false, "192.0.2.10"},
+		{"Malformed relevant hop", "192.0.2.10:1234", []string{"10.0.0.5, invalid, 192.0.2.20"}, true, false, "192.0.2.10"},
+		{"Nil parser", "192.0.2.10:1234", []string{"10.0.0.5"}, true, true, "192.0.2.10"},
+		{"Reverse proxy disabled in scope", "192.0.2.10:1234", []string{"10.0.0.5"}, false, false, "192.0.2.10"},
+		{"IPv6", "[2001:db8:1::10]:1234", []string{"10.0.0.5", " [2001:db8:2::23]:443 , 2001:db8:1::20"}, true, false, "2001:db8:2::23"},
+		{"Unix socket", "@", []string{"10.0.0.5, 198.51.100.23"}, true, false, "198.51.100.23"},
+		{"Entire chain trusted", "192.0.2.10:1234", []string{"192.0.2.30, 192.0.2.20"}, true, false, "192.0.2.30"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			o := testOptions()
+			o.ReverseProxy = true
+			o.RealClientIPHeader = "X-Forwarded-For"
+			o.TrustedProxyIPs = []string{"192.0.2.0/24", "2001:db8:1::/48"}
+			o.Logging.AuthFormat = "{{.Client}}"
+			o.Logging.RequestFormat = "{{.Client}}"
+			require.NoError(t, Validate(o))
+			if test.nilParser {
+				o.SetRealClientIPParser(nil)
+			}
+			trustedProxies, err := ip.ParseNetSet(o.TrustedProxyIPs)
+			require.NoError(t, err)
+			req := httptest.NewRequest(http.MethodGet, "/", nil)
+			req.RemoteAddr = test.remoteAddr
+			for _, value := range test.headerValues {
+				req.Header.Add("X-Forwarded-For", value)
+			}
+			req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{
+				ReverseProxy:   test.reverseProxy,
+				TrustedProxies: trustedProxies,
+			})
+			var output bytes.Buffer
+			logger.SetOutput(&output)
+			logger.PrintAuthf("", req, logger.AuthFailure, "denied")
+			logger.PrintReq("", "", req, *req.URL, time.Now(), http.StatusForbidden, 0)
+			assert.Equal(t, test.expected+"\n"+test.expected+"\n", output.String())
+		})
+	}
+}
+
 func TestProviderCAFilesError(t *testing.T) {
 	file, err := os.CreateTemp("", "absent.*.crt")
 	assert.NoError(t, err)