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

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 PR-URL: #49047 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
nodejs · Aug 17, 2023 · 413c16e · 413c16e
1 parent 3c6a1c6
commit 413c16e
Show file tree

Hide file tree

Showing 24 changed files with 161 additions and 34 deletions.
diff --git a/doc/api/cli.md b/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.
 

diff --git a/doc/api/permissions.md b/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:

diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js
@@ -554,6 +554,22 @@ function initializePermission() {
         'It could invalidate the permission model.', 'SecurityWarning');
       }
     }
+    const warnCommaFlags = [
+      '--allow-fs-read',
+      '--allow-fs-write',
+    ];
+    for (const flag of warnCommaFlags) {
+      const value = 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)) {
+      const value = getOptionValue(flag);
+      if (value.length) {
         throw new ERR_MISSING_OPTION('--experimental-permission');
       }
     });

diff --git a/src/env.cc b/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()) {

diff --git a/src/node_options.h b/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;

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

diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h
@@ -12,7 +12,8 @@ namespace permission {
 
 class ChildProcessPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::vector<std::string>& allow,
+             PermissionScope scope) override;
   bool is_granted(PermissionScope perm,
                   const std::string_view& param = "") override;
 

diff --git a/src/permission/fs_permission.cc b/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;

diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h
@@ -15,7 +15,8 @@ namespace permission {
 
 class FSPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::vector<std::string>& allow,
+             PermissionScope scope) override;
   bool is_granted(PermissionScope perm, const std::string_view& param) override;
 
   struct RadixTree {

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

diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h
@@ -12,7 +12,8 @@ namespace permission {
 
 class InspectorPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::vector<std::string>& allow,
+             PermissionScope scope) override;
   bool is_granted(PermissionScope perm,
                   const std::string_view& param = "") override;
 

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

diff --git a/src/permission/permission.h b/src/permission/permission.h
@@ -49,7 +49,7 @@ class Permission {
                                 const std::string_view& res);
 
   // CLI Call
-  void Apply(const std::string& allow, PermissionScope scope);
+  void Apply(const std::vector<std::string>& allow, PermissionScope scope);
   void EnablePermissions();
 
  private:

diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h
@@ -39,7 +39,8 @@ enum class PermissionScope {
 
 class PermissionBase {
  public:
-  virtual void Apply(const std::string& allow, PermissionScope scope) = 0;
+  virtual void Apply(const std::vector<std::string>& allow,
+                     PermissionScope scope) = 0;
   virtual bool is_granted(PermissionScope perm,
                           const std::string_view& param = "") = 0;
 };

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

diff --git a/src/permission/worker_permission.h b/src/permission/worker_permission.h
@@ -12,7 +12,8 @@ namespace permission {
 
 class WorkerPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::vector<std::string>& allow,
+             PermissionScope scope) override;
   bool is_granted(PermissionScope perm,
                   const std::string_view& param = "") override;
 

diff --git a/test/es-module/test-cjs-legacyMainResolve-permission.js b/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(

diff --git a/test/parallel/test-cli-permission-multiple-allow.js b/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.'));
+}
diff --git a/test/parallel/test-permission-fs-read.js b/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: {

diff --git a/test/parallel/test-permission-fs-symlink-target-write.js b/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,
     ],
     {

diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js
@@ -37,7 +37,7 @@ const symlinkFromBlockedFile = tmpdir.resolve('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,
     ],

diff --git a/test/parallel/test-permission-fs-traversal-path.js b/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,
     ],