Skip to content

Commit 9c99255

Browse files
RafaelGSSaduh95
authored andcommitted
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. PR-URL: #55797 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
1 parent 83ba688 commit 9c99255

11 files changed

+93
-52
lines changed

doc/api/cli.md

-27
Original file line numberDiff line numberDiff line change
@@ -219,15 +219,8 @@ The initializer module also needs to be allowed. Consider the following example:
219219

220220
```console
221221
$ node --experimental-permission t.js
222-
node:internal/modules/cjs/loader:162
223-
const result = internalModuleStat(receiver, filename);
224-
^
225222

226223
Error: Access to this API has been restricted
227-
at stat (node:internal/modules/cjs/loader:162:18)
228-
at Module._findPath (node:internal/modules/cjs/loader:640:16)
229-
at resolveMainPath (node:internal/modules/run_main:15:25)
230-
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:53:24)
231224
at node:internal/main/run_main_module:23:47 {
232225
code: 'ERR_ACCESS_DENIED',
233226
permission: 'FileSystemRead',
@@ -300,18 +293,8 @@ new WASI({
300293

301294
```console
302295
$ node --experimental-permission --allow-fs-read=* index.js
303-
node:wasi:99
304-
const wrap = new _WASI(args, env, preopens, stdio);
305-
^
306296

