FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: allow --disallow-code-generation-from-strings in workers by mcollina · Pull Request #60549 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (2)ChecksFiles changed
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 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 commented Nov 2, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config

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 commented Nov 2, 2025
View reviewed changes
Comment thread src/node.h Outdated

mcollina commented Nov 2, 2025

Copy link
Copy Markdown
Member Author

cc @legendecas I presume this is incorrect then.

legendecas reviewed Nov 2, 2025
View reviewed changes
Comment thread src/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 =

legendecas Nov 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mcollina Jun 16, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 reviewed Nov 2, 2025
View reviewed changes

legendecas 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina commented Nov 19, 2025

Copy link
Copy Markdown
Member Author

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

codecov Bot 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 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.

legendecas commented Jan 8, 2026

Copy link
Copy Markdown
Member

Might need a format-cpp.

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 force-pushed the fix-disallow-codegen-workers branch from 971be87 to 02c96cf Compare June 12, 2026 07:03
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 force-pushed the fix-disallow-codegen-workers branch from 02c96cf to cf47090 Compare June 16, 2026 09:17
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ShogunPanda ShogunPanda approved these changes
legendecas legendecas approved these changes
addaleax Awaiting 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 / 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

6 participants

Back | FazBrowse Home | New Git URL