Match subpath correctly, prevent path traversals

urivalidate.go

@@ -14,6 +14,10 @@ func (e UriValidationError) Error() string {
 	return string(e)
 }
 
+func newUriValidationError(msg string, base string, redirect string) UriValidationError {
+	return UriValidationError(fmt.Sprintf("%s: %s / %s", msg, base, redirect))
+}
+
 // ValidateUriList validates that redirectUri is contained in baseUriList.
 // baseUriList may be a string separated by separator.
 // If separator is blank, validate only 1 URI.
@@ -40,7 +44,7 @@ func ValidateUriList(baseUriList string, redirectUri string, separator string) e
 		}
 	}
 
-	return UriValidationError(fmt.Sprintf("urls don't validate: %s / %s\n", baseUriList, redirectUri))
+	return newUriValidationError("urls don't validate", baseUriList, redirectUri)
 }
 
 // ValidateUri validates that redirectUri is contained in baseUri
@@ -67,11 +71,32 @@ func ValidateUri(baseUri string, redirectUri string) error {
 	}
 
 	// check if urls match
-	if base.Scheme == redirect.Scheme && base.Host == redirect.Host && len(redirect.Path) >= len(base.Path) && strings.HasPrefix(redirect.Path, base.Path) {
+	if base.Scheme != redirect.Scheme {
+		return newUriValidationError("scheme mismatch", baseUri, redirectUri)
+	}
+	if base.Host != redirect.Host {
+		return newUriValidationError("host mismatch", baseUri, redirectUri)
+	}
+
+	// allow exact path matches
+	if base.Path == redirect.Path {
 		return nil
 	}
 
-	return UriValidationError(fmt.Sprintf("urls don't validate: %s / %s\n", baseUri, redirectUri))
+	// 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 nil
 }
 
 // Returns the first uri from an uri list

urivalidate_test.go

@@ -5,24 +5,95 @@ import (
 )
 
 func TestURIValidate(t *testing.T) {
-	// V1
-	if err := ValidateUri("http://localhost:14000/appauth", "http://localhost:14000/appauth"); err != nil {
-		t.Errorf("V1: %s", err)
+	valid := [][]string{
+		{
+			// Exact match
+			"http://localhost:14000/appauth",
+			"http://localhost:14000/appauth",
+		},
+		{
+			// Trailing slash
+			"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/",
+		},
+		{
+			// Subpath
+			"http://www.google.com/myapp",
+			"http://www.google.com/myapp/interface/implementation",
+		},
+		{
+			// Subpath with trailing slash
+			"http://www.google.com/myapp/",
+			"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../...",
+		},
+		{
+			// If the allowed basepath contains path traversals, allow them?
+			"http://www.google.com/traversal/../allowed",
+			"http://www.google.com/traversal/../allowed/with/subpath",
+		},
 	}
-
-	// V2
-	if err := ValidateUri("http://localhost:14000/appauth", "http://localhost:14000/app"); err == nil {
-		t.Error("V2 should have failed")
+	for _, v := range valid {
+		if err := ValidateUri(v[0], v[1]); err != nil {
+			t.Errorf("Expected ValidateUri(%s, %s) to succeed, got %v", v[0], v[1], err)
+		}
 	}
 
-	// V3
-	if err := ValidateUri("http://www.google.com/myapp", "http://www.google.com/myapp/interface/implementation"); err != nil {
-		t.Errorf("V3: %s", err)
+	invalid := [][]string{
+		{
+			// Doesn't satisfy base path
+			"http://localhost:14000/appauth",
+			"http://localhost:14000/app",
+		},
+		{
+			// Doesn't satisfy base path
+			"http://localhost:14000/app/",
+			"http://localhost:14000/app",
+		},
+		{
+			// Not a subpath of base path
+			"http://localhost:14000/appauth",
+			"http://localhost:14000/appauthmodifiedpath",
+		},
+		{
+			// Host mismatch
+			"http://www.google.com/myapp",
+			"http://www2.google.com/myapp",
+		},
+		{
+			// Scheme mismatch
+			"http://www.google.com/myapp",
+			"https://www.google.com/myapp",
+		},
+		{
+			// Path traversal
+			"http://www.google.com/myapp",
+			"http://www.google.com/myapp/..",
+		},
+		{
+			// Embedded path traversal
+			"http://www.google.com/myapp",
+			"http://www.google.com/myapp/../test",
+		},
+		{
+			// Not a subpath
+			"http://www.google.com/myapp",
+			"http://www.google.com/myapp../test",
+		},
 	}
-
-	// V4
-	if err := ValidateUri("http://www.google.com/myapp", "http://www2.google.com/myapp"); err == nil {
-		t.Error("V4 should have failed")
+	for _, v := range invalid {
+		if err := ValidateUri(v[0], v[1]); err == nil {
+			t.Errorf("Expected ValidateUri(%s, %s) to fail", v[0], v[1])
+		}
 	}
 }