From bf59539b980cbada964bf4c0991afe55668526e8 Mon Sep 17 00:00:00 2001
From: RafaelGSS <rafael.nunu@hotmail.com>
Date: Tue, 27 Aug 2024 18:00:12 -0300
Subject: [PATCH] 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: https://github.com/nodejs-private/node-private/pull/629
Refs: https://hackerone.com/reports/2575105
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
CVE-ID: CVE-2025-23083
---
 doc/api/cli.md                                |  2 +
 src/node_worker.cc                            |  6 +--
 test/es-module/test-esm-loader-hooks.mjs      |  8 ++--
 .../test-permission-dc-worker-threads.js      | 19 +++++++++
 test/parallel/test-runner-module-mocking.js   | 41 +++++++++++++++++++
 5 files changed, 68 insertions(+), 8 deletions(-)
 create mode 100644 test/parallel/test-permission-dc-worker-threads.js

diff --git a/doc/api/cli.md b/doc/api/cli.md
index dd058306aa2..e48340024e8 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -1061,6 +1061,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
index e89f60afc39..1fc3774948d 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -495,11 +495,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
index ed5c27cbc4b..9e25232146d 100644
--- 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
new file mode 100644
index 00000000000..73cbf02981f
--- /dev/null
+++ 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
index 803a566a3b4..7e9e49eefeb 100644
--- 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);
+});