diff --git a/packages/@n8n/db/src/repositories/workflow-history.repository.ts b/packages/@n8n/db/src/repositories/workflow-history.repository.ts index b51aaa06d0d..8e484f58674 100644 --- a/packages/@n8n/db/src/repositories/workflow-history.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-history.repository.ts @@ -63,6 +63,22 @@ export class WorkflowHistoryRepository extends BaseRepository { 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>> { + return await this.find({ + select: ['versionId', 'nodes'], + where: { workflowId }, + order: { createdAt: 'DESC', versionId: 'DESC' }, + take: limit, + }); + } + async findVersionSummaries( workflowId: string, versionIds: string[], diff --git a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts index b58048c0053..34e865ed209 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts +++ b/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(); const nodeTypes = mock(); const credentialsPermissionChecker = mock(); + const workflowHistoryRepository = mock(); 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({ 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 diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index f6a116d5688..9b36db171ce 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/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 { + 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( 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(); - for (const node of restoredNodes) { - if (!restoredNodesById.has(node.id)) restoredNodesById.set(node.id, node); + const historyNodesById = new Map(); + 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', { diff --git a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts index ff3f76e42b9..932d2ea103c 100644 --- a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts @@ -45,6 +45,7 @@ describe('EnterpriseWorkflowService', () => { mock(), // policyEnforcementService mock(), // nodeTypes mock(), // credentialsPermissionChecker + mock(), // workflowHistoryRepository ); }); diff --git a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts index 5dd1907f9f9..e67cbe29577 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts +++ b/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',