permission: drop process.permission.deny

PR-URL: nodejs#47335 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
lpinca · Apr 4, 2023 · 6fd147c · 6fd147c
1 parent 7ec93fb
commit 6fd147c
Show file tree

Hide file tree

Showing 36 changed files with 451 additions and 797 deletions.
diff --git a/benchmark/permission/permission-fs-deny.js b/benchmark/permission/permission-fs-deny.js
diff --git a/...rk/permission/permission-fs-is-granted.js → benchmark/permission/permission-fs-read.js b/...rk/permission/permission-fs-is-granted.js → benchmark/permission/permission-fs-read.js
@@ -1,6 +1,5 @@
 'use strict';
 const common = require('../common.js');
-const fs = require('fs/promises');
 const path = require('path');
 
 const configs = {
@@ -19,22 +18,11 @@ const options = {
 
 const bench = common.createBenchmark(main, configs, options);
 
-const recursivelyDenyFiles = async (dir) => {
-  const files = await fs.readdir(dir, { withFileTypes: true });
-  for (const file of files) {
-    if (file.isDirectory()) {
-      await recursivelyDenyFiles(path.join(dir, file.name));
-    } else if (file.isFile()) {
-      process.permission.deny('fs.read', [path.join(dir, file.name)]);
-    }
-  }
-};
-
+// This is a naive benchmark and might not demonstrate real-world use cases.
+// New benchmarks will be created once the permission model config is available
+// through a config file.
 async function main(conf) {
   const benchmarkDir = path.join(__dirname, '../..');
-  // Get all the benchmark files and deny access to it
-  await recursivelyDenyFiles(benchmarkDir);
-
   bench.start();
 
   for (let i = 0; i < conf.n; i++) {

diff --git a/doc/api/permissions.md b/doc/api/permissions.md
@@ -492,24 +492,7 @@ using the [`--allow-child-process`][] and [`--allow-worker`][] respectively.
 
 When enabling the Permission Model through the [`--experimental-permission`][]
 flag a new property `permission` is added to the `process` object.
-This property contains two functions:
-
-##### `permission.deny(scope [,parameters])`
-
-API call to deny permissions at runtime ([`permission.deny()`][])
-
-```js
-process.permission.deny('fs'); // Deny permissions to ALL fs operations
-
-// Deny permissions to ALL FileSystemWrite operations
-process.permission.deny('fs.write');
-// deny FileSystemWrite permissions to the protected-folder
-process.permission.deny('fs.write', ['/home/rafaelgss/protected-folder']);
-// Deny permissions to ALL FileSystemRead operations
-process.permission.deny('fs.read');
-// deny FileSystemRead permissions to the protected-folder
-process.permission.deny('fs.read', ['/home/rafaelgss/protected-folder']);
-```
+This property contains one function:
 
 ##### `permission.has(scope ,parameters)`
 
@@ -519,10 +502,8 @@ API call to check permissions at runtime ([`permission.has()`][])
 process.permission.has('fs.write'); // true
 process.permission.has('fs.write', '/home/rafaelgss/protected-folder'); // true
 
-process.permission.deny('fs.write', '/home/rafaelgss/protected-folder');
-
-process.permission.has('fs.write'); // true
-process.permission.has('fs.write', '/home/rafaelgss/protected-folder'); // false
+process.permission.has('fs.read'); // true
+process.permission.has('fs.read', '/home/rafaelgss/protected-folder'); // false
 ```
 
 #### File System Permissions
@@ -560,39 +541,18 @@ There are constraints you need to know before using this system:
 
 * Native modules are restricted by default when using the Permission Model.
 * Relative paths are not supported through the CLI (`--allow-fs-*`).
-  The runtime API supports relative paths.
 * The model does not inherit to a child node process.
 * The model does not inherit to a worker thread.
 * When creating symlinks the target (first argument) should have read and
   write access.
 * Permission changes are not retroactively applied to existing resources.
-  Consider the following snippet:
-  ```js
-  const fs = require('node:fs');
-
-  // Open a fd
-  const fd = fs.openSync('./README.md', 'r');
-  // Then, deny access to all fs.read operations
-  process.permission.deny('fs.read');
-  // This call will NOT fail and the file will be read
-  const data = fs.readFileSync(fd);
-  ```
-
-Therefore, when possible, apply the permissions rules before any statement:
-
-```js
-process.permission.deny('fs.read');
-const fd = fs.openSync('./README.md', 'r');
-// Error: Access to this API has been restricted
-```
 
 [Security Policy]: https://github.com/nodejs/node/blob/main/SECURITY.md
 [`--allow-child-process`]: cli.md#--allow-child-process
 [`--allow-fs-read`]: cli.md#--allow-fs-read
 [`--allow-fs-write`]: cli.md#--allow-fs-write
 [`--allow-worker`]: cli.md#--allow-worker
 [`--experimental-permission`]: cli.md#--experimental-permission
-[`permission.deny()`]: process.md#processpermissiondenyscope-reference
 [`permission.has()`]: process.md#processpermissionhasscope-reference
 [import maps]: https://url.spec.whatwg.org/#relative-url-with-fragment-string
 [relative-url string]: https://url.spec.whatwg.org/#relative-url-with-fragment-string

diff --git a/doc/api/process.md b/doc/api/process.md
@@ -2634,34 +2634,6 @@ This API is available through the [`--experimental-permission`][] flag.
 for the current process. Additional documentation is available in the
 [Permission Model][].
 
-### `process.permission.deny(scope[, reference])`
-
-<!-- YAML
-added: REPLACEME
--->
-
-* `scopes` {string}
-* `reference` {Array}
-* Returns: {boolean}
-
-Deny permissions at runtime.
-
-The available scopes are:
-
-* `fs` - All File System
-* `fs.read` - File System read operations
-* `fs.write` - File System write operations
-
-The reference has a meaning based on the provided scope. For example,
-the reference when the scope is File System means files and folders.
-
-```js
-// Deny READ operations to the ./README.md file
-process.permission.deny('fs.read', ['./README.md']);
-// Deny ALL WRITE operations
-process.permission.deny('fs.write');
-```
-
 ### `process.permission.has(scope[, reference])`
 
 <!-- YAML

diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js
@@ -2,11 +2,10 @@
 
 const {
   ObjectFreeze,
-  ArrayPrototypePush,
 } = primordials;
 
 const permission = internalBinding('permission');
-const { validateString, validateArray } = require('internal/validators');
+const { validateString } = require('internal/validators');
 const { isAbsolute, resolve } = require('path');
 
 let experimentalPermission;
@@ -20,27 +19,6 @@ module.exports = ObjectFreeze({
     }
     return experimentalPermission;
   },
-  deny(scope, references) {
-    validateString(scope, 'scope');
-    if (references == null) {
-      return permission.deny(scope, references);
-    }
-
-    validateArray(references, 'references');
-    // TODO(rafaelgss): change to call fs_permission.resolve when available
-    const normalizedParams = [];
-    for (let i = 0; i < references.length; ++i) {
-      if (isAbsolute(references[i])) {
-        ArrayPrototypePush(normalizedParams, references[i]);
-      } else {
-        // TODO(aduh95): add support for WHATWG URLs and Uint8Arrays.
-        ArrayPrototypePush(normalizedParams, resolve(references[i]));
-      }
-    }
-
-    return permission.deny(scope, normalizedParams);
-  },
-
   has(scope, reference) {
     validateString(scope, 'scope');
     if (reference != null) {

diff --git a/src/env.cc b/src/env.cc
@@ -784,10 +784,10 @@ Environment::Environment(IsolateData* isolate_data,
     if (!options_->allow_fs_read.empty() || !options_->allow_fs_write.empty()) {
       options_->allow_native_addons = false;
       if (!options_->allow_child_process) {
-        permission()->Deny(permission::PermissionScope::kChildProcess, {});
+        permission()->Apply("*", permission::PermissionScope::kChildProcess);
       }
       if (!options_->allow_worker_threads) {
-        permission()->Deny(permission::PermissionScope::kWorkerThreads, {});
+        permission()->Apply("*", permission::PermissionScope::kWorkerThreads);
       }
     }
 

diff --git a/src/permission/child_process_permission.cc b/src/permission/child_process_permission.cc
@@ -10,12 +10,8 @@ namespace permission {
 // Currently, ChildProcess manage a single state
 // Once denied, it's always denied
 void ChildProcessPermission::Apply(const std::string& deny,
-                                   PermissionScope scope) {}
-
-bool ChildProcessPermission::Deny(PermissionScope perm,
-                                  const std::vector<std::string>& params) {
+                                   PermissionScope scope) {
   deny_all_ = true;
-  return true;
 }
 
 bool ChildProcessPermission::is_granted(PermissionScope perm,

diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h
@@ -13,8 +13,6 @@ namespace permission {
 class ChildProcessPermission final : public PermissionBase {
  public:
   void Apply(const std::string& deny, PermissionScope scope) override;
-  bool Deny(PermissionScope scope,
-            const std::vector<std::string>& params) 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
@@ -48,8 +48,7 @@ void FreeRecursivelyNode(
   delete node;
 }
 
-bool is_tree_granted(node::permission::FSPermission::RadixTree* deny_tree,
-                     node::permission::FSPermission::RadixTree* granted_tree,
+bool is_tree_granted(node::permission::FSPermission::RadixTree* granted_tree,
                      const std::string_view& param) {
 #ifdef _WIN32
   // is UNC file path
@@ -60,11 +59,10 @@ bool is_tree_granted(node::permission::FSPermission::RadixTree* deny_tree,
       starting_pos += 4;  // "UNC\"
     }
     auto normalized = param.substr(starting_pos);
-    return !deny_tree->Lookup(normalized) &&
-           granted_tree->Lookup(normalized, true);
+    return granted_tree->Lookup(normalized, true);
   }
 #endif
-  return !deny_tree->Lookup(param) && granted_tree->Lookup(param, true);
+  return granted_tree->Lookup(param, true);
 }
 
 }  // namespace
@@ -91,40 +89,6 @@ void FSPermission::Apply(const std::string& allow, PermissionScope scope) {
   }
 }
 
-bool FSPermission::Deny(PermissionScope perm,
-                        const std::vector<std::string>& params) {
-  if (perm == PermissionScope::kFileSystem) {
-    deny_all_in_ = true;
-    deny_all_out_ = true;
-    return true;
-  }
-
-  bool deny_all = params.size() == 0;
-  if (perm == PermissionScope::kFileSystemRead) {
-    if (deny_all) deny_all_in_ = true;
-    // when deny_all_in is already true permission.deny should be idempotent
-    if (deny_all_in_) return true;
-    allow_all_in_ = false;
-    for (auto& param : params) {
-      deny_in_fs_.Insert(WildcardIfDir(param));
-    }
-    return true;
-  }
-
-  if (perm == PermissionScope::kFileSystemWrite) {
-    if (deny_all) deny_all_out_ = true;
-    // when deny_all_out is already true permission.deny should be idempotent
-    if (deny_all_out_) return true;
-    allow_all_out_ = false;
-
-    for (auto& param : params) {
-      deny_out_fs_.Insert(WildcardIfDir(param));
-    }
-    return true;
-  }
-  return false;
-}
-
 void FSPermission::GrantAccess(PermissionScope perm, std::string res) {
   const std::string path = WildcardIfDir(res);
   if (perm == PermissionScope::kFileSystemRead) {
@@ -144,11 +108,11 @@ bool FSPermission::is_granted(PermissionScope perm,
     case PermissionScope::kFileSystemRead:
       return !deny_all_in_ &&
              ((param.empty() && allow_all_in_) || allow_all_in_ ||
-              is_tree_granted(&deny_in_fs_, &granted_in_fs_, param));
+              is_tree_granted(&granted_in_fs_, param));
     case PermissionScope::kFileSystemWrite:
       return !deny_all_out_ &&
              ((param.empty() && allow_all_out_) || allow_all_out_ ||
-              is_tree_granted(&deny_out_fs_, &granted_out_fs_, param));
+              is_tree_granted(&granted_out_fs_, param));
     default:
       return false;
   }

diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h
@@ -17,8 +17,6 @@ namespace permission {
 class FSPermission final : public PermissionBase {
  public:
   void Apply(const std::string& deny, PermissionScope scope) override;
-  bool Deny(PermissionScope scope,
-            const std::vector<std::string>& params) override;
   bool is_granted(PermissionScope perm, const std::string_view& param) override;
 
   // For debugging purposes, use the gist function to print the whole tree
@@ -135,18 +133,9 @@ class FSPermission final : public PermissionBase {
   void GrantAccess(PermissionScope scope, std::string param);
   void RestrictAccess(PermissionScope scope,
                       const std::vector<std::string>& params);
-  // /tmp/* --grant
-  // /tmp/dsadsa/t.js denied in runtime
-  //
-  // /tmp/text.txt -- grant
-  // /tmp/text.txt -- denied in runtime
-  //
   // fs granted on startup
   RadixTree granted_in_fs_;
   RadixTree granted_out_fs_;
-  // fs denied in runtime
-  RadixTree deny_in_fs_;
-  RadixTree deny_out_fs_;
 
   bool deny_all_in_ = true;
   bool deny_all_out_ = true;