feat(core): Add credential runtime checks and prevent tampering in manual run

packages/cli/src/UserManagement/PermissionChecker.ts

@@ -0,0 +1,75 @@
+import { INode, NodeOperationError, Workflow } from 'n8n-workflow';
+import { In } from 'typeorm';
+import * as Db from '@/Db';
+
+export class PermissionChecker {
+	/**
+	 * Check if a user is permitted to execute a workflow.
+	 */
+	static async check(workflow: Workflow, userId: string) {
+		// allow if no nodes in this workflow use creds
+
+		const credIdsToNodes = PermissionChecker.mapCredIdsToNodes(workflow);
+
+		const workflowCredIds = Object.keys(credIdsToNodes);
+
+		if (workflowCredIds.length === 0) return;
+
+		// allow if requesting user is instance owner
+
+		const user = await Db.collections.User.findOneOrFail(userId, {
+			relations: ['globalRole'],
+		});
+
+		if (user.globalRole.name === 'owner') return;
+
+		// allow if all creds used in this workflow are a subset of
+		// all creds accessible to users who have access to this workflow
+
+		const workflowSharings = await Db.collections.SharedWorkflow.find({
+			relations: ['workflow'],
+			where: { workflow: { id: Number(workflow.id) } },
+		});
+
+		const workflowUserIds = workflowSharings.map((s) => s.userId);
+
+		const credentialSharings = await Db.collections.SharedCredentials.find({
+			where: { user: In(workflowUserIds) },
+		});
+
+		const accessibleCredIds = credentialSharings.map((s) => s.credentialId.toString());
+
+		const inaccessibleCredIds = workflowCredIds.filter((id) => !accessibleCredIds.includes(id));
+
+		if (inaccessibleCredIds.length === 0) return;
+
+		// if disallowed, flag only first node using first inaccessible cred
+
+		const nodeToFlag = credIdsToNodes[inaccessibleCredIds[0]][0];
+
+		throw new NodeOperationError(nodeToFlag, 'Node has no access to credential', {
+			description: 'Please recreate the credential or ask its owner to share it with you.',
+		});
+	}
+
+	private static mapCredIdsToNodes(workflow: Workflow) {
+		return Object.values(workflow.nodes).reduce<{ [credentialId: string]: INode[] }>(
+			(map, node) => {
+				if (node.disabled || !node.credentials) return map;
+
+				Object.values(node.credentials).forEach((cred) => {
+					if (!cred.id) {
+						throw new NodeOperationError(node, 'Node uses invalid credential', {
+							description: 'Please recreate the credential.',
+						});
+					}
+
+					map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node];
+				});
+
+				return map;
+			},
+			{},
+		);
+	}
+}

packages/cli/src/UserManagement/UserManagementHelper.ts

@@ -145,97 +145,6 @@ export async function getUserById(userId: string): Promise<User> {
 	return user;
 }
 
-export async function checkPermissionsForExecution(
-	workflow: Workflow,
-	userId: string,
-): Promise<boolean> {
-	const credentialIds = new Set();
-	const nodeNames = Object.keys(workflow.nodes);
-	const credentialUsedBy = new Map();
-	// Iterate over all nodes
-	nodeNames.forEach((nodeName) => {
-		const node = workflow.nodes[nodeName];
-		if (node.disabled === true) {
-			// If a node is disabled there is no need to check its credentials
-			return;
-		}
-		// And check if any of the nodes uses credentials.
-		if (node.credentials) {
-			const credentialNames = Object.keys(node.credentials);
-			// For every credential this node uses
-			credentialNames.forEach((credentialName) => {
-				const credentialDetail = node.credentials![credentialName];
-				// If it does not contain an id, it means it is a very old
-				// workflow. Nowadays it should not happen anymore.
-				// Migrations should handle the case where a credential does
-				// not have an id.
-				if (credentialDetail.id === null) {
-					throw new NodeOperationError(
-						node,
-						`The credential on node '${node.name}' is not valid. Please open the workflow and set it to a valid value.`,
-					);
-				}
-				if (!credentialDetail.id) {
-					throw new NodeOperationError(
-						node,
-						`Error initializing workflow: credential ID not present. Please open the workflow and save it to fix this error. [Node: '${node.name}']`,
-					);
-				}
-				credentialIds.add(credentialDetail.id.toString());
-				if (!credentialUsedBy.has(credentialDetail.id)) {
-					credentialUsedBy.set(credentialDetail.id, node);
-				}
-			});
-		}
-	});
-
-	// Now that we obtained all credential IDs used by this workflow, we can
-	// now check if the owner of this workflow has access to all of them.
-
-	const ids = Array.from(credentialIds);
-
-	if (ids.length === 0) {
-		// If the workflow does not use any credentials, then we're fine
-		return true;
-	}
-	// If this check happens on top, we may get
-	// uninitialized db errors.
-	// Db is certainly initialized if workflow uses credentials.
-	const user = await getUserById(userId);
-	if (user.globalRole.name === 'owner') {
-		return true;
-	}
-
-	// Check for the user's permission to all used credentials
-	const credentialsWithAccess = await Db.collections.SharedCredentials.find({
-		where: {
-			user: { id: userId },
-			credentials: In(ids),
-		},
-	});
-
-	// Considering the user needs to have access to all credentials
-	// then both arrays (allowed credentials vs used credentials)
-	// must be the same length
-	if (ids.length !== credentialsWithAccess.length) {
-		credentialsWithAccess.forEach((credential) => {
-			credentialUsedBy.delete(credential.credentialId.toString());
-		});
-
-		// Find the first missing node from the Set - this is arbitrarily fetched
-		const firstMissingCredentialNode = credentialUsedBy.values().next().value as INode;
-		throw new NodeOperationError(
-			firstMissingCredentialNode,
-			'This node does not have access to the required credential',
-			{
-				description:
-					'Maybe the credential was removed or you have lost access to it. Try contacting the owner if this credential does not belong to you',
-			},
-		);
-	}
-	return true;
-}
-
 /**
  * Check if a URL contains an auth-excluded endpoint.
  */

