Skip to content

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

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

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

Conversation

@mcollina

@mcollinamcollina commented Nov 2, 2025

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Nov 2, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-botnodejs-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
Comment threadsrc/node.h Outdated
@mcollina

mcollina commented Nov 2, 2025

Copy link
Copy Markdown
MemberAuthor

cc @legendecas I presume this is incorrect then.

legendecas
legendecas reviewed Nov 2, 2025
View reviewed changes
Comment threadsrc/api/environment.cc Outdated
@@ -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 =

@legendecaslegendecasNov 2, 2025•
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.

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.

@mcollinamcollinaJun 16, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@legendecas this has been open for so long, does this need addressing?

@legendecas

legendecas commented Nov 2, 2025•
edited
Loading

Copy link
Copy Markdown
Member

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

@legendecaslegendecas left a comment

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.

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
mcollinaforce-pushed the fix-disallow-codegen-workers branch from 7c3a9d5 to 90d3623CompareNovember 7, 2025 09:42
@mcollina
mcollina marked this pull request as ready for review November 19, 2025 11:35
@mcollina

mcollina commented Nov 19, 2025

Copy link
Copy Markdown
MemberAuthor

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

@codecov

codecovBot commented Nov 19, 2025•
edited
Loading

Copy link
Copy Markdown

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 linesPatch %Lines
src/api/environment.cc80.00%1 Missing and 2 partials ⚠️
src/node_worker.cc66.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 linesCoverage Δ
src/node.h95.91% <ø> (ø)
src/node_contextify.cc82.45% <ø> (+0.66%)⬆️
src/node_internals.h83.01% <ø> (ø)
src/node_options.cc77.90% <ø> (+0.06%)⬆️
src/node_options.h97.90% <100.00%> (+0.04%)⬆️
src/node_worker.cc81.64% <66.66%> (+0.03%)⬆️
src/api/environment.cc76.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.

@legendecas

legendecas commented Jan 8, 2026

Copy link
Copy Markdown
Member

Might need a format-cpp.

@SudhansuBandha

SudhansuBandha commented Jun 11, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

Hi @mcollina ,

Since the PR is inactive for a while and it needs to have format-cpp.

Would you still like to move this PR forward? If helpful, I'd be happy to assist with rebasing, formatting, or testing against current main.

Thanks!

@mcollina
mcollinaforce-pushed the fix-disallow-codegen-workers branch from 971be87 to 02c96cfCompareJune 12, 2026 07:03
@mcollina
src: allow --disallow-code-generation-from-strings in workers
cf47090
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
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
mcollinaforce-pushed the fix-disallow-codegen-workers branch from 02c96cf to cf47090CompareJune 16, 2026 09:17
@mcollina
fixup: address review comment on InitializeContextRuntime
06063c4
Do not rely on IsCodeGenerationFromStringsAllowed() since V8Option{}
was removed for --disallow-code-generation-from-strings in node_options.cc.
Instead, determine the value directly from the Node.js option.
Refs: nodejs#60549 (comment)
PR-URL: nodejs#60549
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPandaShogunPandaShogunPanda approved these changes
@legendecaslegendecaslegendecas approved these changes
@addaleaxaddaleaxAwaiting requested review from addaleax

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs 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

6 participants

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