permission: ignore internalModuleStat on module loading

This improves Permission Model usage when allowing read access to specifi modules. To achieve that, the permission model check on internalModuleStat has been removed meaning that on module loading, uv_fs_stat is performed on files and folders even when the permission model is enabled. Although a uv_fs_stat is performed, reading/executing the module will still pass by the permission model check. Without this PR when an app tries to --allow-fs-read=./a.js --allow-fs-read=./b.js where `a` attempt to load b, it will fails as it reads $pwd and no permission has been given to this path.
nodejs · Nov 9, 2024 · b6c7580 · b6c7580
1 parent a243225
commit b6c7580
Show file tree

Hide file tree

Showing 11 changed files with 93 additions and 52 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -219,15 +219,8 @@ The initializer module also needs to be allowed. Consider the following example:
 
 ```console
 $ node --experimental-permission t.js
-node:internal/modules/cjs/loader:162
-  const result = internalModuleStat(receiver, filename);
-                 ^
 
 Error: Access to this API has been restricted
-    at stat (node:internal/modules/cjs/loader:162:18)
-    at Module._findPath (node:internal/modules/cjs/loader:640:16)
-    at resolveMainPath (node:internal/modules/run_main:15:25)
-    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:53:24)
     at node:internal/main/run_main_module:23:47 {
   code: 'ERR_ACCESS_DENIED',
   permission: 'FileSystemRead',
@@ -300,18 +293,8 @@ new WASI({
 
 ```console
 $ node --experimental-permission --allow-fs-read=* index.js
-node:wasi:99
-    const wrap = new _WASI(args, env, preopens, stdio);
-                 ^
 
 Error: Access to this API has been restricted
