Skip to content

Commit 389f239

Browse files
committedJan 19, 2025
src,loader,permission: throw on InternalWorker use
Previously this PR it was expected that InternalWorker usage doesn't require the --allow-worker when the permission model is enabled. This, however, exposes a vulnerability whenever the instance gets accessed by the user. For example through diagnostics_channel.subscribe('worker_threads') PR-URL: nodejs-private/node-private#652 Refs: https://hackerone.com/reports/2575105 CVE-ID: CVE-2025-23083
1 parent 42d5821 commit 389f239

File tree

5 files changed

+67
-8
lines changed

5 files changed

+67
-8
lines changed
 

Diff for: ‎doc/api/cli.md

+2
Original file line numberDiff line numberDiff line change
@@ -1076,6 +1076,8 @@ added: v20.18.0
10761076
10771077
Enable module mocking in the test runner.
10781078

1079+
This feature requires `--allow-worker` if used with the [Permission Model][].
1080+
10791081
### `--experimental-vm-modules`
10801082

10811083
<!-- YAML

Diff for: ‎src/node_worker.cc

+2-4
Original file line numberDiff line numberDiff line change
@@ -490,11 +490,9 @@ Worker::~Worker() {
490490

491491
void Worker::New(const FunctionCallbackInfo<Value>& args) {
492492
Environment* env = Environment::GetCurrent(args);
493+
THROW_IF_INSUFFICIENT_PERMISSIONS(
494+
env, permission::PermissionScope::kWorkerThreads, "");
493495
bool is_internal = args[5]->IsTrue();
494-
if (!is_internal) {
495-
THROW_IF_INSUFFICIENT_PERMISSIONS(
496-
env, permission::PermissionScope::kWorkerThreads, "");
497-
}
498496
Isolate* isolate = args.GetIsolate();
499497

500498
CHECK(args.IsConstructCall());

Diff for: ‎test/es-module/test-esm-loader-hooks.mjs

+4-4
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ describe('Loader hooks', { concurrency: true }, () => {
154154
});
155155
});
156156

157-
it('should work without worker permission', async () => {
157+
it('should not work without worker permission', async () => {
158158
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
159159
'--no-warnings',
160160
'--experimental-permission',
@@ -165,9 +165,9 @@ describe('Loader hooks', { concurrency: true }, () => {
165165
fixtures.path('es-modules/esm-top-level-await.mjs'),
166166
]);
167167

168-
assert.strictEqual(stderr, '');
169-
assert.match(stdout, /^1\r?\n2\r?\n$/);
170-
assert.strictEqual(code, 0);
168+
assert.match(stderr, /Error: Access to this API has been restricted/);
169+
assert.strictEqual(stdout, '');
170+
assert.strictEqual(code, 1);
171171
assert.strictEqual(signal, null);
172172
});
173173

Diff for: ‎test/parallel/test-permission-dc-worker-threads.js

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Flags: --experimental-permission --allow-fs-read=* --experimental-test-module-mocks
2+
'use strict';
3+
4+
const common = require('../common');
5+
const assert = require('node:assert');
6+
7+
{
8+
const diagnostics_channel = require('node:diagnostics_channel');
9+
diagnostics_channel.subscribe('worker_threads', common.mustNotCall());
10+
const { mock } = require('node:test');
11+
12+
// Module mocking should throw instead of posting to worker_threads dc
13+
assert.throws(() => {
14+
mock.module('node:path');
15+
}, common.expectsError({
16+
code: 'ERR_ACCESS_DENIED',
17+
permission: 'WorkerThreads',
18+
}));
19+
}

Diff for: ‎test/parallel/test-runner-module-mocking.js

+40
Original file line numberDiff line numberDiff line change
@@ -631,3 +631,43 @@ test('wrong import syntax should throw error after module mocking.', async () =>
631631
assert.match(stderr, /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/);
632632
assert.strictEqual(code, 1);
633633
});
634+
635+
test('should throw ERR_ACCESS_DENIED when permission model is enabled', async (t) => {
636+
const cwd = fixtures.path('test-runner');
637+
const fixture = fixtures.path('test-runner', 'mock-nm.js');
638+
const args = [
639+
'--experimental-permission',
640+
'--allow-fs-read=*',
641+
'--experimental-test-module-mocks',
642+
fixture,
643+
];
644+
const {
645+
code,
646+
stdout,
647+
} = await common.spawnPromisified(process.execPath, args, { cwd });
648+
649+
assert.strictEqual(code, 1);
650+
assert.match(stdout, /Access to this API has been restricted/);
651+
});
652+
653+
test('should work when --allow-worker is passed and permission model is enabled', async (t) => {
654+
const cwd = fixtures.path('test-runner');
655+
const fixture = fixtures.path('test-runner', 'mock-nm.js');
656+
const args = [
657+
'--experimental-permission',
658+
'--allow-fs-read=*',
659+
'--allow-worker',
660+
'--experimental-test-module-mocks',
661+
fixture,
662+
];
663+
const {
664+
code,
665+
stdout,
666+
stderr,
667+
signal,
668+
} = await common.spawnPromisified(process.execPath, args, { cwd });
669+
670+
assert.strictEqual(code, 0, stderr);
671+
assert.strictEqual(signal, null);
672+
assert.match(stdout, /pass 1/, stderr);
673+
});

0 commit comments

Comments
 (0)
Please sign in to comment.