node:crypto: throw (not return) validation errors from createDiffieHellman - #36508
Conversation
…llman Several error paths in constructDiffieHellman() built their errors with createError() and returned them, so crypto.createDiffieHellman(2) evaluated to a TypeError instance rather than throwing one. A constructor returning an object makes that object the result of the new-expression, so the caller saw an Error where a DiffieHellman was expected and no exception was ever raised. Node throws. The bn_g.setWord() failure path also called throwCryptoError() without returning, falling through into DHPointer::New() with a pending exception. Co-authored-by: saklani <50768838+saklani@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 6 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 (2)
Comment |
|
Status: diff is green; ready for a maintainer. Reproduced with CI (#85993, #86070): every failing lane is an unrelated flake that passed on retry (fetch.stream timeout, fastutf8stream-reopen race, install-registry hoisting, napi GC timing on Windows, streams-leak memory threshold, etc.). None touch crypto or the files in this diff. The one non-flaky hit in #85993 was |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap from returning Error objects to throwing them.
What was reviewed:
- All 7
createErrorsites converted tothrowError(...) ; return {}, matching the pattern inJSECDHPrototype.cpp; thebn_g.setWord()fall-through now returns. - Remaining
Bun::ERR::*calls in this constructor already take aThrowScope&and throw correctly. - Test uses an explicit
outcome()helper so a returned Error can't masquerade as a throw; covers bothcreateDiffieHellmanandnew DiffieHellmanentry points.
Extended reasoning...
Overview
Seven error paths in constructDiffieHellman() (src/jsc/bindings/node/crypto/JSDiffieHellmanConstructor.cpp) previously did return JSValue::encode(createError(...)), which returns an Error object from the constructor rather than throwing it — under [[Construct]] semantics that Error becomes the result of the new-expression. Each site is now throwError(globalObject, scope, ErrorCode::..., msg); return {};, and the bn_g.setWord() failure path gains its missing return {} after throwCryptoError. A regression test is added to the existing DiffieHellman describe block in test/js/node/crypto/node-crypto.test.js.
Security risks
None. This is error-path plumbing for argument validation; no cryptographic computation, key handling, or TLS state is touched. The only behavioral change is that invalid arguments now raise instead of returning an Error instance — strictly closer to Node semantics.
Level of scrutiny
Low. The transformation is purely mechanical and identical at every site. I verified throwError(JSGlobalObject*, ThrowScope&, ErrorCode, const WTF::String&) is declared in ErrorCode.h:63 and is already the established idiom in neighbouring ECDH bindings (JSECDHPrototype.cpp). The remaining early returns in this constructor use Bun::ERR::INVALID_ARG_TYPE/CRYPTO_OPERATION_FAILED, which take a ThrowScope& and already throw, so no other sites need conversion.
Other factors
The test correctly avoids toThrow() in favour of an explicit try/catch outcome() helper, so it distinguishes throw-vs-return unambiguously and would fail on the old build with threw: false. It exercises two distinct converted paths (ERR_INVALID_ARG_VALUE from DHPointer::New failure, ERR_INVALID_ARG_TYPE from the non-numeric-generator guard) plus the direct new DiffieHellman constructor route. The PR description reports the vendored Node test-crypto-dh-constructor.js/test-crypto-dh-errors.js still pass. No outstanding reviewer comments; no prior claude review on this PR.
There was a problem hiding this comment.
LGTM — mechanical swap from return JSValue::encode(createError(...)) to throwError(...); return {}; at seven sites, plus the missing return {} after throwCryptoError in the bn_g.setWord() path.
What was reviewed:
- Confirmed
throwError(globalObject, scope, ErrorCode, msg)is the established pattern in the neighbouring ECDH bindings (JSECDHPrototype.cpp). - Traced the two test inputs through the constructor:
createDiffieHellman(2)reaches the!dhguard afterDHPointer::New(2, 2);createDiffieHellman(1024, "abc")reaches the int32-only generator guard before any DH generation, so the test stays fast. - Test uses an explicit
outcome()helper to distinguish throw-vs-return rather thantoThrow(), and covers both the factory andnew DiffieHellman(...).
Extended reasoning...
Overview
The PR touches one C++ binding (JSDiffieHellmanConstructor.cpp) and one test file. Seven error paths in constructDiffieHellman() previously did return JSValue::encode(createError(...)), which — because a [[Construct]] returning an object substitutes that object for the newly-allocated this — made createDiffieHellman(2) evaluate to an Error instance rather than throw one. The fix is a mechanical swap to throwError(globalObject, scope, ErrorCode::…, msg); return {}; at each site, plus an added return {}; after the pre-existing throwCryptoError in the bn_g.setWord() failure branch (which previously fell through into DHPointer::New with a pending exception). The pattern is copied verbatim from the adjacent ECDH bindings, and throwError with this signature is declared in ErrorCode.h.
Security risks
None. This is in node:crypto but the change is purely about how validation failures are surfaced (throw vs. return). No parsing, allocation, key handling, or DH parameter logic is altered; error codes and messages are byte-identical to before.
Level of scrutiny
Low-medium. Native code in a JSC constructor, but the transformation is uniform and the surrounding throwCryptoError(...); return {}; sites in the same function already model exactly this shape. Each of the eight touched sites was audited individually against the diff; there are no other return JSValue::encode(createError(...)) sites left in the function.
Other factors
The new test lives in the existing DiffieHellman describe block in node-crypto.test.js, not a new file. It captures control flow explicitly ({threw, value}) instead of relying on toThrow(), so it fails on the released binary (threw: false) and passes here — satisfying the "prove the test fails for the right reason" bar. The (1024, "abc") case throws before DHPointer::New, so no expensive prime generation runs. The PR description notes vendored test-crypto-dh-constructor.js / test-crypto-dh-errors.js still pass. This adopts the community PR #31708 and is acknowledged to overlap with #33522, which the author says rebases trivially either way.
2 participants
Closes #31708 by @saklani.
Repro
Bun prints
returned: true ERR_INVALID_ARG_VALUE; Node throwsERR_INVALID_ARG_VALUE.Cause
Seven error paths in
constructDiffieHellman()didreturn JSValue::encode(createError(...)), which returns anErrorinstance from the constructor rather than throwing it. A[[Construct]]that returns an object makes that object the result of thenew-expression, socreateDiffieHellman(2)evaluated to aTypeErrorinstead of raising one.The
bn_g.setWord()failure path also calledthrowCryptoError()without returning, falling through intoDHPointer::New()with a pending exception.Fix
Mechanical swap to
throwError(globalObject, scope, ...); return {};at each site, matching the pattern used in the neighbouring ECDH bindings.Verification
New test fails on the released binary (
threw: false), passes on this branch.test/js/node/crypto/node-crypto.test.jsis 203/203, and the vendoredtest-crypto-dh-constructor.js/test-crypto-dh-errors.jsstill pass.The test captures control flow explicitly rather than using
toThrow(), becausetoThrow()accepts a returnedErrorinstance as a throw and cannot distinguish the two cases here.Related
Overlaps with #33522, which fixes the same constructor paths as part of a larger
DH_check()/verifyErrorchange. Whichever lands first, the other rebases trivially.[review] gate passed · iteration 1 · 2 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