-    at new WASI (node:wasi:99:18)
-    at Object.<anonymous> (/home/index.js:3:1)
-    at Module._compile (node:internal/modules/cjs/loader:1476:14)
-    at Module._extensions..js (node:internal/modules/cjs/loader:1555:10)
-    at Module.load (node:internal/modules/cjs/loader:1288:32)
-    at Module._load (node:internal/modules/cjs/loader:1104:12)
-    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:191:14)
     at node:internal/main/run_main_module:30:49 {
   code: 'ERR_ACCESS_DENIED',
   permission: 'WASI',
@@ -341,18 +324,8 @@ new Worker(__filename);
 
 ```console
 $ node --experimental-permission --allow-fs-read=* index.js
-node:internal/worker:188
-    this[kHandle] = new WorkerImpl(url,
-                    ^
 
 Error: Access to this API has been restricted
-    at new Worker (node:internal/worker:188:21)
-    at Object.<anonymous> (/home/index.js.js:3:1)
-    at Module._compile (node:internal/modules/cjs/loader:1120:14)
-    at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
-    at Module.load (node:internal/modules/cjs/loader:998:32)
-    at Module._load (node:internal/modules/cjs/loader:839:12)
-    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
     at node:internal/main/run_main_module:17:47 {
   code: 'ERR_ACCESS_DENIED',
   permission: 'WorkerThreads'

diff --git a/doc/api/permissions.md b/doc/api/permissions.md
@@ -47,15 +47,8 @@ will be restricted.
 
 ```console
 $ node --experimental-permission index.js
-node:internal/modules/cjs/loader:171
-  const result = internalModuleStat(receiver, filename);
-                 ^
 
 Error: Access to this API has been restricted
-    at stat (node:internal/modules/cjs/loader:171:18)
-    at Module._findPath (node:internal/modules/cjs/loader:627:16)
-    at resolveMainPath (node:internal/modules/run_main:19:25)
-    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:24)
     at node:internal/main/run_main_module:23:47 {
   code: 'ERR_ACCESS_DENIED',
   permission: 'FileSystemRead',

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -160,7 +160,6 @@ const packageJsonReader = require('internal/modules/package_json_reader');
 const { getOptionValue, getEmbedderOptions } = require('internal/options');
 const shouldReportRequiredModules = getLazy(() => process.env.WATCH_REPORT_DEPENDENCIES);
 
-const permission = require('internal/process/permission');
 const {
   vm_dynamic_import_default_internal,
 } = internalBinding('symbols');
@@ -729,11 +728,8 @@ Module._findPath = function(request, paths, isMain) {
   // For each path
   for (let i = 0; i < paths.length; i++) {
     // Don't search further if path doesn't exist
-    // or doesn't have permission to it
     const curPath = paths[i];
-    if (insidePath && curPath &&
-      ((permission.isEnabled() && !permission.has('fs.read', curPath)) || _stat(curPath) < 1)
-    ) {
+    if (insidePath && curPath && _stat(curPath) < 1) {
       continue;
     }
 

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1037,6 +1037,8 @@ static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
 // Used to speed up module loading.  Returns 0 if the path refers to
 // a file, 1 when it's a directory or < 0 on error (usually -ENOENT.)
 // The speedup comes from not creating thousands of Stat and Error objects.
+// Do not expose this function through public API as it doesn't hold
+// Permission Model checks.
 static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -1045,8 +1047,6 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
   BufferValue path(env->isolate(), args[1]);
   CHECK_NOT_NULL(*path);
   ToNamespacedPath(env, &path);
-  THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
 
   uv_fs_t req;
   int rc = uv_fs_stat(env->event_loop(), &req, *path, nullptr);
@@ -1069,8 +1069,6 @@ static int32_t FastInternalModuleStat(
   HandleScope scope(env->isolate());
 
   auto path = std::filesystem::path(input.data, input.data + input.length);
-  THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kFileSystemRead, path.string(), -1);
 
   switch (std::filesystem::status(path).type()) {
     case std::filesystem::file_type::directory:

diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js
@@ -282,6 +282,13 @@ const regularFile = __filename;
     permission: 'FileSystemRead',
     resource: path.toNamespacedPath(blockedFolder),
   }));
+  assert.throws(() => {
+    fs.readdirSync(blockedFolder, { recursive: true });
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemRead',
+    resource: path.toNamespacedPath(blockedFolder),
+  }));
   assert.throws(() => {
     fs.readdirSync(blockedFolder);
   }, common.expectsError({

diff --git a/test/fixtures/permission/main-module.js b/test/fixtures/permission/main-module.js
@@ -0,0 +1 @@
+require('./required-module');
diff --git a/test/fixtures/permission/main-module.mjs b/test/fixtures/permission/main-module.mjs
@@ -0,0 +1 @@
+import './required-module.mjs';
diff --git a/test/fixtures/permission/required-module.js b/test/fixtures/permission/required-module.js
@@ -0,0 +1 @@
+console.log('ok');
diff --git a/test/fixtures/permission/required-module.mjs b/test/fixtures/permission/required-module.mjs
@@ -0,0 +1 @@
+console.log('ok');
diff --git a/test/parallel/test-permission-fs-internal-module-stat.js b/test/parallel/test-permission-fs-internal-module-stat.js
@@ -9,20 +9,14 @@ if (!common.hasCrypto) {
 }
 
 const { internalBinding } = require('internal/test/binding');
-const assert = require('node:assert');
-const path = require('node:path');
 const fixtures = require('../common/fixtures');
 
 const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md');
 const internalFsBinding = internalBinding('fs');
 
 // Run this inside a for loop to trigger the fast API
 for (let i = 0; i < 10_000; i++) {
-  assert.throws(() => {
-    internalFsBinding.internalModuleStat(internalFsBinding, blockedFile);
-  }, {
-    code: 'ERR_ACCESS_DENIED',
-    permission: 'FileSystemRead',
-    resource: path.toNamespacedPath(blockedFile),
-  });
+  // internalModuleStat does not use permission model.
+  // doesNotThrow
+  internalFsBinding.internalModuleStat(blockedFile);
 }
diff --git a/test/parallel/test-permission-fs-require.js b/test/parallel/test-permission-fs-require.js
@@ -0,0 +1,76 @@
+// Flags: --experimental-permission --allow-fs-read=* --allow-child-process
+'use strict';
+
+const common = require('../common');
+common.skipIfWorker();
+const fixtures = require('../common/fixtures');
+
+const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
+
+{
+  const mainModule = fixtures.path('permission', 'main-module.js');
+  const requiredModule = fixtures.path('permission', 'required-module.js');
+  const { status, stdout, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      '--allow-fs-read', mainModule,
+      '--allow-fs-read', requiredModule,
+      mainModule,
+    ]
+  );
+
+  assert.strictEqual(status, 0, stderr.toString());
+  assert.strictEqual(stdout.toString(), 'ok\n');
+}
+
+{
+  // When required module is not passed as allowed path
+  const mainModule = fixtures.path('permission', 'main-module.js');
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      '--allow-fs-read', mainModule,
+      mainModule,
+    ]
+  );
+
+  assert.strictEqual(status, 1, stderr.toString());
+  assert.match(stderr.toString(), /Error: Access to this API has been restricted/);
+}
+
+{
+  // ESM loader test
+  const mainModule = fixtures.path('permission', 'main-module.mjs');
+  const requiredModule = fixtures.path('permission', 'required-module.mjs');
+  const { status, stdout, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      '--allow-fs-read', mainModule,
+      '--allow-fs-read', requiredModule,
+      mainModule,
+    ]
+  );
+
+  assert.strictEqual(status, 0, stderr.toString());
+  assert.strictEqual(stdout.toString(), 'ok\n');
+}
+
+{
+  // When required module is not passed as allowed path (ESM)
+  const mainModule = fixtures.path('permission', 'main-module.mjs');
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      '--allow-fs-read', mainModule,
+      mainModule,
+    ]
+  );
+
+  assert.strictEqual(status, 1, stderr.toString());
+  assert.match(stderr.toString(), /Error: Access to this API has been restricted/);
+}