Skip to content

Commit 90d3623

Browse files
committed
src: allow --disallow-code-generation-from-strings in workers
Make --disallow-code-generation-from-strings a per-isolate option instead of a V8-only option, allowing it to be passed via worker execArgv. Fixes: #60371
1 parent a7d9c49 commit 90d3623

File tree

8 files changed

+122
-9
lines changed

8 files changed

+122
-9
lines changed

src/api/environment.cc

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ Environment* CreateEnvironment(
488488
CHECK(!context.IsEmpty());
489489
Context::Scope context_scope(context);
490490

491-
if (InitializeContextRuntime(context).IsNothing()) {
491+
if (InitializeContextRuntime(context, isolate_data).IsNothing()) {
492492
FreeEnvironment(env);
493493
return nullptr;
494494
}
@@ -670,10 +670,16 @@ MaybeLocal<Object> GetPerContextExports(Local<Context> context,
670670
// call NewContext and so they will experience breakages.
671671
Local<Context> NewContext(Isolate* isolate,
672672
Local<ObjectTemplate> object_template) {
673+
return NewContext(isolate, object_template, nullptr);
674+
}
675+
676+
Local<Context> NewContext(Isolate* isolate,
677+
Local<ObjectTemplate> object_template,
678+
IsolateData* isolate_data) {
673679
auto context = Context::New(isolate, nullptr, object_template);
674680
if (context.IsEmpty()) return context;
675681

676-
if (InitializeContext(context).IsNothing()) {
682+
if (InitializeContext(context, isolate_data).IsNothing()) {
677683
return Local<Context>();
678684
}
679685

@@ -686,7 +692,8 @@ void ProtoThrower(const FunctionCallbackInfo<Value>& info) {
686692

687693
// This runs at runtime, regardless of whether the context
688694
// is created from a snapshot.
689-
Maybe<void> InitializeContextRuntime(Local<Context> context) {
695+
Maybe<void> InitializeContextRuntime(Local<Context> context,
696+
IsolateData* isolate_data) {
690697
Isolate* isolate = Isolate::GetCurrent();
691698
HandleScope handle_scope(isolate);
692699

@@ -698,6 +705,18 @@ Maybe<void> InitializeContextRuntime(Local<Context> context) {
698705
// to the runtime flags, propagate the value to the embedder data.
699706
bool is_code_generation_from_strings_allowed =
700707
context->IsCodeGenerationFromStringsAllowed();
708+
709+
// Check if the Node.js option --disallow-code-generation-from-strings is set
710+
// Use isolate_data if provided, otherwise fall back to per_process options
711+
if (isolate_data != nullptr &&
712+
isolate_data->options()->disallow_code_generation_from_strings) {
713+
is_code_generation_from_strings_allowed = false;
714+
} else if (isolate_data == nullptr &&
715+
per_process::cli_options->per_isolate
716+
->disallow_code_generation_from_strings) {
717+
is_code_generation_from_strings_allowed = false;
718+
}
719+
701720
context->AllowCodeGenerationFromStrings(false);
702721
context->SetEmbedderData(
703722
ContextEmbedderIndex::kAllowCodeGenerationFromStrings,
@@ -922,11 +941,16 @@ Maybe<void> InitializePrimordials(Local<Context> context,
922941

923942
// This initializes the main context (i.e. vm contexts are not included).
924943
Maybe<bool> InitializeContext(Local<Context> context) {
944+
return InitializeContext(context, nullptr);
945+
}
946+
947+
Maybe<bool> InitializeContext(Local<Context> context,
948+
IsolateData* isolate_data) {
925949
if (InitializeMainContextForSnapshot(context).IsNothing()) {
926950
return Nothing<bool>();
927951
}
928952

929-
if (InitializeContextRuntime(context).IsNothing()) {
953+
if (InitializeContextRuntime(context, isolate_data).IsNothing()) {
930954
return Nothing<bool>();
931955
}
932956
return Just(true);

src/node.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,10 +567,20 @@ NODE_EXTERN v8::Local<v8::Context> NewContext(
567567
v8::Local<v8::ObjectTemplate> object_template =
568568
v8::Local<v8::ObjectTemplate>());
569569

570+
// Overload that accepts IsolateData for per-isolate options
571+
v8::Local<v8::Context> NewContext(
572+
v8::Isolate* isolate,
573+
v8::Local<v8::ObjectTemplate> object_template,
574+
IsolateData* isolate_data);
575+
570576
// Runs Node.js-specific tweaks on an already constructed context
571577
// Return value indicates success of operation
572578
NODE_EXTERN v8::Maybe<bool> InitializeContext(v8::Local<v8::Context> context);
573579

580+
// Overload that accepts IsolateData for per-isolate options
581+
v8::Maybe<bool> InitializeContext(v8::Local<v8::Context> context,
582+
IsolateData* isolate_data);
583+
574584
// If `platform` is passed, it will be used to register new Worker instances.
575585
// It can be `nullptr`, in which case creating new Workers inside of
576586
// Environments that use this `IsolateData` will not work.

src/node_contextify.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ ContextifyContext* ContextifyContext::New(Local<Context> v8_context,
253253
// only initialized when needed because even deserializing them slows
254254
// things down significantly and they are only needed in rare occasions
255255
// in the vm contexts.
256-
if (InitializeContextRuntime(v8_context).IsNothing()) {
256+
if (InitializeContextRuntime(v8_context, env->isolate_data()).IsNothing()) {
257257
return {};
258258
}
259259

src/node_internals.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ std::string GetHumanReadableProcessName();
116116

117117
v8::Maybe<void> InitializeBaseContextForSnapshot(
118118
v8::Local<v8::Context> context);
119-
v8::Maybe<void> InitializeContextRuntime(v8::Local<v8::Context> context);
119+
v8::Maybe<void> InitializeContextRuntime(v8::Local<v8::Context> context,
120+
IsolateData* isolate_data);
120121
v8::Maybe<void> InitializePrimordials(v8::Local<v8::Context> context,
121122
IsolateData* isolate_data);
122123
v8::MaybeLocal<v8::Object> InitializePrivateSymbols(

src/node_options.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1172,7 +1172,7 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
11721172
kAllowedInEnvvar);
11731173
AddOption("--disallow-code-generation-from-strings",
11741174
"disallow eval and friends",
1175-
V8Option{},
1175+
&PerIsolateOptions::disallow_code_generation_from_strings,
11761176
kAllowedInEnvvar);
11771177
AddOption("--jitless",
11781178
"disable runtime allocation of executable memory",

src/node_options.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ class PerIsolateOptions : public Options {
293293
std::string max_old_space_size;
294294
int64_t stack_trace_limit = 10;
295295
std::string report_signal = "SIGUSR2";
296+
bool disallow_code_generation_from_strings = false;
296297
bool build_snapshot = false;
297298
std::string build_snapshot_config;
298299
inline EnvironmentOptions* get_per_env_options();

src/node_worker.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,13 +352,14 @@ void Worker::Run() {
352352
SnapshotData::kNodeBaseContextIndex)
353353
.ToLocalChecked();
354354
if (!context.IsEmpty() &&
355-
!InitializeContextRuntime(context).IsJust()) {
355+
!InitializeContextRuntime(context, data.isolate_data_.get()).IsJust()) {
356356
context = Local<Context>();
357357
}
358358
} else {
359359
Debug(
360360
this, "Worker %llu builds context from scratch\n", thread_id_.id);
361-
context = NewContext(isolate_);
361+
context = NewContext(isolate_, Local<ObjectTemplate>(),
362+
data.isolate_data_.get());
362363
}
363364
if (context.IsEmpty()) {
364365
// TODO(joyeecheung): maybe this should be kBootstrapFailure instead?
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const { Worker } = require('worker_threads');
5+
6+
// Test that --disallow-code-generation-from-strings can be passed to workers
7+
// and properly blocks eval() and related code generation functions.
8+
9+
// Test 1: Worker with --disallow-code-generation-from-strings should block eval
10+
{
11+
const worker = new Worker(`
12+
const { parentPort } = require('worker_threads');
13+
try {
14+
eval('"test"');
15+
parentPort.postMessage({ evalBlocked: false });
16+
} catch (err) {
17+
parentPort.postMessage({ evalBlocked: true, errorName: err.name });
18+
}
19+
`, {
20+
eval: true,
21+
execArgv: ['--disallow-code-generation-from-strings']
22+
});
23+
24+
worker.on('message', common.mustCall((msg) => {
25+
assert.strictEqual(msg.evalBlocked, true);
26+
assert.strictEqual(msg.errorName, 'EvalError');
27+
}));
28+
29+
worker.on('error', common.mustNotCall());
30+
}
31+
32+
// Test 2: Worker without the flag should allow eval
33+
{
34+
const worker = new Worker(`
35+
const { parentPort } = require('worker_threads');
36+
try {
37+
const result = eval('"test"');
38+
parentPort.postMessage({ evalBlocked: false, result });
39+
} catch (err) {
40+
parentPort.postMessage({ evalBlocked: true });
41+
}
42+
`, {
43+
eval: true,
44+
execArgv: []
45+
});
46+
47+
worker.on('message', common.mustCall((msg) => {
48+
assert.strictEqual(msg.evalBlocked, false);
49+
assert.strictEqual(msg.result, 'test');
50+
}));
51+
52+
worker.on('error', common.mustNotCall());
53+
}
54+
55+
// Test 3: Verify the flag also blocks Function constructor
56+
{
57+
const worker = new Worker(`
58+
const { parentPort } = require('worker_threads');
59+
try {
60+
new Function('return 42')();
61+
parentPort.postMessage({ functionBlocked: false });
62+
} catch (err) {
63+
parentPort.postMessage({ functionBlocked: true, errorName: err.name });
64+
}
65+
`, {
66+
eval: true,
67+
execArgv: ['--disallow-code-generation-from-strings']
68+
});
69+
70+
worker.on('message', common.mustCall((msg) => {
71+
assert.strictEqual(msg.functionBlocked, true);
72+
assert.strictEqual(msg.errorName, 'EvalError');
73+
}));
74+
75+
worker.on('error', common.mustNotCall());
76+
}

0 commit comments

Comments
 (0)