307297
Error: Access to this API has been restricted
308-
at new WASI (node:wasi:99:18)
309-
at Object.<anonymous> (/home/index.js:3:1)
310-
at Module._compile (node:internal/modules/cjs/loader:1476:14)
311-
at Module._extensions..js (node:internal/modules/cjs/loader:1555:10)
312-
at Module.load (node:internal/modules/cjs/loader:1288:32)
313-
at Module._load (node:internal/modules/cjs/loader:1104:12)
314-
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:191:14)
315298
at node:internal/main/run_main_module:30:49 {
316299
code: 'ERR_ACCESS_DENIED',
317300
permission: 'WASI',
@@ -341,18 +324,8 @@ new Worker(__filename);
341324

342325
```console
343326
$ node --experimental-permission --allow-fs-read=* index.js
344-
node:internal/worker:188
345-
this[kHandle] = new WorkerImpl(url,
346-
^
347327

348328
Error: Access to this API has been restricted
349-
at new Worker (node:internal/worker:188:21)
350-
at Object.<anonymous> (/home/index.js.js:3:1)
351-
at Module._compile (node:internal/modules/cjs/loader:1120:14)
352-
at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
353-
at Module.load (node:internal/modules/cjs/loader:998:32)
354-
at Module._load (node:internal/modules/cjs/loader:839:12)
355-
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
356329
at node:internal/main/run_main_module:17:47 {
357330
code: 'ERR_ACCESS_DENIED',
358331
permission: 'WorkerThreads'

doc/api/permissions.md

-7
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,8 @@ will be restricted.
4747

4848
```console
4949
$ node --experimental-permission index.js
50-
node:internal/modules/cjs/loader:171
51-
const result = internalModuleStat(receiver, filename);
52-
^
5350

5451
Error: Access to this API has been restricted
55-
at stat (node:internal/modules/cjs/loader:171:18)
56-
at Module._findPath (node:internal/modules/cjs/loader:627:16)
57-
at resolveMainPath (node:internal/modules/run_main:19:25)
58-
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:24)
5952
at node:internal/main/run_main_module:23:47 {
6053
code: 'ERR_ACCESS_DENIED',
6154
permission: 'FileSystemRead',

lib/internal/modules/cjs/loader.js

+1-5
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,6 @@ const packageJsonReader = require('internal/modules/package_json_reader');
160160
const { getOptionValue, getEmbedderOptions } = require('internal/options');
161161
const shouldReportRequiredModules = getLazy(() => process.env.WATCH_REPORT_DEPENDENCIES);
162162

163-
const permission = require('internal/process/permission');
164163
const {
165164
vm_dynamic_import_default_internal,
166165
} = internalBinding('symbols');
@@ -729,11 +728,8 @@ Module._findPath = function(request, paths, isMain) {
729728
// For each path
730729
for (let i = 0; i < paths.length; i++) {
731730
// Don't search further if path doesn't exist
732-
// or doesn't have permission to it
733731
const curPath = paths[i];
734-
if (insidePath && curPath &&
735-
((permission.isEnabled() && !permission.has('fs.read', curPath)) || _stat(curPath) < 1)
736-
) {
732+
if (insidePath && curPath && _stat(curPath) < 1) {
737733
continue;
738734
}
739735

src/node_file.cc

+2-4
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,8 @@ static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
10371037
// Used to speed up module loading. Returns 0 if the path refers to
10381038
// a file, 1 when it's a directory or < 0 on error (usually -ENOENT.)
10391039
// The speedup comes from not creating thousands of Stat and Error objects.
1040+
// Do not expose this function through public API as it doesn't hold
1041+
// Permission Model checks.
10401042
static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
10411043
Environment* env = Environment::GetCurrent(args);
10421044

@@ -1045,8 +1047,6 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
10451047
BufferValue path(env->isolate(), args[1]);
10461048
CHECK_NOT_NULL(*path);
10471049
ToNamespacedPath(env, &path);
1048-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1049-
env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
10501050

10511051
uv_fs_t req;
10521052
int rc = uv_fs_stat(env->event_loop(), &req, *path, nullptr);
@@ -1069,8 +1069,6 @@ static int32_t FastInternalModuleStat(
10691069
HandleScope scope(env->isolate());
10701070

10711071
auto path = std::filesystem::path(input.data, input.data + input.length);
1072-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1073-
env, permission::PermissionScope::kFileSystemRead, path.string(), -1);
10741072

10751073
switch (std::filesystem::status(path).type()) {
10761074
case std::filesystem::file_type::directory:

test/fixtures/permission/fs-read.js

+7
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,13 @@ const regularFile = __filename;
282282
permission: 'FileSystemRead',
283283
resource: path.toNamespacedPath(blockedFolder),
284284
}));
285+
assert.throws(() => {
286+
fs.readdirSync(blockedFolder, { recursive: true });
287+
}, common.expectsError({
288+
code: 'ERR_ACCESS_DENIED',
289+
permission: 'FileSystemRead',
290+
resource: path.toNamespacedPath(blockedFolder),
291+
}));
285292
assert.throws(() => {
286293
fs.readdirSync(blockedFolder);
287294
}, common.expectsError({
+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
require('./required-module');
+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
import './required-module.mjs';
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.log('ok');
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.log('ok');

test/parallel/test-permission-fs-internal-module-stat.js

+3-9
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,14 @@ if (!common.hasCrypto) {
99
}
1010

1111
const { internalBinding } = require('internal/test/binding');
12-
const assert = require('node:assert');
13-
const path = require('node:path');
1412
const fixtures = require('../common/fixtures');
1513

1614
const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md');
1715
const internalFsBinding = internalBinding('fs');
1816

1917
// Run this inside a for loop to trigger the fast API
2018
for (let i = 0; i < 10_000; i++) {
21-
assert.throws(() => {
22-
internalFsBinding.internalModuleStat(internalFsBinding, blockedFile);
23-
}, {
24-
code: 'ERR_ACCESS_DENIED',
25-
permission: 'FileSystemRead',
26-
resource: path.toNamespacedPath(blockedFile),
27-
});
19+
// internalModuleStat does not use permission model.
20+
// doesNotThrow
21+
internalFsBinding.internalModuleStat(internalFsBinding, blockedFile);
2822
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Flags: --experimental-permission --allow-fs-read=* --allow-child-process
2+
'use strict';
3+
4+
const common = require('../common');
5+
common.skipIfWorker();
6+
const fixtures = require('../common/fixtures');
7+
8+
const assert = require('node:assert');
9+
const { spawnSync } = require('node:child_process');
10+
11+
{
12+
const mainModule = fixtures.path('permission', 'main-module.js');
13+
const requiredModule = fixtures.path('permission', 'required-module.js');
14+
const { status, stdout, stderr } = spawnSync(
15+
process.execPath,
16+
[
17+
'--experimental-permission',
18+
'--allow-fs-read', mainModule,
19+
'--allow-fs-read', requiredModule,
20+
mainModule,
21+
]
22+
);
23+
24+
assert.strictEqual(status, 0, stderr.toString());
25+
assert.strictEqual(stdout.toString(), 'ok\n');
26+
}
27+
28+
{
29+
// When required module is not passed as allowed path
30+
const mainModule = fixtures.path('permission', 'main-module.js');
31+
const { status, stderr } = spawnSync(
32+
process.execPath,
33+
[
34+
'--experimental-permission',
35+
'--allow-fs-read', mainModule,
36+
mainModule,
37+
]
38+
);
39+
40+
assert.strictEqual(status, 1, stderr.toString());
41+
assert.match(stderr.toString(), /Error: Access to this API has been restricted/);
42+
}
43+
44+
{
45+
// ESM loader test
46+
const mainModule = fixtures.path('permission', 'main-module.mjs');
47+
const requiredModule = fixtures.path('permission', 'required-module.mjs');
48+
const { status, stdout, stderr } = spawnSync(
49+
process.execPath,
50+
[
51+
'--experimental-permission',
52+
'--allow-fs-read', mainModule,
53+
'--allow-fs-read', requiredModule,
54+
mainModule,
55+
]
56+
);
57+
58+
assert.strictEqual(status, 0, stderr.toString());
59+
assert.strictEqual(stdout.toString(), 'ok\n');
60+
}
61+
62+
{
63+
// When required module is not passed as allowed path (ESM)
64+
const mainModule = fixtures.path('permission', 'main-module.mjs');
65+
const { status, stderr } = spawnSync(
66+
process.execPath,
67+
[
68+
'--experimental-permission',
69+
'--allow-fs-read', mainModule,
70+
mainModule,
71+
]
72+
);
73+
74+
assert.strictEqual(status, 1, stderr.toString());
75+
assert.match(stderr.toString(), /Error: Access to this API has been restricted/);
76+
}

0 commit comments

Comments
 (0)