update port whitelisting rules, refactor IsValidRedirect tests

oauthproxy.go

@@ -494,6 +494,43 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)
 	return
 }
 
+// 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
+}
+
 // IsValidRedirect checks whether the redirect URL is whitelisted
 func (p *OAuthProxy) IsValidRedirect(redirect string) bool {
 	switch {
@@ -507,19 +544,20 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool {
 		redirectHostname := redirectURL.Hostname()
 
 		for _, domain := range p.whitelistDomains {
-			domainURL := url.URL{
+			domainHostname, domainPort := splitHostPort(strings.TrimLeft(domain, "."))
-				Host: strings.TrimLeft(domain, "."),
+			if err != nil || domainHostname == "" {
-			}
-			domainHostname := domainURL.Hostname()
-			if domainHostname == "" {
 				continue
 			}
 
 			if (redirectHostname == domainHostname) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectHostname, domainHostname)) {
-				// if the domain has a port, only allow that port
+				// the domain names match, now validate the ports
-				// otherwise allow any port
+				// if the whitelisted domain's port is '*', allow all ports
-				domainPort := domainURL.Port()
+				// if the whitelisted domain contains a specific port, only allow that port
-				if (domainPort == "") || (domainPort == redirectURL.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 (domainPort == "*") ||
+					(domainPort == redirectPort) ||
+					(domainPort == "" && redirectPort == "") {
 					return true
 				}
 			}

oauthproxy_test.go

@@ -182,70 +182,158 @@ func TestIsValidRedirect(t *testing.T) {
 	opts.ClientSecret = "fgkdsgj"
 	opts.CookieSecret = "ljgiogbj"
 	// 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"}
+	opts.WhitelistDomains = []string{
+		"foo.bar",
+		".bar.foo",
+		"port.bar:8080",
+		".sub.port.bar:8080",
+		"anyport.bar:*",
+		".sub.anyport.bar:*",
+	}
 	opts.Validate()
 
 	proxy := NewOAuthProxy(opts, func(string) bool { return true })
 
-	noRD := proxy.IsValidRedirect("")
+	testCases := []struct {
-	assert.Equal(t, false, noRD)
+		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,
+		},
+	}
 
-	singleSlash := proxy.IsValidRedirect("/redirect")
+	for _, tc := range testCases {
-	assert.Equal(t, true, singleSlash)
+		t.Run(tc.Desc, func(t *testing.T) {
+			result := proxy.IsValidRedirect(tc.Redirect)
 
-	doubleSlash := proxy.IsValidRedirect("//redirect")
+			if result != tc.ExpectedResult {
-	assert.Equal(t, false, doubleSlash)
+				t.Errorf("expected %t got %t", tc.ExpectedResult, result)
-
+			}
-	validHTTP := proxy.IsValidRedirect("http://foo.bar/redirect")
+		})
-	assert.Equal(t, true, validHTTP)
+	}
-
-	validHTTPS := proxy.IsValidRedirect("https://foo.bar/redirect")
-	assert.Equal(t, true, validHTTPS)
-
-	invalidHTTPSubdomain := proxy.IsValidRedirect("http://baz.foo.bar/redirect")
-	assert.Equal(t, false, invalidHTTPSubdomain)
-
-	invalidHTTPSSubdomain := proxy.IsValidRedirect("https://baz.foo.bar/redirect")
-	assert.Equal(t, false, invalidHTTPSSubdomain)
-
-	validHTTPSubdomain := proxy.IsValidRedirect("http://baz.bar.foo/redirect")
-	assert.Equal(t, true, validHTTPSubdomain)
-
-	validHTTPSSubdomain := proxy.IsValidRedirect("https://baz.bar.foo/redirect")
-	assert.Equal(t, true, validHTTPSSubdomain)
-
-	invalidHTTP1 := proxy.IsValidRedirect("http://foo.bar.evil.corp/redirect")
-	assert.Equal(t, false, invalidHTTP1)
-
-	invalidHTTPS1 := proxy.IsValidRedirect("https://foo.bar.evil.corp/redirect")
-	assert.Equal(t, false, invalidHTTPS1)
-
-	invalidHTTP2 := proxy.IsValidRedirect("http://evil.corp/redirect?rd=foo.bar")
-	assert.Equal(t, false, invalidHTTP2)
-
-	invalidHTTPS2 := proxy.IsValidRedirect("https://evil.corp/redirect?rd=foo.bar")
-	assert.Equal(t, false, invalidHTTPS2)
-
-	invalidPort := proxy.IsValidRedirect("https://evil.corp:3838/redirect")
-	assert.Equal(t, false, invalidPort)
-
-	validAnyPort := proxy.IsValidRedirect("http://foo.bar:3838/redirect")
-	assert.Equal(t, true, validAnyPort)
-
-	validAnyPortSubdomain := proxy.IsValidRedirect("http://baz.bar.foo:3838/redirect")
-	assert.Equal(t, true, validAnyPortSubdomain)
-
-	validSpecificPort := proxy.IsValidRedirect("http://port.bar:8080/redirect")
-	assert.Equal(t, true, validSpecificPort)
-
-	invalidSpecificPort := proxy.IsValidRedirect("http://port.bar:3838/redirect")
-	assert.Equal(t, false, invalidSpecificPort)
-
-	validSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:8080/redirect")
-	assert.Equal(t, true, validSpecificPortSubdomain)
-
-	invalidSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:3838/redirect")
-	assert.Equal(t, false, invalidSpecificPortSubdomain)
 }
 
 type TestProvider struct {