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

doc/api/cli.md

@@ -145,6 +145,10 @@ Error: Access to this API has been restricted
 
 <!-- YAML
 added: v20.0.0
+changes:
+ - version: REPLACEME
+ pr-url: https://github.com/nodejs/node/pull/49047
+ description: Paths delimited by comma (`,`) are no longer allowed.
 -->
 
 > Stability: 1 - Experimental
@@ -155,8 +159,11 @@ the [Permission Model][].
 The valid arguments for the `--allow-fs-read` flag are:
 
 * `*` - To allow all `FileSystemRead` operations.
-* Paths delimited by comma (`,`) to allow only matching `FileSystemRead`
- operations.
+* 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.
+When passing a single flag with a comma a warning will be diplayed
 
 Examples can be found in the [File System Permissions][] documentation.
 
@@ -192,6 +199,10 @@ node --experimental-permission --allow-fs-read=/path/to/index.js index.js
 
 <!-- YAML
 added: v20.0.0
+changes:
+ - version: REPLACEME
+ pr-url: https://github.com/nodejs/node/pull/49047
+ description: Paths delimited by comma (`,`) are no longer allowed.
 -->
 
 > Stability: 1 - Experimental
@@ -202,8 +213,11 @@ the [Permission Model][].
 The valid arguments for the `--allow-fs-write` flag are:
 
 * `*` - To allow all `FileSystemWrite` operations.
-* Paths delimited by comma (`,`) to allow only matching `FileSystemWrite`
- operations.
+* 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.
+When passing a single flag with a comma a warning will be diplayed
 
 Examples can be found in the [File System Permissions][] documentation.
 

doc/api/permissions.md

@@ -532,7 +532,7 @@ Example:
 * `--allow-fs-write=*` - It will allow all `FileSystemWrite` operations.
 * `--allow-fs-write=/tmp/` - It will allow `FileSystemWrite` access to the `/tmp/`
  folder.
-* `--allow-fs-read=/tmp/,/home/.gitignore` - It allows `FileSystemRead` access
+* `--allow-fs-read=/tmp/ --allow-fs-read=/home/.gitignore` - It allows `FileSystemRead` access
  to the `/tmp/` folder **and** the `/home/.gitignore` path.
 
 Wildcards are supported too:

lib/internal/process/pre_execution.js

@@ -554,6 +554,22 @@ function initializePermission() {
 'It could invalidate the permission model.','SecurityWarning');
 }
 }
