Reworked redirect uri validation, resolving uri references to base url, and returning the resolved url to the requesting function

Author: RangelReale

access.go

@@ -212,9 +212,11 @@ func (s *Server) handleAuthorizationCodeRequest(w *Response, r *http.Request) *A
 	if ret.RedirectUri == "" {
 		ret.RedirectUri = FirstUri(ret.Client.GetRedirectUri(), s.Config.RedirectUriSeparator)
 	}
-	if err = ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil {
+	if realRedirectUri, err := ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil {
 		s.setErrorAndLog(w, E_INVALID_REQUEST, err, "auth_code_request=%s", "error validating client redirect")
 		return nil
+	} else {
+		ret.RedirectUri = realRedirectUri
 	}
 	if ret.AuthorizeData.RedirectUri != ret.RedirectUri {
 		s.setErrorAndLog(w, E_INVALID_REQUEST, errors.New("Redirect uri is different"), "auth_code_request=%s", "client redirect does not match authorization data")

authorize.go

@@ -148,10 +148,12 @@ func (s *Server) HandleAuthorizeRequest(w *Response, r *http.Request) *Authorize
 		ret.RedirectUri = FirstUri(ret.Client.GetRedirectUri(), s.Config.RedirectUriSeparator)
 	}
 
-	if err = ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil {
+	if realRedirectUri, err := ValidateUriList(ret.Client.GetRedirectUri(), ret.RedirectUri, s.Config.RedirectUriSeparator); err != nil {
 		w.SetErrorState(E_INVALID_REQUEST, "", ret.State)
 		w.InternalError = err
 		return nil
+	} else {
+		ret.RedirectUri =  realRedirectUri
 	}
 
 	w.SetRedirect(ret.RedirectUri)

urivalidate.go

@@ -18,10 +18,44 @@ func newUriValidationError(msg string, base string, redirect string) UriValidati
 	return UriValidationError(fmt.Sprintf("%s: %s / %s", msg, base, redirect))
 }
 
+// Parse urls, resolving uri references to base url
+func ParseUrls(baseUrl, redirectUrl string) (retBaseUrl, retRedirectUrl *url.URL, err error) {
+	var base, redirect *url.URL
+	// parse base url
+	if base, err = url.Parse(baseUrl); err != nil {
+		return nil, nil, err
+	}
+
+	// parse redirect url
+	if redirect, err = url.Parse(redirectUrl); err != nil {
+		return nil, nil, err
+	}
+
+	// must not have fragment
+	if base.Fragment != "" || redirect.Fragment != "" {
+		return nil, nil, newUriValidationError("url must not include fragment.", baseUrl, redirectUrl)
+	}
+
+	// Scheme must match
+	if redirect.Scheme != base.Scheme {
+		return nil, nil, newUriValidationError("scheme mismatch", baseUrl, redirectUrl)
+	}
+
+	// Host must match
+	if redirect.Host != base.Host {
+		return nil, nil, newUriValidationError("host mismatch", baseUrl, redirectUrl)
+	}
+
+	// resolve references to base url
+	retBaseUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: base.Path})
+	retRedirectUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: redirect.Path})
+	return
+}
+
 // ValidateUriList validates that redirectUri is contained in baseUriList.
 // baseUriList may be a string separated by separator.
 // If separator is blank, validate only 1 URI.
