From aad5dc5cbefde90bd479b1203f438ee19a246372 Mon Sep 17 00:00:00 2001
From: Carlos Espa <carlos.espa@nearform.com>
Date: Tue, 25 Jul 2023 16:10:51 +0200
Subject: [PATCH] src,process: add path delimiter flag to permission

--permission-fs-path-delimiter flag added to permission model.
If not provided default value will be comma.
---
 doc/api/cli.md                               | 24 +++++++++-
 doc/api/permissions.md                       | 18 ++++++++
 src/env.cc                                   |  6 ++-
 src/node_options.cc                          |  4 ++
 src/node_options.h                           |  1 +
 src/permission/child_process_permission.cc   |  3 +-
 src/permission/child_process_permission.h    |  4 +-
 src/permission/fs_permission.cc              |  5 +-
 src/permission/fs_permission.h               |  2 +-
 src/permission/inspector_permission.cc       |  3 +-
 src/permission/inspector_permission.h        |  4 +-
 src/permission/permission.cc                 |  6 ++-
 src/permission/permission.h                  |  2 +-
 src/permission/permission_base.h             |  2 +-
 src/permission/worker_permission.cc          |  4 +-
 src/permission/worker_permission.h           |  4 +-
 test/parallel/test-cli-permission-deny-fs.js | 48 ++++++++++++++++++++
 17 files changed, 124 insertions(+), 16 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index cf509503dc838a..b448467d3167d5 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -554,7 +554,7 @@ Enable the Permission Model for current process. When enabled, the
 following permissions are restricted:
 
 * File System - manageable through
-  [`--allow-fs-read`][], [`--allow-fs-write`][] flags
+  [`--allow-fs-read`][], [`--allow-fs-write`][] and [`--permission-fs-path-delimiter`][] flags
 * Child Process - manageable through [`--allow-child-process`][] flag
 * Worker Threads - manageable through [`--allow-worker`][] flag
 
@@ -1116,6 +1116,27 @@ unless either the `--pending-deprecation` command-line flag, or the
 are used to provide a kind of selective "early warning" mechanism that
 developers may leverage to detect deprecated API usage.
 
+### `--permission-fs-path-delimiter`
+
+<!-- YAML
+added: v20.0.0
+-->
+
+> Stability: 1 - Experimental
+
+This flag configures file system path delimiter for permissions using
+the [Permission Model][].
+
+Examples can be found in the [File System Permissions][] documentation.
+
+Especial characters in bash as `;` must be escaped or quoted:
+
+```bash
+node --experimental-permission --permission-fs-path-delimiter=\; \
+--allow-fs-read=/path/to/index.js index.js
+```
+
+
 ### `--policy-integrity=sri`
 
 <!-- YAML
@@ -2183,6 +2204,7 @@ Node.js options that are allowed are:
 * `--openssl-legacy-provider`
 * `--openssl-shared-config`
 * `--pending-deprecation`
+* `--permission-fs-path-delimiter`
 * `--policy-integrity`
 * `--preserve-symlinks-main`
 * `--preserve-symlinks`
diff --git a/doc/api/permissions.md b/doc/api/permissions.md
index 2c50c4fbabfb55..677311d74029c5 100644
--- a/doc/api/permissions.md
+++ b/doc/api/permissions.md
@@ -540,6 +540,24 @@ Wildcards are supported too:
 * `--allow-fs-read=/home/test*` will allow read access to everything
   that matches the wildcard. e.g: `/home/test/file1` or `/home/test2`
 
+##### Accessing files with comma in path
+
+To access files with comma in path you can change the path delimiter using the
+`--permission-fs-path-delimiter` flag to set a value not used in any of the
+paths you want to access.
+
+```console
+$ node --experimental-permission --allow-fs-read="/with,commas_/home" \
+--permission-fs-path-delimiter=_ index.js
+```
+
+Note when using bash special characters like `;` escape or quoting is required.
+
+```console
+$ node --experimental-permission --allow-fs-read="/home/with,commas;/home" \
+--permission-fs-path-delimiter=";" index.js
+```
+
 #### Limitations and known issues
 
 There are constraints you need to know before using this system:
