Skip to content

fix(core): Only resolve the filepath once #22767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mfsiega merged 12 commits into from
Dec 8, 2025
Merged

fix(core): Only resolve the filepath once #22767

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c3f6350
only resolve the filepath once
mfsiega Dec 2, 2025
5a7bf76
Update packages/core/src/execution-engine/node-execution-context/util…
mfsiega Dec 4, 2025
77d6244
fix lint
mfsiega Dec 4, 2025
ea4923a
fix unit tests
mfsiega Dec 4, 2025
671881a
fix test typecheck
mfsiega Dec 4, 2025
ef47707
fix lint
mfsiega Dec 4, 2025
52be596
Merge branch 'master' into cat-1852-resolve-file-path-once
mfsiega Dec 4, 2025
5dc9144
update test mocks
mfsiega Dec 5, 2025
2e752b4
prevent fs write helper from re-resolving
mfsiega Dec 5, 2025
7140e57
fix test expectation
mfsiega Dec 5, 2025
1fdf078
Merge branch 'master' into cat-1852-resolve-file-path-once
mfsiega Dec 8, 2025
b8df2f6
add docs to resolvePath
mfsiega Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 84 additions & 35 deletions ...cution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from '@/constants';
import { InstanceSettings } from '@/instance-settings';

import { getFileSystemHelperFunctions, isFilePathBlocked } from '../file-system-helper-functions';
import { getFileSystemHelperFunctions } from '../file-system-helper-functions';

jest.mock('node:fs');
jest.mock('node:fs/promises');
Expand All @@ -39,78 +39,80 @@ beforeEach(() => {
});

