diff --git a/packages/frontend/editor-ui/src/composables/useCanvasOperations.test.ts b/packages/frontend/editor-ui/src/composables/useCanvasOperations.test.ts index 3e0ac99782..8d16862982 100644 --- a/packages/frontend/editor-ui/src/composables/useCanvasOperations.test.ts +++ b/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', () => { diff --git a/packages/frontend/editor-ui/src/composables/useCanvasOperations.ts b/packages/frontend/editor-ui/src/composables/useCanvasOperations.ts index 8f225acaa3..6420a9f625 100644 --- a/packages/frontend/editor-ui/src/composables/useCanvasOperations.ts +++ b/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())); diff --git a/packages/workflow/src/workflow.ts b/packages/workflow/src/workflow.ts index 6b54a5d9ae..68b95239f4 100644 --- a/packages/workflow/src/workflow.ts +++ b/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]; diff --git a/packages/workflow/test/workflow.test.ts b/packages/workflow/test/workflow.test.ts index 67782bd68e..0973220b78 100644 --- a/packages/workflow/test/workflow.test.ts +++ b/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', () => {