fix(core): Prevent unauthorised workflow termination (#16405)

packages/cli/src/executions/__tests__/execution.service.test.ts

@@ -70,12 +70,13 @@ describe('ExecutionService', () => {
 			/**
 			 * Arrange
 			 */
-			executionRepository.findSingleExecution.mockResolvedValue(undefined);
+			executionRepository.findWithUnflattenedData.mockResolvedValue(undefined);
+			const req = mock<ExecutionRequest.Stop>({ params: { id: '1234' } });
 
 			/**
 			 * Act
 			 */
-			const stop = executionService.stop('inexistent-123');
+			const stop = executionService.stop(req.params.id, []);
 
 			/**
 			 * Assert
@@ -88,12 +89,13 @@ describe('ExecutionService', () => {
 			 * Arrange
 			 */
 			const execution = mock<IExecutionResponse>({ id: '123', status: 'success' });
-			executionRepository.findSingleExecution.mockResolvedValue(execution);
+			executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
+			const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
 
 			/**
 			 * Act
 			 */
-			const stop = executionService.stop(execution.id);
+			const stop = executionService.stop(req.params.id, [execution.id]);
 
 			/**
 			 * Assert
@@ -107,16 +109,18 @@ describe('ExecutionService', () => {
 				 * Arrange
 				 */
 				const execution = mock<IExecutionResponse>({ id: '123', status: 'running' });
-				executionRepository.findSingleExecution.mockResolvedValue(execution);
+				executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 				concurrencyControl.has.mockReturnValue(false);
 				activeExecutions.has.mockReturnValue(true);
 				waitTracker.has.mockReturnValue(false);
 				executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
 
+				const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+
 				/**
 				 * Act
 				 */
-				await executionService.stop(execution.id);
+				await executionService.stop(req.params.id, [execution.id]);
 
 				/**
 				 * Assert
@@ -132,16 +136,18 @@ describe('ExecutionService', () => {
 				 * Arrange
 				 */
 				const execution = mock<IExecutionResponse>({ id: '123', status: 'waiting' });
-				executionRepository.findSingleExecution.mockResolvedValue(execution);
+				executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 				concurrencyControl.has.mockReturnValue(false);
 				activeExecutions.has.mockReturnValue(true);
 				waitTracker.has.mockReturnValue(true);
 				executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
 
+				const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+
 				/**
 				 * Act
 				 */
-				await executionService.stop(execution.id);
+				await executionService.stop(req.params.id, [execution.id]);
 
 				/**
 				 * Assert
@@ -157,16 +163,18 @@ describe('ExecutionService', () => {
 				 * Arrange
 				 */
 				const execution = mock<IExecutionResponse>({ id: '123', status: 'new', mode: 'trigger' });
-				executionRepository.findSingleExecution.mockResolvedValue(execution);
+				executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 				concurrencyControl.has.mockReturnValue(true);
 				activeExecutions.has.mockReturnValue(false);
 				waitTracker.has.mockReturnValue(false);
 				executionRepository.stopBeforeRun.mockResolvedValue(mock<IExecutionResponse>());
 
+				const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+
 				/**
 				 * Act
 				 */
-				await executionService.stop(execution.id);
+				await executionService.stop(req.params.id, [execution.id]);
 
 				/**
 				 * Assert
@@ -193,11 +201,13 @@ describe('ExecutionService', () => {
 						mode: 'manual',
 						status: 'running',
 					});
-					executionRepository.findSingleExecution.mockResolvedValue(execution);
+					executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 					concurrencyControl.has.mockReturnValue(false);
 					activeExecutions.has.mockReturnValue(true);
 					waitTracker.has.mockReturnValue(false);
-					const job = mock<Job>({ data: { executionId: '123' } });
+
+					const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+					const job = mock<Job>({ data: { executionId: execution.id } });
 					scalingService.findJobsByStatus.mockResolvedValue([job]);
 					executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
 					// @ts-expect-error Private method
@@ -206,7 +216,7 @@ describe('ExecutionService', () => {
 					/**
 					 * Act
 					 */
-					await executionService.stop(execution.id);
+					await executionService.stop(req.params.id, [execution.id]);
 
 					/**
 					 * Assert
@@ -228,16 +238,18 @@ describe('ExecutionService', () => {
 					 */
 					config.set('executions.mode', 'queue');
 					const execution = mock<IExecutionResponse>({ id: '123', status: 'running' });
-					executionRepository.findSingleExecution.mockResolvedValue(execution);
+					executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 					waitTracker.has.mockReturnValue(false);
-					const job = mock<Job>({ data: { executionId: '123' } });
+
+					const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+					const job = mock<Job>({ data: { executionId: execution.id } });
 					scalingService.findJobsByStatus.mockResolvedValue([job]);
 					executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
 
 					/**
 					 * Act
 					 */
-					await executionService.stop(execution.id);
+					await executionService.stop(req.params.id, [execution.id]);
 
 					/**
 					 * Assert
@@ -255,16 +267,18 @@ describe('ExecutionService', () => {
 					 */
 					config.set('executions.mode', 'queue');
 					const execution = mock<IExecutionResponse>({ id: '123', status: 'waiting' });
-					executionRepository.findSingleExecution.mockResolvedValue(execution);
+					executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
 					waitTracker.has.mockReturnValue(true);
-					const job = mock<Job>({ data: { executionId: '123' } });
+
+					const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
+					const job = mock<Job>({ data: { executionId: execution.id } });
 					scalingService.findJobsByStatus.mockResolvedValue([job]);
 					executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
 
 					/**
 					 * Act
 					 */
-					await executionService.stop(execution.id);
+					await executionService.stop(req.params.id, [execution.id]);
 
 					/**
 					 * Assert

packages/cli/src/executions/__tests__/executions.controller.test.ts

@@ -138,7 +138,7 @@ describe('ExecutionsController', () => {
 		const executionId = '999';
 		const req = mock<ExecutionRequest.Stop>({ params: { id: executionId } });
 
-		it('should 404 when execution is inaccessible for user', async () => {
+		it('should throw expected NotFoundError when all workflows are inaccessible for user', async () => {
 			workflowSharingService.getSharedWorkflowIds.mockResolvedValue([]);
 
 			const promise = executionsController.stop(req);
@@ -147,12 +147,12 @@ describe('ExecutionsController', () => {
 			expect(executionService.stop).not.toHaveBeenCalled();
 		});
 
-		it('should call ask for an execution to be stopped', async () => {
+		it('should call execution service with expected data when user has accessible workflows', async () => {
-			workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['123']);
+			const mockAccessibleWorkflowIds = ['1234', '999'];
+			workflowSharingService.getSharedWorkflowIds.mockResolvedValue(mockAccessibleWorkflowIds);
 
 			await executionsController.stop(req);
-
+			expect(executionService.stop).toHaveBeenCalledWith(req.params.id, mockAccessibleWorkflowIds);
-			expect(executionService.stop).toHaveBeenCalledWith(executionId);
 		});
 	});
 });

packages/cli/src/executions/execution.service.ts

@@ -420,13 +420,19 @@ export class ExecutionService {
 		);
 	}
 
-	async stop(executionId: string): Promise<StopResult> {
+	async stop(executionId: string, sharedWorkflowIds: string[]): Promise<StopResult> {
-		const execution = await this.executionRepository.findSingleExecution(executionId, {
+		const execution = await this.executionRepository.findWithUnflattenedData(
-			includeData: true,
+			executionId,
-			unflattenData: true,
+			sharedWorkflowIds,
-		});
+		);
 
-		if (!execution) throw new MissingExecutionStopError(executionId);
+		if (!execution) {
+			this.logger.info(`Unable to stop execution "${executionId}" as it was not found`, {
+				executionId,
+			});
+
+			throw new MissingExecutionStopError(executionId);
+		}
 
 		this.assertStoppable(execution);
 

packages/cli/src/executions/executions.controller.ts

@@ -96,7 +96,9 @@ export class ExecutionsController {
 
 		if (workflowIds.length === 0) throw new NotFoundError('Execution not found');
 
-		return await this.executionService.stop(req.params.id);
+		const executionId = req.params.id;
+
+		return await this.executionService.stop(executionId, workflowIds);
 	}
 
 	@Post('/:id/retry')

packages/cli/test/integration/executions.controller.test.ts

@@ -3,7 +3,11 @@ import type { User } from '@n8n/db';
 import { ConcurrencyControlService } from '@/concurrency/concurrency-control.service';
 import { WaitTracker } from '@/wait-tracker';
 
-import { createSuccessfulExecution, getAllExecutions } from './shared/db/executions';
+import {
+	createSuccessfulExecution,
+	createWaitingExecution,
+	getAllExecutions,
+} from './shared/db/executions';
 import { createTeamProject, linkUserToProject } from './shared/db/projects';
 import { createMember, createOwner } from './shared/db/users';
 import { createWorkflow, shareWorkflowWithUsers } from './shared/db/workflows';
@@ -27,6 +31,11 @@ const saveExecution = async ({ belongingTo }: { belongingTo: User }) => {
 	return await createSuccessfulExecution(workflow);
 };
 
+const saveWaitingExecution = async ({ belongingTo }: { belongingTo: User }) => {
+	const workflow = await createWorkflow({}, belongingTo);
+	return await createWaitingExecution(workflow);
+};
+
 beforeEach(async () => {
 	await testDb.truncate(['ExecutionEntity', 'WorkflowEntity', 'SharedWorkflow']);
 	testServer.license.reset();
@@ -117,3 +126,21 @@ describe('POST /executions/delete', () => {
 		expect(executions).toHaveLength(0);
 	});
 });
+
+describe('POST /executions/stop', () => {
+	test('should not stop an execution we do not have access to', async () => {
+		await saveExecution({ belongingTo: owner });
+		const incorrectExecutionId = '1234';
+
+		await testServer
+			.authAgentFor(owner)
+			.post(`/executions/${incorrectExecutionId}/stop`)
+			.expect(500);
+	});
+
+	test('should stop an execution we have access to', async () => {
+		const execution = await saveWaitingExecution({ belongingTo: owner });
+
+		await testServer.authAgentFor(owner).post(`/executions/${execution.id}/stop`).expect(200);
+	});
+});