Allow `new Function()` with undefined source when eval is blocked by petebacondarwin · Pull Request #6344 · cloudflare/workerd

petebacondarwin · 2026-03-17T15:07:55Z

Summary

Allow new Function() with no arguments and class Foo extends Function { constructor() { super(); } } to succeed even when eval/code-generation-from-strings is blocked. These produce empty functions with no user-provided code, so they pose no code-injection risk.

How it works

V8's ModifyCodeGenerationFromStringsCallback is invoked whenever code generation from strings is attempted. The callback receives a source value and an isCodeLike flag. This PR adds two guards before the existing evalAllowed check:

Guard 1: Undefined source

if (source->IsUndefined()) {
  return {.codegen_allowed = true, .modified_source = {}};
}

When source is undefined (e.g. eval() with no argument or eval(undefined)), there is no string to compile. V8 returns undefined as-is per spec ("If x is not a String, return x"). We allow it through without further checks. (Wasm compilation uses a separate callback — AllowWasmCodeGenerationCallback — and never reaches this path.)

Other non-string primitives (null, true, 42, bigint, symbol) are converted to strings by V8 via ToString() before reaching this callback, so they hit the normal string-based code generation path and don't need special handling here.

Guard 2: Empty-body `new Function()` (exact string match + `isCodeLike`)

static constexpr auto kEmptyFunctionSource = "(function anonymous(\n) {\n\n})"_kj;
if (isCodeLike) {
  // ... exact match against kEmptyFunctionSource ...
}

When the Function constructor is called with no arguments, V8's CreateDynamicFunction (builtins-function.cc:46-71) synthesizes the full source string (function anonymous(\n) {\n\n}) before calling the callback. This string contains no user-provided code. We match it exactly.

Why this is not vulnerable to code injection

The combination of isCodeLike gating and exact string matching prevents all known attack vectors:

Attack vector	Why it's blocked
`eval("<evil> {\n\n}")`	`isCodeLike` is always `false` on the eval path, so Guard 2 is never entered
`new Function('a', 'b', 'return a+b')`	`isCodeLike` is `false` when arguments are plain strings (not CodeLike objects), so Guard 2 is never entered. Even if it were, the synthesized source includes the body and would not match the exact pattern
`new Function('a', 'b', undefined)`	Body becomes `"undefined"` via `ToString()`. `isCodeLike` is `false`. Source includes the body text. Blocked on both counts
`new Function('a}){evil()}{(', '')`	Parameters pollute the source string making it not match. `isCodeLike` is `false`. Blocked on both counts
Passing a CodeLike object to `new Function(codeLikeObj)`	`isCodeLike` would be `true`, but the body is `ToString(codeLikeObj)` which produces a non-matching source string

We intentionally only match the no-parameter, no-body case. Any call to new Function() with arguments produces a different synthesized source that will not match kEmptyFunctionSource.

V8 update safety

The kEmptyFunctionSource constant is tied to V8's CreateDynamicFunction format. If V8 ever changes this format:

The constant stops matching, and new Function() gets blocked (fails safe, never allows)
The setup-test and worker-test will fail, signaling the constant needs updating

Changes

src/workerd/jsg/setup.c++ — Rewrite modifyCodeGenCallback with two guards: undefined source passthrough + exact-match for empty Function
src/workerd/jsg/setup-test.c++ — C++ unit tests covering allowed and blocked cases
src/workerd/api/tests/worker-test.js — JS integration tests in a worker context where eval is blocked at request time

ask-bonk · 2026-03-17T15:09:15Z

Now I have a thorough understanding of the change. Let me analyze:

The code change (setup.c++): After the evalAllowed observer notification block, a new check is added: if source->IsUndefined(), return {.codegen_allowed = true}. This allows new Function() (no args) even when eval is blocked.
Security analysis: This is a security-sensitive area — the modifyCodeGenCallback is the gate that prevents untrusted code from using eval() and friends. The key question is: when V8 calls this callback with undefined source, is there truly no code generation happening?

When new Function() is called with no arguments, V8 internally creates a function with an empty body. The source passed to the callback is undefined because there's no user-provided string to evaluate. This is safe — there's no dynamic code string injection possible.

Observer notification: When evalAllowed is true, the observer's onDynamicEval is called. But when source is undefined and eval is NOT allowed, the new early return means onDynamicEval is never called. This seems correct — if eval is allowed, the observer is still notified for non-undefined sources. The undefined source case doesn't represent meaningful dynamic eval to audit.
Backward compatibility: This is a relaxation of a restriction. Previously, new Function() with no args would throw when eval was blocked. Now it succeeds. This is a behavioral change — but it makes things less restrictive, not more. The question is whether this needs a compat flag.

This is a relaxation that fixes what is effectively a bug — new Function() with no arguments creates a no-op function and shouldn't be considered dynamic code generation. The V8 engine itself signals this by passing undefined as source. This seems like a pure bugfix rather than a behavioral change that needs gating, since no reasonable code would depend on new Function() throwing.

