Skip to content

Commit e5edc60

Browse files
MarcLtomi
MarcL
authored and
tomi
committed
fix(core): Prevent unauthorised workflow termination (#16405)
1 parent 569e522 commit e5edc60

File tree

5 files changed

+81
-32
lines changed

5 files changed

+81
-32
lines changed

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

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,13 @@ describe('ExecutionService', () => {
7070
/**
7171
* Arrange
7272
*/
73-
executionRepository.findSingleExecution.mockResolvedValue(undefined);
73+
executionRepository.findWithUnflattenedData.mockResolvedValue(undefined);
74+
const req = mock<ExecutionRequest.Stop>({ params: { id: '1234' } });
7475

7576
/**
7677
* Act
7778
*/
78-
const stop = executionService.stop('inexistent-123');
79+
const stop = executionService.stop(req.params.id, []);
7980

8081
/**
8182
* Assert
@@ -88,12 +89,13 @@ describe('ExecutionService', () => {
8889
* Arrange
8990
*/
9091
const execution = mock<IExecutionResponse>({ id: '123', status: 'success' });
91-
executionRepository.findSingleExecution.mockResolvedValue(execution);
92+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
93+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
9294

9395
/**
9496
* Act
9597
*/
96-
const stop = executionService.stop(execution.id);
98+
const stop = executionService.stop(req.params.id, [execution.id]);
9799

98100
/**
99101
* Assert
@@ -107,16 +109,18 @@ describe('ExecutionService', () => {
107109
* Arrange
108110
*/
109111
const execution = mock<IExecutionResponse>({ id: '123', status: 'running' });
110-
executionRepository.findSingleExecution.mockResolvedValue(execution);
112+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
111113
concurrencyControl.has.mockReturnValue(false);
112114
activeExecutions.has.mockReturnValue(true);
113115
waitTracker.has.mockReturnValue(false);
114116
executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
115117

118+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
119+
116120
/**
117121
* Act
118122
*/
119-
await executionService.stop(execution.id);
123+
await executionService.stop(req.params.id, [execution.id]);
120124

121125
/**
122126
* Assert
@@ -132,16 +136,18 @@ describe('ExecutionService', () => {
132136
* Arrange
133137
*/
134138
const execution = mock<IExecutionResponse>({ id: '123', status: 'waiting' });
135-
executionRepository.findSingleExecution.mockResolvedValue(execution);
139+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
136140
concurrencyControl.has.mockReturnValue(false);
137141
activeExecutions.has.mockReturnValue(true);
138142
waitTracker.has.mockReturnValue(true);
139143
executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
140144

145+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
146+
141147
/**
142148
* Act
143149
*/
144-
await executionService.stop(execution.id);
150+
await executionService.stop(req.params.id, [execution.id]);
145151

146152
/**
147153
* Assert
@@ -157,16 +163,18 @@ describe('ExecutionService', () => {
157163
* Arrange
158164
*/
159165
const execution = mock<IExecutionResponse>({ id: '123', status: 'new', mode: 'trigger' });
160-
executionRepository.findSingleExecution.mockResolvedValue(execution);
166+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
161167
concurrencyControl.has.mockReturnValue(true);
162168
activeExecutions.has.mockReturnValue(false);
163169
waitTracker.has.mockReturnValue(false);
164170
executionRepository.stopBeforeRun.mockResolvedValue(mock<IExecutionResponse>());
165171

172+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
173+
166174
/**
167175
* Act
168176
*/
169-
await executionService.stop(execution.id);
177+
await executionService.stop(req.params.id, [execution.id]);
170178

171179
/**
172180
* Assert
@@ -193,11 +201,13 @@ describe('ExecutionService', () => {
193201
mode: 'manual',
194202
status: 'running',
195203
});
196-
executionRepository.findSingleExecution.mockResolvedValue(execution);
204+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
197205
concurrencyControl.has.mockReturnValue(false);
198206
activeExecutions.has.mockReturnValue(true);
199207
waitTracker.has.mockReturnValue(false);
200-
const job = mock<Job>({ data: { executionId: '123' } });
208+
209+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
210+
const job = mock<Job>({ data: { executionId: execution.id } });
201211
scalingService.findJobsByStatus.mockResolvedValue([job]);
202212
executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
203213
// @ts-expect-error Private method
@@ -206,7 +216,7 @@ describe('ExecutionService', () => {
206216
/**
207217
* Act
208218
*/
209-
await executionService.stop(execution.id);
219+
await executionService.stop(req.params.id, [execution.id]);
210220

211221
/**
212222
* Assert
@@ -228,16 +238,18 @@ describe('ExecutionService', () => {
228238
*/
229239
config.set('executions.mode', 'queue');
230240
const execution = mock<IExecutionResponse>({ id: '123', status: 'running' });
231-
executionRepository.findSingleExecution.mockResolvedValue(execution);
241+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
232242
waitTracker.has.mockReturnValue(false);
233-
const job = mock<Job>({ data: { executionId: '123' } });
243+
244+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
245+
const job = mock<Job>({ data: { executionId: execution.id } });
234246
scalingService.findJobsByStatus.mockResolvedValue([job]);
235247
executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
236248

237249
/**
238250
* Act
239251
*/
240-
await executionService.stop(execution.id);
252+
await executionService.stop(req.params.id, [execution.id]);
241253

242254
/**
243255
* Assert
@@ -255,16 +267,18 @@ describe('ExecutionService', () => {
255267
*/
256268
config.set('executions.mode', 'queue');
257269
const execution = mock<IExecutionResponse>({ id: '123', status: 'waiting' });
258-
executionRepository.findSingleExecution.mockResolvedValue(execution);
270+
executionRepository.findWithUnflattenedData.mockResolvedValue(execution);
259271
waitTracker.has.mockReturnValue(true);
260-
const job = mock<Job>({ data: { executionId: '123' } });
272+
273+
const req = mock<ExecutionRequest.Stop>({ params: { id: execution.id } });
274+
const job = mock<Job>({ data: { executionId: execution.id } });
261275
scalingService.findJobsByStatus.mockResolvedValue([job]);
262276
executionRepository.stopDuringRun.mockResolvedValue(mock<IExecutionResponse>());
263277

264278
/**
265279
* Act
266280
*/
267-
await executionService.stop(execution.id);
281+
await executionService.stop(req.params.id, [execution.id]);
268282

269283
/**
270284
* Assert

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ describe('ExecutionsController', () => {
138138
const executionId = '999';
139139
const req = mock<ExecutionRequest.Stop>({ params: { id: executionId } });
140140

141-
it('should 404 when execution is inaccessible for user', async () => {
141+
it('should throw expected NotFoundError when all workflows are inaccessible for user', async () => {
142142
workflowSharingService.getSharedWorkflowIds.mockResolvedValue([]);
143143

144144
const promise = executionsController.stop(req);
@@ -147,12 +147,12 @@ describe('ExecutionsController', () => {
147147
expect(executionService.stop).not.toHaveBeenCalled();
148148
});
149149

150-
it('should call ask for an execution to be stopped', async () => {
151-
workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['123']);
150+
it('should call execution service with expected data when user has accessible workflows', async () => {
151+
const mockAccessibleWorkflowIds = ['1234', '999'];
152+
workflowSharingService.getSharedWorkflowIds.mockResolvedValue(mockAccessibleWorkflowIds);
152153

153154
await executionsController.stop(req);
154-
155-
expect(executionService.stop).toHaveBeenCalledWith(executionId);
155+
expect(executionService.stop).toHaveBeenCalledWith(req.params.id, mockAccessibleWorkflowIds);
156156
});
157157
});
158158
});

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -420,13 +420,19 @@ export class ExecutionService {
420420
);
421421
}
422422

423-
async stop(executionId: string): Promise<StopResult> {
424-
const execution = await this.executionRepository.findSingleExecution(executionId, {
425-
includeData: true,
426-
unflattenData: true,
427-
});
423+
async stop(executionId: string, sharedWorkflowIds: string[]): Promise<StopResult> {
424+
const execution = await this.executionRepository.findWithUnflattenedData(
425+
executionId,
426+
sharedWorkflowIds,
427+
);
428428

429-
if (!execution) throw new MissingExecutionStopError(executionId);
429+
if (!execution) {
430+
this.logger.info(`Unable to stop execution "${executionId}" as it was not found`, {
431+
executionId,
432+
});
433+
434+
throw new MissingExecutionStopError(executionId);
435+
}
430436

431437
this.assertStoppable(execution);
432438

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,9 @@ export class ExecutionsController {
9696

9797
if (workflowIds.length === 0) throw new NotFoundError('Execution not found');
9898

99-
return await this.executionService.stop(req.params.id);
99+
const executionId = req.params.id;
100+
101+
return await this.executionService.stop(executionId, workflowIds);
100102
}
101103

102104
@Post('/:id/retry')

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

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@ import type { User } from '@n8n/db';
33
import { ConcurrencyControlService } from '@/concurrency/concurrency-control.service';
44
import { WaitTracker } from '@/wait-tracker';
55

6-
import { createSuccessfulExecution, getAllExecutions } from './shared/db/executions';
6+
import {
7+
createSuccessfulExecution,
8+
createWaitingExecution,
9+
getAllExecutions,
10+
} from './shared/db/executions';
711
import { createTeamProject, linkUserToProject } from './shared/db/projects';
812
import { createMember, createOwner } from './shared/db/users';
913
import { createWorkflow, shareWorkflowWithUsers } from './shared/db/workflows';
@@ -27,6 +31,11 @@ const saveExecution = async ({ belongingTo }: { belongingTo: User }) => {
2731
return await createSuccessfulExecution(workflow);
2832
};
2933

34+
const saveWaitingExecution = async ({ belongingTo }: { belongingTo: User }) => {
35+
const workflow = await createWorkflow({}, belongingTo);
36+
return await createWaitingExecution(workflow);
37+
};
38+
3039
beforeEach(async () => {
3140
await testDb.truncate(['ExecutionEntity', 'WorkflowEntity', 'SharedWorkflow']);
3241
testServer.license.reset();
@@ -117,3 +126,21 @@ describe('POST /executions/delete', () => {
117126
expect(executions).toHaveLength(0);
118127
});
119128
});
129+
130+
describe('POST /executions/stop', () => {
131+
test('should not stop an execution we do not have access to', async () => {
132+
await saveExecution({ belongingTo: owner });
133+
const incorrectExecutionId = '1234';
134+
135+
await testServer
136+
.authAgentFor(owner)
137+
.post(`/executions/${incorrectExecutionId}/stop`)
138+
.expect(500);
139+
});
140+
141+
test('should stop an execution we have access to', async () => {
142+
const execution = await saveWaitingExecution({ belongingTo: owner });
143+
144+
await testServer.authAgentFor(owner).post(`/executions/${execution.id}/stop`).expect(200);
145+
});
146+
});

0 commit comments

Comments
 (0)