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: