fix(core): Redact secrets from credential test error message (#18386)

packages/cli/src/__tests__/utils.test.ts

@@ -1,6 +1,6 @@
 import type { INodeType } from 'n8n-workflow';
 
-import { shouldAssignExecuteMethod } from '../utils';
+import { shouldAssignExecuteMethod, getAllKeyPaths } from '../utils';
 
 describe('shouldAssignExecuteMethod', () => {
 	it('should return true when node has no execute, poll, trigger, webhook (unless declarative), or methods', () => {
@@ -75,3 +75,78 @@ describe('shouldAssignExecuteMethod', () => {
 		expect(shouldAssignExecuteMethod(nodeType)).toBe(true);
 	});
 });
+
+describe('getAllKeyPaths', () => {
+	const testCases = [
+		{
+			description: 'should return empty array for null or undefined',
+			obj: null,
+			valueFilter: (value: string) => value.includes('test'),
+			expected: [],
+		},
+		{
+			description: 'should return empty array for undefined',
+			obj: undefined,
+			valueFilter: (value: string) => value.includes('test'),
+			expected: [],
+		},
+		{
+			description: 'should find keys with matching string values in objects',
+			obj: {
+				name: 'test',
+				age: 25,
+				description: 'contains test data',
+				other: 'no match',
+			},
+			valueFilter: (value: string) => value.includes('test'),
+			expected: ['name', 'description'],
+		},
+		{
+			description: 'should recursively search nested objects',
+			obj: {
+				user: {
+					name: 'testuser',
+					profile: {
+						bio: 'test bio',
+					},
+				},
+				settings: {
+					theme: 'dark',
+				},
+			},
+			valueFilter: (value: string) => value.includes('test'),
+			expected: ['user.name', 'user.profile.bio'],
+		},
+		{
+			description: 'should search arrays',
+			obj: [{ name: 'test1' }, { name: 'other' }, { name: 'test2' }],
+			valueFilter: (value: string) => value.includes('test'),
+			expected: ['[0].name', '[2].name'],
+		},
+		{
+			description: 'should handle mixed arrays and objects',
+			obj: {
+				items: [{ label: 'test item' }, { label: 'normal item' }],
+				title: 'test title',
+			},
+			valueFilter: (value: string) => value.includes('test'),
+			expected: ['items[0].label', 'title'],
+		},
+		{
+			description: 'should handle non-string values by ignoring them',
+			obj: {
+				name: 'test',
+				count: 42,
+				active: true,
+				data: null,
+			},
+			valueFilter: (value: string) => value.includes('test'),
+			expected: ['name'],
+		},
+	];
+
+	it.each(testCases)('$description', ({ obj, valueFilter, expected }) => {
+		const result = getAllKeyPaths(obj, '', [], valueFilter);
+		expect(result).toEqual(expected);
+	});
+});

packages/cli/src/services/__tests__/credentials-tester.service.test.ts

@@ -1,19 +1,22 @@
 import mock from 'jest-mock-extended/lib/Mock';
-import type { ICredentialType, INodeType } from 'n8n-workflow';
+import type { ICredentialType, INodeType, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
 
 import type { CredentialTypes } from '@/credential-types';
+import type { CredentialsHelper } from '@/credentials-helper';
 import type { NodeTypes } from '@/node-types';
 import { CredentialsTester } from '@/services/credentials-tester.service';
+import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data';
 
 describe('CredentialsTester', () => {
 	const credentialTypes = mock<CredentialTypes>();
 	const nodeTypes = mock<NodeTypes>();
+	const credentialsHelper = mock<CredentialsHelper>();
 	const credentialsTester = new CredentialsTester(
 		mock(),
 		mock(),
 		credentialTypes,
 		nodeTypes,
-		mock(),
+		credentialsHelper,
 	);
 
 	beforeEach(() => {
@@ -36,4 +39,108 @@ describe('CredentialsTester', () => {
 
 		expect(testFn.name).toBe('oauth2CredTest');
 	});
+
+	describe('testCredentials', () => {
+		let mockTestFunction: jest.Mock;
+
+		beforeEach(() => {
+			mockTestFunction = jest.fn();
+			credentialTypes.getByName.mockReturnValue(mock<ICredentialType>({ test: undefined }));
+			credentialTypes.getSupportedNodes.mockReturnValue(['testCredentials']);
+			credentialTypes.getParentTypes.mockReturnValue([]);
+			nodeTypes.getByName.mockReturnValue(
+				mock<INodeType>({
+					methods: {
+						credentialTest: {
+							testCredentialsFunction: mockTestFunction,
+						},
+					},
+					description: {
+						credentials: [{ name: 'testCredentials', testedBy: 'testCredentialsFunction' }],
+					},
+				}),
+			);
+			jest
+				.spyOn(WorkflowExecuteAdditionalData, 'getBase')
+				.mockResolvedValue({} as IWorkflowExecuteAdditionalData);
+		});
+
+		it('should redact secrets in error messages', async () => {
+			mockTestFunction.mockResolvedValue({
+				status: 'Error',
+				message: 'Test failed for apiKey secret_api_key',
+			});
+
+			const computedCredentialsData = {
+				testNestedData: {
+					access_token: 'abc123',
+					secretData: {
+						apiKey: 'secret_api_key',
+					},
+				},
+			};
+			credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(computedCredentialsData);
+
+			const rawCredentialsData = {
+				...computedCredentialsData,
+				testNestedData: {
+					...computedCredentialsData.testNestedData,
+					secretData: {
+						apiKey: '{{ $secrets.apiKey }}',
+					},
+				},
+			};
+			const redactedMessage = await credentialsTester.testCredentials(
+				'user-id',
+				'testCredentials',
+				{
+					id: 'credential-id',
+					name: 'credential-name',
+					type: 'oAuth2Api',
+					data: rawCredentialsData,
+				},
+			);
+
+			expect(redactedMessage.status).toBe('Error');
+			expect(redactedMessage.message).toBe('Test failed for apiKey *****key');
+		});
+
+		it('should not redact secrets with value shorter than 3 characters', async () => {
+			mockTestFunction.mockResolvedValue({
+				status: 'Error',
+				message: 'Test failed for apiKey se',
+			});
+
+			const computedCredentialsData = {
+				testNestedData: {
+					access_token: 'abc123',
+					secretData: {
+						apiKey: 'se',
+					},
+				},
+			};
+			credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(computedCredentialsData);
+
+			const rawCredentialsData = {
+				...computedCredentialsData,
+				testNestedData: {
+					...computedCredentialsData.testNestedData,
+					apiKey: '{{ $secrets.apiKey }}',
+				},
+			};
+			const redactedMessage = await credentialsTester.testCredentials(
+				'user-id',
+				'testCredentials',
+				{
+					id: 'credential-id',
+					name: 'credential-name',
+					type: 'oAuth2Api',
+					data: rawCredentialsData,
+				},
+			);
+
+			expect(redactedMessage.status).toBe('Error');
+			expect(redactedMessage.message).toBe('Test failed for apiKey se');
+		});
+	});
 });

packages/cli/src/services/credentials-tester.service.ts

@@ -31,6 +31,7 @@ import { VersionedNodeType, NodeHelpers, Workflow, UnexpectedError } from 'n8n-w
 
 import { CredentialTypes } from '@/credential-types';
 import { NodeTypes } from '@/node-types';
+import { getAllKeyPaths } from '@/utils';
 import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data';
 
 import { RESPONSE_ERROR_MESSAGES } from '../constants';
@@ -102,7 +103,7 @@ export class CredentialsTester {
 			const allNodeTypes: INodeType[] = [];
 			if (node instanceof VersionedNodeType) {
 				// Node is versioned
-				allNodeTypes.push(...Object.values(node.nodeVersions));
+				allNodeTypes.push.apply(allNodeTypes, Object.values(node.nodeVersions));
 			} else {
 				// Node is not versioned
 				allNodeTypes.push(node as INodeType);
@@ -164,6 +165,24 @@ export class CredentialsTester {
 		return undefined;
 	}
 
+	private redactSecrets(
+		message: string,
+		credentialsData: ICredentialsDecrypted['data'],
+		secretPaths: string[],
+	): string {
+		if (secretPaths.length === 0) {
+			return message;
+		}
+		const updatedSecrets = secretPaths
+			.map((path) => get(credentialsData, path))
+			.filter((value) => value !== undefined);
+
+		updatedSecrets.forEach((value) => {
+			message = message.replaceAll(value.toString(), `*****${value.toString().slice(-3)}`);
+		});
+		return message;
+	}
+
 	// eslint-disable-next-line complexity
 	async testCredentials(
 		userId: User['id'],
@@ -178,9 +197,15 @@ export class CredentialsTester {
 			};
 		}
 
+		let credentialsDataSecretKeys: string[] = [];
 		if (credentialsDecrypted.data) {
 			try {
 				const additionalData = await WorkflowExecuteAdditionalData.getBase(userId);
+
+				// Keep all credentials data keys which have a secret value
+				credentialsDataSecretKeys = getAllKeyPaths(credentialsDecrypted.data, '', [], (value) =>
+					value.includes('$secrets.'),
+				);
 				credentialsDecrypted.data = await this.credentialsHelper.applyDefaultsAndOverwrites(
 					additionalData,
 					credentialsDecrypted.data,
@@ -202,7 +227,20 @@ export class CredentialsTester {
 		if (typeof credentialTestFunction === 'function') {
 			// The credentials get tested via a function that is defined on the node
 			const context = new CredentialTestContext();
-			return credentialTestFunction.call(context, credentialsDecrypted);
+			const functionResult = credentialTestFunction.call(context, credentialsDecrypted);
+			if (functionResult instanceof Promise) {
+				const result = await functionResult;
+				if (typeof result?.message === 'string') {
+					// Anonymize secret values in the error message
+					result.message = this.redactSecrets(
+						result.message,
+						credentialsDecrypted.data,
+						credentialsDataSecretKeys,
+					);
+				}
+				return result;
+			}
+			return functionResult;
 		}
 
 		// Credentials get tested via request instructions

packages/cli/src/utils.ts

@@ -92,3 +92,33 @@ export const shouldAssignExecuteMethod = (nodeType: INodeType) => {
 		(!nodeType.methods || isDeclarativeNode)
 	);
 };
+
+/**
+ * Recursively gets all key paths of an object or array, filtered by the provided value filter.
+ * @param obj - The object or array to search.
+ * @param keys - The array to store matching keys.
+ * @param valueFilter - A function to filter values.
+ * @returns The array of matching key paths.
+ */
+export const getAllKeyPaths = (
+	obj: unknown,
+	currentPath = '',
+	paths: string[] = [],
+	valueFilter: (value: string) => boolean,
+): string[] => {
+	if (Array.isArray(obj)) {
+		obj.forEach((item, index) =>
+			getAllKeyPaths(item, `${currentPath}[${index}]`, paths, valueFilter),
+		);
+	} else if (obj && typeof obj === 'object') {
+		for (const [key, value] of Object.entries(obj)) {
+			const newPath = currentPath ? `${currentPath}.${key}` : key;
+			if (typeof value === 'string' && valueFilter(value)) {
+				paths.push(newPath);
+			} else {
+				getAllKeyPaths(value, newPath, paths, valueFilter);
+			}
+		}
+	}
+	return paths;
+};