Validate redirect domain (#13)

* Validate redirect domain This change introduces a validation step prior to redirect as discussed in thomseddon#77 * Fix tests * Try harder to make CodeQL happy * Fix tests * Try just a little bit harder to appease CodeQL Co-authored-by: Thom Seddon <thom@seddonmedia.co.uk>
jordemort · Nov 5, 2022 · 7a01be9 · 7a01be9
1 parent 4cea39d
commit 7a01be9
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 10 deletions.
diff --git a/internal/auth.go b/internal/auth.go
@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -122,9 +123,39 @@ func ValidateDomains(user string, domains CommaSeparatedList) bool {
 	return false
 }
 
+// ValidateRedirect validates that the given redirect is valid and permitted for
+// the given request
+func ValidateRedirect(r *http.Request, redirect string) (*url.URL, error) {
+	redirectURL, err := url.Parse(redirect)
+
+	if err != nil {
+		return nil, errors.New("Unable to parse redirect")
+	}
+
+	if redirectURL.Scheme != "http" && redirectURL.Scheme != "https" {
+		return nil, errors.New("Invalid redirect URL scheme")
+	}
+
+	// If we're using an auth domain?
+	if use, base := useAuthDomain(r); use {
+		// If we are using an auth domain, they redirect must share a common
+		// suffix with the requested redirect
+		if !strings.HasSuffix(redirectURL.Host, base) {
+			return nil, errors.New("Redirect host does not match any expected hosts (should match cookie domain when using auth host)")
+		}
+	} else {
+		// If not, we should only ever redirect to the same domain
+		if redirectURL.Host != r.Header.Get("X-Forwarded-Host") {
+			return nil, errors.New("Redirect host does not match request host (must match when not using auth host)")
+		}
+	}
+
+	return redirectURL, nil
+}
+
 // Utility methods
 
-// Get the redirect base
+// Get the request base from forwarded request
 func redirectBase(r *http.Request) string {
 	return fmt.Sprintf("%s://%s", r.Header.Get("X-Forwarded-Proto"), r.Host)
 }

diff --git a/internal/auth_test.go b/internal/auth_test.go
@@ -215,6 +215,96 @@ func TestAuthValidateUser(t *testing.T) {
 	assert.True(v, "should allow user in whitelist")
 }
 
+func TestAuthValidateRedirect(t *testing.T) {
+	assert := assert.New(t)
+	config, _ = NewConfig([]string{})
+
+	newRedirectRequest := func(urlStr string) *http.Request {
+		u, err := url.Parse(urlStr)
+		assert.Nil(err)
+
+		r, _ := http.NewRequest("GET", urlStr, nil)
+		r.Header.Add("X-Forwarded-Proto", u.Scheme)
+		r.Header.Add("X-Forwarded-Host", u.Host)
+		r.Header.Add("X-Forwarded-Uri", u.RequestURI())
+
+		return r
+	}
+
+	errStr := "Redirect host does not match request host (must match when not using auth host)"
+
+	_, err := ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://app.example.com.bad.com",
+	)
+	if assert.Error(err) {
+		assert.Equal(errStr, err.Error(), "Should not allow redirect to subdomain")
+	}
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://app.example.combad.com",
+	)
+	if assert.Error(err) {
+		assert.Equal(errStr, err.Error(), "Should not allow redirect to overlapping domain")
+	}
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://example.com",
+	)
+	if assert.Error(err) {
+		assert.Equal(errStr, err.Error(), "Should not allow redirect from subdomain")
+	}
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://app.example.com/profile",
+	)
+	assert.Nil(err, "Should allow same domain")
+
+	//
+	// With Auth Host
+	//
+	config.AuthHost = "auth.example.com"
+	config.CookieDomains = []CookieDomain{*NewCookieDomain("example.com")}
+	errStr = "Redirect host does not match any expected hosts (should match cookie domain when using auth host)"
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://app.example.com.bad.com",
+	)
+	if assert.Error(err) {
+		assert.Equal(errStr, err.Error(), "Should not allow redirect to subdomain")
+	}
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://app.example.com/_oauth?state=123"),
+		"http://app.example.combad.com",
+	)
+	if assert.Error(err) {
+		assert.Equal(errStr, err.Error(), "Should not allow redirect to overlapping domain")
+	}
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://auth.example.com/_oauth?state=123"),
+		"http://app.example.com/profile",
+	)
+	assert.Nil(err, "Should allow between subdomains when using auth host")
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://auth.example.com/_oauth?state=123"),
+		"http://auth.example.com/profile",
+	)
+	assert.Nil(err, "Should allow same domain when using auth host")
+
+	_, err = ValidateRedirect(
+		newRedirectRequest("http://auth.example.com/_oauth?state=123"),
+		"http://example.com/profile",
+	)
+	assert.Nil(err, "Should allow from subdomain when using auth host")
+}
+
 func TestRedirectUri(t *testing.T) {
 	assert := assert.New(t)
 

diff --git a/internal/server.go b/internal/server.go
@@ -190,6 +190,16 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc {
 		// Clear CSRF cookie
 		http.SetCookie(w, ClearCSRFCookie(r, c))
 
+		// Validate redirect
+		redirectURL, err := ValidateRedirect(r, redirect)
+		if err != nil {
+			logger.WithFields(logrus.Fields{
+				"receieved_redirect": redirect,
+			}).Warnf("Invalid redirect in CSRF. %v", err)
+			http.Error(w, "Not authorized", 401)
+			return
+		}
+
 		// Exchange code for token
 		token, err := p.ExchangeCode(redirectUri(r), r.URL.Query().Get("code"))
 		if err != nil {
@@ -215,7 +225,7 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc {
 		}).Info("Successfully generated auth cookie, redirecting user.")
 
 		// Redirect
-		http.Redirect(w, r, redirect, http.StatusTemporaryRedirect)
+		http.Redirect(w, r, redirectURL.String(), http.StatusTemporaryRedirect)
 	}
 }
 

