From f5c7741773855fc0902f5f5516fe69b5f48200a1 Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Mon, 11 Nov 2024 14:31:44 -0300 Subject: [PATCH] permission: ignore internalModuleStat on module loading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/55797 Backport-PR-URL: https://github.com/nodejs/node/pull/58185 Reviewed-By: Yagiz Nizipli Reviewed-By: Ulises Gascón --- doc/api/cli.md | 7 -- doc/api/permissions.md | 7 -- lib/internal/modules/cjs/loader.js | 6 +- src/node_file.cc | 4 +- test/fixtures/permission/fs-read.js | 7 ++ test/fixtures/permission/main-module.js | 1 + test/fixtures/permission/main-module.mjs | 1 + test/fixtures/permission/required-module.js | 1 + test/fixtures/permission/required-module.mjs | 1 + ...test-permission-fs-internal-module-stat.js | 19 +++++ test/parallel/test-permission-fs-require.js | 76 +++++++++++++++++++ 11 files changed, 109 insertions(+), 21 deletions(-) create mode 100644 test/fixtures/permission/main-module.js create mode 100644 test/fixtures/permission/main-module.mjs create mode 100644 test/fixtures/permission/required-module.js create mode 100644 test/fixtures/permission/required-module.mjs create mode 100644 test/parallel/test-permission-fs-internal-module-stat.js create mode 100644 test/parallel/test-permission-fs-require.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 85920e71be2cc5..0656c6e35b200d 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -217,15 +217,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(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', diff --git a/doc/api/permissions.md b/doc/api/permissions.md index 80516643e890d4..4a17d949721241 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -500,15 +500,8 @@ will be restricted. ```console $ node --experimental-permission index.js -node:internal/modules/cjs/loader:171 - const result = internalModuleStat(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 index ebccdb28256314..33e80234ce8ff1 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -164,7 +164,6 @@ const policy = getLazy( ); const shouldReportRequiredModules = getLazy(() => process.env.WATCH_REPORT_DEPENDENCIES); -const permission = require('internal/process/permission'); const { vm_dynamic_import_default_internal, } = internalBinding('symbols'); @@ -704,11 +703,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 index 39a70311f4ad21..cb3361ef789f25 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1045,6 +1045,8 @@ static void ExistsSync(const FunctionCallbackInfo& args) { } // Used to speed up module loading. Returns an array [string, boolean] +// Do not expose this function through public API as it doesn't hold +// Permission Model checks. static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1052,8 +1054,6 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); node::Utf8Value path(isolate, args[0]); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); if (strlen(*path) != path.length()) { args.GetReturnValue().Set(Array::New(isolate)); diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js index 03261d975ab94c..5679387cfd6275 100644 --- a/test/fixtures/permission/fs-read.js +++ b/test/fixtures/permission/fs-read.js @@ -281,6 +281,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 new file mode 100644 index 00000000000000..cac52e04dddd24 --- /dev/null +++ b/test/fixtures/permission/main-module.js @@ -0,0 +1 @@ +require('./required-module'); \ No newline at end of file diff --git a/test/fixtures/permission/main-module.mjs b/test/fixtures/permission/main-module.mjs new file mode 100644 index 00000000000000..e7c28f7f6cab19 --- /dev/null +++ b/test/fixtures/permission/main-module.mjs @@ -0,0 +1 @@ +import './required-module.mjs'; \ No newline at end of file diff --git a/test/fixtures/permission/required-module.js b/test/fixtures/permission/required-module.js new file mode 100644 index 00000000000000..e8dbf442c5b1a2 --- /dev/null +++ b/test/fixtures/permission/required-module.js @@ -0,0 +1 @@ +console.log('ok'); \ No newline at end of file diff --git a/test/fixtures/permission/required-module.mjs b/test/fixtures/permission/required-module.mjs new file mode 100644 index 00000000000000..e8dbf442c5b1a2 --- /dev/null +++ b/test/fixtures/permission/required-module.mjs @@ -0,0 +1 @@ +console.log('ok'); \ No newline at end of file diff --git a/test/parallel/test-permission-fs-internal-module-stat.js b/test/parallel/test-permission-fs-internal-module-stat.js new file mode 100644 index 00000000000000..488ccb72494912 --- /dev/null +++ b/test/parallel/test-permission-fs-internal-module-stat.js @@ -0,0 +1,19 @@ +// Flags: --expose-internals --experimental-permission --allow-fs-read=test/common* --allow-fs-read=tools* --allow-fs-read=test/parallel* --allow-child-process +'use strict'; + +const common = require('../common'); +common.skipIfWorker(); + +if (!common.hasCrypto) { + common.skip('no crypto'); +} + +const { internalBinding } = require('internal/test/binding'); +const fixtures = require('../common/fixtures'); + +const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md'); +const internalFsBinding = internalBinding('fs'); + +{ + internalFsBinding.internalModuleStat(internalFsBinding, blockedFile); +} diff --git a/test/parallel/test-permission-fs-require.js b/test/parallel/test-permission-fs-require.js new file mode 100644 index 00000000000000..6a2e9201dac7b4 --- /dev/null +++ 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/); +}