From 46b5c436bb3093cc1da3fa2455f93d4c52389eee Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Tue, 28 Oct 2025 02:46:33 +0200 Subject: [PATCH] MM-66372: Improve OAuth state token validation (#34296) (#34300) --- server/channels/app/oauth.go | 26 ++++- server/channels/app/oauth_test.go | 188 ++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 4 deletions(-) diff --git a/server/channels/app/oauth.go b/server/channels/app/oauth.go index dce9babac9..5c486db859 100644 --- a/server/channels/app/oauth.go +++ b/server/channels/app/oauth.go @@ -850,10 +850,13 @@ func (a *App) AuthorizeOAuthUser(c request.CTX, w http.ResponseWriter, r *http.R return nil, stateProps, nil, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.invalid_state.app_error", nil, "", http.StatusBadRequest).Wrap(cookieErr) } - expectedTokenExtra := generateOAuthStateTokenExtra(stateEmail, stateAction, cookie.Value) - if expectedTokenExtra != expectedToken.Extra { - err := errors.New("Extra token value does not match token generated from state") - return nil, stateProps, nil, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.invalid_state.app_error", nil, "", http.StatusBadRequest).Wrap(err) + tokenEmail, tokenAction, tokenCookie, parseErr := parseOAuthStateTokenExtra(expectedToken.Extra) + if parseErr != nil { + return nil, stateProps, nil, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.invalid_state.app_error", nil, "", http.StatusBadRequest).Wrap(parseErr) + } + + if tokenEmail != stateEmail || tokenAction != stateAction || tokenCookie != cookie.Value { + return nil, stateProps, nil, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.invalid_state.app_error", nil, "", http.StatusBadRequest).Wrap(errors.New("invalid state token")) } appErr = a.DeleteToken(expectedToken) @@ -1037,3 +1040,18 @@ func (a *App) SwitchOAuthToEmail(c request.CTX, email, password, requesterId str func generateOAuthStateTokenExtra(email, action, cookie string) string { return email + ":" + action + ":" + cookie } + +// parseOAuthStateTokenExtra parses a token extra string in the format "email:action:cookie". +// Returns an error if the token does not contain exactly 3 colon-separated parts. +func parseOAuthStateTokenExtra(tokenExtra string) (email, action, cookie string, err error) { + parts := strings.Split(tokenExtra, ":") + if len(parts) != 3 { + return "", "", "", fmt.Errorf("invalid token format: expected exactly 3 parts separated by ':', got %d", len(parts)) + } + + email = parts[0] + action = parts[1] + cookie = parts[2] + + return email, action, cookie, nil +} diff --git a/server/channels/app/oauth_test.go b/server/channels/app/oauth_test.go index 0357ed06ad..331154e58c 100644 --- a/server/channels/app/oauth_test.go +++ b/server/channels/app/oauth_test.go @@ -534,6 +534,7 @@ func TestAuthorizeOAuthUser(t *testing.T) { recorder := httptest.ResponseRecorder{} body, receivedStateProps, _, err := th.App.AuthorizeOAuthUser(th.Context, &recorder, request, model.ServiceGitlab, "", state, "") + require.Nil(t, err) require.NotNil(t, body) bodyBytes, bodyErr := io.ReadAll(body) require.NoError(t, bodyErr) @@ -695,3 +696,190 @@ func TestDeactivatedUserOAuthApp(t *testing.T) { require.Equal(t, http.StatusBadRequest, appErr.StatusCode) assert.Equal(t, "api.oauth.get_access_token.expired_code.app_error", appErr.Id) } + +func TestParseOAuthStateTokenExtra(t *testing.T) { + t.Run("valid token with normal values", func(t *testing.T) { + email, action, cookie, err := parseOAuthStateTokenExtra("user@example.com:email_to_sso:randomcookie123") + require.NoError(t, err) + assert.Equal(t, "user@example.com", email) + assert.Equal(t, "email_to_sso", action) + assert.Equal(t, "randomcookie123", cookie) + }) + + t.Run("valid token with empty email and action", func(t *testing.T) { + email, action, cookie, err := parseOAuthStateTokenExtra("::randomcookie123") + require.NoError(t, err) + assert.Equal(t, "", email) + assert.Equal(t, "", action) + assert.Equal(t, "randomcookie123", cookie) + }) + + t.Run("token with too many colons", func(t *testing.T) { + _, _, _, err := parseOAuthStateTokenExtra("user@example.com:action:value:extra") + require.Error(t, err) + assert.Contains(t, err.Error(), "expected exactly 3 parts") + assert.Contains(t, err.Error(), "got 4") + }) + + t.Run("token with too few colons", func(t *testing.T) { + _, _, _, err := parseOAuthStateTokenExtra("user@example.com:email_to_sso") + require.Error(t, err) + assert.Contains(t, err.Error(), "expected exactly 3 parts") + assert.Contains(t, err.Error(), "got 2") + }) + + t.Run("token with no colons", func(t *testing.T) { + _, _, _, err := parseOAuthStateTokenExtra("invalidtoken") + require.Error(t, err) + assert.Contains(t, err.Error(), "expected exactly 3 parts") + assert.Contains(t, err.Error(), "got 1") + }) + + t.Run("empty token string", func(t *testing.T) { + _, _, _, err := parseOAuthStateTokenExtra("") + require.Error(t, err) + assert.Contains(t, err.Error(), "expected exactly 3 parts") + }) +} + +func TestAuthorizeOAuthUser_InvalidToken(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t) + defer th.TearDown() + + mockProvider := &mocks.OAuthProvider{} + einterfaces.RegisterOAuthProvider(model.ServiceOpenid, mockProvider) + + service := model.ServiceOpenid + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableOAuthServiceProvider = true + cfg.OpenIdSettings.Enable = model.NewPointer(true) + cfg.OpenIdSettings.Id = model.NewPointer("test-client-id") + cfg.OpenIdSettings.Secret = model.NewPointer("test-secret") + cfg.OpenIdSettings.Scope = model.NewPointer(OpenIDScope) + }) + + mockProvider.On("GetSSOSettings", mock.Anything, mock.Anything, service).Return(&model.SSOSettings{ + Enable: model.NewPointer(true), + Id: model.NewPointer("test-client-id"), + Secret: model.NewPointer("test-secret"), + }, nil) + + t.Run("rejects token with extra delimiters in email field", func(t *testing.T) { + cookieValue := model.NewId() + + invalidEmail := "user@example.com:action" + action := "email_to_sso" + + tokenExtra := generateOAuthStateTokenExtra(invalidEmail, action, cookieValue) + token, err := th.App.CreateOAuthStateToken(tokenExtra) + require.Nil(t, err) + + stateProps := map[string]string{ + "token": token.Token, + "email": "user@example.com", + "action": action, + } + state := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(stateProps))) + + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.AddCookie(&http.Cookie{ + Name: CookieOAuth, + Value: "action:" + cookieValue, + }) + + _, _, _, appErr := th.App.AuthorizeOAuthUser(th.Context, w, r, service, "auth-code", state, "http://localhost/callback") + + require.NotNil(t, appErr) + assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + assert.Equal(t, "api.user.authorize_oauth_user.invalid_state.app_error", appErr.Id) + }) + + t.Run("rejects token with mismatched email", func(t *testing.T) { + cookieValue := model.NewId() + action := "email_to_sso" + + tokenExtra := generateOAuthStateTokenExtra("token@example.com", action, cookieValue) + token, err := th.App.CreateOAuthStateToken(tokenExtra) + require.Nil(t, err) + + stateProps := map[string]string{ + "token": token.Token, + "email": "state@example.com", + "action": action, + } + state := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(stateProps))) + + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.AddCookie(&http.Cookie{ + Name: CookieOAuth, + Value: cookieValue, + }) + + _, _, _, appErr := th.App.AuthorizeOAuthUser(th.Context, w, r, service, "auth-code", state, "http://localhost/callback") + + require.NotNil(t, appErr) + assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + assert.Equal(t, "api.user.authorize_oauth_user.invalid_state.app_error", appErr.Id) + }) + + t.Run("rejects token with mismatched action", func(t *testing.T) { + cookieValue := model.NewId() + email := "user@example.com" + + tokenExtra := generateOAuthStateTokenExtra(email, "email_to_sso", cookieValue) + token, err := th.App.CreateOAuthStateToken(tokenExtra) + require.Nil(t, err) + + stateProps := map[string]string{ + "token": token.Token, + "email": email, + "action": "sso_to_email", + } + state := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(stateProps))) + + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.AddCookie(&http.Cookie{ + Name: CookieOAuth, + Value: cookieValue, + }) + + _, _, _, appErr := th.App.AuthorizeOAuthUser(th.Context, w, r, service, "auth-code", state, "http://localhost/callback") + + require.NotNil(t, appErr) + assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + assert.Equal(t, "api.user.authorize_oauth_user.invalid_state.app_error", appErr.Id) + }) + + t.Run("rejects token with mismatched cookie", func(t *testing.T) { + email := "user@example.com" + action := "email_to_sso" + + tokenExtra := generateOAuthStateTokenExtra(email, action, "token-cookie-value") + token, err := th.App.CreateOAuthStateToken(tokenExtra) + require.Nil(t, err) + + stateProps := map[string]string{ + "token": token.Token, + "email": email, + "action": action, + } + state := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(stateProps))) + + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.AddCookie(&http.Cookie{ + Name: CookieOAuth, + Value: "different-cookie-value", + }) + + _, _, _, appErr := th.App.AuthorizeOAuthUser(th.Context, w, r, service, "auth-code", state, "http://localhost/callback") + + require.NotNil(t, appErr) + assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + assert.Equal(t, "api.user.authorize_oauth_user.invalid_state.app_error", appErr.Id) + }) +}