From 4f055a00bc69ecd31df304d9d86a26f7cdc94102 Mon Sep 17 00:00:00 2001 From: mattermost-code Date: Fri, 17 Jul 2026 06:29:21 -0400 Subject: [PATCH] MM-69392 - Make DCR redirect URI allowlist matching URL-component aware (#37170) (#37444) Automatic Merge --- server/channels/api4/oauth_test.go | 44 +++++++++++ server/public/model/oauth_dcr.go | 77 ++++++++++++++++++- server/public/model/oauth_dcr_test.go | 11 +++ .../admin_console/admin_definition.tsx | 2 +- webapp/channels/src/i18n/en.json | 2 +- 5 files changed, 132 insertions(+), 4 deletions(-) diff --git a/server/channels/api4/oauth_test.go b/server/channels/api4/oauth_test.go index 149fe2d6bb7..ed54d0169f0 100644 --- a/server/channels/api4/oauth_test.go +++ b/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/**"} diff --git a/server/public/model/oauth_dcr.go b/server/public/model/oauth_dcr.go index 9f586a4ae2e..7a7250339ea 100644 --- a/server/public/model/oauth_dcr.go +++ b/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 { diff --git a/server/public/model/oauth_dcr_test.go b/server/public/model/oauth_dcr_test.go index 7a41ba7f571..162977ccbb2 100644 --- a/server/public/model/oauth_dcr_test.go +++ b/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")) diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index 799498ae0c1..a4f9ea83daa 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/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( diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 11582dff08d..07977fe99ba 100644 --- a/webapp/channels/src/i18n/en.json +++ b/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. Learn more about notices 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: ",