diff --git a/server/channels/app/oauth.go b/server/channels/app/oauth.go index 848b005bbf..e676ad365e 100644 --- a/server/channels/app/oauth.go +++ b/server/channels/app/oauth.go @@ -294,6 +294,10 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(c request.CTX, clientId, grantType, return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.expired_code.app_error", nil, "", http.StatusBadRequest).Wrap(nErr) } + if authData.ClientId != clientId { + return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.client_id_mismatch.app_error", nil, "", http.StatusBadRequest) + } + if authData.IsExpired() { if nErr = a.Srv().Store().OAuth().RemoveAuthData(authData.Code); nErr != nil { c.Logger().Warn("unable to remove auth data", mlog.Err(nErr)) @@ -368,6 +372,10 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(c request.CTX, clientId, grantType, return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.refresh_token.app_error", nil, "", http.StatusNotFound).Wrap(nErr) } + if accessData.ClientId != clientId { + return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.client_id_mismatch.app_error", nil, "", http.StatusBadRequest) + } + user, nErr := a.Srv().Store().User().Get(context.Background(), accessData.UserId) if nErr != nil { return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.internal_user.app_error", nil, "", http.StatusNotFound).Wrap(nErr) diff --git a/server/channels/app/oauth_test.go b/server/channels/app/oauth_test.go index 331154e58c..18c3237724 100644 --- a/server/channels/app/oauth_test.go +++ b/server/channels/app/oauth_test.go @@ -697,6 +697,102 @@ func TestDeactivatedUserOAuthApp(t *testing.T) { assert.Equal(t, "api.oauth.get_access_token.expired_code.app_error", appErr.Id) } +func TestDifferentClientCannotRedeemCode(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + createApp := func(name string) *model.OAuthApp { + oapp := &model.OAuthApp{ + Name: name + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://example.com/callback"}, + } + oapp, err := th.App.CreateOAuthApp(oapp) + require.Nil(t, err) + return oapp + } + + appA := createApp("TestClientA") + appB := createApp("TestClientB") + + // Get an authorization code for appA + authRequest := &model.AuthorizeRequest{ + ResponseType: model.AuthCodeResponseType, + ClientId: appA.Id, + RedirectURI: appA.CallbackUrls[0], + Scope: "user", + State: "test_state", + } + redirectURI, appErr := th.App.AllowOAuthAppAccessToUser(th.Context, th.BasicUser.Id, authRequest) + require.Nil(t, appErr) + + uri, err := url.Parse(redirectURI) + require.NoError(t, err) + code := uri.Query().Get("code") + require.NotEmpty(t, code) + + // Try to redeem appA's code with appB's credentials + _, appErr = th.App.GetOAuthAccessTokenForCodeFlow(th.Context, appB.Id, model.AccessTokenGrantType, appA.CallbackUrls[0], code, appB.ClientSecret, "") + require.NotNil(t, appErr) + require.Contains(t, appErr.Id, "client_id_mismatch") + require.Equal(t, http.StatusBadRequest, appErr.StatusCode) +} + +func TestDifferentClientCannotUseRefreshToken(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + createApp := func(name string) *model.OAuthApp { + oapp := &model.OAuthApp{ + Name: name + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://example.com/callback"}, + } + oapp, err := th.App.CreateOAuthApp(oapp) + require.Nil(t, err) + return oapp + } + + appA := createApp("TestClientA") + appB := createApp("TestClientB") + + // Get an authorization code and token for appA + authRequest := &model.AuthorizeRequest{ + ResponseType: model.AuthCodeResponseType, + ClientId: appA.Id, + RedirectURI: appA.CallbackUrls[0], + Scope: "user", + State: "test_state", + } + redirectURI, appErr := th.App.AllowOAuthAppAccessToUser(th.Context, th.BasicUser.Id, authRequest) + require.Nil(t, appErr) + + uri, err := url.Parse(redirectURI) + require.NoError(t, err) + code := uri.Query().Get("code") + require.NotEmpty(t, code) + + tokenResp, appErr := th.App.GetOAuthAccessTokenForCodeFlow(th.Context, appA.Id, model.AccessTokenGrantType, appA.CallbackUrls[0], code, appA.ClientSecret, "") + require.Nil(t, appErr) + require.NotEmpty(t, tokenResp.RefreshToken) + + // Try to use appA's refresh token with appB's credentials + _, appErr = th.App.GetOAuthAccessTokenForCodeFlow(th.Context, appB.Id, model.RefreshTokenGrantType, appB.CallbackUrls[0], "", appB.ClientSecret, tokenResp.RefreshToken) + require.NotNil(t, appErr) + require.Contains(t, appErr.Id, "client_id_mismatch") + require.Equal(t, http.StatusBadRequest, appErr.StatusCode) +} + 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") diff --git a/server/i18n/en.json b/server/i18n/en.json index 237dd57193..42f4fd0744 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -2456,6 +2456,10 @@ "id": "api.oauth.get_access_token.bad_request.app_error", "translation": "invalid_request: Bad request." }, + { + "id": "api.oauth.get_access_token.client_id_mismatch.app_error", + "translation": "invalid_grant: Token grant was not issued to this client." + }, { "id": "api.oauth.get_access_token.credentials.app_error", "translation": "invalid_client: Invalid client credentials."