Skip to content

lib,permission: ignore internalModuleStat on module loading #55797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 0 additions & 27 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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'
Expand Down
7 changes: 0 additions & 7 deletions doc/api/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 1 addition & 5 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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) {

This comment was marked as abuse.

continue;
}

Expand Down
6 changes: 2 additions & 4 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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

This comment was marked as abuse.

// Permission Model checks.
static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand All @@ -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);
Expand All @@ -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:
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/permission/fs-read.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/permission/main-module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./required-module');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require('./required-module');
require('./required-module');

1 change: 1 addition & 0 deletions test/fixtures/permission/main-module.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import './required-module.mjs';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import './required-module.mjs';
import './required-module.mjs';

1 change: 1 addition & 0 deletions test/fixtures/permission/required-module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('ok');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log('ok');
console.log('ok');

1 change: 1 addition & 0 deletions test/fixtures/permission/required-module.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('ok');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log('ok');
console.log('ok');

12 changes: 3 additions & 9 deletions test/parallel/test-permission-fs-internal-module-stat.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
76 changes: 76 additions & 0 deletions test/parallel/test-permission-fs-require.js
Original file line number Diff line number Diff line change
@@ -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/);
}
Loading