fix(core): Restrict read/write file paths access #6582

michael-radency · 2023-07-03T12:00:31Z

Github issue / Community forum post (link here to close automatically):

github-actions · 2023-07-03T12:01:03Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Files matching packages/nodes-base/nodes/**:

Added workflow tests for nodes if possible.

Make sure to check off this list before asking for review.

codecov · 2023-07-03T12:14:59Z

Codecov Report

Patch coverage: 5.51% and project coverage change: -0.03% ⚠️

Comparison is base (50b0dc2) 24.64% compared to head (2cbde3e) 24.62%.
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6582      +/-   ##
==========================================
- Coverage   24.64%   24.62%   -0.03%     
==========================================
  Files        3097     3122      +25     
  Lines      190210   190509     +299     
  Branches    20918    20941      +23     
==========================================
+ Hits        46886    46907      +21     
- Misses     142400   142674     +274     
- Partials      924      928       +4

Files Changed	Coverage Δ
packages/cli/src/config/schema.ts	`37.50% <ø> (ø)`
...odes-base/credentials/AlienVaultApi.credentials.ts	`0.00% <0.00%> (ø)`
...base/credentials/Auth0ManagementApi.credentials.ts	`0.00% <0.00%> (ø)`
...des-base/credentials/CarbonBlackApi.credentials.ts	`0.00% <0.00%> (ø)`
...des-base/credentials/CiscoMerakiApi.credentials.ts	`0.00% <0.00%> (ø)`
.../credentials/CiscoSecureEndpointApi.credentials.ts	`0.00% <0.00%> (ø)`
...s-base/credentials/CiscoUmbrellaApi.credentials.ts	`0.00% <0.00%> (ø)`
...ase/credentials/CiscoWebexOAuth2Api.credentials.ts	`0.00% <0.00%> (ø)`
...se/credentials/CrowdStrikeOAuth2Api.credentials.ts	`0.00% <0.00%> (ø)`
...s/nodes-base/credentials/F5BigIpApi.credentials.ts	`0.00% <0.00%> (ø)`
... and 27 more

... and 88 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…v03-dangerous-feature

netroy

some of the proposed changes in 351670f

netroy · 2023-07-04T17:48:43Z

packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts

@@ -67,6 +71,8 @@ export class ReadBinaryFile implements INodeType {
 				}

 				const filePath = this.getNodeParameter('filePath', itemIndex);
+				checkFilePathAccess(filePath);


I think we should not be adding these checks in the node, but in NodeExecuteFunctions instead.

before reading or writing, we should convert the filepath to an absolute path, and then check if that absolute path is not inside any of the blocked paths.

for the read nodes, we should update createReadStream here.

for write node, we should add a new helper method to FileSystemHelperFunctions called writeContentToFile, and add the checks in there

The idea is that these variables should be user-defined.
By making them utilities, we can use them only in specific cases like "readBinary" and "writeBinary," where user input is expected.
This approach keeps the internal logic intact. Otherwise, we might eventually need to find ways to bypass this added restriction in future development.

netroy · 2023-07-04T17:51:19Z

packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts

@@ -21,6 +24,7 @@ export class ReadBinaryFile implements INodeType {
 		inputs: ['main'],
 		outputs: ['main'],
 		properties: [
+			allowedPathsNotice,


IMO, we should not be adding these notices. It's a bit like how forgot-password forms should not say if an email was in the system or not.

I do not think this reasoning is applicable here.
N8N_RESTRICT_FILE_ACCESS_TO allows to restrict access to specific folders only e.g. '/home/user/data;/home/user/documents', all other directories would be forbidden
I think that it's reasonable to let user know what location are available for him to read/write

I haven't thought much about this, but could we not define a base path for reading/writing binary files from/to? Users could then enter a relative path in their binary file nodes. Or they could enter an absolute path. In both cases, access would only be granted if the resulting path is in compliance with N8N_RESTRICT_FILE_ACCESS_TO.

Resolving paths would be somewhat confusing as we basically forbid, in this case, usage of relative paths regarding file system. If we hard code base path for reading/writing we would break functionality for some existing use cases, if we would allow user to define base path it would duplicate N8N_RESTRICT_FILE_ACCESS_TO

netroy · 2023-07-04T17:52:13Z

packages/nodes-base/nodes/ReadBinaryFiles/ReadBinaryFiles.node.ts

@@ -47,8 +53,17 @@ export class ReadBinaryFiles implements INodeType {

 		const files = await glob(fileSelector);

+		if (files.length === 0) {


do we need this new check?

We probably should add option for this, like "Throw error if any matches found", as error in some cases would be preferable/correct response instead empty array

Lets remove this check since watching temporarly empty folders is a valid use case and adding an option is out of scope.

netroy · 2023-07-04T17:57:06Z

packages/cli/src/config/schema.ts

+			default: '',
+			env: 'N8N_RESTRICT_FILE_ACCESS_TO',
+		},
+		blockFileAccessToN8nFiles: {


this should not be optional. we should always block access to .n8n folder

I do agree regarding .n8n folder and we should add this to NodeExecuteFunctions as in proposed changes commit, but as stated in specs it also forbid access to some user defined config/templates files, and this part, I think, should be optional.

michael-radency · 2023-07-05T05:17:50Z

some of the proposed changes in 351670f

logic is somewhat inverted, N8N_RESTRICT_FILE_ACCESS_TO contains paths with allowed access to, forbidding access to all others

…v03-dangerous-feature

…09a1a16d907d9b1765e

…v03-dangerous-feature

…LE_ACCESS_TO

cypress · 2023-07-31T12:00:27Z

1 flaky tests on run #1684 ↗︎

0	219	0	0	1

Details:

🌳 n8n-6490-v03-dangerous-feature 🖥️ browsers:node18.12.0-chrome107 🤖 michael-...
Project: n8n	Commit: `2cbde3e9cf`
Status: Passed	Duration: 07:33 💡
Started: Jul 31, 2023 12:11 PM	Ended: Jul 31, 2023 12:19 PM

cypress/e2e/2-credentials.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Credentials > should create a new credential using Add Credential button		`Output` `Screenshots` `Video`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-07-31T12:00:31Z

✅ All Cypress E2E specs passed

netroy

In the long run, we should aim to remove all filesystem access across all nodes, and have all FS requests go through NodeExecuteFunctions, just like we do for http requests.
This also enables us in the future to run such nodes in v8 isolates, completely isolated from any I/O access.

github-actions · 2023-07-31T12:19:21Z

✅ All Cypress E2E specs passed

janober · 2023-08-09T17:48:21Z

Got released with n8n@1.2.1

# [0.237.0](https://github.com/n8n-io/n8n/compare/n8n@0.236.3...n8n@0.237.0) (2023-08-17) ### Bug Fixes * **API:** Fix issue with workflow setting not supporting newer nanoids ([#6699](#6699)) ([2e64c9d](2e64c9d)) * **AwsS3 Node:** Fix issue if bucket name contains a '.' ([#6542](#6542)) ([219f897](219f897)) * **core:** Add missing primary key on the `execution_data` table on postgres ([#6797](#6797)) ([1c5f98d](1c5f98d)) * **core:** Fix `continueOnFail` for expression error in Set ([#6939](#6939)) ([2a3937f](2a3937f)) * **core:** Fix property existence checks on AugmentObject ([#6842](#6842)) ([3e4483a](3e4483a)) * **core:** Log crash causes to console when sentry is disabled ([#6890](#6890)) ([f19c9c1](f19c9c1)) * **core:** Restrict read/write file paths access ([#6582](#6582)) ([6e6b90e](6e6b90e)) * **core:** Serialize BigInts ([#6805](#6805)) ([17ce49b](17ce49b)) * **core:** Update frontend urls when using the `--tunnel` option ([#6898](#6898)) ([ec3c066](ec3c066)) * **core:** Update packages to address CVE-2023-2142 and CVE-2020-28469 ([#6844](#6844)) ([6dd0850](6dd0850)) * Correct typos in Taiga and ServiceNow nodes ([#6814](#6814)) ([b9f52ec](b9f52ec)) * **editor:** Fix code node’s content property to be reactive ([#6931](#6931)) ([f3b380d](f3b380d)) * **editor:** Prevent Code node linter from erroring on `null` parse ([#6934](#6934)) ([baada92](baada92)) * **Email Trigger (IMAP) Node:** UTF-8 attachments are not correctly named ([#6856](#6856)) ([f3f1c14](f3f1c14)) * **Email Trigger (IMAP) Node:** Fix connection issue with unexpected spaces in host ([#6886](#6886)) ([35c29ac](35c29ac)) * Fix issue with key based credentials not being read correctly ([#6824](#6824)) ([2459b0a](2459b0a)) * Fix issue with key formatting if null or undefined ([#6924](#6924)) ([59d33db](59d33db)) * Fix issue with key formatting introduced in 1.2.0 ([#6896](#6896)) ([8423153](8423153)) * **FTP Node:** List recursive ignore . and .. to prevent infinite loops ([#6707](#6707)) ([6a787dd](6a787dd)) * **GitLab Trigger Node:** Fix trigger activation 404 error ([#6711](#6711)) ([8c5145b](8c5145b)) * **Gmail Trigger Node:** Early returns in case of no data ([#6727](#6727)) ([df2f1d6](df2f1d6)) * **Google BigQuery Node:** Error description improvement ([#6715](#6715)) ([261d73e](261d73e)) * **GoToWebinar Node:** Fix issue with timezone incorrectly being required ([#6865](#6865)) ([2f61e34](2f61e34)) * **HTTP Request Node:** Improve error handling for TCP socket errors when `Continue On Fail` is enabled ([#6925](#6925)) ([e8e4c9e](e8e4c9e)) * **Lemlist Node:** Fix pagination issues with campaigns and activities ([#6734](#6734)) ([73eddc6](73eddc6)) * **Salesforce Node:** Fix Account update owner operation ([#6958](#6958)) ([e71ff76](e71ff76)) * **Telegram Trigger Node:** Add guard to 'include' call on null or undefined ([#6730](#6730)) ([c3c845c](c3c845c)) * **Todoist Node:** Fix issue with section id being ignored ([#6799](#6799)) ([e53f4a0](e53f4a0)) ### Features * Add support for not requiring SMTP auth with user management ([#3742](#3742)) ([fc97d18](fc97d18)) * **core:** Add support for ready hooks, and credentials overwrite endpoint in workers ([#6954](#6954)) ([e5986c5](e5986c5)) * **editor:** Add "Download" button if JSON data is to large ([#6850](#6850)) ([3d815cf](3d815cf)) * Enable parallel processing on multiple queue nodes ([#6295](#6295)) ([e45461a](e45461a)) Co-authored-by: netroy <netroy@users.noreply.github.com>

⚡ fix

1c3e435

michael-radency added n8n team Authored by the n8n team security labels Jul 3, 2023

michael-radency added 5 commits July 3, 2023 22:02

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

51e2ab0

…v03-dangerous-feature

⚡ clean up, allowed paths notice

354ac8f

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

5e0c57e

…v03-dangerous-feature

⚡ resolve relative paths

7e665e5

⚡ added schema entries

00a2246

netroy requested changes Jul 4, 2023

View reviewed changes

maspio and others added 11 commits July 18, 2023 17:21

Fix user n8n folder to work on windows

0c70615

removed ReadBinaryFiles throwing an error if no files

d1adab5

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

feddafb

…v03-dangerous-feature

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

3f9270e

…v03-dangerous-feature

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

ad8647a

…v03-dangerous-feature

⚡ based on 351670f#diff-704ca68b40723f3efef34ba1c79c85745f0135020fe93…

033d28d

…09a1a16d907d9b1765e

⚡ clean up

03a9852

Merge branch 'master' of https://github.com/n8n-io/n8n into n8n-6490-…

a56a4af

…v03-dangerous-feature

respect BLOCK_FILE_ACCESS_TO_N8N_FILES to disable blocking .n8n access

58780c0

improve BLOCK_FILE_ACCESS_TO_N8N_FILES to check after N8N_RESTRICT_FI…

05c925e

…LE_ACCESS_TO

fix typo

2cbde3e

maspio approved these changes Jul 31, 2023

View reviewed changes

michael-radency requested a review from netroy July 31, 2023 12:07

netroy approved these changes Jul 31, 2023

View reviewed changes

michael-radency merged commit f6bf9e9 into master Jul 31, 2023
60 of 61 checks passed

michael-radency deleted the n8n-6490-v03-dangerous-feature branch July 31, 2023 12:20

github-actions bot mentioned this pull request Aug 9, 2023

🚀 Release 1.2.0 #6891

Merged

janober added the Released label Aug 9, 2023

netroy pushed a commit that referenced this pull request Aug 17, 2023

fix(core): Restrict read/write file paths access (#6582)

fd3a196

github-actions bot mentioned this pull request Aug 17, 2023

🚀 Release 0.236.4 #6963

Closed

netroy pushed a commit that referenced this pull request Aug 17, 2023

fix(core): Restrict read/write file paths access (#6582)

6e6b90e

github-actions bot mentioned this pull request Aug 17, 2023

🚀 Release 0.237.0 #6965

Merged

netroy mentioned this pull request Aug 26, 2023

Read Binary File Node fails in ver 1.3.1 #7024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Restrict read/write file paths access #6582

fix(core): Restrict read/write file paths access #6582

michael-radency commented Jul 3, 2023

github-actions bot commented Jul 3, 2023

codecov bot commented Jul 3, 2023 •

edited

Loading

netroy left a comment

netroy Jul 4, 2023

michael-radency Jul 5, 2023

netroy Jul 4, 2023

michael-radency Jul 5, 2023

csuermann Jul 5, 2023

michael-radency Jul 5, 2023

netroy Jul 4, 2023

michael-radency Jul 5, 2023

maspio Jul 18, 2023

netroy Jul 4, 2023

michael-radency Jul 5, 2023 •

edited

Loading

michael-radency commented Jul 5, 2023

cypress bot commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Jul 31, 2023

netroy left a comment

github-actions bot commented Jul 31, 2023

janober commented Aug 9, 2023

		@@ -47,8 +53,17 @@ export class ReadBinaryFiles implements INodeType {

		const files = await glob(fileSelector);

		if (files.length === 0) {

fix(core): Restrict read/write file paths access #6582

fix(core): Restrict read/write file paths access #6582

Conversation

michael-radency commented Jul 3, 2023

github-actions bot commented Jul 3, 2023

codecov bot commented Jul 3, 2023 • edited Loading

Codecov Report

netroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-radency Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

michael-radency commented Jul 5, 2023

cypress bot commented Jul 31, 2023 • edited Loading

1 flaky tests on run #1684 ↗︎

github-actions bot commented Jul 31, 2023

netroy left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2023

janober commented Aug 9, 2023

codecov bot commented Jul 3, 2023 •

edited

Loading

michael-radency Jul 5, 2023 •

edited

Loading

cypress bot commented Jul 31, 2023 •

edited

Loading