From c9cb3ee3abd23fe146cd4a2daed49c4bddb80d97 Mon Sep 17 00:00:00 2001 From: Pierantonio Cangianiello <10360978+pcan@users.noreply.github.com> Date: Mon, 12 Oct 2026 13:08:38 +0000 Subject: [PATCH] fix(core): Let the editor download binary data of an Engine v2 execution (#40663) --- .../binary-data-access.service.test.ts | 113 ++++++++++++++++-- .../binary-data/binary-data-access.service.ts | 41 +++++-- packages/cli/src/executions/execution-id.ts | 7 ++ .../pages/components/RunDataPanel.ts | 5 + .../editor/ndv/binary-data-download.spec.ts | 38 ++++++ .../workflows/binary-data-download.json | 59 +++++++++ 6 files changed, 246 insertions(+), 17 deletions(-) create mode 100644 packages/quality/testing/playwright/tests/e2e/workflows/editor/ndv/binary-data-download.spec.ts create mode 100644 packages/quality/testing/playwright/workflows/binary-data-download.json diff --git a/packages/cli/src/binary-data/__tests__/binary-data-access.service.test.ts b/packages/cli/src/binary-data/__tests__/binary-data-access.service.test.ts index 39fac7008a7..9822e4c9939 100644 --- a/packages/cli/src/binary-data/__tests__/binary-data-access.service.test.ts +++ b/packages/cli/src/binary-data/__tests__/binary-data-access.service.test.ts @@ -3,20 +3,27 @@ import { mock } from 'vitest-mock-extended'; import type { WorkflowSharingService } from '@n8n/backend-services'; +import type { ExecutionSnapshot } from '@n8n/engine'; +import type { EngineDataPlaneProxyService } from '@/services/engine-data-plane-proxy.service'; + import { BinaryDataAccessService } from '../binary-data-access.service'; describe('BinaryDataAccessService', () => { const workflowSharingService = mock(); const executionRepository = mock(); const binaryDataRepository = mock(); + const dataPlane = mock(); const service = new BinaryDataAccessService( workflowSharingService, executionRepository, binaryDataRepository, + dataPlane, ); const user = mock(); const uuid = '2c3f1e5a-0b6d-4c8e-9f11-abc123def456'; + /** Engine v2 mints uuidv7 execution ids, which the v1 execution table cannot hold. */ + const v2ExecutionId = '019a4c2e-7b3d-7c1e-8f2a-3b4c5d6e7f80'; beforeEach(() => { vi.resetAllMocks(); @@ -28,16 +35,14 @@ describe('BinaryDataAccessService', () => { workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf1']); executionRepository.existsForAccessibleWorkflows.mockResolvedValue(true); - const id = `${mode}:workflows/wf1/executions/exec1/binary_data/${uuid}`; + const id = `${mode}:workflows/wf1/executions/1/binary_data/${uuid}`; const result = await service.hasReadAccess(user, id); expect(result).toBe(true); expect(workflowSharingService.getSharedWorkflowIds).toHaveBeenCalledWith(user, { scopes: ['workflow:read'], }); - expect(executionRepository.existsForAccessibleWorkflows).toHaveBeenCalledWith('exec1', [ - 'wf1', - ]); + expect(executionRepository.existsForAccessibleWorkflows).toHaveBeenCalledWith('1', ['wf1']); expect(binaryDataRepository.findSourceByFileId).not.toHaveBeenCalled(); }); }); @@ -46,14 +51,14 @@ describe('BinaryDataAccessService', () => { workflowSharingService.getSharedWorkflowIds.mockResolvedValue([]); executionRepository.existsForAccessibleWorkflows.mockResolvedValue(false); - const id = `filesystem-v2:workflows/wf1/executions/exec1/binary_data/${uuid}`; + const id = `filesystem-v2:workflows/wf1/executions/1/binary_data/${uuid}`; expect(await service.hasReadAccess(user, id)).toBe(false); }); test('resolves the execution from a database row', async () => { binaryDataRepository.findSourceByFileId.mockResolvedValue({ sourceType: 'execution', - sourceId: 'exec9', + sourceId: '9', }); workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf9']); executionRepository.existsForAccessibleWorkflows.mockResolvedValue(true); @@ -62,9 +67,8 @@ describe('BinaryDataAccessService', () => { expect(result).toBe(true); expect(binaryDataRepository.findSourceByFileId).toHaveBeenCalledWith(uuid); - expect(executionRepository.existsForAccessibleWorkflows).toHaveBeenCalledWith('exec9', [ - 'wf9', - ]); + expect(executionRepository.existsForAccessibleWorkflows).toHaveBeenCalledWith('9', ['wf9']); + expect(dataPlane.getExecution).not.toHaveBeenCalled(); }); test('denies a database row that is not an execution', async () => { @@ -129,6 +133,97 @@ describe('BinaryDataAccessService', () => { }); }); + // An engine v2 execution has no row in the execution table, and its uuid + // would fail the numeric lookup. The path, or the data plane, names the + // workflow instead. + describe('for a binary of an engine v2 execution', () => { + beforeEach(() => { + executionRepository.existsForAccessibleWorkflows.mockRejectedValue( + new Error('invalid input syntax for type integer: "NaN"'), + ); + }); + + describe('in a path-format id', () => { + const v2Id = (mode: string) => + `${mode}:workflows/wf1/executions/${v2ExecutionId}/binary_data/${uuid}`; + + test.each(['filesystem', 'filesystem-v2', 's3', 'azure'])( + 'grants access from the workflow in the path for mode %s, without an execution lookup', + async (mode) => { + workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf1']); + + expect(await service.hasReadAccess(user, v2Id(mode))).toBe(true); + expect(executionRepository.existsForAccessibleWorkflows).not.toHaveBeenCalled(); + expect(dataPlane.getExecution).not.toHaveBeenCalled(); + }, + ); + + test('denies when the user cannot read the workflow in the path', async () => { + workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf2']); + + expect(await service.hasReadAccess(user, v2Id('filesystem-v2'))).toBe(false); + }); + }); + + describe('in a database row', () => { + beforeEach(() => { + binaryDataRepository.findSourceByFileId.mockResolvedValue({ + sourceType: 'execution', + sourceId: v2ExecutionId, + }); + }); + + test('grants access from the workflow the data plane names, without the steps', async () => { + dataPlane.getExecution.mockResolvedValue(mock({ workflowId: 'wf9' })); + workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf9']); + + expect(await service.hasReadAccess(user, `database:${uuid}`)).toBe(true); + expect(dataPlane.getExecution).toHaveBeenCalledWith(v2ExecutionId); + expect(executionRepository.existsForAccessibleWorkflows).not.toHaveBeenCalled(); + }); + + test('denies when the user cannot read the workflow the data plane names', async () => { + dataPlane.getExecution.mockResolvedValue(mock({ workflowId: 'wf9' })); + workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf1']); + + expect(await service.hasReadAccess(user, `database:${uuid}`)).toBe(false); + }); + + test('denies when the data plane knows no such execution', async () => { + dataPlane.getExecution.mockResolvedValue(undefined); + + expect(await service.hasReadAccess(user, `database:${uuid}`)).toBe(false); + expect(workflowSharingService.getSharedWorkflowIds).not.toHaveBeenCalled(); + }); + + test('propagates a data plane error, so the controller answers with an error', async () => { + dataPlane.getExecution.mockRejectedValue(new Error('engine unreachable')); + + await expect(service.hasReadAccess(user, `database:${uuid}`)).rejects.toThrow( + 'engine unreachable', + ); + expect(workflowSharingService.getSharedWorkflowIds).not.toHaveBeenCalled(); + }); + }); + }); + + // The execution id column is numeric, so an id that is neither a v1 number + // nor a v2 uuid must be refused before it reaches the query as NaN. + describe('for a path id whose execution segment names no execution', () => { + test.each([ + ['a non-numeric id', 'abc'], + ['a uuid that is not v7', '2c3f1e5a-0b6d-4c8e-9f11-abc123def456'], + ['a number above the 32-bit id column', '2147483648'], + ])('denies %s without a lookup', async (_, executionId) => { + const id = `filesystem-v2:workflows/wf1/executions/${executionId}/binary_data/${uuid}`; + + expect(await service.hasReadAccess(user, id)).toBe(false); + expect(workflowSharingService.getSharedWorkflowIds).not.toHaveBeenCalled(); + expect(executionRepository.existsForAccessibleWorkflows).not.toHaveBeenCalled(); + expect(dataPlane.getExecution).not.toHaveBeenCalled(); + }); + }); + test('denies a malformed id without a mode separator', async () => { expect(await service.hasReadAccess(user, 'no-separator')).toBe(false); expect(workflowSharingService.getSharedWorkflowIds).not.toHaveBeenCalled(); diff --git a/packages/cli/src/binary-data/binary-data-access.service.ts b/packages/cli/src/binary-data/binary-data-access.service.ts index 14bde51d6d4..971dcc79d9a 100644 --- a/packages/cli/src/binary-data/binary-data-access.service.ts +++ b/packages/cli/src/binary-data/binary-data-access.service.ts @@ -5,10 +5,16 @@ import { parseExecutionFileId, TEMP_EXECUTION_ID } from 'n8n-core'; import { WorkflowSharingService } from '@n8n/backend-services'; +import { type ExecutionIdV2, isExecutionIdV1, isExecutionIdV2 } from '@/executions/execution-id'; +import { EngineDataPlaneProxyService } from '@/services/engine-data-plane-proxy.service'; + /** * Workflow that a binary derives its access from. Normally the workflow of the - * execution that owns the binary. A binary written before its execution row - * exists names its workflow in the file path instead. + * execution that owns the binary, looked up by execution id. The workflow is + * named directly when no row can be looked up: a binary written before its + * execution row exists names its workflow in the file path, and a binary of an + * engine v2 execution, which has no row in the execution table, names it in + * the file path or through the data plane. */ type AccessSource = { executionId: string } | { workflowId: string }; @@ -22,6 +28,7 @@ export class BinaryDataAccessService { private readonly workflowSharingService: WorkflowSharingService, private readonly executionRepository: ExecutionRepository, private readonly binaryDataRepository: BinaryDataRepository, + private readonly dataPlane: EngineDataPlaneProxyService, ) {} /** Whether `user` may read the execution that owns `binaryDataId`. */ @@ -57,18 +64,36 @@ export class BinaryDataAccessService { const source = await this.binaryDataRepository.findSourceByFileId(fileId); if (source?.sourceType !== 'execution') return null; + if (isExecutionIdV1(source.sourceId)) return { executionId: source.sourceId }; + // The row names no workflow, so a temp placeholder has nothing to fall back on. - return source.sourceId === TEMP_EXECUTION_ID ? null : { executionId: source.sourceId }; + return isExecutionIdV2(source.sourceId) + ? await this.resolveEngineV2Execution(source.sourceId) + : null; } // filesystem / filesystem-v2 / s3 / azure embed the execution in the path const location = parseExecutionFileId(fileId); if (!location) return null; - // The execution id column is numeric, so the placeholder must not reach the - // query. Authorize on the workflow the path carries instead. - return location.executionId === TEMP_EXECUTION_ID - ? { workflowId: location.workflowId } - : { executionId: location.executionId }; + const { workflowId, executionId } = location; + if (isExecutionIdV1(executionId)) return { executionId }; + + // The execution id column is numeric, so neither the placeholder nor an + // engine v2 id can be looked up, and any other id names no execution. The + // path is where the file was stored, so the workflow in it authorizes the read. + return executionId === TEMP_EXECUTION_ID || isExecutionIdV2(executionId) + ? { workflowId } + : null; + } + + /** + * A `database` row of an engine v2 execution names no workflow, so the data + * plane is asked which workflow the execution belongs to. Its steps are not + * needed. No data plane, or no such execution, means no access. + */ + private async resolveEngineV2Execution(executionId: ExecutionIdV2): Promise { + const execution = await this.dataPlane.getExecution(executionId); + return execution ? { workflowId: execution.workflowId } : null; } } diff --git a/packages/cli/src/executions/execution-id.ts b/packages/cli/src/executions/execution-id.ts index 8c43d8d1c79..e4eaa1be773 100644 --- a/packages/cli/src/executions/execution-id.ts +++ b/packages/cli/src/executions/execution-id.ts @@ -7,6 +7,13 @@ export type ExecutionIdV2 = string & { readonly __brand: 'ExecutionIdV2' }; /** A v1 id is numeric and a v2 id is a UUID, so the shape alone picks the backend. */ export const isExecutionIdV2 = (id: string): id is ExecutionIdV2 => UUID_V7_PATTERN.test(id); +/** The v1 execution id column is a 32-bit integer (`SERIAL` on Postgres). */ +const MAX_EXECUTION_ID_V1 = 2 ** 31 - 1; + +/** Only a number that fits the v1 id column can name a row, so a larger one is refused. */ +export const isExecutionIdV1 = (id: string): boolean => + /^\d+$/.test(id) && Number(id) <= MAX_EXECUTION_ID_V1; + /** * Mints an id for a run the control plane hands to the engine. * diff --git a/packages/quality/testing/playwright/pages/components/RunDataPanel.ts b/packages/quality/testing/playwright/pages/components/RunDataPanel.ts index 8c2191bfa47..169c1ad323e 100644 --- a/packages/quality/testing/playwright/pages/components/RunDataPanel.ts +++ b/packages/quality/testing/playwright/pages/components/RunDataPanel.ts @@ -194,4 +194,9 @@ export class RunDataPanel { async switchDisplayMode(mode: 'table' | 'ai' | 'json' | 'schema' | 'binary'): Promise { await this.root.getByTestId(`radio-button-${mode}`).click(); } + + /** The download button of a binary entry in the binary display mode. */ + getDownloadBinaryDataButton(index = 0) { + return this.root.getByTestId('ndv-download-binary-data').nth(index); + } } diff --git a/packages/quality/testing/playwright/tests/e2e/workflows/editor/ndv/binary-data-download.spec.ts b/packages/quality/testing/playwright/tests/e2e/workflows/editor/ndv/binary-data-download.spec.ts new file mode 100644 index 00000000000..24a4656dfc9 --- /dev/null +++ b/packages/quality/testing/playwright/tests/e2e/workflows/editor/ndv/binary-data-download.spec.ts @@ -0,0 +1,38 @@ +import { test, expect } from '../../../../../fixtures/base'; + +/** + * The editor downloads a file of an execution through `GET /rest/binary-data`, + * which checks that the user may read the execution that owns the file. An + * engine v2 execution has no row in the v1 execution table, so the check must + * not look there. On a stack without engine v2 the same spec covers the v1 + * path. + */ +test.describe( + 'Binary data download from the NDV', + { + annotation: [{ type: 'owner', description: 'Catalysts' }], + }, + () => { + test('downloads a file that the run stored @engine:v2', async ({ n8n }) => { + await n8n.start.fromImportedWorkflow('binary-data-download.json'); + await n8n.canvas.clickExecuteWorkflowButton(); + + await n8n.canvas.openNode('Convert to File'); + await expect(n8n.ndv.outputPanel.getDataContainer()).toBeVisible(); + await n8n.ndv.outputPanel.switchDisplayMode('binary'); + + const downloadPromise = n8n.page.waitForEvent('download'); + await n8n.ndv.outputPanel.getDownloadBinaryDataButton().click(); + const download = await downloadPromise; + + // A refused download arrives as a failed event, not as an exception. + expect(await download.failure()).toBeNull(); + expect(download.suggestedFilename()).toBe('download.txt'); + const chunks: Buffer[] = []; + for await (const chunk of await download.createReadStream()) { + chunks.push(Buffer.from(chunk)); + } + expect(Buffer.concat(chunks).toString('utf8')).toBe('the editor downloads this file'); + }); + }, +); diff --git a/packages/quality/testing/playwright/workflows/binary-data-download.json b/packages/quality/testing/playwright/workflows/binary-data-download.json new file mode 100644 index 00000000000..c07bc2feaed --- /dev/null +++ b/packages/quality/testing/playwright/workflows/binary-data-download.json @@ -0,0 +1,59 @@ +{ + "name": "Binary data download", + "nodes": [ + { + "parameters": {}, + "id": "manual-trigger", + "name": "Manual Trigger", + "type": "n8n-nodes-base.manualTrigger", + "typeVersion": 1, + "position": [0, 0] + }, + { + "parameters": { + "assignments": { + "assignments": [ + { + "id": "text", + "name": "text", + "value": "the editor downloads this file", + "type": "string" + } + ] + }, + "options": {} + }, + "id": "edit-fields", + "name": "Edit Fields", + "type": "n8n-nodes-base.set", + "typeVersion": 3.4, + "position": [220, 0] + }, + { + "parameters": { + "operation": "toText", + "sourceProperty": "text", + "binaryPropertyName": "data", + "options": { + "fileName": "download.txt" + } + }, + "id": "convert-to-file", + "name": "Convert to File", + "type": "n8n-nodes-base.convertToFile", + "typeVersion": 1.1, + "position": [440, 0] + } + ], + "connections": { + "Manual Trigger": { + "main": [[{ "node": "Edit Fields", "type": "main", "index": 0 }]] + }, + "Edit Fields": { + "main": [[{ "node": "Convert to File", "type": "main", "index": 0 }]] + } + }, + "settings": { + "executionOrder": "v1" + } +}