cli: implement Node.js hardening flags (--disable-proto, --disallow-code-generation-from-strings, --frozen-intrinsics) - #35766
cli: implement Node.js hardening flags (--disable-proto, --disallow-code-generation-from-strings, --frozen-intrinsics)#35766robobun wants to merge 12 commits into
Conversation
…ode-generation-from-strings, --frozen-intrinsics) These flags were silently accepted and echoed in process.execArgv but had no effect, so an operator porting a Node.js lockdown configuration would get zero protection plus a receipt claiming it was applied. --disallow-code-generation-from-strings: routes to JSC's setEvalEnabled(false, msg) in Zig::GlobalObject::finishCreation, which gates eval() and the Function/AsyncFunction/GeneratorFunction constructors with an EvalError matching V8's text. Applies to worker globals. node:vm contexts keep their own codeGeneration option, matching Node.js. --disable-proto=delete|throw: deletes the Object.prototype.__proto__ accessor, and for =throw re-installs an accessor that throws ERR_PROTO_ACCESS. Applied to the main global, worker globals, and node:vm contexts (matching Node.js). Invalid modes exit 12. Also fixes EventEmitter to use Object.getPrototypeOf(this) instead of this.__proto__ so node:events works under --disable-proto=throw. --frozen-intrinsics: ports Node.js's lib/internal/freeze_intrinsics.js (Apache-2.0, SES/Caja-derived) as internal/freeze_intrinsics, triggered from internal/process/pre_execution before user code. Deep-freezes the ECMA-262 intrinsics plus console/timers, with the override-mistake mitigation so assignment on derived objects still works. Bun's console carries _stdout/_stderr as data properties (vs Node's getters), so those are seeded as already-visited to keep stream prototypes unfrozen like Node.js. --secure-heap / --secure-heap-min: recognised and warns that Bun links BoringSSL, which has no secure heap; the call would have failed silently otherwise.
|
Warning Review limit reached
Next review available in: 34 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
Comment |
|
Status: at f7726a8. Fail-before/fail-after verified locally (17/22 fail on released 1.3.14, 22/22 pass on this build). CI:
Since the original push:
Regression-checked: |
…y, use $getPrototypeOf, drop trailing newline, tighten exit-code assertion, trim comments, consolidate frozen-intrinsics tests
…sableStack, ShadowRealm JSC exposes these (ES Explicit Resource Management and ShadowRealm proposals); Node.js does not, so they are absent from the port source. Same coverage gap as Float16Array.
…ceLimit, cover Iterator - Move the freeze from pre_execution to a dedicated module-eval hook (Bun__freezeIntrinsics) called from reload_entry_point after load_preloads, so polyfill preloads land on intrinsic prototypes before they are frozen. Node.js documents that --require/--import run before the freeze. - Guard Error.stackTraceLimit writes in assertion_error.ts and util/inspect.js with isErrorStackTraceLimitWritable so node:assert throws AssertionError (not TypeError) under --frozen-intrinsics. Added the helper to internal/shared. - Add Iterator to the intrinsics list (its %IteratorPrototype%.constructor is an accessor, so deepFreeze never reached it).
…lper in inspect.js, array-based freeze queue, trim comments
…sole instrumentation under --frozen-intrinsics
… in repl/node-errors
…(frozen Error) instead of a string
There was a problem hiding this comment.
The f7726a8 fix for util.getCallSites() looks right. Beyond the inline nit, this run also checked whether Intl.Locale/Segmenter/DisplayNames/DurationFormat prototypes are missing from the freeze — they're reached transitively via deepFreeze(Intl), so coverage matches Node's list. Given the scope (ZigGlobalObject/NodeVM finishCreation, VM entry-point ordering, a 347-line ported SES freeze) and that these are hardening flags, worth a human sign-off.
Extended reasoning...
All six prior review rounds have been addressed in-PR; the remaining inline comment is a follow-up nit about per-worker execArgv overrides that follows the pre-existing pattern of sibling process-wide flags and fails closed in the security-relevant direction. Deferring rather than approving because the change is large and security-adjacent — it wires new behaviour into Zig::GlobalObject::finishCreation, NodeVMGlobalObject::finishCreation, and three call sites in VirtualMachine::reload_entry_point, and ports Node's SES-derived freeze_intrinsics — which is exactly the kind of surface a maintainer should eyeball once before merge.
What
Implements the Node.js process-hardening flags that Bun was silently accepting and echoing in
process.execArgvwithout honoring. An operator porting a Node.js lockdown configuration was getting zero protection plus a receipt claiming it was applied.Reproduction (before this PR)
Node.js honors all of these.
Changes
--disallow-code-generation-from-strings: routes to JSC'ssetEvalEnabled(false, msg)inZig::GlobalObject::finishCreation, which gateseval()and theFunction/AsyncFunction/GeneratorFunctionconstructors with anEvalErrormatching V8's message text. Inherited by worker globals from the parent process's flags.node:vmcontexts keep their owncodeGenerationoption, matching Node.js.--disable-proto=delete|throw: deletes theObject.prototype.__proto__accessor, and for=throwre-installs an accessor that throwsERR_PROTO_ACCESS. Applied to the main global, inherited by worker globals, and applied tonode:vmcontexts (matching Node.js). Invalid modes exit 12. Also fixesEventEmitterto use$getPrototypeOf(this)instead ofthis.__proto__sonode:eventsworks under--disable-proto=throw.--frozen-intrinsics: ports Node.js'slib/internal/freeze_intrinsics.js(Apache-2.0, SES/Caja-derived) asinternal/freeze_intrinsics, triggered after--require/--importpreloads (Node.js documented ordering, so polyfills land first) and before the entry module. Deep-freezes the ECMA-262 intrinsics plusconsole/timers, with the override-mistake mitigation so assignment on derived objects still works. Extends Node's coverage to JSC-exposedFloat16Array,SuppressedError,DisposableStack,AsyncDisposableStack,ShadowRealm, andIterator. Every builtin write to a now-frozen intrinsic (Error.stackTraceLimitin assert/inspect/pre_execution,Error.prepareStackTraceinutil.getCallSites,console.*in trace_events) is guarded so those APIs do not throw under the flag. Bun'sconsolecarries_stdout/_stderras data properties (vs Node's getters), so those are seeded as already-visited to keep stream prototypes unfrozen like Node.js. Emits the sameExperimentalWarningas Node.js.--secure-heap/--secure-heap-min: recognised and warns that Bun links BoringSSL, which has no secure heap. Previously silent.All three flags are stored process-wide (matching the existing
--no-deprecation/--zero-fill-bufferspattern), so a worker inherits its parent's hardening. A worker that passes these via its ownexecArgvoption from an unhardened parent is not yet honored; that matches the behavior of every sibling flag and fails closed in the security-relevant direction (a hardened parent's workers cannot opt out).Verification
22 new tests in
test/js/node/process/node-hardening-flags.test.tscovering: each flag's primary effect, worker-thread inheritance,node:vmcontext inheritance for--disable-proto, the override-mistake mitigation under--frozen-intrinsics,--requirepreload ordering (including a preload that patchesModule.runMain), builtin-write guards under--frozen-intrinsics, invalid--disable-protomode rejection, and no-flag baselines. All fail with the released binary and all pass with this change.test/js/node/events/,test/js/node/assert/,test/js/node/vm/vm.test.ts, and the vendoredtest-console-with-frozen-intrinsics.jsall still pass.Not in scope
The general "unknown
--flagexits 0" behaviour is left as-is: changing it would be a much broader compatibility break, and the specific hardening fail-open this PR addresses is resolved by implementing the flags rather than rejecting them.