From b922174f48f45e98b77c4b302bfcf1967bc02675 Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Wed, 29 Oct 2025 18:29:20 +0200 Subject: [PATCH] Fix MM-65152 (#34199) (#34331) Automatic Merge --- .../services/remotecluster/mocks_test.go | 24 +- .../platform/services/remotecluster/recv.go | 4 + .../services/remotecluster/recv_test.go | 346 ++++++++++++++++++ 3 files changed, 371 insertions(+), 3 deletions(-) create mode 100644 server/platform/services/remotecluster/recv_test.go diff --git a/server/platform/services/remotecluster/mocks_test.go b/server/platform/services/remotecluster/mocks_test.go index f2f9eb06b3..d6bf79c777 100644 --- a/server/platform/services/remotecluster/mocks_test.go +++ b/server/platform/services/remotecluster/mocks_test.go @@ -17,9 +17,10 @@ import ( ) type mockServer struct { - remotes []*model.RemoteCluster - logger *mlog.Logger - user *model.User + remotes []*model.RemoteCluster + logger *mlog.Logger + user *model.User + customStore store.Store // if set, GetStore() returns this instead of creating default } func newMockServer(t *testing.T, remotes []*model.RemoteCluster) *mockServer { @@ -31,6 +32,17 @@ func newMockServer(t *testing.T, remotes []*model.RemoteCluster) *mockServer { } } +// newMockServerWithStore creates a mockServer with a custom store for fine-grained test control. +// Use this when you need to mock specific store behaviors that aren't covered by the default. +func newMockServerWithStore(t *testing.T, customStore store.Store) *mockServer { + logger := mlog.CreateConsoleTestLogger(t) + + return &mockServer{ + logger: logger, + customStore: customStore, + } +} + func (ms *mockServer) SetUser(user *model.User) { ms.user = user } @@ -44,6 +56,12 @@ func (ms *mockServer) Log() *mlog.Logger { return ms.logger } func (ms *mockServer) GetStore() store.Store { + // If a custom store was provided, use it (for unit tests with fine-grained control) + if ms.customStore != nil { + return ms.customStore + } + + // Otherwise, return the default pre-configured store (for integration-style tests) anyQueryFilter := mock.MatchedBy(func(filter model.RemoteClusterQueryFilter) bool { return true }) diff --git a/server/platform/services/remotecluster/recv.go b/server/platform/services/remotecluster/recv.go index 85fa060b58..b9db91e940 100644 --- a/server/platform/services/remotecluster/recv.go +++ b/server/platform/services/remotecluster/recv.go @@ -72,6 +72,10 @@ func (rcs *Service) ReceiveInviteConfirmation(confirm model.RemoteClusterInvite) // If the accepting cluster sent a RefreshedToken (its RemoteToken), set it as our Token if confirm.Version >= 2 && confirm.RefreshedToken != "" { rc.Token = confirm.RefreshedToken + } else { + // For older versions or if no RefreshedToken was provided, generate a new token + // to invalidate the original invite token and prevent reuse + rc.Token = model.NewId() } rcUpdated, err := store.Update(rc) diff --git a/server/platform/services/remotecluster/recv_test.go b/server/platform/services/remotecluster/recv_test.go new file mode 100644 index 0000000000..8fa18073da --- /dev/null +++ b/server/platform/services/remotecluster/recv_test.go @@ -0,0 +1,346 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package remotecluster + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" +) + +// TestReceiveInviteConfirmation_TokenInvalidation tests that the invite token is properly +// invalidated after the first successful confirmation, preventing token reuse attacks. +// This addresses the security vulnerability described in MM-65152. +func TestReceiveInviteConfirmation_TokenInvalidation(t *testing.T) { + t.Run("Protocol v2+ with RefreshedToken - token is rotated", func(t *testing.T) { + // Setup + originalToken := model.NewId() + refreshedToken := model.NewId() + remoteId := model.NewId() + + originalRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: originalToken, + SiteURL: model.SiteURLPending + model.NewId(), + CreateAt: model.GetMillis(), + } + + // Mock store + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + remoteClusterStoreMock.On("Get", remoteId, false).Return(originalRC, nil) + + // Capture the Update call to verify the token was rotated + var capturedRC *model.RemoteCluster + remoteClusterStoreMock.On("Update", mock.AnythingOfType("*model.RemoteCluster")).Run(func(args mock.Arguments) { + capturedRC = args.Get(0).(*model.RemoteCluster) + }).Return(func(rc *model.RemoteCluster) *model.RemoteCluster { + return rc + }, nil) + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + // Create invite confirmation with protocol v2+ (includes RefreshedToken) + confirm := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://example.com", + Token: model.NewId(), + RefreshedToken: refreshedToken, + Version: 3, // v3 protocol + } + + // Execute + rcUpdated, err := service.ReceiveInviteConfirmation(confirm) + + // Assert + require.NoError(t, err) + require.NotNil(t, rcUpdated) + require.NotNil(t, capturedRC, "Update should have been called") + + assert.Equal(t, refreshedToken, capturedRC.Token, "Token should be rotated to RefreshedToken") + assert.NotEqual(t, originalToken, capturedRC.Token, "Original invite token should be invalidated") + assert.Equal(t, remoteId, capturedRC.RemoteId, "RemoteId should be preserved") + + remoteClusterStoreMock.AssertExpectations(t) + }) + + t.Run("Protocol v1 or no RefreshedToken - token is regenerated", func(t *testing.T) { + // Setup + originalToken := model.NewId() + remoteId := model.NewId() + + originalRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: originalToken, + SiteURL: model.SiteURLPending + model.NewId(), + CreateAt: model.GetMillis(), + } + + // Mock store + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + remoteClusterStoreMock.On("Get", remoteId, false).Return(originalRC, nil) + + // Capture what Update was called with for explicit assertions + var capturedRC *model.RemoteCluster + remoteClusterStoreMock.On("Update", mock.AnythingOfType("*model.RemoteCluster")).Run(func(args mock.Arguments) { + capturedRC = args.Get(0).(*model.RemoteCluster) + }).Return(func(rc *model.RemoteCluster) *model.RemoteCluster { + return rc + }, nil) + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + // Create invite confirmation WITHOUT RefreshedToken (protocol v1) + confirm := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://example.com", + Token: model.NewId(), + RefreshedToken: "", // No refreshed token + Version: 1, // v1 protocol + } + + // Execute + rcUpdated, err := service.ReceiveInviteConfirmation(confirm) + + // Assert + require.NoError(t, err) + require.NotNil(t, rcUpdated) + require.NotNil(t, capturedRC, "Update should have been called") + + assert.NotEmpty(t, capturedRC.Token, "Token should be set") + assert.NotEqual(t, originalToken, capturedRC.Token, "Original invite token must be invalidated") + assert.Len(t, capturedRC.Token, 26, "New token should be a valid ID (26 chars)") + assert.Equal(t, remoteId, capturedRC.RemoteId, "RemoteId should be preserved") + + remoteClusterStoreMock.AssertExpectations(t) + }) + + t.Run("Already confirmed cluster - returns error", func(t *testing.T) { + // Setup - cluster is already confirmed (has a real SiteURL) + remoteId := model.NewId() + confirmedRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: model.NewId(), + SiteURL: "http://already-confirmed.com", // NOT a pending URL + CreateAt: model.GetMillis(), + } + + // Mock store + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + remoteClusterStoreMock.On("Get", remoteId, false).Return(confirmedRC, nil) + // Update should NOT be called + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + confirm := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://attacker.com", + Token: model.NewId(), + RefreshedToken: model.NewId(), + Version: 3, + } + + // Execute + rcUpdated, err := service.ReceiveInviteConfirmation(confirm) + + // Assert + require.Error(t, err, "Should return error for already confirmed cluster") + assert.Nil(t, rcUpdated) + assert.Contains(t, err.Error(), "already been confirmed", + "Error should indicate cluster is already confirmed") + + // Verify Update was NOT called (token reuse prevented) + remoteClusterStoreMock.AssertNotCalled(t, "Update", mock.Anything) + }) + + t.Run("Token reuse scenario - second confirmation with old token fails", func(t *testing.T) { + // This test simulates the security vulnerability: + // 1. First confirmation succeeds and rotates token + // 2. Attacker tries to reuse the old token + // 3. Second confirmation should fail because token was rotated + + originalToken := model.NewId() + newToken := model.NewId() + remoteId := model.NewId() + + // Initial state - unconfirmed cluster + originalRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: originalToken, + SiteURL: model.SiteURLPending + model.NewId(), + CreateAt: model.GetMillis(), + } + + // After first confirmation - token has been rotated + confirmedRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: newToken, // Token was rotated + RemoteToken: model.NewId(), + SiteURL: "http://legitimate.com", + CreateAt: model.GetMillis(), + } + + // Mock store + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + + // First call: return unconfirmed cluster + remoteClusterStoreMock.On("Get", remoteId, false).Return(originalRC, nil).Once() + + // First update: rotate token + remoteClusterStoreMock.On("Update", mock.MatchedBy(func(rc *model.RemoteCluster) bool { + return rc.RemoteId == remoteId + })).Return(confirmedRC, nil).Once() + + // Second call: return confirmed cluster with new token + remoteClusterStoreMock.On("Get", remoteId, false).Return(confirmedRC, nil).Once() + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + // First confirmation (legitimate) + confirm1 := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://legitimate.com", + Token: model.NewId(), + RefreshedToken: newToken, + Version: 3, + } + + rcUpdated, err := service.ReceiveInviteConfirmation(confirm1) + require.NoError(t, err) + require.NotNil(t, rcUpdated) + assert.Equal(t, newToken, rcUpdated.Token, "Token should be rotated after first confirmation") + + // Second confirmation attempt (attacker trying to reuse invite) + confirm2 := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://attacker.com", // Attacker's malicious URL + Token: originalToken, // Reusing OLD token from original invite + RefreshedToken: model.NewId(), + Version: 3, + } + + rcUpdated2, err := service.ReceiveInviteConfirmation(confirm2) + + // Assert: Second confirmation should fail + require.Error(t, err, "Second confirmation should fail - cluster already confirmed") + assert.Nil(t, rcUpdated2, "No cluster should be returned on failed reuse attempt") + assert.Contains(t, err.Error(), "already been confirmed", + "Should indicate cluster already confirmed, preventing token reuse") + + remoteClusterStoreMock.AssertExpectations(t) + }) +} + +// TestReceiveInviteConfirmation_EdgeCases tests various edge cases +func TestReceiveInviteConfirmation_EdgeCases(t *testing.T) { + t.Run("Non-existent remote ID", func(t *testing.T) { + remoteId := model.NewId() + + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + remoteClusterStoreMock.On("Get", remoteId, false).Return(nil, &model.AppError{ + Message: "not found", + }) + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + confirm := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://example.com", + Token: model.NewId(), + Version: 3, + } + + rcUpdated, err := service.ReceiveInviteConfirmation(confirm) + + require.Error(t, err) + assert.Nil(t, rcUpdated) + assert.Contains(t, err.Error(), "cannot accept invite confirmation") + }) + + t.Run("Protocol v2+ with empty RefreshedToken - falls back to NewId", func(t *testing.T) { + originalToken := model.NewId() + remoteId := model.NewId() + + originalRC := &model.RemoteCluster{ + RemoteId: remoteId, + Token: originalToken, + SiteURL: model.SiteURLPending + model.NewId(), + CreateAt: model.GetMillis(), + } + + remoteClusterStoreMock := &mocks.RemoteClusterStore{} + remoteClusterStoreMock.On("Get", remoteId, false).Return(originalRC, nil) + + // Capture what Update was called with for explicit assertions + var capturedRC *model.RemoteCluster + remoteClusterStoreMock.On("Update", mock.AnythingOfType("*model.RemoteCluster")).Run(func(args mock.Arguments) { + capturedRC = args.Get(0).(*model.RemoteCluster) + }).Return(func(rc *model.RemoteCluster) *model.RemoteCluster { + return rc + }, nil) + + storeMock := &mocks.Store{} + storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) + + mockServer := newMockServerWithStore(t, storeMock) + mockApp := newMockApp(t, nil) + service, err := NewRemoteClusterService(mockServer, mockApp) + require.NoError(t, err) + + // v3 protocol but RefreshedToken is empty + confirm := model.RemoteClusterInvite{ + RemoteId: remoteId, + SiteURL: "http://example.com", + Token: model.NewId(), + RefreshedToken: "", // Empty - should trigger fallback + Version: 3, + } + + rcUpdated, err := service.ReceiveInviteConfirmation(confirm) + + require.NoError(t, err) + require.NotNil(t, rcUpdated) + require.NotNil(t, capturedRC, "Update should have been called") + + assert.NotEqual(t, originalToken, capturedRC.Token, "Original token should be invalidated") + assert.Len(t, capturedRC.Token, 26, "Should generate new token via fallback (26 chars)") + assert.Equal(t, remoteId, capturedRC.RemoteId, "RemoteId should be preserved") + + remoteClusterStoreMock.AssertExpectations(t) + }) +}