diff --git a/internal/server_test.go b/internal/server_test.go
@@ -222,26 +222,26 @@ func TestServerAuthCallback(t *testing.T) {
 
 	// Should catch invalid csrf cookie
 	nonce := "12345678901234567890123456789012"
-	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":http://redirect")
+	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":http://example.com")
 	c := MakeCSRFCookie(req, "nononononononononononononononono")
 	res, _ = doHttpRequest(req, c)
 	assert.Equal(401, res.StatusCode, "auth callback with invalid cookie shouldn't be authorised")
 
 	// Should catch invalid provider cookie
-	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":invalid:http://redirect")
+	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":invalid:http://example.com")
 	c = MakeCSRFCookie(req, nonce)
 	res, _ = doHttpRequest(req, c)
 	assert.Equal(401, res.StatusCode, "auth callback with invalid provider shouldn't be authorised")
 
 	// Should redirect valid request
-	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":google:http://redirect")
+	req = newHTTPRequest("GET", "http://example.com/_oauth?state="+nonce+":google:http://example.com")
 	c = MakeCSRFCookie(req, nonce)
 	res, _ = doHttpRequest(req, c)
 	require.Equal(307, res.StatusCode, "valid auth callback should be allowed")
 
 	fwd, _ := res.Location()
 	assert.Equal("http", fwd.Scheme, "valid request should be redirected to return url")
-	assert.Equal("redirect", fwd.Host, "valid request should be redirected to return url")
+	assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url")
 	assert.Equal("", fwd.Path, "valid request should be redirected to return url")
 }
 
@@ -264,7 +264,7 @@ func TestServerAuthCallbackExchangeFailure(t *testing.T) {
 	}
 
 	// Should handle failed code exchange
-	req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://redirect")
+	req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://example.com")
 	c := MakeCSRFCookie(req, "12345678901234567890123456789012")
 	res, _ := doHttpRequest(req, c)
 	assert.Equal(503, res.StatusCode, "auth callback should handle failed code exchange")
@@ -291,7 +291,7 @@ func TestServerAuthCallbackUserFailure(t *testing.T) {
 	}
 
 	// Should handle failed user request
-	req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://redirect")
+	req := newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:google:http://example.com")
 	c := MakeCSRFCookie(req, "12345678901234567890123456789012")
 	res, _ := doHttpRequest(req, c)
 	assert.Equal(503, res.StatusCode, "auth callback should handle failed user request")
@@ -317,7 +317,7 @@ func TestServerLogout(t *testing.T) {
 	require.Less(cookie.Expires.Local().Unix(), time.Now().Local().Unix()-50, "cookie should have expired")
 
 	// Test with redirect
-	config.LogoutRedirect = "http://redirect/path"
+	config.LogoutRedirect = "http://example.com/path"
 	req = newDefaultHttpRequest("/_oauth/logout")
 	res, _ = doHttpRequest(req, nil)
 	require.Equal(307, res.StatusCode, "should return a 307")
@@ -335,7 +335,7 @@ func TestServerLogout(t *testing.T) {
 	fwd, _ := res.Location()
 	require.NotNil(fwd)
 	assert.Equal("http", fwd.Scheme, "valid request should be redirected to return url")
-	assert.Equal("redirect", fwd.Host, "valid request should be redirected to return url")
+	assert.Equal("example.com", fwd.Host, "valid request should be redirected to return url")
 	assert.Equal("/path", fwd.Path, "valid request should be redirected to return url")
 
 }