Merge 7361885fae into 1f049e5ebd

CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ## Changes since v7.15.3
 
+- [#2940](https://github.com/oauth2-proxy/oauth2-proxy/issues/2940) fix: preserve originally requested URL in `forward_auth`/`auth_request` redirects when the host is not whitelisted (@zhaoxinyi02)
+
 # V7.15.3
 
 ## Release Highlights

pkg/app/redirect/director_test.go

@@ -206,3 +206,96 @@ var _ = Describe("Director Suite", func() {
 		Expect(redirect).To(Equal("/foo?bar"))
 	})
 })
+
+var _ = Describe("forward_auth redirect preservation (issue #2940)", func() {
+	// These tests model the Caddy `forward_auth` (and nginx `auth_request`)
+	// flow: when /oauth2/auth returns 401, the reverse proxy redirects the
+	// client to /oauth2/sign_in?rd={scheme}://{host}{uri}. The {uri} includes
+	// the originally requested path and query, so `rd` is an absolute URL for
+	// the same host the client used to reach the proxy.
+	//
+	// The Caddy integration docs require --reverse-proxy=true but do not mention
+	// --whitelist-domain. Without it the absolute `rd` used to be rejected and,
+	// because sign_in/start are served under the proxy prefix, the redirect
+	// collapsed to "/", silently losing the originally requested URL (and its
+	// query) after login.
+	var (
+		appDirector    AppDirector
+		trustedProxies *ip.NetSet
+	)
+
+	BeforeEach(func() {
+		var err error
+		trustedProxies, err = ip.ParseNetSet([]string{"127.0.0.1"})
+		Expect(err).ToNot(HaveOccurred())
+		appDirector = NewAppDirector(AppDirectorOpts{
+			ProxyPrefix: testProxyPrefix,
+			// No --whitelist-domain configured, matching the Caddy docs.
+			Validator: NewValidator([]string{}),
+		})
+	})
+
+	// makeSignInRequest builds the /oauth2/sign_in request that Caddy's
+	// `redir * /oauth2/sign_in?rd={scheme}://{host}{uri}` produces for an
+	// original request on the given host, with `rd` set to rd.
+	makeSignInRequest := func(host, rd string) *http.Request {
+		req, _ := http.NewRequest("GET", "https://oauth2-proxy.internal/oauth2/sign_in?rd="+rd, nil)
+		req.Header.Set("X-Forwarded-Proto", "http")
+		req.Header.Set("X-Forwarded-Host", host)
+		// Caddy's `reverse_proxy /oauth2/* { header_up X-Forwarded-Uri {uri} }`
+		// sets X-Forwarded-Uri to the sign_in request's own URI, which is under
+		// the proxy prefix.
+		req.Header.Set("X-Forwarded-Uri", "/oauth2/sign_in?rd="+rd)
+		req.RemoteAddr = "127.0.0.1:4180"
+		req = middleware.AddRequestScope(req, &middleware.RequestScope{
+			ReverseProxy:   true,
+			TrustedProxies: trustedProxies,
+		})
+		return req
+	}
+
+	It("preserves the originally requested path and query from a same-host absolute rd", func() {
+		// Original request was /echo/foo?bar=baz on localhost.
+		req := makeSignInRequest("localhost", "http://localhost/echo/foo?bar=baz")
+		redirect, err := appDirector.GetRedirect(req)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(redirect).To(Equal("/echo/foo?bar=baz"))
+	})
+
+	It("preserves the path when the same-host rd includes a port", func() {
+		req := makeSignInRequest("localhost:8080", "http://localhost:8080/echo/foo?bar=baz")
+		redirect, err := appDirector.GetRedirect(req)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(redirect).To(Equal("/echo/foo?bar=baz"))
+	})
+
+	It("preserves the absolute rd as-is when the host is whitelisted", func() {
+		whitelisted := NewAppDirector(AppDirectorOpts{
+			ProxyPrefix: testProxyPrefix,
+			Validator:   NewValidator([]string{"localhost"}),
+		})
+		req := makeSignInRequest("localhost", "http://localhost/echo/foo?bar=baz")
+		redirect, err := whitelisted.GetRedirect(req)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(redirect).To(Equal("http://localhost/echo/foo?bar=baz"))
+	})
+
+	It("does not redirect to a different, non-whitelisted host's path", func() {
+		// rd points at evil.example.com while the request was served on
+		// localhost: the path must not be extracted and the redirect must not
+		// leak the attacker-controlled host.
+		req := makeSignInRequest("localhost", "http://evil.example.com/echo/foo")
+		redirect, err := appDirector.GetRedirect(req)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(redirect).To(Equal("/"))
+	})
+
+	It("still rejects open-redirect rd values", func() {
+		// Extracted paths are re-validated, so protocol-relative and other
+		// open-redirect payloads are rejected even when the host matches.
+		req := makeSignInRequest("localhost", "//evil.example.com/path")
+		redirect, err := appDirector.GetRedirect(req)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(redirect).To(Equal("/"))
+	})
+})

