fix(editor): Prevent node renaming to restricted JS method names (#16270)

packages/frontend/editor-ui/src/composables/useCanvasOperations.test.ts

@@ -7,7 +7,7 @@ import type {
 	INodeConnections,
 	WorkflowExecuteMode,
 } from 'n8n-workflow';
-import { NodeConnectionTypes, NodeHelpers } from 'n8n-workflow';
+import { NodeConnectionTypes, NodeHelpers, UserError } from 'n8n-workflow';
 import { useCanvasOperations } from '@/composables/useCanvasOperations';
 import type { CanvasConnection, CanvasNode } from '@/types';
 import { CanvasConnectionMode } from '@/types';
@@ -1041,6 +1041,35 @@ describe('useCanvasOperations', () => {
 
 			expect(ndvStore.activeNodeName).toBe(oldName);
 		});
+
+		it('should show error toast when renameNode throws an error', async () => {
+			const workflowsStore = mockedStore(useWorkflowsStore);
+			const ndvStore = mockedStore(useNDVStore);
+			const toast = useToast();
+			const oldName = 'Old Node';
+			const newName = 'New Node';
+			const errorMessage = 'Node name already exists';
+			const errorDescription = 'Please choose a different name';
+
+			const workflowObject = createTestWorkflowObject();
+			workflowObject.renameNode = vi.fn().mockImplementation(() => {
+				const error = new UserError(errorMessage, { description: errorDescription });
+				throw error;
+			});
+			workflowsStore.getCurrentWorkflow.mockReturnValue(workflowObject);
+			workflowsStore.getNodeByName = vi.fn().mockReturnValue({ name: oldName });
+			ndvStore.activeNodeName = oldName;
+
+			const { renameNode } = useCanvasOperations();
+			await renameNode(oldName, newName);
+
+			expect(workflowObject.renameNode).toHaveBeenCalledWith(oldName, newName);
+			expect(toast.showMessage).toHaveBeenCalledWith({
+				type: 'error',
+				title: errorMessage,
+				message: errorDescription,
+			});
+		});
 	});
 
 	describe('revertRenameNode', () => {

packages/frontend/editor-ui/src/composables/useCanvasOperations.ts

@@ -301,7 +301,16 @@ export function useCanvasOperations() {
 
 		// Rename the node and update the connections
 		const workflow = workflowsStore.getCurrentWorkflow(true);
-		workflow.renameNode(currentName, newName);
+		try {
+			workflow.renameNode(currentName, newName);
+		} catch (error) {
+			toast.showMessage({
+				type: 'error',
+				title: error.message,
+				message: error.description,
+			});
+			return;
+		}
 
 		if (trackHistory) {
 			historyStore.pushCommandToUndo(new RenameNodeCommand(currentName, newName, Date.now()));

packages/workflow/src/workflow.ts

@@ -7,6 +7,7 @@ import {
 	NODES_WITH_RENAMABLE_CONTENT,
 	STARTING_NODE_TYPES,
 } from './constants';
+import { UserError } from './errors';
 import { ApplicationError } from './errors/application.error';
 import { Expression } from './expression';
 import { getGlobalState } from './global-state';
@@ -379,6 +380,29 @@ export class Workflow {
 	 * @param {string} newName The new name
 	 */
 	renameNode(currentName: string, newName: string) {
+		// These keys are excluded to prevent accidental modification of inherited properties and
+		// to avoid any issues related to JavaScript's built-in methods that can cause unexpected behavior
+		const restrictedKeys = [
+			'hasOwnProperty',
+			'isPrototypeOf',
+			'propertyIsEnumerable',
+			'toLocaleString',
+			'toString',
+			'valueOf',
+			'constructor',
+			'prototype',
+			'__proto__',
+			'__defineGetter__',
+			'__defineSetter__',
+			'__lookupGetter__',
+			'__lookupSetter__',
+		];
+
+		if (restrictedKeys.map((k) => k.toLowerCase()).includes(newName.toLowerCase())) {
+			throw new UserError(`Node name "${newName}" is a restricted name.`, {
+				description: `Node names cannot be any of the following: ${restrictedKeys.join(', ')}`,
+			});
+		}
 		// Rename the node itself
 		if (this.nodes[currentName] !== undefined) {
 			this.nodes[newName] = this.nodes[currentName];

packages/workflow/test/workflow.test.ts

@@ -1,5 +1,6 @@
 import { mock } from 'jest-mock-extended';
 
+import { UserError } from '@/errors';
 import { NodeConnectionTypes } from '@/interfaces';
 import type {
 	IBinaryKeyData,
@@ -1077,6 +1078,129 @@ describe('Workflow', () => {
 				expect(workflow.connectionsBySourceNode).toEqual(testData.output.connections);
 			});
 		}
+
+		describe('with restricted node names', () => {
+			const restrictedNames = [
+				'hasOwnProperty',
+				'isPrototypeOf',
+				'propertyIsEnumerable',
+				'toLocaleString',
+				'toString',
+				'valueOf',
+				'constructor',
+				'prototype',
+				'__proto__',
+				'__defineGetter__',
+				'__defineSetter__',
+				'__lookupGetter__',
+				'__lookupSetter__',
+			];
+
+			test.each(restrictedNames)(
+				'should throw error when renaming node to %s',
+				(restrictedName) => {
+					const workflow = new Workflow({
+						nodes: [
+							{
+								name: 'Node1',
+								parameters: {},
+								type: 'test.set',
+								typeVersion: 1,
+								id: 'uuid-1',
+								position: [100, 100],
+							},
+						],
+						connections: {},
+						active: false,
+						nodeTypes,
+					});
+
+					expect(() => workflow.renameNode('Node1', restrictedName)).toThrow(
+						`Node name "${restrictedName}" is a restricted name.`,
+					);
+				},
+			);
+
+			test.each(restrictedNames)(
+				'should throw error when renaming node to %s with different case',
+				(restrictedName) => {
+					const workflow = new Workflow({
+						nodes: [
+							{
+								name: 'Node1',
+								parameters: {},
+								type: 'test.set',
+								typeVersion: 1,
+								id: 'uuid-1',
+								position: [100, 100],
+							},
+						],
+						connections: {},
+						active: false,
+						nodeTypes,
+					});
+
+					const upperCaseName = restrictedName.toUpperCase();
+					expect(() => workflow.renameNode('Node1', upperCaseName)).toThrow(
+						`Node name "${upperCaseName}" is a restricted name.`,
+					);
+				},
+			);
+
+			test('should throw error with proper description', () => {
+				const workflow = new Workflow({
+					nodes: [
+						{
+							name: 'Node1',
+							parameters: {},
+							type: 'test.set',
+							typeVersion: 1,
+							id: 'uuid-1',
+							position: [100, 100],
+						},
+					],
+					connections: {},
+					active: false,
+					nodeTypes,
+				});
+
+				try {
+					workflow.renameNode('Node1', 'toString');
+				} catch (error) {
+					if (!(error instanceof UserError)) {
+						throw new Error('Expected error to be an instance of UserError');
+					}
+					expect(error).toBeInstanceOf(UserError);
+					expect(error.message).toBe('Node name "toString" is a restricted name.');
+					expect(error.description).toBe(
+						'Node names cannot be any of the following: hasOwnProperty, isPrototypeOf, propertyIsEnumerable, toLocaleString, toString, valueOf, constructor, prototype, __proto__, __defineGetter__, __defineSetter__, __lookupGetter__, __lookupSetter__',
+					);
+				}
+			});
+
+			test('should allow renaming to names that contain restricted names as substring', () => {
+				const workflow = new Workflow({
+					nodes: [
+						{
+							name: 'Node1',
+							parameters: {},
+							type: 'test.set',
+							typeVersion: 1,
+							id: 'uuid-1',
+							position: [100, 100],
+						},
+					],
+					connections: {},
+					active: false,
+					nodeTypes,
+				});
+
+				// These should not throw as they're not exact matches
+				expect(() => workflow.renameNode('Node1', 'myToString')).not.toThrow();
+				expect(() => workflow.renameNode('Node1', 'toStringNode')).not.toThrow();
+				expect(() => workflow.renameNode('Node1', 'hasOwnPropertyChecker')).not.toThrow();
+			});
+		});
 	});
 
 	describe('getParameterValue', () => {