diff --git a/src/env.cc b/src/env.cc
index cc0c7b2642c106..8053f8823e8b72 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -859,12 +859,14 @@ Environment::Environment(IsolateData* isolate_data,
 
     if (!options_->allow_fs_read.empty()) {
       permission()->Apply(options_->allow_fs_read,
-                          permission::PermissionScope::kFileSystemRead);
+                          permission::PermissionScope::kFileSystemRead,
+                          {{"delimiter", options_->permission_fs_path_delimiter}});
     }
 
     if (!options_->allow_fs_write.empty()) {
       permission()->Apply(options_->allow_fs_write,
-                          permission::PermissionScope::kFileSystemWrite);
+                          permission::PermissionScope::kFileSystemWrite,
+                          {{"delimiter", options_->permission_fs_path_delimiter}});
     }
   }
 }
diff --git a/src/node_options.cc b/src/node_options.cc
index c02752464c4ab5..fc1bab852306c6 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -422,6 +422,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "allow permissions to read the filesystem",
             &EnvironmentOptions::allow_fs_read,
             kAllowedInEnvvar);
+  AddOption("--permission-fs-path-delimiter",
+            "set the delimiter for the permissions path",
+            &EnvironmentOptions::permission_fs_path_delimiter,
+            kAllowedInEnvvar);
   AddOption("--allow-fs-write",
             "allow permissions to write in the filesystem",
             &EnvironmentOptions::allow_fs_write,
diff --git a/src/node_options.h b/src/node_options.h
index bb8b68894b4430..71268b00f557fa 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -123,6 +123,7 @@ class EnvironmentOptions : public Options {
   bool experimental_permission = false;
   std::string allow_fs_read;
   std::string allow_fs_write;
+  std::string permission_fs_path_delimiter = ",";
   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
index 7151eb15f90da2..7e10c45d987eb3 100644
--- a/src/permission/child_process_permission.cc
+++ b/src/permission/child_process_permission.cc
@@ -10,7 +10,8 @@ namespace permission {
 // Currently, ChildProcess manage a single state
 // Once denied, it's always denied
 void ChildProcessPermission::Apply(const std::string& allow,
-                                   PermissionScope scope) {
+                                   PermissionScope scope,
+                                   const std::unordered_map<std::string, std::string>& options) {
   deny_all_ = true;
 }
 
diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h
index b67169f1c4e180..0a39368e46cb77 100644
--- a/src/permission/child_process_permission.h
+++ b/src/permission/child_process_permission.h
@@ -12,7 +12,9 @@ namespace permission {
 
 class ChildProcessPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::string& allow,
+             PermissionScope scope,
+             const std::unordered_map<std::string, std::string>& options = {}) 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
index 91c63dff6582a8..04fe404afddbfd 100644
--- a/src/permission/fs_permission.cc
+++ b/src/permission/fs_permission.cc
@@ -116,9 +116,10 @@ namespace permission {
 
 // allow = '*'
 // allow = '/tmp/,/home/example.js'
-void FSPermission::Apply(const std::string& allow, PermissionScope scope) {
+void FSPermission::Apply(const std::string& allow, PermissionScope scope, const std::unordered_map<std::string, std::string>& options) {
   using std::string_view_literals::operator""sv;
-  for (const std::string_view res : SplitString(allow, ","sv)) {
+  std::string delimiter = options.find("delimiter") != options.end() ? options.at("delimiter") : ",";
+  for (const std::string_view res : SplitString(allow, delimiter)) {
     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
index 217d0a92d6ce71..7313495324f1ef 100644
--- a/src/permission/fs_permission.h
+++ b/src/permission/fs_permission.h
@@ -15,7 +15,7 @@ namespace permission {
 
 class FSPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::string& allow, PermissionScope scope, const std::unordered_map<std::string, std::string>& options = {}) 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
index 3cff03433b4225..5763e84521d3ca 100644
--- a/src/permission/inspector_permission.cc
+++ b/src/permission/inspector_permission.cc
@@ -9,7 +9,8 @@ namespace permission {
 // Currently, Inspector manage a single state
 // Once denied, it's always denied
 void InspectorPermission::Apply(const std::string& allow,
-                                PermissionScope scope) {
+                                PermissionScope scope,
+                                const std::unordered_map<std::string, std::string>& options) {
   deny_all_ = true;
 }
 
diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h
index 33eb25732c0d4d..de07e7c776e668 100644
--- a/src/permission/inspector_permission.h
+++ b/src/permission/inspector_permission.h
@@ -12,7 +12,9 @@ namespace permission {
 
 class InspectorPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::string& allow,
+             PermissionScope scope,
+             const std::unordered_map<std::string, std::string>& options = {}) override;
   bool is_granted(PermissionScope perm,
                   const std::string_view& param = "") override;
 
diff --git a/src/permission/permission.cc b/src/permission/permission.cc
index 38767e46093f0b..b5ce5c0704eef7 100644
--- a/src/permission/permission.cc
+++ b/src/permission/permission.cc
@@ -130,10 +130,12 @@ void Permission::EnablePermissions() {
   }
 }
 
-void Permission::Apply(const std::string& allow, PermissionScope scope) {
+void Permission::Apply(const std::string& allow,
+                       PermissionScope scope,
+                       const std::unordered_map<std::string, std::string>& options) {
   auto permission = nodes_.find(scope);
   if (permission != nodes_.end()) {
-    permission->second->Apply(allow, scope);
+    permission->second->Apply(allow, scope, options);
   }
 }
 
diff --git a/src/permission/permission.h b/src/permission/permission.h
index 3252e8d540d306..d76973416f718a 100644
--- 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::string& allow, PermissionScope scope, const std::unordered_map<std::string, std::string>& options = {});
   void EnablePermissions();
 
  private:
diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h
index c4728e40ce8f2c..08d613d4a11ae8 100644
--- a/src/permission/permission_base.h
+++ b/src/permission/permission_base.h
@@ -39,7 +39,7 @@ enum class PermissionScope {
 
 class PermissionBase {
  public:
-  virtual void Apply(const std::string& allow, PermissionScope scope) = 0;
+  virtual void Apply(const std::string& allow, PermissionScope scope, const std::unordered_map<std::string, std::string>& options = {}) = 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
index 69c89a4a4fea87..ac61afb5cc8b15 100644
--- a/src/permission/worker_permission.cc
+++ b/src/permission/worker_permission.cc
@@ -9,7 +9,9 @@ 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::string& allow,
+                             PermissionScope scope,
+                             const std::unordered_map<std::string, std::string>& options) {
   deny_all_ = true;
 }
 
diff --git a/src/permission/worker_permission.h b/src/permission/worker_permission.h
index 71681a4485a82f..3c43ea0715b99f 100644
--- a/src/permission/worker_permission.h
+++ b/src/permission/worker_permission.h
@@ -12,7 +12,9 @@ namespace permission {
 
 class WorkerPermission final : public PermissionBase {
  public:
-  void Apply(const std::string& allow, PermissionScope scope) override;
+  void Apply(const std::string& allow,
+             PermissionScope scope,
+             const std::unordered_map<std::string, std::string>& options = {}) override;
   bool is_granted(PermissionScope perm,
                   const std::string_view& param = "") override;
 
diff --git a/test/parallel/test-cli-permission-deny-fs.js b/test/parallel/test-cli-permission-deny-fs.js
index 964a0ad0a0e0c2..f26c330e6bd547 100644
--- a/test/parallel/test-cli-permission-deny-fs.js
+++ b/test/parallel/test-cli-permission-deny-fs.js
@@ -47,6 +47,54 @@ const path = require('path');
   assert.strictEqual(status, 0);
 }
 
+{
+  const tmpPath = path.resolve('/tmp/');
+  const otherPath = path.resolve('/other-path/');
+  const { status, stdout } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      '--allow-fs-write', `${tmpPath},${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};${pathWithComma}`, '--permission-fs-path-delimiter=;', '-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 { status, stdout } = spawnSync(
     process.execPath,