Test coverage: Good — both C++ unit tests and JS integration tests cover both new Function() and class extends Function patterns.

The change looks correct and well-tested. The only potential concern is whether this should be gated behind a compat flag, but given it's a bugfix (relaxing an overly-strict restriction where V8 itself indicates no code string is involved), I think it's fine without one.

LGTM

github run

src/workerd/jsg/setup.c++

src/workerd/api/tests/worker-test.js

src/workerd/jsg/setup.c++

codecov-commenter · 2026-03-18T10:52:47Z

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.68%. Comparing base (b5dbc80) to head (d539c4e).
⚠️ Report is 159 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/jsg/setup.c++	75.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6344   +/-   ##
=======================================
  Coverage   70.68%   70.68%           
=======================================
  Files         420      420           
  Lines      113010   113025   +15     
  Branches    18531    18534    +3     
=======================================
+ Hits        79881    79893   +12     
  Misses      22086    22086           
- Partials    11043    11046    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

src/workerd/jsg/setup.c++

When V8's code generation callback receives an undefined source (e.g. `new Function()` with no arguments, or `class Foo extends Function { constructor() { super(); } }`), there is no dynamic code to evaluate. Allow these cases even when eval is otherwise blocked, since they pose no code-injection risk.

Narrow the non-string source check from !IsString() to !IsString() && !IsObject(), so only known-safe primitives (undefined, null, boolean, number, bigint, symbol) are allowed through. Object types are denied by default as a defense-in-depth measure per review feedback.

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

This PR allows new Function() with no arguments and class extends Function { constructor() { super(); } } to succeed even when eval/code-generation is blocked, by adding two guards to modifyCodeGenCallback.

One issue:

[Medium] Defensive IsString check before As<v8::String>() — Line 543 casts source to v8::String guarded only by isCodeLike, without verifying source->IsString(). V8's current implementation guarantees source is a string when isCodeLike is true (since CreateDynamicFunction always synthesizes a string via IncrementalStringBuilder), but this invariant is internal to V8 and not part of the public API contract (ModifyCodeGenerationFromStringsCallback takes Local<Value>). If a future V8 update introduces a path where isCodeLike is true with a non-string source, the As<v8::String>() would crash instead of gracefully denying. Adding source->IsString() to the condition makes this fail-safe: a broken invariant falls through to the deny path rather than crashing the process.

src/workerd/jsg/setup.c++

ask-bonk · 2026-03-19T12:06:09Z

Review posted successfully on PR #6344.

Summary: The PR is well-designed with thorough security analysis and good test coverage. I found one medium-severity issue: the source.As<v8::String>() cast on line 543 should include a defensive source->IsString() check to be fail-safe against future V8 changes. I posted a suggestion comment with the concrete fix.

github run

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>

src/workerd/jsg/setup.c++

ask-bonk · 2026-03-19T14:39:27Z

Now let me understand the full context. The latest commit (d539c4e) narrowed the non-string check to just undefined. But the diff I see still shows source->IsUndefined() — which aligns with the latest discussion where jasnell suggested an explicit accept list and Pete responded with "let's just handle undefined for now" and "done". Let me verify the current code matches the diff.

Let me also check if the previous bonk review's suggestion (adding source->IsString() guard) was already applied — looking at line 541 of the current diff, it shows if (isCodeLike && source->IsString() && which confirms it was incorporated.

Let me now carefully analyze the current state of the code for any remaining issues:

Guard 1 (line 514): source->IsUndefined() — narrowed per kentonv/jasnell feedback. Only allows undefined through. This is correct and safe.
Guard 2 (lines 540-543): isCodeLike && source->IsString() && kj::str(source.As<v8::String>()) == kEmptyFunctionSource — exact string match with IsString() guard (from the previous bonk suggestion). This is correct and safe.
Tests: Both C++ unit tests and JS integration tests cover the relevant cases.
No compat flag: This is a pure relaxation/bugfix. No existing code depends on new Function() throwing when eval is blocked.
Observer notification: The early returns bypass the onDynamicEval observer notification, which was explicitly discussed and agreed on by kentonv and jasnell.

All the previous review feedback has been addressed:

kentonv's concern about short-circuiting before observer → addressed
jasnell's request for eval() test coverage → added
kentonv's concern about the non-string check being too loose → narrowed to IsUndefined() only
jasnell's suggestion to use kj::str → done
jasnell's suggestion for explicit accept list → addressed by narrowing to undefined only
bonk's suggestion for IsString() guard → incorporated

The code is well-designed, all review feedback has been resolved, test coverage is thorough, and there are no remaining actionable issues.

LGTM

github run

petebacondarwin · 2026-03-19T15:39:37Z

Thanks for the thoughtful and effective reviews @kentonv and @jasnell.

petebacondarwin requested review from a team as code owners March 17, 2026 15:07

kentonv reviewed Mar 17, 2026

View reviewed changes