diff --git a/CHANGELOG.md b/CHANGELOG.md index 8daf2107..b2446269 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/oauthproxy.go b/oauthproxy.go index ea94194e..d6479609 100644 --- a/oauthproxy.go +++ b/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 @@ -87,13 +84,15 @@ type OAuthProxy struct { realClientIPParser ipapi.RealClientIPParser trustedIPs *ip.NetSet - sessionChain alice.Chain - headersChain alice.Chain - preAuthChain alice.Chain - pageWriter pagewriter.Writer - server proxyhttp.Server - upstreamProxy http.Handler - serveMux *mux.Router + sessionChain alice.Chain + headersChain alice.Chain + preAuthChain alice.Chain + pageWriter pagewriter.Writer + 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. diff --git a/oauthproxy_test.go b/oauthproxy_test.go index d8678396..94beccce 100644 --- a/oauthproxy_test.go +++ b/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 diff --git a/pkg/app/redirect/director.go b/pkg/app/redirect/director.go new file mode 100644 index 00000000..0ed8e408 --- /dev/null +++ b/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) +} diff --git a/pkg/app/redirect/director_test.go b/pkg/app/redirect/director_test.go new file mode 100644 index 00000000..55359f52 --- /dev/null +++ b/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", + }), + ) +}) diff --git a/pkg/app/redirect/getters.go b/pkg/app/redirect/getters.go new file mode 100644 index 00000000..5240abeb --- /dev/null +++ b/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 +} diff --git a/pkg/app/redirect/pagewriter_suite_test.go b/pkg/app/redirect/pagewriter_suite_test.go new file mode 100644 index 00000000..71301f58 --- /dev/null +++ b/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 +} diff --git a/pkg/app/redirect/validator.go b/pkg/app/redirect/validator.go new file mode 100644 index 00000000..78834425 --- /dev/null +++ b/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 +} diff --git a/pkg/app/redirect/validator_test.go b/pkg/app/redirect/validator_test.go new file mode 100644 index 00000000..f7692bff --- /dev/null +++ b/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: "", + }), + ) + }) +}) diff --git a/pkg/requests/util/util.go b/pkg/requests/util/util.go index 9805db61..44e1ab0e 100644 --- a/pkg/requests/util/util.go +++ b/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) +}