From dd3c59677bbdcb49332975fd84d9e88852f76c8d Mon Sep 17 00:00:00 2001 From: Michael Auerswald Date: Wed, 26 Oct 2022 13:30:35 +0200 Subject: [PATCH] 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 | 1 + packages/cli/src/api/executions.api.ts | 200 +++++++++++++----- .../cli/src/workflows/workflows.controller.ts | 94 +++++--- 3 files changed, 210 insertions(+), 85 deletions(-) diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 60df23f96e..f5a5692d6d 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/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 { const sharedWorkflows = await Db.collections.SharedWorkflow.find({ diff --git a/packages/cli/src/api/executions.api.ts b/packages/cli/src/api/executions.api.ts index ebcac6bce9..67e8126386 100644 --- a/packages/cli/src/api/executions.api.ts +++ b/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; + finished?: boolean; + mode?: string; + retryOf?: string; + retrySuccessId?: string; + workflowId?: number | string; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + waitTill?: FindOperator | boolean; +} + /** * Initialise Logger if needed */ @@ -111,7 +144,57 @@ async function getExecutionsCount( executionsController.get( '/', ResponseHelper.send(async (req: ExecutionRequest.GetAll): Promise => { - const filter = req.query.filter ? jsonParse(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 = { - 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 => { - 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; diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index e6a2ea88ab..dff75a59f3 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/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;