fix(core): Handle credential decryption failures gracefully on the API (

#13166)
n8n-io · Feb 10, 2025 · a4c5334 · a4c5334
1 parent 5d05f7f
commit a4c5334
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 35 deletions.
diff --git 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<ErrorReporter>();
 	const credentialTypes = mock<CredentialTypes>();
 	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<CredentialsEntity>({
@@ -141,29 +145,34 @@ describe('CredentialsService', () => {
 	});
 
 	describe('decrypt', () => {
+		const data = {
+			clientId: 'abc123',
+			clientSecret: 'sensitiveSecret',
+			accessToken: '',
+			oauthTokenData: 'super-secret',
+			csrfSecret: 'super-secret',
+		};
+		const credentialEntity = mock<CredentialsEntity>({
+			id: '123',
+			name: 'Test Credential',
+			type: 'oauth2',
+		});
+		const credentials = mock<Credentials>({ 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<CredentialsEntity>({
-				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<CredentialsEntity>({
-				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
@@ -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) {