describe('isFilePathBlocked', () => {
const node = { type: 'TestNode' } as INode;
const { isFilePathBlocked, resolvePath } = getFileSystemHelperFunctions(node);
beforeEach(() => {
process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
});

it('should return true for static cache dir', async () => {
const filePath = instanceSettings.staticCacheDir;
expect(await isFilePathBlocked(filePath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(filePath))).toBe(true);
});

it('should return true for restricted paths', async () => {
const restrictedPath = instanceSettings.n8nFolder;
expect(await isFilePathBlocked(restrictedPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
});

it('should handle empty allowed paths', async () => {
securityConfig.restrictFileAccessTo = '';
const result = await isFilePathBlocked('/some/random/path');
const result = isFilePathBlocked(await resolvePath('/some/random/path'));
expect(result).toBe(false);
});

it('should handle multiple allowed paths', async () => {
securityConfig.restrictFileAccessTo = '/path1;/path2;/path3';
const allowedPath = '/path2/somefile';
expect(await isFilePathBlocked(allowedPath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
});

it('should handle empty strings in allowed paths', async () => {
securityConfig.restrictFileAccessTo = '/path1;;/path2';
const allowedPath = '/path2/somefile';
expect(await isFilePathBlocked(allowedPath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
});

it('should trim whitespace in allowed paths', async () => {
securityConfig.restrictFileAccessTo = ' /path1 ; /path2 ; /path3 ';
const allowedPath = '/path2/somefile';
expect(await isFilePathBlocked(allowedPath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(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(await isFilePathBlocked(restrictedPath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(false);
});

it('should return true when path is in allowed paths but still restricted', async () => {
securityConfig.restrictFileAccessTo = '/some/allowed/path';
const restrictedPath = instanceSettings.n8nFolder;
expect(await isFilePathBlocked(restrictedPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
});

it('should return false when path is in allowed paths', async () => {
const allowedPath = '/some/allowed/path';
securityConfig.restrictFileAccessTo = allowedPath;
expect(await isFilePathBlocked(allowedPath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
});

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(await isFilePathBlocked(configPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(configPath))).toBe(true);
});

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(await isFilePathBlocked(extensionPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(extensionPath))).toBe(true);
});

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(await isFilePathBlocked(binaryPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(binaryPath))).toBe(true);
});

it('should block file paths in email template paths', async () => {
Expand All @@ -120,8 +122,8 @@ describe('isFilePathBlocked', () => {
const invitePath = '/path/to/invite/templates/invite.html';
const pwResetPath = '/path/to/pwreset/templates/reset.html';

expect(await isFilePathBlocked(invitePath)).toBe(true);
expect(await isFilePathBlocked(pwResetPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(invitePath))).toBe(true);
expect(isFilePathBlocked(await resolvePath(pwResetPath))).toBe(true);
});

it('should block access to n8n files if restrict and block are set', async () => {
Expand All @@ -131,7 +133,7 @@ describe('isFilePathBlocked', () => {
securityConfig.restrictFileAccessTo = userHome;
process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
const restrictedPath = instanceSettings.n8nFolder;
expect(await isFilePathBlocked(restrictedPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
});

it('should allow access to parent folder if restrict and block are set', async () => {
Expand All @@ -140,8 +142,8 @@ describe('isFilePathBlocked', () => {

securityConfig.restrictFileAccessTo = userHome;
process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
const restrictedPath = join(userHome, 'somefile.txt');
expect(await isFilePathBlocked(restrictedPath)).toBe(false);
const restrictedPath = await resolvePath(join(userHome, 'somefile.txt'));
expect(isFilePathBlocked(restrictedPath)).toBe(false);
});

it('should not block similar paths', async () => {
Expand All @@ -150,8 +152,8 @@ describe('isFilePathBlocked', () => {

securityConfig.restrictFileAccessTo = userHome;
process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
const restrictedPath = join(userHome, '.n8n_x');
expect(await isFilePathBlocked(restrictedPath)).toBe(false);
const restrictedPath = await resolvePath(join(userHome, '.n8n_x'));
expect(isFilePathBlocked(restrictedPath)).toBe(false);
});

it('should return true for a symlink in a allowed path to a restricted path', async () => {
Expand All @@ -161,7 +163,7 @@ describe('isFilePathBlocked', () => {
(fsRealpath as jest.Mock).mockImplementation((path: string) =>
path === allowedPath ? actualPath : path,
);
expect(await isFilePathBlocked(allowedPath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(true);
});

it('should handle non-existent file when it is allowed', async () => {
Expand All @@ -170,7 +172,7 @@ describe('isFilePathBlocked', () => {
// @ts-expect-error undefined property
error.code = 'ENOENT';
(fsRealpath as jest.Mock).mockRejectedValueOnce(error);
expect(await isFilePathBlocked(filePath)).toBe(false);
expect(isFilePathBlocked(await resolvePath(filePath))).toBe(false);
});

it('should handle non-existent file when it is not allowed', async () => {
Expand All @@ -181,7 +183,7 @@ describe('isFilePathBlocked', () => {
// @ts-expect-error undefined property
error.code = 'ENOENT';
(fsRealpath as jest.Mock).mockRejectedValueOnce(error);
expect(await isFilePathBlocked(filePath)).toBe(true);
expect(isFilePathBlocked(await resolvePath(filePath))).toBe(true);
});
});

Expand Down Expand Up @@ -213,17 +215,17 @@ describe('getFileSystemHelperFunctions', () => {
error.code = 'ENOENT';
(fsAccess as jest.Mock).mockRejectedValueOnce(error);

await expect(helperFunctions.createReadStream(filePath)).rejects.toThrow(
`The file "${filePath}" could not be accessed.`,
);
await expect(
helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)),
).rejects.toThrow(`The file "${filePath}" could not be accessed.`);
});

it('should throw when file access is blocked', async () => {
securityConfig.restrictFileAccessTo = '/allowed/path';
(fsAccess as jest.Mock).mockResolvedValueOnce({});
await expect(helperFunctions.createReadStream('/blocked/path')).rejects.toThrow(
'Access to the file is not allowed',
);
await expect(
helperFunctions.createReadStream(await helperFunctions.resolvePath('/blocked/path')),
).rejects.toThrow('Access to the file is not allowed');
});

it('should not reveal if file exists if it is within restricted path', async () => {
Expand All @@ -234,16 +236,63 @@ describe('getFileSystemHelperFunctions', () => {
error.code = 'ENOENT';
(fsAccess as jest.Mock).mockRejectedValueOnce(error);

await expect(helperFunctions.createReadStream('/blocked/path')).rejects.toThrow(
'Access to the file is not allowed',
);
await expect(
helperFunctions.createReadStream(await helperFunctions.resolvePath('/blocked/path')),
).rejects.toThrow('Access to the file is not allowed');
});

it('should create a read stream if file access is permitted', async () => {
const filePath = '/allowed/path';
(fsAccess as jest.Mock).mockResolvedValueOnce({});
await helperFunctions.createReadStream(filePath);
expect(createReadStream).toHaveBeenCalledWith(filePath);

// Mock createReadStream to return a proper stream-like object
const mockStream: { once: jest.Mock } = {
once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => {
if (event === 'open') {
// Immediately call the open callback
setImmediate(() => callback());
}
return mockStream;
}),
};
(createReadStream as jest.Mock).mockReturnValueOnce(mockStream);

await helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath));
expect(createReadStream).toHaveBeenCalledWith(
filePath,
expect.objectContaining({
flags: expect.any(Number),
}),
);
});

it('should reject symlinks with O_NOFOLLOW to prevent TOCTOU attacks', async () => {
const filePath = '/allowed/path/file';

// Clear previous mocks and set up fresh mocks
(fsAccess as jest.Mock).mockReset();
(fsAccess as jest.Mock).mockResolvedValue(undefined);

// Simulate the ELOOP error that occurs when O_NOFOLLOW encounters a symlink
const eloopError = new Error('ELOOP: too many symbolic links encountered');
// @ts-expect-error undefined property
eloopError.code = 'ELOOP';

// Mock createReadStream to return a stream that emits an error event
const mockStream: { once: jest.Mock } = {
once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => {
if (event === 'error') {
// Emit the error asynchronously
setImmediate(() => callback(eloopError));
}
return mockStream;
}),
};
(createReadStream as jest.Mock).mockReturnValueOnce(mockStream);

await expect(
helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)),
).rejects.toThrow('could not be opened');
});
});

Expand All @@ -253,7 +302,7 @@ describe('getFileSystemHelperFunctions', () => {

await expect(
helperFunctions.writeContentToFile(
instanceSettings.n8nFolder + '/test.txt',
await helperFunctions.resolvePath(instanceSettings.n8nFolder + '/test.txt'),
'content',
'w',
),
Expand Down
69 changes: 48 additions & 21 deletions ...es/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { isContainedWithin, safeJoinPath } from '@n8n/backend-common';
import { SecurityConfig } from '@n8n/config';
import { Container } from '@n8n/di';
import type { FileSystemHelperFunctions, INode } from 'n8n-workflow';
import { NodeOperationError } from 'n8n-workflow';
import { createReadStream } from 'node:fs';
import type { FileSystemHelperFunctions, INode, ResolvedFilePath } from 'n8n-workflow';
import type { PathLike } from 'node:fs';
import { constants, createReadStream } from 'node:fs';
import {
access as fsAccess,
writeFile as fsWriteFile,
Expand Down Expand Up @@ -35,18 +36,19 @@ const getAllowedPaths = () => {
return allowedPaths;
};

export async function isFilePathBlocked(filePath: string): Promise<boolean> {
const allowedPaths = getAllowedPaths();
let resolvedFilePath = '';
const resolvePath = async (path: PathLike): Promise<ResolvedFilePath> => {
Copy link
Collaborator

@tomi tomi Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Rest of the functions in this file have been defined with function. Maybe we could keep it consistent?

Copy link
Contributor Author

@mfsiega mfsiega Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

try {
resolvedFilePath = await fsRealpath(filePath);
return (await fsRealpath(path)) as ResolvedFilePath; // apply brand, since we know it's resolved now
Copy link
Contributor

@Cadiac Cadiac Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Branded types seem like a smart way to enforce this 👍

} catch (error: unknown) {
if (error instanceof Error && 'code' in error && error.code === 'ENOENT') {
resolvedFilePath = resolve(filePath);
} else {
throw error;
return resolve(path.toString()) as ResolvedFilePath; // apply brand, since we know it's resolved now
}
throw error;
}
};

function isFilePathBlocked(resolvedFilePath: ResolvedFilePath): boolean {
const allowedPaths = getAllowedPaths();
const blockFileAccessToN8nFiles = process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] !== 'false';

const restrictedPaths = blockFileAccessToN8nFiles ? getN8nRestrictedPaths() : [];
Expand All @@ -64,8 +66,8 @@ export async function isFilePathBlocked(filePath: string): Promise<boolean> {
}

export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunctions => ({
async createReadStream(filePath) {
if (await isFilePathBlocked(filePath.toString())) {
async createReadStream(resolvedFilePath) {
if (isFilePathBlocked(resolvedFilePath)) {
const allowedPaths = getAllowedPaths();
const message = allowedPaths.length ? ` Allowed paths: ${allowedPaths.join(', ')}` : '';
throw new NodeOperationError(node, `Access to the file is not allowed.${message}`, {
Expand All @@ -74,34 +76,59 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct
}

try {
await fsAccess(filePath);
await fsAccess(resolvedFilePath);
Copy link
Contributor Author

@mfsiega mfsiega Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: the NodeJS docs say: Using fsPromises.access() to check for the accessibility of a file before calling fsPromises.open() is not recommended.. Rather you should just try to open the file, and handle any potential failure. We should think about fixing this, potentially in this PR; but it's a bit orthogonal to the issue at hand.

} catch (error) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
throw error.code === 'ENOENT'
? // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
new NodeOperationError(node, error, {
message: `The file "${String(filePath)}" could not be accessed.`,
message: `The file "${String(resolvedFilePath)}" could not be accessed.`,
level: 'warning',
})
: error;
}

return createReadStream(filePath);
// Use O_NOFOLLOW to prevent createReadStream from following symlinks. We require that the path
// already be resolved beforehand.
const stream = createReadStream(resolvedFilePath, {
flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string,
});

return await new Promise<ReturnType<typeof createReadStream>>((resolve, reject) => {
stream.once('error', (error) => {
if ((error as NodeJS.ErrnoException).code === 'ELOOP') {
reject(
new NodeOperationError(
node,
`The file "${String(resolvedFilePath)}" could not be opened.`,
{ level: 'warning' },
Copy link
Collaborator

@tomi tomi Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we attach the original error as cause?

Copy link
Contributor Author

@mfsiega mfsiega Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (I think this is what you meant?)

),
);
} else {
reject(error);
}
});
stream.once('open', () => resolve(stream));
});
},

getStoragePath() {
return safeJoinPath(Container.get(InstanceSettings).n8nFolder, `storage/${node.type}`);
},

async writeContentToFile(filePath, content, flag) {
if (await isFilePathBlocked(filePath as string)) {
throw new NodeOperationError(node, `The file "${String(filePath)}" is not writable.`, {
level: 'warning',
});
async writeContentToFile(resolvedFilePath, content, flag) {
if (isFilePathBlocked(resolvedFilePath)) {
throw new NodeOperationError(
node,
`The file "${String(resolvedFilePath)}" is not writable.`,
{
level: 'warning',
},
);
}
return await fsWriteFile(filePath, content, { encoding: 'binary', flag });
return await fsWriteFile(resolvedFilePath, content, { encoding: 'binary', flag });
Copy link
Collaborator

@tomi tomi Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this still have the issue? Can we make this so that fsWriteFile doesn't follow symlinks?

Copy link
Contributor Author

@mfsiega mfsiega Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! I think I'm handling the flags correctly here.

},

resolvePath,
isFilePathBlocked,
});

Expand Down
Loading
Loading