fix(API): validate excecutions and workflow filter parameters (#4424)

* typeorm queries with AND for filter, schema validation
* validate filters
* replace json.parse with jsonParse()
* limited fields further
* limited workflow fields further
* removes date filter fields and fix waitTill filter
* simplified filter name arrays

packages/cli/src/WorkflowHelpers.ts

@@ -597,6 +597,7 @@ export function whereClause({
 
 /**
  * Get the IDs of the workflows that have been shared with the user.
+ * Returns all IDs if user is global owner (see `whereClause`)
  */
 export async function getSharedWorkflowIds(user: User): Promise<number[]> {
 	const sharedWorkflows = await Db.collections.SharedWorkflow.find({

packages/cli/src/api/executions.api.ts

@@ -9,10 +9,18 @@
 /* eslint-disable @typescript-eslint/no-unsafe-argument */
 /* eslint-disable @typescript-eslint/no-unused-vars */
 import express from 'express';
+import { validate as jsonSchemaValidate } from 'jsonschema';
 import _, { cloneDeep } from 'lodash';
 import { BinaryDataManager } from 'n8n-core';
-import { IDataObject, IWorkflowBase, LoggerProxy, jsonParse, Workflow } from 'n8n-workflow';
-import { FindManyOptions, In, IsNull, LessThanOrEqual, Not, Raw } from 'typeorm';
+import {
+	IDataObject,
+	IWorkflowBase,
+	JsonObject,
+	jsonParse,
+	LoggerProxy,
+	Workflow,
+} from 'n8n-workflow';
+import { FindOperator, In, IsNull, LessThanOrEqual, Not, Raw } from 'typeorm';
 
 import {
 	ActiveExecutions,
@@ -26,7 +34,6 @@ import {
 	NodeTypes,
 	WorkflowRunner,
 	ResponseHelper,
-	IExecutionFlattedDb,
 } from '..';
 import * as config from '../../config';
 import { User } from '../databases/entities/User';
@@ -38,6 +45,32 @@ import { getSharedWorkflowIds } from '../WorkflowHelpers';
 
 export const executionsController = express.Router();
 
+const schemaGetExecutionsQueryFilter = {
+	$id: '/IGetExecutionsQueryFilter',
+	type: 'object',
+	properties: {
+		finished: { type: 'boolean' },
+		mode: { type: 'string' },
+		retryOf: { type: 'string' },
+		retrySuccessId: { type: 'string' },
+		waitTill: { type: 'boolean' },
+		workflowId: { anyOf: [{ type: 'integer' }, { type: 'string' }] },
+	},
+};
+
+const allowedExecutionsQueryFilterFields = Object.keys(schemaGetExecutionsQueryFilter.properties);
+
+interface IGetExecutionsQueryFilter {
+	id?: FindOperator<string>;
+	finished?: boolean;
+	mode?: string;
+	retryOf?: string;
+	retrySuccessId?: string;
+	workflowId?: number | string;
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any
+	waitTill?: FindOperator<any> | boolean;
+}
+
 /**
  * Initialise Logger if needed
  */
@@ -111,7 +144,57 @@ async function getExecutionsCount(
 executionsController.get(
 	'/',
 	ResponseHelper.send(async (req: ExecutionRequest.GetAll): Promise<IExecutionsListResponse> => {
-		const filter = req.query.filter ? jsonParse<IDataObject>(req.query.filter) : {};
+		const sharedWorkflowIds = await getSharedWorkflowIds(req.user);
+		if (sharedWorkflowIds.length === 0) {
+			// return early since without shared workflows there can be no hits
+			// (note: getSharedWorkflowIds() returns _all_ workflow ids for global owners)
+			return {
+				count: 0,
+				estimated: false,
+				results: [],
+			};
+		}
+
+		// parse incoming filter object and remove non-valid fields
+		let filter: IGetExecutionsQueryFilter | undefined = undefined;
+		if (req.query.filter) {
+			try {
+				const filterJson: JsonObject = jsonParse(req.query.filter);
+				if (filterJson) {
+					Object.keys(filterJson).map((key) => {
+						if (!allowedExecutionsQueryFilterFields.includes(key)) delete filterJson[key];
+					});
+					if (jsonSchemaValidate(filterJson, schemaGetExecutionsQueryFilter).valid) {
+						filter = filterJson as IGetExecutionsQueryFilter;
+					}
+				}
+			} catch (error) {
+				LoggerProxy.error('Failed to parse filter', {
+					userId: req.user.id,
+					filter: req.query.filter,
+				});
+				throw new ResponseHelper.ResponseError(
+					`Parameter "filter" contained invalid JSON string.`,
+					500,
+					500,
+				);
+			}
+		}
+
+		// safeguard against querying workflowIds not shared with the user
+		if (filter?.workflowId !== undefined) {
+			const workflowId = parseInt(filter.workflowId.toString());
+			if (workflowId && !sharedWorkflowIds.includes(workflowId)) {
+				LoggerProxy.verbose(
+					`User ${req.user.id} attempted to query non-shared workflow ${workflowId}`,
+				);
+				return {
+					count: 0,
+					estimated: false,
+					results: [],
+				};
+			}
+		}
 
 		const limit = req.query.limit
 			? parseInt(req.query.limit, 10)
@@ -131,42 +214,7 @@ executionsController.get(
 				.map(({ id }) => id),
 		);
 
-		const countFilter = cloneDeep(filter);
-		countFilter.waitTill &&= Not(IsNull());
-		countFilter.id = Not(In(executingWorkflowIds));
-
-		const sharedWorkflowIds = await getSharedWorkflowIds(req.user);
-
-		const findOptions: FindManyOptions<IExecutionFlattedDb> = {
-			select: [
-				'id',
-				'finished',
-				'mode',
-				'retryOf',
-				'retrySuccessId',
-				'waitTill',
-				'startedAt',
-				'stoppedAt',
-				'workflowData',
-			],
-			where: { workflowId: In(sharedWorkflowIds) },
-			order: { id: 'DESC' },
-			take: limit,
-		};
-
-		Object.entries(filter).forEach(([key, value]) => {
-			let filterToAdd = {};
-
-			if (key === 'waitTill') {
-				filterToAdd = { waitTill: Not(IsNull()) };
-			} else if (key === 'finished' && value === false) {
-				filterToAdd = { finished: false, waitTill: IsNull() };
-			} else {
-				filterToAdd = { [key]: value };
-			}
-
-			Object.assign(findOptions.where!, filterToAdd);
-		});
+		const findWhere = { workflowId: In(sharedWorkflowIds) };
 
 		const rangeQuery: string[] = [];
 		const rangeQueryParams: {
@@ -191,14 +239,35 @@ executionsController.get(
 		}
 
 		if (rangeQuery.length) {
-			Object.assign(findOptions.where!, {
+			Object.assign(findWhere, {
 				id: Raw(() => rangeQuery.join(' and '), rangeQueryParams),
 			});
 		}
 
-		const executions = await Db.collections.Execution.find(findOptions);
+		let query = Db.collections.Execution.createQueryBuilder()
+			.select()
+			.orderBy('id', 'DESC')
+			.take(limit)
+			.where(findWhere);
 
-		const { count, estimated } = await getExecutionsCount(countFilter, req.user);
+		if (filter) {
+			if (filter.waitTill === true) {
+				filter.waitTill = Not(IsNull());
+				// eslint-disable-next-line @typescript-eslint/no-unnecessary-boolean-literal-compare
+			} else if (filter.finished === false) {
+				filter.waitTill = IsNull();
+			} else {
+				delete filter.waitTill;
+			}
+			query = query.andWhere(filter);
+		}
+
+		const countFilter = cloneDeep(filter ?? {});
+		countFilter.id = Not(In(executingWorkflowIds));
+
+		const executions = await query.getMany();
+
+		const { count, estimated } = await getExecutionsCount(countFilter as IDataObject, req.user);
 
 		const formattedExecutions = executions.map((execution) => {
 			return {
@@ -211,7 +280,7 @@ executionsController.get(
 				startedAt: execution.startedAt,
 				stoppedAt: execution.stoppedAt,
 				workflowId: execution.workflowData?.id?.toString() ?? '',
-				workflowName: execution.workflowData.name,
+				workflowName: execution.workflowData?.name,
 			};
 		});
 
@@ -406,13 +475,36 @@ executionsController.post(
 executionsController.post(
 	'/delete',
 	ResponseHelper.send(async (req: ExecutionRequest.Delete): Promise<void> => {
-		const { deleteBefore, ids, filters: requestFilters } = req.body;
+		const { deleteBefore, ids, filters: requestFiltersRaw } = req.body;
+		let requestFilters;
+		if (requestFiltersRaw) {
+			try {
+				Object.keys(requestFiltersRaw).map((key) => {
+					if (!allowedExecutionsQueryFilterFields.includes(key)) delete requestFiltersRaw[key];
+				});
+				if (jsonSchemaValidate(requestFiltersRaw, schemaGetExecutionsQueryFilter).valid) {
+					requestFilters = requestFiltersRaw as IGetExecutionsQueryFilter;
+				}
+			} catch (error) {
+				throw new ResponseHelper.ResponseError(
+					`Parameter "filter" contained invalid JSON string.`,
+					500,
+					500,
+				);
+			}
+		}
 
 		if (!deleteBefore && !ids) {
 			throw new Error('Either "deleteBefore" or "ids" must be present in the request body');
 		}
 
 		const sharedWorkflowIds = await getSharedWorkflowIds(req.user);
+		if (sharedWorkflowIds.length === 0) {
+			// return early since without shared workflows there can be no hits
+			// (note: getSharedWorkflowIds() returns _all_ workflow ids for global owners)
+			return;
+		}
+
 		const binaryDataManager = BinaryDataManager.getInstance();
 
 		// delete executions by date, if user may access the underlying workflows
@@ -422,16 +514,18 @@ executionsController.post(
 				startedAt: LessThanOrEqual(deleteBefore),
 			};
 
-			if (filters) {
-				Object.assign(filters, requestFilters);
+			let query = Db.collections.Execution.createQueryBuilder()
+				.select()
+				.where({
+					...filters,
+					workflowId: In(sharedWorkflowIds),
+				});
+
+			if (requestFilters) {
+				query = query.andWhere(requestFilters);
 			}
 
-			const executions = await Db.collections.Execution.find({
-				where: {
-					workflowId: In(sharedWorkflowIds),
-					...filters,
-				},
-			});
+			const executions = await query.getMany();
 
 			if (!executions.length) return;
 

packages/cli/src/workflows/workflows.controller.ts

@@ -2,7 +2,7 @@
 /* eslint-disable import/no-cycle */
 
 import express from 'express';
-import { IDataObject, INode, IPinData, LoggerProxy, Workflow } from 'n8n-workflow';
+import { INode, IPinData, JsonObject, jsonParse, LoggerProxy, Workflow } from 'n8n-workflow';
 
 import axios from 'axios';
 import { FindManyOptions, In } from 'typeorm';
@@ -31,12 +31,31 @@ import { InternalHooksManager } from '../InternalHooksManager';
 import { externalHooks } from '../Server';
 import { getLogger } from '../Logger';
 import type { WorkflowRequest } from '../requests';
-import { isBelowOnboardingThreshold } from '../WorkflowHelpers';
+import { getSharedWorkflowIds, isBelowOnboardingThreshold } from '../WorkflowHelpers';
 import { EEWorkflowController } from './workflows.controller.ee';
+import { validate as jsonSchemaValidate } from 'jsonschema';
 
 const activeWorkflowRunner = ActiveWorkflowRunner.getInstance();
 export const workflowsController = express.Router();
 
+const schemaGetWorkflowsQueryFilter = {
+	$id: '/IGetWorkflowsQueryFilter',
+	type: 'object',
+	properties: {
+		id: { anyOf: [{ type: 'integer' }, { type: 'string' }] },
+		name: { type: 'string' },
+		active: { type: 'boolean' },
+	},
+};
+
+const allowedWorkflowsQueryFilterFields = Object.keys(schemaGetWorkflowsQueryFilter.properties);
+
+interface IGetWorkflowsQueryFilter {
+	id?: number | string;
+	name?: string;
+	active?: boolean;
+}
+
 /**
  * Initialize Logger if needed
  */
@@ -142,18 +161,47 @@ workflowsController.post(
 workflowsController.get(
 	`/`,
 	ResponseHelper.send(async (req: WorkflowRequest.GetAll) => {
-		let workflows: WorkflowEntity[] = [];
+		const sharedWorkflowIds = await getSharedWorkflowIds(req.user);
+		if (sharedWorkflowIds.length === 0) {
+			// return early since without shared workflows there can be no hits
+			// (note: getSharedWorkflowIds() returns _all_ workflow ids for global owners)
+			return [];
+		}
 
-		let filter: IDataObject = {};
+		// parse incoming filter object and remove non-valid fields
+		let filter: IGetWorkflowsQueryFilter | undefined = undefined;
 		if (req.query.filter) {
 			try {
-				filter = (JSON.parse(req.query.filter) as IDataObject) || {};
+				const filterJson: JsonObject = jsonParse(req.query.filter);
+				if (filterJson) {
+					Object.keys(filterJson).map((key) => {
+						if (!allowedWorkflowsQueryFilterFields.includes(key)) delete filterJson[key];
+					});
+					if (jsonSchemaValidate(filterJson, schemaGetWorkflowsQueryFilter).valid) {
+						filter = filterJson as IGetWorkflowsQueryFilter;
+					}
+				}
 			} catch (error) {
 				LoggerProxy.error('Failed to parse filter', {
 					userId: req.user.id,
 					filter: req.query.filter,
 				});
-				throw new ResponseHelper.ResponseError('Failed to parse filter');
+				throw new ResponseHelper.ResponseError(
+					`Parameter "filter" contained invalid JSON string.`,
+					500,
+					500,
+				);
+			}
+		}
+
+		// safeguard against querying ids not shared with the user
+		if (filter?.id !== undefined) {
+			const workflowId = parseInt(filter.id.toString());
+			if (workflowId && !sharedWorkflowIds.includes(workflowId)) {
+				LoggerProxy.verbose(
+					`User ${req.user.id} attempted to query non-shared workflow ${workflowId}`,
+				);
+				return [];
 			}
 		}
 
@@ -166,32 +214,14 @@ workflowsController.get(
 			delete query.relations;
 		}
 
-		if (req.user.globalRole.name === 'owner') {
-			workflows = await Db.collections.Workflow.find(
-				Object.assign(query, {
-					where: filter,
-				}),
-			);
-		} else {
-			const shared = await Db.collections.SharedWorkflow.find({
-				relations: ['workflow'],
-				where: whereClause({
-					user: req.user,
-					entityType: 'workflow',
-				}),
-			});
-
-			if (!shared.length) return [];
-
-			workflows = await Db.collections.Workflow.find(
-				Object.assign(query, {
-					where: {
-						id: In(shared.map(({ workflow }) => workflow.id)),
-						...filter,
-					},
-				}),
-			);
-		}
+		const workflows = await Db.collections.Workflow.find(
+			Object.assign(query, {
+				where: {
+					id: In(sharedWorkflowIds),
+					...filter,
+				},
+			}),
+		);
 
 		return workflows.map((workflow) => {
 			const { id, ...rest } = workflow;