Skip to content

process: generate list of allowed env flags programmatically #22638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
addaleax wants to merge 4 commits into from
Closed

process: generate list of allowed env flags programmatically #22638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aec3668
process: generate list of allowed env flags programmatically
addaleax Aug 31, 2018
f05091c
fixup! process: generate list of allowed env flags programmatically
addaleax Sep 2, 2018
0002b1e
lib: generate allowedNodeEnvironmentFlags lazily
addaleax Aug 31, 2018
1f37916
fixup! process: generate list of allowed env flags programmatically
addaleax Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
process: generate list of allowed env flags programmatically 
Avoids having a separate, second source of truth on this matter.
  • Loading branch information
@addaleax
addaleax committed Sep 5, 2018
commit aec3668e1cb41f846fa8dfebd42ab58b9c870dec
42 changes: 39 additions & 3 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,42 @@
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 =
[...options].filter(([name, info]) =>
info.type === kV8Option &&
info.envVarSettings === kAllowedInEnvironment)
.map(([name]) => name);

@BridgeAR BridgeAR Sep 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to pass through filter options into getOptions instead of doing the filtering here.
Since it has a string option at the moment, it would likely be best to accept an object for the options instead.

Another alternative to the current way would be to only iterate once over the entries (this is also nicer to read for me):

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);
    }
  }
}

@addaleax addaleax Sep 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve gone with the second option for now, but yes, ultimately it might be better to do this in C++ land.

const allowedNodeEnvironmentFlags =
[...options].filter(([name, info]) =>
info.type !== kV8Option &&
info.envVarSettings === kAllowedInEnvironment)
.map(([name]) => name);

for (const [ from, expansion ] of aliases) {
let isAccepted = true;
for (const to of expansion) {
if (!to.startsWith('-')) continue;
if (aliases.get(to)) {
expansion.push(...aliases.get(to));

@BridgeAR BridgeAR Sep 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: retrieving the value multiple times is not necessary.

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 @@ -703,6 +736,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,68 +590,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
View file
Open in desktop
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
View file
Open in desktop
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