Skip to content

Commit

Permalink
process: generate list of allowed env flags programmatically
Browse files Browse the repository at this point in the history
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
addaleax authored and targos committed Sep 20, 2018
1 parent ed142e1 commit 4ab9d6f
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 92 deletions.
45 changes: 42 additions & 3 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,9 +620,45 @@
const test = Function.call.bind(RegExp.prototype.test);

const {
allowedV8EnvironmentFlags,
allowedNodeEnvironmentFlags
} = process.binding('config');
getOptions,
types: { kV8Option },
envSettings: { kAllowedInEnvironment }
} = internalBinding('options');
const { options, aliases } = getOptions();

const allowedV8EnvironmentFlags = [];
const allowedNodeEnvironmentFlags = [];
for (const [name, info] of options) {
if (info.envVarSettings === kAllowedInEnvironment) {
if (info.type === kV8Option) {
allowedV8EnvironmentFlags.push(name);
} else {
allowedNodeEnvironmentFlags.push(name);
}
}
}

for (const [ from, expansion ] of aliases) {
let isAccepted = true;
for (const to of expansion) {
if (!to.startsWith('-')) continue;
const recursiveExpansion = aliases.get(to);
if (recursiveExpansion) {
expansion.push(...recursiveExpansion);
continue;
}
isAccepted = options.get(to).envVarSettings === kAllowedInEnvironment;
if (!isAccepted) break;
}
if (isAccepted) {
let canonical = from;
if (canonical.endsWith('='))
canonical = canonical.substr(0, canonical.length - 1);
if (canonical.endsWith(' <arg>'))
canonical = canonical.substr(0, canonical.length - 4);
allowedNodeEnvironmentFlags.push(canonical);
}
}

const trimLeadingDashes = (flag) => replace(flag, leadingDashesRegex, '');

Expand Down Expand Up @@ -660,6 +696,9 @@
// permutations of a flag, including present/missing leading
// dash(es) and/or underscores-for-dashes in the case of V8-specific
// flags. Strips any values after `=`, inclusive.
// TODO(addaleax): It might be more flexible to run the option parser
// on a dummy option set and see whether it rejects the argument or
// not.
if (typeof key === 'string') {
key = replace(key, trailingValuesRegex, '');
if (test(leadingDashesRegex, key)) {
Expand Down
62 changes: 0 additions & 62 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -597,68 +597,6 @@ const char* signo_string(int signo) {
}
}

// These are all flags available for use with NODE_OPTIONS.
//
// Disallowed flags:
// These flags cause Node to do things other than run scripts:
// --version / -v
// --eval / -e
// --print / -p
// --check / -c
// --interactive / -i
// --prof-process
// --v8-options
// These flags are disallowed because security:
// --preserve-symlinks
const char* const environment_flags[] = {
// Node options, sorted in `node --help` order for ease of comparison.
"--enable-fips",
"--experimental-modules",
"--experimenatl-repl-await",
"--experimental-vm-modules",
"--experimental-worker",
"--force-fips",
"--icu-data-dir",
"--inspect",
"--inspect-brk",
"--inspect-port",
"--loader",
"--napi-modules",
"--no-deprecation",
"--no-force-async-hooks-checks",
"--no-warnings",
"--openssl-config",
"--pending-deprecation",
"--redirect-warnings",
"--require",
"--throw-deprecation",
"--tls-cipher-list",
"--trace-deprecation",
"--trace-event-categories",
"--trace-event-file-pattern",
"--trace-events-enabled",
"--trace-sync-io",
"--trace-warnings",
"--track-heap-objects",
"--use-bundled-ca",
"--use-openssl-ca",
"--v8-pool-size",
"--zero-fill-buffers",
"-r"
};

// V8 options (define with '_', which allows '-' or '_')
const char* const v8_environment_flags[] = {
"--abort_on_uncaught_exception",
"--max_old_space_size",
"--perf_basic_prof",
"--perf_prof",
"--stack_trace_limit",
};

int v8_environment_flags_count = arraysize(v8_environment_flags);
int environment_flags_count = arraysize(environment_flags);

// Look up environment variable unless running as setuid root.
bool SafeGetenv(const char* key, std::string* text) {
#if !defined(__CloudABI__) && !defined(_WIN32)
Expand Down
17 changes: 0 additions & 17 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

namespace node {

using v8::Array;
using v8::Boolean;
using v8::Context;
using v8::Integer;
Expand Down Expand Up @@ -137,22 +136,6 @@ static void Initialize(Local<Object> target,
READONLY_PROPERTY(debug_options_obj,
"inspectorEnabled",
Boolean::New(isolate, debug_options->inspector_enabled));

Local<Array> environmentFlags = Array::New(env->isolate(),
environment_flags_count);
READONLY_PROPERTY(target, "allowedNodeEnvironmentFlags", environmentFlags);
for (int i = 0; i < environment_flags_count; ++i) {
environmentFlags->Set(i, OneByteString(env->isolate(),
environment_flags[i]));
}

Local<Array> v8EnvironmentFlags = Array::New(env->isolate(),
v8_environment_flags_count);
READONLY_PROPERTY(target, "allowedV8EnvironmentFlags", v8EnvironmentFlags);
for (int i = 0; i < v8_environment_flags_count; ++i) {
v8EnvironmentFlags->Set(i, OneByteString(env->isolate(),
v8_environment_flags[i]));
}
} // InitConfig

} // namespace node
Expand Down
5 changes: 0 additions & 5 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,6 @@ extern bool v8_initialized;
extern Mutex per_process_opts_mutex;
extern std::shared_ptr<PerProcessOptions> per_process_opts;

extern const char* const environment_flags[];
extern int environment_flags_count;
extern const char* const v8_environment_flags[];
extern int v8_environment_flags_count;

// Forward declaration
class Environment;

Expand Down
11 changes: 6 additions & 5 deletions test/parallel/test-process-env-allowed-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ require('../common');
// assert legit flags are allowed, and bogus flags are disallowed
{
const goodFlags = [
'--inspect-brk',
'inspect-brk',
'--perf_basic_prof',
'--perf-basic-prof',
'perf-basic-prof',
Expand All @@ -17,8 +15,11 @@ require('../common');
'-r',
'r',
'--stack-trace-limit=100',
'--stack-trace-limit=-=xX_nodejs_Xx=-'
];
'--stack-trace-limit=-=xX_nodejs_Xx=-',
].concat(process.config.variables.v8_enable_inspector ? [
'--inspect-brk',
'inspect-brk',
] : []);

const badFlags = [
'--inspect_brk',
Expand Down Expand Up @@ -50,7 +51,7 @@ require('../common');
// assert all "canonical" flags begin with dash(es)
{
process.allowedNodeEnvironmentFlags.forEach((flag) => {
assert(/^--?[a-z8_-]+$/.test(flag), `Unexpected format for flag ${flag}`);
assert(/^--?[a-z28_-]+$/.test(flag), `Unexpected format for flag ${flag}`);
});
}

Expand Down

0 comments on commit 4ab9d6f

Please sign in to comment.