pkg/app/redirect/getters.go

@@ -3,6 +3,8 @@ package redirect
 import (
 	"fmt"
 	"net/http"
+	"net/url"
+	"strings"
 
 	requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util"
 )
@@ -14,10 +16,51 @@ type redirectGetter func(req *http.Request) string
 // getRdQuerystringRedirect handles this getAppRedirect strategy:
 // - `rd` querysting parameter
 func (a *appDirector) getRdQuerystringRedirect(req *http.Request) string {
-	return a.validateRedirect(
+	rd := req.Form.Get("rd")
-		req.Form.Get("rd"),
+	redirect := a.validateRedirect(rd,
 		"Invalid redirect provided in rd querystring parameter: %s",
 	)
+	if redirect != "" {
+		return redirect
+	}
+
+	// If `rd` is an absolute http(s) URL that was rejected because its host is
+	// not on the whitelist, fall back to its path component when it targets the
+	// same host the request was served on. See getRdPathRedirect for details.
+	return a.getRdPathRedirect(req, rd)
+}
+
+// getRdPathRedirect extracts the path (and query) of an absolute http(s) `rd`
+// redirect and validates it as a relative redirect, but only when the `rd`
+// URL's host matches the host the request was served on.
+//
+// Reverse-proxy / forward_auth setups (e.g. Caddy `forward_auth` and nginx
+// `auth_request`) build the post-login redirect as `{scheme}://{host}{uri}`,
+// an absolute URL for the same host the client used to reach the proxy. The
+// Caddy integration docs do not mention that such hosts must also be added to
+// `--whitelist-domain`, so without that option the absolute `rd` is rejected
+// and, because the `sign_in`/`start` requests are served under the proxy
+// prefix, the remaining redirect strategies collapse to "/" — silently losing
+// the originally requested URL (and its query) after login.
+//
+// Falling back to the path component is safe: it is a same-origin relative
+// redirect, it is still run through the validator (so open-redirect protections
+// such as "//" and "/../" still apply), and a `rd` pointing at a different,
+// non-whitelisted host is left untouched so the other strategies can run.
+func (a *appDirector) getRdPathRedirect(req *http.Request, rd string) string {
+	if rd == "" || (!strings.HasPrefix(rd, "http://") && !strings.HasPrefix(rd, "https://")) {
+		return ""
+	}
+	rdURL, err := url.Parse(rd)
+	if err != nil || rdURL.Host == "" {
+		return ""
+	}
+	if rdURL.Host != requestutil.GetRequestHost(req) {
+		return ""
+	}
+	return a.validateRedirect(rdURL.RequestURI(),
+		"Invalid redirect extracted from rd querystring parameter: %s",
+	)
 }
 
 // getXAuthRequestRedirect handles this getAppRedirect strategy: