Skip to content

src: allow --disallow-code-generation-from-strings in workers#60549

Open
mcollina wants to merge 2 commits intonodejs:mainfrom
mcollina:fix-disallow-codegen-workers
Open

src: allow --disallow-code-generation-from-strings in workers#60549
mcollina wants to merge 2 commits intonodejs:mainfrom
mcollina:fix-disallow-codegen-workers

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Nov 2, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 2, 2025

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 2, 2025
mcollina
mcollina commented Nov 2, 2025
View reviewed changes
@mcollina
Copy link
Member Author

mcollina commented Nov 2, 2025

cc @legendecas I presume this is incorrect then.

legendecas
legendecas reviewed Nov 2, 2025
View reviewed changes
src/api/environment.cc
@@ -703,6 +700,18 @@ Maybe<void> InitializeContextRuntime(Local<Context> context) {
// to the runtime flags, propagate the value to the embedder data.
bool is_code_generation_from_strings_allowed =
Copy link
Member

@legendecas legendecas Nov 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The V8Option{} is removed in node_options.cc for --disallow-code-generation-from-strings, this will always be true. Because the flag is not set in V8.

@legendecas
Copy link
Member

legendecas commented Nov 2, 2025 •
edited
Loading

I presume this is incorrect then.

My comment at #60371 (comment) was answering the question "why aren't v8 options supported for a worker thread?" and to most V8 options, we should not change them for a single worker.

However, we declared a same name flag --disallow-code-generation-from-strings in Node.js (it was derived from the V8 flag). It is safe if we didn't use the APIs like V8::SetFlagsFromString, which sets the per-process V8 flag storage.

In short, I think this PR should be good.

legendecas
legendecas reviewed Nov 2, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

A side-effect of this could be add-ons using v8::Context::New API directly, and these contexts will always allow code generation regardless if --disallow-code-generation-from-strings is specified or not.

@mcollina
src: allow --disallow-code-generation-from-strings in workers
90d3623 
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: nodejs#60371
@mcollina mcollina force-pushed the fix-disallow-codegen-workers branch from 7c3a9d5 to 90d3623 Compare November 7, 2025 09:42
@mcollina mcollina marked this pull request as ready for review November 19, 2025 11:35
@mcollina
Copy link
Member Author

mcollina commented Nov 19, 2025

@LegendCas @addaleax can you take another look? This should be ready.

@codecov
Copy link

codecov bot commented Nov 19, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (9bfff20) to head (971be87).
⚠️ Report is 340 commits behind head on main.

Files with missing lines Patch % Lines
src/api/environment.cc 80.00% 1 Missing and 2 partials ⚠️
src/node_worker.cc 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #60549      +/-   ##
==========================================
- Coverage   88.58%   88.52%   -0.07%     
==========================================
  Files         704      703       -1     
  Lines      207777   208447     +670     
  Branches    40033    40196     +163     
==========================================
+ Hits       184068   184525     +457     
- Misses      15755    15936     +181     
- Partials     7954     7986      +32
Files with missing lines Coverage Δ
src/node.h 95.91% <ø> (ø)
src/node_contextify.cc 82.45% <ø> (+0.66%) ⬆️
src/node_internals.h 83.01% <ø> (ø)
src/node_options.cc 77.90% <ø> (+0.06%) ⬆️
src/node_options.h 97.90% <100.00%> (+0.04%) ⬆️
src/node_worker.cc 81.64% <66.66%> (+0.03%) ⬆️
src/api/environment.cc 76.59% <80.00%> (-0.25%) ⬇️

... and 133 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
src: fix line length lint error in node_worker.cc
971be87 
Break long line to comply with 80-character limit.
@legendecas
Copy link
Member

legendecas commented Jan 8, 2026

Might need a format-cpp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecas legendecas legendecas approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for "--disallow-code-generation-from-strings" flag to workers

4 participants

@mcollina @nodejs-github-bot @legendecas @addaleax