fix(core): Enforce credential access checks in dynamic node parameter requests (backport to release-candidate/2.17.x) (#28861)

Co-authored-by: Stephen Wright <sjw948@gmail.com>

packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts

@@ -11,19 +11,25 @@ import {
 import { DynamicNodeParametersService } from '../dynamic-node-parameters.service';
 import { WorkflowLoaderService } from '../workflow-loader.service';
 
+import { CredentialsFinderService } from '@/credentials/credentials-finder.service';
+import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
 import { NodeTypes } from '@/node-types';
+import * as checkAccess from '@/permissions.ee/check-access';
 import { SharedWorkflowRepository } from '@n8n/db';
+import type { User } from '@n8n/db';
 
 describe('DynamicNodeParametersService', () => {
 	const logger = mockInstance(Logger);
 	const nodeTypes = mockInstance(NodeTypes);
 	const workflowLoaderService = mockInstance(WorkflowLoaderService);
 	const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);
+	const credentialsFinderService = mockInstance(CredentialsFinderService);
 	const service = new DynamicNodeParametersService(
 		logger,
 		nodeTypes,
 		workflowLoaderService,
 		sharedWorkflowRepository,
+		credentialsFinderService,
 	);
 
 	beforeEach(() => {
@@ -124,4 +130,132 @@ describe('DynamicNodeParametersService', () => {
 			});
 		});
 	});
+
+	describe('refineResourceIds', () => {
+		const user = mock<User>();
+
+		beforeEach(() => {
+			jest.spyOn(checkAccess, 'userHasScopes').mockResolvedValue(true);
+			sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(undefined);
+		});
+
+		afterEach(() => {
+			jest.restoreAllMocks();
+		});
+
+		it('should not call findCredentialIdsWithScopeForUser when no credentials provided', async () => {
+			await service.refineResourceIds(user, { credentials: undefined });
+			expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled();
+		});
+
+		it('should not call findCredentialIdsWithScopeForUser when credentials object is empty', async () => {
+			await service.refineResourceIds(user, { credentials: {} });
+			expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled();
+		});
+
+		it('should not call findCredentialIdsWithScopeForUser when all credential entries have no id', async () => {
+			await service.refineResourceIds(user, {
+				credentials: { openAi: { id: null, name: 'My OpenAI' } },
+			});
+			expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled();
+		});
+
+		it('should allow request when user has access to all supplied credentials', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(
+				new Set(['cred-1']),
+			);
+
+			await expect(
+				service.refineResourceIds(user, {
+					credentials: { openAi: { id: 'cred-1', name: 'My OpenAI' } },
+				}),
+			).resolves.not.toThrow();
+		});
+
+		it('should throw when user does not have access to a credential', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set());
+
+			await expect(
+				service.refineResourceIds(user, {
+					credentials: { openAi: { id: 'cred-1', name: 'Foreign OpenAI' } },
+				}),
+			).rejects.toThrow(ForbiddenError);
+		});
+
+		it('should throw when any credential in a multi-credential request is inaccessible', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(
+				new Set(['cred-1']),
+			);
+
+			await expect(
+				service.refineResourceIds(user, {
+					credentials: {
+						openAi: { id: 'cred-1', name: 'My OpenAI' },
+						openAi2: { id: 'cred-2', name: 'Foreign OpenAI' },
+					},
+				}),
+			).rejects.toThrow(ForbiddenError);
+		});
+
+		it('should allow request when all of multiple credentials are accessible', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(
+				new Set(['cred-1', 'cred-2']),
+			);
+
+			await expect(
+				service.refineResourceIds(user, {
+					credentials: {
+						openAi: { id: 'cred-1', name: 'My OpenAI' },
+						openAi2: { id: 'cred-2', name: 'My OpenAI 2' },
+					},
+				}),
+			).resolves.not.toThrow();
+		});
+
+		it('should skip credential entries without an id when validating', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(
+				new Set(['cred-1']),
+			);
+
+			await service.refineResourceIds(user, {
+				credentials: {
+					anon: { id: null, name: 'anonymous' },
+					named: { id: 'cred-1', name: 'Named Credential' },
+				},
+			});
+
+			expect(credentialsFinderService.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith(
+				['cred-1'],
+				user,
+				['credential:read'],
+			);
+		});
+
+		it('should call findCredentialIdsWithScopeForUser with credential:read scope', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(
+				new Set(['cred-1']),
+			);
+
+			await service.refineResourceIds(user, {
+				credentials: { openAi: { id: 'cred-1', name: 'My OpenAI' } },
+			});
+
+			expect(credentialsFinderService.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith(
+				expect.any(Array),
+				user,
+				['credential:read'],
+			);
+		});
+
+		it('should run credential check regardless of whether workflowId is present', async () => {
+			credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set());
+
+			await expect(
+				service.refineResourceIds(user, {
+					credentials: { openAi: { id: 'cred-1', name: 'Foreign OpenAI' } },
+					// workflowId intentionally omitted
+				}),
+			).rejects.toThrow(ForbiddenError);
+		});
+	});
 });

