From 37a9a30f4082291c3a8811ef0f43f4cd526909e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20V=C3=A9lez?= Date: Mon, 16 Feb 2026 11:07:02 -0500 Subject: [PATCH] Mm 66813 sso callback metadata (#34955) * MM-66813 - Add server origin verification to mobile SSO callbacks * Enhance mobile SSO security and deprecate code-exchange * Update code-exchange deprecation to follow MM standards * Use config SiteURL for srv param, fix flow terminology --------- Co-authored-by: Mattermost Build --- api/v4/source/users.yaml | 8 ++- server/channels/api4/user.go | 20 ++++++- server/channels/api4/user_test.go | 33 ++++++++--- server/channels/web/oauth.go | 1 + server/channels/web/oauth_test.go | 53 +++++++++++++++++ server/channels/web/saml.go | 14 ++--- server/channels/web/saml_test.go | 88 ++++++++++++++++++++++++++++ server/i18n/en.json | 4 ++ server/public/model/feature_flags.go | 7 ++- 9 files changed, 206 insertions(+), 22 deletions(-) create mode 100644 server/channels/web/saml_test.go diff --git a/api/v4/source/users.yaml b/api/v4/source/users.yaml index c1d52c400f3..c09f6dac650 100644 --- a/api/v4/source/users.yaml +++ b/api/v4/source/users.yaml @@ -79,13 +79,15 @@ $ref: "#/components/responses/Forbidden" /api/v4/users/login/sso/code-exchange: post: + deprecated: true tags: - users summary: Exchange SSO login code for session tokens description: > Exchange a short-lived login_code for session tokens using SAML code exchange (mobile SSO flow). - This endpoint is part of the mobile SSO code-exchange flow to prevent tokens - from appearing in deep links. + + **Deprecated:** This endpoint is deprecated and will be removed in a future release. + Mobile clients should use the direct SSO callback flow instead. ##### Permissions @@ -130,6 +132,8 @@ $ref: "#/components/responses/BadRequest" "403": $ref: "#/components/responses/Forbidden" + "410": + description: Endpoint is deprecated and disabled /oauth/intune: post: tags: diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 837dc6630e6..2904d584de9 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -116,12 +116,28 @@ func (api *API) InitUser() { api.BaseRoutes.Users.Handle("/trigger-notify-admin-posts", api.APISessionRequired(handleTriggerNotifyAdminPosts)).Methods(http.MethodPost) } -// loginSSOCodeExchange exchanges a short-lived login_code for session tokens (mobile SAML code exchange) +// loginSSOCodeExchange exchanges a short-lived login_code for session tokens. +// +// Deprecated: This endpoint is deprecated and will be removed in a future release. +// Mobile clients should use the direct SSO callback flow instead. func loginSSOCodeExchange(c *Context, w http.ResponseWriter, r *http.Request) { + // Set deprecation headers to inform clients + w.Header().Set("Deprecation", "true") + if !c.App.Config().FeatureFlags.MobileSSOCodeExchange { - c.Err = model.NewAppError("loginSSOCodeExchange", "api.oauth.get_access_token.bad_request.app_error", nil, "feature disabled", http.StatusBadRequest) + c.Logger.Warn("Deprecated endpoint called", + mlog.String("endpoint", "/login/sso/code-exchange"), + mlog.String("status", "disabled"), + ) + c.Err = model.NewAppError("loginSSOCodeExchange", "api.user.login_sso_code_exchange.deprecated.app_error", nil, "", http.StatusGone) return } + + c.Logger.Warn("Deprecated endpoint called", + mlog.String("endpoint", "/login/sso/code-exchange"), + mlog.String("status", "enabled but deprecated"), + ) + props := model.MapFromJSON(r.Body) loginCode := props["login_code"] codeVerifier := props["code_verifier"] diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 20b0cfaf52c..110e2f7b09f 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -8624,15 +8624,34 @@ func TestLoginWithDesktopToken(t *testing.T) { }) } +func TestLoginSSOCodeExchangeDeprecated(t *testing.T) { + mainHelper.Parallel(t) + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.MobileSSOCodeExchange = false + }).InitBasic(t) + + props := map[string]string{ + "login_code": "test_code", + "code_verifier": "test_verifier", + "state": "test_state", + } + + resp, err := th.Client.DoAPIPost(context.Background(), "/users/login/sso/code-exchange", model.MapToJSON(props)) + require.Error(t, err) + require.Equal(t, http.StatusGone, resp.StatusCode) + assert.Equal(t, "true", resp.Header.Get("Deprecation")) +} + +// TestLoginSSOCodeExchange tests the code-exchange endpoint when enabled via feature flag. +// Note: This endpoint is deprecated and disabled by default. These tests verify behavior +// when explicitly enabled via feature flag (for backwards compatibility during rollout). func TestLoginSSOCodeExchange(t *testing.T) { mainHelper.Parallel(t) - th := Setup(t).InitBasic(t) + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.MobileSSOCodeExchange = true + }).InitBasic(t) t.Run("wrong token type cannot be used for code exchange", func(t *testing.T) { - th.App.UpdateConfig(func(cfg *model.Config) { - cfg.FeatureFlags.MobileSSOCodeExchange = true - }) - token := model.NewToken(model.TokenTypeOAuth, "extra-data") require.NoError(t, th.App.Srv().Store().Token().Save(token)) defer func() { @@ -8651,10 +8670,6 @@ func TestLoginSSOCodeExchange(t *testing.T) { }) t.Run("successful code exchange with S256 challenge", func(t *testing.T) { - th.App.UpdateConfig(func(cfg *model.Config) { - cfg.FeatureFlags.MobileSSOCodeExchange = true - }) - samlUser := th.CreateUserWithAuth(t, model.UserAuthServiceSaml) codeVerifier := "test_code_verifier_123456789" diff --git a/server/channels/web/oauth.go b/server/channels/web/oauth.go index 37be49847e3..6cbbdbc6a79 100644 --- a/server/channels/web/oauth.go +++ b/server/channels/web/oauth.go @@ -436,6 +436,7 @@ func completeOAuth(c *Context, w http.ResponseWriter, r *http.Request) { redirectURL = utils.AppendQueryParamsToURL(redirectURL, map[string]string{ model.SessionCookieToken: c.AppContext.Session().Token, model.SessionCookieCsrf: c.AppContext.Session().GetCSRF(), + "srv": c.App.GetSiteURL(), // Server URL for mobile client verification }) utils.RenderMobileAuthComplete(w, redirectURL) diff --git a/server/channels/web/oauth_test.go b/server/channels/web/oauth_test.go index f63eb4d623f..74e5cb36aec 100644 --- a/server/channels/web/oauth_test.go +++ b/server/channels/web/oauth_test.go @@ -853,6 +853,59 @@ func (th *TestHelper) AddPermissionToRole(tb testing.TB, permission string, role require.Nil(tb, appErr) } +// TestOAuthMobileCallbackIncludesSrvParameter verifies that mobile OAuth callbacks +// include the 'srv' parameter for origin verification +func TestOAuthMobileCallbackIncludesSrvParameter(t *testing.T) { + // The 'srv' parameter is added to mobile callbacks to allow the client + // to verify the server origin + + t.Run("srv parameter should be included in mobile callback URL construction", func(t *testing.T) { + // Verify the pattern: when we construct a redirect URL for mobile OAuth, + // it should include "srv" parameter with the server's site URL + + siteURL := "https://mattermost.example.com" + sessionToken := "test-session-token" + csrfToken := "test-csrf-token" + + // Simulate what the code does when constructing the callback + params := map[string]string{ + model.SessionCookieToken: sessionToken, + model.SessionCookieCsrf: csrfToken, + "srv": siteURL, + } + + // Verify all expected parameters are present + assert.Equal(t, sessionToken, params[model.SessionCookieToken]) + assert.Equal(t, csrfToken, params[model.SessionCookieCsrf]) + assert.Equal(t, siteURL, params["srv"]) + }) + + t.Run("srv parameter detects OAuth server mismatch", func(t *testing.T) { + // Scenario: The srv parameter from callback doesn't match expected server + // Mobile should detect the mismatch + + expectedServer := "https://server-a.example.com" + actualSrvFromCallback := "https://server-b.example.com" + + // This is the check that should happen in mobile + isMismatch := expectedServer != actualSrvFromCallback + assert.True(t, isMismatch, "Should detect server mismatch") + }) + + t.Run("srv parameter allows legitimate OAuth login", func(t *testing.T) { + // Scenario: Normal OAuth login to legitimate.com + // Server adds srv=legitimate.com to callback + // Mobile verifies: expected (legitimate.com) == srv (legitimate.com) + + expectedServer := "https://legitimate.example.com" + actualSrvFromCallback := "https://legitimate.example.com" + + // This is the check that should happen in mobile + isLegitimate := expectedServer == actualSrvFromCallback + assert.True(t, isLegitimate, "Should allow legitimate OAuth login") + }) +} + func TestFullyQualifiedRedirectURL(t *testing.T) { const siteURL = "https://xxx.yyy/mm" diff --git a/server/channels/web/saml.go b/server/channels/web/saml.go index b5cbba94535..a2c30d189c2 100644 --- a/server/channels/web/saml.go +++ b/server/channels/web/saml.go @@ -261,6 +261,7 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) { redirectURL = utils.AppendQueryParamsToURL(redirectURL, map[string]string{ "login_code": code.Token, + "srv": c.App.GetSiteURL(), // Server URL for mobile client verification }) utils.RenderMobileAuthComplete(w, redirectURL) return @@ -281,13 +282,12 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) { if hasRedirectURL { if isMobile { // Mobile clients with redirect url support - // Legacy mobile path: return tokens only when SAML code exchange was not requested - if samlChallenge == "" { - redirectURL = utils.AppendQueryParamsToURL(redirectURL, map[string]string{ - model.SessionCookieToken: c.AppContext.Session().Token, - model.SessionCookieCsrf: c.AppContext.Session().GetCSRF(), - }) - } + // Always add tokens for mobile in legacy path (we only reach here if code-exchange was skipped) + redirectURL = utils.AppendQueryParamsToURL(redirectURL, map[string]string{ + model.SessionCookieToken: c.AppContext.Session().Token, + model.SessionCookieCsrf: c.AppContext.Session().GetCSRF(), + "srv": c.App.GetSiteURL(), // Server URL for mobile client verification (config-based, not request Host) + }) utils.RenderMobileAuthComplete(w, redirectURL) } else { http.Redirect(w, r, redirectURL, http.StatusFound) diff --git a/server/channels/web/saml_test.go b/server/channels/web/saml_test.go new file mode 100644 index 00000000000..f09509f06a7 --- /dev/null +++ b/server/channels/web/saml_test.go @@ -0,0 +1,88 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package web + +import ( + "encoding/base64" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" +) + +// TestSamlCallbackIncludesSrvParameter verifies that mobile SAML callbacks +// include the 'srv' parameter for origin verification +func TestSamlCallbackIncludesSrvParameter(t *testing.T) { + // The 'srv' parameter is added to mobile callbacks to allow the client + // to verify the server origin + + t.Run("srv parameter should be included in redirect URL construction", func(t *testing.T) { + // Verify the pattern: when we construct a redirect URL for mobile, + // it should include "srv" parameter with the server's site URL + + siteURL := "https://mattermost.example.com" + sessionToken := "test-session-token" + csrfToken := "test-csrf-token" + + // Simulate what the code does when constructing the callback + params := map[string]string{ + model.SessionCookieToken: sessionToken, + model.SessionCookieCsrf: csrfToken, + "srv": siteURL, + } + + // Verify all expected parameters are present + assert.Equal(t, sessionToken, params[model.SessionCookieToken]) + assert.Equal(t, csrfToken, params[model.SessionCookieCsrf]) + assert.Equal(t, siteURL, params["srv"]) + }) + + t.Run("srv parameter detects server mismatch", func(t *testing.T) { + // Scenario: The srv parameter from callback doesn't match expected server + // Mobile should detect the mismatch + + expectedServer := "https://server-a.example.com" + actualSrvFromCallback := "https://server-b.example.com" + + // This is the check that should happen in mobile + isMismatch := expectedServer != actualSrvFromCallback + assert.True(t, isMismatch, "Should detect server mismatch") + }) + + t.Run("srv parameter allows legitimate login", func(t *testing.T) { + // Scenario: Normal login to legitimate server + // Server adds srv=server.com to callback + // Mobile verifies: expected == srv + + expectedServer := "https://server.example.com" + actualSrvFromCallback := "https://server.example.com" + + // This is the check that should happen in mobile + isLegitimate := expectedServer == actualSrvFromCallback + assert.True(t, isLegitimate, "Should allow legitimate login") + }) +} + +// TestCompleteSamlRelayState tests that relay state is properly handled +func TestCompleteSamlRelayState(t *testing.T) { + t.Run("should decode relay state correctly", func(t *testing.T) { + relayProps := map[string]string{ + "action": model.OAuthActionMobile, + "redirect_to": "mmauth://callback", + } + + relayState := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(relayProps))) + + // Decode and verify + decoded, err := base64.StdEncoding.DecodeString(relayState) + require.NoError(t, err) + + decodedProps := model.MapFromJSON(strings.NewReader(string(decoded))) + assert.Equal(t, model.OAuthActionMobile, decodedProps["action"]) + assert.Equal(t, "mmauth://callback", decodedProps["redirect_to"]) + }) +} diff --git a/server/i18n/en.json b/server/i18n/en.json index 0e7d5853cc1..9ae8a81f911 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -4546,6 +4546,10 @@ "id": "api.user.login_ldap.not_available.app_error", "translation": "AD/LDAP not available on this server." }, + { + "id": "api.user.login_sso_code_exchange.deprecated.app_error", + "translation": "This endpoint is deprecated and disabled. Please update your mobile app." + }, { "id": "api.user.login_with_desktop_token.not_oauth_or_saml_user.app_error", "translation": "User is not an OAuth or SAML user." diff --git a/server/public/model/feature_flags.go b/server/public/model/feature_flags.go index 6e270253cb3..d73af7629e0 100644 --- a/server/public/model/feature_flags.go +++ b/server/public/model/feature_flags.go @@ -76,7 +76,9 @@ type FeatureFlags struct { EnableMattermostEntry bool - // Enable mobile SSO SAML code-exchange flow (no tokens in deep links) + // DEPRECATED: Mobile SSO SAML code-exchange flow - disabled by default + // This feature is deprecated and will be removed in a future release. + // Mobile clients should use the direct SSO callback flow with srv parameter verification. MobileSSOCodeExchange bool // FEATURE_FLAG_REMOVAL: AutoTranslation - Remove this when MVP is to be released @@ -123,7 +125,8 @@ func (f *FeatureFlags) SetDefaults() { f.InteractiveDialogAppsForm = true f.EnableMattermostEntry = true - f.MobileSSOCodeExchange = true + // DEPRECATED: Disabled by default - mobile clients use direct SSO callback flow + f.MobileSSOCodeExchange = false f.AutoTranslation = true