permission: ignore internalModuleStat on module loading

RafaelGSS · ruyadorno · commit 2beb4f1f8cde · 2025-01-05T12:57:20.000-05:00
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. PR-URL: #55797 Backport-PR-URL: #56058 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
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',
@@ -298,18 +291,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',
@@ -339,18 +322,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
@@ -159,7 +159,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');
@@ -736,11 +735,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
@@ -1031,6 +1031,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);
 
@@ -1039,8 +1041,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);
@@ -1067,15 +1067,8 @@ static int32_t FastInternalModuleStat(
   }
 
   HandleScope scope(isolate);
-  Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked());
 
   auto path = std::filesystem::path(input.data, input.data + input.length);
-  if (!env->permission()->is_granted(
-          env, permission::PermissionScope::kFileSystemRead, path.string()))
-      [[unlikely]] {
-    options.fallback = true;
-    return -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(internalFsBinding, 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/);
+}
