From bca44e6d60eda1e517c58d4899a70eb207536406 Mon Sep 17 00:00:00 2001 From: yunshingng Date: Tue, 18 Aug 2026 10:53:04 -0400 Subject: [PATCH 1/5] permission: clamp worker grants to parent for explicit execArgv C++-side intersection after Worker option parse when the parent has the Permission Model enabled: - No JS process.execArgv copying (avoids NODE_OPTIONS / repeated-flag gaps) - If the worker did not configure permission flags (e.g. execArgv: []), effective grants become the parent grant set - If the worker configured permission flags, boolean and fs grants are intersected with the parent so the worker cannot exceed the parent - Non-permission execArgv differences remain possible May be semver-major relative to documented non-inheritance; for reviewer call. Signed-off-by: yunshingng --- src/node_worker.cc | 111 ++++++++++++++++++ .../test-permission-worker-empty-execargv.js | 98 ++++++++++++++++ 2 files changed, 209 insertions(+) create mode 100644 test/parallel/test-permission-worker-empty-execargv.js diff --git a/src/node_worker.cc b/src/node_worker.cc index edc21e7e5561..539a7a89331e 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -504,6 +504,113 @@ Worker::~Worker() { Debug(this, "Worker %llu destroyed", thread_id_.id); } + +// When the parent has --permission enabled, explicit Worker execArgv (including +// []) must not yield a wider grant set than the parent. Enforcement is on the +// C++ side after options are parsed so NODE_OPTIONS and repeated --allow-* are +// already reflected in EnvironmentOptions (no JS process.execArgv copying). +// +// If the worker did not configure Permission Model flags at all, treat requested +// grants as unrestricted under the model so the intersection equals the parent +// grant set. If the worker did configure permission flags, intersect with parent. + +static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { + if (w->permission || w->permission_audit) return true; + if (!w->allow_fs_read.empty() || !w->allow_fs_write.empty()) return true; + if (w->allow_addons || w->allow_inspector || w->allow_child_process || + w->allow_net || w->allow_wasi || w->allow_ffi || w->allow_openssl_store || + w->allow_worker_threads) { + return true; + } + return false; +} + +static bool PathGrantedByParentList(const std::vector& parent_paths, + const std::string& requested) { + if (parent_paths.empty()) return false; + for (const std::string& p : parent_paths) { + if (p == "*" || p == requested) return true; + if (p.empty() || requested.size() < p.size()) continue; + if (requested.compare(0, p.size(), p) != 0) continue; + if (requested.size() == p.size()) return true; + if (p.back() == '/' || requested[p.size()] == '/') return true; + } + return false; +} + +static void IntersectPathList(std::vector* worker, + const std::vector& parent) { + if (worker == nullptr || worker->empty()) return; + std::vector out; + out.reserve(worker->size()); + for (const std::string& wpath : *worker) { + if (wpath == "*") { + for (const std::string& p : parent) { + if (p == "*") { + out.push_back(wpath); + break; + } + } + continue; + } + if (PathGrantedByParentList(parent, wpath)) out.push_back(wpath); + } + *worker = std::move(out); +} + +static void CopyParentPermissionGrants(EnvironmentOptions* w, + const EnvironmentOptions* parent) { + w->permission = parent->permission; + w->permission_audit = parent->permission_audit; + w->allow_addons = parent->allow_addons; + w->allow_inspector = parent->allow_inspector; + w->allow_child_process = parent->allow_child_process; + w->allow_net = parent->allow_net; + w->allow_wasi = parent->allow_wasi; + w->allow_ffi = parent->allow_ffi; + w->allow_openssl_store = parent->allow_openssl_store; + w->allow_worker_threads = parent->allow_worker_threads; + w->allow_fs_read = parent->allow_fs_read; + w->allow_fs_write = parent->allow_fs_write; +} + +static void IntersectPermissionGrants(EnvironmentOptions* w, + const EnvironmentOptions* parent) { + w->permission = true; + w->permission_audit = w->permission_audit || parent->permission_audit; + + w->allow_addons = w->allow_addons && parent->allow_addons; + w->allow_inspector = w->allow_inspector && parent->allow_inspector; + w->allow_child_process = w->allow_child_process && parent->allow_child_process; + w->allow_net = w->allow_net && parent->allow_net; + w->allow_wasi = w->allow_wasi && parent->allow_wasi; + w->allow_ffi = w->allow_ffi && parent->allow_ffi; + w->allow_openssl_store = w->allow_openssl_store && parent->allow_openssl_store; + w->allow_worker_threads = + w->allow_worker_threads && parent->allow_worker_threads; + + IntersectPathList(&w->allow_fs_read, parent->allow_fs_read); + IntersectPathList(&w->allow_fs_write, parent->allow_fs_write); +} + +static void ClampWorkerPermissionToParent(Environment* env, + PerIsolateOptions* worker_opts) { + if (worker_opts == nullptr || !env->permission()->enabled()) return; + + EnvironmentOptions* parent = + env->isolate_data()->options()->get_per_env_options(); + EnvironmentOptions* w = worker_opts->get_per_env_options(); + if (parent == nullptr || w == nullptr) return; + + if (!WorkerConfiguredPermission(w)) { + CopyParentPermissionGrants(w, parent); + w->permission = true; + return; + } + + IntersectPermissionGrants(w, parent); +} + void Worker::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_IF_INSUFFICIENT_PERMISSIONS( @@ -688,6 +795,10 @@ void Worker::New(const FunctionCallbackInfo& args) { // essential to load user codes and must not be blocked by the inspector // for internal scripts. // Still, `--inspect-node` can break on the first line of internal scripts. + if (env->permission()->enabled() && per_isolate_opts) { + ClampWorkerPermissionToParent(env, per_isolate_opts.get()); + } + if (is_internal) { per_isolate_opts->per_env->get_debug_options() ->DisableWaitOrBreakFirstLine(); diff --git a/test/parallel/test-permission-worker-empty-execargv.js b/test/parallel/test-permission-worker-empty-execargv.js new file mode 100644 index 000000000000..23e35a031680 --- /dev/null +++ b/test/parallel/test-permission-worker-empty-execargv.js @@ -0,0 +1,98 @@ +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); +if (!isMainThread) common.skip('main thread only'); + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { spawnSync } = require('child_process'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const allowed = tmpdir.path; +const deniedFile = path.join(tmpdir.path, '..', 'permission-worker-denied-file'); +fs.writeFileSync(deniedFile, 'secret\n'); +fs.writeFileSync(path.join(allowed, 'ok.txt'), 'allowed\n'); + +function runWorker(workerSource, execArgvFragment) { + const code = ` + const { Worker } = require('worker_threads'); + const w = new Worker(${JSON.stringify(workerSource)}, { + eval: true, + ${execArgvFragment} + }); + w.on('message', (msg) => { + process.stdout.write(JSON.stringify(msg) + '\\n'); + process.exit(0); + }); + w.on('error', (err) => { console.error(err); process.exit(1); }); + `; + return spawnSync(process.execPath, [ + '--permission', + `--allow-fs-read=${allowed}`, + '--allow-worker', + '-e', + code, + ], { encoding: 'utf8', timeout: 20000 }); +} + +const readDenied = ` + const { parentPort } = require('worker_threads'); + const fs = require('fs'); + try { + parentPort.postMessage({ + ok: true, + data: fs.readFileSync(${JSON.stringify(deniedFile)}, 'utf8'), + }); + } catch (err) { + parentPort.postMessage({ ok: false, code: err.code }); + } +`; + +const readAllowed = ` + const { parentPort } = require('worker_threads'); + const fs = require('fs'); + try { + parentPort.postMessage({ + ok: true, + data: fs.readFileSync(${JSON.stringify(path.join(allowed, 'ok.txt'))}, 'utf8'), + }); + } catch (err) { + parentPort.postMessage({ ok: false, code: err.code }); + } +`; + +function lastMsg(r) { + assert.strictEqual(r.status, 0, r.stderr); + return JSON.parse(r.stdout.trim().split('\n').pop()); +} + +{ + const msg = lastMsg(runWorker(readDenied, '')); + assert.strictEqual(msg.ok, false); + assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); +} + +{ + const msg = lastMsg(runWorker(readDenied, 'execArgv: [],')); + assert.strictEqual(msg.ok, false, JSON.stringify(msg)); + assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); +} + +{ + const msg = lastMsg(runWorker(readAllowed, 'execArgv: [],')); + assert.strictEqual(msg.ok, true, JSON.stringify(msg)); +} + +{ + const frag = `execArgv: ${JSON.stringify([ + '--permission', + '--allow-fs-read=*', + '--allow-worker', + ])},`; + const msg = lastMsg(runWorker(readDenied, frag)); + assert.strictEqual(msg.ok, false, JSON.stringify(msg)); + assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); +} From 9a7e3eff13755ad53fba831a8d77472d04644ef5 Mon Sep 17 00:00:00 2001 From: yunshingng Date: Tue, 18 Aug 2026 11:09:21 -0400 Subject: [PATCH 2/5] permission: fix worker grant clamp edge cases - Normalize path boundary checks for allow-list intersection - Rewrite permission-related exec_argv to match clamped options - Preserve non-permission execArgv entries - Expand tests: --no-warnings, allow path success, repeated allows Signed-off-by: yunshingng --- src/node_worker.cc | 112 ++++++++++++++---- .../test-permission-worker-empty-execargv.js | 80 +++++++------ 2 files changed, 137 insertions(+), 55 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 539a7a89331e..9a255c997bf0 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -505,14 +505,19 @@ Worker::~Worker() { } -// When the parent has --permission enabled, explicit Worker execArgv (including -// []) must not yield a wider grant set than the parent. Enforcement is on the -// C++ side after options are parsed so NODE_OPTIONS and repeated --allow-* are -// already reflected in EnvironmentOptions (no JS process.execArgv copying). + +// Permission Model clamp for Worker explicit execArgv (including []). +// +// When the parent has --permission enabled, the worker must not receive a +// wider permission-related grant set than the parent. Implemented in C++ after +// options parse so NODE_OPTIONS and repeated --allow-* flags are already in +// EnvironmentOptions (no JS process.execArgv copying). // -// If the worker did not configure Permission Model flags at all, treat requested -// grants as unrestricted under the model so the intersection equals the parent -// grant set. If the worker did configure permission flags, intersect with parent. +// Semantics: +// - Worker did not configure permission flags → effective grants = parent grants +// - Worker configured permission flags → intersect with parent (no escalation) +// - Non-permission execArgv entries are preserved; permission-related argv is +// rewritten to match the clamped options (avoids argv/options skew). static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { if (w->permission || w->permission_audit) return true; @@ -525,15 +530,70 @@ static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { return false; } +static bool IsPermissionArg(const std::string& arg) { + return arg == "--permission" || arg == "--permission-audit" || + arg.starts_with("--allow-fs-read") || + arg.starts_with("--allow-fs-write") || + arg.starts_with("--allow-addons") || + arg.starts_with("--allow-inspector") || + arg.starts_with("--allow-child-process") || + arg.starts_with("--allow-net") || arg.starts_with("--allow-wasi") || + arg.starts_with("--allow-ffi") || + arg.starts_with("--allow-openssl-store") || + arg.starts_with("--allow-worker"); +} + +static void StripPermissionArgs(std::vector* argv) { + if (argv == nullptr) return; + std::vector out; + out.reserve(argv->size()); + for (const std::string& a : *argv) { + if (!IsPermissionArg(a)) out.push_back(a); + } + *argv = std::move(out); +} + +static void AppendPermissionArgsFromOptions( + std::vector* argv, const EnvironmentOptions* o) { + if (argv == nullptr || o == nullptr || !o->permission) return; + argv->push_back("--permission"); + if (o->permission_audit) argv->push_back("--permission-audit"); + for (const std::string& p : o->allow_fs_read) { + argv->push_back("--allow-fs-read=" + p); + } + for (const std::string& p : o->allow_fs_write) { + argv->push_back("--allow-fs-write=" + p); + } + if (o->allow_addons) argv->push_back("--allow-addons"); + if (o->allow_inspector) argv->push_back("--allow-inspector"); + if (o->allow_child_process) argv->push_back("--allow-child-process"); + if (o->allow_net) argv->push_back("--allow-net"); + if (o->allow_wasi) argv->push_back("--allow-wasi"); + if (o->allow_ffi) argv->push_back("--allow-ffi"); + if (o->allow_openssl_store) argv->push_back("--allow-openssl-store"); + if (o->allow_worker_threads) argv->push_back("--allow-worker"); +} + +// Conservative path grant check used only for intersection filtering. +// Runtime enforcement still uses FSPermission; this only prevents clearly +// wider list entries from surviving into worker options. static bool PathGrantedByParentList(const std::vector& parent_paths, const std::string& requested) { if (parent_paths.empty()) return false; - for (const std::string& p : parent_paths) { - if (p == "*" || p == requested) return true; - if (p.empty() || requested.size() < p.size()) continue; - if (requested.compare(0, p.size(), p) != 0) continue; - if (requested.size() == p.size()) return true; - if (p.back() == '/' || requested[p.size()] == '/') return true; + for (const std::string& raw_p : parent_paths) { + std::string p = raw_p; + std::string r = requested; + // Normalize trailing separators for comparison (except root). + while (p.size() > 1 && (p.back() == '/' || p.back() == '\\')) p.pop_back(); + while (r.size() > 1 && (r.back() == '/' || r.back() == '\\')) r.pop_back(); + + if (p == "*" || p == r) return true; + if (p.empty() || r.size() < p.size()) continue; + if (r.compare(0, p.size(), p) != 0) continue; + if (r.size() == p.size()) return true; + // Directory prefix: parent "/tmp" or "/tmp/" allows "/tmp/x", not "/tmpfoo". + char next = r[p.size()]; + if (next == '/' || next == '\\') return true; } return false; } @@ -560,7 +620,7 @@ static void IntersectPathList(std::vector* worker, static void CopyParentPermissionGrants(EnvironmentOptions* w, const EnvironmentOptions* parent) { - w->permission = parent->permission; + w->permission = true; w->permission_audit = parent->permission_audit; w->allow_addons = parent->allow_addons; w->allow_inspector = parent->allow_inspector; @@ -593,22 +653,32 @@ static void IntersectPermissionGrants(EnvironmentOptions* w, IntersectPathList(&w->allow_fs_write, parent->allow_fs_write); } +static EnvironmentOptions* GetPerEnvOptions(PerIsolateOptions* opts) { + if (opts == nullptr) return nullptr; + return opts->get_per_env_options(); +} + static void ClampWorkerPermissionToParent(Environment* env, - PerIsolateOptions* worker_opts) { + PerIsolateOptions* worker_opts, + std::vector* exec_argv) { if (worker_opts == nullptr || !env->permission()->enabled()) return; EnvironmentOptions* parent = env->isolate_data()->options()->get_per_env_options(); - EnvironmentOptions* w = worker_opts->get_per_env_options(); + EnvironmentOptions* w = GetPerEnvOptions(worker_opts); if (parent == nullptr || w == nullptr) return; if (!WorkerConfiguredPermission(w)) { CopyParentPermissionGrants(w, parent); - w->permission = true; - return; + } else { + IntersectPermissionGrants(w, parent); } - IntersectPermissionGrants(w, parent); + // Keep exec_argv permission flags consistent with clamped options. + if (exec_argv != nullptr) { + StripPermissionArgs(exec_argv); + AppendPermissionArgsFromOptions(exec_argv, w); + } } void Worker::New(const FunctionCallbackInfo& args) { @@ -795,8 +865,10 @@ void Worker::New(const FunctionCallbackInfo& args) { // essential to load user codes and must not be blocked by the inspector // for internal scripts. // Still, `--inspect-node` can break on the first line of internal scripts. + if (env->permission()->enabled() && per_isolate_opts) { - ClampWorkerPermissionToParent(env, per_isolate_opts.get()); + ClampWorkerPermissionToParent(env, per_isolate_opts.get(), + &exec_argv_out); } if (is_internal) { diff --git a/test/parallel/test-permission-worker-empty-execargv.js b/test/parallel/test-permission-worker-empty-execargv.js index 23e35a031680..0ff3c667661e 100644 --- a/test/parallel/test-permission-worker-empty-execargv.js +++ b/test/parallel/test-permission-worker-empty-execargv.js @@ -12,9 +12,10 @@ const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); const allowed = tmpdir.path; +const allowedFile = path.join(allowed, 'ok.txt'); const deniedFile = path.join(tmpdir.path, '..', 'permission-worker-denied-file'); +fs.writeFileSync(allowedFile, 'allowed\n'); fs.writeFileSync(deniedFile, 'secret\n'); -fs.writeFileSync(path.join(allowed, 'ok.txt'), 'allowed\n'); function runWorker(workerSource, execArgvFragment) { const code = ` @@ -38,61 +39,70 @@ function runWorker(workerSource, execArgvFragment) { ], { encoding: 'utf8', timeout: 20000 }); } -const readDenied = ` - const { parentPort } = require('worker_threads'); - const fs = require('fs'); - try { - parentPort.postMessage({ - ok: true, - data: fs.readFileSync(${JSON.stringify(deniedFile)}, 'utf8'), - }); - } catch (err) { - parentPort.postMessage({ ok: false, code: err.code }); - } -`; - -const readAllowed = ` - const { parentPort } = require('worker_threads'); - const fs = require('fs'); - try { - parentPort.postMessage({ - ok: true, - data: fs.readFileSync(${JSON.stringify(path.join(allowed, 'ok.txt'))}, 'utf8'), - }); - } catch (err) { - parentPort.postMessage({ ok: false, code: err.code }); - } -`; - function lastMsg(r) { assert.strictEqual(r.status, 0, r.stderr); return JSON.parse(r.stdout.trim().split('\n').pop()); } +function srcRead(file) { + return ` + const { parentPort } = require('worker_threads'); + const fs = require('fs'); + try { + parentPort.postMessage({ + ok: true, + data: fs.readFileSync(${JSON.stringify(file)}, 'utf8'), + }); + } catch (err) { + parentPort.postMessage({ ok: false, code: err.code }); + } + `; +} + +// default: denied blocked { - const msg = lastMsg(runWorker(readDenied, '')); + const msg = lastMsg(runWorker(srcRead(deniedFile), '')); assert.strictEqual(msg.ok, false); assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); } +// execArgv []: denied blocked, allowed readable { - const msg = lastMsg(runWorker(readDenied, 'execArgv: [],')); - assert.strictEqual(msg.ok, false, JSON.stringify(msg)); - assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); + const denied = lastMsg(runWorker(srcRead(deniedFile), 'execArgv: [],')); + assert.strictEqual(denied.ok, false, JSON.stringify(denied)); + assert.strictEqual(denied.code, 'ERR_ACCESS_DENIED'); + const ok = lastMsg(runWorker(srcRead(allowedFile), 'execArgv: [],')); + assert.strictEqual(ok.ok, true, JSON.stringify(ok)); } +// non-permission flag only: same boundary { - const msg = lastMsg(runWorker(readAllowed, 'execArgv: [],')); - assert.strictEqual(msg.ok, true, JSON.stringify(msg)); + const denied = lastMsg(runWorker(srcRead(deniedFile), 'execArgv: ["--no-warnings"],')); + assert.strictEqual(denied.ok, false); + assert.strictEqual(denied.code, 'ERR_ACCESS_DENIED'); + const ok = lastMsg(runWorker(srcRead(allowedFile), 'execArgv: ["--no-warnings"],')); + assert.strictEqual(ok.ok, true, JSON.stringify(ok)); } +// wider than parent +{ + const frag = `execArgv: ${JSON.stringify([ + '--permission', '--allow-fs-read=*', '--allow-worker', + ])},`; + const msg = lastMsg(runWorker(srcRead(deniedFile), frag)); + assert.strictEqual(msg.ok, false, JSON.stringify(msg)); + assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); +} + +// repeated allow flags in worker execArgv still cannot exceed parent { const frag = `execArgv: ${JSON.stringify([ '--permission', - '--allow-fs-read=*', + `--allow-fs-read=${allowed}`, + `--allow-fs-read=${deniedFile}`, '--allow-worker', ])},`; - const msg = lastMsg(runWorker(readDenied, frag)); + const msg = lastMsg(runWorker(srcRead(deniedFile), frag)); assert.strictEqual(msg.ok, false, JSON.stringify(msg)); assert.strictEqual(msg.code, 'ERR_ACCESS_DENIED'); } From abc762fe47d2c7751f07a21a82b5acda74c23ee2 Mon Sep 17 00:00:00 2001 From: yunshingng Date: Tue, 18 Aug 2026 11:16:07 -0400 Subject: [PATCH 3/5] permission: repair corrupted worker grant clamp block Replace merged/broken helper text with a single clean implementation: complete WorkerConfiguredPermission, one path-intersection loop, one clamp call site, and exec_argv rewrite consistent with options. Signed-off-by: yunshingng --- src/node_worker.cc | 126 ++++++++++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 52 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 9a255c997bf0..e48634bf50ce 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -506,22 +506,25 @@ Worker::~Worker() { + // Permission Model clamp for Worker explicit execArgv (including []). // // When the parent has --permission enabled, the worker must not receive a // wider permission-related grant set than the parent. Implemented in C++ after -// options parse so NODE_OPTIONS and repeated --allow-* flags are already in +// options parse so NODE_OPTIONS and repeated --allow-* are already in // EnvironmentOptions (no JS process.execArgv copying). // -// Semantics: -// - Worker did not configure permission flags → effective grants = parent grants -// - Worker configured permission flags → intersect with parent (no escalation) -// - Non-permission execArgv entries are preserved; permission-related argv is -// rewritten to match the clamped options (avoids argv/options skew). +// - Worker did not configure permission flags → effective grants = parent +// - Worker configured permission flags → intersect with parent +// - Non-permission execArgv entries preserved; permission argv rewritten static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { - if (w->permission || w->permission_audit) return true; - if (!w->allow_fs_read.empty() || !w->allow_fs_write.empty()) return true; + if (w->permission || w->permission_audit) { + return true; + } + if (!w->allow_fs_read.empty() || !w->allow_fs_write.empty()) { + return true; + } if (w->allow_addons || w->allow_inspector || w->allow_child_process || w->allow_net || w->allow_wasi || w->allow_ffi || w->allow_openssl_store || w->allow_worker_threads) { @@ -532,32 +535,41 @@ static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { static bool IsPermissionArg(const std::string& arg) { return arg == "--permission" || arg == "--permission-audit" || - arg.starts_with("--allow-fs-read") || - arg.starts_with("--allow-fs-write") || - arg.starts_with("--allow-addons") || - arg.starts_with("--allow-inspector") || - arg.starts_with("--allow-child-process") || - arg.starts_with("--allow-net") || arg.starts_with("--allow-wasi") || - arg.starts_with("--allow-ffi") || - arg.starts_with("--allow-openssl-store") || - arg.starts_with("--allow-worker"); + arg.rfind("--allow-fs-read", 0) == 0 || + arg.rfind("--allow-fs-write", 0) == 0 || + arg.rfind("--allow-addons", 0) == 0 || + arg.rfind("--allow-inspector", 0) == 0 || + arg.rfind("--allow-child-process", 0) == 0 || + arg.rfind("--allow-net", 0) == 0 || + arg.rfind("--allow-wasi", 0) == 0 || + arg.rfind("--allow-ffi", 0) == 0 || + arg.rfind("--allow-openssl-store", 0) == 0 || + arg.rfind("--allow-worker", 0) == 0; } static void StripPermissionArgs(std::vector* argv) { - if (argv == nullptr) return; + if (argv == nullptr) { + return; + } std::vector out; out.reserve(argv->size()); for (const std::string& a : *argv) { - if (!IsPermissionArg(a)) out.push_back(a); + if (!IsPermissionArg(a)) { + out.push_back(a); + } } *argv = std::move(out); } -static void AppendPermissionArgsFromOptions( - std::vector* argv, const EnvironmentOptions* o) { - if (argv == nullptr || o == nullptr || !o->permission) return; +static void AppendPermissionArgsFromOptions(std::vector* argv, + const EnvironmentOptions* o) { + if (argv == nullptr || o == nullptr || !o->permission) { + return; + } argv->push_back("--permission"); - if (o->permission_audit) argv->push_back("--permission-audit"); + if (o->permission_audit) { + argv->push_back("--permission-audit"); + } for (const std::string& p : o->allow_fs_read) { argv->push_back("--allow-fs-read=" + p); } @@ -574,33 +586,45 @@ static void AppendPermissionArgsFromOptions( if (o->allow_worker_threads) argv->push_back("--allow-worker"); } -// Conservative path grant check used only for intersection filtering. -// Runtime enforcement still uses FSPermission; this only prevents clearly -// wider list entries from surviving into worker options. static bool PathGrantedByParentList(const std::vector& parent_paths, const std::string& requested) { - if (parent_paths.empty()) return false; + if (parent_paths.empty()) { + return false; + } for (const std::string& raw_p : parent_paths) { std::string p = raw_p; std::string r = requested; - // Normalize trailing separators for comparison (except root). - while (p.size() > 1 && (p.back() == '/' || p.back() == '\\')) p.pop_back(); - while (r.size() > 1 && (r.back() == '/' || r.back() == '\\')) r.pop_back(); - - if (p == "*" || p == r) return true; - if (p.empty() || r.size() < p.size()) continue; - if (r.compare(0, p.size(), p) != 0) continue; - if (r.size() == p.size()) return true; - // Directory prefix: parent "/tmp" or "/tmp/" allows "/tmp/x", not "/tmpfoo". - char next = r[p.size()]; - if (next == '/' || next == '\\') return true; + while (p.size() > 1 && (p.back() == '/' || p.back() == '\\')) { + p.pop_back(); + } + while (r.size() > 1 && (r.back() == '/' || r.back() == '\\')) { + r.pop_back(); + } + if (p == "*" || p == r) { + return true; + } + if (p.empty() || r.size() < p.size()) { + continue; + } + if (r.compare(0, p.size(), p) != 0) { + continue; + } + if (r.size() == p.size()) { + return true; + } + const char next = r[p.size()]; + if (next == '/' || next == '\\') { + return true; + } } return false; } static void IntersectPathList(std::vector* worker, const std::vector& parent) { - if (worker == nullptr || worker->empty()) return; + if (worker == nullptr || worker->empty()) { + return; + } std::vector out; out.reserve(worker->size()); for (const std::string& wpath : *worker) { @@ -613,7 +637,9 @@ static void IntersectPathList(std::vector* worker, } continue; } - if (PathGrantedByParentList(parent, wpath)) out.push_back(wpath); + if (PathGrantedByParentList(parent, wpath)) { + out.push_back(wpath); + } } *worker = std::move(out); } @@ -638,7 +664,6 @@ static void IntersectPermissionGrants(EnvironmentOptions* w, const EnvironmentOptions* parent) { w->permission = true; w->permission_audit = w->permission_audit || parent->permission_audit; - w->allow_addons = w->allow_addons && parent->allow_addons; w->allow_inspector = w->allow_inspector && parent->allow_inspector; w->allow_child_process = w->allow_child_process && parent->allow_child_process; @@ -648,25 +673,22 @@ static void IntersectPermissionGrants(EnvironmentOptions* w, w->allow_openssl_store = w->allow_openssl_store && parent->allow_openssl_store; w->allow_worker_threads = w->allow_worker_threads && parent->allow_worker_threads; - IntersectPathList(&w->allow_fs_read, parent->allow_fs_read); IntersectPathList(&w->allow_fs_write, parent->allow_fs_write); } -static EnvironmentOptions* GetPerEnvOptions(PerIsolateOptions* opts) { - if (opts == nullptr) return nullptr; - return opts->get_per_env_options(); -} - static void ClampWorkerPermissionToParent(Environment* env, PerIsolateOptions* worker_opts, std::vector* exec_argv) { - if (worker_opts == nullptr || !env->permission()->enabled()) return; - + if (worker_opts == nullptr || !env->permission()->enabled()) { + return; + } EnvironmentOptions* parent = env->isolate_data()->options()->get_per_env_options(); - EnvironmentOptions* w = GetPerEnvOptions(worker_opts); - if (parent == nullptr || w == nullptr) return; + EnvironmentOptions* w = worker_opts->get_per_env_options(); + if (parent == nullptr || w == nullptr) { + return; + } if (!WorkerConfiguredPermission(w)) { CopyParentPermissionGrants(w, parent); @@ -674,7 +696,6 @@ static void ClampWorkerPermissionToParent(Environment* env, IntersectPermissionGrants(w, parent); } - // Keep exec_argv permission flags consistent with clamped options. if (exec_argv != nullptr) { StripPermissionArgs(exec_argv); AppendPermissionArgsFromOptions(exec_argv, w); @@ -866,6 +887,7 @@ void Worker::New(const FunctionCallbackInfo& args) { // for internal scripts. // Still, `--inspect-node` can break on the first line of internal scripts. + if (env->permission()->enabled() && per_isolate_opts) { ClampWorkerPermissionToParent(env, per_isolate_opts.get(), &exec_argv_out); From 445a62c14575339653cf668d5f7a38c688357a24 Mon Sep 17 00:00:00 2001 From: yunshingng Date: Tue, 18 Aug 2026 12:30:11 -0400 Subject: [PATCH 4/5] permission: harden worker path intersection edge cases - Normalize allow-list paths with PathResolve before prefix checks - Case-insensitive path prefix matching on Windows - Strip space-separated --allow-fs-read/--allow-fs-write path tokens from exec_argv when rewriting permission flags Signed-off-by: yunshingng --- src/node_worker.cc | 132 +++++++++++++++++++++++++++++++++------------ 1 file changed, 99 insertions(+), 33 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index e48634bf50ce..ae119e589fbd 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -11,6 +11,7 @@ #include "node_profiling.h" #include "node_snapshot_builder.h" #include "permission/permission.h" +#include "path.h" #include "util-inl.h" #include "v8-cppgc.h" #include "v8-profiler.h" @@ -507,6 +508,7 @@ Worker::~Worker() { + // Permission Model clamp for Worker explicit execArgv (including []). // // When the parent has --permission enabled, the worker must not receive a @@ -516,7 +518,9 @@ Worker::~Worker() { // // - Worker did not configure permission flags → effective grants = parent // - Worker configured permission flags → intersect with parent -// - Non-permission execArgv entries preserved; permission argv rewritten +// - Non-permission execArgv preserved; permission argv rewritten +// - Path lists: normalize via PathResolve when possible; case-insensitive +// prefix match on Windows; runtime FSPermission remains authoritative static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { if (w->permission || w->permission_audit) { @@ -547,16 +551,30 @@ static bool IsPermissionArg(const std::string& arg) { arg.rfind("--allow-worker", 0) == 0; } +// Flags that may take a separate following argv token (space form). +static bool PermissionArgTakesNext(const std::string& arg) { + return arg == "--allow-fs-read" || arg == "--allow-fs-write"; +} + static void StripPermissionArgs(std::vector* argv) { if (argv == nullptr) { return; } std::vector out; out.reserve(argv->size()); - for (const std::string& a : *argv) { - if (!IsPermissionArg(a)) { - out.push_back(a); + for (size_t i = 0; i < argv->size(); i++) { + const std::string& a = (*argv)[i]; + if (IsPermissionArg(a)) { + if (PermissionArgTakesNext(a) && i + 1 < argv->size()) { + const std::string& next = (*argv)[i + 1]; + // Skip path token if it is not another flag. + if (!next.empty() && next[0] != '-') { + i++; + } + } + continue; } + out.push_back(a); } *argv = std::move(out); } @@ -586,41 +604,87 @@ static void AppendPermissionArgsFromOptions(std::vector* argv, if (o->allow_worker_threads) argv->push_back("--allow-worker"); } -static bool PathGrantedByParentList(const std::vector& parent_paths, +static void StripTrailingSeparators(std::string* s) { + while (s->size() > 1 && (s->back() == '/' || s->back() == '\\')) { + s->pop_back(); + } +} + +static std::string NormalizeListPath(Environment* env, const std::string& in) { + if (in.empty() || in == "*") { + return in; + } + // PathResolve handles relative segments using the environment cwd when + // possible. If resolution fails for any reason, fall back to the original. + std::string resolved = PathResolve(env, std::vector{in}); + if (resolved.empty()) { + resolved = in; + } + StripTrailingSeparators(&resolved); +#ifdef _WIN32 + for (char& c : resolved) { + if (c >= 'A' && c <= 'Z') { + c = static_cast(c - 'A' + 'a'); + } + if (c == '/') { + c = '\\'; + } + } +#endif + return resolved; +} + +static bool PathPrefixGranted(const std::string& parent, + const std::string& requested) { + if (parent == "*" || parent == requested) { + return true; + } + if (parent.empty() || requested.size() < parent.size()) { + return false; + } +#ifdef _WIN32 + // Case-insensitive prefix compare for Windows path grants. + for (size_t i = 0; i < parent.size(); i++) { + char a = parent[i]; + char b = requested[i]; + if (a >= 'A' && a <= 'Z') a = static_cast(a - 'A' + 'a'); + if (b >= 'A' && b <= 'Z') b = static_cast(b - 'A' + 'a'); + if (a == '/') a = '\\'; + if (b == '/') b = '\\'; + if (a != b) { + return false; + } + } +#else + if (requested.compare(0, parent.size(), parent) != 0) { + return false; + } +#endif + if (requested.size() == parent.size()) { + return true; + } + const char next = requested[parent.size()]; + return next == '/' || next == '\\'; +} + +static bool PathGrantedByParentList(Environment* env, + const std::vector& parent_paths, const std::string& requested) { if (parent_paths.empty()) { return false; } + const std::string req = NormalizeListPath(env, requested); for (const std::string& raw_p : parent_paths) { - std::string p = raw_p; - std::string r = requested; - while (p.size() > 1 && (p.back() == '/' || p.back() == '\\')) { - p.pop_back(); - } - while (r.size() > 1 && (r.back() == '/' || r.back() == '\\')) { - r.pop_back(); - } - if (p == "*" || p == r) { - return true; - } - if (p.empty() || r.size() < p.size()) { - continue; - } - if (r.compare(0, p.size(), p) != 0) { - continue; - } - if (r.size() == p.size()) { - return true; - } - const char next = r[p.size()]; - if (next == '/' || next == '\\') { + const std::string p = NormalizeListPath(env, raw_p); + if (PathPrefixGranted(p, req)) { return true; } } return false; } -static void IntersectPathList(std::vector* worker, +static void IntersectPathList(Environment* env, + std::vector* worker, const std::vector& parent) { if (worker == nullptr || worker->empty()) { return; @@ -637,7 +701,7 @@ static void IntersectPathList(std::vector* worker, } continue; } - if (PathGrantedByParentList(parent, wpath)) { + if (PathGrantedByParentList(env, parent, wpath)) { out.push_back(wpath); } } @@ -660,7 +724,8 @@ static void CopyParentPermissionGrants(EnvironmentOptions* w, w->allow_fs_write = parent->allow_fs_write; } -static void IntersectPermissionGrants(EnvironmentOptions* w, +static void IntersectPermissionGrants(Environment* env, + EnvironmentOptions* w, const EnvironmentOptions* parent) { w->permission = true; w->permission_audit = w->permission_audit || parent->permission_audit; @@ -673,8 +738,8 @@ static void IntersectPermissionGrants(EnvironmentOptions* w, w->allow_openssl_store = w->allow_openssl_store && parent->allow_openssl_store; w->allow_worker_threads = w->allow_worker_threads && parent->allow_worker_threads; - IntersectPathList(&w->allow_fs_read, parent->allow_fs_read); - IntersectPathList(&w->allow_fs_write, parent->allow_fs_write); + IntersectPathList(env, &w->allow_fs_read, parent->allow_fs_read); + IntersectPathList(env, &w->allow_fs_write, parent->allow_fs_write); } static void ClampWorkerPermissionToParent(Environment* env, @@ -693,7 +758,7 @@ static void ClampWorkerPermissionToParent(Environment* env, if (!WorkerConfiguredPermission(w)) { CopyParentPermissionGrants(w, parent); } else { - IntersectPermissionGrants(w, parent); + IntersectPermissionGrants(env, w, parent); } if (exec_argv != nullptr) { @@ -888,6 +953,7 @@ void Worker::New(const FunctionCallbackInfo& args) { // Still, `--inspect-node` can break on the first line of internal scripts. + if (env->permission()->enabled() && per_isolate_opts) { ClampWorkerPermissionToParent(env, per_isolate_opts.get(), &exec_argv_out); From f89a4b92ed6874d01401dff34ca29ef764e4e062 Mon Sep 17 00:00:00 2001 From: yunshingng Date: Tue, 18 Aug 2026 12:36:29 -0400 Subject: [PATCH 5/5] permission: match permission CLI flags by exact name or =value Avoid treating longer distinct options that share a prefix (e.g. --allow-fs-read-extra) as permission args when stripping/rewriting exec_argv. Signed-off-by: yunshingng --- src/node_worker.cc | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index ae119e589fbd..8b1bf92f8288 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -537,21 +537,36 @@ static bool WorkerConfiguredPermission(const EnvironmentOptions* w) { return false; } +// True only for exact flag names or "flag=value". Does not match a longer +// distinct option that merely shares a prefix (e.g. --allow-fs-read-extra). +static bool IsExactPermissionFlag(const std::string& arg, const char* name) { + const size_t n = std::char_traits::length(name); + if (arg.size() < n) { + return false; + } + if (arg.compare(0, n, name) != 0) { + return false; + } + return arg.size() == n || arg[n] == '='; +} + static bool IsPermissionArg(const std::string& arg) { - return arg == "--permission" || arg == "--permission-audit" || - arg.rfind("--allow-fs-read", 0) == 0 || - arg.rfind("--allow-fs-write", 0) == 0 || - arg.rfind("--allow-addons", 0) == 0 || - arg.rfind("--allow-inspector", 0) == 0 || - arg.rfind("--allow-child-process", 0) == 0 || - arg.rfind("--allow-net", 0) == 0 || - arg.rfind("--allow-wasi", 0) == 0 || - arg.rfind("--allow-ffi", 0) == 0 || - arg.rfind("--allow-openssl-store", 0) == 0 || - arg.rfind("--allow-worker", 0) == 0; + return IsExactPermissionFlag(arg, "--permission") || + IsExactPermissionFlag(arg, "--permission-audit") || + IsExactPermissionFlag(arg, "--allow-fs-read") || + IsExactPermissionFlag(arg, "--allow-fs-write") || + IsExactPermissionFlag(arg, "--allow-addons") || + IsExactPermissionFlag(arg, "--allow-inspector") || + IsExactPermissionFlag(arg, "--allow-child-process") || + IsExactPermissionFlag(arg, "--allow-net") || + IsExactPermissionFlag(arg, "--allow-wasi") || + IsExactPermissionFlag(arg, "--allow-ffi") || + IsExactPermissionFlag(arg, "--allow-openssl-store") || + IsExactPermissionFlag(arg, "--allow-worker"); } // Flags that may take a separate following argv token (space form). +// Only the bare form (no =value) may be followed by a separate path token. static bool PermissionArgTakesNext(const std::string& arg) { return arg == "--allow-fs-read" || arg == "--allow-fs-write"; } @@ -567,7 +582,8 @@ static void StripPermissionArgs(std::vector* argv) { if (IsPermissionArg(a)) { if (PermissionArgTakesNext(a) && i + 1 < argv->size()) { const std::string& next = (*argv)[i + 1]; - // Skip path token if it is not another flag. + // Bare --allow-fs-read/--allow-fs-write may be followed by a path + // token (space-separated CLI form). Only skip one non-flag token. if (!next.empty() && next[0] != '-') { i++; }