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#629 Refs: https://hackerone.com/reports/2575105 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> CVE-ID: CVE-2025-23083
nodejs · Jan 19, 2025 · 51938f0 · 51938f0
1 parent b9ce2bc
commit 51938f0
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 8 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -1051,6 +1051,8 @@ added:
 
 Enable module mocking in the test runner.
 
+This feature requires `--allow-worker` if used with the [Permission Model][].
+
 ### `--experimental-transform-types`
 
 <!-- YAML

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -493,11 +493,9 @@ Worker::~Worker() {
 
 void Worker::New(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, permission::PermissionScope::kWorkerThreads, "");
   bool is_internal = args[5]->IsTrue();
-  if (!is_internal) {
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kWorkerThreads, "");
-  }
   Isolate* isolate = args.GetIsolate();
 
   CHECK(args.IsConstructCall());

diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs
@@ -179,7 +179,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
     });
   });
 
-  it('should work without worker permission', async () => {
+  it('should not work without worker permission', async () => {
     const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
       '--no-warnings',
       '--permission',
@@ -190,9 +190,9 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
       fixtures.path('es-modules/esm-top-level-await.mjs'),
     ]);
 
-    assert.strictEqual(stderr, '');
-    assert.match(stdout, /^1\r?\n2\r?\n$/);
-    assert.strictEqual(code, 0);
+    assert.match(stderr, /Error: Access to this API has been restricted/);
+    assert.strictEqual(stdout, '');
+    assert.strictEqual(code, 1);
     assert.strictEqual(signal, null);
   });
 

diff --git a/test/parallel/test-permission-dc-worker-threads.js b/test/parallel/test-permission-dc-worker-threads.js
@@ -0,0 +1,19 @@
+// Flags: --experimental-permission --allow-fs-read=* --experimental-test-module-mocks
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+
+{
+  const diagnostics_channel = require('node:diagnostics_channel');
+  diagnostics_channel.subscribe('worker_threads', common.mustNotCall());
+  const { mock } = require('node:test');
+
+  // Module mocking should throw instead of posting to worker_threads dc
+  assert.throws(() => {
+    mock.module('node:path');
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'WorkerThreads',
+  }));
+}
diff --git a/test/parallel/test-runner-module-mocking.js b/test/parallel/test-runner-module-mocking.js
@@ -650,3 +650,44 @@ test('wrong import syntax should throw error after module mocking', async () =>
   assert.match(stderr, /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/);
   assert.strictEqual(code, 1);
 });
+
+test('should throw ERR_ACCESS_DENIED when permission model is enabled', async (t) => {
+  const cwd = fixtures.path('test-runner');
+  const fixture = fixtures.path('test-runner', 'mock-nm.js');
+  const args = [
+    '--experimental-permission',
+    '--allow-fs-read=*',
+    '--experimental-test-module-mocks',
+    fixture,
+  ];
+  const {
+    code,
+    stdout,
+  } = await common.spawnPromisified(process.execPath, args, { cwd });
+
+  assert.strictEqual(code, 1);
+  assert.match(stdout, /Error: Access to this API has been restricted/);
+  assert.match(stdout, /permission: 'WorkerThreads'/);
+});
+
+test('should work when --allow-worker is passed and permission model is enabled', async (t) => {
+  const cwd = fixtures.path('test-runner');
+  const fixture = fixtures.path('test-runner', 'mock-nm.js');
+  const args = [
+    '--experimental-permission',
+    '--allow-fs-read=*',
+    '--allow-worker',
+    '--experimental-test-module-mocks',
+    fixture,
+  ];
+  const {
+    code,
+    stdout,
+    stderr,
+    signal,
+  } = await common.spawnPromisified(process.execPath, args, { cwd });
+
+  assert.strictEqual(code, 0, stderr);
+  assert.strictEqual(signal, null);
+  assert.match(stdout, /pass 1/, stderr);
+});