-func ValidateUriList(baseUriList string, redirectUri string, separator string) error {
+func ValidateUriList(baseUriList string, redirectUri string, separator string) (realRedirectUri string, err error) {
 	// make a list of uris
 	var slist []string
 	if separator != "" {
@@ -32,71 +66,44 @@ func ValidateUriList(baseUriList string, redirectUri string, separator string) e
 	}
 
 	for _, sitem := range slist {
-		err := ValidateUri(sitem, redirectUri)
+		realRedirectUri, err = ValidateUri(sitem, redirectUri)
 		// validated, return no error
 		if err == nil {
-			return nil
+			return realRedirectUri, nil
 		}
 
 		// if there was an error that is not a validation error, return it
 		if _, iok := err.(UriValidationError); !iok {
-			return err
+			return "", err
 		}
 	}
 
-	return newUriValidationError("urls don't validate", baseUriList, redirectUri)
+	return "", newUriValidationError("urls don't validate", baseUriList, redirectUri)
 }
 
 // ValidateUri validates that redirectUri is contained in baseUri
-func ValidateUri(baseUri string, redirectUri string) error {
+func ValidateUri(baseUri string, redirectUri string) (realRedirectUri string, err error) {
 	if baseUri == "" || redirectUri == "" {
-		return errors.New("urls cannot be blank.")
+		return "", errors.New("urls cannot be blank.")
 	}
 
-	// parse base url
-	base, err := url.Parse(baseUri)
-	if err != nil {
-		return err
-	}
-
-	// parse passed url
-	redirect, err := url.Parse(redirectUri)
+	base, redirect, err := ParseUrls(baseUri, redirectUri)
 	if err != nil {
-		return err
-	}
-
-	// must not have fragment
-	if base.Fragment != "" || redirect.Fragment != "" {
-		return errors.New("url must not include fragment.")
-	}
-
-	// check if urls match
-	if base.Scheme != redirect.Scheme {
-		return newUriValidationError("scheme mismatch", baseUri, redirectUri)
-	}
-	if base.Host != redirect.Host {
-		return newUriValidationError("host mismatch", baseUri, redirectUri)
+		return "", err
 	}
 
 	// allow exact path matches
 	if base.Path == redirect.Path {
-		return nil
+		return redirect.String(), nil
 	}
 
 	// ensure prefix matches are actually subpaths
 	requiredPrefix := strings.TrimRight(base.Path, "/") + "/"
 	if !strings.HasPrefix(redirect.Path, requiredPrefix) {
-		return newUriValidationError("path is not a subpath", baseUri, redirectUri)
-	}
-
-	// ensure prefix matches don't contain path traversals
-	for _, s := range strings.Split(strings.TrimPrefix(redirect.Path, requiredPrefix), "/") {
-		if s == ".." {
-			return newUriValidationError("subpath cannot contain path traversal", baseUri, redirectUri)
-		}
+		return "", newUriValidationError("path prefix doesn't match", baseUri, redirectUri)
 	}
 
-	return nil
+	return redirect.String(),nil
 }
 
 // Returns the first uri from an uri list

urivalidate_test.go

@@ -10,41 +10,62 @@ func TestURIValidate(t *testing.T) {
 			// Exact match
 			"http://localhost:14000/appauth",
 			"http://localhost:14000/appauth",
+			"http://localhost:14000/appauth",
 		},
 		{
 			// Trailing slash
 			"http://www.google.com/myapp",
 			"http://www.google.com/myapp/",
+			"http://www.google.com/myapp/",
 		},
 		{
 			// Exact match with trailing slash
 			"http://www.google.com/myapp/",
 			"http://www.google.com/myapp/",
+			"http://www.google.com/myapp/",
 		},
 		{
 			// Subpath
 			"http://www.google.com/myapp",
 			"http://www.google.com/myapp/interface/implementation",
+			"http://www.google.com/myapp/interface/implementation",
 		},
 		{
 			// Subpath with trailing slash
 			"http://www.google.com/myapp/",
 			"http://www.google.com/myapp/interface/implementation",
+			"http://www.google.com/myapp/interface/implementation",
 		},
 		{
 			// Subpath with things that are close to path traversals, but aren't
 			"http://www.google.com/myapp",
 			"http://www.google.com/myapp/.../..implementation../...",
+			"http://www.google.com/myapp/.../..implementation../...",
 		},
 		{
 			// If the allowed basepath contains path traversals, allow them?
 			"http://www.google.com/traversal/../allowed",
 			"http://www.google.com/traversal/../allowed/with/subpath",
+			"http://www.google.com/allowed/with/subpath",
+		},
+		{
+			// Backslashes
+			"https://mysafewebsite.com/secure/redirect",
+			"https://mysafewebsite.com/secure/redirect/\\../\\../\\../evil",
+			"https://mysafewebsite.com/secure/redirect/%5C../%5C../%5C../evil",
+		},
+		{
+			// Backslashes
+			"https://mysafewebsite.com/secure/redirect",
+			"https://mysafewebsite.com/secure/redirect/\\..\\../\\../evil",
+			"https://mysafewebsite.com/secure/redirect/%5C..%5C../%5C../evil",
 		},
 	}
 	for _, v := range valid {
-		if err := ValidateUri(v[0], v[1]); err != nil {
+		if realRedirectUri, err := ValidateUri(v[0], v[1]); err != nil {
 			t.Errorf("Expected ValidateUri(%s, %s) to succeed, got %v", v[0], v[1], err)
+		} else if len(v) == 3 && realRedirectUri != v[2] {
+			t.Errorf("Expected ValidateUri(%s, %s) to return uri %s, got %s", v[0], v[1], v[2], realRedirectUri)
 		}
 	}
 
@@ -89,32 +110,37 @@ func TestURIValidate(t *testing.T) {
 			"http://www.google.com/myapp",
 			"http://www.google.com/myapp../test",
 		},
+		{
+			// Backslashes
+			"https://mysafewebsite.com/secure/redirect",
+			"https://mysafewebsite.com/secure%2fredirect/../evil",
+		},
 	}
 	for _, v := range invalid {
-		if err := ValidateUri(v[0], v[1]); err == nil {
+		if _, err := ValidateUri(v[0], v[1]); err == nil {
 			t.Errorf("Expected ValidateUri(%s, %s) to fail", v[0], v[1])
 		}
 	}
 }
 
 func TestURIListValidate(t *testing.T) {
 	// V1
-	if err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/appauth", ""); err != nil {
+	if _, err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/appauth", ""); err != nil {
 		t.Errorf("V1: %s", err)
 	}
 
 	// V2
-	if err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/app", ""); err == nil {
+	if _, err := ValidateUriList("http://localhost:14000/appauth", "http://localhost:14000/app", ""); err == nil {
 		t.Error("V2 should have failed")
 	}
 
 	// V3
-	if err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/appauth", ";"); err != nil {
+	if _, err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/appauth", ";"); err != nil {
 		t.Errorf("V3: %s", err)
 	}
 
 	// V4
-	if err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/app", ";"); err == nil {
+	if _, err := ValidateUriList("http://xxx:14000/appauth;http://localhost:14000/appauth", "http://localhost:14000/app", ";"); err == nil {
 		t.Error("V4 should have failed")
 	}
 }