From 50f793e2d3948de95c3eceb968cb294cc932f38b Mon Sep 17 00:00:00 2001 From: nabeel378 Date: Sun, 19 Apr 2026 15:04:25 +0500 Subject: [PATCH] feat: add --allow-env option for environment variable permissions Signed-off-by: nabeel378 --- lib/internal/process/permission.js | 1 + node.gyp | 2 + src/env.cc | 7 ++ src/node_env_var.cc | 52 +++++++++++++- src/node_options.cc | 6 ++ src/node_options.h | 1 + src/permission/env_var_permission.cc | 42 ++++++++++++ src/permission/env_var_permission.h | 35 ++++++++++ src/permission/permission.cc | 8 +++ src/permission/permission.h | 1 + src/permission/permission_base.h | 6 +- .../parallel/test-permission-env-allow-all.js | 23 +++++++ test/parallel/test-permission-env-cli.js | 68 +++++++++++++++++++ test/parallel/test-permission-env-deny-all.js | 38 +++++++++++ 14 files changed, 288 insertions(+), 2 deletions(-) create mode 100644 src/permission/env_var_permission.cc create mode 100644 src/permission/env_var_permission.h create mode 100644 test/parallel/test-permission-env-allow-all.js create mode 100644 test/parallel/test-permission-env-cli.js create mode 100644 test/parallel/test-permission-env-deny-all.js diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index 186bcf5ea6a0df..776aaba545d026 100644 --- a/lib/internal/process/permission.js +++ b/lib/internal/process/permission.js @@ -39,6 +39,7 @@ module.exports = ObjectFreeze({ '--allow-fs-write', '--allow-addons', '--allow-child-process', + '--allow-env', '--allow-net', '--allow-inspector', '--allow-wasi', diff --git a/node.gyp b/node.gyp index b245011181d660..cf1b9bb66fdf59 100644 --- a/node.gyp +++ b/node.gyp @@ -170,6 +170,7 @@ 'src/node_zlib.cc', 'src/path.cc', 'src/permission/child_process_permission.cc', + 'src/permission/env_var_permission.cc', 'src/permission/fs_permission.cc', 'src/permission/inspector_permission.cc', 'src/permission/permission.cc', @@ -304,6 +305,7 @@ 'src/node_worker.h', 'src/path.h', 'src/permission/child_process_permission.h', + 'src/permission/env_var_permission.h', 'src/permission/fs_permission.h', 'src/permission/inspector_permission.h', 'src/permission/permission.h', diff --git a/src/env.cc b/src/env.cc index 04807ffae13437..58d50e4d567953 100644 --- a/src/env.cc +++ b/src/env.cc @@ -946,6 +946,13 @@ Environment::Environment(IsolateData* isolate_data, permission()->Apply(this, {"*"}, permission::PermissionScope::kWASI); } + if (!options_->allow_env.empty()) { + permission()->Apply( + this, options_->allow_env, permission::PermissionScope::kEnvVar); + } else { + permission()->Apply(this, {}, permission::PermissionScope::kEnvVar); + } + // Implicit allow entrypoint to kFileSystemRead if (!options_->has_eval_string && !options_->force_repl) { std::string first_argv; diff --git a/src/node_env_var.cc b/src/node_env_var.cc index e94180cd659d35..d8f1de4cc5ff2e 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -4,6 +4,7 @@ #include "node_external_reference.h" #include "node_i18n.h" #include "node_process-inl.h" +#include "permission/permission.h" #include "util.h" #include // tzset(), _tzset() @@ -435,6 +436,14 @@ static Intercepted EnvGetter(Local property, return Intercepted::kYes; } CHECK(property->IsString()); + + Utf8Value key(env->isolate(), property); + if (env->permission()->enabled() && + !env->permission()->is_granted( + env, permission::PermissionScope::kEnvVar, key.ToStringView())) { + return Intercepted::kNo; + } + MaybeLocal value_string = env->env_vars()->Get(env->isolate(), property.As()); @@ -453,6 +462,16 @@ static Intercepted EnvSetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); + + if (property->IsString()) { + Utf8Value key(env->isolate(), property); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, + permission::PermissionScope::kEnvVar, + key.ToStringView(), + Intercepted::kYes); + } + // calling env->EmitProcessEnvWarning() sets a variable indicating that // warnings have been emitted. It should be called last after other // conditions leading to a warning have been met. @@ -489,6 +508,13 @@ static Intercepted EnvQuery(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { + Utf8Value key(env->isolate(), property); + if (env->permission()->enabled() && + !env->permission()->is_granted( + env, permission::PermissionScope::kEnvVar, key.ToStringView())) { + return Intercepted::kNo; + } + int32_t rc = env->env_vars()->Query(env->isolate(), property.As()); bool has_env = (rc != -1); TraceEnvVar(env, "query", property.As()); @@ -506,6 +532,13 @@ static Intercepted EnvDeleter(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { + Utf8Value key(env->isolate(), property); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, + permission::PermissionScope::kEnvVar, + key.ToStringView(), + Intercepted::kYes); + env->env_vars()->Delete(env->isolate(), property.As()); TraceEnvVar(env, "delete", property.As()); @@ -525,7 +558,24 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { Local ret; if (env->env_vars()->Enumerate(env->isolate()).ToLocal(&ret)) { - info.GetReturnValue().Set(ret); + if (env->permission()->enabled()) { + LocalVector filtered(env->isolate()); + for (uint32_t i = 0; i < ret->Length(); i++) { + Local elem; + if (!ret->Get(env->context(), i).ToLocal(&elem)) continue; + Utf8Value key(env->isolate(), elem); + if (env->permission()->is_granted( + env, + permission::PermissionScope::kEnvVar, + key.ToStringView())) { + filtered.push_back(elem); + } + } + info.GetReturnValue().Set( + Array::New(env->isolate(), filtered.data(), filtered.size())); + } else { + info.GetReturnValue().Set(ret); + } } } diff --git a/src/node_options.cc b/src/node_options.cc index 70a8824d9e05f5..52845015e00666 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -697,6 +697,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { kAllowedInEnvvar, false, OptionNamespaces::kPermissionNamespace); + AddOption("--allow-env", + "allow access to environment variables when any permissions are " + "set", + &EnvironmentOptions::allow_env, + kAllowedInEnvvar, + OptionNamespaces::kPermissionNamespace); AddOption("--experimental-repl-await", "experimental await keyword support in REPL", &EnvironmentOptions::experimental_repl_await, diff --git a/src/node_options.h b/src/node_options.h index c3187d73353105..fee33e314d6b6f 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -146,6 +146,7 @@ class EnvironmentOptions : public Options { bool allow_net = false; bool allow_wasi = false; bool allow_worker_threads = false; + std::vector allow_env; bool experimental_repl_await = true; bool experimental_vm_modules = false; bool async_context_frame = true; diff --git a/src/permission/env_var_permission.cc b/src/permission/env_var_permission.cc new file mode 100644 index 00000000000000..a5698032610dc4 --- /dev/null +++ b/src/permission/env_var_permission.cc @@ -0,0 +1,42 @@ +#include "env_var_permission.h" + +#include +#include + +namespace node { + +namespace permission { + +void EnvVarPermission::Apply(Environment* env, + const std::vector& allow, + PermissionScope scope) { + deny_all_ = true; + if (!allow.empty()) { + if (allow.size() == 1 && allow[0] == "*") { + allow_all_ = true; + return; + } + for (const std::string& arg : allow) { + allowed_env_vars_.insert(arg); + } + } +} + +bool EnvVarPermission::is_granted(Environment* env, + PermissionScope perm, + const std::string_view& param) const { + if (deny_all_) { + if (allow_all_) { + return true; + } + if (param.empty()) { + return false; + } + return allowed_env_vars_.find(std::string(param)) != + allowed_env_vars_.end(); + } + return true; +} + +} // namespace permission +} // namespace node diff --git a/src/permission/env_var_permission.h b/src/permission/env_var_permission.h new file mode 100644 index 00000000000000..71d64ba087e5b6 --- /dev/null +++ b/src/permission/env_var_permission.h @@ -0,0 +1,35 @@ +#ifndef SRC_PERMISSION_ENV_VAR_PERMISSION_H_ +#define SRC_PERMISSION_ENV_VAR_PERMISSION_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include +#include +#include +#include "permission/permission_base.h" + +namespace node { + +namespace permission { + +class EnvVarPermission final : public PermissionBase { + public: + void Apply(Environment* env, + const std::vector& allow, + PermissionScope scope) override; + bool is_granted(Environment* env, + PermissionScope perm, + const std::string_view& param = "") const override; + + private: + bool deny_all_ = false; + bool allow_all_ = false; + std::set allowed_env_vars_; +}; + +} // namespace permission + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#endif // SRC_PERMISSION_ENV_VAR_PERMISSION_H_ diff --git a/src/permission/permission.cc b/src/permission/permission.cc index f845044b18a717..1b19b9577ae6ac 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -46,6 +46,8 @@ constexpr std::string_view GetDiagnosticsChannelName(PermissionScope scope) { return "node:permission-model:wasi"; case PermissionScope::kAddon: return "node:permission-model:addon"; + case PermissionScope::kEnvVar: + return "node:permission-model:env"; default: return {}; } @@ -105,6 +107,8 @@ Permission::Permission() : enabled_(false), warning_only_(false) { std::shared_ptr wasi = std::make_shared(); std::shared_ptr net = std::make_shared(); std::shared_ptr addon = std::make_shared(); + std::shared_ptr env_var = + std::make_shared(); #define V(Name, _, __, ___) \ nodes_.insert(std::make_pair(PermissionScope::k##Name, fs)); FILESYSTEM_PERMISSIONS(V) @@ -133,6 +137,10 @@ Permission::Permission() : enabled_(false), warning_only_(false) { nodes_.insert(std::make_pair(PermissionScope::k##Name, addon)); ADDON_PERMISSIONS(V) #undef V +#define V(Name, _, __, ___) \ + nodes_.insert(std::make_pair(PermissionScope::k##Name, env_var)); + ENV_VAR_PERMISSIONS(V) +#undef V } const char* GetErrorFlagSuggestion(node::permission::PermissionScope perm) { diff --git a/src/permission/permission.h b/src/permission/permission.h index 41efd41f36a880..b10c999bd5b7ed 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -8,6 +8,7 @@ #include "node_options.h" #include "permission/addon_permission.h" #include "permission/child_process_permission.h" +#include "permission/env_var_permission.h" #include "permission/fs_permission.h" #include "permission/inspector_permission.h" #include "permission/net_permission.h" diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index 8942c16fd9bcef..04fba615adcb71 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -35,6 +35,9 @@ namespace permission { #define ADDON_PERMISSIONS(V) \ V(Addon, "addon", PermissionsRoot, "--allow-addons") +#define ENV_VAR_PERMISSIONS(V) \ + V(EnvVar, "env", PermissionsRoot, "--allow-env") + #define PERMISSIONS(V) \ FILESYSTEM_PERMISSIONS(V) \ CHILD_PROCESS_PERMISSIONS(V) \ @@ -42,7 +45,8 @@ namespace permission { WORKER_THREADS_PERMISSIONS(V) \ INSPECTOR_PERMISSIONS(V) \ NET_PERMISSIONS(V) \ - ADDON_PERMISSIONS(V) + ADDON_PERMISSIONS(V) \ + ENV_VAR_PERMISSIONS(V) #define V(name, _, __, ___) k##name, enum class PermissionScope { diff --git a/test/parallel/test-permission-env-allow-all.js b/test/parallel/test-permission-env-allow-all.js new file mode 100644 index 00000000000000..7e4e10538d575b --- /dev/null +++ b/test/parallel/test-permission-env-allow-all.js @@ -0,0 +1,23 @@ +// Flags: --permission --allow-fs-read=* --allow-env=* +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +// With --allow-env=* all env vars should be accessible +{ + assert.ok(process.permission.has('env')); + // Should not throw for any env var + const home = process.env.HOME; + const path = process.env.PATH; + process.env.TEST_PERMISSION_VAR = 'test'; + assert.strictEqual(process.env.TEST_PERMISSION_VAR, 'test'); + delete process.env.TEST_PERMISSION_VAR; + assert.strictEqual(process.env.TEST_PERMISSION_VAR, undefined); +} diff --git a/test/parallel/test-permission-env-cli.js b/test/parallel/test-permission-env-cli.js new file mode 100644 index 00000000000000..8b7a276ef2fb2d --- /dev/null +++ b/test/parallel/test-permission-env-cli.js @@ -0,0 +1,68 @@ +// Flags: --permission --allow-fs-read=* --allow-env=HOME --allow-env=PATH +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +// Guarantee the initial state +{ + assert.ok(!process.permission.has('env')); +} + +// Allowed env vars should be accessible +{ + // Reading allowed env vars should not throw + const home = process.env.HOME; + const path = process.env.PATH; + assert.ok(process.permission.has('env', 'HOME')); + assert.ok(process.permission.has('env', 'PATH')); +} + +// Disallowed env vars should return undefined (silently denied) +{ + assert.strictEqual(process.env.SECRET_KEY, undefined); +} + +// Setting a disallowed env var should throw +{ + assert.throws(() => { + process.env.NEW_VAR = 'value'; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + resource: 'NEW_VAR', + })); +} + +// Deleting a disallowed env var should throw +{ + assert.throws(() => { + delete process.env.SECRET_KEY; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + resource: 'SECRET_KEY', + })); +} + +// Querying a disallowed env var should return false (not found) +{ + assert.strictEqual('SECRET_KEY' in process.env, false); +} + +// Enumerating should only return allowed env vars +{ + const keys = Object.keys(process.env); + for (const key of keys) { + assert.ok( + key === 'HOME' || key === 'PATH', + `Unexpected env var in enumeration: ${key}` + ); + } +} diff --git a/test/parallel/test-permission-env-deny-all.js b/test/parallel/test-permission-env-deny-all.js new file mode 100644 index 00000000000000..e3be9387395c1d --- /dev/null +++ b/test/parallel/test-permission-env-deny-all.js @@ -0,0 +1,38 @@ +// Flags: --permission --allow-fs-read=* +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +// When --permission is set without --allow-env, all env access is denied +{ + assert.ok(!process.permission.has('env')); +} + +// Reading any env var should return undefined (silently denied) +{ + assert.strictEqual(process.env.HOME, undefined); + assert.strictEqual(process.env.PATH, undefined); +} + +// Setting any env var should throw +{ + assert.throws(() => { + process.env.TEST_VAR = 'value'; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + })); +} + +// Enumerating should return empty +{ + const keys = Object.keys(process.env); + assert.strictEqual(keys.length, 0); +}