fix(core): Prevent credentials saving if missing required properties (#22213)

packages/@n8n/backend-test-utils/src/random.ts

@@ -46,9 +46,10 @@ export const randomEmail = () => `${randomName()}@${randomName()}.${randomTopLev
 export const randomCredentialPayload = ({
 	isManaged = false,
 	isGlobal,
-}: { isManaged?: boolean; isGlobal?: boolean } = {}): CredentialPayload => ({
+	type,
+}: { isManaged?: boolean; isGlobal?: boolean; type?: string } = {}): CredentialPayload => ({
 	name: randomName(),
-	type: randomName(),
+	type: type ?? 'githubApi',
 	data: { accessToken: randomString(6, 16) },
 	isManaged,
 	isGlobal,

packages/cli/src/credentials/__tests__/credentials.service.test.ts

@@ -1,3 +1,4 @@
+import type { Logger } from '@n8n/backend-common';
 import type {
 	CredentialsEntity,
 	CredentialsRepository,
@@ -10,17 +11,17 @@ import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db';
 import { mock } from 'jest-mock-extended';
 import { CREDENTIAL_ERRORS, CredentialDataError, Credentials, type ErrorReporter } from 'n8n-core';
 import { CREDENTIAL_EMPTY_VALUE, type ICredentialType } from 'n8n-workflow';
-import type { Logger } from '@n8n/backend-common';
 
 import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
 import type { CredentialTypes } from '@/credential-types';
-import { CredentialsService } from '@/credentials/credentials.service';
 import type { CredentialsFinderService } from '@/credentials/credentials-finder.service';
+import { CredentialsService } from '@/credentials/credentials.service';
+import type { CredentialsHelper } from '@/credentials-helper';
+import type { ExternalHooks } from '@/external-hooks';
+import type { CredentialsTester } from '@/services/credentials-tester.service';
 import type { OwnershipService } from '@/services/ownership.service';
 import type { ProjectService } from '@/services/project.service.ee';
 import type { RoleService } from '@/services/role.service';
-import type { CredentialsTester } from '@/services/credentials-tester.service';
-import type { ExternalHooks } from '@/external-hooks';
 
 describe('CredentialsService', () => {
 	const credType = mock<ICredentialType>({
@@ -54,6 +55,7 @@ describe('CredentialsService', () => {
 	const roleService = mock<RoleService>();
 	const userRepository = mock<UserRepository>();
 	const credentialsFinderService = mock<CredentialsFinderService>();
+	const credentialsHelper = mock<CredentialsHelper>();
 
 	const service = new CredentialsService(
 		credentialsRepository,
@@ -69,6 +71,7 @@ describe('CredentialsService', () => {
 		roleService,
 		userRepository,
 		credentialsFinderService,
+		credentialsHelper,
 	);
 
 	beforeEach(() => jest.resetAllMocks());
@@ -1383,6 +1386,7 @@ describe('CredentialsService', () => {
 				id: 'project-1',
 			} as any);
 			projectService.getProjectRelationsForUser.mockResolvedValue([]);
+			credentialsHelper.getCredentialsProperties.mockReturnValue([]);
 		});
 
 		it('should allow owner to create global credential', async () => {
@@ -1476,4 +1480,96 @@ describe('CredentialsService', () => {
 			expect(savedCredential.isGlobal).toBeUndefined();
 		});
 	});
+
+	describe('createManagedCredential', () => {
+		const ownerUser = mock<User>({ id: 'owner-id', role: GLOBAL_OWNER_ROLE });
+
+		const credentialData = {
+			name: 'Managed Credential',
+			type: 'oauth2',
+			oauthProvider: 'google',
+			projectId: 'project-1',
+			data: { someData: 'value' },
+		};
+
+		beforeEach(async () => {
+			// Mock the save chain
+			roleService.addScopes.mockImplementation(
+				(c) =>
+					({
+						...c,
+						scopes: ['credential:read', 'credential:update'],
+					}) as any,
+			);
+			roleService.combineResourceScopes.mockReturnValue([
+				'credential:read',
+				'credential:update',
+			] as any);
+			sharedCredentialsRepository.findOne.mockResolvedValue({ role: 'credential:owner' } as any);
+			sharedCredentialsRepository.create.mockImplementation((data) => data as any);
+			sharedCredentialsRepository.find.mockResolvedValue([]);
+			externalHooks.run.mockResolvedValue();
+			projectService.getProjectWithScope.mockResolvedValue({
+				id: 'project-1',
+			} as any);
+			projectService.getProjectRelationsForUser.mockResolvedValue([]);
+		});
+
+		it('should throw BadRequestError when credential is missing required properties', async () => {
+			// ARRANGE
+			const payload = { ...credentialData };
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'required prop',
+					name: 'requiredProp',
+					type: 'string',
+					required: true,
+					default: null,
+				},
+			]);
+
+			// ACT
+			await expect(service.createManagedCredential(payload, ownerUser)).rejects.toThrow(
+				'The field "requiredProp" is mandatory for credentials of type "oauth2"',
+			);
+		});
+
+		it('should create managed credential when all required properties are provided', async () => {
+			// ARRANGE
+			const payload = {
+				...credentialData,
+				data: {
+					requiredProp: 'some-value',
+				},
+			};
+			credentialsHelper.getCredentialsProperties.mockReturnValue([
+				{
+					displayName: 'required prop',
+					name: 'requiredProp',
+					type: 'string',
+					required: true,
+					default: null,
+				},
+			]);
+			credentialsRepository.create.mockImplementation((data) => ({ ...data }) as any);
+			// @ts-expect-error - Mocking manager for testing
+			credentialsRepository.manager = {
+				transaction: jest.fn().mockImplementation(async (callback) => {
+					const mockManager = {
+						save: jest.fn().mockImplementation(async (entity) => {
+							return { ...entity, id: 'new-managed-cred-id' };
+						}),
+					};
+					return await callback(mockManager);
+				}),
+			};
+
+			// ACT
+			const result = await service.createManagedCredential(payload, ownerUser);
+
+			// ASSERT
+			expect(result).toHaveProperty('id', 'new-managed-cred-id');
+			expect(result).toHaveProperty('name', 'Managed Credential');
+		});
+	});
 });

packages/cli/src/credentials/credentials.service.ts

@@ -30,6 +30,7 @@ import type {
 import {
 	CREDENTIAL_EMPTY_VALUE,
 	deepCopy,
+	displayParameter,
 	isINodePropertyCollection,
 	NodeHelpers,
 	UnexpectedError,
@@ -39,7 +40,7 @@ import { CredentialsFinderService } from './credentials-finder.service';
 
 import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
 import { CredentialTypes } from '@/credential-types';
-import { createCredentialsFromCredentialsEntity } from '@/credentials-helper';
+import { createCredentialsFromCredentialsEntity, CredentialsHelper } from '@/credentials-helper';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { ExternalHooks } from '@/external-hooks';
@@ -76,6 +77,7 @@ export class CredentialsService {
 		private readonly roleService: RoleService,
 		private readonly userRepository: UserRepository,
 		private readonly credentialsFinderService: CredentialsFinderService,
+		private readonly credentialsHelper: CredentialsHelper,
 	) {}
 
 	private async addGlobalCredentials(
@@ -863,6 +865,23 @@ export class CredentialsService {
 		return await this.createCredential({ ...dto, isManaged: false }, user);
 	}
 
+	private checkCredentialData(type: string, data: ICredentialDataDecryptedObject) {
+		// check mandatory fields are present
+		const credentialProperties = this.credentialsHelper.getCredentialsProperties(type);
+		for (const property of credentialProperties) {
+			if (property.required && displayParameter(data, property, null, null)) {
+				const value = data[property.name];
+				if (value === undefined || value === null || value === '') {
+					throw new BadRequestError(
+						`The field "${property.name}" is mandatory for credentials of type "${type}"`,
+					);
+				}
+			}
+		}
+
+		// TODO: add further validation if needed
+	}
+
 	/**
 	 * Create a new managed credential in user's account and return it along the scopes.
 	 * Managed credentials are managed by n8n and cannot be edited by the user.
@@ -872,6 +891,7 @@ export class CredentialsService {
 	}
 
 	private async createCredential(opts: CreateCredentialOptions, user: User) {
+		this.checkCredentialData(opts.type, opts.data as ICredentialDataDecryptedObject);
 		const encryptedCredential = this.createEncryptedData({
 			id: null,
 			name: opts.name,

packages/cli/test/integration/access-control/built-in-roles.test.ts

@@ -60,6 +60,8 @@ describe('Built-in Role Matrix Testing', () => {
 			passwordReset: jest.fn(),
 		});
 
+		await utils.initCredentialsTypes();
+
 		// Create standard users
 		owner = await createOwner();
 		member1 = await createMember();

packages/cli/test/integration/access-control/cross-project-access.test.ts

@@ -63,6 +63,8 @@ describe('Cross-Project Access Control Tests', () => {
 			passwordReset: jest.fn(),
 		});
 
+		await utils.initCredentialsTypes();
+
 		// Create standard users
 		owner = await createOwner();
 		member1 = await createMember();

packages/cli/test/integration/access-control/custom-roles-functionality.test.ts

@@ -67,6 +67,8 @@ describe('Custom Role Functionality Tests', () => {
 			passwordReset: jest.fn(),
 		});
 
+		await utils.initCredentialsTypes();
+
 		// Create standard users
 		owner = await createOwner();
 		member1 = await createMember();

packages/cli/test/integration/access-control/resource-access-matrix.test.ts

@@ -64,6 +64,8 @@ describe('Resource Access Control Matrix Tests', () => {
 		ownerAgent = testServer.authAgentFor(owner);
 		testUserAgent = testServer.authAgentFor(testUser);
 
+		await utils.initCredentialsTypes();
+
 		// Create custom roles with specific scopes
 		workflowReadOnlyRole = await createCustomRoleWithScopeSlugs(
 			['workflow:read', 'workflow:list'],

packages/cli/test/integration/ai/ai.api.test.ts

@@ -16,7 +16,7 @@ import { AiService } from '@/services/ai.service';
 
 import { createOwner } from '../shared/db/users';
 import type { SuperAgentTest } from '../shared/types';
-import { setupTestServer } from '../shared/utils';
+import { initCredentialsTypes, setupTestServer } from '../shared/utils';
 
 const createAiCreditsResponse = {
 	apiKey: randomUUID(),
@@ -37,6 +37,10 @@ let ownerPersonalProject: Project;
 
 let authOwnerAgent: SuperAgentTest;
 
+beforeAll(async () => {
+	await initCredentialsTypes();
+});
+
 beforeEach(async () => {
 	await testDb.truncate(['SharedCredentials', 'CredentialsEntity']);
 

packages/cli/test/integration/credentials/credentials.api.ee.test.ts

@@ -62,6 +62,8 @@ let projectRepository: ProjectRepository;
 
 beforeAll(async () => {
 	await Container.get(RoleCacheService).refreshCache();
+
+	await utils.initCredentialsTypes();
 });
 
 beforeEach(async () => {

packages/cli/test/integration/credentials/credentials.api.test.ts

@@ -20,6 +20,7 @@ import { randomString } from 'n8n-workflow';
 
 import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
 import { CredentialsService } from '@/credentials/credentials.service';
+import { createCredentialsFromCredentialsEntity } from '@/credentials-helper';
 import { CredentialsTester } from '@/services/credentials-tester.service';
 
 import {
@@ -31,11 +32,13 @@ import {
 } from '../shared/db/credentials';
 import { createAdmin, createManyUsers, createMember, createOwner } from '../shared/db/users';
 import type { SuperAgentTest } from '../shared/types';
-import { setupTestServer } from '../shared/utils';
+import { initCredentialsTypes, setupTestServer } from '../shared/utils';
 
 const { any } = expect;
 
-const testServer = setupTestServer({ endpointGroups: ['credentials'] });
+const testServer = setupTestServer({
+	endpointGroups: ['credentials'],
+});
 
 let owner: User;
 let member: User;
@@ -52,6 +55,10 @@ let authAdminAgent: SuperAgentTest;
 let projectRepository: ProjectRepository;
 let sharedCredentialsRepository: SharedCredentialsRepository;
 
+beforeAll(async () => {
+	await initCredentialsTypes();
+});
+
 beforeEach(async () => {
 	await testDb.truncate(['SharedCredentials', 'CredentialsEntity']);
 
@@ -792,7 +799,6 @@ describe('POST /credentials', () => {
 		const payload = randomCredentialPayload();
 
 		const response = await authMemberAgent.post('/credentials').send(payload);
-
 		expect(response.statusCode).toBe(200);
 
 		const { id, name, type, data: encryptedData, scopes } = response.body.data;
@@ -1301,15 +1307,16 @@ describe('PATCH /credentials/:id', () => {
 			.query({ includeData: true })
 			.expect(200);
 
-		const { id, data } = response.body.data;
+		const { id } = response.body.data;
 
 		expect(id).toBe(savedCredential.id);
-		// was overwritten
-		expect(data.accessToken).toBe(patchPayload.data.accessToken);
 		// was not overwritten
 		const dbCredential = await getCredentialById(savedCredential.id);
-		const unencryptedData = Container.get(CredentialsService).decrypt(dbCredential!);
-		expect(unencryptedData.oauthTokenData).toEqual(credential.data.oauthTokenData);
+		const unencryptedData = createCredentialsFromCredentialsEntity(dbCredential!);
+		expect(unencryptedData.getData().oauthTokenData).toEqual(credential.data.oauthTokenData);
+
+		// was overwritten
+		expect(unencryptedData.getData().accessToken).toBe(patchPayload.data.accessToken);
 	});
 
 	test('should fail with invalid inputs', async () => {

packages/cli/test/integration/shared/utils/index.ts

@@ -13,6 +13,9 @@ import {
 } from 'n8n-core';
 import { Ftp } from 'n8n-nodes-base/credentials/Ftp.credentials';
 import { GithubApi } from 'n8n-nodes-base/credentials/GithubApi.credentials';
+import { HttpBasicAuth } from 'n8n-nodes-base/credentials/HttpBasicAuth.credentials';
+import { HttpHeaderAuth } from 'n8n-nodes-base/credentials/HttpHeaderAuth.credentials';
+import { OpenAiApi } from 'n8n-nodes-base/credentials/OpenAiApi.credentials';
 import { Cron } from 'n8n-nodes-base/nodes/Cron/Cron.node';
 import { FormTrigger } from 'n8n-nodes-base/nodes/Form/FormTrigger.node';
 import { ScheduleTrigger } from 'n8n-nodes-base/nodes/Schedule/ScheduleTrigger.node';
@@ -64,6 +67,18 @@ export async function initCredentialsTypes(): Promise<void> {
 			type: new Ftp(),
 			sourcePath: '',
 		},
+		openAiApi: {
+			type: new OpenAiApi(),
+			sourcePath: '',
+		},
+		httpHeaderAuth: {
+			type: new HttpHeaderAuth(),
+			sourcePath: '',
+		},
+		httpBasicAuth: {
+			type: new HttpBasicAuth(),
+			sourcePath: '',
+		},
 	};
 }
 

packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue

@@ -690,6 +690,7 @@ async function saveCredential(): Promise<ICredentialsResponse | null> {
 	if (!requiredPropertiesFilled.value) {
 		showValidationWarning.value = true;
 		scrollToTop();
+		return null;
 	} else {
 		showValidationWarning.value = false;
 	}

packages/testing/playwright/composables/TemplatesComposer.ts

@@ -31,10 +31,14 @@ export class TemplatesComposer {
 	 * Opens credential creation, fills name, saves, and closes modal
 	 * @param appName - The name of the app (e.g. 'Shopify', 'X (Formerly Twitter)')
 	 */
-	async fillDummyCredentialForApp(appName: string): Promise<void> {
+	async fillDummyCredentialForApp(
+		appName: string,
+		{ fields }: { fields: Record<string, string> } = { fields: {} },
+	): Promise<void> {
 		await this.n8n.templateCredentialSetup.openCredentialCreation(appName);
 		await this.n8n.templateCredentialSetup.credentialModal.getCredentialName().click();
 		await this.n8n.templateCredentialSetup.credentialModal.getNameInput().fill('test');
+		await this.n8n.templateCredentialSetup.credentialModal.fillAllFields(fields);
 		await this.n8n.templateCredentialSetup.credentialModal.save();
 		await this.n8n.templateCredentialSetup.credentialModal.close();
 	}
@@ -43,8 +47,11 @@ export class TemplatesComposer {
 	 * Fill in dummy credentials for an app and handle confirmation dialog
 	 * @param appName - The name of the app
 	 */
-	async fillDummyCredentialForAppWithConfirm(appName: string): Promise<void> {
-		await this.fillDummyCredentialForApp(appName);
+	async fillDummyCredentialForAppWithConfirm(
+		appName: string,
+		{ fields }: { fields: Record<string, string> } = { fields: {} },
+	): Promise<void> {
+		await this.fillDummyCredentialForApp(appName, { fields });
 		await this.n8n.templateCredentialSetup.dismissMessageBox();
 	}
 }

packages/testing/playwright/tests/ui/34-template-credentials-setup.spec.ts

@@ -113,12 +113,18 @@ test.describe('Template credentials setup @db:reset', () => {
 
 		await expect(n8n.templateCredentialSetup.getContinueButton()).toBeDisabled();
 
-		await n8n.templatesComposer.fillDummyCredentialForApp('Shopify');
+		await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', {
+			fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' },
+		});
 
 		await expect(n8n.templateCredentialSetup.getContinueButton()).toBeEnabled();
 
-		await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)');
-		await n8n.templatesComposer.fillDummyCredentialForApp('Telegram');
+		await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)', {
+			fields: { consumerKey: 'consumer-key', consumerSecret: 'consumer-secret' },
+		});
+		await n8n.templatesComposer.fillDummyCredentialForApp('Telegram', {
+			fields: { accessToken: 'bot-token' },
+		});
 
 		await n8n.notifications.quickCloseAll();
 
@@ -214,7 +220,9 @@ test.describe('Template credentials setup @db:reset', () => {
 			n8n,
 		}) => {
 			await n8n.navigate.toTemplateCredentialSetup(TEMPLATE_ID);
-			await n8n.templatesComposer.fillDummyCredentialForApp('Shopify');
+			await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', {
+				fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' },
+			});
 
 			await n8n.notifications.quickCloseAll();
 
@@ -232,9 +240,15 @@ test.describe('Template credentials setup @db:reset', () => {
 			await n8n.canvas.getSetupWorkflowCredentialsButton().click();
 			await expect(n8n.workflowCredentialSetupModal.getModal()).toBeVisible();
 
-			await n8n.templatesComposer.fillDummyCredentialForApp('Shopify');
-			await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)');
-			await n8n.templatesComposer.fillDummyCredentialForApp('Telegram');
+			await n8n.templatesComposer.fillDummyCredentialForApp('Shopify', {
+				fields: { shopSubdomain: 'test-shop', apiKey: 'test-token', password: 'test-key' },
+			});
+			await n8n.templatesComposer.fillDummyCredentialForAppWithConfirm('X (Formerly Twitter)', {
+				fields: { consumerKey: 'consumer-key', consumerSecret: 'consumer-secret' },
+			});
+			await n8n.templatesComposer.fillDummyCredentialForApp('Telegram', {
+				fields: { accessToken: 'bot-token' },
+			});
 
 			await n8n.notifications.quickCloseAll();
 

packages/workflow/src/node-helpers.ts

@@ -2,11 +2,11 @@
 
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/prefer-nullish-coalescing */
+import { ApplicationError } from '@n8n/errors';
 import get from 'lodash/get';
 import isEqual from 'lodash/isEqual';
 
 import { EXECUTE_WORKFLOW_NODE_TYPE, WORKFLOW_TOOL_LANGCHAIN_NODE_TYPE } from './constants';
-import { ApplicationError } from '@n8n/errors';
 import { NodeConnectionTypes } from './interfaces';
 import type {
 	FieldType,
@@ -34,6 +34,7 @@ import type {
 	GenericValue,
 	DisplayCondition,
 	NodeConnectionType,
+	ICredentialDataDecryptedObject,
 } from './interfaces';
 import { validateFilterParameter } from './node-parameters/filter-parameter';
 import type { IRunExecutionData } from './run-execution-data/run-execution-data';
@@ -252,11 +253,11 @@ export function isSubNodeType(
 }
 
 const getPropertyValues = (
-	nodeValues: INodeParameters,
+	nodeValues: INodeParameters | ICredentialDataDecryptedObject,
 	propertyName: string,
 	node: Pick<INode, 'typeVersion'> | null,
 	nodeTypeDescription: INodeTypeDescription | null,
-	nodeValuesRoot: INodeParameters,
+	nodeValuesRoot: INodeParameters | ICredentialDataDecryptedObject,
 ) => {
 	let value;
 	if (propertyName.charAt(0) === '/') {
@@ -350,11 +351,11 @@ const checkConditions = (
  * @param {INodeParameters} [nodeValuesRoot] The root node-parameter-data
  */
 export function displayParameter(
-	nodeValues: INodeParameters,
+	nodeValues: INodeParameters | ICredentialDataDecryptedObject,
 	parameter: INodeProperties | INodeCredentialDescription | INodePropertyOptions,
 	node: Pick<INode, 'typeVersion'> | null, // Allow null as it does also get used by credentials and they do not have versioning yet
 	nodeTypeDescription: INodeTypeDescription | null,
-	nodeValuesRoot?: INodeParameters,
+	nodeValuesRoot?: INodeParameters | ICredentialDataDecryptedObject,
 	displayKey: 'displayOptions' | 'disabledOptions' = 'displayOptions',
 ) {
 	if (!parameter[displayKey]) {