packages/cli/src/services/dynamic-node-parameters.service.ts

@@ -23,6 +23,8 @@ import type {
 import { Workflow, UnexpectedError, createEmptyRunExecutionData } from 'n8n-workflow';
 
 import { NodeTypes } from '@/node-types';
+import { CredentialsFinderService } from '@/credentials/credentials-finder.service';
+import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
 
 import { WorkflowLoaderService } from './workflow-loader.service';
 import { SharedWorkflowRepository, User } from '@n8n/db';
@@ -58,9 +60,13 @@ export class DynamicNodeParametersService {
 		private nodeTypes: NodeTypes,
 		private workflowLoaderService: WorkflowLoaderService,
 		private sharedWorkflowRepository: SharedWorkflowRepository,
+		private credentialsFinderService: CredentialsFinderService,
 	) {}
 
-	async refineResourceIds(user: User, payload: { projectId?: string; workflowId?: string }) {
+	async refineResourceIds(
+		user: User,
+		payload: { projectId?: string; workflowId?: string; credentials?: INodeCredentials },
+	) {
 		// We want to avoid relying on generic project:read permissions to enable
 		// a future with fine-grained permission control dependent on the respective resource
 		// For now we use the dataTable:listProject scope as this is the existing consumer of
@@ -77,25 +83,41 @@ export class DynamicNodeParametersService {
 			payload.projectId = undefined;
 		}
 
-		if (!payload.workflowId) return;
+		if (payload.workflowId) {
+			const hasAccess = await userHasScopes(user, ['workflow:read'], false, {
+				workflowId: payload.workflowId,
+			});
 
-		const hasAccess = await userHasScopes(user, ['workflow:read'], false, {
-			workflowId: payload.workflowId,
-		});
-
-		if (!hasAccess) {
-			this.logger.warn(
-				`Scrubbed inaccessible workflowId ${payload.workflowId} from DynamicNodeParameters request`,
-			);
-			payload.workflowId = undefined;
-			return;
+			if (!hasAccess) {
+				this.logger.warn(
+					`Scrubbed inaccessible workflowId ${payload.workflowId} from DynamicNodeParameters request`,
+				);
+				payload.workflowId = undefined;
+			} else if (payload.projectId === undefined) {
+				const project = await this.sharedWorkflowRepository.getWorkflowOwningProject(
+					payload.workflowId,
+				);
+				payload.projectId = project?.id;
+			}
 		}
 
-		if (payload.projectId === undefined) {
-			const project = await this.sharedWorkflowRepository.getWorkflowOwningProject(
-				payload.workflowId,
-			);
-			payload.projectId = project?.id;
+		if (payload.credentials) {
+			const credentialIds = Object.values(payload.credentials)
+				.map((details) => details.id)
+				.filter((id): id is string => id !== undefined && id !== null);
+
+			if (credentialIds.length > 0) {
+				const accessibleIds = await this.credentialsFinderService.findCredentialIdsWithScopeForUser(
+					credentialIds,
+					user,
+					['credential:read'],
+				);
+
+				const forbiddenId = credentialIds.find((id) => !accessibleIds.has(id));
+				if (forbiddenId !== undefined) {
+					throw new ForbiddenError();
+				}
+			}
 		}
 	}
 

packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts

@@ -1,4 +1,5 @@
-import { mockInstance } from '@n8n/backend-test-utils';
+import { randomCredentialPayload, testDb } from '@n8n/backend-test-utils';
+import { Container } from '@n8n/di';
 import { mock } from 'jest-mock-extended';
 import type {
 	INodeListSearchResult,
@@ -10,20 +11,20 @@ import type {
 import { DynamicNodeParametersService } from '@/services/dynamic-node-parameters.service';
 import * as AdditionalData from '@/workflow-execute-additional-data';
 
-import { createOwner } from '../shared/db/users';
+import { saveCredential } from '../shared/db/credentials';
+import { createMember, createOwner } from '../shared/db/users';
 import type { SuperAgentTest } from '../shared/types';
 import { setupTestServer } from '../shared/utils';
 
 describe('DynamicNodeParametersController', () => {
 	const additionalData = mock<IWorkflowExecuteAdditionalData>();
-	const service = mockInstance(DynamicNodeParametersService);
 
 	const testServer = setupTestServer({ endpointGroups: ['dynamic-node-parameters'] });
 	let ownerAgent: SuperAgentTest;
+	let service: DynamicNodeParametersService;
 
-	beforeAll(async () => {
-		const owner = await createOwner();
-		ownerAgent = testServer.authAgentFor(owner);
+	beforeAll(() => {
+		service = Container.get(DynamicNodeParametersService);
 	});
 
 	beforeEach(() => {
@@ -31,6 +32,10 @@ describe('DynamicNodeParametersController', () => {
 		jest.spyOn(AdditionalData, 'getBase').mockResolvedValue(additionalData);
 	});
 
+	afterEach(() => {
+		jest.restoreAllMocks();
+	});
+
 	const commonRequestParams = {
 		credentials: {},
 		currentNodeParameters: {},
@@ -40,7 +45,10 @@ describe('DynamicNodeParametersController', () => {
 
 	describe('POST /dynamic-node-parameters/options', () => {
 		it('should take params via body', async () => {
-			service.getOptionsViaMethodName.mockResolvedValue([]);
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
+			jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]);
 
 			await ownerAgent
 				.post('/dynamic-node-parameters/options')
@@ -52,8 +60,11 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should take params with loadOptions', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const expectedResult = [{ name: 'Test Option', value: 'test' }];
-			service.getOptionsViaLoadOptions.mockResolvedValue(expectedResult);
+			jest.spyOn(service, 'getOptionsViaLoadOptions').mockResolvedValue(expectedResult);
 
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/options')
@@ -67,6 +78,9 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should return empty array when no method or loadOptions provided', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/options')
 				.send({
@@ -80,8 +94,11 @@ describe('DynamicNodeParametersController', () => {
 
 	describe('POST /dynamic-node-parameters/resource-locator-results', () => {
 		it('should return resource locator results', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const expectedResult: INodeListSearchResult = { results: [] };
-			service.getResourceLocatorResults.mockResolvedValue(expectedResult);
+			jest.spyOn(service, 'getResourceLocatorResults').mockResolvedValue(expectedResult);
 
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/resource-locator-results')
@@ -97,8 +114,10 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should handle resource locator results without pagination', async () => {
-			const mockResults = mock<INodeListSearchResult>();
-			service.getResourceLocatorResults.mockResolvedValue(mockResults);
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
+			jest.spyOn(service, 'getResourceLocatorResults').mockResolvedValue({ results: [] });
 
 			await ownerAgent
 				.post('/dynamic-node-parameters/resource-locator-results')
@@ -110,6 +129,9 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should return a 400 if methodName is not defined', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			await ownerAgent
 				.post('/dynamic-node-parameters/resource-locator-results')
 				.send(commonRequestParams)
@@ -119,8 +141,11 @@ describe('DynamicNodeParametersController', () => {
 
 	describe('POST /dynamic-node-parameters/resource-mapper-fields', () => {
 		it('should return resource mapper fields', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const expectedResult: ResourceMapperFields = { fields: [] };
-			service.getResourceMappingFields.mockResolvedValue(expectedResult);
+			jest.spyOn(service, 'getResourceMappingFields').mockResolvedValue(expectedResult);
 
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/resource-mapper-fields')
@@ -135,6 +160,9 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should return a 400 if methodName is not defined', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			await ownerAgent
 				.post('/dynamic-node-parameters/resource-mapper-fields')
 				.send(commonRequestParams)
@@ -144,8 +172,11 @@ describe('DynamicNodeParametersController', () => {
 
 	describe('POST /dynamic-node-parameters/local-resource-mapper-fields', () => {
 		it('should return local resource mapper fields', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const expectedResult: ResourceMapperFields = { fields: [] };
-			service.getLocalResourceMappingFields.mockResolvedValue(expectedResult);
+			jest.spyOn(service, 'getLocalResourceMappingFields').mockResolvedValue(expectedResult);
 
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/local-resource-mapper-fields')
@@ -159,6 +190,9 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should return a 400 if methodName is not defined', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			await ownerAgent
 				.post('/dynamic-node-parameters/local-resource-mapper-fields')
 				.send(commonRequestParams)
@@ -168,8 +202,11 @@ describe('DynamicNodeParametersController', () => {
 
 	describe('POST /dynamic-node-parameters/action-result', () => {
 		it('should return action result with handler', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			const expectedResult: NodeParameterValueType = { test: true };
-			service.getActionResult.mockResolvedValue(expectedResult);
+			jest.spyOn(service, 'getActionResult').mockResolvedValue(expectedResult);
 
 			const response = await ownerAgent
 				.post('/dynamic-node-parameters/action-result')
@@ -184,6 +221,9 @@ describe('DynamicNodeParametersController', () => {
 		});
 
 		it('should return a 400 if handler is not defined', async () => {
+			const owner = await createOwner();
+			ownerAgent = testServer.authAgentFor(owner);
+
 			await ownerAgent
 				.post('/dynamic-node-parameters/action-result')
 				.send({
@@ -193,4 +233,128 @@ describe('DynamicNodeParametersController', () => {
 				.expect(400);
 		});
 	});
+
+	describe('credential access enforcement', () => {
+		let member: Awaited<ReturnType<typeof createMember>>;
+		let memberAgent: SuperAgentTest;
+
+		beforeEach(async () => {
+			await testDb.truncate(['SharedCredentials', 'CredentialsEntity']);
+			member = await createMember();
+			memberAgent = testServer.authAgentFor(member);
+		});
+
+		it('should return 403 when user does not have access to supplied credential on /options', async () => {
+			const owner = await createOwner();
+			const ownerCredential = await saveCredential(randomCredentialPayload(), {
+				user: owner,
+				role: 'credential:owner',
+			});
+
+			await memberAgent
+				.post('/dynamic-node-parameters/options')
+				.send({
+					...commonRequestParams,
+					credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } },
+				})
+				.expect(403);
+		});
+
+		it('should return 403 when user does not have access to supplied credential on /resource-locator-results', async () => {
+			const owner = await createOwner();
+			const ownerCredential = await saveCredential(randomCredentialPayload(), {
+				user: owner,
+				role: 'credential:owner',
+			});
+
+			await memberAgent
+				.post('/dynamic-node-parameters/resource-locator-results')
+				.send({
+					...commonRequestParams,
+					methodName: 'searchItems',
+					credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } },
+				})
+				.expect(403);
+		});
+
+		it('should return 403 when user does not have access to supplied credential on /resource-mapper-fields', async () => {
+			const owner = await createOwner();
+			const ownerCredential = await saveCredential(randomCredentialPayload(), {
+				user: owner,
+				role: 'credential:owner',
+			});
+
+			await memberAgent
+				.post('/dynamic-node-parameters/resource-mapper-fields')
+				.send({
+					...commonRequestParams,
+					methodName: 'getFields',
+					credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } },
+				})
+				.expect(403);
+		});
+
+		it('should return 403 when user does not have access to supplied credential on /action-result', async () => {
+			const owner = await createOwner();
+			const ownerCredential = await saveCredential(randomCredentialPayload(), {
+				user: owner,
+				role: 'credential:owner',
+			});
+
+			await memberAgent
+				.post('/dynamic-node-parameters/action-result')
+				.send({
+					...commonRequestParams,
+					handler: 'handleAction',
+					payload: {},
+					credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } },
+				})
+				.expect(403);
+		});
+
+		it('should return 403 when workflowId is absent but credential is inaccessible', async () => {
+			const owner = await createOwner();
+			const ownerCredential = await saveCredential(randomCredentialPayload(), {
+				user: owner,
+				role: 'credential:owner',
+			});
+
+			// No workflowId — ensures the early-return bypass path is also blocked
+			await memberAgent
+				.post('/dynamic-node-parameters/options')
+				.send({
+					...commonRequestParams,
+					credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } },
+				})
+				.expect(403);
+		});
+
+		it('should not return 403 when user has access to the supplied credential', async () => {
+			const ownCredential = await saveCredential(randomCredentialPayload(), {
+				user: member,
+				role: 'credential:owner',
+			});
+
+			jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]);
+
+			const { status } = await memberAgent.post('/dynamic-node-parameters/options').send({
+				...commonRequestParams,
+				methodName: 'testMethod',
+				credentials: { httpBasicAuth: { id: ownCredential.id, name: ownCredential.name } },
+			});
+
+			expect(status).not.toBe(403);
+		});
+
+		it('should not return 403 when no credentials are supplied', async () => {
+			jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]);
+
+			const { status } = await memberAgent.post('/dynamic-node-parameters/options').send({
+				...commonRequestParams,
+				methodName: 'testMethod',
+			});
+
+			expect(status).not.toBe(403);
+		});
+	});
 });