Ignore query, fragment and trailing slash in validating callbackURL

ref #1211

pkg/auth/dependency/sso/auth_handler_html.go

@@ -27,11 +27,28 @@ function validateCallbackURL(callbackURL, authorizedURLs) {
 	if (!callbackURL) {
 		return false;
 	}
+
+	var u;
+	try {
+		u = new URL(callbackURL);
+		u.search = "";
+		u.hash = "";
+		callbackURL = u.href;
+	} catch (e) {
+		return false;
+	}
+
+	// URL.pathname always has a leading /
+	// So new URL(u).href !== u
+	// The matching here ignore trailing slash.
+	callbackURL = callbackURL.replace(/\/$/, "");
+
 	for (var i = 0; i < authorizedURLs.length; ++i) {
-		if (callbackURL === authorizedURLs[i]) {
+		if (callbackURL === authorizedURLs[i].replace(/\/$/, "")) {
 			return true;
 		}
 	}
+
 	return false;
 }
 

pkg/auth/dependency/sso/callback_url.go

@@ -1,18 +1,36 @@
 package sso
 
 import (
+	"net/url"
+	"strings"
+
 	"github.com/skygeario/skygear-server/pkg/core/errors"
 )
 
+// ValidateCallbackURL assumes allowedCallbackURLs are URL without query or fragment.
+// It removes query or fragment of callbackURL and check if it appears in allowedCallbackURLs.
+// It also ignore trailing slash in allowedCallbackURLs and callbackURL.
 func ValidateCallbackURL(allowedCallbackURLs []string, callbackURL string) (err error) {
 	// The logic of this function must be in sync with the inline javascript implementation.
 	if callbackURL == "" {
 		err = errors.New("missing callback URL")
 		return
 	}
 
+	u, err := url.Parse(callbackURL)
+	if err != nil {
+		err = errors.New("invalid callback URL")
+		return
+	}
+
+	u.RawQuery = ""
+	u.Fragment = ""
+	callbackURL = u.String()
+
+	callbackURL = strings.TrimSuffix(callbackURL, "/")
 	for _, v := range allowedCallbackURLs {
-		if callbackURL == v {
+		allowed := strings.TrimSuffix(v, "/")
+		if callbackURL == allowed {
 			return nil
 		}
 	}

pkg/auth/dependency/sso/callback_url_test.go

@@ -25,6 +25,49 @@ func TestValidateCallbackURL(t *testing.T) {
 			{[]string{"/A/B"}, "/a/b", false},
 
 			{[]string{"a"}, "a", true},
+
+			// no query nor fragment
+			{[]string{"/a"}, "/a", true},
+
+			// Ignore query in callbackURL
+			{[]string{"/a"}, "/a?q=1", true},
+			// Does not ignore query in allowedCallbackURLs, leading to impossible match.
+			{[]string{"/a?q=1"}, "/a?q=1", false},
+
+			// Ignore fragment in callbackURL
+			{[]string{"/a"}, "/a#f", true},
+			// Does not ignore fragment in allowedCallbackURLs, leading to impossible match.
+			{[]string{"/a#f"}, "/a#f", false},
+
+			// Ignore trailing slash
+			{[]string{"http://example.com/"}, "http://example.com", true},
+			{[]string{"http://example.com/"}, "http://example.com/?q=1", true},
+			{[]string{"http://example.com/"}, "http://example.com/#f", true},
+			{[]string{"http://example.com/"}, "http://example.com/?q=1#f", true},
+
+			{[]string{"http://example.com"}, "http://example.com/", true},
+			{[]string{"http://example.com"}, "http://example.com/?q=1", true},
+			{[]string{"http://example.com"}, "http://example.com/#f", true},
+			{[]string{"http://example.com"}, "http://example.com/?q=1#f", true},
+
+			{[]string{"http://example.com/a"}, "http://example.com/a", true},
+			{[]string{"http://example.com/a"}, "http://example.com/a?q=1", true},
+			{[]string{"http://example.com/a"}, "http://example.com/a#f", true},
+			{[]string{"http://example.com/a"}, "http://example.com/a?q=1#f", true},
+
+			{[]string{"http://example.com/a"}, "http://example.com/ab", false},
+			{[]string{"http://example.com/a"}, "http://example.com/ab?q=1", false},
+			{[]string{"http://example.com/a"}, "http://example.com/ab#f", false},
+			{[]string{"http://example.com/a"}, "http://example.com/ab?q=1#f", false},
+
+			{[]string{"http://example.com/a/"}, "http://example.com/a", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a?q=1", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a#f", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a?q=1#f", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a/", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a/?q=1", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a/#f", true},
+			{[]string{"http://example.com/a/"}, "http://example.com/a/?q=1#f", true},
 		}
 
 		for _, c := range cases {