MM-69392 - Make DCR redirect URI allowlist matching URL-component aware (#37170) (#37444)

Automatic Merge

server/channels/api4/oauth_test.go

@@ -859,6 +859,50 @@ func TestRegisterOAuthClient_RedirectURIAllowlist(t *testing.T) {
 		assert.NotEmpty(t, dcrErr.ErrorDescription)
 	})
 
+	t.Run("wildcard host cannot be satisfied by query string", func(t *testing.T) {
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.ServiceSettings.DCRRedirectURIAllowlist = []string{"https://*.example.com/**"}
+		})
+
+		registerRedirectURI := func(redirectURI string) (*http.Response, model.DCRError) {
+			body, _ := json.Marshal(&model.ClientRegistrationRequest{
+				RedirectURIs:            []string{redirectURI},
+				ClientName:              model.NewPointer("Test Client"),
+				TokenEndpointAuthMethod: model.NewPointer(model.ClientAuthMethodNone),
+			})
+			req, err := http.NewRequest(http.MethodPost, client.APIURL+"/oauth/apps/register", bytes.NewReader(body))
+			require.NoError(t, err)
+			req.Header.Set("Content-Type", "application/json")
+
+			httpResp, err := client.HTTPClient.Do(req)
+			require.NoError(t, err)
+
+			var dcrErr model.DCRError
+			if httpResp.StatusCode == http.StatusBadRequest {
+				jsonErr := json.NewDecoder(httpResp.Body).Decode(&dcrErr)
+				require.NoError(t, jsonErr)
+			}
+			require.NoError(t, httpResp.Body.Close())
+
+			return httpResp, dcrErr
+		}
+
+		time.Sleep(time.Second)
+		httpResp, dcrErr := registerRedirectURI("https://attacker.example.net/cb")
+		require.Equal(t, http.StatusBadRequest, httpResp.StatusCode)
+		assert.Equal(t, model.DCRErrorInvalidRedirectURI, dcrErr.Error)
+
+		time.Sleep(time.Second)
+		httpResp, dcrErr = registerRedirectURI("https://attacker.example.net?x=.example.com/cb")
+		require.Equal(t, http.StatusBadRequest, httpResp.StatusCode)
+		assert.Equal(t, model.DCRErrorInvalidRedirectURI, dcrErr.Error)
+
+		time.Sleep(time.Second)
+		httpResp, dcrErr = registerRedirectURI("https://app.example.com/cb")
+		require.Equal(t, http.StatusCreated, httpResp.StatusCode)
+		assert.Empty(t, dcrErr.Error)
+	})
+
 	t.Run("multi redirect partial mismatch rejects request", func(t *testing.T) {
 		th.App.UpdateConfig(func(cfg *model.Config) {
 			cfg.ServiceSettings.DCRRedirectURIAllowlist = []string{"https://allowed.com/**"}

server/public/model/oauth_dcr.go

@@ -5,6 +5,7 @@ package model
 
 import (
 	"net/http"
+	"net/url"
 	"strings"
 )
 
@@ -38,6 +39,13 @@ type DCRError struct {
 	ErrorDescription string `json:"error_description,omitempty"`
 }
 
+type dcrRedirectURIPattern struct {
+	scheme   string
+	host     string
+	path     string
+	rawQuery string
+}
+
 func (r *ClientRegistrationRequest) IsValid() *AppError {
 	if len(r.RedirectURIs) == 0 {
 		return NewAppError("ClientRegistrationRequest.IsValid", "model.dcr.is_valid.redirect_uris.app_error", nil, "", http.StatusBadRequest)
@@ -122,9 +130,74 @@ func IsValidDCRRedirectURIPattern(pattern string) bool {
 }
 
 // RedirectURIMatchesGlob returns true if uri matches the glob pattern.
-// * matches any chars except /, ** matches any chars including /, full-string anchored.
+// Matching is URL-component aware, so host, path, and query wildcards cannot
+// satisfy requirements from another component.
 func RedirectURIMatchesGlob(uri, pattern string) bool {
-	return redirectURIMatchesGlobRecur(uri, pattern, 0, 0)
+	candidate, err := url.ParseRequestURI(uri)
+	if err != nil || candidate.Scheme == "" || candidate.Host == "" {
+		return false
+	}
+
+	if !IsValidDCRRedirectURIPattern(pattern) {
+		return false
+	}
+
+	parsedPattern, ok := parseDCRRedirectURIPattern(pattern)
+	if !ok {
+		return false
+	}
+
+	if candidate.Scheme != parsedPattern.scheme {
+		return false
+	}
+	if !redirectURIMatchesGlobRecur(candidate.Host, parsedPattern.host, 0, 0) {
+		return false
+	}
+	if !redirectURIMatchesGlobRecur(candidate.EscapedPath(), parsedPattern.path, 0, 0) {
+		return false
+	}
+	if parsedPattern.rawQuery == "" {
+		return candidate.RawQuery == ""
+	}
+	if candidate.RawQuery == "" {
+		return false
+	}
+	return redirectURIMatchesGlobRecur(candidate.RawQuery, parsedPattern.rawQuery, 0, 0)
+}
+
+func parseDCRRedirectURIPattern(pattern string) (dcrRedirectURIPattern, bool) {
+	scheme, rest, ok := strings.Cut(pattern, "://")
+	if !ok {
+		return dcrRedirectURIPattern{}, false
+	}
+
+	hostEnd := len(rest)
+	for _, separator := range []string{"/", "?"} {
+		if i := strings.Index(rest, separator); i >= 0 && i < hostEnd {
+			hostEnd = i
+		}
+	}
+
+	host := rest[:hostEnd]
+	if host == "" {
+		return dcrRedirectURIPattern{}, false
+	}
+
+	remainder := rest[hostEnd:]
+	path := ""
+	rawQuery := ""
+	if strings.HasPrefix(remainder, "/") {
+		path, rawQuery, _ = strings.Cut(remainder, "?")
+	} else if strings.HasPrefix(remainder, "?") {
+		rawQuery = remainder[1:]
+	}
+
+	return dcrRedirectURIPattern{
+		scheme:   scheme,
+		host:     host,
+		path:     path,
+		rawQuery: rawQuery,
+	}, true
 }
 
 func redirectURIMatchesGlobRecur(uri, pattern string, ui, pi int) bool {

server/public/model/oauth_dcr_test.go

@@ -104,10 +104,21 @@ func TestRedirectURIMatchesGlob(t *testing.T) {
 
 	t.Run("host wildcard", func(t *testing.T) {
 		require.True(t, RedirectURIMatchesGlob("https://app.example.com/cb", "https://*.example.com/cb"))
+		require.True(t, RedirectURIMatchesGlob("https://app.example.com/cb", "https://*.example.com/**"))
 		require.True(t, RedirectURIMatchesGlob("https://foo.example.com/path", "https://*.example.com/*"))
 		require.False(t, RedirectURIMatchesGlob("https://example.com.evil/cb", "https://*.example.com/cb"))
 	})
 
+	t.Run("wildcards do not cross URL component boundaries", func(t *testing.T) {
+		require.False(t, RedirectURIMatchesGlob("https://attacker.example.net?x=.example.com/cb", "https://*.example.com/**"))
+		require.False(t, RedirectURIMatchesGlob("https://app.example.com/callback?x=/admin", "https://app.example.com/callback/admin"))
+	})
+
+	t.Run("query string must be explicitly allowed", func(t *testing.T) {
+		require.False(t, RedirectURIMatchesGlob("https://app.example.com/callback?tenant=foo", "https://app.example.com/callback"))
+		require.True(t, RedirectURIMatchesGlob("https://app.example.com/callback?tenant=foo", "https://app.example.com/callback?tenant=*"))
+	})
+
 	t.Run("port wildcard", func(t *testing.T) {
 		require.True(t, RedirectURIMatchesGlob("https://localhost:3000/cb", "https://localhost:*/cb"))
 		require.False(t, RedirectURIMatchesGlob("https://localhost:3000/cb", "https://localhost:8080/cb"))

webapp/channels/src/components/admin_console/admin_definition.tsx

@@ -5621,7 +5621,7 @@ const AdminDefinition: AdminDefinitionType = {
                             key: 'ServiceSettings.DCRRedirectURIAllowlist',
                             multiple: true,
                             label: defineMessage({id: 'admin.oauth.dcrRedirectURIAllowlistTitle', defaultMessage: 'DCR Redirect URI Allowlist:'}),
-                            help_text: defineMessage({id: 'admin.oauth.dcrRedirectURIAllowlistDesc', defaultMessage: 'When Dynamic Client Registration is enabled, optionally restrict which redirect URIs can be registered. Enter comma-separated glob patterns (e.g. https://*.example.com/**). If empty, all valid redirect URIs are allowed. Patterns support * (single path segment) and ** (multi-segment path).'}),
+                            help_text: defineMessage({id: 'admin.oauth.dcrRedirectURIAllowlistDesc', defaultMessage: 'When Dynamic Client Registration is enabled, optionally restrict which redirect URIs can be registered. Enter comma-separated URL glob patterns (e.g. https://*.example.com/**). If empty, all valid redirect URIs are allowed. Wildcards are matched within URL components only: host wildcards apply to the host, path wildcards apply to the path, and query strings must be explicitly included if allowed.'}),
                             help_text_markdown: false,
                             placeholder: defineMessage({id: 'admin.oauth.dcrRedirectURIAllowlistPlaceholder', defaultMessage: 'E.g.: https://*.example.com/**, https://app.example.com/callback'}),
                             isDisabled: it.any(

webapp/channels/src/i18n/en.json

@@ -1995,7 +1995,7 @@
   "admin.notices.enableEndUserNoticesDescription": "When enabled, all users will receive notices about available client upgrades and relevant end user features to improve user experience. <link>Learn more about notices</link> in our documentation.",
   "admin.notices.enableEndUserNoticesTitle": "Enable End User Notices: ",
   "admin.oauth.dcrDescription": "When true, external applications can dynamically register as OAuth 2.0 clients with Mattermost. Only enable this if you need third-party applications to register OAuth clients programmatically.",
-  "admin.oauth.dcrRedirectURIAllowlistDesc": "When Dynamic Client Registration is enabled, optionally restrict which redirect URIs can be registered. Enter comma-separated glob patterns (e.g. https://*.example.com/**). If empty, all valid redirect URIs are allowed. Patterns support * (single path segment) and ** (multi-segment path).",
+  "admin.oauth.dcrRedirectURIAllowlistDesc": "When Dynamic Client Registration is enabled, optionally restrict which redirect URIs can be registered. Enter comma-separated URL glob patterns (e.g. https://*.example.com/**). If empty, all valid redirect URIs are allowed. Wildcards are matched within URL components only: host wildcards apply to the host, path wildcards apply to the path, and query strings must be explicitly included if allowed.",
   "admin.oauth.dcrRedirectURIAllowlistPlaceholder": "E.g.: https://*.example.com/**, https://app.example.com/callback",
   "admin.oauth.dcrRedirectURIAllowlistTitle": "DCR Redirect URI Allowlist:",
   "admin.oauth.dcrTitle": "Enable OAuth 2.0 Dynamic Client Registration: ",