[MM-64911] Ensure redirect URL is validated before redirecting (#33559) (#33596)

Automatic Merge

server/channels/web/oauth.go

@@ -9,6 +9,7 @@ import (
 	"html"
 	"net/http"
 	"net/url"
+	"path"
 	"path/filepath"
 	"strings"
 	"time"
@@ -535,13 +536,48 @@ func signupWithOAuth(c *Context, w http.ResponseWriter, r *http.Request) {
 }
 
 func fullyQualifiedRedirectURL(siteURLPrefix, targetURL string) string {
-	parsed, _ := url.Parse(targetURL)
+	parsed, err := url.Parse(targetURL)
-	if parsed == nil || parsed.Scheme != "" || parsed.Host != "" {
+	if err != nil {
-		return targetURL
+		return siteURLPrefix
+	}
+	prefixParsed, err := url.Parse(siteURLPrefix)
+	if err != nil {
+		return siteURLPrefix
 	}
 
+	// Check if the targetURL is a valid URL and is within the siteURLPrefix
+	sameScheme := parsed.Scheme == prefixParsed.Scheme
+	sameHost := parsed.Host == prefixParsed.Host
+	safePath := strings.HasPrefix(path.Clean(parsed.Path), path.Clean(prefixParsed.Path))
+
+	if sameScheme && sameHost && safePath {
+		return targetURL
+	} else if parsed.Scheme != "" || parsed.Host != "" {
+		return siteURLPrefix
+	}
+
+	// For relative URLs, normalize and join with siteURLPrefix
 	if targetURL != "" && targetURL[0] != '/' {
 		targetURL = "/" + targetURL
 	}
-	return siteURLPrefix + targetURL
+
+	// Check for path traversal
+	joinedURL, err := url.JoinPath(siteURLPrefix, targetURL)
+	if err != nil {
+		return siteURLPrefix
+	}
+	unescapedURL, err := url.PathUnescape(joinedURL)
+	if err != nil {
+		return siteURLPrefix
+	}
+	parsed, err = url.Parse(unescapedURL)
+	if err != nil {
+		return siteURLPrefix
+	}
+
+	if !strings.HasPrefix(path.Clean(parsed.Path), path.Clean(prefixParsed.Path)) {
+		return siteURLPrefix
+	}
+
+	return parsed.String()
 }

server/channels/web/oauth_test.go

@@ -862,11 +862,25 @@ func (th *TestHelper) AddPermissionToRole(permission string, roleName string) {
 func TestFullyQualifiedRedirectURL(t *testing.T) {
 	const siteURL = "https://xxx.yyy/mm"
 	for target, expected := range map[string]string{
-		"":            "https://xxx.yyy/mm",
+		"":                                     siteURL,
-		"/":           "https://xxx.yyy/mm/",
+		"/":                                    siteURL + "/",
-		"some-path":   "https://xxx.yyy/mm/some-path",
+		"some-path":                            siteURL + "/some-path",
-		"/some-path":  "https://xxx.yyy/mm/some-path",
+		"/some-path":                           siteURL + "/some-path",
-		"/some-path/": "https://xxx.yyy/mm/some-path/",
+		"/some-path/":                          siteURL + "/some-path/",
+		"/some-path?foo=bar":                   siteURL + "/some-path?foo=bar",
+		"/some-path#section":                   siteURL + "/some-path#section",
+		"../bad-path":                          siteURL,
+		"/index.html":                          siteURL + "/index.html",
+		"//evil.com":                           siteURL,
+		"https://xxx.yyy/mm":                   siteURL,
+		"https://xxx.yyy/mm//double-concat":    siteURL + "//double-concat",
+		"https://xxx.yyy/other-path/":          siteURL,
+		"https://xxx.yyy/mm/some-path":         siteURL + "/some-path",
+		"https://yyy.zzz/mm/some-path":         siteURL,
+		"https://xxx.yyy/mm/some-path?foo=bar": siteURL + "/some-path?foo=bar",
+		"https://xxx.yyy/mm/some-path#section": siteURL + "/some-path#section",
+		"https://xxx.yyy/mm/../malicious-path": siteURL,
+		":foo":                                 siteURL,
 	} {
 		t.Run(target, func(t *testing.T) {
 			require.Equal(t, expected, fullyQualifiedRedirectURL(siteURL, target))