fix(core): Let the editor download binary data of an Engine v2 execution (#40663)

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<WorkflowSharingService>();
 	const executionRepository = mock<ExecutionRepository>();
 	const binaryDataRepository = mock<BinaryDataRepository>();
+	const dataPlane = mock<EngineDataPlaneProxyService>();
 	const service = new BinaryDataAccessService(
 		workflowSharingService,
 		executionRepository,
 		binaryDataRepository,
+		dataPlane,
 	);
 
 	const user = mock<User>();
 	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<ExecutionSnapshot>({ 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<ExecutionSnapshot>({ 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();

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<AccessSource | null> {
+		const execution = await this.dataPlane.getExecution(executionId);
+		return execution ? { workflowId: execution.workflowId } : null;
 	}
 }

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.
  *

packages/quality/testing/playwright/pages/components/RunDataPanel.ts

@@ -194,4 +194,9 @@ export class RunDataPanel {
 	async switchDisplayMode(mode: 'table' | 'ai' | 'json' | 'schema' | 'binary'): Promise<void> {
 		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);
+	}
 }

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');
+		});
+	},
+);

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"
+	}
+}