From 3c91f3f276c12088331fc0bd1e9554d77acd4993 Mon Sep 17 00:00:00 2001 From: Paul Rastoin <45004772+prastoin@users.noreply.github.com> Date: Thu, 21 May 2026 18:53:03 +0200 Subject: [PATCH] fix(server): encrypt token post refresh (#20819) # Introduction Jobs were refreshing token and returning them as plain text, resulting to underlying code flow failure as expecting encrypted tokens ## Next We should define a strong typescript signature to avoid such things to happen again, or least have an explicit naming --- .../application-connections.module.ts | 2 + .../application-connections-list.service.ts | 9 +- ...ted-account-refresh-tokens.service.spec.ts | 109 +++++++++--------- ...onnected-account-refresh-tokens.service.ts | 59 ++++------ 4 files changed, 84 insertions(+), 95 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/application-connections.module.ts b/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/application-connections.module.ts index 3651a96eeb..52fb9e6f48 100644 --- a/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/application-connections.module.ts +++ b/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/application-connections.module.ts @@ -6,6 +6,7 @@ import { ApplicationConnectionsController } from 'src/engine/core-modules/applic import { ApplicationConnectionsListService } from 'src/engine/core-modules/application/connection-provider/connections/services/application-connections-list.service'; import { TokenModule } from 'src/engine/core-modules/auth/token/token.module'; import { ConnectedAccountEntity } from 'src/engine/metadata-modules/connected-account/entities/connected-account.entity'; +import { ConnectedAccountTokenEncryptionModule } from 'src/engine/metadata-modules/connected-account/services/connected-account-token-encryption.module'; import { WorkspaceCacheStorageModule } from 'src/engine/workspace-cache-storage/workspace-cache-storage.module'; import { RefreshTokensManagerModule } from 'src/modules/connected-account/refresh-tokens-manager/connected-account-refresh-tokens-manager.module'; @@ -22,6 +23,7 @@ import { RefreshTokensManagerModule } from 'src/modules/connected-account/refres TokenModule, WorkspaceCacheStorageModule, RefreshTokensManagerModule, + ConnectedAccountTokenEncryptionModule, ], providers: [ApplicationConnectionsListService], controllers: [ApplicationConnectionsController], diff --git a/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/services/application-connections-list.service.ts b/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/services/application-connections-list.service.ts index 2a514625d8..a1d697228c 100644 --- a/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/services/application-connections-list.service.ts +++ b/packages/twenty-server/src/engine/core-modules/application/connection-provider/connections/services/application-connections-list.service.ts @@ -9,6 +9,7 @@ import { isDefined } from 'twenty-shared/utils'; import { ConnectionProviderEntity } from 'src/engine/core-modules/application/connection-provider/connection-provider.entity'; import { type AppConnectionDto } from 'src/engine/core-modules/application/connection-provider/connections/dtos/app-connection.dto'; import { ConnectedAccountEntity } from 'src/engine/metadata-modules/connected-account/entities/connected-account.entity'; +import { ConnectedAccountTokenEncryptionService } from 'src/engine/metadata-modules/connected-account/services/connected-account-token-encryption.service'; import { ConnectedAccountRefreshTokensService } from 'src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service'; type ListArgs = { @@ -38,6 +39,7 @@ export class ApplicationConnectionsListService { constructor( private readonly refreshTokensService: ConnectedAccountRefreshTokensService, + private readonly connectedAccountTokenEncryptionService: ConnectedAccountTokenEncryptionService, @InjectRepository(ConnectedAccountEntity) private readonly connectedAccountRepository: Repository, @InjectRepository(ConnectionProviderEntity) @@ -207,7 +209,7 @@ export class ApplicationConnectionsListService { } try { - const tokens = await this.refreshTokensService.resolveTokens( + const encryptedTokens = await this.refreshTokensService.resolveTokens( account, workspaceId, ); @@ -219,7 +221,10 @@ export class ApplicationConnectionsListService { handle: account.handle, visibility: account.visibility as 'user' | 'workspace', userWorkspaceId: account.userWorkspaceId, - accessToken: tokens.accessToken, + accessToken: this.connectedAccountTokenEncryptionService.decrypt({ + ciphertext: encryptedTokens.accessToken, + workspaceId, + }), scopes: account.scopes ?? provider.oauthConfig?.scopes ?? [], authFailedAt: account.authFailedAt?.toISOString() ?? null, }; diff --git a/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.spec.ts b/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.spec.ts index 241a645b64..74ebf85700 100644 --- a/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.spec.ts +++ b/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.spec.ts @@ -12,7 +12,6 @@ import { ConnectedAccountRefreshAccessTokenExceptionCode, } from 'src/engine/metadata-modules/connected-account/exceptions/connected-account-refresh-tokens.exception'; import { ConnectedAccountTokenEncryptionService } from 'src/engine/metadata-modules/connected-account/services/connected-account-token-encryption.service'; -import { GlobalWorkspaceOrmManager } from 'src/engine/twenty-orm/global-workspace-datasource/global-workspace-orm.manager'; import { GoogleAPIRefreshAccessTokenService } from 'src/modules/connected-account/refresh-tokens-manager/drivers/google/services/google-api-refresh-tokens.service'; import { MicrosoftAPIRefreshAccessTokenService } from 'src/modules/connected-account/refresh-tokens-manager/drivers/microsoft/services/microsoft-api-refresh-tokens.service'; @@ -106,15 +105,6 @@ describe('ConnectedAccountRefreshTokensService', () => { refreshTokens: jest.fn(), }, }, - { - provide: GlobalWorkspaceOrmManager, - useValue: { - executeInWorkspaceContext: jest - .fn() - - .mockImplementation((fn: () => any, _authContext?: any) => fn()), - }, - }, { provide: getRepositoryToken(ConnectedAccountEntity), useValue: { @@ -149,7 +139,7 @@ describe('ConnectedAccountRefreshTokensService', () => { }); describe('resolveTokens', () => { - it('should reuse the cached token, decrypt before returning to the caller, and skip the refresh call entirely', async () => { + it('should reuse the cached encrypted tokens as-is when valid, skipping decrypt and the refresh call entirely', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.MICROSOFT, @@ -164,28 +154,19 @@ describe('ConnectedAccountRefreshTokensService', () => { ); expect(result).toEqual({ - accessToken: mockAccessTokenPlaintext, - refreshToken: mockRefreshTokenPlaintext, + accessToken: mockEncryptedAccessToken, + refreshToken: mockEncryptedRefreshToken, }); expect( connectedAccountTokenEncryptionService.decrypt, - ).toHaveBeenCalledWith({ - ciphertext: mockEncryptedAccessToken, - workspaceId: mockWorkspaceId, - }); - expect( - connectedAccountTokenEncryptionService.decrypt, - ).toHaveBeenCalledWith({ - ciphertext: mockEncryptedRefreshToken, - workspaceId: mockWorkspaceId, - }); + ).not.toHaveBeenCalled(); expect( microsoftAPIRefreshAccessTokenService.refreshTokens, ).not.toHaveBeenCalled(); expect(connectedAccountRepository.update).not.toHaveBeenCalled(); }); - it('should decrypt the stored refresh token before sending to Microsoft, then re-encrypt the rotated tokens before persisting', async () => { + it('should decrypt the stored refresh token before sending to Microsoft, persist the re-encrypted tokens, and return them encrypted', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.MICROSOFT, @@ -194,35 +175,41 @@ describe('ConnectedAccountRefreshTokensService', () => { lastCredentialsRefreshedAt: new Date(Date.now() - 2 * 60 * 60 * 1000), // 2 hours ago } as ConnectedAccountEntity; - const newTokens = { + const newPlaintextTokens = { accessToken: mockNewAccessTokenPlaintext, refreshToken: mockRefreshTokenPlaintext, }; jest .spyOn(microsoftAPIRefreshAccessTokenService, 'refreshTokens') - .mockResolvedValue(newTokens); + .mockResolvedValue(newPlaintextTokens); const result = await service.resolveTokens( connectedAccount, mockWorkspaceId, ); - expect(result).toEqual(newTokens); + const expectedEncryptedNewAccessToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`; + const expectedEncryptedNewRefreshToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`; + + expect(result).toEqual({ + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, + }); expect( microsoftAPIRefreshAccessTokenService.refreshTokens, ).toHaveBeenCalledWith(mockRefreshTokenPlaintext); expect(connectedAccountRepository.update).toHaveBeenCalledWith( { id: mockConnectedAccountId, workspaceId: mockWorkspaceId }, expect.objectContaining({ - accessToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`, - refreshToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`, + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, lastCredentialsRefreshedAt: expect.any(Date), }), ); }); - it('should decrypt the stored refresh token before sending to Google, then re-encrypt the rotated tokens before persisting', async () => { + it('should decrypt the stored refresh token before sending to Google, persist the re-encrypted tokens, and return them encrypted', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.GOOGLE, @@ -231,29 +218,35 @@ describe('ConnectedAccountRefreshTokensService', () => { lastCredentialsRefreshedAt: new Date(Date.now() - 2 * 60 * 60 * 1000), // 2 hours ago } as ConnectedAccountEntity; - const newTokens = { + const newPlaintextTokens = { accessToken: mockNewAccessTokenPlaintext, refreshToken: mockRefreshTokenPlaintext, }; jest .spyOn(googleAPIRefreshAccessTokenService, 'refreshTokens') - .mockResolvedValue(newTokens); + .mockResolvedValue(newPlaintextTokens); const result = await service.resolveTokens( connectedAccount, mockWorkspaceId, ); - expect(result).toEqual(newTokens); + const expectedEncryptedNewAccessToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`; + const expectedEncryptedNewRefreshToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`; + + expect(result).toEqual({ + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, + }); expect( googleAPIRefreshAccessTokenService.refreshTokens, ).toHaveBeenCalledWith(mockRefreshTokenPlaintext); expect(connectedAccountRepository.update).toHaveBeenCalledWith( { id: mockConnectedAccountId, workspaceId: mockWorkspaceId }, expect.objectContaining({ - accessToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`, - refreshToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`, + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, lastCredentialsRefreshedAt: expect.any(Date), }), ); @@ -268,35 +261,41 @@ describe('ConnectedAccountRefreshTokensService', () => { lastCredentialsRefreshedAt: null, } as ConnectedAccountEntity; - const newTokens = { + const newPlaintextTokens = { accessToken: mockNewAccessTokenPlaintext, refreshToken: mockRefreshTokenPlaintext, }; jest .spyOn(microsoftAPIRefreshAccessTokenService, 'refreshTokens') - .mockResolvedValue(newTokens); + .mockResolvedValue(newPlaintextTokens); const result = await service.resolveTokens( connectedAccount, mockWorkspaceId, ); - expect(result).toEqual(newTokens); + const expectedEncryptedNewAccessToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`; + const expectedEncryptedNewRefreshToken = `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`; + + expect(result).toEqual({ + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, + }); expect( microsoftAPIRefreshAccessTokenService.refreshTokens, ).toHaveBeenCalledWith(mockRefreshTokenPlaintext); expect(connectedAccountRepository.update).toHaveBeenCalledWith( { id: mockConnectedAccountId, workspaceId: mockWorkspaceId }, expect.objectContaining({ - accessToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockNewAccessTokenPlaintext})`, - refreshToken: `${FAKE_CIPHER_PREFIX}CIPHER(${mockRefreshTokenPlaintext})`, + accessToken: expectedEncryptedNewAccessToken, + refreshToken: expectedEncryptedNewRefreshToken, lastCredentialsRefreshedAt: expect.any(Date), }), ); }); - it('should return decrypted access token and null refresh token when access token is valid but no refresh token exists', async () => { + it('should return the encrypted access token and null refresh token when access token is valid but no refresh token exists', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.APP, @@ -311,18 +310,12 @@ describe('ConnectedAccountRefreshTokensService', () => { ); expect(result).toEqual({ - accessToken: mockAccessTokenPlaintext, + accessToken: mockEncryptedAccessToken, refreshToken: null, }); expect( connectedAccountTokenEncryptionService.decrypt, - ).toHaveBeenCalledWith({ - ciphertext: mockEncryptedAccessToken, - workspaceId: mockWorkspaceId, - }); - expect( - connectedAccountTokenEncryptionService.decrypt, - ).toHaveBeenCalledTimes(1); + ).not.toHaveBeenCalled(); expect(connectedAccountRepository.update).not.toHaveBeenCalled(); }); @@ -474,7 +467,7 @@ describe('ConnectedAccountRefreshTokensService', () => { }); describe('resolveTokens - OIDC/SAML', () => { - it('should decrypt and return existing tokens for OIDC without attempting a refresh', async () => { + it('should return existing encrypted tokens for OIDC as-is without attempting a refresh', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.OIDC, @@ -489,9 +482,12 @@ describe('ConnectedAccountRefreshTokensService', () => { ); expect(result).toEqual({ - accessToken: mockAccessTokenPlaintext, - refreshToken: mockRefreshTokenPlaintext, + accessToken: mockEncryptedAccessToken, + refreshToken: mockEncryptedRefreshToken, }); + expect( + connectedAccountTokenEncryptionService.decrypt, + ).not.toHaveBeenCalled(); expect( googleAPIRefreshAccessTokenService.refreshTokens, ).not.toHaveBeenCalled(); @@ -501,7 +497,7 @@ describe('ConnectedAccountRefreshTokensService', () => { expect(connectedAccountRepository.update).not.toHaveBeenCalled(); }); - it('should decrypt and return existing tokens for SAML without attempting a refresh', async () => { + it('should return existing encrypted tokens for SAML as-is without attempting a refresh', async () => { const connectedAccount = { id: mockConnectedAccountId, provider: ConnectedAccountProvider.SAML, @@ -516,9 +512,12 @@ describe('ConnectedAccountRefreshTokensService', () => { ); expect(result).toEqual({ - accessToken: mockAccessTokenPlaintext, - refreshToken: mockRefreshTokenPlaintext, + accessToken: mockEncryptedAccessToken, + refreshToken: mockEncryptedRefreshToken, }); + expect( + connectedAccountTokenEncryptionService.decrypt, + ).not.toHaveBeenCalled(); expect( googleAPIRefreshAccessTokenService.refreshTokens, ).not.toHaveBeenCalled(); diff --git a/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.ts b/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.ts index f8d7223adc..99c463ce87 100644 --- a/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.ts +++ b/packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/services/connected-account-refresh-tokens.service.ts @@ -12,8 +12,6 @@ import { ConnectedAccountRefreshAccessTokenExceptionCode, } from 'src/engine/metadata-modules/connected-account/exceptions/connected-account-refresh-tokens.exception'; import { ConnectedAccountTokenEncryptionService } from 'src/engine/metadata-modules/connected-account/services/connected-account-token-encryption.service'; -import { GlobalWorkspaceOrmManager } from 'src/engine/twenty-orm/global-workspace-datasource/global-workspace-orm.manager'; -import { buildSystemAuthContext } from 'src/engine/twenty-orm/utils/build-system-auth-context.util'; import { GoogleAPIRefreshAccessTokenService } from 'src/modules/connected-account/refresh-tokens-manager/drivers/google/services/google-api-refresh-tokens.service'; import { MicrosoftAPIRefreshAccessTokenService } from 'src/modules/connected-account/refresh-tokens-manager/drivers/microsoft/services/microsoft-api-refresh-tokens.service'; @@ -34,7 +32,6 @@ export class ConnectedAccountRefreshTokensService { private readonly googleAPIRefreshAccessTokenService: GoogleAPIRefreshAccessTokenService, private readonly microsoftAPIRefreshAccessTokenService: MicrosoftAPIRefreshAccessTokenService, private readonly appOAuthRefreshAccessTokenService: AppOAuthRefreshAccessTokenService, - private readonly globalWorkspaceOrmManager: GlobalWorkspaceOrmManager, private readonly connectedAccountTokenEncryptionService: ConnectedAccountTokenEncryptionService, @InjectRepository(ConnectedAccountEntity) private readonly connectedAccountRepository: Repository, @@ -52,7 +49,7 @@ export class ConnectedAccountRefreshTokensService { `Reusing valid access token for connected account ${connectedAccount.id.slice(0, 7)} in workspace ${workspaceId.slice(0, 7)}`, ); - return this.decryptExistingTokens(connectedAccount, workspaceId); + return this.getExistingEncryptedTokens(connectedAccount, workspaceId); } const encryptedRefreshToken = connectedAccount.refreshToken; @@ -75,16 +72,11 @@ export class ConnectedAccountRefreshTokensService { ); } - private decryptExistingTokens( + private getExistingEncryptedTokens( connectedAccount: ConnectedAccountEntity, workspaceId: string, ): ConnectedAccountTokens { - const { - accessToken: encryptedAccessToken, - refreshToken: encryptedRefreshToken, - } = connectedAccount; - - if (!isDefined(encryptedAccessToken)) { + if (!isDefined(connectedAccount.accessToken)) { throw new ConnectedAccountRefreshAccessTokenException( `Access token is required for connected account ${connectedAccount.id} in workspace ${workspaceId}`, ConnectedAccountRefreshAccessTokenExceptionCode.ACCESS_TOKEN_NOT_FOUND, @@ -92,16 +84,8 @@ export class ConnectedAccountRefreshTokensService { } return { - accessToken: this.connectedAccountTokenEncryptionService.decrypt({ - ciphertext: encryptedAccessToken, - workspaceId, - }), - refreshToken: isDefined(encryptedRefreshToken) - ? this.connectedAccountTokenEncryptionService.decrypt({ - ciphertext: encryptedRefreshToken, - workspaceId, - }) - : null, + accessToken: connectedAccount.accessToken, + refreshToken: connectedAccount.refreshToken, }; } @@ -116,35 +100,34 @@ export class ConnectedAccountRefreshTokensService { workspaceId, }); - const connectedAccountTokens = await this.refreshTokens( + const plaintextTokens = await this.refreshTokens( connectedAccount, decryptedRefreshToken, workspaceId, ); const { - encryptedAccessToken: reEncryptedAccessToken, + encryptedAccessToken, encryptedRefreshToken: reEncryptedRefreshToken, } = this.connectedAccountTokenEncryptionService.encryptTokenPair({ - accessToken: connectedAccountTokens.accessToken, - refreshToken: connectedAccountTokens.refreshToken, + accessToken: plaintextTokens.accessToken, + refreshToken: plaintextTokens.refreshToken, workspaceId, }); - const authContext = buildSystemAuthContext(workspaceId); + await this.connectedAccountRepository.update( + { id: connectedAccount.id, workspaceId }, + { + accessToken: encryptedAccessToken, + refreshToken: reEncryptedRefreshToken, + lastCredentialsRefreshedAt: new Date(), + }, + ); - await this.globalWorkspaceOrmManager.executeInWorkspaceContext(async () => { - await this.connectedAccountRepository.update( - { id: connectedAccount.id, workspaceId }, - { - accessToken: reEncryptedAccessToken, - refreshToken: reEncryptedRefreshToken, - lastCredentialsRefreshedAt: new Date(), - }, - ); - }, authContext); - - return connectedAccountTokens; + return { + accessToken: encryptedAccessToken, + refreshToken: reEncryptedRefreshToken, + }; } async isAccessTokenStillValid(