MM-64755: Fix redirect in oauth login (#33388) (#33569)

Automatic Merge
2025-07-28 09:33:59 +03:00
--- a/server/channels/utils/utils.go
+++ b/server/channels/utils/utils.go
@@ -191,14 +191,17 @@ func AppendQueryParamsToURL(baseURL string, params map[string]string) string {
 // Validates RedirectURL passed during OAuth or SAML
 func IsValidWebAuthRedirectURL(config *model.Config, redirectURL string) bool {
 	u, err := url.Parse(redirectURL)
-	if err == nil && (u.Scheme == "http" || u.Scheme == "https") {
-		if config.ServiceSettings.SiteURL != nil {
-			siteURL := *config.ServiceSettings.SiteURL
-			return strings.Index(strings.ToLower(redirectURL), strings.ToLower(siteURL)) == 0
-		}
+	if err != nil || config.ServiceSettings.SiteURL == nil {
 		return false
 	}
-	return true
+	siteURL, err := url.Parse(*config.ServiceSettings.SiteURL)
+	if err != nil {
+		return false
+	}
+	if u.Scheme == siteURL.Scheme && u.Host == siteURL.Host {
+		return true
+	}
+	return false
 }

 // Validates Mobile Custom URL Scheme passed during OAuth or SAML
--- a/server/channels/utils/utils_test.go
+++ b/server/channels/utils/utils_test.go
@@ -9,6 +9,8 @@ import (
 	"testing"

 	"github.com/stretchr/testify/assert"
+
+	"github.com/mattermost/mattermost/server/public/model"
 )

 func TestStringArrayIntersection(t *testing.T) {
@@ -432,3 +434,173 @@ func TestRoundOffToZeroesResolution(t *testing.T) {
 		})
 	}
 }
+
+func TestIsValidWebAuthRedirectURL(t *testing.T) {
+	t.Run("Valid redirect URL with matching scheme and host", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "https://example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Valid redirect URL with matching scheme and host with port", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com:8080"),
+			},
+		}
+		redirectURL := "https://example.com:8080/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Invalid redirect URL with different scheme", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "http://example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Invalid redirect URL with different host", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "https://malicious.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Invalid redirect URL with different port", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com:8080"),
+			},
+		}
+		redirectURL := "https://example.com:9090/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Invalid redirect URL - malformed URL", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "not-a-valid-url"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Invalid config - nil SiteURL", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: nil,
+			},
+		}
+		redirectURL := "https://example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Invalid config - malformed SiteURL", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("not-a-valid-url"),
+			},
+		}
+		redirectURL := "https://example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Valid redirect URL with subdomain", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://app.example.com"),
+			},
+		}
+		redirectURL := "https://app.example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Invalid redirect URL with different subdomain", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://app.example.com"),
+			},
+		}
+		redirectURL := "https://api.example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+
+	t.Run("Valid redirect URL with path", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com/mattermost"),
+			},
+		}
+		redirectURL := "https://example.com/mattermost/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Valid redirect URL with query parameters", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "https://example.com/oauth/callback?state=abc123&code=def456"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Valid redirect URL with fragment", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://example.com"),
+			},
+		}
+		redirectURL := "https://example.com/oauth/callback#token=abc123"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.True(t, result)
+	})
+
+	t.Run("Invalid redirect URL with @ symbol in host", func(t *testing.T) {
+		config := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: model.NewPointer("https://qa-release.test.mattermost.cloud"),
+			},
+		}
+		redirectURL := "https://qa-release.test.mattermost.cloud@example.com/oauth/callback"
+
+		result := IsValidWebAuthRedirectURL(config, redirectURL)
+		assert.False(t, result)
+	})
+}