From 94505bfad456b72fd4edd7f43bb2be8ba0329a68 Mon Sep 17 00:00:00 2001 From: Jaakko Husso Date: Fri, 21 Nov 2025 13:58:41 +0200 Subject: [PATCH] fix(core): Safely handle paths at Data Table CSV imports (no-changelog) (#22139) --- .../__tests__/csv-parser.service.test.ts | 78 +++++++++++++++++++ .../data-table-file-cleanup.service.test.ts | 8 ++ .../modules/data-table/csv-parser.service.ts | 20 +++-- .../data-table-file-cleanup.service.ts | 6 +- 4 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 packages/cli/src/modules/data-table/__tests__/csv-parser.service.test.ts diff --git a/packages/cli/src/modules/data-table/__tests__/csv-parser.service.test.ts b/packages/cli/src/modules/data-table/__tests__/csv-parser.service.test.ts new file mode 100644 index 00000000000..e7e687fa8f8 --- /dev/null +++ b/packages/cli/src/modules/data-table/__tests__/csv-parser.service.test.ts @@ -0,0 +1,78 @@ +import { testModules } from '@n8n/backend-test-utils'; +import type { GlobalConfig } from '@n8n/config'; +import { mock } from 'jest-mock-extended'; + +import { CsvParserService } from '../csv-parser.service'; + +beforeAll(async () => { + await testModules.loadModules(['data-table']); +}); +describe('CsvParserService', () => { + describe('parseFile', () => { + it('should not allow path traversal when parsing CSV file metadata', async () => { + const globalConfig = mock({ + dataTable: { + uploadDir: '/safe/upload/dir', + }, + }); + + const csvParserService = new CsvParserService(globalConfig); + + const maliciousFileId = '../some/other/directory/malicious-file.csv'; + + await expect(csvParserService.parseFile(maliciousFileId)).rejects.toThrowError( + 'Path traversal detected', + ); + }); + + it('should try to access file if it is within upload directory', async () => { + const globalConfig = mock({ + dataTable: { + uploadDir: '/safe/upload/dir', + }, + }); + + const csvParserService = new CsvParserService(globalConfig); + const safeFileId = 'valid-file.csv'; + + // Since we are not actually testing file reading here, just ensure no error is thrown + await expect(csvParserService.parseFile(safeFileId)).rejects.toThrowError( + "ENOENT: no such file or directory, open '/safe/upload/dir/valid-file.csv", + ); + }); + }); + + describe('parseFileData', () => { + it('should not allow path traversal when parsing CSV file data', async () => { + const globalConfig = mock({ + dataTable: { + uploadDir: '/safe/upload/dir', + }, + }); + + const csvParserService = new CsvParserService(globalConfig); + + const maliciousFileId = '../some/other/directory/malicious-file.csv'; + + await expect(csvParserService.parseFileData(maliciousFileId)).rejects.toThrowError( + 'Path traversal detected', + ); + }); + + it('should try to access file if it is within upload directory', async () => { + const globalConfig = mock({ + dataTable: { + uploadDir: '/safe/upload/dir', + }, + }); + + const csvParserService = new CsvParserService(globalConfig); + const safeFileId = 'valid-file.csv'; + + // Since we are not actually testing file reading here, just ensure no error is thrown + await expect(csvParserService.parseFileData(safeFileId)).rejects.toThrowError( + "ENOENT: no such file or directory, open '/safe/upload/dir/valid-file.csv", + ); + }); + }); +}); diff --git a/packages/cli/src/modules/data-table/__tests__/data-table-file-cleanup.service.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table-file-cleanup.service.test.ts index f0c752a317f..0ad6732359f 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table-file-cleanup.service.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table-file-cleanup.service.test.ts @@ -66,6 +66,14 @@ describe('DataTableFileCleanupService', () => { await expect(service.deleteFile(fileId)).rejects.toThrow('Permission denied'); }); + + it('should not allow path traversal when deleting file', async () => { + const maliciousFileId = '../some/other/directory/malicious-file.csv'; + + await expect(service.deleteFile(maliciousFileId)).rejects.toThrowError( + 'Path traversal detected', + ); + }); }); describe('start and shutdown', () => { diff --git a/packages/cli/src/modules/data-table/csv-parser.service.ts b/packages/cli/src/modules/data-table/csv-parser.service.ts index 0683686d5df..3b62b447511 100644 --- a/packages/cli/src/modules/data-table/csv-parser.service.ts +++ b/packages/cli/src/modules/data-table/csv-parser.service.ts @@ -1,8 +1,8 @@ +import { safeJoinPath } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import { parse } from 'csv-parse'; import { createReadStream } from 'fs'; -import path from 'path'; export interface CsvColumnMetadata { name: string; @@ -44,7 +44,7 @@ export class CsvParserService { * Parses a CSV file and returns metadata including row count, column count, and inferred column types */ async parseFile(fileId: string, hasHeaders: boolean = true): Promise { - const filePath = path.join(this.uploadDir, fileId); + const filePath = safeJoinPath(this.uploadDir, fileId); let rowCount = 0; let firstDataRow: Record | null = null; let columnNames: string[] = []; @@ -58,10 +58,7 @@ export class CsvParserService { } : false, skip_empty_lines: true, - }); - - createReadStream(filePath) - .pipe(parser) + }) .on('data', (row: Record | string[]) => { rowCount++; @@ -90,6 +87,8 @@ export class CsvParserService { }); }) .on('error', reject); + + createReadStream(filePath).on('error', reject).pipe(parser); }); } @@ -100,7 +99,7 @@ export class CsvParserService { fileId: string, hasHeaders: boolean = true, ): Promise>> { - const filePath = path.join(this.uploadDir, fileId); + const filePath = safeJoinPath(this.uploadDir, fileId); const rows: Array> = []; let columnNames: string[] = []; @@ -109,10 +108,7 @@ export class CsvParserService { const parser = parse({ columns: hasHeaders ? true : false, skip_empty_lines: true, - }); - - createReadStream(filePath) - .pipe(parser) + }) .on('data', (row: Record | string[]) => { if (!hasHeaders && Array.isArray(row)) { const processed = this.processRowWithoutHeaders(row, columnNames); @@ -126,6 +122,8 @@ export class CsvParserService { resolve(rows); }) .on('error', reject); + + createReadStream(filePath).on('error', reject).pipe(parser); }); } diff --git a/packages/cli/src/modules/data-table/data-table-file-cleanup.service.ts b/packages/cli/src/modules/data-table/data-table-file-cleanup.service.ts index 9d2b0f3a3b9..a2dacce02c9 100644 --- a/packages/cli/src/modules/data-table/data-table-file-cleanup.service.ts +++ b/packages/cli/src/modules/data-table/data-table-file-cleanup.service.ts @@ -1,7 +1,7 @@ +import { safeJoinPath } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import { promises as fs } from 'fs'; -import path from 'path'; @Service() export class DataTableFileCleanupService { @@ -47,7 +47,7 @@ export class DataTableFileCleanupService { const maxAge = this.globalConfig.dataTable.fileMaxAgeMs; for (const file of files) { - const filePath = path.join(this.uploadDir, file); + const filePath = safeJoinPath(this.uploadDir, file); try { const stats = await fs.stat(filePath); const fileAge = now - stats.mtimeMs; @@ -74,7 +74,7 @@ export class DataTableFileCleanupService { * Deletes a specific CSV file by its fileId */ async deleteFile(fileId: string): Promise { - const filePath = path.join(this.uploadDir, fileId); + const filePath = safeJoinPath(this.uploadDir, fileId); try { await fs.unlink(filePath); } catch (error) {