packages/cli/src/WorkflowExecuteAdditionalData.ts

@@ -62,12 +62,9 @@ import * as Push from '@/Push';
 import * as ResponseHelper from '@/ResponseHelper';
 import * as WebhookHelpers from '@/WebhookHelpers';
 import * as WorkflowHelpers from '@/WorkflowHelpers';
-import {
-	checkPermissionsForExecution,
-	getUserById,
-	getWorkflowOwner,
-} from '@/UserManagement/UserManagementHelper';
+import { getUserById, getWorkflowOwner } from '@/UserManagement/UserManagementHelper';
 import { findSubworkflowStart } from '@/utils';
+import { PermissionChecker } from './UserManagement/PermissionChecker';
 
 const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');
 
@@ -942,7 +939,7 @@ export async function executeWorkflow(
 
 	let data;
 	try {
-		await checkPermissionsForExecution(workflow, additionalData.userId);
+		await PermissionChecker.check(workflow, additionalData.userId);
 
 		// Create new additionalData to have different workflow loaded and to call
 		// different webhooks

packages/cli/src/WorkflowRunner.ts

@@ -53,9 +53,9 @@ import * as WebhookHelpers from '@/WebhookHelpers';
 import * as WorkflowHelpers from '@/WorkflowHelpers';
 import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData';
 import { InternalHooksManager } from '@/InternalHooksManager';
-import { checkPermissionsForExecution } from '@/UserManagement/UserManagementHelper';
 import { generateFailedExecutionFromError } from '@/WorkflowHelpers';
 import { initErrorHandling } from '@/ErrorReporting';
+import { PermissionChecker } from '@/UserManagement/PermissionChecker';
 
 export class WorkflowRunner {
 	activeExecutions: ActiveExecutions.ActiveExecutions;
@@ -267,7 +267,7 @@ export class WorkflowRunner {
 			);
 
 			try {
-				await checkPermissionsForExecution(workflow, data.userId);
+				await PermissionChecker.check(workflow, data.userId);
 			} catch (error) {
 				ErrorReporter.error(error);
 				// Create a failed execution with the data for the node

packages/cli/src/WorkflowRunnerProcess.ts

@@ -46,10 +46,10 @@ import { getLogger } from '@/Logger';
 
 import config from '@/config';
 import { InternalHooksManager } from '@/InternalHooksManager';
-import { checkPermissionsForExecution } from '@/UserManagement/UserManagementHelper';
 import { loadClassInIsolation } from '@/CommunityNodes/helpers';
 import { generateFailedExecutionFromError } from '@/WorkflowHelpers';
 import { initErrorHandling } from '@/ErrorReporting';
+import { PermissionChecker } from '@/UserManagement/PermissionChecker';
 
 export class WorkflowRunnerProcess {
 	data: IWorkflowExecutionDataProcessWithExecution | undefined;
@@ -225,7 +225,7 @@ export class WorkflowRunnerProcess {
 			pinData: this.data.pinData,
 		});
 		try {
-			await checkPermissionsForExecution(this.workflow, userId);
+			await PermissionChecker.check(this.workflow, userId);
 		} catch (error) {
 			const caughtError = error as NodeOperationError;
 			const failedExecutionData = generateFailedExecutionFromError(

packages/cli/src/commands/worker.ts

@@ -38,13 +38,11 @@ import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData'
 import { InternalHooksManager } from '@/InternalHooksManager';
 import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
 import { getLogger } from '@/Logger';
+import { PermissionChecker } from '@/UserManagement/PermissionChecker';
 
 import config from '@/config';
 import * as Queue from '@/Queue';
-import {
-	checkPermissionsForExecution,
-	getWorkflowOwner,
-} from '@/UserManagement/UserManagementHelper';
+import { getWorkflowOwner } from '@/UserManagement/UserManagementHelper';
 import { generateFailedExecutionFromError } from '@/WorkflowHelpers';
 
 export class Worker extends Command {
@@ -199,7 +197,7 @@ export class Worker extends Command {
 		);
 
 		try {
-			await checkPermissionsForExecution(workflow, workflowOwner.id);
+			await PermissionChecker.check(workflow, workflowOwner.id);
 		} catch (error) {
 			const failedExecution = generateFailedExecutionFromError(
 				currentExecutionDb.mode,

packages/cli/src/requests.d.ts

@@ -39,7 +39,7 @@ export type AuthenticatedRequest<
 // ----------------------------------
 
 export declare namespace WorkflowRequest {
-	type RequestBody = Partial<{
+	type CreateUpdatePayload = Partial<{
 		id: string; // delete if sent
 		name: string;
 		nodes: INode[];
@@ -50,13 +50,26 @@ export declare namespace WorkflowRequest {
 		hash: string;
 	}>;
 
-	type Create = AuthenticatedRequest<{}, {}, RequestBody>;
+	type ManualRunPayload = {
+		workflowData: IWorkflowDb;
+		runData: IRunData;
+		pinData: IPinData;
+		startNodes?: string[];
+		destinationNode?: string;
+	};
+
+	type Create = AuthenticatedRequest<{}, {}, CreateUpdatePayload>;
 
 	type Get = AuthenticatedRequest<{ id: string }>;
 
 	type Delete = Get;
 
-	type Update = AuthenticatedRequest<{ id: string }, {}, RequestBody, { forceSave?: string }>;
+	type Update = AuthenticatedRequest<
+		{ id: string },
+		{},
+		CreateUpdatePayload,
+		{ forceSave?: string }
+	>;
 
 	type NewName = AuthenticatedRequest<{}, {}, {}, { name?: string }>;
 
@@ -66,17 +79,7 @@ export declare namespace WorkflowRequest {
 
 	type GetAllActivationErrors = Get;
 
-	type ManualRun = AuthenticatedRequest<
-		{},
-		{},
-		{
-			workflowData: IWorkflowDb;
-			runData: IRunData;
-			pinData: IPinData;
-			startNodes?: string[];
-			destinationNode?: string;
-		}
-	>;
+	type ManualRun = AuthenticatedRequest<{}, {}, ManualRunPayload>;
 
 	type Share = AuthenticatedRequest<{ workflowId: string }, {}, { shareWithIds: string[] }>;
 }

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

@@ -15,6 +15,8 @@ import { LoggerProxy } from 'n8n-workflow';
 import * as TagHelpers from '@/TagHelpers';
 import { EECredentialsService as EECredentials } from '../credentials/credentials.service.ee';
 import { WorkflowsService } from './workflows.services';
+import { IExecutionPushResponse } from '@/Interfaces';
+import * as GenericHelpers from '@/GenericHelpers';
 
 // eslint-disable-next-line @typescript-eslint/naming-convention
 export const EEWorkflowController = express.Router();
@@ -214,9 +216,11 @@ EEWorkflowController.patch(
 		const { tags, ...rest } = req.body;
 		Object.assign(updateData, rest);
 
-		const updatedWorkflow = await EEWorkflows.updateWorkflow(
+		const safeWorkflow = await EEWorkflows.preventTampering(updateData, workflowId, req.user);
+
+		const updatedWorkflow = await WorkflowsService.update(
 			req.user,
-			updateData,
+			safeWorkflow,
 			workflowId,
 			tags,
 			forceSave,
@@ -230,3 +234,24 @@ EEWorkflowController.patch(
 		};
 	}),
 );
+
+/**
+ * (EE) POST /workflows/run
+ */
+EEWorkflowController.post(
+	'/run',
+	ResponseHelper.send(async (req: WorkflowRequest.ManualRun): Promise<IExecutionPushResponse> => {
+		const workflow = new WorkflowEntity();
+		Object.assign(workflow, req.body.workflowData);
+
+		const safeWorkflow = await EEWorkflows.preventTampering(
+			workflow,
+			workflow.id.toString(),
+			req.user,
+		);
+
+		req.body.workflowData.nodes = safeWorkflow.nodes;
+
+		return WorkflowsService.runManually(req.body, req.user, GenericHelpers.getSessionId(req));
+	}),
+);