Repository navigation
Conversation
…ndings
Several native entry points handed ERR_INVALID_ARG_TYPE an already
flattened "A, B, or C" string, which the single-string overload always
renders as "must be of type A, B, or C". Node passes a list and groups it
into primitive names ("one of type number or string") and class names
("an instance of Buffer, TypedArray, or DataView").
Pass the same lists Node passes through the span overload (which now
also accepts a dynamic argument name, for setgroups' "groups[i]"), use
INVALID_ARG_INSTANCE for the two class-only lists, and give
subtle.digest() the same webidl-style message importKey() already uses.
Sign#sign() also names the argument "privateKey" like Node does.
Sites: StringDecoder#write, subtle.digest, createDiffieHellman
(sizeOrKey, generator), getCipherInfo, generatePrime/generatePrimeSync
(options.add, options.rem), hkdf/hkdfSync (salt, info), generateKeyPair
"dh" options.prime, Sign#sign, process.emitWarning, process.set*id,
setgroups and initgroups.
The four vendored Node tests that had been edited to expect the old
wording are restored to their upstream text.
|
Warning Review limit reached
Next review available in: 18 minutes 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 (19)
Comment |
|
Status: reproduced and fixed; ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical Node-compat fix that swaps pre-flattened error strings for the existing list-rendering overload at ~13 C++ sites; only error-message wording changes.
What was reviewed:
- Each call site swaps one
ERR::INVALID_ARG_TYPE/INVALID_ARG_INSTANCEhelper for another; error codes, classes, and accepted inputs are unchanged. - The
arg_namewidening fromASCIILiteraltoconst WTF::String&on the span overload stays unambiguous — the second parameter (std::spanvsStringvsJSValue) disambiguates, and C arrays convert to span directly. JSSubtleCrypto.cppdigest:throwErrorreturnsEncodedJSValuelike the helper it replaces, and the priortryClearException()is preserved; the exception-scope-fixture transcript tests the algorithm arg, not data, so unaffected.- Every changed site has a new exact-message test; four vendored Node tests are restored to upstream v26.3.0 text.
Extended reasoning...
Overview
This PR fixes Node.js compatibility for ERR_INVALID_ARG_TYPE error message wording at ~13 native C++ call sites across BunProcess.cpp, JSStringDecoder.cpp, six node/crypto/ files, and JSSubtleCrypto.cpp. Each site previously passed a pre-flattened string like "number or string"_s to the single-string overload (always renders "must be of type X"); now they pass static constexpr ASCIILiteral[] lists to the existing span overload, which buckets primitives vs classes exactly as Node's lib/internal/errors.js does. One signature widening in ErrorCode.{h,cpp} (arg_name: ASCIILiteral → const WTF::String&) lets setgroups pass its computed groups[i] name. Test additions cover every changed site with Node v26.3.0's verbatim messages, and four vendored Node parallel tests are restored to their upstream text.
Security risks
None. The changes are entirely to error-message text on already-rejected inputs. No validation logic, accepted types, error codes, or control flow changes. The static constexpr arrays are compile-time constants with no user input.
Level of scrutiny
Low-to-medium. This is a mechanical pattern application: the span-overload renderer already existed (used by initgroups), and each site is a 1-3 line swap between error helpers with identical signatures. The only non-trivial pieces are (a) the parameter-type widening, which I checked stays unambiguous because the second parameter type (span vs String vs JSValue) disambiguates all overloads and C arrays bind to std::span via the array-reference constructor; and (b) JSSubtleCrypto.cpp's switch from ERR::INVALID_ARG_TYPE to Bun::throwError, both of which throw and return EncodedJSValue, with the preceding tryClearException() unchanged.
Other factors
- Test coverage is thorough: 16 new crypto cases, 8 new process cases, plus string-decoder and web-crypto, all asserting exact Node v26.3.0 messages with
name/code/message. The PR description documents that each expected string was executed against Node before committing. - Restoring vendored tests to upstream text moves toward parity rather than away.
- The
Sign#signargument rename"key"→"privateKey"matches Node'spreparePrivateKey(privateKey, 'privateKey')and is covered by two new tests. - The
CryptoHkdf.cpplist also fixes a pre-existing omission (DataView was missing, no "or") — now matches Node'svalidateByteSource. - No prior human reviews or outstanding comments on the PR.
Problem
ERR_INVALID_ARG_TYPEwith a different message than Node for the same input, for example:new StringDecoder().write(123): BunThe "buf" argument must be of type Buffer, TypedArray, or DataView., Node... must be an instance of Buffer, TypedArray, or DataView.process.setuid({}): BunThe "id" argument must be of type number or string., Node... must be one of type number or string.crypto.createDiffieHellman({}): Bun... must be of type number, string, ArrayBuffer, Buffer, TypedArray, or DataView., Node... must be one of type number or string or an instance of ArrayBuffer, Buffer, TypedArray, or DataView.process.emitWarning(123): Bun... must be of type string or Error., Node... must be of type string or an instance of Error."number or string"_s,"Buffer, TypedArray, or DataView"_s, ...) to the single-string overload ofBun::ERR::INVALID_ARG_TYPE, which always emits"of type " + string(src/jsc/bindings/ErrorCode.cpp,Message::ERR_INVALID_ARG_TYPE). Node'slib/internal/errors.jsreceives a list and groups it into primitive names and class names.Message::ERR_INVALID_ARG_TYPE(..., ArgList, ...), exposed as thestd::span<const ASCIILiteral>overload ofERR::INVALID_ARG_TYPE);process.initgroupswas its only caller.StringDecoder#write,subtle.digest,createDiffieHellman(sizeOrKey,generator),getCipherInfo,generatePrime/generatePrimeSync(options.add,options.rem),hkdf/hkdfSync(salt,info),generateKeyPair("dh")options.prime,Sign#sign,process.emitWarning,process.setuid/seteuid/setgid/setegid,process.setgroups.test-string-decoder,test-process-uid-gid,test-process-euid-egid,test-process-setgroups) had been edited when they were vendored so that they expect Bun's wording.Fix
static constexpr ASCIILiteral xs[] = {...}) through the span overload, so the grouping is computed by the same renderer Node uses instead of being hand-written per site. The two class-only lists (StringDecoder#write, dhoptions.prime) use the existingERR::INVALID_ARG_INSTANCE, the same helpervalidateBufferuses for this exact list.arg_nameparameter is widened fromASCIILiteraltoconst WTF::String&sosetgroupscan pass its computedgroups[i]name;ASCIILiteralcallers convert implicitly and the existing overloads stay unambiguous (no other overload accepts an array or a span as the expected type).BunProcess.cpp: theinitgroupslist becomes a sharedidTypesused by every site that corresponds to Node'svalidateId()(set*id, setgroups, initgroups), so all seven ids render identically.subtle.digest(): Node's message here comes from its webidlBufferSourceconverter, not from the list renderer, so this site uses the literal webidl message thatimportKey()in the same file already emits.Sign#sign(): besides the list, the argument is namedprivateKeyas in Node'ssig.js(preparePrivateKey(privateKey, 'privateKey')).CryptoHkdf.cpp: the old string also omittedDataViewand had no "or"; the list is Node'svalidateByteSourcelist.nodev26.3.0 before being committed), and the error codes, error classes and accepted inputs are unchanged; only the wording differs.bun bd test test/js/node/crypto/node-crypto.test.js(newdescribe, 16 cases: all fail on Bun 1.4.0, pass with this change)bun bd test test/js/node/process/process.test.js(new id and emitWarning tests: 6 of 8 cases fail on 1.4.0, the 2initgroupscases document the already-correct site; the one pre-existing failure in that file,process.env.USERbeing unset in the container, is unrelated)bun bd test test/js/node/string_decoder/string-decoder.test.js,bun bd test test/js/web/crypto/web-crypto.test.ts(one new test each, fail on 1.4.0, pass now)bun bd test/js/node/test/parallel/<file>.jsfor the 4 restored tests plustest-process-initgroups,test-process-emitwarning,test-crypto-dh,test-crypto-dh-errors,test-crypto-hkdf,test-crypto-prime,test-crypto-getcipherinfo,test-crypto-sign-verify,test-crypto-keygen-dh-classic,test-webcrypto-digest,test-process-exception-capture-errors: all exit 0KeyObject.cpp(createPrivateKey, raw key formats): node:crypto: render prepareAsymmetricKey ERR_INVALID_ARG_TYPE lists like Node #38431.perf_hookshistogram.record(): node:perf_hooks: align createHistogram/record/add validation with Node.js #33585 already makes the same change.process.setUncaughtExceptionCaptureCallback("function or null"): already renders exactly like Node, since Node's list is['Function', 'null'].process.kill()signal type check: Node throwsERR_UNKNOWN_SIGNALthere rather than a differently wordedERR_INVALID_ARG_TYPE, so that is a separate fix.Background
ERR_INVALID_ARG_TYPErendering in Node:expectedis an array; each entry that is one of Node's primitive type names (string,number,bigint,Function,Object, ...) goes into a "types" bucket rendered asof type X(orone of type X or Yfor several), each entry matching/^[A-Z][a-zA-Z0-9]*$/goes into an "instances" bucket rendered asan instance of A, B, or C, and the buckets are joined withor. So the order of the list does not matter, but which bucket each entry lands in does.Bun::ERR::INVALID_ARG_TYPEoverloads (src/jsc/bindings/ErrorCode.h): the(name, const String& expected, value)overload is for a single type and printsof type <expected>verbatim; the(name, std::span<const ASCIILiteral>, value)overload is the port of Node's bucketing above;INVALID_ARG_INSTANCE(name, "A, B, or C", value)printsan instance of ...for lists that are classes only.addParameterpicksargumentorpropertydepending on whether the name contains a dot, which is whyoptions.addrenders as a property.subtle.digestis a WebCrypto method; Node validates its arguments with its own webidl layer, whose failures areTypeErrors withcode: 'ERR_INVALID_ARG_TYPE'and aFailed to execute '<method>' on 'SubtleCrypto': <n>th argument ...message, which is why that one site does not use the list renderer.Before/after for every changed site (Node v26.3.0 is the "after" text)
new StringDecoder().write(123)The "buf" argument must be of type Buffer, TypedArray, or DataView. Received type number (123)The "buf" argument must be an instance of Buffer, TypedArray, or DataView. Received type number (123)crypto.subtle.digest("SHA-256", 123)The "data" argument must be of type ArrayBuffer, Buffer, TypedArray, or DataView. Received type number (123)Failed to execute 'digest' on 'SubtleCrypto': 2nd argument is not instance of ArrayBuffer, Buffer, TypedArray, or DataView.createDiffieHellman({})The "sizeOrKey" argument must be of type number, string, ArrayBuffer, Buffer, TypedArray, or DataView. Received an instance of ObjectThe "sizeOrKey" argument must be one of type number or string or an instance of ArrayBuffer, Buffer, TypedArray, or DataView. Received an instance of ObjectcreateDiffieHellman(prime, {})"generator""generator"getCipherInfo({})The "nameOrNid" argument must be of type string or number. Received an instance of ObjectThe "nameOrNid" argument must be one of type string or number. Received an instance of ObjectgeneratePrimeSync(8, { add: {} })(alsorem, also asyncgeneratePrime)The "options.add" property must be of type ArrayBuffer, Buffer, TypedArray, DataView, or bigint. Received an instance of ObjectThe "options.add" property must be of type bigint or an instance of ArrayBuffer, TypedArray, Buffer, or DataView. Received an instance of ObjecthkdfSync("sha256", "key", {}, "info", 16)(alsoinfo, also asynchkdf)The "salt" argument must be of type string, ArrayBuffer, TypedArray, Buffer. Received an instance of ObjectThe "salt" argument must be of type string or an instance of ArrayBuffer, TypedArray, DataView, or Buffer. Received an instance of ObjectgenerateKeyPairSync("dh", { prime: "x" })The "options.prime" property must be of type Buffer, TypedArray, or DataView. Received type string ('x')The "options.prime" property must be an instance of Buffer, TypedArray, or DataView. Received type string ('x')createSign("SHA256").sign(123)The "key" argument must be of type ArrayBuffer, Buffer, TypedArray, DataView, string, KeyObject, or CryptoKey. Received type number (123)The "privateKey" argument must be of type string or an instance of ArrayBuffer, Buffer, TypedArray, DataView, KeyObject, or CryptoKey. Received type number (123)process.emitWarning(123)The "warning" argument must be of type string or Error. Received type number (123)The "warning" argument must be of type string or an instance of Error. Received type number (123)process.setuid({})(alsoseteuid,setgid,setegid)The "id" argument must be of type number or string. Received an instance of ObjectThe "id" argument must be one of type number or string. Received an instance of Objectprocess.setgroups([1, true])The "groups[1]" argument must be of type number or string. Received type boolean (true)The "groups[1]" argument must be one of type number or string. Received type boolean (true)Remaining differences from the probe:
histogram.record("x")(#33585), and, each a separate bug,StringDecoder#end(123)throws a plainExpected Uint8ArrayTypeError;Sign#sign({ key: 123 })andprivateDecrypt(123, ...)saykey/key.keywhere Node saysprivateKey/privateKey.key(the name is not threaded throughKeyObject::prepareAsymmetricKey);process.kill(pid, {})throwsERR_INVALID_ARG_TYPEwhere Node throwsERR_UNKNOWN_SIGNAL; and thekill EINVALerrno message uses Bun's general system-error format.