Conversation
Contributor
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Comment |
Collaborator
Author
|
Updated 11:46 AM PT - Sep 6th, 2026
⏳ @robobun, your commit 35e9cbb is still building in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
Author
|
Status
|
Contributor
There was a problem hiding this comment.
All three points from my earlier review are addressed: SecureContext is now a $toClass-backed function with the if (!(this instanceof SecureContext)) guard (and a test for the no-new call), getCiphers() dedupes via sorted-adjacent comparison using only captured primordials (no Set/Array.from), and the fresh-array test now asserts .not.toBe against a spread copy. I have nothing further to flag — deferring only because this is node:tls, so a human sign-off is still appropriate.
What was reviewed:
getSSLCiphersin NodeTLS.cpp — throw scope,MarkPopErrorOnReturn,DeleteFnPtr<SSL_CTX>RAII,MarkedArgumentBufferoverflow check,RETURN_IF_EXCEPTION/RELEASE_AND_RETURNall in place.kCachedContextsymbol gating — the no-newrecursive call passes onlyoptions, so a user cannot forge the cached path; every internalInternalSecureContextsite was updated.- Test hygiene —
tls.DEFAULT_CIPHERSrestored intry/finally; assertions are non-vacuous.
Extended reasoning...
Overview
Two commits landed since my last review (91f59aa, 0615160). The diff now converts SecureContext from a class to a function-class via $toClass with an explicit instanceof self-guard, replaces the Set/Array.from dedup in getCiphers() with a sort-then-skip-adjacent loop over captured primordials, and tightens the fresh-array test to use both an identity check and a spread copy before mutation. All three of my prior inline comments map directly onto these edits.
Security risks
The change touches node:tls but does not alter handshake, verification, or cipher-selection behavior. getSSLCiphers is a read-only enumeration of a fresh throwaway SSL_CTX; the SecureContext refactor preserves the invariant that user-constructed contexts own their SSL_CTX exclusively (the shared-cache opt-in is now gated by a module-private Symbol, which is strictly tighter than the previous public boolean second argument). No security-relevant defaults changed.
Level of scrutiny
Medium. The code itself is small and mechanical, and the C++ follows the exception-scope/RAII conventions REVIEW.md requires. However src/js/node/tls.ts and src/jsc/bindings/NodeTLS.cpp are security-adjacent files, and the approval guidelines say not to auto-approve changes in crypto/TLS paths regardless of apparent simplicity — a human should confirm the SecureContext unification did not change any internal caller's semantics.
Other factors
Tests were added in the existing files for both behaviors, including the no-new construction path and the DEFAULT_CIPHERS-independence of getCiphers(). The github-actions inline comments (likely lint) were followed by a fixup commit and the current diff is clean. No third-party CHANGES_REQUESTED reviews are outstanding.
… shared empty-object structure
$toClass(fn, name) with no base class built the prototype from the cached
empty-object structure and set mayBePrototype on that structure in place.
Every {} created afterwards inherited the bit and JSON.parse's fast path
asserts !newStructure->mayBePrototype(). node:tls is the first caller
without a base, so loading it aborted debug builds on the next repeated
JSON.parse shape. Use didBecomePrototype(), which transitions only the
prototype object.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…fieHellman, dns.Resolver, $.Shell, HTTPParser and ConnectionsList (#42528) ### Problem - `class X extends C {}; new X()` returns an instance of `C`, not of `X`, for `Bun.Cookie`, `Bun.CookieMap`, `crypto.ECDH`, `crypto.DiffieHellman`, `dns.Resolver`, `$.Shell`, and `HTTPParser` and `ConnectionsList` of the `http_parser` binding. `new X() instanceof X` is `false` and the methods of `X` are missing. Node v26.3.0 returns an `X`. - The native constructors ignore `newTarget` and always use the base structure (`JSCookie.cpp`, `JSCookieMap.cpp`, `JSECDHConstructor.cpp:75`, `JSDiffieHellmanConstructor.cpp:193`, `JSHTTPParserConstructor.cpp:23`, `JSConnectionsListConstructor.cpp:23`). `$.Shell` (`shell.ts:344`) always sets `ShellPrototype.prototype`. - `dns.Resolver` (`dns.ts:694`) returns `new InternalResolver(options)`. So `new dns.Resolver() instanceof dns.Resolver` is `false` too, and a method patched on `dns.Resolver.prototype` never runs. ### Fix - `Bun.Cookie` and `Bun.CookieMap` call `setSubclassStructureIfNeeded`, like the other 22 `JSDOMConstructor` bindings. - `structureForNewTarget` (new, beside `defaultGlobalObject`) holds the `newTarget` block that `crypto.DiffieHellmanGroup` had. `ECDH`, `DiffieHellman`, `DiffieHellmanGroup`, `HTTPParser` and `ConnectionsList` call it. `$.Shell` uses `new.target.prototype`. - `dns.Resolver` is the class itself, like `dns.promises.Resolver`. `dns.Resolver()` without `new` now throws a `TypeError`, as in Node. The compat page drops its note about this class. - Verified: tests in `cookie-map.test.ts`, `ecdh.test.ts`, `node-crypto.test.js`, `node-http-parser.test.ts`, `bunshell-instance.test.ts` and `dns-resolver-class.test.ts`. 1.4.3-canary.1 fails 17 of them. ### Background - `newTarget` is the constructor that `new` was applied to. In `new X()`, `C` runs with `newTarget === X`. The object must get `X.prototype`. - A JSC `Structure` holds the prototype of an object. `InternalFunction::createSubclassStructure(newTarget, base)` returns the base structure with `newTarget.prototype`. - `getFunctionRealm(newTarget)` is the global of `newTarget`. For a class from a `node:vm` context it is not a Bun global, so the code passes it through `defaultGlobalObject`. <details><summary>Notes</summary> Repro: ```js const crypto = require("node:crypto"), dns = require("node:dns"), { HTTPParser, ConnectionsList } = process.binding("http_parser"); const T = [["Bun.Cookie", Bun.Cookie, ["a", "b"]], ["Bun.CookieMap", Bun.CookieMap, ["a=b"]], ["crypto.ECDH", crypto.ECDH, ["prime256v1"]], ["crypto.DiffieHellman", crypto.DiffieHellman, [Buffer.from("17", "hex")]], ["dns.Resolver", dns.Resolver, []], ["$.Shell", Bun.$.Shell, []], ["HTTPParser", HTTPParser, []], ["ConnectionsList", ConnectionsList, []], ["control crypto.DiffieHellmanGroup", crypto.DiffieHellmanGroup, ["modp14"]], ["control dns.promises.Resolver", dns.promises.Resolver, []], ["control URL", URL, ["http://a/"]]]; for (const [n, C, a] of T) { class X extends C { extra() { return 1; } } const r = new X(...a); console.log(n.padEnd(34), "instanceof X:", r instanceof X, "| instanceof C:", r instanceof C, "| extra:", typeof r.extra); } ``` | | 1.4.3-canary.1 | this branch | node v26.3.0 | | --- | --- | --- | --- | | `Bun.Cookie`, `Bun.CookieMap`, `$.Shell` | `false`, `true`, `undefined` | `true`, `true`, `function` | | | `crypto.ECDH`, `crypto.DiffieHellman`, `_http_common.HTTPParser` | `false`, `true`, `undefined` | `true`, `true`, `function` | `true`, `true`, `function` | | `ConnectionsList` | `false`, `true`, `undefined` | `true`, `true`, `function` | not reachable | | `dns.Resolver` | `false`, `false`, `undefined` | `true`, `true`, `function` | `true`, `true`, `function` | | the three controls | `true`, `true`, `function` | `true`, `true`, `function` | `true`, `true`, `function` | `dns.Resolver`: - The wrapper function is from the first `node:dns` implementation (2b1b897). `$toClass` gave it a prototype that inherits from `InternalResolver.prototype`, but the returned object had `InternalResolver.prototype` itself. No object ever had `dns.Resolver.prototype`. - dd-trace wraps `dns.Resolver.prototype.resolve`, `reverse` and the shorthands. On 1.4.3-canary.1 a wrapper installed there is never called on an instance. It is called on this branch and on Node. - Node declares `class Resolver extends ResolverBase {}`, so `dns.Resolver()` without `new` throws there too: `Class constructor Resolver cannot be invoked without 'new'`. No test in the repo called it without `new`. - The test is in a new file because `node-dns.test.js` resolves public hostnames. 29 of its tests fail in a sandbox with no outbound DNS, before and after this change (the same 29). - `docs/runtime/nodejs-compat.mdx` said "the callback-style `Resolver` class cannot be subclassed (`dns.promises.Resolver` can)" since #38441. This PR removes that clause. `structureForNewTarget`: - It takes the lexical global, `newTarget`, and a pointer to the `LazyClassStructure` member of `Zig::GlobalObject`. It returns the base structure when `newTarget` is the constructor, and the subclass structure otherwise. - `DiffieHellmanGroup` calls it at the place where its block was, after the argument checks, so its behaviour does not change. Its `if (!newTarget)` branch is gone. A host `construct` function always has a `newTarget`. `callDiffieHellmanGroup` goes through `JSC::construct`, which sets it to the constructor. - In `ECDH` and `DiffieHellman` the call is at the top of the constructor. Node's `function ECDH(curve)` gets its `this` before `validateString(curve)` runs, so a `prototype` getter on `newTarget` runs before the argument error. `Reflect.construct(ECDH, [{}], proxyWithThrowingPrototypeGetter)` throws the getter's error on Node and on this branch. - The other constructors that have the same block by hand (`Hash`, `Hmac`, `Sign`, `X509Certificate`, `MIMEType`, `MIMEParams`, `Dirent`, `vm.Script`, `DatabaseSync`) already honor `newTarget`. This PR does not move them to the helper. That is a refactor with no change in behaviour. - One difference from Node remains, and `DiffieHellmanGroup`, `Hash`, `Hmac` and `Sign` already have it. For `Reflect.construct(ECDH, args, F)` where `F.prototype` does not inherit from `ECDH.prototype`, Node's `if (!(this instanceof ECDH)) return new ECDH(curve)` returns a plain `ECDH`. Bun returns an object with `F.prototype`. The tests only assert what is the same on Node. Other cases checked by hand on this branch, for all the classes. None crashes. `BUN_JSC_validateExceptionChecks=1` is clean on the subclass, revoked Proxy and throwing getter paths. - `Reflect.construct(C, args, F)` with a plain function, a bound function (no `prototype`, so the base prototype is used), a Proxy, and a function whose `prototype` is not an object. - A revoked Proxy as `newTarget` throws `TypeError: Cannot get function realm from revoked Proxy` (a `TypeError` on Node too). - A subclass declared in a `node:vm` context, and `Reflect.construct` with a `newTarget` from a context. The crypto tests cover the first case. - `ECDH("prime256v1")` and `DiffieHellman(prime)` without `new` still return a base instance. Census: I walked every constructible function reachable from `globalThis`, `Bun`, `node:*` and `bun:*` to depth 3 and constructed a subclass of each with a list of guessed arguments. 772 functions are constructible. 210 returned an object for the subclass. On this branch the ones that are still not an instance of the subclass are: - `string_decoder.StringDecoder`. It returns the subclass constructor itself. It has a separate change. - `Buffer` and `SlowBuffer`. Node also returns a plain `Buffer`. - `diagnostics_channel.Channel`. It overrides `Symbol.hasInstance`. The prototype is correct, and Node prints the same. - `module.Module`, `ShadowRealm` and `WebAssembly.Suspending`. Known, and the last two are in JavaScriptCore. The census has blind spots: - 506 of the 772 did not construct with any guessed argument list. - It skipped modules whose name starts with `_` and everything behind `process.binding`. A review found `HTTPParser` and `ConnectionsList` there, and this PR fixes them. - It did not report a constructor whose `extends` clause throws. `tls.SecureContext`, `Bun.ArrayBufferSink` and `Bun.SQL` have no `prototype` object, so `class X extends C` throws `The value of the superclass's prototype property is not an object or null`, and `new C() instanceof C` throws too. #41696 (SecureContext) and #39936 (the sinks) are open for the first two. `Bun.SQL` is a separate follow-up. - A class from a native addon that uses the V8 API directly is not reachable from JS globals. `FunctionTemplate::functionConstruct` (`src/jsc/bindings/v8/shim/FunctionTemplate.cpp`) takes the prototype from the callee and not from `newTarget`. That is a separate follow-up. `$.Shell` was not in the original report. The census found it. Separate from this PR: `Object.getPrototypeOf(Bun.Cookie)` and `Object.getPrototypeOf(Bun.CookieMap)` are `Object.prototype`, not `Function.prototype` (`prototypeForStructure` in both files). So `Bun.Cookie.bind` is `undefined`, also on a subclass. This PR does not change that. Suites run on the debug build: `test/js/bun/cookie/`, `test/js/node/crypto/node-crypto.test.js`, `ecdh.test.ts`, `crypto-invalid-this.test.ts`, `test/js/node/dns/dns-resolver-class.test.ts`, `node-dns.test.js`, `test/js/node/http/node-http-parser.test.ts`, `test/js/bun/shell/bunshell-instance.test.ts`, `bunshell-default.test.ts`, `env.positionals.test.ts`, and the Node tests `test-crypto-dh*.js` (13 files), `test-crypto-ecdh*.js`, `test-crypto-classes.js`, `test-dns*.js` (25 files), `test-worker-dns-terminate*.js`, `test-http-parser*.js`, `test-http-common.js`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 18 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cookie/cookie-map.test.ts test/js/bun/shell/bunshell-instance.test.ts test/js/node/crypto/ecdh.test.ts test/js/node/crypto/node-crypto.test.js test/js/node/dns/dns-resolver-class.test.ts test/js/node/http/node-http-parser.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/166] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/166] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/166] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/fil ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (138f46d) test/js/bun/cookie/cookie-map.test.ts: (pass) Bun.Cookie and Bun.CookieMap > can create a basic Cookie [0.08ms] (pass) Bun.Cookie and Bun.CookieMap > can create a Cookie with options [0.07ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.toString() formats properly [0.10ms] (pass) Bun.Cookie and Bun.CookieMap > can set Cookie expires as Date [7.65ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() returns correct value [0.15ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() gives Max-Age precedence over Expires [0.11ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse works with all attributes [0.10ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse keeps Expires when Max-Age comes first [0.12ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse takes the last value of a repeated attribute [0.04ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse reads every attribute in any order [0.17ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.serialize creates cookie string [0.09ms] (pass) Bun.Cookie and Bun.CookieMap > can create an empty CookieMap [0.06ms] (pass) Bun.Cookie and Bun.CookieMap > can create Cooki ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cookie/cookie-map.test.ts test/js/bun/shell/bunshell-instance.test.ts test/js/node/crypto/ecdh.test.ts test/js/node/crypto/node-crypto.test.js test/js/node/dns/dns-resolver-class.test.ts test/js/node/http/node-http-parser.test.ts bun test v1.4.3 (6a92015) test/js/bun/cookie/cookie-map.test.ts: (pass) Bun.Cookie and Bun.CookieMap > can create a basic Cookie [4.56ms] (pass) Bun.Cookie and Bun.CookieMap > can create a Cookie with options [5.05ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.toString() formats properly [5.87ms] (pass) Bun.Cookie and Bun.CookieMap > can set Cookie expires as Date [160.37ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() returns correct value [4.67ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() gives Max-Age precedence over Expires [6.09ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse works with all attributes [6.01ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse keeps Expires when Max-Age comes first [8.01ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 726ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/127] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/127] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/127] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts - Glob (5 fields) Found 1 classes from /workspace/bun/src/runtime/api ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/runtime/nodejs-compat.mdx | 2 +- src/js/builtins/shell.ts | 3 +- src/js/node/dns.ts | 9 +--- src/jsc/bindings/ZigGlobalObject.cpp | 14 +++++ src/jsc/bindings/ZigGlobalObject.h | 3 ++ .../node/crypto/JSDiffieHellmanConstructor.cpp | 7 ++- .../crypto/JSDiffieHellmanGroupConstructor.cpp | 18 +------ src/jsc/bindings/node/crypto/JSECDHConstructor.cpp | 6 +-- .../node/http/JSConnectionsListConstructor.cpp | 3 +- .../bindings/node/http/JSHTTPParserConstructor.cpp | 3 +- src/jsc/bindings/webcore/JSCookie.cpp | 23 ++++---- src/jsc/bindings/webcore/JSCookieMap.cpp | 5 +- test/js/bun/cookie/cookie-map.test.ts | 61 ++++++++++++++++++++++ test/js/bun/shell/bunshell-instance.test.ts | 15 ++++++ test/js/node/crypto/ecdh.test.ts | 33 ++++++++++++ test/js/node/crypto/node-crypto.test.js | 34 ++++++++++++ test/js/node/dns/dns-resolver-class.test.ts | 34 ++++++++++++ test/js/node/http/node-http-parser.test.ts | 27 ++++++++++ 18 files changed, 253 insertions(+), 47 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/runtime/nodejs-compat.mdx 1 1 29 src/js/builtins/shell.ts 1 1 29 src/js/node/dns.ts 2 0 29 src/jsc/bindings/ZigGlobalObject.cpp 0 0 29 src/jsc/bindings/ZigGlobalObject.h 3 2 29 …jsc/bindings/node/crypto/JSDiffieHellmanConstructor.cpp 0 0 29 …indings/node/crypto/JSDiffieHellmanGroupConstructor.cpp 0 0 29 src/jsc/bindings/node/crypto/JSECDHConstructor.cpp 1 1 29 …jsc/bindings/node/http/JSConnectionsListConstructor.cpp 0 0 29 src/jsc/bindings/node/http/JSHTTPParserConstructor.cpp 0 0 29 src/jsc/bindings/webcore/JSCookie.cpp 1 0 29 src/jsc/bindings/webcore/JSCookieMap.cpp 1 1 29 test/js/bun/cookie/cookie-map.test.ts 1 2 7 test/js/bun/shell/bunshell-instance.test.ts 1 2 6 test/js/node/crypto/ecdh.test.ts 0 0 9 test/js/node/crypto/node-crypto.test.js 0 0 9 (+ 2 more files) ``` </details> <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…fieHellman, dns.Resolver, $.Shell, HTTPParser and ConnectionsList (oven-sh#42528) ### Problem - `class X extends C {}; new X()` returns an instance of `C`, not of `X`, for `Bun.Cookie`, `Bun.CookieMap`, `crypto.ECDH`, `crypto.DiffieHellman`, `dns.Resolver`, `$.Shell`, and `HTTPParser` and `ConnectionsList` of the `http_parser` binding. `new X() instanceof X` is `false` and the methods of `X` are missing. Node v26.3.0 returns an `X`. - The native constructors ignore `newTarget` and always use the base structure (`JSCookie.cpp`, `JSCookieMap.cpp`, `JSECDHConstructor.cpp:75`, `JSDiffieHellmanConstructor.cpp:193`, `JSHTTPParserConstructor.cpp:23`, `JSConnectionsListConstructor.cpp:23`). `$.Shell` (`shell.ts:344`) always sets `ShellPrototype.prototype`. - `dns.Resolver` (`dns.ts:694`) returns `new InternalResolver(options)`. So `new dns.Resolver() instanceof dns.Resolver` is `false` too, and a method patched on `dns.Resolver.prototype` never runs. ### Fix - `Bun.Cookie` and `Bun.CookieMap` call `setSubclassStructureIfNeeded`, like the other 22 `JSDOMConstructor` bindings. - `structureForNewTarget` (new, beside `defaultGlobalObject`) holds the `newTarget` block that `crypto.DiffieHellmanGroup` had. `ECDH`, `DiffieHellman`, `DiffieHellmanGroup`, `HTTPParser` and `ConnectionsList` call it. `$.Shell` uses `new.target.prototype`. - `dns.Resolver` is the class itself, like `dns.promises.Resolver`. `dns.Resolver()` without `new` now throws a `TypeError`, as in Node. The compat page drops its note about this class. - Verified: tests in `cookie-map.test.ts`, `ecdh.test.ts`, `node-crypto.test.js`, `node-http-parser.test.ts`, `bunshell-instance.test.ts` and `dns-resolver-class.test.ts`. 1.4.3-canary.1 fails 17 of them. ### Background - `newTarget` is the constructor that `new` was applied to. In `new X()`, `C` runs with `newTarget === X`. The object must get `X.prototype`. - A JSC `Structure` holds the prototype of an object. `InternalFunction::createSubclassStructure(newTarget, base)` returns the base structure with `newTarget.prototype`. - `getFunctionRealm(newTarget)` is the global of `newTarget`. For a class from a `node:vm` context it is not a Bun global, so the code passes it through `defaultGlobalObject`. <details><summary>Notes</summary> Repro: ```js const crypto = require("node:crypto"), dns = require("node:dns"), { HTTPParser, ConnectionsList } = process.binding("http_parser"); const T = [["Bun.Cookie", Bun.Cookie, ["a", "b"]], ["Bun.CookieMap", Bun.CookieMap, ["a=b"]], ["crypto.ECDH", crypto.ECDH, ["prime256v1"]], ["crypto.DiffieHellman", crypto.DiffieHellman, [Buffer.from("17", "hex")]], ["dns.Resolver", dns.Resolver, []], ["$.Shell", Bun.$.Shell, []], ["HTTPParser", HTTPParser, []], ["ConnectionsList", ConnectionsList, []], ["control crypto.DiffieHellmanGroup", crypto.DiffieHellmanGroup, ["modp14"]], ["control dns.promises.Resolver", dns.promises.Resolver, []], ["control URL", URL, ["http://a/"]]]; for (const [n, C, a] of T) { class X extends C { extra() { return 1; } } const r = new X(...a); console.log(n.padEnd(34), "instanceof X:", r instanceof X, "| instanceof C:", r instanceof C, "| extra:", typeof r.extra); } ``` | | 1.4.3-canary.1 | this branch | node v26.3.0 | | --- | --- | --- | --- | | `Bun.Cookie`, `Bun.CookieMap`, `$.Shell` | `false`, `true`, `undefined` | `true`, `true`, `function` | | | `crypto.ECDH`, `crypto.DiffieHellman`, `_http_common.HTTPParser` | `false`, `true`, `undefined` | `true`, `true`, `function` | `true`, `true`, `function` | | `ConnectionsList` | `false`, `true`, `undefined` | `true`, `true`, `function` | not reachable | | `dns.Resolver` | `false`, `false`, `undefined` | `true`, `true`, `function` | `true`, `true`, `function` | | the three controls | `true`, `true`, `function` | `true`, `true`, `function` | `true`, `true`, `function` | `dns.Resolver`: - The wrapper function is from the first `node:dns` implementation (2b1b897). `$toClass` gave it a prototype that inherits from `InternalResolver.prototype`, but the returned object had `InternalResolver.prototype` itself. No object ever had `dns.Resolver.prototype`. - dd-trace wraps `dns.Resolver.prototype.resolve`, `reverse` and the shorthands. On 1.4.3-canary.1 a wrapper installed there is never called on an instance. It is called on this branch and on Node. - Node declares `class Resolver extends ResolverBase {}`, so `dns.Resolver()` without `new` throws there too: `Class constructor Resolver cannot be invoked without 'new'`. No test in the repo called it without `new`. - The test is in a new file because `node-dns.test.js` resolves public hostnames. 29 of its tests fail in a sandbox with no outbound DNS, before and after this change (the same 29). - `docs/runtime/nodejs-compat.mdx` said "the callback-style `Resolver` class cannot be subclassed (`dns.promises.Resolver` can)" since oven-sh#38441. This PR removes that clause. `structureForNewTarget`: - It takes the lexical global, `newTarget`, and a pointer to the `LazyClassStructure` member of `Zig::GlobalObject`. It returns the base structure when `newTarget` is the constructor, and the subclass structure otherwise. - `DiffieHellmanGroup` calls it at the place where its block was, after the argument checks, so its behaviour does not change. Its `if (!newTarget)` branch is gone. A host `construct` function always has a `newTarget`. `callDiffieHellmanGroup` goes through `JSC::construct`, which sets it to the constructor. - In `ECDH` and `DiffieHellman` the call is at the top of the constructor. Node's `function ECDH(curve)` gets its `this` before `validateString(curve)` runs, so a `prototype` getter on `newTarget` runs before the argument error. `Reflect.construct(ECDH, [{}], proxyWithThrowingPrototypeGetter)` throws the getter's error on Node and on this branch. - The other constructors that have the same block by hand (`Hash`, `Hmac`, `Sign`, `X509Certificate`, `MIMEType`, `MIMEParams`, `Dirent`, `vm.Script`, `DatabaseSync`) already honor `newTarget`. This PR does not move them to the helper. That is a refactor with no change in behaviour. - One difference from Node remains, and `DiffieHellmanGroup`, `Hash`, `Hmac` and `Sign` already have it. For `Reflect.construct(ECDH, args, F)` where `F.prototype` does not inherit from `ECDH.prototype`, Node's `if (!(this instanceof ECDH)) return new ECDH(curve)` returns a plain `ECDH`. Bun returns an object with `F.prototype`. The tests only assert what is the same on Node. Other cases checked by hand on this branch, for all the classes. None crashes. `BUN_JSC_validateExceptionChecks=1` is clean on the subclass, revoked Proxy and throwing getter paths. - `Reflect.construct(C, args, F)` with a plain function, a bound function (no `prototype`, so the base prototype is used), a Proxy, and a function whose `prototype` is not an object. - A revoked Proxy as `newTarget` throws `TypeError: Cannot get function realm from revoked Proxy` (a `TypeError` on Node too). - A subclass declared in a `node:vm` context, and `Reflect.construct` with a `newTarget` from a context. The crypto tests cover the first case. - `ECDH("prime256v1")` and `DiffieHellman(prime)` without `new` still return a base instance. Census: I walked every constructible function reachable from `globalThis`, `Bun`, `node:*` and `bun:*` to depth 3 and constructed a subclass of each with a list of guessed arguments. 772 functions are constructible. 210 returned an object for the subclass. On this branch the ones that are still not an instance of the subclass are: - `string_decoder.StringDecoder`. It returns the subclass constructor itself. It has a separate change. - `Buffer` and `SlowBuffer`. Node also returns a plain `Buffer`. - `diagnostics_channel.Channel`. It overrides `Symbol.hasInstance`. The prototype is correct, and Node prints the same. - `module.Module`, `ShadowRealm` and `WebAssembly.Suspending`. Known, and the last two are in JavaScriptCore. The census has blind spots: - 506 of the 772 did not construct with any guessed argument list. - It skipped modules whose name starts with `_` and everything behind `process.binding`. A review found `HTTPParser` and `ConnectionsList` there, and this PR fixes them. - It did not report a constructor whose `extends` clause throws. `tls.SecureContext`, `Bun.ArrayBufferSink` and `Bun.SQL` have no `prototype` object, so `class X extends C` throws `The value of the superclass's prototype property is not an object or null`, and `new C() instanceof C` throws too. oven-sh#41696 (SecureContext) and oven-sh#39936 (the sinks) are open for the first two. `Bun.SQL` is a separate follow-up. - A class from a native addon that uses the V8 API directly is not reachable from JS globals. `FunctionTemplate::functionConstruct` (`src/jsc/bindings/v8/shim/FunctionTemplate.cpp`) takes the prototype from the callee and not from `newTarget`. That is a separate follow-up. `$.Shell` was not in the original report. The census found it. Separate from this PR: `Object.getPrototypeOf(Bun.Cookie)` and `Object.getPrototypeOf(Bun.CookieMap)` are `Object.prototype`, not `Function.prototype` (`prototypeForStructure` in both files). So `Bun.Cookie.bind` is `undefined`, also on a subclass. This PR does not change that. Suites run on the debug build: `test/js/bun/cookie/`, `test/js/node/crypto/node-crypto.test.js`, `ecdh.test.ts`, `crypto-invalid-this.test.ts`, `test/js/node/dns/dns-resolver-class.test.ts`, `node-dns.test.js`, `test/js/node/http/node-http-parser.test.ts`, `test/js/bun/shell/bunshell-instance.test.ts`, `bunshell-default.test.ts`, `env.positionals.test.ts`, and the Node tests `test-crypto-dh*.js` (13 files), `test-crypto-ecdh*.js`, `test-crypto-classes.js`, `test-dns*.js` (25 files), `test-worker-dns-terminate*.js`, `test-http-parser*.js`, `test-http-common.js`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 18 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cookie/cookie-map.test.ts test/js/bun/shell/bunshell-instance.test.ts test/js/node/crypto/ecdh.test.ts test/js/node/crypto/node-crypto.test.js test/js/node/dns/dns-resolver-class.test.ts test/js/node/http/node-http-parser.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/166] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/166] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/166] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/fil ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (138f46d) test/js/bun/cookie/cookie-map.test.ts: (pass) Bun.Cookie and Bun.CookieMap > can create a basic Cookie [0.08ms] (pass) Bun.Cookie and Bun.CookieMap > can create a Cookie with options [0.07ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.toString() formats properly [0.10ms] (pass) Bun.Cookie and Bun.CookieMap > can set Cookie expires as Date [7.65ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() returns correct value [0.15ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() gives Max-Age precedence over Expires [0.11ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse works with all attributes [0.10ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse keeps Expires when Max-Age comes first [0.12ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse takes the last value of a repeated attribute [0.04ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse reads every attribute in any order [0.17ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.serialize creates cookie string [0.09ms] (pass) Bun.Cookie and Bun.CookieMap > can create an empty CookieMap [0.06ms] (pass) Bun.Cookie and Bun.CookieMap > can create Cooki ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/cookie/cookie-map.test.ts test/js/bun/shell/bunshell-instance.test.ts test/js/node/crypto/ecdh.test.ts test/js/node/crypto/node-crypto.test.js test/js/node/dns/dns-resolver-class.test.ts test/js/node/http/node-http-parser.test.ts bun test v1.4.3 (6a92015) test/js/bun/cookie/cookie-map.test.ts: (pass) Bun.Cookie and Bun.CookieMap > can create a basic Cookie [4.56ms] (pass) Bun.Cookie and Bun.CookieMap > can create a Cookie with options [5.05ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.toString() formats properly [5.87ms] (pass) Bun.Cookie and Bun.CookieMap > can set Cookie expires as Date [160.37ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() returns correct value [4.67ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.isExpired() gives Max-Age precedence over Expires [6.09ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse works with all attributes [6.01ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse keeps Expires when Max-Age comes first [8.01ms] (pass) Bun.Cookie and Bun.CookieMap > Cookie.parse ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 726ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/127] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/127] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/127] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts - Glob (5 fields) Found 1 classes from /workspace/bun/src/runtime/api ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/runtime/nodejs-compat.mdx | 2 +- src/js/builtins/shell.ts | 3 +- src/js/node/dns.ts | 9 +--- src/jsc/bindings/ZigGlobalObject.cpp | 14 +++++ src/jsc/bindings/ZigGlobalObject.h | 3 ++ .../node/crypto/JSDiffieHellmanConstructor.cpp | 7 ++- .../crypto/JSDiffieHellmanGroupConstructor.cpp | 18 +------ src/jsc/bindings/node/crypto/JSECDHConstructor.cpp | 6 +-- .../node/http/JSConnectionsListConstructor.cpp | 3 +- .../bindings/node/http/JSHTTPParserConstructor.cpp | 3 +- src/jsc/bindings/webcore/JSCookie.cpp | 23 ++++---- src/jsc/bindings/webcore/JSCookieMap.cpp | 5 +- test/js/bun/cookie/cookie-map.test.ts | 61 ++++++++++++++++++++++ test/js/bun/shell/bunshell-instance.test.ts | 15 ++++++ test/js/node/crypto/ecdh.test.ts | 33 ++++++++++++ test/js/node/crypto/node-crypto.test.js | 34 ++++++++++++ test/js/node/dns/dns-resolver-class.test.ts | 34 ++++++++++++ test/js/node/http/node-http-parser.test.ts | 27 ++++++++++ 18 files changed, 253 insertions(+), 47 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/runtime/nodejs-compat.mdx 1 1 29 src/js/builtins/shell.ts 1 1 29 src/js/node/dns.ts 2 0 29 src/jsc/bindings/ZigGlobalObject.cpp 0 0 29 src/jsc/bindings/ZigGlobalObject.h 3 2 29 …jsc/bindings/node/crypto/JSDiffieHellmanConstructor.cpp 0 0 29 …indings/node/crypto/JSDiffieHellmanGroupConstructor.cpp 0 0 29 src/jsc/bindings/node/crypto/JSECDHConstructor.cpp 1 1 29 …jsc/bindings/node/http/JSConnectionsListConstructor.cpp 0 0 29 src/jsc/bindings/node/http/JSHTTPParserConstructor.cpp 0 0 29 src/jsc/bindings/webcore/JSCookie.cpp 1 0 29 src/jsc/bindings/webcore/JSCookieMap.cpp 1 1 29 test/js/bun/cookie/cookie-map.test.ts 1 2 7 test/js/bun/shell/bunshell-instance.test.ts 1 2 6 test/js/node/crypto/ecdh.test.ts 0 0 9 test/js/node/crypto/node-crypto.test.js 0 0 9 (+ 2 more files) ``` </details> <!-- robobun:evidence:end -->
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
tls.getCiphers()returns the tokens of theDEFAULT_CIPHERSpolicy string split on:. Node documents it as the supported cipher names, lower-cased and sorted. On bun it returns 19 entries such as!MD5and!aNULL, it changes whentls.DEFAULT_CIPHERSis assigned, andtls.getCiphers().includes("ecdhe-rsa-aes128-gcm-sha256")is false.tls.SecureContextis a plain builtin function withprototype === undefined.ctx instanceof tls.SecureContextthrowsTypeError: instanceof called on an object with an invalid prototype property, so option-normalising code that branches on it throws instead.SecureContexta prototype with$toClass(fn, name)(no base class) exposed a bug injsFunctionToClass(src/jsc/bindings/ZigGlobalObject.cpp): it setmayBePrototypein place on the shared cached empty-object structure. Every{}created afterwards inherited the bit and debug builds hitASSERTION FAILED: !newStructure->mayBePrototype()inLiteralParser.cppon the next repeatedJSON.parseshape.Fix
getSSLCiphersinsrc/jsc/bindings/NodeTLS.cpp: a freshSSL_CTX,SSL_CTX_get_ciphers, plus the three TLS 1.3 suites that BoringSSL keeps out of the cipher list, as Node does.tls.getCiphers()lower-cases, de-duplicates, sorts and caches that list and returns a copy per call.SecureContextconstructor is now the export. Like Node it returns an instance when called withoutnew. The "share the memoised SSL_CTX" flag became a module-private symbol, so a user-constructed context always owns its handle.jsFunctionToClasscallsprototype->didBecomePrototype(vm), which transitions only the prototype object, instead of flagging its shared structure. All existing$toClasscallers pass a base class, where the polluted structure was the per-prototype cache entry and nothing asserted, which is why this stayed hidden.test/js/node/tls/node-tls-internals.test.tsandtest/js/node/tls/node-tls-create-secure-context-args.test.ts(new tests fail on 1.4.3, pass here, including a subprocess that reproduced the assertion). Alsotest/js/node/tls/,test/js/bun/net/socket.test.ts,fetch.tls.test.ts,express.json.test.ts, net/readline/tty suites, and the vendoredtest-tls-getcipher,test-tls-set-ciphers*,test-tls-secure-context-usage-order,test-tls-sni-*scripts.Background
SSL_CTX_get_ciphersreturns the TLS 1.2 and older suites a context can negotiate. BoringSSL has a fixed TLS 1.3 set (TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384,TLS_CHACHA20_POLY1305_SHA256) that the cipher-list API neither configures nor reports.prototypeproperty from JSC, so$toClass(fn, name, base?)installsprototype,constructorandnameby hand.Structurecarries amayBePrototypebit. The supported way to set it isJSObject::didBecomePrototype, a structure transition on that one object.constructEmptyObject(globalObject)hands out the one cached structure that every empty object starts from, so mutating it in place changes all of them.Notes
LiteralParser.cpp(1597)assertion in the test runner process. Local repro:require("node:tls"); const o = {}; o.zz1 = 1; for (let i = 0; i < 3; i++) JSON.parse('{"zz1":1}')aborts a debug build before theZigGlobalObject.cppchange and prints after it. The first parse creates the transition, the second follows it and asserts.DES-CBC3-SHAis absent because BoringSSL'sALLexcludes 3DES unless named. Node's list also has CCM suites that BoringSSL does not implement.class, which madetls.SecureContext(opts)withoutnewthrow. Node's constructor has aninstanceofguard and returns an instance. The function form keeps that and the test covers it.getCiphers()de-duplicates by sorting in place and skipping adjacent equal names with captured primordials, so a replacedArray.fromorSet.prototype.addcannot poison the cached list.test/js/node/tls/node-tls-server.test.ts(SNICallback bind hostname, destroySoon) fail in this container on main as well.socket.test.ts"should not call drain before handshake" needs DNS for www.example.com and fails here for that reason only.[human-review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file