From 7361885fae459cd5e5940948594dccebd952ca6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B5=B5=E9=91=AB=E4=BA=BF?= <98445030+zhaoxinyi02@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:16:00 +0800 Subject: [PATCH] fix(redirect): preserve requested URL in forward_auth flows without whitelist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In reverse-proxy / forward_auth setups (Caddy `forward_auth`, nginx `auth_request`), the post-login redirect is built as an absolute URL `{scheme}://{host}{uri}` and passed to oauth2-proxy via the `rd` query parameter. The Caddy integration docs require `--reverse-proxy=true` but do not mention that the host must also be added to `--whitelist-domain`. Without `--whitelist-domain`, the absolute `rd` was rejected by the redirect validator and, because the `sign_in`/`start` requests are served under the proxy prefix, the remaining redirect strategies collapsed to "/", silently losing the originally requested URL (and its query) after login. When `rd` is an absolute http(s) URL that fails whitelist validation but targets the same host the request was served on, fall back to its path component and re-validate it as a relative redirect. This is safe: - it is a same-origin relative redirect, so it cannot redirect to a different host; - the extracted path is still run through the validator, so open-redirect protections (e.g. "//", "/../") still apply; - a `rd` pointing at a different, non-whitelisted host is left untouched, so the other redirect strategies still run unchanged. Regression tests model the Caddy `forward_auth` sign_in request and verify that the originally requested path+query is preserved without a whitelist, that a different non-whitelisted host is still rejected, and that open-redirect payloads are still blocked. Fixes #2940 Co-Authored-By: Claude Signed-off-by: 赵鑫亿 <98445030+zhaoxinyi02@users.noreply.github.com> --- CHANGELOG.md | 2 + pkg/app/redirect/director_test.go | 93 +++++++++++++++++++++++++++++++ pkg/app/redirect/getters.go | 47 +++++++++++++++- 3 files changed, 140 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 788e82c2..c59aa8e4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/app/redirect/director_test.go b/pkg/app/redirect/director_test.go index 58d0e8c3..94c53db1 100644 --- a/pkg/app/redirect/director_test.go +++ b/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("/")) + }) +}) diff --git a/pkg/app/redirect/getters.go b/pkg/app/redirect/getters.go index 5240abeb..29ab2613 100644 --- a/pkg/app/redirect/getters.go +++ b/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( - req.Form.Get("rd"), + 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: