Merge pull request #1226 from oauth2-proxy/app-redirect

Move app redirection logic to its own package

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Changes since v7.1.3
 
+- [#1226](https://github.com/oauth2-proxy/oauth2-proxy/pull/1226) Move app redirection logic to its own package (@JoelSpeed)
 - [#1128](https://github.com/oauth2-proxy/oauth2-proxy/pull/1128) Use gorilla mux for OAuth Proxy routing (@JoelSpeed)
 - [#1238](https://github.com/oauth2-proxy/oauth2-proxy/pull/1238) Added ADFS provider (@samirachoadi)
 - [#1227](https://github.com/oauth2-proxy/oauth2-proxy/pull/1227) Fix Refresh Session not working for multiple cookies (@rishi1111)

oauthproxy.go

@@ -22,6 +22,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 	sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/pagewriter"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/redirect"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies"
 	proxyhttp "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/http"
@@ -55,10 +56,6 @@ var (
 
 	// ErrAccessDenied means the user should receive a 401 Unauthorized response
 	ErrAccessDenied = errors.New("access denied")
-
-	// Used to check final redirects are not susceptible to open redirects.
-	// Matches //, /\ and both of these with whitespace in between (eg / / or / \).
-	invalidRedirectRegex = regexp.MustCompile(`[/\\](?:[\s\v]*|\.{1,2})[/\\]`)
 )
 
 // allowedRoute manages method + path based allowlists
@@ -94,6 +91,8 @@ type OAuthProxy struct {
 	server            proxyhttp.Server
 	upstreamProxy     http.Handler
 	serveMux          *mux.Router
+	redirectValidator redirect.Validator
+	appDirector       redirect.AppDirector
 }
 
 // NewOAuthProxy creates a new instance of OAuthProxy from the options provided
@@ -176,6 +175,12 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 		return nil, fmt.Errorf("could not build headers chain: %v", err)
 	}
 
+	redirectValidator := redirect.NewValidator(opts.WhitelistDomains)
+	appDirector := redirect.NewAppDirector(redirect.AppDirectorOpts{
+		ProxyPrefix: opts.ProxyPrefix,
+		Validator:   redirectValidator,
+	})
+
 	p := &OAuthProxy{
 		CookieOptions: &opts.Cookie,
 		Validator:     validator,
@@ -200,6 +205,8 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 		preAuthChain:       preAuthChain,
 		pageWriter:         pageWriter,
 		upstreamProxy:      upstreamProxy,
+		redirectValidator:  redirectValidator,
+		appDirector:        appDirector,
 	}
 	p.buildServeMux(opts.ProxyPrefix)
 
@@ -465,59 +472,13 @@ func (p *OAuthProxy) SaveSession(rw http.ResponseWriter, req *http.Request, s *s
 	return p.sessionStore.Save(rw, req, s)
 }
 
-// IsValidRedirect checks whether the redirect URL is whitelisted
-func (p *OAuthProxy) IsValidRedirect(redirect string) bool {
-	switch {
-	case redirect == "":
-		// The user didn't specify a redirect, should fallback to `/`
-		return false
-	case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !invalidRedirectRegex.MatchString(redirect):
-		return true
-	case strings.HasPrefix(redirect, "http://") || strings.HasPrefix(redirect, "https://"):
-		redirectURL, err := url.Parse(redirect)
-		if err != nil {
-			logger.Printf("Rejecting invalid redirect %q: scheme unsupported or missing", redirect)
-			return false
-		}
-		redirectHostname := redirectURL.Hostname()
-
-		for _, allowedDomain := range p.whitelistDomains {
-			allowedHost, allowedPort := splitHostPort(allowedDomain)
-			if allowedHost == "" {
-				continue
-			}
-
-			if redirectHostname == strings.TrimPrefix(allowedHost, ".") ||
-				(strings.HasPrefix(allowedHost, ".") &&
-					strings.HasSuffix(redirectHostname, allowedHost)) {
-				// the domain names match, now validate the ports
-				// if the whitelisted domain's port is '*', allow all ports
-				// if the whitelisted domain contains a specific port, only allow that port
-				// if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https
-				redirectPort := redirectURL.Port()
-				if allowedPort == "*" ||
-					allowedPort == redirectPort ||
-					(allowedPort == "" && redirectPort == "") {
-					return true
-				}
-			}
-		}
-
-		logger.Printf("Rejecting invalid redirect %q: domain / port not in whitelist", redirect)
-		return false
-	default:
-		logger.Printf("Rejecting invalid redirect %q: not an absolute or relative URL", redirect)
-		return false
-	}
-}
-
 func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	p.serveMux.ServeHTTP(rw, req)
 }
 
 // ErrorPage writes an error response
 func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string, messages ...interface{}) {
-	redirectURL, err := p.getAppRedirect(req)
+	redirectURL, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
 	}
@@ -582,7 +543,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code
 	}
 	rw.WriteHeader(code)
 
-	redirectURL, err := p.getAppRedirect(req)
+	redirectURL, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -617,7 +578,7 @@ func (p *OAuthProxy) ManualSignIn(req *http.Request) (string, bool) {
 
 // SignIn serves a page prompting users to sign in
 func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) {
-	redirect, err := p.getAppRedirect(req)
+	redirect, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -681,7 +642,7 @@ func (p *OAuthProxy) UserInfo(rw http.ResponseWriter, req *http.Request) {
 
 // SignOut sends a response to clear the authentication cookie
 func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) {
-	redirect, err := p.getAppRedirect(req)
+	redirect, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -707,7 +668,7 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	appRedirect, err := p.getAppRedirect(req)
+	appRedirect, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining application redirect: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -790,7 +751,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 	csrf.SetSessionNonce(session)
 	p.provider.ValidateSession(req.Context(), session)
 
-	if !p.IsValidRedirect(appRedirect) {
+	if !p.redirectValidator.IsValidRedirect(appRedirect) {
 		appRedirect = "/"
 	}
 
@@ -947,158 +908,6 @@ func (p *OAuthProxy) getOAuthRedirectURI(req *http.Request) string {
 	return rd.String()
 }
 
-// getAppRedirect determines the full URL or URI path to redirect clients to
-// once authenticated with the OAuthProxy
-// Strategy priority (first legal result is used):
-// - `rd` querysting parameter
-// - `X-Auth-Request-Redirect` header
-// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
-// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
-// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
-// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
-// - `/`
-func (p *OAuthProxy) getAppRedirect(req *http.Request) (string, error) {
-	err := req.ParseForm()
-	if err != nil {
-		return "", err
-	}
-
-	// These redirect getter functions are strategies ordered by priority
-	// for figuring out the redirect URL.
-	type redirectGetter func(req *http.Request) string
-	for _, rdGetter := range []redirectGetter{
-		p.getRdQuerystringRedirect,
-		p.getXAuthRequestRedirect,
-		p.getXForwardedHeadersRedirect,
-		p.getURIRedirect,
-	} {
-		redirect := rdGetter(req)
-		// Call `p.IsValidRedirect` again here a final time to be safe
-		if redirect != "" && p.IsValidRedirect(redirect) {
-			return redirect, nil
-		}
-	}
-
-	return "/", nil
-}
-
-func isForwardedRequest(req *http.Request) bool {
-	return requestutil.IsProxied(req) &&
-		req.Host != requestutil.GetRequestHost(req)
-}
-
-func (p *OAuthProxy) hasProxyPrefix(path string) bool {
-	return strings.HasPrefix(path, fmt.Sprintf("%s/", p.ProxyPrefix))
-}
-
-func (p *OAuthProxy) validateRedirect(redirect string, errorFormat string) string {
-	if p.IsValidRedirect(redirect) {
-		return redirect
-	}
-	if redirect != "" {
-		logger.Errorf(errorFormat, redirect)
-	}
-	return ""
-}
-
-// getRdQuerystringRedirect handles this getAppRedirect strategy:
-// - `rd` querysting parameter
-func (p *OAuthProxy) getRdQuerystringRedirect(req *http.Request) string {
-	return p.validateRedirect(
-		req.Form.Get("rd"),
-		"Invalid redirect provided in rd querystring parameter: %s",
-	)
-}
-
-// getXAuthRequestRedirect handles this getAppRedirect strategy:
-// - `X-Auth-Request-Redirect` Header
-func (p *OAuthProxy) getXAuthRequestRedirect(req *http.Request) string {
-	return p.validateRedirect(
-		req.Header.Get("X-Auth-Request-Redirect"),
-		"Invalid redirect provided in X-Auth-Request-Redirect header: %s",
-	)
-}
-
-// getXForwardedHeadersRedirect handles these getAppRedirect strategies:
-// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
-// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
-func (p *OAuthProxy) getXForwardedHeadersRedirect(req *http.Request) string {
-	if !isForwardedRequest(req) {
-		return ""
-	}
-
-	uri := requestutil.GetRequestURI(req)
-	if p.hasProxyPrefix(uri) {
-		uri = "/"
-	}
-
-	redirect := fmt.Sprintf(
-		"%s://%s%s",
-		requestutil.GetRequestProto(req),
-		requestutil.GetRequestHost(req),
-		uri,
-	)
-
-	return p.validateRedirect(redirect,
-		"Invalid redirect generated from X-Forwarded-* headers: %s")
-}
-
-// getURIRedirect handles these getAppRedirect strategies:
-// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
-// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
-// - `/`
-func (p *OAuthProxy) getURIRedirect(req *http.Request) string {
-	redirect := p.validateRedirect(
-		requestutil.GetRequestURI(req),
-		"Invalid redirect generated from X-Forwarded-Uri header: %s",
-	)
-	if redirect == "" {
-		redirect = req.URL.RequestURI()
-	}
-
-	if p.hasProxyPrefix(redirect) {
-		return "/"
-	}
-	return redirect
-}
-
-// splitHostPort separates host and port. If the port is not valid, it returns
-// the entire input as host, and it doesn't check the validity of the host.
-// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric.
-// *** taken from net/url, modified validOptionalPort() to accept ":*"
-func splitHostPort(hostport string) (host, port string) {
-	host = hostport
-
-	colon := strings.LastIndexByte(host, ':')
-	if colon != -1 && validOptionalPort(host[colon:]) {
-		host, port = host[:colon], host[colon+1:]
-	}
-
-	if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
-		host = host[1 : len(host)-1]
-	}
-
-	return
-}
-
-// validOptionalPort reports whether port is either an empty string
-// or matches /^:\d*$/
-// *** taken from net/url, modified to accept ":*"
-func validOptionalPort(port string) bool {
-	if port == "" || port == ":*" {
-		return true
-	}
-	if port[0] != ':' {
-		return false
-	}
-	for _, b := range port[1:] {
-		if b < '0' || b > '9' {
-			return false
-		}
-	}
-	return true
-}
-
 // getAuthenticatedSession checks whether a user is authenticated and returns a session object and nil error if so
 // Returns:
 // - `nil, ErrNeedsLogin` if user needs to login.

oauthproxy_test.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"bufio"
 	"context"
 	"crypto"
 	"encoding/base64"
@@ -11,7 +10,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"os"
 	"regexp"
 	"strings"
 	"testing"
@@ -19,7 +17,6 @@ import (
 
 	"github.com/coreos/go-oidc"
 	"github.com/mbland/hmacauth"
-	"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/apis/sessions"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies"
@@ -28,10 +25,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/upstream"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/validation"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/providers"
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -63,304 +57,6 @@ func TestRobotsTxt(t *testing.T) {
 	assert.Equal(t, "User-agent: *\nDisallow: /\n", rw.Body.String())
 }
 
-func TestIsValidRedirect(t *testing.T) {
-	opts := baseTestOptions()
-	// Should match domains that are exactly foo.bar and any subdomain of bar.foo
-	opts.WhitelistDomains = []string{
-		"foo.bar",
-		".bar.foo",
-		"port.bar:8080",
-		".sub.port.bar:8080",
-		"anyport.bar:*",
-		".sub.anyport.bar:*",
-	}
-	err := validation.Validate(opts)
-	assert.NoError(t, err)
-
-	proxy, err := NewOAuthProxy(opts, func(string) bool { return true })
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	testCases := []struct {
-		Desc, Redirect string
-		ExpectedResult bool
-	}{
-		{
-			Desc:           "noRD",
-			Redirect:       "",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "singleSlash",
-			Redirect:       "/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "doubleSlash",
-			Redirect:       "//redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "validHTTP",
-			Redirect:       "http://foo.bar/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validHTTPS",
-			Redirect:       "https://foo.bar/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "invalidHTTPSubdomain",
-			Redirect:       "http://baz.foo.bar/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidHTTPSSubdomain",
-			Redirect:       "https://baz.foo.bar/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "validHTTPSubdomain",
-			Redirect:       "http://baz.bar.foo/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validHTTPSSubdomain",
-			Redirect:       "https://baz.bar.foo/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validHTTPDomain",
-			Redirect:       "http://bar.foo/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "invalidHTTP1",
-			Redirect:       "http://foo.bar.evil.corp/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidHTTPS1",
-			Redirect:       "https://foo.bar.evil.corp/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidHTTP2",
-			Redirect:       "http://evil.corp/redirect?rd=foo.bar",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidHTTPS2",
-			Redirect:       "https://evil.corp/redirect?rd=foo.bar",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidPort",
-			Redirect:       "https://evil.corp:3838/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidEmptyPort",
-			Redirect:       "http://foo.bar:3838/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "invalidEmptyPortSubdomain",
-			Redirect:       "http://baz.bar.foo:3838/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "validSpecificPort",
-			Redirect:       "http://port.bar:8080/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "invalidSpecificPort",
-			Redirect:       "http://port.bar:3838/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "validSpecificPortSubdomain",
-			Redirect:       "http://foo.sub.port.bar:8080/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "invalidSpecificPortSubdomain",
-			Redirect:       "http://foo.sub.port.bar:3838/redirect",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "validAnyPort1",
-			Redirect:       "http://anyport.bar:8080/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validAnyPort2",
-			Redirect:       "http://anyport.bar:8081/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validAnyPortSubdomain1",
-			Redirect:       "http://a.sub.anyport.bar:8080/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "validAnyPortSubdomain2",
-			Redirect:       "http://a.sub.anyport.bar:8081/redirect",
-			ExpectedResult: true,
-		},
-		{
-			Desc:           "openRedirect1",
-			Redirect:       "/\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectSpace1",
-			Redirect:       "/ /evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectSpace2",
-			Redirect:       "/ \\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectTab1",
-			Redirect:       "/\t/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectTab2",
-			Redirect:       "/\t\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectVerticalTab1",
-			Redirect:       "/\v/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectVerticalTab2",
-			Redirect:       "/\v\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectNewLine1",
-			Redirect:       "/\n/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectNewLine2",
-			Redirect:       "/\n\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectCarriageReturn1",
-			Redirect:       "/\r/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectCarriageReturn2",
-			Redirect:       "/\r\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectTripleTab",
-			Redirect:       "/\t\t/\t/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectTripleTab2",
-			Redirect:       "/\t\t\\\t/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectQuadTab1",
-			Redirect:       "/\t\t/\t\t\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectQuadTab2",
-			Redirect:       "/\t\t\\\t\t/evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectPeriod1",
-			Redirect:       "/./\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectPeriod2",
-			Redirect:       "/./../../\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectDoubleTab",
-			Redirect:       "/\t/\t\\evil.com",
-			ExpectedResult: false,
-		},
-		{
-			Desc:           "openRedirectPartialSubdomain",
-			Redirect:       "http://evilbar.foo",
-			ExpectedResult: false,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.Desc, func(t *testing.T) {
-			result := proxy.IsValidRedirect(tc.Redirect)
-
-			if result != tc.ExpectedResult {
-				t.Errorf("expected %t got %t", tc.ExpectedResult, result)
-			}
-		})
-	}
-}
-
-var _ = Describe("OpenRedirect Tests", func() {
-	var proxy *OAuthProxy
-
-	BeforeEach(func() {
-		opts := baseTestOptions()
-		// Should match domains that are exactly foo.bar and any subdomain of bar.foo
-		opts.WhitelistDomains = []string{
-			"foo.bar",
-			".bar.foo",
-			"port.bar:8080",
-			".sub.port.bar:8080",
-			"anyport.bar:*",
-			".sub.anyport.bar:*",
-			"www.whitelisteddomain.tld",
-		}
-		Expect(validation.Validate(opts)).To(Succeed())
-
-		var err error
-		proxy, err = NewOAuthProxy(opts, func(string) bool { return true })
-		Expect(err).ToNot(HaveOccurred())
-	})
-
-	file, err := os.Open("./testdata/openredirects.txt")
-	Expect(err).ToNot(HaveOccurred())
-	defer func() {
-		Expect(file.Close()).To(Succeed())
-	}()
-
-	scanner := bufio.NewScanner(file)
-	for scanner.Scan() {
-		rd := scanner.Text()
-		It(rd, func() {
-			rdUnescaped, err := url.QueryUnescape(rd)
-			Expect(err).ToNot(HaveOccurred())
-
-			Expect(proxy.IsValidRedirect(rdUnescaped)).To(BeFalse(), "Expected redirect not to be valid")
-		})
-	}
-
-	Expect(scanner.Err()).ToNot(HaveOccurred())
-})
-
 type TestProvider struct {
 	*providers.ProviderData
 	EmailAddress   string
@@ -1770,165 +1466,6 @@ func TestRequestSignature(t *testing.T) {
 	}
 }
 
-func Test_getAppRedirect(t *testing.T) {
-	opts := baseTestOptions()
-	opts.WhitelistDomains = append(opts.WhitelistDomains, ".example.com", ".example.com:8443")
-	err := validation.Validate(opts)
-	assert.NoError(t, err)
-	require.NotEmpty(t, opts.ProxyPrefix)
-	proxy, err := NewOAuthProxy(opts, func(s string) bool { return false })
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	tests := []struct {
-		name             string
-		url              string
-		headers          map[string]string
-		reverseProxy     bool
-		expectedRedirect string
-	}{
-		{
-			name:             "request outside of ProxyPrefix redirects to original URL",
-			url:              "/foo/bar",
-			headers:          nil,
-			reverseProxy:     false,
-			expectedRedirect: "/foo/bar",
-		},
-		{
-			name:             "request with query preserves query",
-			url:              "/foo?bar",
-			headers:          nil,
-			reverseProxy:     false,
-			expectedRedirect: "/foo?bar",
-		},
-		{
-			name:             "request under ProxyPrefix redirects to root",
-			url:              proxy.ProxyPrefix + "/foo/bar",
-			headers:          nil,
-			reverseProxy:     false,
-			expectedRedirect: "/",
-		},
-		{
-			name: "proxied request outside of ProxyPrefix redirects to proxied URL",
-			url:  "https://oauth.example.com/foo/bar",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "a-service.example.com",
-				"X-Forwarded-Uri":   "/foo/bar",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com/foo/bar",
-		},
-		{
-			name: "non-proxied request with spoofed proxy headers wouldn't redirect",
-			url:  "https://oauth.example.com/foo?bar",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "a-service.example.com",
-				"X-Forwarded-Uri":   "/foo/bar",
-			},
-			reverseProxy:     false,
-			expectedRedirect: "/foo?bar",
-		},
-		{
-			name: "proxied request under ProxyPrefix redirects to root",
-			url:  "https://oauth.example.com" + proxy.ProxyPrefix + "/foo/bar",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "a-service.example.com",
-				"X-Forwarded-Uri":   proxy.ProxyPrefix + "/foo/bar",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com/",
-		},
-		{
-			name: "proxied request with port under ProxyPrefix redirects to root",
-			url:  "https://oauth.example.com" + proxy.ProxyPrefix + "/foo/bar",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "a-service.example.com:8443",
-				"X-Forwarded-Uri":   proxy.ProxyPrefix + "/foo/bar",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com:8443/",
-		},
-		{
-			name: "proxied request with missing uri header would still redirect to desired redirect",
-			url:  "https://oauth.example.com/foo?bar",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "a-service.example.com",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com/foo?bar",
-		},
-		{
-			name:             "request with headers proxy not being set (and reverse proxy enabled) would still redirect to desired redirect",
-			url:              "https://oauth.example.com/foo?bar",
-			headers:          nil,
-			reverseProxy:     true,
-			expectedRedirect: "/foo?bar",
-		},
-		{
-			name: "proxied request with X-Auth-Request-Redirect being set outside of ProxyPrefix redirects to proxied URL",
-			url:  "https://oauth.example.com/foo/bar",
-			headers: map[string]string{
-				"X-Auth-Request-Redirect": "https://a-service.example.com/foo/bar",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com/foo/bar",
-		},
-		{
-			name:             "proxied request with rd query string redirects to proxied URL",
-			url:              "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbar",
-			headers:          nil,
-			reverseProxy:     false,
-			expectedRedirect: "https://a-service.example.com/foo/bar",
-		},
-		{
-			name: "proxied request with rd query string and all headers set (and reverse proxy not enabled) redirects to proxied URL on rd query string",
-			url:  "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
-			headers: map[string]string{
-				"X-Auth-Request-Redirect": "https://a-service.example.com/foo/baz",
-				"X-Forwarded-Proto":       "http",
-				"X-Forwarded-Host":        "another-service.example.com",
-				"X-Forwarded-Uri":         "/seasons/greetings",
-			},
-			reverseProxy:     false,
-			expectedRedirect: "https://a-service.example.com/foo/jazz",
-		},
-		{
-			name: "proxied request with rd query string and some headers set redirects to proxied URL on rd query string",
-			url:  "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbaz",
-			headers: map[string]string{
-				"X-Forwarded-Proto": "https",
-				"X-Forwarded-Host":  "another-service.example.com",
-				"X-Forwarded-Uri":   "/seasons/greetings",
-			},
-			reverseProxy:     true,
-			expectedRedirect: "https://a-service.example.com/foo/baz",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			req, _ := http.NewRequest("GET", tt.url, nil)
-			for header, value := range tt.headers {
-				if value != "" {
-					req.Header.Add(header, value)
-				}
-			}
-			req = middleware.AddRequestScope(req, &middleware.RequestScope{
-				ReverseProxy: tt.reverseProxy,
-			})
-			redirect, err := proxy.getAppRedirect(req)
-
-			assert.NoError(t, err)
-			assert.Equal(t, tt.expectedRedirect, redirect)
-		})
-	}
-}
-
 type ajaxRequestTest struct {
 	opts  *options.Options
 	proxy *OAuthProxy

pkg/app/redirect/director.go

@@ -0,0 +1,96 @@
+package redirect
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
+)
+
+// AppDirector is responsible for determining where OAuth2 Proxy should redirect
+// a users request to after the user has authenticated with the identity provider.
+type AppDirector interface {
+	GetRedirect(req *http.Request) (string, error)
+}
+
+// AppDirectorOpts are the requirements for constructing a new AppDirector.
+type AppDirectorOpts struct {
+	ProxyPrefix string
+	Validator   Validator
+}
+
+// NewAppDirector constructs a new AppDirector for getting the application
+// redirect URL.
+func NewAppDirector(opts AppDirectorOpts) AppDirector {
+	prefix := opts.ProxyPrefix
+	if !strings.HasSuffix(prefix, "/") {
+		prefix = fmt.Sprintf("%s/", prefix)
+	}
+
+	return &appDirector{
+		proxyPrefix: prefix,
+		validator:   opts.Validator,
+	}
+}
+
+// appDirector implements the AppDirector interface.
+type appDirector struct {
+	proxyPrefix string
+	validator   Validator
+}
+
+// GetRedirect determines the full URL or URI path to redirect clients to once
+// authenticated with the OAuthProxy.
+// Strategy priority (first legal result is used):
+// - `rd` querysting parameter
+// - `X-Auth-Request-Redirect` header
+// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
+// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
+// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
+// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
+// - `/`
+func (a *appDirector) GetRedirect(req *http.Request) (string, error) {
+	err := req.ParseForm()
+	if err != nil {
+		return "", err
+	}
+
+	// These redirect getter functions are strategies ordered by priority
+	// for figuring out the redirect URL.
+	for _, rdGetter := range []redirectGetter{
+		a.getRdQuerystringRedirect,
+		a.getXAuthRequestRedirect,
+		a.getXForwardedHeadersRedirect,
+		a.getURIRedirect,
+	} {
+		redirect := rdGetter(req)
+		// Call `p.IsValidRedirect` again here a final time to be safe
+		if redirect != "" && a.validator.IsValidRedirect(redirect) {
+			return redirect, nil
+		}
+	}
+
+	return "/", nil
+}
+
+// validateRedirect checks that the redirect is valid.
+// When an invalid, non-empty redirect is found, an error will be logged using
+// the provided format.
+func (a *appDirector) validateRedirect(redirect string, errorFormat string) string {
+	if a.validator.IsValidRedirect(redirect) {
+		return redirect
+	}
+	if redirect != "" {
+		logger.Errorf(errorFormat, redirect)
+	}
+	return ""
+}
+
+// hasProxyPrefix determines whether the obtained path would be a request to
+// one of OAuth2 Proxy's own endpoints, eg. th callback URL.
+// Redirects to these endpoints should not be allowed as they will create
+// redirection loops.
+func (a *appDirector) hasProxyPrefix(path string) bool {
+	return strings.HasPrefix(path, a.proxyPrefix)
+}

pkg/app/redirect/director_test.go

@@ -0,0 +1,177 @@
+package redirect
+
+import (
+	"net/http"
+
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+)
+
+const testProxyPrefix = "/oauth2"
+
+var _ = Describe("Director Suite", func() {
+	type getRedirectTableInput struct {
+		requestURL       string
+		headers          map[string]string
+		reverseProxy     bool
+		validator        Validator
+		expectedRedirect string
+	}
+
+	DescribeTable("GetRedirect",
+		func(in getRedirectTableInput) {
+			appDirector := NewAppDirector(AppDirectorOpts{
+				ProxyPrefix: testProxyPrefix,
+				Validator:   in.validator,
+			})
+
+			req, _ := http.NewRequest("GET", in.requestURL, nil)
+			for header, value := range in.headers {
+				if value != "" {
+					req.Header.Add(header, value)
+				}
+			}
+			req = middleware.AddRequestScope(req, &middleware.RequestScope{
+				ReverseProxy: in.reverseProxy,
+			})
+
+			redirect, err := appDirector.GetRedirect(req)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(redirect).To(Equal(in.expectedRedirect))
+		},
+		Entry("Request outside of the proxy prefix, redirects to original request", getRedirectTableInput{
+			requestURL:       "/foo/bar",
+			headers:          nil,
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "/foo/bar",
+		}),
+		Entry("Request with query, preserves the query", getRedirectTableInput{
+			requestURL:       "/foo?bar",
+			headers:          nil,
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "/foo?bar",
+		}),
+		Entry("Request under the proxy prefix, redirects to root", getRedirectTableInput{
+			requestURL:       testProxyPrefix + "/foo/bar",
+			headers:          nil,
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "/",
+		}),
+		Entry("Proxied request with headers, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo/bar",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com",
+				"X-Forwarded-Uri":   "/foo/bar",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo/bar",
+		}),
+		Entry("Non-proxied request with spoofed headers, wouldn't redirect", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo?bar",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com",
+				"X-Forwarded-Uri":   "/foo/bar",
+			},
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "/foo?bar",
+		}),
+		Entry("Proxied request with headers, under ProxyPrefix, redirects to  root", getRedirectTableInput{
+			requestURL: "https://oauth.example.com" + testProxyPrefix + "/foo/bar",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com",
+				"X-Forwarded-Uri":   testProxyPrefix + "/foo/bar",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/",
+		}),
+		Entry("Proxied request with port, under ProxyPrefix, redirects to  root", getRedirectTableInput{
+			requestURL: "https://oauth.example.com" + testProxyPrefix + "/foo/bar",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com:8443",
+				"X-Forwarded-Uri":   testProxyPrefix + "/foo/bar",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com:8443/",
+		}),
+		Entry("Proxied request with headers, missing URI header, redirects to the desired redirect URL", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo?bar",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo?bar",
+		}),
+		Entry("Proxied request without headers, with reverse proxy enabled, redirects to the desired URL", getRedirectTableInput{
+			requestURL:       "https://oauth.example.com/foo?bar",
+			headers:          nil,
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "/foo?bar",
+		}),
+		Entry("Proxied request with X-Auth-Request-Redirect, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo/bar",
+			headers: map[string]string{
+				"X-Auth-Request-Redirect": "https://a-service.example.com/foo/bar",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo/bar",
+		}),
+		Entry("Proxied request with RD parameter, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+			requestURL:       "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbar",
+			headers:          nil,
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo/bar",
+		}),
+		Entry("Proxied request with RD parameter and all headers set, reverse proxy disabled, redirects to proxied URL based on the RD parameter", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+			headers: map[string]string{
+				"X-Auth-Request-Redirect": "https://a-service.example.com/foo/baz",
+				"X-Forwarded-Proto":       "http",
+				"X-Forwarded-Host":        "another-service.example.com",
+				"X-Forwarded-Uri":         "/seasons/greetings",
+			},
+			reverseProxy:     false,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo/jazz",
+		}),
+		Entry("Proxied request with RD parameter and some headers set, reverse proxy enabled, redirects to proxied URL based on the RD parameter", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "http",
+				"X-Forwarded-Host":  "another-service.example.com",
+				"X-Forwarded-Uri":   "/seasons/greetings",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(true),
+			expectedRedirect: "https://a-service.example.com/foo/jazz",
+		}),
+		Entry("Proxied request with invalid RD parameter and some headers set, reverse proxy enabled, redirects to proxied URL based on the headers", getRedirectTableInput{
+			requestURL: "https://oauth.example.com/foo/bar?rd=http%3A%2F%2Fanother%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+			headers: map[string]string{
+				"X-Forwarded-Proto": "https",
+				"X-Forwarded-Host":  "a-service.example.com",
+				"X-Forwarded-Uri":   "/foo/bar",
+			},
+			reverseProxy:     true,
+			validator:        testValidator(false, "https://a-service.example.com/foo/bar"),
+			expectedRedirect: "https://a-service.example.com/foo/bar",
+		}),
+	)
+})

pkg/app/redirect/getters.go

@@ -0,0 +1,73 @@
+package redirect
+
+import (
+	"fmt"
+	"net/http"
+
+	requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util"
+)
+
+// redirectGetter represents a method to allow the proxy to determine a redirect
+// based on the original request.
+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"),
+		"Invalid redirect provided in rd querystring parameter: %s",
+	)
+}
+
+// getXAuthRequestRedirect handles this getAppRedirect strategy:
+// - `X-Auth-Request-Redirect` Header
+func (a *appDirector) getXAuthRequestRedirect(req *http.Request) string {
+	return a.validateRedirect(
+		req.Header.Get("X-Auth-Request-Redirect"),
+		"Invalid redirect provided in X-Auth-Request-Redirect header: %s",
+	)
+}
+
+// getXForwardedHeadersRedirect handles these getAppRedirect strategies:
+// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
+// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
+func (a *appDirector) getXForwardedHeadersRedirect(req *http.Request) string {
+	if !requestutil.IsForwardedRequest(req) {
+		return ""
+	}
+
+	uri := requestutil.GetRequestURI(req)
+	if a.hasProxyPrefix(uri) {
+		uri = "/"
+	}
+
+	redirect := fmt.Sprintf(
+		"%s://%s%s",
+		requestutil.GetRequestProto(req),
+		requestutil.GetRequestHost(req),
+		uri,
+	)
+
+	return a.validateRedirect(redirect,
+		"Invalid redirect generated from X-Forwarded-* headers: %s")
+}
+
+// getURIRedirect handles these getAppRedirect strategies:
+// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
+// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
+// - `/`
+func (a *appDirector) getURIRedirect(req *http.Request) string {
+	redirect := a.validateRedirect(
+		requestutil.GetRequestURI(req),
+		"Invalid redirect generated from X-Forwarded-Uri header: %s",
+	)
+	if redirect == "" {
+		redirect = req.URL.RequestURI()
+	}
+
+	if a.hasProxyPrefix(redirect) {
+		return "/"
+	}
+	return redirect
+}

pkg/app/redirect/pagewriter_suite_test.go

@@ -0,0 +1,39 @@
+package redirect
+
+import (
+	"testing"
+
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestOptionsSuite(t *testing.T) {
+	logger.SetOutput(GinkgoWriter)
+	logger.SetErrOutput(GinkgoWriter)
+
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Redirect Suite")
+}
+
+// testValidator creates a mock validator that will always return the given result.
+func testValidator(result bool, allowedRedirects ...string) Validator {
+	return &mockValidator{result: result, allowedRedirects: allowedRedirects}
+}
+
+// mockValidator implements the Validator interface for use in testing.
+type mockValidator struct {
+	result           bool
+	allowedRedirects []string
+}
+
+// IsValidRedirect implements the Validator interface.
+func (m *mockValidator) IsValidRedirect(redirect string) bool {
+	for _, allowed := range m.allowedRedirects {
+		if redirect == allowed {
+			return true
+		}
+	}
+
+	return m.result
+}

pkg/app/redirect/validator.go

@@ -0,0 +1,120 @@
+package redirect
+
+import (
+	"net/url"
+	"regexp"
+	"strings"
+
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
+)
+
+var (
+	// Used to check final redirects are not susceptible to open redirects.
+	// Matches //, /\ and both of these with whitespace in between (eg / / or / \).
+	invalidRedirectRegex = regexp.MustCompile(`[/\\](?:[\s\v]*|\.{1,2})[/\\]`)
+)
+
+// Validator is an interface to allow validation of application
+// redirect URLs.
+// As these values are determined from the request, they must go
+// through thorough checks to ensure the safety of the end user.
+type Validator interface {
+	IsValidRedirect(redirect string) bool
+}
+
+// NewValidator constructs a new redirect validator.
+func NewValidator(allowedDomains []string) Validator {
+	return &validator{
+		allowedDomains: allowedDomains,
+	}
+}
+
+// validator implements the Validator interface to allow validation
+// of redirect URLs.
+type validator struct {
+	allowedDomains []string
+}
+
+// IsValidRedirect checks whether the redirect URL is safe and allowed.
+func (v *validator) IsValidRedirect(redirect string) bool {
+	switch {
+	case redirect == "":
+		// The user didn't specify a redirect.
+		// In this case, we expect the proxt to fallback to `/`
+		return false
+	case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !invalidRedirectRegex.MatchString(redirect):
+		return true
+	case strings.HasPrefix(redirect, "http://") || strings.HasPrefix(redirect, "https://"):
+		redirectURL, err := url.Parse(redirect)
+		if err != nil {
+			logger.Printf("Rejecting invalid redirect %q: scheme unsupported or missing", redirect)
+			return false
+		}
+		redirectHostname := redirectURL.Hostname()
+
+		for _, allowedDomain := range v.allowedDomains {
+			allowedHost, allowedPort := splitHostPort(allowedDomain)
+			if allowedHost == "" {
+				continue
+			}
+
+			if redirectHostname == strings.TrimPrefix(allowedHost, ".") ||
+				(strings.HasPrefix(allowedHost, ".") &&
+					strings.HasSuffix(redirectHostname, allowedHost)) {
+				// the domain names match, now validate the ports
+				// if the whitelisted domain's port is '*', allow all ports
+				// if the whitelisted domain contains a specific port, only allow that port
+				// if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https
+				redirectPort := redirectURL.Port()
+				if allowedPort == "*" ||
+					allowedPort == redirectPort ||
+					(allowedPort == "" && redirectPort == "") {
+					return true
+				}
+			}
+		}
+
+		logger.Printf("Rejecting invalid redirect %q: domain / port not in whitelist", redirect)
+		return false
+	default:
+		logger.Printf("Rejecting invalid redirect %q: not an absolute or relative URL", redirect)
+		return false
+	}
+}
+
+// splitHostPort separates host and port. If the port is not valid, it returns
+// the entire input as host, and it doesn't check the validity of the host.
+// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric.
+// *** taken from net/url, modified validOptionalPort() to accept ":*"
+func splitHostPort(hostport string) (host, port string) {
+	host = hostport
+
+	colon := strings.LastIndexByte(host, ':')
+	if colon != -1 && validOptionalPort(host[colon:]) {
+		host, port = host[:colon], host[colon+1:]
+	}
+
+	if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
+		host = host[1 : len(host)-1]
+	}
+
+	return
+}
+
+// validOptionalPort reports whether port is either an empty string
+// or matches /^:\d*$/
+// *** taken from net/url, modified to accept ":*"
+func validOptionalPort(port string) bool {
+	if port == "" || port == ":*" {
+		return true
+	}
+	if port[0] != ':' {
+		return false
+	}
+	for _, b := range port[1:] {
+		if b < '0' || b > '9' {
+			return false
+		}
+	}
+	return true
+}

pkg/app/redirect/validator_test.go

@@ -0,0 +1,143 @@
+package redirect
+
+import (
+	"bufio"
+	"net/url"
+	"os"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Validator suite", func() {
+	var testAllowedDomains []string
+
+	BeforeEach(func() {
+		testAllowedDomains = []string{
+			"foo.bar",
+			".bar.foo",
+			"port.bar:8080",
+			".sub.port.bar:8080",
+			"anyport.bar:*",
+			".sub.anyport.bar:*",
+			"www.whitelisteddomain.tld",
+		}
+	})
+
+	Context("OpenRedirect List", func() {
+		file, err := os.Open("../../../testdata/openredirects.txt")
+		Expect(err).ToNot(HaveOccurred())
+		defer func() {
+			Expect(file.Close()).To(Succeed())
+		}()
+
+		scanner := bufio.NewScanner(file)
+		for scanner.Scan() {
+			rd := scanner.Text()
+			It(rd, func() {
+				rdUnescaped, err := url.QueryUnescape(rd)
+				Expect(err).ToNot(HaveOccurred())
+
+				validator := NewValidator(testAllowedDomains)
+				Expect(validator.IsValidRedirect(rdUnescaped)).To(BeFalse(), "Expected redirect not to be valid")
+			})
+		}
+
+		Expect(scanner.Err()).ToNot(HaveOccurred())
+	})
+
+	Context("Validator", func() {
+		DescribeTable("IsValidRedirect",
+			func(testRedirect string, expected bool) {
+				validator := NewValidator(testAllowedDomains)
+				Expect(validator.IsValidRedirect(testRedirect)).To(Equal(expected))
+			},
+			Entry("No Redirect", "", false),
+			Entry("Single Slash", "/redirect", true),
+			Entry("Double Slash", "//redirect", false),
+			Entry("Valid HTTP", "http://foo.bar/redirect", true),
+			Entry("Valid HTTPS", "https://foo.bar/redirect", true),
+			Entry("Invalid HTTP subdomain", "http://baz.foo.bar/redirect", false),
+			Entry("Invalid HTTPS subdomain", "https://baz.foo.bar/redirect", false),
+			Entry("Valid HTTP subdomain", "http://baz.bar.foo/redirect", true),
+			Entry("Valid HTTPS subdomain", "https://baz.bar.foo/redirect", true),
+			Entry("Valid HTTP Domain", "http://bar.foo/redirect", true), // Is this correct, do we want to match the root domain?
+			Entry("Invalid HTTP Similar Domain", "http://foo.bar.evil.corp/redirect", false),
+			Entry("Invalid HTTPS Similar Domain", "https://foo.bar.evil.corp/redirect", false),
+			Entry("Invalid HTTP RD Parameter", "http://evil.corp/redirect?rd=foo.bar", false),
+			Entry("Invalid HTTPS RD Parameter", "https://evil.corp/redirect?rd=foo.bar", false),
+			Entry("Invalid Port and Domain", "https://evil.corp:3838/redirect", false),
+			Entry("Invalid Port on Allowed Domain", "http://foo.bar:3838/redirect", false),
+			Entry("Invalid Port on Allowed Subdomain", "http://baz.bar.foo:3838/redirect", false),
+			Entry("Valid Specified Port and Domain", "http://port.bar:8080/redirect", true),
+			Entry("Invalid Specified Port and Domain", "http://port.bar:3838/redirect", false),
+			Entry("Valid Specified Port and Subdomain", "http://foo.sub.port.bar:8080/redirect", true),
+			Entry("Invalid Specified Port andSubdomain", "http://foo.subport.bar:3838/redirect", false),
+			Entry("Valid Any Port, Specified Domain", "http://anyport.bar:8080/redirect", true),
+			Entry("Valid Different Any Port, Specified Domain", "http://anyport.bar:8081/redirect", true),
+			Entry("Valid Any Port, Specified Subdomain", "http://a.sub.anyport.bar:8080/redirect", true),
+			Entry("Valid Different Any Port, Specified Subdomain", "http://a.sub.anyport.bar:8081/redirect", true),
+			Entry("Escape Double Slash", "/\\evil.com", false),
+			Entry("Space Single Slash", "/ /evil.com", false),
+			Entry("Space Double Slash", "/ \\evil.com", false),
+			Entry("Tab Single Slash", "/\t/evil.com", false),
+			Entry("Tab Double Slash", "/\t\\evil.com", false),
+			Entry("Vertical Tab Single Slash", "/\v/evil.com", false),
+			Entry("Vertiacl Tab Double Slash", "/\v\\evil.com", false),
+			Entry("New Line Single Slash", "/\n/evil.com", false),
+			Entry("New Line Double Slash", "/\n\\evil.com", false),
+			Entry("Carriage Return Single Slash", "/\r/evil.com", false),
+			Entry("Carriage Return Double Slash", "/\r\\evil.com", false),
+			Entry("Double Tab", "/\t/\t\\evil.com", false),
+			Entry("Triple Tab 1", "/\t\t/\t/evil.com", false),
+			Entry("Triple Tab 2", "/\t\t\\\t/evil.com", false),
+			Entry("Quad Tab 1", "/\t\t/\t\t\\evil.com", false),
+			Entry("Quad Tab 2", "/\t\t\\\t\t/evil.com", false),
+			Entry("Relative Path", "/./\\evil.com", false),
+			Entry("Relative Subpath", "/./../../\\evil.com", false),
+			Entry("Partial Subdomain", "evilbar.foo", false),
+		)
+	})
+
+	Context("SplitHostPort", func() {
+		type splitHostPortTableInput struct {
+			hostport     string
+			expectedHost string
+			expectedPort string
+		}
+
+		DescribeTable("Should split the host and port",
+			func(in splitHostPortTableInput) {
+				host, port := splitHostPort(in.hostport)
+				Expect(host).To(Equal(in.expectedHost))
+				Expect(port).To(Equal(in.expectedPort))
+			},
+			Entry("when no port is specified", splitHostPortTableInput{
+				hostport:     "foo.bar",
+				expectedHost: "foo.bar",
+				expectedPort: "",
+			}),
+			Entry("with a valid port specified", splitHostPortTableInput{
+				hostport:     "foo.bar:8080",
+				expectedHost: "foo.bar",
+				expectedPort: "8080",
+			}),
+			Entry("with an invalid port specified", splitHostPortTableInput{
+				hostport:     "foo.bar:808a",
+				expectedHost: "foo.bar:808a",
+				expectedPort: "",
+			}),
+			Entry("with a wildcard port specified", splitHostPortTableInput{
+				hostport:     "foo.bar:*",
+				expectedHost: "foo.bar",
+				expectedPort: "*",
+			}),
+			Entry("when the host is specified with brackets", splitHostPortTableInput{
+				hostport:     "[foo.bar]",
+				expectedHost: "foo.bar",
+				expectedPort: "",
+			}),
+		)
+	})
+})

pkg/requests/util/util.go

@@ -52,3 +52,8 @@ func IsProxied(req *http.Request) bool {
 	}
 	return scope.ReverseProxy
 }
+
+func IsForwardedRequest(req *http.Request) bool {
+	return IsProxied(req) &&
+		req.Host != GetRequestHost(req)
+}