From c2c3e08cdf33570d9051e659812cbfbdd3c077fd Mon Sep 17 00:00:00 2001 From: RomanDavydchuk Date: Fri, 1 Aug 2025 10:59:53 +0300 Subject: [PATCH] fix(core): Handle symlinks in blocked paths (#17735) --- .../file-system-helper-functions.test.ts | 79 +++++++++++-------- .../utils/file-system-helper-functions.ts | 15 ++-- .../nodes/Crypto/test/Crypto.test.ts | 2 + .../ReadWriteFile/test/ReadWriteFile.test.ts | 4 + .../test/WriteBinaryFile.test.ts | 4 + 5 files changed, 64 insertions(+), 40 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index 8972b1210e..c952767160 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -1,7 +1,7 @@ import { Container } from '@n8n/di'; import type { INode } from 'n8n-workflow'; import { createReadStream } from 'node:fs'; -import { access as fsAccess } from 'node:fs/promises'; +import { access as fsAccess, realpath as fsRealpath } from 'node:fs/promises'; import { join } from 'node:path'; import { @@ -30,6 +30,7 @@ beforeEach(() => { // @ts-expect-error undefined property error.code = 'ENOENT'; (fsAccess as jest.Mock).mockRejectedValue(error); + (fsRealpath as jest.Mock).mockImplementation((path: string) => path); instanceSettings = Container.get(InstanceSettings); }); @@ -39,115 +40,125 @@ describe('isFilePathBlocked', () => { process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true'; }); - it('should return true for static cache dir', () => { + it('should return true for static cache dir', async () => { const filePath = instanceSettings.staticCacheDir; - expect(isFilePathBlocked(filePath)).toBe(true); + expect(await isFilePathBlocked(filePath)).toBe(true); }); - it('should return true for restricted paths', () => { + it('should return true for restricted paths', async () => { const restrictedPath = instanceSettings.n8nFolder; - expect(isFilePathBlocked(restrictedPath)).toBe(true); + expect(await isFilePathBlocked(restrictedPath)).toBe(true); }); - it('should handle empty allowed paths', () => { + it('should handle empty allowed paths', async () => { delete process.env[RESTRICT_FILE_ACCESS_TO]; - const result = isFilePathBlocked('/some/random/path'); + const result = await isFilePathBlocked('/some/random/path'); expect(result).toBe(false); }); - it('should handle multiple allowed paths', () => { + it('should handle multiple allowed paths', async () => { process.env[RESTRICT_FILE_ACCESS_TO] = '/path1;/path2;/path3'; const allowedPath = '/path2/somefile'; - expect(isFilePathBlocked(allowedPath)).toBe(false); + expect(await isFilePathBlocked(allowedPath)).toBe(false); }); - it('should handle empty strings in allowed paths', () => { + it('should handle empty strings in allowed paths', async () => { process.env[RESTRICT_FILE_ACCESS_TO] = '/path1;;/path2'; const allowedPath = '/path2/somefile'; - expect(isFilePathBlocked(allowedPath)).toBe(false); + expect(await isFilePathBlocked(allowedPath)).toBe(false); }); - it('should trim whitespace in allowed paths', () => { + it('should trim whitespace in allowed paths', async () => { process.env[RESTRICT_FILE_ACCESS_TO] = ' /path1 ; /path2 ; /path3 '; const allowedPath = '/path2/somefile'; - expect(isFilePathBlocked(allowedPath)).toBe(false); + expect(await isFilePathBlocked(allowedPath)).toBe(false); }); - it('should return false when BLOCK_FILE_ACCESS_TO_N8N_FILES is false', () => { + it('should return false when BLOCK_FILE_ACCESS_TO_N8N_FILES is false', async () => { process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'false'; const restrictedPath = instanceSettings.n8nFolder; - expect(isFilePathBlocked(restrictedPath)).toBe(false); + expect(await isFilePathBlocked(restrictedPath)).toBe(false); }); - it('should return true when path is in allowed paths but still restricted', () => { + it('should return true when path is in allowed paths but still restricted', async () => { process.env[RESTRICT_FILE_ACCESS_TO] = '/some/allowed/path'; const restrictedPath = instanceSettings.n8nFolder; - expect(isFilePathBlocked(restrictedPath)).toBe(true); + expect(await isFilePathBlocked(restrictedPath)).toBe(true); }); - it('should return false when path is in allowed paths', () => { + it('should return false when path is in allowed paths', async () => { const allowedPath = '/some/allowed/path'; process.env[RESTRICT_FILE_ACCESS_TO] = allowedPath; - expect(isFilePathBlocked(allowedPath)).toBe(false); + expect(await isFilePathBlocked(allowedPath)).toBe(false); }); - it('should return true when file paths in CONFIG_FILES', () => { + it('should return true when file paths in CONFIG_FILES', async () => { process.env[CONFIG_FILES] = '/path/to/config1,/path/to/config2'; const configPath = '/path/to/config1/somefile'; - expect(isFilePathBlocked(configPath)).toBe(true); + expect(await isFilePathBlocked(configPath)).toBe(true); }); - it('should return true when file paths in CUSTOM_EXTENSION_ENV', () => { + it('should return true when file paths in CUSTOM_EXTENSION_ENV', async () => { process.env[CUSTOM_EXTENSION_ENV] = '/path/to/extensions1;/path/to/extensions2'; const extensionPath = '/path/to/extensions1/somefile'; - expect(isFilePathBlocked(extensionPath)).toBe(true); + expect(await isFilePathBlocked(extensionPath)).toBe(true); }); - it('should return true when file paths in BINARY_DATA_STORAGE_PATH', () => { + it('should return true when file paths in BINARY_DATA_STORAGE_PATH', async () => { process.env[BINARY_DATA_STORAGE_PATH] = '/path/to/binary/storage'; const binaryPath = '/path/to/binary/storage/somefile'; - expect(isFilePathBlocked(binaryPath)).toBe(true); + expect(await isFilePathBlocked(binaryPath)).toBe(true); }); - it('should block file paths in email template paths', () => { + it('should block file paths in email template paths', async () => { process.env[UM_EMAIL_TEMPLATES_INVITE] = '/path/to/invite/templates'; process.env[UM_EMAIL_TEMPLATES_PWRESET] = '/path/to/pwreset/templates'; const invitePath = '/path/to/invite/templates/invite.html'; const pwResetPath = '/path/to/pwreset/templates/reset.html'; - expect(isFilePathBlocked(invitePath)).toBe(true); - expect(isFilePathBlocked(pwResetPath)).toBe(true); + expect(await isFilePathBlocked(invitePath)).toBe(true); + expect(await isFilePathBlocked(pwResetPath)).toBe(true); }); - it('should block access to n8n files if restrict and block are set', () => { + it('should block access to n8n files if restrict and block are set', async () => { const homeVarName = process.platform === 'win32' ? 'USERPROFILE' : 'HOME'; const userHome = process.env.N8N_USER_FOLDER ?? process.env[homeVarName] ?? process.cwd(); process.env[RESTRICT_FILE_ACCESS_TO] = userHome; process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true'; const restrictedPath = instanceSettings.n8nFolder; - expect(isFilePathBlocked(restrictedPath)).toBe(true); + expect(await isFilePathBlocked(restrictedPath)).toBe(true); }); - it('should allow access to parent folder if restrict and block are set', () => { + it('should allow access to parent folder if restrict and block are set', async () => { const homeVarName = process.platform === 'win32' ? 'USERPROFILE' : 'HOME'; const userHome = process.env.N8N_USER_FOLDER ?? process.env[homeVarName] ?? process.cwd(); process.env[RESTRICT_FILE_ACCESS_TO] = userHome; process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true'; const restrictedPath = join(userHome, 'somefile.txt'); - expect(isFilePathBlocked(restrictedPath)).toBe(false); + expect(await isFilePathBlocked(restrictedPath)).toBe(false); }); - it('should not block similar paths', () => { + it('should not block similar paths', async () => { const homeVarName = process.platform === 'win32' ? 'USERPROFILE' : 'HOME'; const userHome = process.env.N8N_USER_FOLDER ?? process.env[homeVarName] ?? process.cwd(); process.env[RESTRICT_FILE_ACCESS_TO] = userHome; process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true'; const restrictedPath = join(userHome, '.n8n_x'); - expect(isFilePathBlocked(restrictedPath)).toBe(false); + expect(await isFilePathBlocked(restrictedPath)).toBe(false); + }); + + it('should return true for a symlink in a allowed path to a restricted path', async () => { + process.env[RESTRICT_FILE_ACCESS_TO] = '/path1'; + const allowedPath = '/path1/symlink'; + const actualPath = '/path2/realfile'; + (fsRealpath as jest.Mock).mockImplementation((path: string) => + path === allowedPath ? actualPath : path, + ); + expect(await isFilePathBlocked(allowedPath)).toBe(true); }); }); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts index e575b29845..60dc26460d 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts @@ -3,8 +3,11 @@ import { Container } from '@n8n/di'; import type { FileSystemHelperFunctions, INode } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; import { createReadStream } from 'node:fs'; -import { access as fsAccess, writeFile as fsWriteFile } from 'node:fs/promises'; -import { resolve } from 'node:path'; +import { + access as fsAccess, + writeFile as fsWriteFile, + realpath as fsRealpath, +} from 'node:fs/promises'; import { BINARY_DATA_STORAGE_PATH, @@ -29,9 +32,9 @@ const getAllowedPaths = () => { return allowedPaths; }; -export function isFilePathBlocked(filePath: string): boolean { +export async function isFilePathBlocked(filePath: string): Promise { const allowedPaths = getAllowedPaths(); - const resolvedFilePath = resolve(filePath); + const resolvedFilePath = await fsRealpath(filePath); const blockFileAccessToN8nFiles = process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] !== 'false'; const restrictedPaths = blockFileAccessToN8nFiles ? getN8nRestrictedPaths() : []; @@ -62,7 +65,7 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }) : error; } - if (isFilePathBlocked(filePath as string)) { + if (await isFilePathBlocked(filePath as string)) { const allowedPaths = getAllowedPaths(); const message = allowedPaths.length ? ` Allowed paths: ${allowedPaths.join(', ')}` : ''; throw new NodeOperationError(node, `Access to the file is not allowed.${message}`, { @@ -77,7 +80,7 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }, async writeContentToFile(filePath, content, flag) { - if (isFilePathBlocked(filePath as string)) { + if (await isFilePathBlocked(filePath as string)) { throw new NodeOperationError(node, `The file "${String(filePath)}" is not writable.`, { level: 'warning', }); diff --git a/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts b/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts index 966aeb69e7..b6a53e8d61 100644 --- a/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts +++ b/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts @@ -7,6 +7,8 @@ describe('Test Crypto Node', () => { jest.mock('fast-glob', () => async () => ['/test/binary.data']); jest.mock('fs/promises'); fsPromises.access = async () => {}; + const realpathSpy = jest.spyOn(fsPromises, 'realpath'); + realpathSpy.mockImplementation(async (path) => path as string); jest.mock('fs'); fs.createReadStream = () => Readable.from(Buffer.from('test')) as fs.ReadStream; diff --git a/packages/nodes-base/nodes/Files/ReadWriteFile/test/ReadWriteFile.test.ts b/packages/nodes-base/nodes/Files/ReadWriteFile/test/ReadWriteFile.test.ts index 0687b67e66..6d4c4677eb 100644 --- a/packages/nodes-base/nodes/Files/ReadWriteFile/test/ReadWriteFile.test.ts +++ b/packages/nodes-base/nodes/Files/ReadWriteFile/test/ReadWriteFile.test.ts @@ -1,4 +1,5 @@ import { NodeTestHarness } from '@nodes-testing/node-test-harness'; +import fsPromises from 'fs/promises'; import type { WorkflowTestData } from 'n8n-workflow'; describe('Test ReadWriteFile Node', () => { @@ -13,6 +14,9 @@ describe('Test ReadWriteFile Node', () => { const writeFileNode = workflowData.nodes.find((n) => n.name === 'Write to Disk')!; writeFileNode.parameters.fileName = `${testHarness.temporaryDir}/image-written.jpg`; + const realpathSpy = jest.spyOn(fsPromises, 'realpath'); + realpathSpy.mockImplementation(async (path) => path as string); + const tests: WorkflowTestData[] = [ { description: 'nodes/Files/ReadWriteFile/test/ReadWriteFile.workflow.json', diff --git a/packages/nodes-base/nodes/WriteBinaryFile/test/WriteBinaryFile.test.ts b/packages/nodes-base/nodes/WriteBinaryFile/test/WriteBinaryFile.test.ts index 7b93a09d88..a130bdaf5e 100644 --- a/packages/nodes-base/nodes/WriteBinaryFile/test/WriteBinaryFile.test.ts +++ b/packages/nodes-base/nodes/WriteBinaryFile/test/WriteBinaryFile.test.ts @@ -1,4 +1,5 @@ import { NodeTestHarness } from '@nodes-testing/node-test-harness'; +import fsPromises from 'fs/promises'; import type { WorkflowTestData } from 'n8n-workflow'; import path from 'path'; @@ -12,6 +13,9 @@ describe('Test Write Binary File Node', () => { const writeFilePath = path.join(testHarness.temporaryDir, 'image-written.jpg'); writeFileNode.parameters.fileName = writeFilePath; + const realpathSpy = jest.spyOn(fsPromises, 'realpath'); + realpathSpy.mockImplementation(async (path) => path as string); + const tests: WorkflowTestData[] = [ { description: 'nodes/WriteBinaryFile/test/WriteBinaryFile.workflow.json',