fix(core): Allow switching a node back to a credential the user cannot use after a save (#41198)

packages/@n8n/db/src/repositories/workflow-history.repository.ts

@@ -63,6 +63,22 @@ export class WorkflowHistoryRepository extends BaseRepository<WorkflowHistory> {
 		return await this.delete({ createdAt: LessThan(date) });
 	}
 
+	/**
+	 * The newest `limit` versions of a workflow, with only their nodes, for a
+	 * caller that looks for a node as a recent version stored it.
+	 */
+	async findNewestVersionNodes(
+		workflowId: string,
+		limit: number,
+	): Promise<Array<Pick<WorkflowHistory, 'versionId' | 'nodes'>>> {
+		return await this.find({
+			select: ['versionId', 'nodes'],
+			where: { workflowId },
+			order: { createdAt: 'DESC', versionId: 'DESC' },
+			take: limit,
+		});
+	}
+
 	async findVersionSummaries(
 		workflowId: string,
 		versionIds: string[],

packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts

@@ -6,6 +6,7 @@ import type {
 	SharedWorkflow,
 	User,
 	WorkflowEntity,
+	WorkflowHistoryRepository,
 	WorkflowPublishHistoryRepository,
 	WorkflowRepository,
 } from '@n8n/db';
@@ -47,6 +48,7 @@ describe('EnterpriseWorkflowService', () => {
 	const credentialsFinderService = mock<CredentialsFinderService>();
 	const nodeTypes = mock<NodeTypes>();
 	const credentialsPermissionChecker = mock<CredentialsPermissionChecker>();
+	const workflowHistoryRepository = mock<WorkflowHistoryRepository>();
 
 	beforeEach(() => {
 		vi.clearAllMocks();
@@ -70,6 +72,7 @@ describe('EnterpriseWorkflowService', () => {
 			policyEnforcementService,
 			nodeTypes,
 			credentialsPermissionChecker,
+			workflowHistoryRepository,
 		);
 	});
 
@@ -719,13 +722,28 @@ describe('EnterpriseWorkflowService', () => {
 			expect(result.nodes[0]).toEqual(switched);
 		});
 
-		describe('restoring a history version', () => {
+		describe('a node as a history version stored it', () => {
 			const historyNode = () =>
 				httpNode(
 					{ httpHeaderAuth: { id: 'foreign-cred', name: 'Theirs' } },
 					{ url: 'https://v1.test' },
 				);
 
+			it('keeps a node renamed, moved and disabled since the version stored it', () => {
+				const previousVersion = { nodes: [] } as unknown as IWorkflowBase;
+				const edited = { ...historyNode(), name: 'Renamed', position: [10, 20], disabled: true };
+				const newVersion = { nodes: [edited] } as unknown as IWorkflowBase;
+
+				const result = service.validateWorkflowCredentialUsage(
+					newVersion,
+					previousVersion,
+					accessible,
+					[historyNode()],
+				);
+
+				expect(result.nodes[0]).toEqual(edited);
+			});
+
 			it('keeps a node exactly as the restored version stored it', () => {
 				// The current version dropped the node; the user cannot use its credential.
 				const previousVersion = { nodes: [] } as unknown as IWorkflowBase;
@@ -936,6 +954,160 @@ describe('EnterpriseWorkflowService', () => {
 		});
 	});
 
+	describe('preventTampering() - a credential switched back from the workflow history', () => {
+		const user = mock<User>({ id: 'bob', role: { scopes: [] } });
+		const httpNode = (credentialId: string, url = 'https://v1.test') =>
+			({
+				id: 'node-1',
+				name: 'Call',
+				type: 'n8n-nodes-base.httpRequest',
+				typeVersion: 4.2,
+				position: [0, 0],
+				parameters: { url },
+				credentials: { httpHeaderAuth: { id: credentialId, name: credentialId } },
+			}) as unknown as INode;
+		const version = (versionId: string, nodes: INode[]) => ({ versionId, nodes });
+
+		beforeEach(() => {
+			workflowRepository.get.mockResolvedValue({
+				nodes: [httpNode('own-cred')],
+			} as unknown as WorkflowEntity);
+			credentialsService.getCredentialsAUserCanUseInAWorkflow.mockResolvedValue([
+				mock({ id: 'own-cred' }),
+			]);
+		});
+
+		it('saves a node switched back to a credential an earlier version stored, with its position', async () => {
+			workflowHistoryRepository.findNewestVersionNodes.mockResolvedValue([
+				version('v3', [httpNode('own-cred')]),
+				version('v2', [httpNode('foreign-cred')]),
+			]);
+			const submitted = { ...httpNode('foreign-cred'), position: [10, 20] };
+
+			const result = await service.preventTampering(
+				{ nodes: [submitted] } as IWorkflowBase,
+				'w1',
+				user,
+			);
+
+			expect(result.nodes[0]).toEqual(submitted);
+			expect(workflowHistoryRepository.findNewestVersionNodes).toHaveBeenCalledWith('w1', 20);
+		});
+
+		it('matches a stored node after its credential was renamed, also when disabled', async () => {
+			workflowHistoryRepository.findNewestVersionNodes.mockResolvedValue([
+				version('v2', [httpNode('foreign-cred')]),
+			]);
+			const submitted = { ...httpNode('foreign-cred'), disabled: true };
+			submitted.credentials!.httpHeaderAuth.name = 'Renamed';
+
+			const result = await service.preventTampering(
+				{ nodes: [submitted] } as IWorkflowBase,
+				'w1',
+				user,
+			);
+
+			expect(result.nodes[0]).toEqual(submitted);
+		});
+
+		it('rejects a node that no version stored with that credential', async () => {
+			workflowHistoryRepository.findNewestVersionNodes.mockResolvedValue([
+				version('v3', [httpNode('own-cred')]),
+			]);
+
+			await expect(
+				service.preventTampering(
+					{ nodes: [httpNode('foreign-cred')] } as IWorkflowBase,
+					'w1',
+					user,
+				),
+			).rejects.toThrow(/credentials in the 'Call' node/);
+		});
+
+		it('reads only the newest versions, so an older match does not count', async () => {
+			// The repository returns what it was asked for: the limit is the lookback.
+			workflowHistoryRepository.findNewestVersionNodes.mockImplementation(
+				async (_workflowId, limit) =>
+					Array.from({ length: limit }, (_, i) => version(`v${limit - i}`, [httpNode('own-cred')])),
+			);
+
+			await expect(
+				service.preventTampering(
+					{ nodes: [httpNode('foreign-cred')] } as IWorkflowBase,
+					'w1',
+					user,
+				),
+			).rejects.toThrow(/credentials in the 'Call' node/);
+			expect(workflowHistoryRepository.findNewestVersionNodes).toHaveBeenCalledTimes(1);
+			expect(workflowHistoryRepository.findNewestVersionNodes).toHaveBeenCalledWith('w1', 20);
+		});
+
+		it('rejects a node whose other parameters changed since the version stored it', async () => {
+			workflowHistoryRepository.findNewestVersionNodes.mockResolvedValue([
+				version('v2', [httpNode('foreign-cred')]),
+			]);
+
+			await expect(
+				service.preventTampering(
+					{ nodes: [httpNode('foreign-cred', 'https://changed.test')] } as IWorkflowBase,
+					'w1',
+					user,
+				),
+			).rejects.toThrow(/credentials in the 'Call' node/);
+		});
+
+		it('reads no history when the user can use every credential in the save', async () => {
+			const result = await service.preventTampering(
+				{ nodes: [httpNode('own-cred', 'https://changed.test')] } as IWorkflowBase,
+				'w1',
+				user,
+			);
+
+			expect(result.nodes[0].parameters).toEqual({ url: 'https://changed.test' });
+			expect(workflowHistoryRepository.findNewestVersionNodes).not.toHaveBeenCalled();
+		});
+
+		it('reads no history for a node without an id', async () => {
+			const { id: _, ...withoutId } = httpNode('foreign-cred');
+
+			await expect(
+				service.preventTampering({ nodes: [withoutId] } as IWorkflowBase, 'w1', user),
+			).rejects.toThrow(/credentials in the 'Call' node/);
+			expect(workflowHistoryRepository.findNewestVersionNodes).not.toHaveBeenCalled();
+		});
+
+		it('reads no history for a node blocked only by a sub-workflow source it cannot inspect', async () => {
+			const urlSourcedNode = {
+				id: 'sub-1',
+				name: 'Sub',
+				type: 'n8n-nodes-base.executeWorkflow',
+				typeVersion: 1.2,
+				position: [0, 0],
+				parameters: { source: 'url', workflowUrl: 'https://example.com/workflow.json' },
+			} as unknown as INode;
+
+			await expect(
+				service.preventTampering({ nodes: [urlSourcedNode] } as IWorkflowBase, 'w1', user),
+			).rejects.toThrow();
+			expect(workflowHistoryRepository.findNewestVersionNodes).not.toHaveBeenCalled();
+		});
+
+		it('reads no history for a node that is read only already', async () => {
+			workflowRepository.get.mockResolvedValue({
+				nodes: [httpNode('foreign-cred')],
+			} as unknown as WorkflowEntity);
+
+			const result = await service.preventTampering(
+				{ nodes: [httpNode('foreign-cred', 'https://changed.test')] } as IWorkflowBase,
+				'w1',
+				user,
+			);
+
+			expect(result.nodes[0]).toEqual(httpNode('foreign-cred'));
+			expect(workflowHistoryRepository.findNewestVersionNodes).not.toHaveBeenCalled();
+		});
+	});
+
 	describe('attemptWorkflowReactivation', () => {
 		// Workflow and folder transfers deactivate, transfer, then re-add. A failed
 		// re-add may have partially registered triggers, in memory and as durable

packages/cli/src/workflows/workflow.service.ee.ts

@@ -14,6 +14,7 @@ import {
 	FolderRepository,
 	SharedWorkflowRepository,
 	WorkflowRepository,
+	WorkflowHistoryRepository,
 	WorkflowPublishHistoryRepository,
 } from '@n8n/db';
 import { Service } from '@n8n/di';
@@ -81,6 +82,10 @@ export type CredentialGuardOptions = {
 	allowUninspectableSource?: boolean;
 };
 
+const EDITABLE_NODE_FIELDS = ['name', 'position', 'disabled'] as const;
+
+const HISTORY_LOOKBACK_LIMIT = 20;
+
 @Service()
 export class EnterpriseWorkflowService {
 	constructor(
@@ -101,6 +106,7 @@ export class EnterpriseWorkflowService {
 		private readonly policyEnforcementService: PolicyEnforcementService,
 		private readonly nodeTypes: NodeTypes,
 		private readonly credentialsPermissionChecker: CredentialsPermissionChecker,
+		private readonly workflowHistoryRepository: WorkflowHistoryRepository,
 	) {}
 
 	async shareWithProjects(
@@ -248,7 +254,7 @@ export class EnterpriseWorkflowService {
 		workflow: T,
 		workflowId: string,
 		user: User,
-		restoredNodes?: INode[],
+		restoredNodes: INode[] = [],
 	) {
 		const previousVersion = await this.workflowRepository.get({ id: workflowId });
 
@@ -261,13 +267,29 @@ export class EnterpriseWorkflowService {
 			{ workflowId },
 		);
 
+		const options = { allowUninspectableSource: this.canUseEveryCredential(user) };
+
+		// A node switched back to a credential the user cannot use is saved as the
+		// version that last stored it did, the same as a restore of that version.
+		const historyNodes = [
+			...restoredNodes,
+			...(await this.findNodesAsHistoryStoredThem(
+				workflow,
+				previousVersion,
+				allCredentials,
+				restoredNodes,
+				options,
+				workflowId,
+			)),
+		];
+
 		try {
 			return this.validateWorkflowCredentialUsage(
 				workflow,
 				previousVersion,
 				allCredentials,
-				restoredNodes,
-				{ allowUninspectableSource: this.canUseEveryCredential(user) },
+				historyNodes,
+				options,
 			);
 		} catch (error) {
 			if (error instanceof NodeOperationError) {
@@ -279,11 +301,70 @@ export class EnterpriseWorkflowService {
 		}
 	}
 
+	private async findNodesAsHistoryStoredThem(
+		workflow: IWorkflowBase,
+		previousVersion: IWorkflowBase,
+		credentialsUserHasAccessTo: Array<{ id: string }>,
+		restoredNodes: INode[],
+		options: CredentialGuardOptions,
+		workflowId: string,
+	): Promise<INode[]> {
+		const allowedCredentialIds = credentialsUserHasAccessTo.map((cred) => cred.id);
+		// Read-only and restored nodes are settled by the other rules; a node without
+		// an id is new. None of them needs the history.
+		const settledNodeIds = new Set([
+			...this.getNodesWithInaccessibleCreds(previousVersion, allowedCredentialIds, options).map(
+				(node) => node.id,
+			),
+			...restoredNodes.map((node) => node.id),
+		]);
+		const allowed = new Set(allowedCredentialIds);
+		const candidates = this.getNodesWithInaccessibleCreds(
+			workflow,
+			allowedCredentialIds,
+			options,
+		).filter((node) => {
+			if (!node.id || settledNodeIds.has(node.id)) return false;
+			const { ids, hasUnresolved } = this.getNodeCredentialRefs(node);
+			return hasUnresolved || ids.some((id) => !allowed.has(id));
+		});
+		if (candidates.length === 0) return [];
+
+		const versions = await this.workflowHistoryRepository.findNewestVersionNodes(
+			workflowId,
+			HISTORY_LOOKBACK_LIMIT,
+		);
+		const storedNodes = versions.flatMap((version) => version.nodes);
+		return candidates.flatMap((candidate) => {
+			const stored = storedNodes.find(
+				(node) =>
+					node.id === candidate.id && this.isSameNodeApartFromEditableFields(node, candidate),
+			);
+			return stored ? [stored] : [];
+		});
+	}
+
+	private isSameNodeApartFromEditableFields(a: INode, b: INode): boolean {
+		return isEqual(this.toComparableNode(a), this.toComparableNode(b));
+	}
+
+	private toComparableNode(node: INode) {
+		const { credentials, ...rest } = omit(node, EDITABLE_NODE_FIELDS);
+		if (!credentials) return rest;
+		const credentialIdentities = Object.fromEntries(
+			Object.entries(credentials).map(([type, credential]) => [
+				type,
+				credential?.id ?? credential?.name ?? null,
+			]),
+		);
+		return { ...rest, credentials: credentialIdentities };
+	}
+
 	validateWorkflowCredentialUsage<T extends IWorkflowBase>(
 		newWorkflowVersion: T,
 		previousWorkflowVersion: IWorkflowBase,
 		credentialsUserHasAccessTo: Array<{ id: string }>,
-		restoredNodes: INode[] = [],
+		historyNodes: INode[] = [],
 		options: CredentialGuardOptions = {},
 	) {
 		const allowedCredentialIds = credentialsUserHasAccessTo.map((cred) => cred.id);
@@ -320,9 +401,9 @@ export class EnterpriseWorkflowService {
 			submittedIdCounts.set(node.id, (submittedIdCounts.get(node.id) ?? 0) + 1);
 		}
 
-		const restoredNodesById = new Map<string, INode>();
-		for (const node of restoredNodes) {
-			if (!restoredNodesById.has(node.id)) restoredNodesById.set(node.id, node);
+		const historyNodesById = new Map<string, INode>();
+		for (const node of historyNodes) {
+			if (!historyNodesById.has(node.id)) historyNodesById.set(node.id, node);
 		}
 
 		newWorkflowVersion.nodes = newWorkflowVersion.nodes.map((node) => {
@@ -335,10 +416,14 @@ export class EnterpriseWorkflowService {
 			const idClaimedOnce = submittedIdCounts.get(node.id) === 1;
 			const blockedBySource = this.isBlockedByUninspectableSource(node, options);
 
-			// A restore brings back a node exactly as a version from this workflow's history
-			// stored it. Any change to it makes it a new edit, which the checks below judge.
-			const historyNode = restoredNodesById.get(node.id);
-			if (idClaimedOnce && historyNode && isEqual(historyNode, node)) return node;
+			const historyNode = historyNodesById.get(node.id);
+			if (
+				idClaimedOnce &&
+				historyNode &&
+				this.isSameNodeApartFromEditableFields(historyNode, node)
+			) {
+				return node;
+			}
 
 			if (!previousNode || !idClaimedOnce || !readOnlyNodeIds.has(node.id)) {
 				this.logger.warn('Blocked workflow update due to tampering attempt', {

packages/cli/test/integration/workflows/workflow.service.ee.test.ts

@@ -45,6 +45,7 @@ describe('EnterpriseWorkflowService', () => {
 			mock(), // policyEnforcementService
 			mock(), // nodeTypes
 			mock(), // credentialsPermissionChecker
+			mock(), // workflowHistoryRepository
 		);
 	});
 

packages/cli/test/integration/workflows/workflows.controller.ee.test.ts

@@ -1489,36 +1489,36 @@ describe('PATCH /workflows/:workflowId', () => {
 			});
 		});
 
-		describe('restoring a history version', () => {
-			const nodeWith = (credential: { id: string; name: string }, url = 'https://v1.test') => ({
-				id: 'uuid-restore-1',
-				name: 'Call',
-				parameters: { url },
-				position: [0, 0] as [number, number],
-				type: 'n8n-nodes-base.httpRequest',
-				typeVersion: 4.2,
-				credentials: { httpHeaderAuth: { id: credential.id, name: credential.name } },
+		const nodeWith = (credential: { id: string; name: string }, url = 'https://v1.test') => ({
+			id: 'uuid-restore-1',
+			name: 'Call',
+			parameters: { url },
+			position: [0, 0] as [number, number],
+			type: 'n8n-nodes-base.httpRequest',
+			typeVersion: 4.2,
+			credentials: { httpHeaderAuth: { id: credential.id, name: credential.name } },
+		});
+
+		// The type must match the node's credential key, so the server resolves it by id.
+		const saveHttpHeaderCredential = async (user: User) =>
+			await saveCredential({ ...randomCredentialPayload(), type: 'httpHeaderAuth' }, { user });
+
+		/**
+		 * A member's workflow whose earlier version used the owner's credential. The
+		 * current version no longer has that node, and the member cannot use it.
+		 */
+		const setUp = async () => {
+			const credential = await saveHttpHeaderCredential(owner);
+			const workflow = await createWorkflow({}, member);
+			const oldVersionId = uuid();
+			await createWorkflowHistory(workflow, member, undefined, {
+				versionId: oldVersionId,
+				nodes: [nodeWith(credential)],
 			});
+			return { credential, workflow, oldVersionId };
+		};
 
-			/**
-			 * A member's workflow whose earlier version used the owner's credential. The
-			 * current version no longer has that node, and the member cannot use it.
-			 */
-			const setUp = async () => {
-				// The type must match the node's credential key, so the server resolves it by id.
-				const credential = await saveCredential(
-					{ ...randomCredentialPayload(), type: 'httpHeaderAuth' },
-					{ user: owner },
-				);
-				const workflow = await createWorkflow({}, member);
-				const oldVersionId = uuid();
-				await createWorkflowHistory(workflow, member, undefined, {
-					versionId: oldVersionId,
-					nodes: [nodeWith(credential)],
-				});
-				return { credential, workflow, oldVersionId };
-			};
-
+		describe('restoring a history version', () => {
 			test('restores a version with a credential the user cannot use', async () => {
 				const { credential, workflow, oldVersionId } = await setUp();
 
@@ -1550,12 +1550,13 @@ describe('PATCH /workflows/:workflowId', () => {
 				});
 			});
 
-			test('blocks the same nodes in an ordinary save', async () => {
-				const { credential, workflow } = await setUp();
+			test('blocks a credential that no version stored on the node in an ordinary save', async () => {
+				const { workflow } = await setUp();
+				const otherCredential = await saveHttpHeaderCredential(owner);
 
 				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
 					versionId: workflow.versionId,
-					nodes: [nodeWith(credential)],
+					nodes: [nodeWith(otherCredential)],
 				});
 
 				expect(response.statusCode).toBe(400);
@@ -1598,6 +1599,92 @@ describe('PATCH /workflows/:workflowId', () => {
 			});
 		});
 
+		describe('switching a node back to a credential a history version stored', () => {
+			/**
+			 * The member switched the node from the owner's credential to their own and
+			 * autosave stored it: the current version has the member's credential, an
+			 * earlier version the owner's.
+			 */
+			const setUpSwitched = async () => {
+				const { credential, workflow } = await setUp();
+				const ownCredential = await saveHttpHeaderCredential(member);
+				await Container.get(WorkflowRepository).update(workflow.id, {
+					nodes: [nodeWith(ownCredential)],
+				});
+				return { credential, ownCredential, workflow };
+			};
+
+			test('saves the node switched back, renamed and moved', async () => {
+				const { credential, workflow } = await setUpSwitched();
+				const switchedBack = { ...nodeWith(credential), name: 'Renamed', position: [10, 20] };
+
+				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
+					versionId: workflow.versionId,
+					nodes: [switchedBack],
+				});
+
+				expect(response.statusCode).toBe(200);
+				expect(response.body.data.nodes).toEqual([switchedBack]);
+			});
+
+			test('saves the node switched back after its credential was renamed', async () => {
+				const { credential, workflow } = await setUpSwitched();
+				await Container.get(CredentialsRepository).update(credential.id, { name: 'Renamed' });
+
+				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
+					versionId: workflow.versionId,
+					nodes: [nodeWith({ id: credential.id, name: 'Renamed' })],
+				});
+
+				expect(response.statusCode).toBe(200);
+				expect(response.body.data.nodes[0].credentials.httpHeaderAuth).toEqual({
+					id: credential.id,
+					name: 'Renamed',
+				});
+			});
+
+			test('saves the node switched back and disabled after its credential was renamed', async () => {
+				const { credential, workflow } = await setUpSwitched();
+				await Container.get(CredentialsRepository).update(credential.id, { name: 'Renamed' });
+
+				// A disabled node is not normalized, so the old credential name reaches the guard.
+				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
+					versionId: workflow.versionId,
+					nodes: [{ ...nodeWith(credential), disabled: true }],
+				});
+
+				expect(response.statusCode).toBe(200);
+				expect(response.body.data.nodes[0].disabled).toBe(true);
+				expect(response.body.data.nodes[0].credentials.httpHeaderAuth.id).toBe(credential.id);
+			});
+
+			test('blocks the switch back when another parameter changed too', async () => {
+				const { credential, workflow } = await setUpSwitched();
+
+				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
+					versionId: workflow.versionId,
+					nodes: [nodeWith(credential, 'https://changed.test')],
+				});
+
+				expect(response.statusCode).toBe(400);
+				expect(response.body.message).toBe(
+					"You don't have access to the credentials in the 'Call' node. Ask the owner to share them with you.",
+				);
+			});
+
+			test('blocks a credential the user cannot use that no version stored on the node', async () => {
+				const { workflow } = await setUpSwitched();
+				const otherCredential = await saveHttpHeaderCredential(owner);
+
+				const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({
+					versionId: workflow.versionId,
+					nodes: [nodeWith(otherCredential)],
+				});
+
+				expect(response.statusCode).toBe(400);
+			});
+		});
+
 		describe('sub-workflow with an inspectable source', () => {
 			const databaseSubworkflowNode = (workflowId: string) => ({
 				id: 'uuid-sub-db',