From f2bb9c4934dd5314c741de272129314e481d1841 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 13 May 2023 18:09:46 +0900 Subject: [PATCH 01/14] permission: implement process.permission.has for env scope Signed-off-by: Daeyeon Jeong --- doc/api/cli.md | 1 + doc/node.1 | 3 + lib/internal/process/permission.js | 16 ++ lib/internal/process/pre_execution.js | 1 + node.gyp | 4 + src/env.cc | 5 + src/node_options.cc | 4 + src/node_options.h | 1 + src/permission/env_permission.cc | 97 +++++++++ src/permission/env_permission.h | 45 +++++ src/permission/fs_permission.h | 2 + src/permission/permission.cc | 6 + src/permission/permission.h | 1 + src/permission/permission_base.h | 5 +- src/permission/permission_util.cc | 78 ++++++++ src/permission/permission_util.h | 41 ++++ test/parallel/test-permission-env-has.js | 243 +++++++++++++++++++++++ test/parallel/test-permission-has.js | 18 ++ 18 files changed, 570 insertions(+), 1 deletion(-) create mode 100644 src/permission/env_permission.cc create mode 100644 src/permission/env_permission.h create mode 100644 src/permission/permission_util.cc create mode 100644 src/permission/permission_util.h create mode 100644 test/parallel/test-permission-env-has.js diff --git a/doc/api/cli.md b/doc/api/cli.md index dfb29cc4e20667..0e775d4d80044e 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -2101,6 +2101,7 @@ Node.js options that are allowed are: * `--allow-child-process` +* `--allow-env` * `--allow-fs-read` * `--allow-fs-write` * `--allow-worker` diff --git a/doc/node.1 b/doc/node.1 index 90d0e42e90deb4..b0b321492298c3 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -88,6 +88,9 @@ Allow spawning process when using the permission model. .It Fl -allow-worker Allow creating worker threads when using the permission model. . +.It Fl -allow-env +Allow environment variables access when using the permission model. +. .It Fl -completion-bash Print source-able bash completion script for Node.js. . diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index e400dcae9f934e..044c7e2425e8db 100644 --- a/lib/internal/process/permission.js +++ b/lib/internal/process/permission.js @@ -2,15 +2,29 @@ const { ObjectFreeze, + RegExpPrototypeExec, StringPrototypeStartsWith, } = primordials; const permission = internalBinding('permission'); const { validateString } = require('internal/validators'); const { isAbsolute, resolve } = require('path'); +const { + codes: { ERR_INVALID_ARG_VALUE }, +} = require('internal/errors'); let experimentalPermission; +function validateHasEnvReference(value, name) { + if (!RegExpPrototypeExec(/^[a-zA-Z_][a-zA-Z0-9_]*$/, value)) { + throw new ERR_INVALID_ARG_VALUE( + name, + value, + 'must start with a letter or underscore, follewed by letters, digits, or underscores', + ); + } +} + module.exports = ObjectFreeze({ __proto__: null, isEnabled() { @@ -29,6 +43,8 @@ module.exports = ObjectFreeze({ if (!isAbsolute(reference)) { reference = resolve(reference); } + } else if (StringPrototypeStartsWith(scope, 'env')) { + validateHasEnvReference(reference, 'reference'); } } diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index fc6d9ccf4f517b..6a0f371223da0f 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -536,6 +536,7 @@ function initializePermission() { '--allow-fs-write', '--allow-child-process', '--allow-worker', + '--allow-env', ]; ArrayPrototypeForEach(availablePermissionFlags, (flag) => { if (getOptionValue(flag)) { diff --git a/node.gyp b/node.gyp index 4562d23c728dc6..bacddc1ddf6b75 100644 --- a/node.gyp +++ b/node.gyp @@ -138,8 +138,10 @@ 'src/node_worker.cc', 'src/node_zlib.cc', 'src/permission/child_process_permission.cc', + 'src/permission/env_permission.cc', 'src/permission/fs_permission.cc', 'src/permission/permission.cc', + 'src/permission/permission_util.cc', 'src/permission/worker_permission.cc', 'src/pipe_wrap.cc', 'src/process_wrap.cc', @@ -257,8 +259,10 @@ 'src/node_watchdog.h', 'src/node_worker.h', 'src/permission/child_process_permission.h', + 'src/permission/env_permission.h', 'src/permission/fs_permission.h', 'src/permission/permission.h', + 'src/permission/permission_util.h', 'src/permission/worker_permission.h', 'src/pipe_wrap.h', 'src/req_wrap.h', diff --git a/src/env.cc b/src/env.cc index ff8d3952cf20a3..307245b5f2e999 100644 --- a/src/env.cc +++ b/src/env.cc @@ -813,6 +813,11 @@ Environment::Environment(IsolateData* isolate_data, permission()->Apply(options_->allow_fs_write, permission::PermissionScope::kFileSystemWrite); } + + if (!options_->allow_env.empty()) { + permission()->Apply(options_->allow_env, + permission::PermissionScope::kEnvironment); + } } } diff --git a/src/node_options.cc b/src/node_options.cc index 4e3633e5b5b8a6..5e48b4e0c61bd8 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -416,6 +416,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { &EnvironmentOptions::experimental_policy_integrity, kAllowedInEnvvar); Implies("--policy-integrity", "[has_policy_integrity_string]"); + AddOption("--allow-env", + "allow permissions to read the environment variables", + &EnvironmentOptions::allow_env, + kAllowedInEnvvar); AddOption("--allow-fs-read", "allow permissions to read the filesystem", &EnvironmentOptions::allow_fs_read, diff --git a/src/node_options.h b/src/node_options.h index e0d338f203b0c4..09f89a45b1c1ea 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -121,6 +121,7 @@ class EnvironmentOptions : public Options { std::string experimental_policy_integrity; bool has_policy_integrity_string = false; bool experimental_permission = false; + std::string allow_env; std::string allow_fs_read; std::string allow_fs_write; bool allow_child_process = false; diff --git a/src/permission/env_permission.cc b/src/permission/env_permission.cc new file mode 100644 index 00000000000000..d26f1bf88a0698 --- /dev/null +++ b/src/permission/env_permission.cc @@ -0,0 +1,97 @@ +#include "permission/env_permission.h" +#include "util.h" + +static std::string_view TrimString(const std::string_view& str) { + static const std::string whitespace = " \n\r\t"; + size_t first = str.find_first_not_of(whitespace); + if (first == std::string_view::npos) return ""; + + size_t last = str.find_last_not_of(whitespace); + return str.substr(first, last - first + 1); +} + +namespace node { + +namespace permission { + +bool EnvPermission::IsGranted(const std::string_view& param) { + bool has_wildcard_granted = granted_patterns_->Lookup("*"); + bool has_wildcard_denied = denied_patterns_->Lookup("*"); + + if (param.empty()) { + bool has_any_denied = (has_wildcard_denied || !denied_patterns_->Empty()); + return has_wildcard_granted && !has_any_denied; + } + + if (has_wildcard_denied) { + return false; + } + + if (has_wildcard_granted && denied_patterns_->Empty()) { + return true; + } + + if (denied_patterns_->Lookup(param)) { + return false; + } + + if (!has_wildcard_granted && !granted_patterns_->Lookup(param)) { + return false; + } + + return true; +} + +bool EnvPermission::Apply(const std::shared_ptr& patterns, + const std::string_view& pattern) { + const auto pos = pattern.find_first_of('*'); + if (pos != std::string_view::npos) { + if (pattern.substr(pos + 1).size() > 0) { + // Filter out this input if a wildcard is followed by any character. (e.g: + // *ODE, *ODE*, N*DE) Only a string with a wild card of the last + // character is accepted. (e.g: *, -*, NODE*) + return false; + } + } + return patterns->Insert(std::string(pattern)); +} + +void EnvPermission::Apply(const std::string& allow, PermissionScope scope) { + if (scope == PermissionScope::kEnvironment) { + for (const auto& str : SplitString(allow, ',', true)) { + const std::string_view token = TrimString(str); + if (token.front() == '-') { + Apply(denied_patterns_, token.substr(1)); + } else { + Apply(granted_patterns_, token); + } + } + } +} + +bool EnvPermission::is_granted(PermissionScope scope, + const std::string_view& param) { + if (scope == PermissionScope::kEnvironment) { + return IsGranted(param); + } + return false; +} + +bool EnvPermission::Patterns::Insert(const std::string& s) { +#ifdef _WIN32 + return SearchTree::Insert(ToUpper(s)); +#else + return SearchTree::Insert(s); +#endif +} + +bool EnvPermission::Patterns::Lookup(const std::string_view& s, + bool when_empty_return) { +#ifdef _WIN32 + return SearchTree::Lookup(ToUpper(std::string(s)), false); +#endif + return SearchTree::Lookup(s, false); +} + +} // namespace permission +} // namespace node diff --git a/src/permission/env_permission.h b/src/permission/env_permission.h new file mode 100644 index 00000000000000..8200c4971b0715 --- /dev/null +++ b/src/permission/env_permission.h @@ -0,0 +1,45 @@ +#ifndef SRC_PERMISSION_ENV_PERMISSION_H_ +#define SRC_PERMISSION_ENV_PERMISSION_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include +#include +#include "permission/permission_util.h" + +namespace node { + +namespace permission { + +class EnvPermission final : public PermissionBase { + public: + class Patterns : public SearchTree { + public: + bool Insert(const std::string& s) override; + bool Lookup(const std::string_view& s, + bool when_empty_return = false) override; + }; + + EnvPermission() + : denied_patterns_(std::make_shared()), + granted_patterns_(std::make_shared()) {} + + void Apply(const std::string& allow, PermissionScope scope) override; + bool is_granted(PermissionScope scope, + const std::string_view& param = "") override; + + private: + bool IsGranted(const std::string_view& param); + bool Apply(const std::shared_ptr& patterns, + const std::string_view& pattern); + + std::shared_ptr denied_patterns_; + std::shared_ptr granted_patterns_; +}; + +} // namespace permission + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#endif // SRC_PERMISSION_ENV_PERMISSION_H_ diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h index 8d735a9095502e..9bee1b9afcca08 100644 --- a/src/permission/fs_permission.h +++ b/src/permission/fs_permission.h @@ -123,6 +123,8 @@ class FSPermission final : public PermissionBase { void Insert(const std::string& s); bool Lookup(const std::string_view& s) { return Lookup(s, false); } bool Lookup(const std::string_view& s, bool when_empty_return); + bool Empty() { return root_node_->children.empty(); } + Node* GetRoot() { return root_node_; } private: Node* root_node_; diff --git a/src/permission/permission.cc b/src/permission/permission.cc index ee2e83ff22d7d8..908df134548365 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -79,6 +79,8 @@ Permission::Permission() : enabled_(false) { std::make_shared(); std::shared_ptr worker_t = std::make_shared(); + std::shared_ptr env_permission = + std::make_shared(); #define V(Name, _, __) \ nodes_.insert(std::make_pair(PermissionScope::k##Name, fs)); FILESYSTEM_PERMISSIONS(V) @@ -91,6 +93,10 @@ Permission::Permission() : enabled_(false) { nodes_.insert(std::make_pair(PermissionScope::k##Name, worker_t)); WORKER_THREADS_PERMISSIONS(V) #undef V +#define V(Name, _, __) \ + nodes_.insert(std::make_pair(PermissionScope::k##Name, env_permission)); + ENVIRONMENT_PERMISSIONS(V) +#undef V } void Permission::ThrowAccessDenied(Environment* env, diff --git a/src/permission/permission.h b/src/permission/permission.h index 61341ab29134f2..563406732135d5 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -6,6 +6,7 @@ #include "debug_utils.h" #include "node_options.h" #include "permission/child_process_permission.h" +#include "permission/env_permission.h" #include "permission/fs_permission.h" #include "permission/permission_base.h" #include "permission/worker_permission.h" diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index 86fefa06e65c57..cdb186f0030c1d 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -22,10 +22,13 @@ namespace permission { #define WORKER_THREADS_PERMISSIONS(V) \ V(WorkerThreads, "worker", PermissionsRoot) +#define ENVIRONMENT_PERMISSIONS(V) V(Environment, "env", PermissionsRoot) + #define PERMISSIONS(V) \ FILESYSTEM_PERMISSIONS(V) \ CHILD_PROCESS_PERMISSIONS(V) \ - WORKER_THREADS_PERMISSIONS(V) + WORKER_THREADS_PERMISSIONS(V) \ + ENVIRONMENT_PERMISSIONS(V) #define V(name, _, __) k##name, enum class PermissionScope { diff --git a/src/permission/permission_util.cc b/src/permission/permission_util.cc new file mode 100644 index 00000000000000..ccb2ba33fbe622 --- /dev/null +++ b/src/permission/permission_util.cc @@ -0,0 +1,78 @@ +#include "permission_util.h" + +namespace node { + +namespace permission { + +bool SearchTree::Insert(const std::string& s) { + tree_.Insert(s); + return true; +} + +bool SearchTree::Lookup(const std::string_view& s, bool when_empty_return) { + FSPermission::RadixTree::Node* current_node = tree_.GetRoot(); + + if (current_node->children.size() == 0) { + return when_empty_return; + } + + // Check if the root node has a wildcard. + auto it = current_node->children.find('*'); + if (it != current_node->children.end()) { + return true; + } + + unsigned int parent_node_prefix_len = current_node->prefix.length(); + const std::string path(s); + auto path_len = path.length(); + + while (true) { + if (parent_node_prefix_len == path_len && current_node->IsEndNode()) { + return true; + } + + auto node = NextNode(current_node, path, parent_node_prefix_len); + if (node == nullptr) { + return false; + } + + current_node = node; + parent_node_prefix_len += current_node->prefix.length(); + if (current_node->wildcard_child != nullptr && + path_len >= parent_node_prefix_len - 1 /* -1: * */) { + return true; + } + } + return false; +} + +FSPermission::RadixTree::Node* SearchTree::NextNode( + const FSPermission::RadixTree::Node* node, + const std::string& path, + unsigned int idx) { + if (idx >= path.length()) { + return nullptr; + } + + auto it = node->children.find(path[idx]); + if (it == node->children.end()) { + return nullptr; + } + auto child = it->second; + // match prefix + unsigned int prefix_len = child->prefix.length(); + for (unsigned int i = 0; i < path.length(); ++i) { + if (i >= prefix_len || child->prefix[i] == '*') { + return child; + } + + if (path[idx++] != child->prefix[i]) { + return nullptr; + } + } + return child; +} + +} // namespace permission + +} // namespace node diff --git a/src/permission/permission_util.h b/src/permission/permission_util.h new file mode 100644 index 00000000000000..455e3ae1655006 --- /dev/null +++ b/src/permission/permission_util.h @@ -0,0 +1,41 @@ +#ifndef SRC_PERMISSION_PERMISSION_UTIL_H_ +#define SRC_PERMISSION_PERMISSION_UTIL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include +#include "permission/fs_permission.h" + +namespace node { + +namespace permission { + +class SearchTree { + public: + virtual bool Insert(const std::string& s); + virtual bool Lookup(const std::string_view& s, + bool when_empty_return = false); + bool Empty() { return tree_.Empty(); } + + private: + // This class delegates most of its feature to FSPermission::RadixTree. The + // modified parts are as follows, while the rest of the implementations is + // from RadixTree: + // 1. Removed handling the path string in Lookup and NextNode. + // 2. Added checking if the root node has a wildcard in Lookup. + FSPermission::RadixTree::Node* NextNode( + const FSPermission::RadixTree::Node* node, + const std::string& path, + unsigned int idx); + FSPermission::RadixTree::Node* GetRoot() { return tree_.GetRoot(); } + + // TODO(daeyeon): modify the file system specific parts in RadixTree to make + // it usable for both FsPermission and EnvPermission. + FSPermission::RadixTree tree_; +}; + +} // namespace permission + +} // namespace node +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#endif // SRC_PERMISSION_PERMISSION_UTIL_H_ diff --git a/test/parallel/test-permission-env-has.js b/test/parallel/test-permission-env-has.js new file mode 100644 index 00000000000000..c35b5db8abc7cd --- /dev/null +++ b/test/parallel/test-permission-env-has.js @@ -0,0 +1,243 @@ +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +'use strict'; + +require('../common'); +const { spawnSync } = require('node:child_process'); +const { debuglog } = require('node:util'); +const { strictEqual, deepStrictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const debug = debuglog('test'); + +function runTest(args, options = {}) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--experimental-permission', '--allow-fs-read=*', ...args], + options + ); + debug('status:', status); + if (status) debug('stdout:', stdout.toString().split('\n')); + if (status) debug('stderr:', stderr.toString().split('\n')); + return { status, stdout, stderr }; +} + +describe('permission: has "env" with reference', () => { + const code = ` + const has = (...args) => console.log(process.permission.has(...args)); + + has('env', 'A1'); + has('env', 'A2'); + has('env', 'B1'); + has('env', 'B2'); + has('env', 'NODE_OPTIONS'); + `; + + it('allow one', () => { + const { status, stdout } = runTest(['--allow-env=A1', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'false', + 'false', + 'false', + 'false', + ]); + }); + + it('allow more than one', () => { + const { status, stdout } = runTest(['--allow-env=A1,A2', '-e', code]); + + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'false', + ]); + }); + + it('allow more than one using wildcard', () => { + const { status, stdout } = runTest(['--allow-env=A*', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'false', + ]); + }); + + it('allow more than one with spaces', () => { + const { status, stdout } = runTest(['--allow-env=A1, A2 ', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'false', + ]); + }); + + it('allow all', () => { + const { status, stdout } = runTest(['--allow-env=*', '-e', code]); + + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'true', + 'true', + 'true', + ]); + }); + + it('allow all except one', () => { + const { status, stdout } = runTest(['--allow-env=*,-B1', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'true', + 'true', + ]); + }); + + it('allow all except more than one', () => { + const { status, stdout } = runTest(['--allow-env=*,-B1,-B2', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'true', + ]); + }); + + it('allow all except more than one using wildcard', () => { + const { status, stdout } = runTest(['--allow-env=*,-B*', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'true', + ]); + }); + + it('allow all except more than one using wildcard (reverse order)', () => { + const { status, stdout } = runTest(['--allow-env=-B*,*', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + 'false', + 'true', + ]); + }); + + it('deny one', () => { + const { status, stdout } = runTest(['--allow-env=-B1', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'false', + 'false', + 'false', + 'false', + 'false', + ]); + }); + + it('deny all', () => { + const { status, stdout } = runTest(['--allow-env=-*', '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'false', + 'false', + 'false', + 'false', + 'false', + ]); + }); +}); + +describe('permission: has "env" with no reference', () => { + it('initial state', () => { + const { status } = runTest([ + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); + + it('has no reference with *', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + 'require("assert").strictEqual(process.permission.has("env"), true);', + ]); + strictEqual(status, 0); + }); + + it('has no reference with A*', () => { + const { status } = runTest([ + '--allow-env=A*', + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); + + it('has no reference with *,-A', () => { + const { status } = runTest([ + '--allow-env=*,-A', + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); + + it('has no reference with *,-A*', () => { + const { status } = runTest([ + '--allow-env=*,-A*', + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); + + it('has no reference with -*,*', () => { + const { status } = runTest([ + '--allow-env=-*,*', + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); +}); + +describe('permission: reference', () => { + it('reference is case-sensitive', () => { + const { status, stdout } = runTest([ + '--allow-env=FoO', + '-e', + ` + console.log(process.permission.has('env', 'FOO')); + console.log(process.permission.has('env', 'foo')); + console.log(process.permission.has('env', 'FoO')); + `, + ]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'false', + 'false', + 'true', + ]); + }); +}); diff --git a/test/parallel/test-permission-has.js b/test/parallel/test-permission-has.js index f0fb582959f721..88a00496a5db36 100644 --- a/test/parallel/test-permission-has.js +++ b/test/parallel/test-permission-has.js @@ -21,3 +21,21 @@ const assert = require('assert'); message: 'The "reference" argument must be of type string. Received an instance of Object', })); } + +{ + assert.throws(() => { + process.permission.has('env', ''); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_VALUE' + })); + assert.throws(() => { + process.permission.has('env', '0'); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_VALUE' + })); + assert.throws(() => { + process.permission.has('env', '*'); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_VALUE' + })); +} From 5daf0cba90783ac4357be65c3aacb680ba84cf71 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 15 May 2023 01:03:33 +0900 Subject: [PATCH 02/14] process: add an env proxy used by builtins only Signed-off-by: Daeyeon Jeong --- lib/child_process.js | 13 ++++++--- lib/cluster.js | 8 +++++- lib/internal/cluster/child.js | 7 ++++- lib/internal/cluster/primary.js | 9 +++++-- lib/internal/console/constructor.js | 9 +++++-- lib/internal/debugger/inspect_repl.js | 7 ++++- lib/internal/main/repl.js | 12 ++++++--- lib/internal/main/watch_mode.js | 10 ++++++- lib/internal/main/worker_thread.js | 7 ++++- lib/internal/modules/cjs/loader.js | 7 ++--- lib/internal/modules/esm/loader.js | 7 ++++- lib/internal/modules/esm/resolve.js | 7 ++++- lib/internal/options.js | 7 ++++- lib/internal/process/pre_execution.js | 29 ++++++++++++--------- lib/internal/readline/interface.js | 7 ++++- lib/internal/repl/utils.js | 9 +++++-- lib/internal/source_map/source_map_cache.js | 9 +++++-- lib/internal/test_runner/coverage.js | 13 ++++++--- lib/internal/test_runner/runner.js | 7 ++++- lib/internal/test_runner/utils.js | 9 +++++-- lib/internal/tty.js | 7 ++++- lib/internal/util/colors.js | 8 +++++- lib/internal/util/inspector.js | 7 ++++- lib/internal/worker.js | 9 +++++-- lib/net.js | 13 ++++++--- lib/os.js | 11 +++++--- lib/path.js | 7 ++++- lib/readline.js | 7 ++++- lib/repl.js | 7 ++++- src/env_properties.h | 4 ++- src/node_env_var.cc | 15 +++++++++++ src/node_realm.cc | 12 +++++++++ test/fixtures/errors/force_colors.snapshot | 8 +++--- 33 files changed, 242 insertions(+), 66 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 59c37b97672d39..69bafe1a4416fd 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -43,6 +43,11 @@ const { StringPrototypeSlice, StringPrototypeToUpperCase, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { convertToValidSignal, @@ -534,10 +539,10 @@ ObjectDefineProperty(execFile, promisify.custom, { }); function copyProcessEnvToEnv(env, name, optionEnv) { - if (process.env[name] && + if (process[env_private_symbol][name] && (!optionEnv || !ObjectPrototypeHasOwnProperty(optionEnv, name))) { - env[name] = process.env[name]; + env[name] = process[env_private_symbol][name]; } } @@ -622,7 +627,7 @@ function normalizeSpawnArguments(file, args, options) { if (typeof options.shell === 'string') file = options.shell; else - file = process.env.comspec || 'cmd.exe'; + file = process[env_private_symbol].comspec || 'cmd.exe'; // '/d /s /c' is used only for cmd.exe. if (RegExpPrototypeExec(/^(?:.*\\)?cmd(?:\.exe)?$/i, file) !== null) { args = ['/d', '/s', '/c', `"${command}"`]; @@ -647,7 +652,7 @@ function normalizeSpawnArguments(file, args, options) { ArrayPrototypeUnshift(args, file); } - const env = options.env || process.env; + const env = options.env || process[env_private_symbol]; const envPairs = []; // process.env.NODE_V8_COVERAGE always propagates, making it possible to diff --git a/lib/cluster.js b/lib/cluster.js index 7ca8b532ee243f..7fdccff60928ef 100644 --- a/lib/cluster.js +++ b/lib/cluster.js @@ -21,5 +21,11 @@ 'use strict'; -const childOrPrimary = 'NODE_UNIQUE_ID' in process.env ? 'child' : 'primary'; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); + +const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary'; module.exports = require(`internal/cluster/${childOrPrimary}`); diff --git a/lib/internal/cluster/child.js b/lib/internal/cluster/child.js index 1bddd3ca0ac103..8450c6e643cd9d 100644 --- a/lib/internal/cluster/child.js +++ b/lib/internal/cluster/child.js @@ -8,6 +8,11 @@ const { SafeMap, SafeSet, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const assert = require('internal/assert'); const path = require('path'); @@ -34,7 +39,7 @@ cluster.Worker = Worker; cluster._setupWorker = function() { const worker = new Worker({ - id: +process.env.NODE_UNIQUE_ID | 0, + id: +process[env_private_symbol].NODE_UNIQUE_ID | 0, process: process, state: 'online', }); diff --git a/lib/internal/cluster/primary.js b/lib/internal/cluster/primary.js index 945f440cd19797..a8084053545715 100644 --- a/lib/internal/cluster/primary.js +++ b/lib/internal/cluster/primary.js @@ -14,6 +14,11 @@ const { ERR_SOCKET_BAD_PORT, }, } = require('internal/errors'); +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const assert = require('internal/assert'); const { fork } = require('child_process'); @@ -45,7 +50,7 @@ let ids = 0; let initialized = false; // XXX(bnoordhuis) Fold cluster.schedulingPolicy into cluster.settings? -let schedulingPolicy = process.env.NODE_CLUSTER_SCHED_POLICY; +let schedulingPolicy = process[env_private_symbol].NODE_CLUSTER_SCHED_POLICY; if (schedulingPolicy === 'rr') schedulingPolicy = SCHED_RR; else if (schedulingPolicy === 'none') @@ -116,7 +121,7 @@ function setupSettingsNT(settings) { } function createWorkerProcess(id, env) { - const workerEnv = { ...process.env, ...env, NODE_UNIQUE_ID: `${id}` }; + const workerEnv = { ...process[env_private_symbol], ...env, NODE_UNIQUE_ID: `${id}` }; const execArgv = [...cluster.settings.execArgv]; if (cluster.settings.inspectPort === null) { diff --git a/lib/internal/console/constructor.js b/lib/internal/console/constructor.js index 44fa0f32d76ff3..1a6b08fb81499c 100644 --- a/lib/internal/console/constructor.js +++ b/lib/internal/console/constructor.js @@ -52,7 +52,12 @@ const { validateInteger, validateObject, } = require('internal/validators'); -const { previewEntries } = internalBinding('util'); +const { + previewEntries, + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { Buffer: { isBuffer } } = require('buffer'); const { inspect, @@ -442,7 +447,7 @@ const consoleMethods = { clear() { // It only makes sense to clear if _stdout is a TTY. // Otherwise, do nothing. - if (this._stdout.isTTY && process.env.TERM !== 'dumb') { + if (this._stdout.isTTY && process[env_private_symbol].TERM !== 'dumb') { // The require is here intentionally to avoid readline being // required too early when console is first loaded. const { diff --git a/lib/internal/debugger/inspect_repl.js b/lib/internal/debugger/inspect_repl.js index 7fba613f09c632..2c5f41313f2b80 100644 --- a/lib/internal/debugger/inspect_repl.js +++ b/lib/internal/debugger/inspect_repl.js @@ -43,6 +43,11 @@ const { StringPrototypeToUpperCase, StringPrototypeTrim, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { ERR_DEBUGGER_ERROR } = require('internal/errors').codes; @@ -887,7 +892,7 @@ function createRepl(inspector) { } Debugger.on('paused', ({ callFrames, reason /* , hitBreakpoints */ }) => { - if (process.env.NODE_INSPECT_RESUME_ON_START === '1' && + if (process[env_private_symbol].NODE_INSPECT_RESUME_ON_START === '1' && reason === 'Break on start') { debuglog('Paused on start, but NODE_INSPECT_RESUME_ON_START' + ' environment variable is set to 1, resuming'); diff --git a/lib/internal/main/repl.js b/lib/internal/main/repl.js index da1764a9c80d95..48ec5bf0bef35c 100644 --- a/lib/internal/main/repl.js +++ b/lib/internal/main/repl.js @@ -18,14 +18,20 @@ const { getOptionValue } = require('internal/options'); const { exitCodes: { kInvalidCommandLineArgument } } = internalBinding('errors'); +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); + prepareMainThreadExecution(); markBootstrapComplete(); -if (process.env.NODE_REPL_EXTERNAL_MODULE) { +if (process[env_private_symbol].NODE_REPL_EXTERNAL_MODULE) { require('internal/modules/cjs/loader') .Module - ._load(process.env.NODE_REPL_EXTERNAL_MODULE, undefined, true); + ._load(process[env_private_symbol].NODE_REPL_EXTERNAL_MODULE, undefined, true); } else { // --input-type flag not supported in REPL if (getOptionValue('--input-type')) { @@ -41,7 +47,7 @@ if (process.env.NODE_REPL_EXTERNAL_MODULE) { 'Type ".help" for more information.'); const cliRepl = require('internal/repl'); - cliRepl.createInternalRepl(process.env, (err, repl) => { + cliRepl.createInternalRepl(process[env_private_symbol], (err, repl) => { if (err) { throw err; } diff --git a/lib/internal/main/watch_mode.js b/lib/internal/main/watch_mode.js index 5d0d29cc2ff9da..98a11392459948 100644 --- a/lib/internal/main/watch_mode.js +++ b/lib/internal/main/watch_mode.js @@ -8,6 +8,11 @@ const { ArrayPrototypeSlice, StringPrototypeStartsWith, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { prepareMainThreadExecution, @@ -54,7 +59,10 @@ let exited; function start() { exited = false; const stdio = kShouldFilterModules ? ['inherit', 'inherit', 'inherit', 'ipc'] : 'inherit'; - child = spawn(process.execPath, args, { stdio, env: { ...process.env, WATCH_REPORT_DEPENDENCIES: '1' } }); + child = spawn(process.execPath, args, { + stdio, + env: { ...process[env_private_symbol], WATCH_REPORT_DEPENDENCIES: '1' }, + }); watcher.watchChildProcessModules(child); child.once('exit', (code) => { exited = true; diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index 12ae4a9b23212d..ffb0eb134ca3a0 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -16,6 +16,11 @@ const { SharedArrayBuffer, }, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { prepareWorkerThreadExecution, @@ -65,7 +70,7 @@ const port = getEnvMessagePort(); // If the main thread is spawned with env NODE_CHANNEL_FD, it's probably // spawned by our child_process module. In the work threads, mark the // related IPC properties as unavailable. -if (process.env.NODE_CHANNEL_FD) { +if (process[env_private_symbol].NODE_CHANNEL_FD) { const workerThreadSetup = require('internal/process/worker_thread_only'); ObjectDefineProperty(process, 'channel', { __proto__: null, diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 60fa7e79d19693..77798cf0c4d923 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -96,6 +96,7 @@ const { internalModuleStat } = internalBinding('fs'); const { safeGetenv } = internalBinding('credentials'); const { privateSymbols: { + env_private_symbol, require_private_symbol, }, } = internalBinding('util'); @@ -113,7 +114,7 @@ const { getOptionValue, getEmbedderOptions } = require('internal/options'); const policy = getLazy( () => (getOptionValue('--experimental-policy') ? require('internal/process/policy') : null), ); -const shouldReportRequiredModules = getLazy(() => process.env.WATCH_REPORT_DEPENDENCIES); +const shouldReportRequiredModules = getLazy(() => process[env_private_symbol].WATCH_REPORT_DEPENDENCIES); const getCascadedLoader = getLazy( () => require('internal/process/esm_loader').esmLoader, @@ -1383,8 +1384,8 @@ function createRequire(filename) { Module.createRequire = createRequire; Module._initPaths = function() { - const homeDir = isWindows ? process.env.USERPROFILE : safeGetenv('HOME'); - const nodePath = isWindows ? process.env.NODE_PATH : safeGetenv('NODE_PATH'); + const homeDir = isWindows ? process[env_private_symbol].USERPROFILE : safeGetenv('HOME'); + const nodePath = isWindows ? process[env_private_symbol].NODE_PATH : safeGetenv('NODE_PATH'); // process.execPath is $PREFIX/bin/node except on Windows where it is // $PREFIX\node.exe where $PREFIX is the root of the Node.js installation. diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 9d94b7275fdbdd..68fe46c928959f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -12,6 +12,11 @@ const { SafePromiseAllReturnArrayLike, SafeWeakMap, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { ERR_UNKNOWN_MODULE_FORMAT, @@ -211,7 +216,7 @@ class DefaultModuleLoader { getOptionValue('--inspect-brk') ); - if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { + if (process[env_private_symbol].WATCH_REPORT_DEPENDENCIES && process.send) { process.send({ 'watch:import': [url] }); } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 21703d63f6aa41..87ec2b34ce7d9a 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -23,6 +23,11 @@ const { StringPrototypeSplit, StringPrototypeStartsWith, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const internalFS = require('internal/fs/utils'); const { BuiltinModule } = require('internal/bootstrap/realm'); const { realpathSync } = require('fs'); @@ -220,7 +225,7 @@ function finalizeResolution(resolved, base, preserveSymlinks) { throw err; } else if (stats !== 0) { // Check for !stats.isFile() - if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { + if (process[env_private_symbol].WATCH_REPORT_DEPENDENCIES && process.send) { process.send({ 'watch:require': [path || resolved.pathname] }); } throw new ERR_MODULE_NOT_FOUND( diff --git a/lib/internal/options.js b/lib/internal/options.js index effe3249888efd..53e23d0fea7a82 100644 --- a/lib/internal/options.js +++ b/lib/internal/options.js @@ -4,6 +4,11 @@ const { getCLIOptions, getEmbedderOptions: getEmbedderOptionsFromBinding, } = internalBinding('options'); +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); let warnOnAllowUnauthorized = true; @@ -51,7 +56,7 @@ function getOptionValue(optionName) { } function getAllowUnauthorized() { - const allowUnauthorized = process.env.NODE_TLS_REJECT_UNAUTHORIZED === '0'; + const allowUnauthorized = process[env_private_symbol].NODE_TLS_REJECT_UNAUTHORIZED === '0'; if (allowUnauthorized && warnOnAllowUnauthorized) { warnOnAllowUnauthorized = false; diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 6a0f371223da0f..3e31cc9787b3c8 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -10,6 +10,11 @@ const { StringPrototypeStartsWith, globalThis, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { getOptionValue, @@ -204,7 +209,7 @@ function setupWarningHandler() { resetForSerialization, } = require('internal/process/warning'); if (getOptionValue('--warnings') && - process.env.NODE_NO_WARNINGS !== '1') { + process[env_private_symbol].NODE_NO_WARNINGS !== '1') { process.on('warning', onWarning); // The code above would add the listener back during deserialization, @@ -308,10 +313,10 @@ function setupCodeCoverage() { // to child processes even when they switch cwd. Don't do anything if the // --experimental-test-coverage flag is present, as the test runner will // handle coverage. - if (process.env.NODE_V8_COVERAGE && + if (process[env_private_symbol].NODE_V8_COVERAGE && !getOptionValue('--experimental-test-coverage')) { - process.env.NODE_V8_COVERAGE = - setupCoverageHooks(process.env.NODE_V8_COVERAGE); + process[env_private_symbol].NODE_V8_COVERAGE = + setupCoverageHooks(process[env_private_symbol].NODE_V8_COVERAGE); } } @@ -349,7 +354,7 @@ function initializeReport() { } function setupDebugEnv() { - require('internal/util/debuglog').initializeDebugEnv(process.env.NODE_DEBUG); + require('internal/util/debuglog').initializeDebugEnv(process[env_private_symbol].NODE_DEBUG); if (getOptionValue('--expose-internals')) { require('internal/bootstrap/realm').BuiltinModule.exposeInternals(); } @@ -476,18 +481,18 @@ function initializeDeprecations() { } function setupChildProcessIpcChannel() { - if (process.env.NODE_CHANNEL_FD) { + if (process[env_private_symbol].NODE_CHANNEL_FD) { const assert = require('internal/assert'); - const fd = NumberParseInt(process.env.NODE_CHANNEL_FD, 10); + const fd = NumberParseInt(process[env_private_symbol].NODE_CHANNEL_FD, 10); assert(fd >= 0); // Make sure it's not accidentally inherited by child processes. - delete process.env.NODE_CHANNEL_FD; + delete process[env_private_symbol].NODE_CHANNEL_FD; const serializationMode = - process.env.NODE_CHANNEL_SERIALIZATION_MODE || 'json'; - delete process.env.NODE_CHANNEL_SERIALIZATION_MODE; + process[env_private_symbol].NODE_CHANNEL_SERIALIZATION_MODE || 'json'; + delete process[env_private_symbol].NODE_CHANNEL_SERIALIZATION_MODE; require('child_process')._forkChild(fd, serializationMode); assert(process.send); @@ -495,11 +500,11 @@ function setupChildProcessIpcChannel() { } function initializeClusterIPC() { - if (process.argv[1] && process.env.NODE_UNIQUE_ID) { + if (process.argv[1] && process[env_private_symbol].NODE_UNIQUE_ID) { const cluster = require('cluster'); cluster._setupWorker(); // Make sure it's not accidentally inherited by child processes. - delete process.env.NODE_UNIQUE_ID; + delete process[env_private_symbol].NODE_UNIQUE_ID; } } diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index df08875cc79ae6..7ffd8db04a2c96 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -31,6 +31,11 @@ const { SymbolAsyncIterator, SafeStringIterator, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { codes } = require('internal/errors'); @@ -383,7 +388,7 @@ class Interface extends InterfaceConstructor { */ prompt(preserveCursor) { if (this.paused) this.resume(); - if (this.terminal && process.env.TERM !== 'dumb') { + if (this.terminal && process[env_private_symbol].TERM !== 'dumb') { if (!preserveCursor) this.cursor = 0; this[kRefreshLine](); } else { diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js index f3697aa9b68ae2..4160742abb6d6b 100644 --- a/lib/internal/repl/utils.js +++ b/lib/internal/repl/utils.js @@ -20,6 +20,11 @@ const { StringPrototypeTrim, Symbol, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { tokTypes: tt, Parser: AcornParser } = require('internal/deps/acorn/acorn/dist/acorn'); @@ -142,7 +147,7 @@ function isRecoverableError(e, code) { function setupPreview(repl, contextSymbol, bufferSymbol, active) { // Simple terminals can't handle previews. - if (process.env.TERM === 'dumb' || !active) { + if (process[env_private_symbol].TERM === 'dumb' || !active) { return { showPreview() {}, clearPreview() {} }; } @@ -512,7 +517,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { function setupReverseSearch(repl) { // Simple terminals can't use reverse search. - if (process.env.TERM === 'dumb') { + if (process[env_private_symbol].TERM === 'dumb') { return { reverseSearch() { return false; } }; } diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index ee1f9c404e8b6f..92af8df02bb08f 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -9,6 +9,11 @@ const { SafeMap, StringPrototypeSplit, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); // See https://sourcemaps.info/spec.html for SourceMap V3 specification. const { Buffer } = require('buffer'); @@ -107,7 +112,7 @@ function extractSourceMapURLMagicComment(content) { function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource, sourceURL, sourceMapURL) { const sourceMapsEnabled = getSourceMapsEnabled(); - if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return; + if (!(process[env_private_symbol].NODE_V8_COVERAGE || sourceMapsEnabled)) return; try { const { normalizeReferrerURL } = require('internal/modules/helpers'); filename = normalizeReferrerURL(filename); @@ -172,7 +177,7 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSo function maybeCacheGeneratedSourceMap(content) { const sourceMapsEnabled = getSourceMapsEnabled(); - if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return; + if (!(process[env_private_symbol].NODE_V8_COVERAGE || sourceMapsEnabled)) return; const sourceURL = extractSourceURLMagicComment(content); if (sourceURL === null) { diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js index 5a098f71af633b..74bf2f5c9ecd79 100644 --- a/lib/internal/test_runner/coverage.js +++ b/lib/internal/test_runner/coverage.js @@ -14,6 +14,11 @@ const { StringPrototypeLocaleCompare, StringPrototypeStartsWith, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { copyFileSync, mkdirSync, @@ -242,11 +247,11 @@ class TestCoverage { // Restore the original value of process.env.NODE_V8_COVERAGE. Then, copy // all of the created coverage files to the original coverage directory. if (this.originalCoverageDirectory === undefined) { - delete process.env.NODE_V8_COVERAGE; + delete process[env_private_symbol].NODE_V8_COVERAGE; return; } - process.env.NODE_V8_COVERAGE = this.originalCoverageDirectory; + process[env_private_symbol].NODE_V8_COVERAGE = this.originalCoverageDirectory; let dir; try { @@ -275,7 +280,7 @@ function sortCoverageFiles(a, b) { } function setupCoverage() { - let originalCoverageDirectory = process.env.NODE_V8_COVERAGE; + let originalCoverageDirectory = process[env_private_symbol].NODE_V8_COVERAGE; const cwd = process.cwd(); if (originalCoverageDirectory) { @@ -296,7 +301,7 @@ function setupCoverage() { // Ensure that NODE_V8_COVERAGE is set so that coverage can propagate to // child processes. - process.env.NODE_V8_COVERAGE = coverageDirectory; + process[env_private_symbol].NODE_V8_COVERAGE = coverageDirectory; return new TestCoverage(coverageDirectory, originalCoverageDirectory, cwd); } diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 056955f04566c3..7e2d975610da59 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -27,6 +27,11 @@ const { TypedArrayPrototypeGetLength, TypedArrayPrototypeSubarray, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { spawn } = require('child_process'); const { readdirSync, statSync } = require('fs'); @@ -328,7 +333,7 @@ function runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns) { const subtest = root.createSubtest(FileTest, path, async (t) => { const args = getRunArgs({ path, inspectPort, testNamePatterns }); const stdio = ['pipe', 'pipe', 'pipe']; - const env = { ...process.env, NODE_TEST_CONTEXT: 'child-v8' }; + const env = { ...process[env_private_symbol], NODE_TEST_CONTEXT: 'child-v8' }; if (filesWatcher) { stdio.push('ipc'); env.WATCH_REPORT_DEPENDENCIES = '1'; diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index de9d4e5a1e4345..70bf9248cc0b56 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -10,6 +10,11 @@ const { RegExpPrototypeExec, SafeMap, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { basename, relative } = require('path'); const { createWriteStream } = require('fs'); @@ -170,8 +175,8 @@ function parseCommandLine() { const isTestRunner = getOptionValue('--test'); const coverage = getOptionValue('--experimental-test-coverage'); - const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child'; - const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8'; + const isChildProcess = process[env_private_symbol].NODE_TEST_CONTEXT === 'child'; + const isChildProcessV8 = process[env_private_symbol].NODE_TEST_CONTEXT === 'child-v8'; let destinations; let reporters; let testNamePatterns; diff --git a/lib/internal/tty.js b/lib/internal/tty.js index 4d3c768f65b6cd..0ccb5f18a8e872 100644 --- a/lib/internal/tty.js +++ b/lib/internal/tty.js @@ -30,6 +30,11 @@ const { } = primordials; const { validateInteger } = require('internal/validators'); +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); let OSRelease; @@ -103,7 +108,7 @@ function warnOnDeactivatedColors(env) { // The `getColorDepth` API got inspired by multiple sources such as // https://github.com/chalk/supports-color, // https://github.com/isaacs/color-support. -function getColorDepth(env = process.env) { +function getColorDepth(env = process[env_private_symbol]) { // Use level 0-3 to support the same levels as `chalk` does. This is done for // consistency throughout the ecosystem. if (env.FORCE_COLOR !== undefined) { diff --git a/lib/internal/util/colors.js b/lib/internal/util/colors.js index 9e1ed05d88130e..77c745b29d1fe8 100644 --- a/lib/internal/util/colors.js +++ b/lib/internal/util/colors.js @@ -1,5 +1,11 @@ 'use strict'; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); + let internalTTy; function lazyInternalTTY() { internalTTy ??= require('internal/tty'); @@ -15,7 +21,7 @@ module.exports = { clear: '', hasColors: false, shouldColorize(stream) { - if (process.env.FORCE_COLOR !== undefined) { + if (process[env_private_symbol].FORCE_COLOR !== undefined) { return lazyInternalTTY().getColorDepth() > 2; } return stream?.isTTY && ( diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js index 0d9580c83224e4..092f3b7802e379 100644 --- a/lib/internal/util/inspector.js +++ b/lib/internal/util/inspector.js @@ -10,6 +10,11 @@ const { RegExpPrototypeExec, SafeWeakMap, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { validatePort } = require('internal/validators'); @@ -23,7 +28,7 @@ function isUsingInspector(execArgv = process.execArgv) { if (!_isUsingInspector.has(execArgv)) { _isUsingInspector.set(execArgv, ArrayPrototypeSome(execArgv, (arg) => RegExpPrototypeExec(kInspectArgRegex, arg) !== null) || - RegExpPrototypeExec(kInspectArgRegex, process.env.NODE_OPTIONS) !== null); + RegExpPrototypeExec(kInspectArgRegex, process[env_private_symbol].NODE_OPTIONS) !== null); } return _isUsingInspector.get(execArgv); } diff --git a/lib/internal/worker.js b/lib/internal/worker.js index eda9dbcdeb5c04..be5cb8872bb40f 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -23,6 +23,11 @@ const { Uint32Array, globalThis: { Atomics, SharedArrayBuffer }, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const EventEmitter = require('events'); const assert = require('internal/assert'); @@ -192,7 +197,7 @@ class Worker extends EventEmitter { ({ 0: key, 1: value }) => { env[key] = `${value}`; }, ); } else if (options.env == null) { - env = process.env; + env = process[env_private_symbol]; } else if (options.env !== SHARE_ENV) { throw new ERR_INVALID_ARG_TYPE( 'options.env', @@ -209,7 +214,7 @@ class Worker extends EventEmitter { debug('instantiating Worker.', `url: ${url}`, `doEval: ${doEval}`); // Set up the C++ handle for the worker, as well as some internal wiring. this[kHandle] = new WorkerImpl(url, - env === process.env ? null : env, + env === process[env_private_symbol] ? null : env, options.execArgv, parseResourceLimits(options.resourceLimits), !!(options.trackUnmanagedFds ?? true), diff --git a/lib/net.js b/lib/net.js index 0c12ff72aa3c50..8c842cf94edbaf 100644 --- a/lib/net.js +++ b/lib/net.js @@ -59,7 +59,12 @@ const { } = internalBinding('uv'); const { Buffer } = require('buffer'); -const { guessHandleType } = internalBinding('util'); +const { + guessHandleType, + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { ShutdownWrap } = internalBinding('stream_wrap'); const { TCP, @@ -1744,7 +1749,7 @@ function createServerHandle(address, port, addressType, fd, flags) { } else if (port === -1 && addressType === -1) { handle = new Pipe(PipeConstants.SERVER); if (isWindows) { - const instances = NumberParseInt(process.env.NODE_PENDING_PIPE_INSTANCES); + const instances = NumberParseInt(process[env_private_symbol].NODE_PENDING_PIPE_INSTANCES); if (!NumberIsNaN(instances)) { handle.setPendingInstances(instances); } @@ -2296,8 +2301,8 @@ if (isWindows) { } if (simultaneousAccepts === undefined) { - simultaneousAccepts = (process.env.NODE_MANY_ACCEPTS && - process.env.NODE_MANY_ACCEPTS !== '0'); + simultaneousAccepts = (process[env_private_symbol].NODE_MANY_ACCEPTS && + process[env_private_symbol].NODE_MANY_ACCEPTS !== '0'); } if (handle._simultaneousAccepts !== simultaneousAccepts) { diff --git a/lib/os.js b/lib/os.js index 34391697b5891c..d0e5e9512be1f4 100644 --- a/lib/os.js +++ b/lib/os.js @@ -30,6 +30,11 @@ const { StringPrototypeSlice, SymbolToPrimitive, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { safeGetenv } = internalBinding('credentials'); const constants = internalBinding('constants').os; @@ -181,9 +186,9 @@ platform[SymbolToPrimitive] = () => process.platform; function tmpdir() { let path; if (isWindows) { - path = process.env.TEMP || - process.env.TMP || - (process.env.SystemRoot || process.env.windir) + '\\temp'; + path = process[env_private_symbol].TEMP || + process[env_private_symbol].TMP || + (process[env_private_symbol].SystemRoot || process[env_private_symbol].windir) + '\\temp'; if (path.length > 1 && StringPrototypeEndsWith(path, '\\') && !StringPrototypeEndsWith(path, ':\\')) path = StringPrototypeSlice(path, 0, -1); diff --git a/lib/path.js b/lib/path.js index 1a2b3e38eca03f..9bdf514a251fe3 100644 --- a/lib/path.js +++ b/lib/path.js @@ -30,6 +30,11 @@ const { StringPrototypeSlice, StringPrototypeToLowerCase, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { CHAR_UPPERCASE_A, @@ -182,7 +187,7 @@ const win32 = { // absolute path, get cwd for that drive, or the process cwd if // the drive cwd is not available. We're sure the device is not // a UNC path at this points, because UNC paths are always absolute. - path = process.env[`=${resolvedDevice}`] || process.cwd(); + path = process[env_private_symbol][`=${resolvedDevice}`] || process.cwd(); // Verify that a cwd was found and that it actually points // to our drive. If not, default to the drive's root. diff --git a/lib/readline.js b/lib/readline.js index b9c6f17c52b4b0..64f81a1707c04f 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -31,6 +31,11 @@ const { PromiseReject, StringPrototypeSlice, } = primordials; +const { + privateSymbols: { + env_private_symbol, + }, +} = internalBinding('util'); const { clearLine, @@ -113,7 +118,7 @@ function Interface(input, output, completer, terminal) { FunctionPrototypeCall(InterfaceConstructor, this, input, output, completer, terminal); - if (process.env.TERM === 'dumb') { + if (process[env_private_symbol].TERM === 'dumb') { this._ttyWrite = FunctionPrototypeBind(_ttyWriteDumb, this); } } diff --git a/lib/repl.js b/lib/repl.js index df038901d7834a..bebbf1b00f4025 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -171,6 +171,9 @@ const { SKIP_SYMBOLS, }, getOwnNonIndexProperties, + privateSymbols: { + env_private_symbol, + }, } = internalBinding('util'); const { startSigintWatchdog, @@ -271,7 +274,9 @@ function REPLServer(prompt, if (options.terminal && options.useColors === undefined) { // If possible, check if stdout supports colors or not. - options.useColors = shouldColorize(options.output) || process.env.NODE_DISABLE_COLORS === undefined; + options.useColors = + shouldColorize(options.output) || + process[env_private_symbol].NODE_DISABLE_COLORS === undefined; } // TODO(devsnek): Add a test case for custom eval functions. diff --git a/src/env_properties.h b/src/env_properties.h index 3ef589397dbdcc..25d12f72ee53b4 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -25,7 +25,8 @@ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ V(exit_info_private_symbol, "node:exit_info_private_symbol") \ - V(require_private_symbol, "node:require_private_symbol") + V(require_private_symbol, "node:require_private_symbol") \ + V(env_private_symbol, "node:env_private_symbol") // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. @@ -342,6 +343,7 @@ V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(crypto_key_object_handle_constructor, v8::FunctionTemplate) \ V(env_proxy_template, v8::ObjectTemplate) \ + V(env_privileged_proxy_template, v8::ObjectTemplate) \ V(env_proxy_ctor_template, v8::FunctionTemplate) \ V(dir_instance_template, v8::ObjectTemplate) \ V(fd_constructor_template, v8::ObjectTemplate) \ diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 94936ea1c2bd22..b748cbd4fab517 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -475,6 +475,21 @@ void CreateEnvProxyTemplate(Isolate* isolate, IsolateData* isolate_data) { PropertyHandlerFlags::kHasNoSideEffect)); isolate_data->set_env_proxy_template(env_proxy_template); isolate_data->set_env_proxy_ctor_template(env_proxy_ctor_template); + + Local env_privileged_proxy_template = + ObjectTemplate::New(isolate); + env_privileged_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( + EnvGetter, + EnvSetter, + EnvQuery, + EnvDeleter, + EnvEnumerator, + EnvDefiner, + nullptr, + True(isolate), + PropertyHandlerFlags::kHasNoSideEffect)); + isolate_data->set_env_privileged_proxy_template( + env_privileged_proxy_template); } void RegisterEnvVarExternalReferences(ExternalReferenceRegistry* registry) { diff --git a/src/node_realm.cc b/src/node_realm.cc index 80b2f9d2784611..c003789bf575b3 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -332,6 +332,7 @@ MaybeLocal PrincipalRealm::BootstrapRealm() { return MaybeLocal(); } + Local ctx = context(); Local env_string = FIXED_ONE_BYTE_STRING(isolate_, "env"); Local env_proxy; CreateEnvProxyTemplate(isolate_, env_->isolate_data()); @@ -340,6 +341,17 @@ MaybeLocal PrincipalRealm::BootstrapRealm() { return MaybeLocal(); } + Local env_privileged_proxy; + if (env_->env_privileged_proxy_template()->NewInstance(ctx).ToLocal( + &env_privileged_proxy)) { + // process[env_private_symbol] + if (process_object() + ->SetPrivate(ctx, env_->env_private_symbol(), env_privileged_proxy) + .IsNothing()) { + return MaybeLocal(); + } + } + return v8::True(isolate_); } diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot index a38745e396a8d9..8738e3391a9cb3 100644 --- a/test/fixtures/errors/force_colors.snapshot +++ b/test/fixtures/errors/force_colors.snapshot @@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace') Error: Should include grayed stack trace at Object. (/test*force_colors.js:1:7) - at Module._compile (node:internal*modules*cjs*loader:1255:14) - at Module._extensions..js (node:internal*modules*cjs*loader:1309:10) - at Module.load (node:internal*modules*cjs*loader:1113:32) - at Module._load (node:internal*modules*cjs*loader:960:12) + at Module._compile (node:internal*modules*cjs*loader:1256:14) + at Module._extensions..js (node:internal*modules*cjs*loader:1310:10) + at Module.load (node:internal*modules*cjs*loader:1114:32) + at Module._load (node:internal*modules*cjs*loader:961:12)  at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)  at node:internal*main*run_main_module:23:47 From 9538af6689ad6d45c9221e546403c189a300ace8 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 18 May 2023 14:31:32 +0900 Subject: [PATCH 03/14] permission: check env permission in proxy traps and clone Signed-off-by: Daeyeon Jeong --- src/node_env_var.cc | 74 ++++++- src/node_messaging.cc | 25 ++- src/permission/permission.h | 2 + .../test-permission-env-access-enum-clone.js | 108 +++++++++++ .../test-permission-env-access-windows.js | 58 ++++++ .../test-permission-env-access-worker.js | 80 ++++++++ test/parallel/test-permission-env-access.js | 181 ++++++++++++++++++ 7 files changed, 519 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-permission-env-access-enum-clone.js create mode 100644 test/parallel/test-permission-env-access-windows.js create mode 100644 test/parallel/test-permission-env-access-worker.js create mode 100644 test/parallel/test-permission-env-access.js diff --git a/src/node_env_var.cc b/src/node_env_var.cc index b748cbd4fab517..8cde78db997d1e 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -4,10 +4,12 @@ #include "node_external_reference.h" #include "node_i18n.h" #include "node_process-inl.h" +#include "permission/permission.h" #include // tzset(), _tzset() namespace node { +using permission::PermissionScope; using v8::Array; using v8::Boolean; using v8::Context; @@ -336,6 +338,12 @@ Maybe KVStore::AssignToObject(v8::Isolate* isolate, return Just(true); } +template +inline static bool HasEnvAccess(const PropertyCallbackInfo& info) { + if (!Environment::GetCurrent(info)->permission()->is_enabled()) return true; + return info.Data()->IsBoolean(); +} + static void EnvGetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); @@ -344,8 +352,18 @@ static void EnvGetter(Local property, return info.GetReturnValue().SetUndefined(); } CHECK(property->IsString()); - MaybeLocal value_string = - env->env_vars()->Get(env->isolate(), property.As()); + + Isolate* isolate = env->isolate(); + Local key = property.As(); + + if (!HasEnvAccess(info)) { + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + PermissionScope::kEnvironment, + Utf8Value(isolate, key).ToStringView(), + info.GetReturnValue().SetUndefined()); + } + + MaybeLocal value_string = env->env_vars()->Get(isolate, key); if (!value_string.IsEmpty()) { info.GetReturnValue().Set(value_string.ToLocalChecked()); } @@ -380,7 +398,15 @@ static void EnvSetter(Local property, return; } - env->env_vars()->Set(env->isolate(), key, value_string); + Isolate* isolate = env->isolate(); + + if (!HasEnvAccess(info)) { + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + PermissionScope::kEnvironment, + Utf8Value(isolate, key).ToStringView()); + } + + env->env_vars()->Set(isolate, key, value_string); // Whether it worked or not, always return value. info.GetReturnValue().Set(value); @@ -391,7 +417,15 @@ static void EnvQuery(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { - int32_t rc = env->env_vars()->Query(env->isolate(), property.As()); + Isolate* isolate = env->isolate(); + Local key = property.As(); + + if (!HasEnvAccess(info)) { + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + PermissionScope::kEnvironment, + Utf8Value(isolate, key).ToStringView()); + } + int32_t rc = env->env_vars()->Query(isolate, key); if (rc != -1) info.GetReturnValue().Set(rc); } } @@ -401,7 +435,15 @@ static void EnvDeleter(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { - env->env_vars()->Delete(env->isolate(), property.As()); + Isolate* isolate = env->isolate(); + Local key = property.As(); + + if (!HasEnvAccess(info)) { + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + PermissionScope::kEnvironment, + Utf8Value(isolate, key).ToStringView()); + } + env->env_vars()->Delete(isolate, key); } // process.env never has non-configurable properties, so always @@ -413,8 +455,26 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); - info.GetReturnValue().Set( - env->env_vars()->Enumerate(env->isolate())); + Isolate* isolate = env->isolate(); + Local context = isolate->GetCurrentContext(); + Local keys = env->env_vars()->Enumerate(isolate); + + if (!HasEnvAccess(info) && !keys.IsEmpty()) { + uint32_t keys_length = keys->Length(); + for (uint32_t i = 0; i < keys_length; i++) { + Local key = keys->Get(context, i).ToLocalChecked(); + CHECK(key->IsString()); + // We may want to remove access denied keys from the array instead of + // throwing an error here. In this case, users won't know exactly if the + // keys doesn't exist, or if they don't have access to it. + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, + PermissionScope::kEnvironment, + Utf8Value(isolate, key.As()).ToStringView()); + } + } + + info.GetReturnValue().Set(keys); } static void EnvDefiner(Local property, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 19b393bb308db6..90c2526bbcdde8 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -8,10 +8,12 @@ #include "node_errors.h" #include "node_external_reference.h" #include "node_process-inl.h" +#include "permission/permission.h" #include "util-inl.h" using node::contextify::ContextifyContext; using node::errors::TryCatchScope; +using node::permission::PermissionScope; using v8::Array; using v8::ArrayBuffer; using v8::BackingStore; @@ -328,14 +330,33 @@ class SerializerDelegate : public ValueSerializer::Delegate { if (!env_proxy_ctor_template.IsEmpty() && env_proxy_ctor_template->HasInstance(object)) { HandleScope scope(isolate); + Local context = env_->context(); + // We may want to remove access denied keys from the object newly created + // below instead of throwing an error here. In this case, users won't know + // exactly if the keys doesn't exist, or if they don't have access to it. + if (env_->permission()->is_enabled()) { + Local keys = env_->env_vars()->Enumerate(isolate); + if (!keys.IsEmpty()) { + uint32_t keys_length = keys->Length(); + for (uint32_t i = 0; i < keys_length; i++) { + Local key = keys->Get(context, i).ToLocalChecked(); + CHECK(key->IsString()); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env_, + PermissionScope::kEnvironment, + Utf8Value(isolate, key.As()).ToStringView(), + Nothing()); + } + } + } // TODO(bnoordhuis) Prototype-less object in case process.env contains // a "__proto__" key? process.env has a prototype with concomitant // methods like toString(). It's probably confusing if that gets lost // in transmission. Local normal_object = Object::New(isolate); - env_->env_vars()->AssignToObject(isolate, env_->context(), normal_object); + env_->env_vars()->AssignToObject(isolate, context, normal_object); serializer->WriteUint32(kNormalObject); // Instead of a BaseObject. - return serializer->WriteValue(env_->context(), normal_object); + return serializer->WriteValue(context, normal_object); } ThrowDataCloneError(env_->clone_unsupported_type_str()); diff --git a/src/permission/permission.h b/src/permission/permission.h index 563406732135d5..dd9dac5a83d65c 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -40,6 +40,8 @@ class Permission { return is_scope_granted(permission, res); } + FORCE_INLINE bool is_enabled() { return enabled_; } + static PermissionScope StringToPermission(const std::string& perm); static const char* PermissionToString(PermissionScope perm); static void ThrowAccessDenied(Environment* env, diff --git a/test/parallel/test-permission-env-access-enum-clone.js b/test/parallel/test-permission-env-access-enum-clone.js new file mode 100644 index 00000000000000..c91bc3ad51f1b3 --- /dev/null +++ b/test/parallel/test-permission-env-access-enum-clone.js @@ -0,0 +1,108 @@ +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +'use strict'; + +require('../common'); +const { spawnSync } = require('node:child_process'); +const { debuglog } = require('node:util'); +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const debug = debuglog('test'); + +function runTest(args, options = {}) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--experimental-permission', '--allow-fs-read=*', ...args], + options + ); + debug('status:', status); + if (status) debug('stdout:', stdout?.toString().split('\n')); + if (status) debug('stderr:', stderr?.toString().split('\n')); + return { status, stdout, stderr }; +} + +describe('permission: enumerate', () => { + const error = JSON.stringify({ + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + }); + + it('enumerate with *,-UNDEFINED', () => { + const { status } = runTest([ + '--allow-env=*,-UNDEFINED', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + Object.keys(process.env); + }); + `, + ]); + strictEqual(status, 0); + }); + + it('enumerate with *,-DEFINED', () => { + const { status } = runTest( + [ + '--allow-env=*,-DEFINED', + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + Object.keys(process.env); + }, ${error}); + `, + ], + { + env: { + ...process.env, + DEFINED: 0, + }, + } + ); + strictEqual(status, 0); + }); +}); + +describe('permission: structuredClone', () => { + const error = JSON.stringify({ + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + }); + + it('structuredClone process.env with *,-UNDEFINED', () => { + const { status } = runTest([ + '--allow-env=*,-UNDEFINED', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + structuredClone(process.env); + }); + `, + ]); + strictEqual(status, 0); + }); + + it('structuredClone process.env with *,-DEFINED', () => { + const { status } = runTest( + [ + '--allow-env=*,-DEFINED', + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + structuredClone(process.env); + }, ${error}); + `, + ], + { + env: { + ...process.env, + DEFINED: 0, + }, + } + ); + strictEqual(status, 0); + }); +}); diff --git a/test/parallel/test-permission-env-access-windows.js b/test/parallel/test-permission-env-access-windows.js new file mode 100644 index 00000000000000..0a963596649a8a --- /dev/null +++ b/test/parallel/test-permission-env-access-windows.js @@ -0,0 +1,58 @@ +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +'use strict'; + +const common = require('../common'); +const { spawnSync } = require('node:child_process'); +const { debuglog } = require('node:util'); +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +if (!common.isWindows) { + common.skip('process.env on Windows test'); +} + +const debug = debuglog('test'); + +function runTest(args, options = {}) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--experimental-permission', '--allow-fs-read=*', ...args], + options + ); + debug('status:', status); + if (status) debug('stdout:', stdout?.toString().split('\n')); + if (status) debug('stderr:', stderr?.toString().split('\n')); + return { status, stdout, stderr }; +} + +describe('permission: reference', () => { + const error = JSON.stringify({ + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + }); + + it('reference on Windows is case-insensitive', () => { + const { status } = runTest([ + '--allow-env=FoO', + '-e', + ` + const { throws, doesNotThrow, strictEqual } = require('node:assert'); + + strictEqual(process.permission.has('env', 'FOO'), true); + strictEqual(process.permission.has('env', 'foo'), true); + strictEqual(process.permission.has('env', 'FoO'), true); + + doesNotThrow(() => { + process.env.FOO; + process.env.foo; + process.env.FoO; + }); + + throws(() => { + process.env.OTHERS; + }, ${error}); + `, + ]); + strictEqual(status, 0); + }); +}); diff --git a/test/parallel/test-permission-env-access-worker.js b/test/parallel/test-permission-env-access-worker.js new file mode 100644 index 00000000000000..26388ddbec08a6 --- /dev/null +++ b/test/parallel/test-permission-env-access-worker.js @@ -0,0 +1,80 @@ +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +'use strict'; + +require('../common'); +const { spawnSync } = require('node:child_process'); +const { debuglog } = require('node:util'); +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const debug = debuglog('test'); + +function runTest(args, options = {}) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--experimental-permission', '--allow-fs-read=*', ...args], + options + ); + debug('status:', status); + if (status) debug('stdout:', stdout?.toString().split('\n')); + if (status) debug('stderr:', stderr?.toString().split('\n')); + return { status, stdout, stderr }; +} + +describe('permission: "env" access on worker thread', () => { + it('worker_threads', () => { + const { status } = runTest([ + '--allow-worker', + '-e', + ` + const assert = require('node:assert'); + const { Worker, SHARE_ENV } = require('node:worker_threads'); + const w = new Worker('process.env.UNDEFINED', { eval: true }); + w.on('error', () => {}); + w.on('exit', (code) => assert.strictEqual(code, 1)); + `, + ]); + strictEqual(status, 0); + }); + + it('worker_threads with SHARE_ENV', () => { + const { status } = runTest([ + '--allow-worker', + '-e', + ` + const assert = require('node:assert'); + const { Worker, SHARE_ENV } = require('node:worker_threads'); + const w = new Worker('process.env.UNDEFINED', { eval: true, env: SHARE_ENV }); + w.on('error', () => {}); + w.on('exit', (code) => assert.strictEqual(code, 1)); + `, + ]); + strictEqual(status, 0); + }); + + it('worker_threads with DENIED_IN_MAIN_THREAD', () => { + const error = JSON.stringify({ + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + }); + + const { status } = runTest([ + '--allow-worker', + '--allow-env=*,-DENIED_IN_MAIN_THREAD', + '-e', + ` + const { throws, strictEqual } = require('node:assert'); + const { Worker, SHARE_ENV } = require('node:worker_threads'); + const w = new Worker('process.env.DENIED_IN_MAIN_THREAD', { + eval: true, + env: { DENIED_IN_MAIN_THREAD: 1 }, + }); + w.on('exit', (code) => strictEqual(code, 0)); + throws(() => { + process.env['DENIED_IN_MAIN_THREAD'] + }, ${error}); + `, + ]); + strictEqual(status, 0); + }); +}); diff --git a/test/parallel/test-permission-env-access.js b/test/parallel/test-permission-env-access.js new file mode 100644 index 00000000000000..a29ce0c431bd13 --- /dev/null +++ b/test/parallel/test-permission-env-access.js @@ -0,0 +1,181 @@ +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +'use strict'; + +require('../common'); +const { spawnSync } = require('node:child_process'); +const { debuglog } = require('node:util'); +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const debug = debuglog('test'); + +function runTest(args, options = {}) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--experimental-permission', '--allow-fs-read=*', ...args], + options + ); + debug('status:', status); + if (status) debug('stdout:', stdout?.toString().split('\n')); + if (status) debug('stderr:', stderr?.toString().split('\n')); + return { status, stdout, stderr }; +} + +describe('permission: "env" access', () => { + const error = JSON.stringify({ + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + }); + + it('get', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + process.env.UNDEFINED; + }, ${error});`, + ]); + strictEqual(status, 0); + }); + + it('set', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + process.env.UNDEFINED = 0; + }, ${error});`, + ]); + strictEqual(status, 0); + }); + + it('query', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + 'UNDEFINED' in process.env; + }, ${error});`, + ]); + strictEqual(status, 0); + }); + + it('delete', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + delete process.env.UNDEFINED; + }, ${error});`, + ]); + strictEqual(status, 0); + }); + + it('enumerate', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + Object.keys(process.env); + }, ${error});`, + ]); + strictEqual(status, 0); + }); + + it('structuredClone', () => { + const { status } = runTest([ + '-e', + ` + const { throws } = require('node:assert'); + throws(() => { + structuredClone(process.env); + }, ${error});`, + ]); + strictEqual(status, 0); + }); +}); + +describe('permission: "env" access with --allow-env=*', () => { + it('get', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + process.env.UNDEFINED; + });`, + ]); + strictEqual(status, 0); + }); + + it('set', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + process.env.UNDEFINED = 0; + });`, + ]); + strictEqual(status, 0); + }); + + it('query', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + 'UNDEFINED' in process.env; + });`, + ]); + strictEqual(status, 0); + }); + + it('delete', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + delete process.env.UNDEFINED; + });`, + ]); + strictEqual(status, 0); + }); + + it('enumerate', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + Object.keys(process.env); + });`, + ]); + strictEqual(status, 0); + }); + + it('structuredClone', () => { + const { status } = runTest([ + '--allow-env=*', + '-e', + ` + const { doesNotThrow } = require('node:assert'); + doesNotThrow(() => { + structuredClone(process.env); + });`, + ]); + strictEqual(status, 0); + }); +}); From cb85eba58409e05b6793ee41c61540f19dbcfd0c Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Tue, 16 May 2023 00:10:45 +0900 Subject: [PATCH 04/14] test: allow other permission tests to access env vars Signed-off-by: Daeyeon Jeong --- test/addons/no-addons/permission.js | 2 +- test/parallel/test-permission-allow-child-process-cli.js | 2 +- test/parallel/test-permission-allow-worker-cli.js | 2 +- test/parallel/test-permission-child-process-cli.js | 2 +- test/parallel/test-permission-experimental.js | 2 +- test/parallel/test-permission-fs-read.js | 7 +++++-- test/parallel/test-permission-fs-relative-path.js | 2 +- test/parallel/test-permission-fs-symlink-target-write.js | 3 ++- test/parallel/test-permission-fs-symlink.js | 3 ++- test/parallel/test-permission-fs-wildcard.js | 3 ++- test/parallel/test-permission-fs-windows-path.js | 2 +- test/parallel/test-permission-fs-write.js | 3 ++- test/parallel/test-permission-has.js | 2 +- test/parallel/test-permission-worker-threads-cli.js | 2 +- 14 files changed, 22 insertions(+), 15 deletions(-) diff --git a/test/addons/no-addons/permission.js b/test/addons/no-addons/permission.js index 0fbcd2bb1ee782..89d88e7a5e7de1 100644 --- a/test/addons/no-addons/permission.js +++ b/test/addons/no-addons/permission.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* 'use strict'; diff --git a/test/parallel/test-permission-allow-child-process-cli.js b/test/parallel/test-permission-allow-child-process-cli.js index 6cffc19719350b..021b6dfc45d2d4 100644 --- a/test/parallel/test-permission-allow-child-process-cli.js +++ b/test/parallel/test-permission-allow-child-process-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-child-process --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-child-process --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-allow-worker-cli.js b/test/parallel/test-permission-allow-worker-cli.js index ae5a28fdae3597..6fda436fbc2cea 100644 --- a/test/parallel/test-permission-allow-worker-cli.js +++ b/test/parallel/test-permission-allow-worker-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-worker --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-worker --allow-fs-read=* 'use strict'; require('../common'); diff --git a/test/parallel/test-permission-child-process-cli.js b/test/parallel/test-permission-child-process-cli.js index 3ce473ab498e0e..12c8adff5bdda7 100644 --- a/test/parallel/test-permission-child-process-cli.js +++ b/test/parallel/test-permission-child-process-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-experimental.js b/test/parallel/test-permission-experimental.js index bec66e5a731a95..5ea884f5d6cce9 100644 --- a/test/parallel/test-permission-experimental.js +++ b/test/parallel/test-permission-experimental.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-read.js b/test/parallel/test-permission-fs-read.js index 010a5932c4eae1..8d96e71e303f94 100644 --- a/test/parallel/test-permission-fs-read.js +++ b/test/parallel/test-permission-fs-read.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -28,7 +28,10 @@ const commonPath = path.join(__filename, '../../common'); const { status, stderr } = spawnSync( process.execPath, [ - '--experimental-permission', `--allow-fs-read=${file},${commonPathWildcard}`, file, + '--experimental-permission', + '--allow-env=*', + `--allow-fs-read=${file},${commonPathWildcard}`, + file, ], { env: { diff --git a/test/parallel/test-permission-fs-relative-path.js b/test/parallel/test-permission-fs-relative-path.js index 74b4095e86663a..26ace2df6514e2 100644 --- a/test/parallel/test-permission-fs-relative-path.js +++ b/test/parallel/test-permission-fs-relative-path.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-symlink-target-write.js b/test/parallel/test-permission-fs-symlink-target-write.js index f6a99fe06c9d1f..74095454668081 100644 --- a/test/parallel/test-permission-fs-symlink-target-write.js +++ b/test/parallel/test-permission-fs-symlink-target-write.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -36,6 +36,7 @@ fs.writeFileSync(path.join(readWriteFolder, 'file'), 'NO evil file contents'); process.execPath, [ '--experimental-permission', + '--allow-env=*', `--allow-fs-read=${file},${commonPathWildcard},${readOnlyFolder},${readWriteFolder}`, `--allow-fs-write=${readWriteFolder},${writeOnlyFolder}`, file, diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js index 96d4fdad4d2d4b..2fedc19f48086a 100644 --- a/test/parallel/test-permission-fs-symlink.js +++ b/test/parallel/test-permission-fs-symlink.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -37,6 +37,7 @@ const symlinkFromBlockedFile = path.join(tmpdir.path, 'example-symlink.md'); process.execPath, [ '--experimental-permission', + '--allow-env=*', `--allow-fs-read=${file},${commonPathWildcard},${symlinkFromBlockedFile}`, `--allow-fs-write=${symlinkFromBlockedFile}`, file, diff --git a/test/parallel/test-permission-fs-wildcard.js b/test/parallel/test-permission-fs-wildcard.js index eb8d4ca53ced04..1fd68fbf905a14 100644 --- a/test/parallel/test-permission-fs-wildcard.js +++ b/test/parallel/test-permission-fs-wildcard.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); @@ -85,6 +85,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', + '--allow-env=*', `--allow-fs-read=${file},${commonPathWildcard},${allowList.join(',')}`, file, ], diff --git a/test/parallel/test-permission-fs-windows-path.js b/test/parallel/test-permission-fs-windows-path.js index b64cef12b47c18..c0ef4866a84752 100644 --- a/test/parallel/test-permission-fs-windows-path.js +++ b/test/parallel/test-permission-fs-windows-path.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-write.js b/test/parallel/test-permission-fs-write.js index 9f257df86f8672..6a4bfa8fdc8a9c 100644 --- a/test/parallel/test-permission-fs-write.js +++ b/test/parallel/test-permission-fs-write.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); @@ -25,6 +25,7 @@ const file = fixtures.path('permission', 'fs-write.js'); process.execPath, [ '--experimental-permission', + '--allow-env=*', '--allow-fs-read=*', `--allow-fs-write=${regularFile},${commonPath}`, file, diff --git a/test/parallel/test-permission-has.js b/test/parallel/test-permission-has.js index 88a00496a5db36..c21c73ed81e17b 100644 --- a/test/parallel/test-permission-has.js +++ b/test/parallel/test-permission-has.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-worker-threads-cli.js b/test/parallel/test-permission-worker-threads-cli.js index e817a7877226c1..d88bfb7a2c6671 100644 --- a/test/parallel/test-permission-worker-threads-cli.js +++ b/test/parallel/test-permission-worker-threads-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-fs-read=* +// Flags: --experimental-permission --allow-env=* --allow-fs-read=* 'use strict'; const common = require('../common'); From 555bbf5974b6dd21de63bde806fd5c49c5b18847 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 18 May 2023 23:58:35 +0900 Subject: [PATCH 05/14] doc: add env permission Signed-off-by: Daeyeon Jeong --- doc/api/cli.md | 57 ++++++++++++++++++++++++++++++++++++++++++ doc/api/permissions.md | 50 +++++++++++++++++++++++++++++++++--- doc/api/process.md | 6 +++++ 3 files changed, 110 insertions(+), 3 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 0e775d4d80044e..6e6f071be248f4 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -141,6 +141,62 @@ Error: Access to this API has been restricted } ``` +### `--allow-env` + + + +> Stability: 1 - Experimental + +When using the [Permission Model][], access to the environment variables via +`process.env` is denied by default. Attempts to do so will throw an +`ERR_ACCESS_DENIED`. Users can explicitly configure permissions by passing the +`--allow-env` flag when starting Node.js. + +The valid arguments for the `--allow-env` flag are: + +* Strings delimited by comma (`,`). +* `*` - Used as a wildcard. +* `-` - Used to deny access. By prefixing environment variables with `-`, you + can restrict access to them. It's worth noting that denied permissions always + take precedence over allowed permissions, regardless of the input order. + +See the [Environment Permissions][] documentation for more details. + +Example: + +```js +// Attempt to bypass the permission +process.env.SECRET; +``` + +```console +$ node --experimental-permission --allow-fs-read=* index.js +node:internal/modules/cjs/loader:162 + const result = internalModuleStat(filename); + ^ + +Error: Access to this API has been restricted + at Object. (/home/index.js.js:1:13) + at Module._compile (node:internal/modules/cjs/loader:1256:14) + at Module._extensions..js (node:internal/modules/cjs/loader:1310:10) + at Module.load (node:internal/modules/cjs/loader:1114:32) + at Module._load (node:internal/modules/cjs/loader:961:12) + at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12) + at node:internal/main/run_main_module:23:47 { + code: 'ERR_ACCESS_DENIED', + permission: 'Environment', + resource: 'SECRET' +} +``` + +The process needs to have access to the `SECRET` in the environment variables: + +```console +$ node --experimental-permission --allow-fs-read=* --allow-env=SECRET index.js +``` + ### `--allow-fs-read` * `--allow-child-process` +* `--allow-env-key` * `--allow-env` * `--allow-fs-read` * `--allow-fs-write` diff --git a/src/env.cc b/src/env.cc index 307245b5f2e999..c690a16317e001 100644 --- a/src/env.cc +++ b/src/env.cc @@ -814,8 +814,8 @@ Environment::Environment(IsolateData* isolate_data, permission::PermissionScope::kFileSystemWrite); } - if (!options_->allow_env.empty()) { - permission()->Apply(options_->allow_env, + if (options_->allow_env || !options_->allow_env_key.empty()) { + permission()->Apply(options_->allow_env_key, permission::PermissionScope::kEnvironment); } } diff --git a/src/node_options.cc b/src/node_options.cc index 5e48b4e0c61bd8..f9e010f524bdbf 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -416,10 +416,16 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { &EnvironmentOptions::experimental_policy_integrity, kAllowedInEnvvar); Implies("--policy-integrity", "[has_policy_integrity_string]"); + AddOption( + "--allow-env-key", + "allow permissions to access specified keys of the environment variables", + &EnvironmentOptions::allow_env_key, + kAllowedInEnvvar); AddOption("--allow-env", - "allow permissions to read the environment variables", + "allow permissions to access the environment variables", &EnvironmentOptions::allow_env, kAllowedInEnvvar); + AddAlias("--allow-env=", {"--allow-env-key", "--allow-env"}); AddOption("--allow-fs-read", "allow permissions to read the filesystem", &EnvironmentOptions::allow_fs_read, diff --git a/src/node_options.h b/src/node_options.h index 09f89a45b1c1ea..7a021cd1ccd300 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -121,7 +121,8 @@ class EnvironmentOptions : public Options { std::string experimental_policy_integrity; bool has_policy_integrity_string = false; bool experimental_permission = false; - std::string allow_env; + bool allow_env; + std::string allow_env_key; std::string allow_fs_read; std::string allow_fs_write; bool allow_child_process = false; From 2be253facae1620dd9edbd934cbc94a43a0b489b Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 22 May 2023 18:52:22 +0900 Subject: [PATCH 10/14] dev: clean up env permission Signed-off-by: Daeyeon Jeong --- lib/internal/process/permission.js | 16 ------ node.gyp | 2 - src/permission/env_permission.cc | 90 ++++++------------------------ src/permission/env_permission.h | 23 ++------ src/permission/fs_permission.h | 2 - src/permission/permission_util.cc | 78 -------------------------- src/permission/permission_util.h | 42 -------------- 7 files changed, 20 insertions(+), 233 deletions(-) delete mode 100644 src/permission/permission_util.cc delete mode 100644 src/permission/permission_util.h diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index 044c7e2425e8db..e400dcae9f934e 100644 --- a/lib/internal/process/permission.js +++ b/lib/internal/process/permission.js @@ -2,29 +2,15 @@ const { ObjectFreeze, - RegExpPrototypeExec, StringPrototypeStartsWith, } = primordials; const permission = internalBinding('permission'); const { validateString } = require('internal/validators'); const { isAbsolute, resolve } = require('path'); -const { - codes: { ERR_INVALID_ARG_VALUE }, -} = require('internal/errors'); let experimentalPermission; -function validateHasEnvReference(value, name) { - if (!RegExpPrototypeExec(/^[a-zA-Z_][a-zA-Z0-9_]*$/, value)) { - throw new ERR_INVALID_ARG_VALUE( - name, - value, - 'must start with a letter or underscore, follewed by letters, digits, or underscores', - ); - } -} - module.exports = ObjectFreeze({ __proto__: null, isEnabled() { @@ -43,8 +29,6 @@ module.exports = ObjectFreeze({ if (!isAbsolute(reference)) { reference = resolve(reference); } - } else if (StringPrototypeStartsWith(scope, 'env')) { - validateHasEnvReference(reference, 'reference'); } } diff --git a/node.gyp b/node.gyp index bacddc1ddf6b75..f2a56a0375f188 100644 --- a/node.gyp +++ b/node.gyp @@ -141,7 +141,6 @@ 'src/permission/env_permission.cc', 'src/permission/fs_permission.cc', 'src/permission/permission.cc', - 'src/permission/permission_util.cc', 'src/permission/worker_permission.cc', 'src/pipe_wrap.cc', 'src/process_wrap.cc', @@ -262,7 +261,6 @@ 'src/permission/env_permission.h', 'src/permission/fs_permission.h', 'src/permission/permission.h', - 'src/permission/permission_util.h', 'src/permission/worker_permission.h', 'src/pipe_wrap.h', 'src/req_wrap.h', diff --git a/src/permission/env_permission.cc b/src/permission/env_permission.cc index a4b1afe14782b6..b9d2ecab0417bb 100644 --- a/src/permission/env_permission.cc +++ b/src/permission/env_permission.cc @@ -1,70 +1,22 @@ #include "permission/env_permission.h" #include "util-inl.h" -static std::string_view TrimString(const std::string_view& str) { - static const std::string whitespace = " \n\r\t"; - size_t first = str.find_first_not_of(whitespace); - if (first == std::string_view::npos) return ""; - - size_t last = str.find_last_not_of(whitespace); - return str.substr(first, last - first + 1); -} - namespace node { namespace permission { -bool EnvPermission::IsGranted(const std::string_view& param) { - bool has_wildcard_granted = granted_patterns_->Lookup("*"); - bool has_wildcard_denied = denied_patterns_->Lookup("*"); - - if (param.empty()) { - bool has_any_denied = (has_wildcard_denied || !denied_patterns_->Empty()); - return has_wildcard_granted && !has_any_denied; - } - - if (has_wildcard_denied) { - return false; - } - - if (has_wildcard_granted && denied_patterns_->Empty()) { - return true; - } - - if (denied_patterns_->Lookup(param)) { - return false; - } - - if (!has_wildcard_granted && !granted_patterns_->Lookup(param)) { - return false; - } - - return true; -} - -bool EnvPermission::Apply(const std::shared_ptr& patterns, - const std::string_view& pattern) { - const auto pos = pattern.find_first_of('*'); - if (pos != std::string_view::npos) { - if (pattern.substr(pos + 1).size() > 0) { - // Filter out this input if a wildcard is followed by any character. (e.g: - // *ODE, *ODE*, N*DE) Only a string with a wild card of the last - // character is accepted. (e.g: *, -*, NODE*) - return false; - } - } - return patterns->Insert(std::string(pattern)); -} - void EnvPermission::Apply(const std::string& allow, PermissionScope scope) { if (scope == PermissionScope::kEnvironment) { - for (const auto& str : SplitString(allow, ',', true)) { - const std::string_view token = TrimString(str); - if (token.front() == '-') { - Apply(denied_patterns_, token.substr(1)); - } else { - Apply(granted_patterns_, token); - } + if (allow.empty()) { + allow_all_ = true; + return; + } + for (const auto& token : SplitString(allow, ',')) { +#ifdef _WIN32 + allowed_.insert(ToUpper(token)); +#else + allowed_.insert(token); +#endif } } } @@ -72,25 +24,15 @@ void EnvPermission::Apply(const std::string& allow, PermissionScope scope) { bool EnvPermission::is_granted(PermissionScope scope, const std::string_view& param) { if (scope == PermissionScope::kEnvironment) { - return IsGranted(param); - } - return false; -} - -bool EnvPermission::Patterns::Insert(const std::string& s) { + const std::string token = std::string(param); #ifdef _WIN32 - return SearchTree::Insert(ToUpper(s)); + if (allow_all_ || allowed_.find(ToUpper(token)) != allowed_.end()) + return true; #else - return SearchTree::Insert(s); -#endif -} - -bool EnvPermission::Patterns::Lookup(const std::string_view& s, - bool when_empty_return) { -#ifdef _WIN32 - return SearchTree::Lookup(ToUpper(std::string(s))); + if (allow_all_ || allowed_.find(token) != allowed_.end()) return true; #endif - return SearchTree::Lookup(s); + } + return false; } } // namespace permission diff --git a/src/permission/env_permission.h b/src/permission/env_permission.h index 80a30bbfaa3da9..1bb1a8733dea3c 100644 --- a/src/permission/env_permission.h +++ b/src/permission/env_permission.h @@ -3,9 +3,9 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include #include -#include "permission/permission_util.h" +#include +#include "permission/permission_base.h" namespace node { @@ -13,28 +13,13 @@ namespace permission { class EnvPermission final : public PermissionBase { public: - class Patterns final : public SearchTree { - public: - bool Insert(const std::string& s) override; - bool Lookup(const std::string_view& s, - bool when_empty_return = false) override; - }; - - EnvPermission() - : denied_patterns_(std::make_shared()), - granted_patterns_(std::make_shared()) {} - void Apply(const std::string& allow, PermissionScope scope) override; bool is_granted(PermissionScope scope, const std::string_view& param = "") override; private: - bool IsGranted(const std::string_view& param); - bool Apply(const std::shared_ptr& patterns, - const std::string_view& pattern); - - std::shared_ptr denied_patterns_; - std::shared_ptr granted_patterns_; + bool allow_all_ = false; + std::unordered_set allowed_; }; } // namespace permission diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h index 9bee1b9afcca08..8d735a9095502e 100644 --- a/src/permission/fs_permission.h +++ b/src/permission/fs_permission.h @@ -123,8 +123,6 @@ class FSPermission final : public PermissionBase { void Insert(const std::string& s); bool Lookup(const std::string_view& s) { return Lookup(s, false); } bool Lookup(const std::string_view& s, bool when_empty_return); - bool Empty() { return root_node_->children.empty(); } - Node* GetRoot() { return root_node_; } private: Node* root_node_; diff --git a/src/permission/permission_util.cc b/src/permission/permission_util.cc deleted file mode 100644 index ccb2ba33fbe622..00000000000000 --- a/src/permission/permission_util.cc +++ /dev/null @@ -1,78 +0,0 @@ -#include "permission_util.h" - -namespace node { - -namespace permission { - -bool SearchTree::Insert(const std::string& s) { - tree_.Insert(s); - return true; -} - -bool SearchTree::Lookup(const std::string_view& s, bool when_empty_return) { - FSPermission::RadixTree::Node* current_node = tree_.GetRoot(); - - if (current_node->children.size() == 0) { - return when_empty_return; - } - - // Check if the root node has a wildcard. - auto it = current_node->children.find('*'); - if (it != current_node->children.end()) { - return true; - } - - unsigned int parent_node_prefix_len = current_node->prefix.length(); - const std::string path(s); - auto path_len = path.length(); - - while (true) { - if (parent_node_prefix_len == path_len && current_node->IsEndNode()) { - return true; - } - - auto node = NextNode(current_node, path, parent_node_prefix_len); - if (node == nullptr) { - return false; - } - - current_node = node; - parent_node_prefix_len += current_node->prefix.length(); - if (current_node->wildcard_child != nullptr && - path_len >= parent_node_prefix_len - 1 /* -1: * */) { - return true; - } - } - return false; -} - -FSPermission::RadixTree::Node* SearchTree::NextNode( - const FSPermission::RadixTree::Node* node, - const std::string& path, - unsigned int idx) { - if (idx >= path.length()) { - return nullptr; - } - - auto it = node->children.find(path[idx]); - if (it == node->children.end()) { - return nullptr; - } - auto child = it->second; - // match prefix - unsigned int prefix_len = child->prefix.length(); - for (unsigned int i = 0; i < path.length(); ++i) { - if (i >= prefix_len || child->prefix[i] == '*') { - return child; - } - - if (path[idx++] != child->prefix[i]) { - return nullptr; - } - } - return child; -} - -} // namespace permission - -} // namespace node diff --git a/src/permission/permission_util.h b/src/permission/permission_util.h deleted file mode 100644 index 5455e6cbdad90d..00000000000000 --- a/src/permission/permission_util.h +++ /dev/null @@ -1,42 +0,0 @@ -#ifndef SRC_PERMISSION_PERMISSION_UTIL_H_ -#define SRC_PERMISSION_PERMISSION_UTIL_H_ - -#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#include -#include "permission/fs_permission.h" - -namespace node { - -namespace permission { - -class SearchTree { - public: - virtual ~SearchTree() = default; - virtual bool Insert(const std::string& s); - virtual bool Lookup(const std::string_view& s, - bool when_empty_return = false); - bool Empty() { return tree_.Empty(); } - - private: - // This class delegates most of its feature to FSPermission::RadixTree. The - // modified parts are as follows, while the rest of the implementations is - // from RadixTree: - // 1. Removed handling the path string in Lookup and NextNode. - // 2. Added checking if the root node has a wildcard in Lookup. - FSPermission::RadixTree::Node* NextNode( - const FSPermission::RadixTree::Node* node, - const std::string& path, - unsigned int idx); - FSPermission::RadixTree::Node* GetRoot() { return tree_.GetRoot(); } - - // TODO(daeyeon): modify the file system specific parts in RadixTree to make - // it usable for both FsPermission and EnvPermission. - FSPermission::RadixTree tree_; -}; - -} // namespace permission - -} // namespace node -#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#endif // SRC_PERMISSION_PERMISSION_UTIL_H_ From 64ef23e1c06f8fc9dd3082f82658de0844b11a1b Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 22 May 2023 18:56:18 +0900 Subject: [PATCH 11/14] dev: update other permission tests to access env vars Signed-off-by: Daeyeon Jeong --- benchmark/fs/readfile-permission-enabled.js | 2 +- benchmark/permission/permission-fs-read.js | 2 +- test/addons/no-addons/permission.js | 2 +- test/parallel/test-permission-allow-child-process-cli.js | 2 +- test/parallel/test-permission-allow-worker-cli.js | 2 +- test/parallel/test-permission-child-process-cli.js | 2 +- test/parallel/test-permission-experimental.js | 2 +- test/parallel/test-permission-fs-read.js | 4 ++-- test/parallel/test-permission-fs-relative-path.js | 2 +- test/parallel/test-permission-fs-symlink-target-write.js | 4 ++-- test/parallel/test-permission-fs-symlink.js | 4 ++-- test/parallel/test-permission-fs-wildcard.js | 4 ++-- test/parallel/test-permission-fs-windows-path.js | 2 +- test/parallel/test-permission-fs-write.js | 4 ++-- test/parallel/test-permission-worker-threads-cli.js | 2 +- 15 files changed, 20 insertions(+), 20 deletions(-) diff --git a/benchmark/fs/readfile-permission-enabled.js b/benchmark/fs/readfile-permission-enabled.js index 30248522d5ec55..a98c370f08216a 100644 --- a/benchmark/fs/readfile-permission-enabled.js +++ b/benchmark/fs/readfile-permission-enabled.js @@ -24,7 +24,7 @@ const bench = common.createBenchmark(main, { '--allow-fs-read=*', '--allow-fs-write=*', '--allow-child-process', - '--allow-env=*', + '--allow-env', ], }); diff --git a/benchmark/permission/permission-fs-read.js b/benchmark/permission/permission-fs-read.js index a8918fef9ab9aa..24482d19d36e27 100644 --- a/benchmark/permission/permission-fs-read.js +++ b/benchmark/permission/permission-fs-read.js @@ -13,7 +13,7 @@ const options = { flags: [ '--experimental-permission', `--allow-fs-read=${rootPath}`, - '--allow-env=*', + '--allow-env', ], }; diff --git a/test/addons/no-addons/permission.js b/test/addons/no-addons/permission.js index 89d88e7a5e7de1..a06fead1ff42e1 100644 --- a/test/addons/no-addons/permission.js +++ b/test/addons/no-addons/permission.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-fs-read=* 'use strict'; diff --git a/test/parallel/test-permission-allow-child-process-cli.js b/test/parallel/test-permission-allow-child-process-cli.js index 021b6dfc45d2d4..1d7ee731226d21 100644 --- a/test/parallel/test-permission-allow-child-process-cli.js +++ b/test/parallel/test-permission-allow-child-process-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-child-process --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-child-process --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-allow-worker-cli.js b/test/parallel/test-permission-allow-worker-cli.js index 6fda436fbc2cea..5791ff7e0c9c9d 100644 --- a/test/parallel/test-permission-allow-worker-cli.js +++ b/test/parallel/test-permission-allow-worker-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-worker --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-worker --allow-fs-read=* 'use strict'; require('../common'); diff --git a/test/parallel/test-permission-child-process-cli.js b/test/parallel/test-permission-child-process-cli.js index 12c8adff5bdda7..3528d63948540b 100644 --- a/test/parallel/test-permission-child-process-cli.js +++ b/test/parallel/test-permission-child-process-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-experimental.js b/test/parallel/test-permission-experimental.js index 5ea884f5d6cce9..f3f9e8eaa5b551 100644 --- a/test/parallel/test-permission-experimental.js +++ b/test/parallel/test-permission-experimental.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-fs-read=* 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-read.js b/test/parallel/test-permission-fs-read.js index 8d96e71e303f94..8a32e4aa696f00 100644 --- a/test/parallel/test-permission-fs-read.js +++ b/test/parallel/test-permission-fs-read.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -29,7 +29,7 @@ const commonPath = path.join(__filename, '../../common'); process.execPath, [ '--experimental-permission', - '--allow-env=*', + '--allow-env', `--allow-fs-read=${file},${commonPathWildcard}`, file, ], diff --git a/test/parallel/test-permission-fs-relative-path.js b/test/parallel/test-permission-fs-relative-path.js index 26ace2df6514e2..1acd18782cd812 100644 --- a/test/parallel/test-permission-fs-relative-path.js +++ b/test/parallel/test-permission-fs-relative-path.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-symlink-target-write.js b/test/parallel/test-permission-fs-symlink-target-write.js index 74095454668081..da2f6f4e105494 100644 --- a/test/parallel/test-permission-fs-symlink-target-write.js +++ b/test/parallel/test-permission-fs-symlink-target-write.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -36,7 +36,7 @@ fs.writeFileSync(path.join(readWriteFolder, 'file'), 'NO evil file contents'); process.execPath, [ '--experimental-permission', - '--allow-env=*', + '--allow-env', `--allow-fs-read=${file},${commonPathWildcard},${readOnlyFolder},${readWriteFolder}`, `--allow-fs-write=${readWriteFolder},${writeOnlyFolder}`, file, diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js index 2fedc19f48086a..33a9eb0a9eb90b 100644 --- a/test/parallel/test-permission-fs-symlink.js +++ b/test/parallel/test-permission-fs-symlink.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-fs-write=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-fs-write=* --allow-child-process 'use strict'; const common = require('../common'); @@ -37,7 +37,7 @@ const symlinkFromBlockedFile = path.join(tmpdir.path, 'example-symlink.md'); process.execPath, [ '--experimental-permission', - '--allow-env=*', + '--allow-env', `--allow-fs-read=${file},${commonPathWildcard},${symlinkFromBlockedFile}`, `--allow-fs-write=${symlinkFromBlockedFile}`, file, diff --git a/test/parallel/test-permission-fs-wildcard.js b/test/parallel/test-permission-fs-wildcard.js index 1fd68fbf905a14..4bdd8d44c54d86 100644 --- a/test/parallel/test-permission-fs-wildcard.js +++ b/test/parallel/test-permission-fs-wildcard.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); @@ -85,7 +85,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', - '--allow-env=*', + '--allow-env', `--allow-fs-read=${file},${commonPathWildcard},${allowList.join(',')}`, file, ], diff --git a/test/parallel/test-permission-fs-windows-path.js b/test/parallel/test-permission-fs-windows-path.js index c0ef4866a84752..8074f088f24611 100644 --- a/test/parallel/test-permission-fs-windows-path.js +++ b/test/parallel/test-permission-fs-windows-path.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-fs-write.js b/test/parallel/test-permission-fs-write.js index 6a4bfa8fdc8a9c..4e52c37304ed12 100644 --- a/test/parallel/test-permission-fs-write.js +++ b/test/parallel/test-permission-fs-write.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); @@ -25,7 +25,7 @@ const file = fixtures.path('permission', 'fs-write.js'); process.execPath, [ '--experimental-permission', - '--allow-env=*', + '--allow-env', '--allow-fs-read=*', `--allow-fs-write=${regularFile},${commonPath}`, file, diff --git a/test/parallel/test-permission-worker-threads-cli.js b/test/parallel/test-permission-worker-threads-cli.js index d88bfb7a2c6671..8ee55bc666b669 100644 --- a/test/parallel/test-permission-worker-threads-cli.js +++ b/test/parallel/test-permission-worker-threads-cli.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-fs-read=* 'use strict'; const common = require('../common'); From 9b5a4f342d3bd513649591610af6cae2fa8ac3d8 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Mon, 22 May 2023 19:07:21 +0900 Subject: [PATCH 12/14] dev: update tcs Signed-off-by: Daeyeon Jeong --- ...e.js => test-permission-env-enum-clone.js} | 22 +-- test/parallel/test-permission-env-has.js | 164 ++---------------- ...dows.js => test-permission-env-windows.js} | 2 +- ...orker.js => test-permission-env-worker.js} | 8 +- ...n-env-access.js => test-permission-env.js} | 16 +- test/parallel/test-permission-has.js | 20 +-- 6 files changed, 41 insertions(+), 191 deletions(-) rename test/parallel/{test-permission-env-access-enum-clone.js => test-permission-env-enum-clone.js} (79%) rename test/parallel/{test-permission-env-access-windows.js => test-permission-env-windows.js} (94%) rename test/parallel/{test-permission-env-access-worker.js => test-permission-env-worker.js} (89%) rename test/parallel/{test-permission-env-access.js => test-permission-env.js} (92%) diff --git a/test/parallel/test-permission-env-access-enum-clone.js b/test/parallel/test-permission-env-enum-clone.js similarity index 79% rename from test/parallel/test-permission-env-access-enum-clone.js rename to test/parallel/test-permission-env-enum-clone.js index ad46f4135e21fb..8e11376d0a6b68 100644 --- a/test/parallel/test-permission-env-access-enum-clone.js +++ b/test/parallel/test-permission-env-enum-clone.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; require('../common'); @@ -27,9 +27,9 @@ describe('permission: enumerate', () => { permission: 'Environment', }); - it('enumerate with *,-UNDEFINED', () => { + it('enumerate with --allow-env', () => { const { status } = runTest([ - '--allow-env=*,-UNDEFINED', + '--allow-env', '-e', ` // doesNotThrow @@ -39,10 +39,10 @@ describe('permission: enumerate', () => { strictEqual(status, 0); }); - it('enumerate with *,-DEFINED', () => { + it('enumerate with --allow-env=ALLOWED_ONLY', () => { const { status } = runTest( [ - '--allow-env=*,-DEFINED', + '--allow-env=ALLOWED_ONLY', '-e', ` const { throws } = require('node:assert'); @@ -54,7 +54,7 @@ describe('permission: enumerate', () => { { env: { ...process.env, - DEFINED: 0, + ALLOWED_ONLY: 0, }, } ); @@ -68,9 +68,9 @@ describe('permission: structuredClone', () => { permission: 'Environment', }); - it('structuredClone process.env with *,-UNDEFINED', () => { + it('structuredClone process.env with --allow-env', () => { const { status } = runTest([ - '--allow-env=*,-UNDEFINED', + '--allow-env', '-e', ` // doesNotThrow @@ -80,10 +80,10 @@ describe('permission: structuredClone', () => { strictEqual(status, 0); }); - it('structuredClone process.env with *,-DEFINED', () => { + it('structuredClone process.env with --allow-env=ALLOWED_ONLY', () => { const { status } = runTest( [ - '--allow-env=*,-DEFINED', + '--allow-env=ALLOWED_ONLY', '-e', ` const { throws } = require('node:assert'); @@ -95,7 +95,7 @@ describe('permission: structuredClone', () => { { env: { ...process.env, - DEFINED: 0, + ALLOWED_ONLY: 0, }, } ); diff --git a/test/parallel/test-permission-env-has.js b/test/parallel/test-permission-env-has.js index c35b5db8abc7cd..eafaf9e3e8b7a6 100644 --- a/test/parallel/test-permission-env-has.js +++ b/test/parallel/test-permission-env-has.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; require('../common'); @@ -21,150 +21,45 @@ function runTest(args, options = {}) { return { status, stdout, stderr }; } -describe('permission: has "env" with reference', () => { +describe('permission: has "env" with the reference', () => { const code = ` const has = (...args) => console.log(process.permission.has(...args)); - - has('env', 'A1'); - has('env', 'A2'); - has('env', 'B1'); - has('env', 'B2'); - has('env', 'NODE_OPTIONS'); + has('env', 'HOME'); + has('env', 'PORT'); + has('env', 'DEBUG'); `; - it('allow one', () => { - const { status, stdout } = runTest(['--allow-env=A1', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'false', - 'false', - 'false', - 'false', - ]); - }); - - it('allow more than one', () => { - const { status, stdout } = runTest(['--allow-env=A1,A2', '-e', code]); - - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'false', - 'false', - 'false', - ]); - }); - - it('allow more than one using wildcard', () => { - const { status, stdout } = runTest(['--allow-env=A*', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'false', - 'false', - 'false', - ]); - }); - - it('allow more than one with spaces', () => { - const { status, stdout } = runTest(['--allow-env=A1, A2 ', '-e', code]); + it('allow one: --allow-env=HOME', () => { + const { status, stdout } = runTest(['--allow-env=HOME', '-e', code]); strictEqual(status, 0); deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ 'true', - 'true', - 'false', 'false', 'false', ]); }); - it('allow all', () => { - const { status, stdout } = runTest(['--allow-env=*', '-e', code]); - - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'true', - 'true', - 'true', - ]); - }); - - it('allow all except one', () => { - const { status, stdout } = runTest(['--allow-env=*,-B1', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'false', - 'true', - 'true', - ]); - }); + it('allow more than one: --allow-env=HOME,PORT', () => { + const { status, stdout } = runTest(['--allow-env=HOME,PORT', '-e', code]); - it('allow all except more than one', () => { - const { status, stdout } = runTest(['--allow-env=*,-B1,-B2', '-e', code]); strictEqual(status, 0); deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ 'true', 'true', 'false', - 'false', - 'true', ]); }); - it('allow all except more than one using wildcard', () => { - const { status, stdout } = runTest(['--allow-env=*,-B*', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'false', - 'false', - 'true', - ]); - }); + it('allow all: --allow-env', () => { + const { status, stdout } = runTest(['--allow-env', '-e', code]); - it('allow all except more than one using wildcard (reverse order)', () => { - const { status, stdout } = runTest(['--allow-env=-B*,*', '-e', code]); strictEqual(status, 0); deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ 'true', 'true', - 'false', - 'false', 'true', ]); }); - - it('deny one', () => { - const { status, stdout } = runTest(['--allow-env=-B1', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'false', - 'false', - 'false', - 'false', - 'false', - ]); - }); - - it('deny all', () => { - const { status, stdout } = runTest(['--allow-env=-*', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'false', - 'false', - 'false', - 'false', - 'false', - ]); - }); }); describe('permission: has "env" with no reference', () => { @@ -176,45 +71,18 @@ describe('permission: has "env" with no reference', () => { strictEqual(status, 0); }); - it('has no reference with *', () => { + it('has no reference: --allow-env', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', 'require("assert").strictEqual(process.permission.has("env"), true);', ]); strictEqual(status, 0); }); - it('has no reference with A*', () => { - const { status } = runTest([ - '--allow-env=A*', - '-e', - 'require("assert").strictEqual(process.permission.has("env"), false);', - ]); - strictEqual(status, 0); - }); - - it('has no reference with *,-A', () => { - const { status } = runTest([ - '--allow-env=*,-A', - '-e', - 'require("assert").strictEqual(process.permission.has("env"), false);', - ]); - strictEqual(status, 0); - }); - - it('has no reference with *,-A*', () => { - const { status } = runTest([ - '--allow-env=*,-A*', - '-e', - 'require("assert").strictEqual(process.permission.has("env"), false);', - ]); - strictEqual(status, 0); - }); - - it('has no reference with -*,*', () => { + it('has no reference: --allow-env=HOME', () => { const { status } = runTest([ - '--allow-env=-*,*', + '--allow-env=HOME', '-e', 'require("assert").strictEqual(process.permission.has("env"), false);', ]); @@ -222,7 +90,7 @@ describe('permission: has "env" with no reference', () => { }); }); -describe('permission: reference', () => { +describe('permission: has "env" reference', () => { it('reference is case-sensitive', () => { const { status, stdout } = runTest([ '--allow-env=FoO', diff --git a/test/parallel/test-permission-env-access-windows.js b/test/parallel/test-permission-env-windows.js similarity index 94% rename from test/parallel/test-permission-env-access-windows.js rename to test/parallel/test-permission-env-windows.js index 7c161602d544c0..0a22d1980b5204 100644 --- a/test/parallel/test-permission-env-access-windows.js +++ b/test/parallel/test-permission-env-windows.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; const common = require('../common'); diff --git a/test/parallel/test-permission-env-access-worker.js b/test/parallel/test-permission-env-worker.js similarity index 89% rename from test/parallel/test-permission-env-access-worker.js rename to test/parallel/test-permission-env-worker.js index 26388ddbec08a6..db24f16faedc97 100644 --- a/test/parallel/test-permission-env-access-worker.js +++ b/test/parallel/test-permission-env-worker.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; require('../common'); @@ -52,7 +52,7 @@ describe('permission: "env" access on worker thread', () => { strictEqual(status, 0); }); - it('worker_threads with DENIED_IN_MAIN_THREAD', () => { + it('worker_threads with --allow-env=ALLOED_IN_MAIN_THREAD', () => { const error = JSON.stringify({ code: 'ERR_ACCESS_DENIED', permission: 'Environment', @@ -60,11 +60,11 @@ describe('permission: "env" access on worker thread', () => { const { status } = runTest([ '--allow-worker', - '--allow-env=*,-DENIED_IN_MAIN_THREAD', + '--allow-env=ALLOED_IN_MAIN_THREAD', '-e', ` const { throws, strictEqual } = require('node:assert'); - const { Worker, SHARE_ENV } = require('node:worker_threads'); + const { Worker } = require('node:worker_threads'); const w = new Worker('process.env.DENIED_IN_MAIN_THREAD', { eval: true, env: { DENIED_IN_MAIN_THREAD: 1 }, diff --git a/test/parallel/test-permission-env-access.js b/test/parallel/test-permission-env.js similarity index 92% rename from test/parallel/test-permission-env-access.js rename to test/parallel/test-permission-env.js index 9b271c7e29a4bf..2dab41417345f9 100644 --- a/test/parallel/test-permission-env-access.js +++ b/test/parallel/test-permission-env.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* --allow-child-process +// Flags: --experimental-permission --allow-env --allow-fs-read=* --allow-child-process 'use strict'; require('../common'); @@ -100,10 +100,10 @@ describe('permission: "env" access', () => { }); }); -describe('permission: "env" access with --allow-env=*', () => { +describe('permission: "env" access with --allow-env', () => { it('get', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow @@ -115,7 +115,7 @@ describe('permission: "env" access with --allow-env=*', () => { it('set', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow @@ -127,7 +127,7 @@ describe('permission: "env" access with --allow-env=*', () => { it('query', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow @@ -139,7 +139,7 @@ describe('permission: "env" access with --allow-env=*', () => { it('delete', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow @@ -151,7 +151,7 @@ describe('permission: "env" access with --allow-env=*', () => { it('enumerate', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow @@ -163,7 +163,7 @@ describe('permission: "env" access with --allow-env=*', () => { it('structuredClone', () => { const { status } = runTest([ - '--allow-env=*', + '--allow-env', '-e', ` // doesNotThrow diff --git a/test/parallel/test-permission-has.js b/test/parallel/test-permission-has.js index c21c73ed81e17b..55c0f5971a211d 100644 --- a/test/parallel/test-permission-has.js +++ b/test/parallel/test-permission-has.js @@ -1,4 +1,4 @@ -// Flags: --experimental-permission --allow-env=* --allow-fs-read=* +// Flags: --experimental-permission --allow-env --allow-fs-read=* 'use strict'; const common = require('../common'); @@ -21,21 +21,3 @@ const assert = require('assert'); message: 'The "reference" argument must be of type string. Received an instance of Object', })); } - -{ - assert.throws(() => { - process.permission.has('env', ''); - }, common.expectsError({ - code: 'ERR_INVALID_ARG_VALUE' - })); - assert.throws(() => { - process.permission.has('env', '0'); - }, common.expectsError({ - code: 'ERR_INVALID_ARG_VALUE' - })); - assert.throws(() => { - process.permission.has('env', '*'); - }, common.expectsError({ - code: 'ERR_INVALID_ARG_VALUE' - })); -} From 4f5e3f27df7a5744573e8e81e2d395c7918455dc Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Tue, 23 May 2023 23:04:28 +0900 Subject: [PATCH 13/14] dev: add --allow-env-name Signed-off-by: Daeyeon Jeong --- doc/api/cli.md | 3 +- src/env.cc | 4 +- src/node_options.cc | 6 +- src/node_options.h | 4 +- test/parallel/test-permission-env-has.js | 94 +++++++++++++----------- 5 files changed, 58 insertions(+), 53 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 8ed688c72203df..61dcf855cfb8f3 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -2157,8 +2157,7 @@ Node.js options that are allowed are: * `--allow-child-process` -* `--allow-env-key` -* `--allow-env` +* `--allow-env`, `--allow-env-name` * `--allow-fs-read` * `--allow-fs-write` * `--allow-worker` diff --git a/src/env.cc b/src/env.cc index c690a16317e001..c3b635a7521bb8 100644 --- a/src/env.cc +++ b/src/env.cc @@ -814,8 +814,8 @@ Environment::Environment(IsolateData* isolate_data, permission::PermissionScope::kFileSystemWrite); } - if (options_->allow_env || !options_->allow_env_key.empty()) { - permission()->Apply(options_->allow_env_key, + if (options_->allow_env || !options_->allow_env_name.empty()) { + permission()->Apply(options_->allow_env_name, permission::PermissionScope::kEnvironment); } } diff --git a/src/node_options.cc b/src/node_options.cc index f9e010f524bdbf..8e546a52a5d040 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -417,15 +417,15 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { kAllowedInEnvvar); Implies("--policy-integrity", "[has_policy_integrity_string]"); AddOption( - "--allow-env-key", + "--allow-env-name", "allow permissions to access specified keys of the environment variables", - &EnvironmentOptions::allow_env_key, + &EnvironmentOptions::allow_env_name, kAllowedInEnvvar); AddOption("--allow-env", "allow permissions to access the environment variables", &EnvironmentOptions::allow_env, kAllowedInEnvvar); - AddAlias("--allow-env=", {"--allow-env-key", "--allow-env"}); + AddAlias("--allow-env=", {"--allow-env-name", "--allow-env"}); AddOption("--allow-fs-read", "allow permissions to read the filesystem", &EnvironmentOptions::allow_fs_read, diff --git a/src/node_options.h b/src/node_options.h index 7a021cd1ccd300..c6f10416bd408a 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -121,8 +121,8 @@ class EnvironmentOptions : public Options { std::string experimental_policy_integrity; bool has_policy_integrity_string = false; bool experimental_permission = false; - bool allow_env; - std::string allow_env_key; + bool allow_env = false; + std::string allow_env_name; std::string allow_fs_read; std::string allow_fs_write; bool allow_child_process = false; diff --git a/test/parallel/test-permission-env-has.js b/test/parallel/test-permission-env-has.js index eafaf9e3e8b7a6..71d97232d69c43 100644 --- a/test/parallel/test-permission-env-has.js +++ b/test/parallel/test-permission-env-has.js @@ -29,28 +29,30 @@ describe('permission: has "env" with the reference', () => { has('env', 'DEBUG'); `; - it('allow one: --allow-env=HOME', () => { - const { status, stdout } = runTest(['--allow-env=HOME', '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'false', - 'false', - ]); - }); + for (const flag of ['--allow-env', '--allow-env-name']) { + it(`has: ${flag}=HOME`, () => { + const { status, stdout } = runTest([`${flag}=HOME`, '-e', code]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'false', + 'false', + ]); + }); - it('allow more than one: --allow-env=HOME,PORT', () => { - const { status, stdout } = runTest(['--allow-env=HOME,PORT', '-e', code]); + it(`has: ${flag}=HOME,PORT`, () => { + const { status, stdout } = runTest([`${flag}=HOME,PORT`, '-e', code]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'true', - 'true', - 'false', - ]); - }); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'true', + 'true', + 'false', + ]); + }); + } - it('allow all: --allow-env', () => { + it('has: --allow-env', () => { const { status, stdout } = runTest(['--allow-env', '-e', code]); strictEqual(status, 0); @@ -80,32 +82,36 @@ describe('permission: has "env" with no reference', () => { strictEqual(status, 0); }); - it('has no reference: --allow-env=HOME', () => { - const { status } = runTest([ - '--allow-env=HOME', - '-e', - 'require("assert").strictEqual(process.permission.has("env"), false);', - ]); - strictEqual(status, 0); - }); + for (const flag of ['--allow-env', '--allow-env-name']) { + it(`has no reference: ${flag}=HOME`, () => { + const { status } = runTest([ + `${flag}=HOME`, + '-e', + 'require("assert").strictEqual(process.permission.has("env"), false);', + ]); + strictEqual(status, 0); + }); + } }); describe('permission: has "env" reference', () => { - it('reference is case-sensitive', () => { - const { status, stdout } = runTest([ - '--allow-env=FoO', - '-e', - ` - console.log(process.permission.has('env', 'FOO')); - console.log(process.permission.has('env', 'foo')); - console.log(process.permission.has('env', 'FoO')); - `, - ]); - strictEqual(status, 0); - deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ - 'false', - 'false', - 'true', - ]); - }); + for (const flag of ['--allow-env', '--allow-env-name']) { + it(`reference is case-sensitive: ${flag}=FoO`, () => { + const { status, stdout } = runTest([ + `${flag}=FoO`, + '-e', + ` + console.log(process.permission.has('env', 'FOO')); + console.log(process.permission.has('env', 'foo')); + console.log(process.permission.has('env', 'FoO')); + `, + ]); + strictEqual(status, 0); + deepStrictEqual(stdout.toString().split('\n').slice(0, -1), [ + 'false', + 'false', + 'true', + ]); + }); + } }); From 5f72c0fdb3f3fac4b4f9b8f9ef64cd47d811f375 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 24 May 2023 00:00:26 +0900 Subject: [PATCH 14/14] dev: update docs Signed-off-by: Daeyeon Jeong --- doc/api/cli.md | 23 +++++++++++------------ doc/api/permissions.md | 34 ++++++++-------------------------- 2 files changed, 19 insertions(+), 38 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 61dcf855cfb8f3..08684b659f9a9d 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -141,7 +141,7 @@ Error: Access to this API has been restricted } ``` -### `--allow-env` +### `--allow-env`, `--allow-env-name`