From a4c5334853cbc71eddbb035b86d3dda68c3ef81e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 10 Feb 2025 13:40:31 +0100 Subject: [PATCH] fix(core): Handle credential decryption failures gracefully on the API (#13166) --- .../__tests__/credentials.service.test.ts | 79 ++++++++++++------- .../src/credentials/credentials.service.ts | 23 ++++-- 2 files changed, 67 insertions(+), 35 deletions(-) diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 526acfc17d..ee166c1cff 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -1,6 +1,6 @@ import { mock } from 'jest-mock-extended'; import { nanoId, date } from 'minifaker'; -import { Credentials } from 'n8n-core'; +import { CREDENTIAL_ERRORS, CredentialDataError, Credentials, type ErrorReporter } from 'n8n-core'; import { CREDENTIAL_EMPTY_VALUE, type ICredentialType } from 'n8n-workflow'; import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; @@ -32,12 +32,14 @@ describe('CredentialsService', () => { ], }); + const errorReporter = mock(); const credentialTypes = mock(); const service = new CredentialsService( mock(), mock(), mock(), mock(), + errorReporter, mock(), mock(), credentialTypes, @@ -47,6 +49,8 @@ describe('CredentialsService', () => { mock(), ); + beforeEach(() => jest.resetAllMocks()); + describe('redact', () => { it('should redact sensitive values', () => { const credential = mock({ @@ -141,29 +145,34 @@ describe('CredentialsService', () => { }); describe('decrypt', () => { + const data = { + clientId: 'abc123', + clientSecret: 'sensitiveSecret', + accessToken: '', + oauthTokenData: 'super-secret', + csrfSecret: 'super-secret', + }; + const credentialEntity = mock({ + id: '123', + name: 'Test Credential', + type: 'oauth2', + }); + const credentials = mock({ id: credentialEntity.id }); + + beforeEach(() => { + credentialTypes.getByName.calledWith(credentialEntity.type).mockReturnValueOnce(credType); + }); + it('should redact sensitive values by default', () => { // ARRANGE - const data = { - clientId: 'abc123', - clientSecret: 'sensitiveSecret', - accessToken: '', - oauthTokenData: 'super-secret', - csrfSecret: 'super-secret', - }; - const credential = mock({ - id: '123', - name: 'Test Credential', - type: 'oauth2', - }); jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data); - credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType); // ACT - const redactedData = service.decrypt(credential); + const redactedData = service.decrypt(credentialEntity); // ASSERT expect(redactedData).toEqual({ - clientId: 'abc123', + ...data, clientSecret: CREDENTIAL_BLANKING_VALUE, accessToken: CREDENTIAL_EMPTY_VALUE, oauthTokenData: CREDENTIAL_BLANKING_VALUE, @@ -173,26 +182,36 @@ describe('CredentialsService', () => { it('should return sensitive values if `includeRawData` is true', () => { // ARRANGE - const data = { - clientId: 'abc123', - clientSecret: 'sensitiveSecret', - accessToken: '', - oauthTokenData: 'super-secret', - csrfSecret: 'super-secret', - }; - const credential = mock({ - id: '123', - name: 'Test Credential', - type: 'oauth2', - }); jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data); - credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType); // ACT - const redactedData = service.decrypt(credential, true); + const redactedData = service.decrypt(credentialEntity, true); // ASSERT expect(redactedData).toEqual(data); }); + + it('should return return an empty object if decryption fails', () => { + // ARRANGE + const decryptionError = new CredentialDataError( + credentials, + CREDENTIAL_ERRORS.DECRYPTION_FAILED, + ); + jest.spyOn(Credentials.prototype, 'getData').mockImplementation(() => { + throw decryptionError; + }); + + // ACT + const redactedData = service.decrypt(credentialEntity, true); + + // ASSERT + expect(redactedData).toEqual({}); + expect(credentialTypes.getByName).not.toHaveBeenCalled(); + expect(errorReporter.error).toHaveBeenCalledWith(decryptionError, { + extra: { credentialId: credentialEntity.id }, + level: 'error', + tags: { credentialType: credentialEntity.type }, + }); + }); }); }); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 18d01a198a..ac0711e51b 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -7,7 +7,7 @@ import { type FindOptionsRelations, type FindOptionsWhere, } from '@n8n/typeorm'; -import { Credentials, Logger } from 'n8n-core'; +import { CredentialDataError, Credentials, ErrorReporter, Logger } from 'n8n-core'; import type { ICredentialDataDecryptedObject, ICredentialsDecrypted, @@ -51,6 +51,7 @@ export class CredentialsService { private readonly sharedCredentialsRepository: SharedCredentialsRepository, private readonly ownershipService: OwnershipService, private readonly logger: Logger, + private readonly errorReporter: ErrorReporter, private readonly credentialsTester: CredentialsTester, private readonly externalHooks: ExternalHooks, private readonly credentialTypes: CredentialTypes, @@ -330,11 +331,23 @@ export class CredentialsService { */ decrypt(credential: CredentialsEntity, includeRawData = false) { const coreCredential = createCredentialsFromCredentialsEntity(credential); - const data = coreCredential.getData(); - if (includeRawData) { - return data; + try { + const data = coreCredential.getData(); + if (includeRawData) { + return data; + } + return this.redact(data, credential); + } catch (error) { + if (error instanceof CredentialDataError) { + this.errorReporter.error(error, { + level: 'error', + extra: { credentialId: credential.id }, + tags: { credentialType: credential.type }, + }); + return {}; + } + throw error; } - return this.redact(data, credential); } async update(credentialId: string, newCredentialData: ICredentialsDb) {