src,permission: add multiple allow-fs-* flags #49047

Ceres6 · 2023-08-06T21:18:13Z

Breaking change: Support for a single comma separates list for allow-fs-* flags is removed.

This means that

--alow-fs-read=/path/to/file.txt,/other/file/path.txt

Will be interpreted as a single file.

When using a single flag and including commas in said flag a warning will be emitted explaining the change.

Instead now multiple flags can be passed to allow multiple paths.

--allow-fs-read=/path/to/file.txt --allow-fs-read=/other/file/path.txt

Will allow access to both paths.

Fixes: nodejs/security-wg#1039

nodejs-github-bot · 2023-08-06T21:18:17Z

Review requested:

@nodejs/security-wg

RafaelGSS

Can we have a test for the warning too?

doc/api/cli.md

lib/internal/process/pre_execution.js

src/node_options.h

Ceres6 · 2023-08-10T15:40:33Z

Can we have a test for the warning too?

@RafaelGSS We do have one https://github.com/nodejs/node/pull/49047/files#diff-5928310ee410bbccf3599140510e07ca18546548955f67505349fa1fef2d6b48R83

richardlau

Possibly add changes metadata to the YAML blocks? e.g.

changes:
  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/49047
    description: Paths delimited by comma (`,`) are no longer allowed.

richardlau · 2023-08-10T15:50:15Z

doc/api/cli.md

+* Multiple paths can be allowed using multiple `--allow-fs-read` flags.
+  Example `--allow-fs-read=/folder1/ --allow-fs-read=/folder1/`
+
+NOTE: Paths delimited by comma (`,`) are no longer allowed.


This could also be added as changes metadata in the YAML block above.

richardlau · 2023-08-10T15:50:38Z

doc/api/cli.md

+* Multiple paths can be allowed using multiple `--allow-fs-read` flags.
+  Example `--allow-fs-read=/folder1/ --allow-fs-read=/folder1/`
+
+Paths delimited by comma (`,`) are no longer allowed.


This could also be added as changes metadata in the YAML block above.

nodejs-github-bot · 2023-08-11T11:57:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53186/

RafaelGSS

One import left

src/permission/fs_permission.cc

nodejs-github-bot · 2023-08-11T21:01:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53210/

RafaelGSS · 2023-08-11T21:35:58Z

It seems the machines are broken. I'll wait a bit to request another CI.

nodejs-github-bot · 2023-08-12T15:17:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53235/

Support for a single comma separates list for allow-fs-* flags is removed. Instead now multiple flags can be passed to allow multiple paths. Fixes: nodejs/security-wg#1039

Co-authored-by: Rafael Gonzaga <rafael.nunu@hotmail.com>

Notable changes: crypto: * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341 doc: * move and rename loaders section (Geoffrey Booth) #49261 * add release key for Ulises Gascon (Ulises Gascón) #49196 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 src: * support multiple `--env-file` declarations (Yagiz Nizipli) #49542 src,permission: * add multiple allow-fs-* flags (Carlos Espa) #49047 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49592

Notable changes: crypto: * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341 deps: * upgrade npm to 10.0.0 (npm team) #49423 * upgrade npm to 10.1.0 (npm team) #49570 doc: * move and rename loaders section (Geoffrey Booth) #49261 * add release key for Ulises Gascon (Ulises Gascón) #49196 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 src: * support multiple `--env-file` declarations (Yagiz Nizipli) #49542 src,permission: * add multiple allow-fs-* flags (Carlos Espa) #49047 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49592

Notable changes: crypto: * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341 deps: * upgrade npm to 10.1.0 (npm team) #49570 * upgrade npm to 10.0.0 (npm team) #49423 doc: * move and rename loaders section (Geoffrey Booth) #49261 * add release key for Ulises Gascon (Ulises Gascón) #49196 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 src: * support multiple `--env-file` declarations (Yagiz Nizipli) #49542 src,permission: * add multiple allow-fs-* flags (Carlos Espa) #49047 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49592

Notable changes: crypto: * update root certificates to NSS 3.93 (Node.js GitHub Bot) nodejs#49341 deps: * upgrade npm to 10.1.0 (npm team) nodejs#49570 * upgrade npm to 10.0.0 (npm team) nodejs#49423 doc: * move and rename loaders section (Geoffrey Booth) nodejs#49261 * add release key for Ulises Gascon (Ulises Gascón) nodejs#49196 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs#46391 src: * support multiple `--env-file` declarations (Yagiz Nizipli) nodejs#49542 src,permission: * add multiple allow-fs-* flags (Carlos Espa) nodejs#49047 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs#48975 PR-URL: nodejs#49592

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a7918, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: nodejs#48491 Refs: nodejs#49047

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a7918, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: #48491 Refs: #49047 PR-URL: #50662 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: nodejs#49047