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

mfsiega · 2025-12-04T18:01:01Z

Summary

Make sure we only resolve filepaths once, so we know that when we read them they haven't been changed to link somewhere else.

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

codecov · 2025-12-04T18:01:21Z

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...tion-context/utils/file-system-helper-functions.ts	80.95%	3 Missing and 1 partial ⚠️
...des/Files/ReadWriteFile/actions/write.operation.ts	66.66%	0 Missing and 1 partial ⚠️
...base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts	66.66%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

cubic-dev-ai

1 issue found across 11 files

Prompt for AI agents (all 1 issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts">

<violation number="1" location="packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts:94">
P1: The try/catch block will not catch `ELOOP` errors because `createReadStream` opens files asynchronously. The error is emitted via the stream&#39;s `&#39;error&#39;` event, not thrown synchronously. You need to attach an error handler to the stream or use `fs.open()` with `O_NOFOLLOW` first, then create the stream from the fd.</violation>
</file>

_{Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR}

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts

currents-bot · 2025-12-04T18:18:12Z

✅

E2E Tests: n8n tests passed after 10m 9s

🟢 573 · 🔴 0 · ⚪️ 43 · 🟣 4

View Run Details

Run Details

Project: n8n
Groups: 2
Framework: Playwright
Run Status: Passed
Commit: 7140e57
Spec files: 129
Overall tests: 616
Duration: 10m 9s
Parallelization: 9

Groups

GroupId	Results	Spec Files Progress
multi-main:ui	🟢 519 · 🔴 0 · ⚪️ 43 · 🟣 3	120 / 120
multi-main:ui:isolated	🟢 54 · 🔴 0 · ⚪️ 0 · 🟣 1	9 / 9

This message was posted automatically by currents.dev | Integration Settings

…s/file-system-helper-functions.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

mfsiega · 2025-12-05T10:50:16Z

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts


 		try {
-			await fsAccess(filePath);
+			await fsAccess(resolvedFilePath);


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.

tomi · 2025-12-05T11:27:43Z

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts

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


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

tomi · 2025-12-05T11:28:24Z

packages/workflow/src/interfaces.ts

 	returnJsonArray(jsonData: IDataObject | IDataObject[]): INodeExecutionData[];
 }

+const __brand = Symbol('brand');


Or:

Suggested change

const __brand = Symbol('brand');

const __brand = Symbol('resolvedFilePath');

tomi · 2025-12-05T11:30:24Z

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts

+						new NodeOperationError(
+							node,
+							`The file "${String(resolvedFilePath)}" could not be opened.`,
+							{ level: 'warning' },


Could we attach the original error as cause?

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

tomi · 2025-12-05T11:33:26Z

packages/workflow/src/interfaces.ts

+	resolvePath(path: PathLike): Promise<ResolvedFilePath>;
+	isFilePathBlocked(filePath: ResolvedFilePath): boolean;
+	createReadStream(filePath: ResolvedFilePath): Promise<Readable>;
 	getStoragePath(): string;
 	writeContentToFile(


Could we add docs to all these functions with mention that "Use resolvePath to resolve the path"?

tomi · 2025-12-05T11:35:40Z

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts

+			);
 		}
-		return await fsWriteFile(filePath, content, { encoding: 'binary', flag });
+		return await fsWriteFile(resolvedFilePath, content, { encoding: 'binary', flag });


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

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

Cadiac

Seems like a smart way to solve this 👍

Cadiac · 2025-12-05T11:37:06Z

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts

+const resolvePath = async (path: PathLike): Promise<ResolvedFilePath> => {
 	try {
-		resolvedFilePath = await fsRealpath(filePath);
+		return (await fsRealpath(path)) as ResolvedFilePath; // apply brand, since we know it's resolved now


Branded types seem like a smart way to enforce this 👍

only resolve the filepath once

c3f6350

cubic-dev-ai bot reviewed Dec 4, 2025

View reviewed changes

packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts Outdated Show resolved Hide resolved

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Dec 4, 2025

mfsiega and others added 2 commits December 4, 2025 21:16

Update packages/core/src/execution-engine/node-execution-context/util…

5a7bf76

…s/file-system-helper-functions.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

fix lint

77d6244

This comment has been minimized.

Sign in to view

mfsiega and others added 5 commits December 4, 2025 21:46

fix unit tests

ea4923a

fix test typecheck

671881a

fix lint

ef47707

Merge branch 'master' into cat-1852-resolve-file-path-once

52be596

update test mocks

5dc9144

mfsiega force-pushed the cat-1852-resolve-file-path-once branch from 15a88a9 to 5dc9144 Compare December 5, 2025 10:07

mfsiega commented Dec 5, 2025

View reviewed changes

mfsiega requested a review from tomi December 5, 2025 10:50

tomi requested a review from Cadiac December 5, 2025 10:53

tomi reviewed Dec 5, 2025

View reviewed changes

Cadiac previously approved these changes Dec 5, 2025

View reviewed changes

prevent fs write helper from re-resolving

2e752b4

mfsiega dismissed Cadiac’s stale review via 2e752b4 December 5, 2025 16:00

This comment has been minimized.

Sign in to view

fix test expectation

7140e57

mfsiega requested review from Cadiac and tomi December 5, 2025 20:10

Cadiac approved these changes Dec 5, 2025

View reviewed changes

	const __brand = Symbol('brand');
	const __brand = Symbol('resolvedFilePath');

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

Are you sure you want to change the base?

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

Conversation

mfsiega commented Dec 4, 2025

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

Uh oh!

codecov bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

currents-bot bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadiac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 4, 2025 •

edited

Loading

currents-bot bot commented Dec 4, 2025 •

edited

Loading