+constwarnCommaFlags=[
+'--allow-fs-read',
+'--allow-fs-write',
+];
+for(constflagofwarnCommaFlags){
+constvalue=getOptionValue(flag);
+if(value.length===1&&value[0].includes(',')){
+process.emitWarning(
+`The ${flag} CLI flag has changed. `+
+'Passing a comma-separated list of paths is no longer valid. '+
+'Documentation can be found at '+
+'https://nodejs.org/api/permissions.html#file-system-permissions',
+'Warning',
+);
+}
+}
 
 ObjectDefineProperty(process,'permission',{
 __proto__: null,
@@ -572,7 +588,8 @@ function initializePermission() {
 '--allow-worker',
 ];
 ArrayPrototypeForEach(availablePermissionFlags,(flag)=>{
-if(getOptionValue(flag)){
+constvalue=getOptionValue(flag);
+if(value.length){
 thrownewERR_MISSING_OPTION('--experimental-permission');
 }
 });

src/env.cc

@@ -875,12 +875,12 @@ Environment::Environment(IsolateData* isolate_data,
  // unless explicitly allowed by the user
  options_->allow_native_addons = false;
  flags_ = flags_ | EnvironmentFlags::kNoCreateInspector;
- permission()->Apply("*", permission::PermissionScope::kInspector);
+ permission()->Apply({"*"}, permission::PermissionScope::kInspector);
  if (!options_->allow_child_process) {
- permission()->Apply("*", permission::PermissionScope::kChildProcess);
+ permission()->Apply({"*"}, permission::PermissionScope::kChildProcess);
  }
  if (!options_->allow_worker_threads) {
- permission()->Apply("*", permission::PermissionScope::kWorkerThreads);
+ permission()->Apply({"*"}, permission::PermissionScope::kWorkerThreads);
  }
 
  if (!options_->allow_fs_read.empty()) {

src/node_options.h

@@ -121,8 +121,8 @@ class EnvironmentOptions : public Options {
  std::string experimental_policy_integrity;
  bool has_policy_integrity_string = false;
  bool experimental_permission = false;
- std::string allow_fs_read;
- std::string allow_fs_write;
+ std::vector<std::string> allow_fs_read;
+ std::vector<std::string> allow_fs_write;
  bool allow_child_process = false;
  bool allow_worker_threads = false;
  bool experimental_repl_await = true;

src/permission/child_process_permission.cc

@@ -9,7 +9,7 @@ namespace permission {
 
 // Currently, ChildProcess manage a single state
 // Once denied, it's always denied
-voidChildProcessPermission::Apply(const std::string& allow,
+voidChildProcessPermission::Apply(const std::vector<std::string>& allow,
  PermissionScope scope) {
  deny_all_ = true;
 }

src/permission/child_process_permission.h

@@ -12,7 +12,8 @@ namespace permission {
 
 classChildProcessPermissionfinal : public PermissionBase {
 public:
-voidApply(const std::string& allow, PermissionScope scope) override;
+voidApply(const std::vector<std::string>& allow,
+ PermissionScope scope) override;
 boolis_granted(PermissionScope perm,
 const std::string_view& param = "") override;
 

src/permission/fs_permission.cc

@@ -116,9 +116,11 @@ namespace permission {
 
 // allow = '*'
 // allow = '/tmp/,/home/example.js'
-void FSPermission::Apply(const std::string& allow, PermissionScope scope) {
+void FSPermission::Apply(const std::vector<std::string>& allow,
+ PermissionScope scope) {
  using std::string_view_literals::operator""sv;
- for (const std::string_view res : SplitString(allow, ","sv)) {
+
+ for (const std::string_view res : allow) {
  if (res == "*"sv) {
  if (scope == PermissionScope::kFileSystemRead) {
  deny_all_in_ = false;

src/permission/fs_permission.h

@@ -15,7 +15,8 @@ namespace permission {
 
 classFSPermissionfinal : public PermissionBase {
 public:
-voidApply(const std::string& allow, PermissionScope scope) override;
+voidApply(const std::vector<std::string>& allow,
+ PermissionScope scope) override;
 boolis_granted(PermissionScope perm, const std::string_view& param) override;
 
 structRadixTree {

src/permission/inspector_permission.cc

@@ -8,7 +8,7 @@ namespace permission {
 
 // Currently, Inspector manage a single state
 // Once denied, it's always denied
-voidInspectorPermission::Apply(const std::string& allow,
+voidInspectorPermission::Apply(const std::vector<std::string>& allow,
  PermissionScope scope) {
  deny_all_ = true;
 }

src/permission/inspector_permission.h

@@ -12,7 +12,8 @@ namespace permission {
 
 classInspectorPermissionfinal : public PermissionBase {
 public:
-voidApply(const std::string& allow, PermissionScope scope) override;
+voidApply(const std::vector<std::string>& allow,
+ PermissionScope scope) override;
 boolis_granted(PermissionScope perm,
 const std::string_view& param = "") override;
 

src/permission/permission.cc

@@ -130,7 +130,8 @@ void Permission::EnablePermissions() {
  }
 }
 
-voidPermission::Apply(const std::string& allow, PermissionScope scope) {
+voidPermission::Apply(const std::vector<std::string>& allow,
+ PermissionScope scope) {
 auto permission = nodes_.find(scope);
 if (permission != nodes_.end()) {
  permission->second->Apply(allow, scope);

src/permission/permission.h

@@ -49,7 +49,7 @@ class Permission {
 const std::string_view& res);
 
 // CLI Call
-voidApply(const std::string& allow, PermissionScope scope);
+voidApply(const std::vector<std::string>& allow, PermissionScope scope);
 voidEnablePermissions();
 
 private:

src/permission/permission_base.h

@@ -39,7 +39,8 @@ enum class PermissionScope {
 
 classPermissionBase {
 public:
-virtualvoidApply(const std::string& allow, PermissionScope scope) = 0;
+virtualvoidApply(const std::vector<std::string>& allow,
+ PermissionScope scope) = 0;
 virtualboolis_granted(PermissionScope perm,
 const std::string_view& param = "") = 0;
 };

src/permission/worker_permission.cc

@@ -9,7 +9,8 @@ namespace permission {
 
 // Currently, PolicyDenyWorker manage a single state
 // Once denied, it's always denied
-voidWorkerPermission::Apply(const std::string& allow, PermissionScope scope) {
+voidWorkerPermission::Apply(const std::vector<std::string>& allow,
+ PermissionScope scope) {
  deny_all_ = true;
 }
 

src/permission/worker_permission.h

@@ -12,7 +12,8 @@ namespace permission {
 
 classWorkerPermissionfinal : public PermissionBase {
 public:
-voidApply(const std::string& allow, PermissionScope scope) override;
+voidApply(const std::vector<std::string>& allow,
+ PermissionScope scope) override;
 boolis_granted(PermissionScope perm,
 const std::string_view& param = "") override;
 

test/es-module/test-cjs-legacyMainResolve-permission.js

@@ -31,7 +31,9 @@ describe('legacyMainResolve', () => {
 
  for (const [mainOrFolder, allowReads] of paths) {
  const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, filepath));
- const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : [];
+ const allowReadFiles = allowReads?.length > 0 ?
+ allowReadFilePaths.flatMap((path) => ['--allow-fs-read', path]) :
+ [];
  const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder);
 
  const { status, stderr } = spawnSync(
@@ -85,7 +87,9 @@ describe('legacyMainResolve', () => {
 
  for (const [folder, expectedFile, allowReads] of paths) {
  const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, folder, filepath));
- const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : [];
+ const allowReadFiles = allowReads?.length > 0 ?
+ allowReadFilePaths.flatMap((path) => ['--allow-fs-read', path]) :
+ [];
  const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder);
 
  const { status, stderr } = spawnSync(

test/parallel/test-cli-permission-multiple-allow.js

@@ -0,0 +1,83 @@
+'use strict';
+
+require('../common');
+
+const { spawnSync } = require('child_process');
+const assert = require('assert');
+const path = require('path');
+
+{
+ const tmpPath = path.resolve('/tmp/');
+ const otherPath = path.resolve('/other-path/');
+ const { status, stdout } = spawnSync(
+ process.execPath,
+ [
+ '--experimental-permission',
+ '--allow-fs-write', tmpPath, '--allow-fs-write', otherPath, '-e',
+ `console.log(process.permission.has("fs"));
+ console.log(process.permission.has("fs.read"));
+ console.log(process.permission.has("fs.write"));
+ console.log(process.permission.has("fs.write", "/tmp/"));
+ console.log(process.permission.has("fs.write", "/other-path/"));`,
+ ]
+ );
+ const [fs, fsIn, fsOut, fsOutAllowed1, fsOutAllowed2] = stdout.toString().split('\n');
+ assert.strictEqual(fs, 'false');
+ assert.strictEqual(fsIn, 'false');
+ assert.strictEqual(fsOut, 'false');
+ assert.strictEqual(fsOutAllowed1, 'true');
+ assert.strictEqual(fsOutAllowed2, 'true');
+ assert.strictEqual(status, 0);
+}
+
+{
+ const tmpPath = path.resolve('/tmp/');
+ const pathWithComma = path.resolve('/other,path/');
+ const { status, stdout } = spawnSync(
+ process.execPath,
+ [
+ '--experimental-permission',
+ '--allow-fs-write',
+ tmpPath,
+ '--allow-fs-write',
+ pathWithComma,
+ '-e',
+ `console.log(process.permission.has("fs"));
+ console.log(process.permission.has("fs.read"));
+ console.log(process.permission.has("fs.write"));
+ console.log(process.permission.has("fs.write", "/tmp/"));
+ console.log(process.permission.has("fs.write", "/other,path/"));`,
+ ]
+ );
+ const [fs, fsIn, fsOut, fsOutAllowed1, fsOutAllowed2] = stdout.toString().split('\n');
+ assert.strictEqual(fs, 'false');
+ assert.strictEqual(fsIn, 'false');
+ assert.strictEqual(fsOut, 'false');
+ assert.strictEqual(fsOutAllowed1, 'true');
+ assert.strictEqual(fsOutAllowed2, 'true');
+ assert.strictEqual(status, 0);
+}
+
+{
+ const filePath = path.resolve('/tmp/file,with,comma.txt');
+ const { status, stdout, stderr } = spawnSync(
+ process.execPath,
+ [
+ '--experimental-permission',
+ '--allow-fs-read=*',
+ `--allow-fs-write=${filePath}`,
+ '-e',
+ `console.log(process.permission.has("fs"));
+ console.log(process.permission.has("fs.read"));
+ console.log(process.permission.has("fs.write"));
+ console.log(process.permission.has("fs.write", "/tmp/file,with,comma.txt"));`,
+ ]
+ );
+ const [fs, fsIn, fsOut, fsOutAllowed] = stdout.toString().split('\n');
+ assert.strictEqual(fs, 'false');
+ assert.strictEqual(fsIn, 'true');
+ assert.strictEqual(fsOut, 'false');
+ assert.strictEqual(fsOutAllowed, 'true');
+ assert.strictEqual(status, 0);
+ assert.ok(stderr.toString().includes('Warning: The --allow-fs-write CLI flag has changed.'));
+}

test/parallel/test-permission-fs-read.js

@@ -28,7 +28,7 @@ const commonPath = path.join(__filename, '../../common');
  const { status, stderr } = spawnSync(
  process.execPath,
  [
- '--experimental-permission', `--allow-fs-read=${file},${commonPathWildcard}`, file,
+ '--experimental-permission', `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, file,
  ],
  {
  env: {

test/parallel/test-permission-fs-symlink-target-write.js

@@ -36,8 +36,8 @@ fs.writeFileSync(path.join(readWriteFolder, 'file'), 'NO evil file contents');
  process.execPath,
  [
  '--experimental-permission',
- `--allow-fs-read=${file},${commonPathWildcard},${readOnlyFolder},${readWriteFolder}`,
- `--allow-fs-write=${readWriteFolder},${writeOnlyFolder}`,
+ `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${readOnlyFolder}`, `--allow-fs-read=${readWriteFolder}`,
+ `--allow-fs-write=${readWriteFolder}`, `--allow-fs-write=${writeOnlyFolder}`,
  file,
  ],
  {

test/parallel/test-permission-fs-symlink.js

@@ -37,7 +37,7 @@ const symlinkFromBlockedFile = path.join(tmpdir.path, 'example-symlink.md');
  process.execPath,
  [
  '--experimental-permission',
- `--allow-fs-read=${file},${commonPathWildcard},${symlinkFromBlockedFile}`,
+ `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${symlinkFromBlockedFile}`,
  `--allow-fs-write=${symlinkFromBlockedFile}`,
  file,
  ],

test/parallel/test-permission-fs-traversal-path.js

@@ -31,7 +31,7 @@ const commonPathWildcard = path.join(__filename, '../../common*');
  process.execPath,
  [
  '--experimental-permission',
- `--allow-fs-read=${file},${commonPathWildcard},${allowedFolder}`,
+ `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${allowedFolder}`,
  `--allow-fs-write=${allowedFolder}`,
  file,
  ],