diff --git a/internal/auth.go b/internal/auth.go index 1962e3c..72cff26 100644 --- a/internal/auth.go +++ b/internal/auth.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strconv" "strings" "time" @@ -81,9 +82,35 @@ func ValidateEmail(email string) bool { return found } +// ValidateRedirect validates that the given redirect is valid and permitted for +// the given request +func ValidateRedirect(r *http.Request, redirect string) error { + redirectURL, err := url.Parse(redirect) + + if err != nil { + return errors.New("Unable to parse redirect") + } + + // 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 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 errors.New("Redirect host does not match request host (must match when not using auth host)") + } + } + + return nil +} + // Utility methods -// Get the redirect base +// Get the request base from forwarded request func redirectBase(r *http.Request) string { proto := r.Header.Get("X-Forwarded-Proto") host := r.Header.Get("X-Forwarded-Host") diff --git a/internal/auth_test.go b/internal/auth_test.go index 5122b26..001b953 100644 --- a/internal/auth_test.go +++ b/internal/auth_test.go @@ -95,6 +95,96 @@ func TestAuthValidateEmail(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 index 9027a47..9374449 100644 --- a/internal/server.go +++ b/internal/server.go @@ -143,6 +143,16 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc { // Clear CSRF cookie http.SetCookie(w, ClearCSRFCookie(r)) + // Validate redirect + 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 { @@ -193,7 +203,8 @@ func (s *Server) authRedirect(logger *logrus.Entry, w http.ResponseWriter, r *ht func (s *Server) logger(r *http.Request, rule, msg string) *logrus.Entry { // Create logger logger := log.WithFields(logrus.Fields{ - "source_ip": r.Header.Get("X-Forwarded-For"), + "source_ip": r.Header.Get("X-Forwarded-For"), + "request_host": r.Header.Get("X-Forwarded-Host"), }) // Log request diff --git a/internal/server_test.go b/internal/server_test.go index 42919bc..703dd76 100644 --- a/internal/server_test.go +++ b/internal/server_test.go @@ -141,20 +141,20 @@ func TestServerAuthCallback(t *testing.T) { assert.Equal(401, res.StatusCode, "auth callback without cookie shouldn't be authorised") // Should catch invalid csrf cookie - req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012:http://redirect") + req = newDefaultHttpRequest("/_oauth?state=12345678901234567890123456789012: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 redirect valid 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(307, res.StatusCode, "valid auth callback should be allowed") + require.Equal(t, 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") }