node:crypto: import OKP JWKs like Node 26, in one raw-key arm shared with AKP by robobun · Pull Request #32910 · oven-sh/bun

robobun · 2026-06-27T21:35:56Z

Problem

An OKP JWK with the d of key A and the x of key B is two keys. sign uses A, and verify accepts a signature by B. Node 26 throws ERR_CRYPTO_INVALID_JWK ("Invalid JWK OKP key") at every entry point.
The cause is case Kty::Okp of KeyObject::getKeyObjectHandleFromJwk (src/jsc/bindings/node/crypto/KeyObject.cpp). The caller's mode picks which member is the key, and nothing compares the other.

Fix

The OKP arm shares the body of the AKP arm, as in Node's ImportJWKEdKey. A string d makes a private key at every entry point, and x must be its public key.
The arm reads crv, x and d once each, then validates them with one error.
Verified: test/js/node/crypto/crypto.key-objects.test.ts (342 of 344 new tests fail on main) and crypto-pqc.test.ts.

Background

OKP is the JWK type of Ed25519 and X25519. x is the public key and a function of the private scalar d.
AKP is the JWK type of ML-DSA and ML-KEM. Its arm (node:crypto: ML-DSA and ML-KEM key support (+7 tests) #34549) already has this shape.
Considered: a compare on private modes only leaves verify open. A d-keyed compare in the old arm keeps three error shapes at the same cost.

Downsides

Breaking, as on Node 26: a JWK with a placeholder or stale x or d now throws. @libp2p/crypto below 5.1.18 passes x: '' (49.6% of its downloads). A maintainer must accept this.
createPublicKey and verify with a JWK that carries d now derive the key: 3,557 to 151,567 instructions. A public-only JWK costs 294 more.
Still open: the EC, RSA and WebCrypto JWK imports.

Notes

Decisions for a maintainer.

The break. createPrivateKey({ format: "jwk", key: { kty: "OKP", crv: "Ed25519", x: "", d } }) works on main and throws with this PR, as it does on Node v26.3.0. @libp2p/crypto 4.1.9, 5.1.7, 5.1.12, 5.1.13 and 5.1.17 make that call in derivePublicKey. 5.1.18 no longer passes x: ''. Versions below 5.1.18 had 93,959 of the 189,619 npm downloads of the last week. Node has no exception for a placeholder x, so the only alternative is a Bun-only one: treat an empty x next to a string d as absent. That keeps old libp2p working and differs from Node for that one shape. This PR does not do that.
The AKP arm. The shared case Kty::Akp: case Kty::Okp: body also changes AKP. A public-only AKP JWK with a malformed pub, given to a private-key consumer, now reports "Invalid JWK AKP key" and no longer "JWK does not contain private key material". That is Node's answer (36 of 1,352 probed AKP cells move, all to Node v26.3.0's result, and no other AKP cell moves). If the AKP arm must stay untouched, a cloned OKP arm gives the same OKP behaviour at about 1 KB more in this minsize function.

Repro. Node v26.3.0 prints ERR_CRYPTO_INVALID_JWK four times. main prints A, B, false, true. This PR prints ERR_CRYPTO_INVALID_JWK four times.

import crypto from "node:crypto";
const A = crypto.generateKeyPairSync("ed25519").privateKey.export({ format: "jwk" });
const B = crypto.generateKeyPairSync("ed25519").privateKey.export({ format: "jwk" });
const jwk = { ...A, x: B.x }, key = { key: jwk, format: "jwk" }, m = Buffer.from("m");
const t = f => { try { return f(); } catch (e) { return e.code; } };
console.log("private key, x:", t(() => crypto.createPrivateKey(key).export({ format: "jwk" }).x === A.x ? "A" : "B"));
console.log("public key,  x:", t(() => crypto.createPublicKey(key).export({ format: "jwk" }).x === A.x ? "A" : "B"));
console.log("verify(jwk, sign(jwk)):", t(() => crypto.verify(null, m, key, crypto.sign(null, m, key))));
console.log("verify(jwk, a signature by B):", t(() => crypto.verify(null, m, key, crypto.sign(null, m, { key: B, format: "jwk" }))));

What was run, and on which commit. Every test run and measurement below was made on commit cd53fd7. The head differs from it in two ways: 7 comment lines in KeyObject.cpp (no other source line), and the removal of three changes under test/js/node/test/. I have not built the head itself. CI is the check for the head.

Node reference (v26.3.0).

ImportJWKEdKey: https://github.com/nodejs/node/blob/v26.3.0/src/crypto/crypto_ec.cc#L597-L676 (added by crypto: unify asymmetric key import through KeyObjectHandle::Init nodejs/node#62499). It takes no mode. A string d decides the type.
The caller applies the mode afterwards: https://github.com/nodejs/node/blob/v26.3.0/src/crypto/crypto_keys.cc#L1145-L1159
Node v26.10.0 imports OKP and AKP through one function and tests both with one table (test/parallel/test-crypto-jwk-raw-validation.js). Run as a script with that version's test/common/crypto.js, that file exits 0 on a build of this change and 1 on main. It is not added to the tree: see Tests.

Measurements. Release builds of main bc7a813 and of cd53fd7 on the same commit, linux-x64. perf, valgrind and bloaty are not available in the build container, so instruction counts are gdb single-step counts of one import call with the JIT off.

getKeyObjectHandleFromJwk: 6,800 bytes (main bc7a813: 7,448 bytes); release text: 768 bytes less than main (nm -S, size).
OKP JWK import per call (gdb hit counts, Ed25519 and X25519 alike): public-only JWK reads 3 -> 4, mallocs 6 -> 6; private JWK at a public entry point scalar mults 0 -> 1, decodes 1 -> 2, mallocs 6 -> 10; private JWK at a private entry point decodes 1 -> 2, mallocs 6 -> 10.
Instructions per OKP import (Ed25519; X25519 in brackets): public-only JWK 3,557 -> 3,851 (3,657 -> 3,956); private JWK at a public entry point 3,557 -> 151,567 (3,657 -> 148,075); private JWK at a private entry point 149,899 -> 151,567 (146,402 -> 148,075).
Uint8Array cells per private-JWK import: 1 -> 2; per public-only import: 1 -> 1.
AKP JWK import per call (ML-DSA-44): reads 4 -> 4, decodes 1 -> 1 (public-only) and 2 -> 2 (private), mallocs 9 -> 9 and 14 -> 14. Unchanged.
createPublicKey(private Ed25519 JWK): 1.49 us -> 18.38 us (median of 12 interleaved runs of 200,000 imports, min-max 0.91 to 5.48 and 15.87 to 23.13; Node 26.3.0 69.00 us). X25519: 0.97 -> 16.03 us. createPublicKey(public-only Ed25519 JWK): 0.88 -> 0.93 us (min-max 0.62 to 0.98 and 0.62 to 1.05), inside the noise.
Forged OKP JWK rejected at 12 of 12 call forms (main 0, Node 12); upstream OKP assertions 4 of 4 (main 0); AKP cells changed 36, all equal to Node 26.3.0.

Behaviour against Node v26.3.0. The 12 call forms are createPrivateKey, createPublicKey, sign, sign with a callback, verify, verify with a callback, createSign().sign, createVerify().verify, publicEncrypt, publicDecrypt, privateEncrypt and privateDecrypt.

A grid of 52 JWK shapes, 13 call forms and 2 curves has 1,352 OKP cells. main differs from Node on 1,292, cd53fd7 on 168. No cell that equals Node on main changes.
The 168 cells that still differ are of three kinds. (1) 112 cells: the operation error for a valid key that the operation does not support. BoringSSL and OpenSSL word it differently. (2) 30 cells: a public-only JWK at sign, createSign().sign, privateEncrypt or privateDecrypt. The import throws "JWK does not contain private key material". Node fails later, in the operation: sign with an Ed25519 key gives ERR_OSSL_NOT_A_PRIVATE_KEY on v26.3.0. The AKP arm already did this. (3) 26 cells: a crv with a trailing NUL and more characters. Node compares C strings and accepts it. main and this PR reject it.
Each call form reads kty, crv, x, d once each, as Node does (97 of 97 read-log cells equal Node, main 24).
A public key object made from a private JWK behaves as the one made from a private PEM or a private key object: no d in any export, and sign refuses it (25 probe lines, equal to Node and to main).

Not fixed here.

case Kty::Ec: d of one key with x, y of another. node:crypto: validate EC JWK private scalar matches public point in createPrivateKey #33752 is open for the private modes.
case Kty::Ec: the decode of d has no exception check before its result is used. That line is the whole of node:crypto: fix null deref when worker.terminate() lands during an EC JWK private key import #37443, which is open.
case Kty::Rsa: another key's n. node:crypto: validate RSA private key material on JWK import #33914 was closed by a stale-PR cleanup, with no judgment on the fix.
WebCrypto importKey("jwk") for Ed25519 and X25519. webcrypto: validate the OKP JWK x parameter against d on import #32827 was closed by the same cleanup.
A missing or unknown kty still gives ERR_INVALID_ARG_TYPE or ERR_INVALID_ARG_VALUE. Node gives ERR_CRYPTO_INVALID_JWK.
Ed448 and X448 JWKs fail at key construction: BoringSSL has no such keys.

Tests.

crypto.key-objects.test.ts: 13 malformed or inconsistent JWKs at 12 call forms on 2 curves, the read order, a getter for x, the order of the two createPrivateKey errors, and base64 forms of x. 2 of the 344 new tests pass on main by design: they pin that a consistent private JWK still imports as a public key.
crypto-pqc.test.ts: the AKP order change.
Nothing under test/js/node/test/ changes. An earlier head added the OKP block of upstream test-crypto-key-objects.js, upstream v26.10.0's test-crypto-jwk-raw-validation.js, and an isBoringSSL export to test/common/crypto.js. That was wrong: test/js/node/test/parallel/CLAUDE.md says those tests are not modified, test/common/crypto.js was last synced to v26.3.0, and neither that file nor that export exists at v26.3.0. The four upstream OKP assertions are rows of the table above.
Not yet covered by a Bun test: a lower-case crv, an AKP alg used as crv, a getter that throws, and the AKP order of the alg check against the pub check. The removed upstream file covered them, and it passed on cd53fd7. Tests for them are written and not pushed, because I could not run them on a build of this change.
Also run on cd53fd7: the new tests with BUN_JSC_validateExceptionChecks=1, and scripts/jsc-exception-lint on KeyObject.cpp (no finding in the file).

Self-review. I checked the diff by hand for: the order of checks against Node's source, the lifetime of the two decoded buffers across allocations, exception checks after each call that can throw, the BoringSSL length checks that replace the deleted per-curve switch (Ed25519, X25519, ML-DSA, ML-KEM), and the state of a public key object that holds private material. None needed a code change.

History. The first version of this PR compared x with d only when the caller asked for a private key. That closed createPrivateKey and sign and left createPublicKey and verify open.

coderabbitai · 2026-06-27T21:36:02Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Essentials
Run ID: b662d174-116d-41d0-be21-a39387ba7e18

📥 Commits

Reviewing files that changed from the base of the PR and between 272ff43 and 5a165e3.

📒 Files selected for processing (3)

src/jsc/bindings/node/crypto/KeyObject.cpp
test/js/node/crypto/crypto-pqc.test.ts
test/js/node/crypto/crypto.key-objects.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

The JWK import path now handles OKP and AKP keys in a shared branch. Tests cover field validation, public/private key consistency, and missing private material across key-import APIs.

Changes

OKP and AKP JWK Import

Layer / File(s)	Summary
Shared parsing, construction, and validation `src/jsc/bindings/node/crypto/KeyObject.cpp`, `test/js/node/crypto/crypto.key-objects.test.ts`, `test/js/node/crypto/crypto-pqc.test.ts`	OKP and AKP JWKs share field parsing, key construction, and public/private consistency checks. The private-key mode reports missing private material after validation. Tests cover malformed and inconsistent fields, getter reads, padded base64, public-only keys, and OKP and AKP import cases.

Suggested reviewers: cirospaciari, dylan-conway

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 5a165

No actionable issue is established for this change; it is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main change: matching Node 26 OKP JWK import behavior by sharing the raw-key arm with AKP.
Description check	✅ Passed	The description explains the problem, fix, scope, compatibility impact, and verification. It does not use the template’s exact headings, but it provides the requested information, including how the co…

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-06-27T21:36:06Z

^{Updated 9:48 PM PT - Oct 2nd, 2026}

❌ @robobun, your commit 5a165e3 has 1 failures in Build #123115 (All Failures):

test/js/bun/spawn/spawn.test.ts - code 1: stdout reader of an unref'd child and process lifetime > an idle reader stopped at the highwater … on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 32910

That installs a local version of the PR into your bun-32910 executable, so you can run:

bun-32910 --bun

github-actions · 2026-06-27T21:43:14Z

This PR may be a duplicate of:

webcrypto: validate the OKP JWK x parameter against d on import #32827 - Both validate that OKP (Ed25519/X25519) private JWK x matches the public key derived from d on import, modifying the same files (CryptoKeyOKP.cpp, KeyObject.cpp)

🤖 Generated with Claude Code

claude

I didn't find any issues — the validation logic and size checks look correct and the tests are thorough — but since this changes crypto key import validation paths it's worth a quick human look.

Extended reasoning...

Overview

This PR adds validation in two OKP JWK private-key import paths to reject keys whose x (public) component is missing or doesn't match the public key derived from d (private), per RFC 8037 §2. It touches:

src/jsc/bindings/webcrypto/CryptoKeyOKP.cpp (~12 lines): replaces a FIXME with an explicit x presence check, base64url decode, a d length check (32 bytes), public-key derivation via existing ed25519PublicFromPrivate/x25519PublicFromPrivate, and a constant-time compare.
src/jsc/bindings/node/crypto/KeyObject.cpp (~11 lines): after building the private EVP_PKEY from d, decodes x and compares it against key.rawPublicKey() with CRYPTO_memcmp, throwing ERR_CRYPTO_INVALID_JWK on mismatch.
New test cases in web-crypto.test.ts and crypto.key-objects.test.ts covering missing/mismatched/wrong-length x, valid round-trip, and public-only JWK for both Ed25519 and X25519.

Security risks

The change strictly tightens validation — previously-accepted malformed keys are now rejected — so the direction is security-positive. I verified the new d->size() != externalKeySizeInBytesFromNamedCurve(namedCurve) guard runs before ED25519_keypair_from_seed/X25519_public_from_private are called on the decoded d, preventing under-read of a short seed buffer. Both compare paths use constant-time memcmp. No new attack surface is introduced; the only behavioral change is that some inputs now throw DataError/ERR_CRYPTO_INVALID_JWK instead of being silently repaired.

Level of scrutiny

This is crypto key-import code, which I treat as requiring human review even when the change is small and additive. The implementation reuses existing, already-exercised helpers (ed25519PublicFromPrivate, x25519PublicFromPrivate, rawPublicKey, constantTimeMemcmp, CRYPTO_memcmp, decodeJwkString) and follows the established pattern from the EC import path, so the risk is low — but it still merits a maintainer's eyes.

Other factors

No CODEOWNERS match the modified files. The bug-hunting system found nothing. Test coverage is comprehensive (5 cases × 2 curves for WebCrypto, 2 cases × 2 curves for node:crypto, plus the existing positive round-trip tests). The PR description includes a Node v26.3.0 comparison table showing the new behavior matches Node.

robobun · 2026-06-27T22:48:43Z

Good catch by the duplicate detector: #32827 (already open) covers the WebCrypto crypto.subtle.importKey("jwk") half of this bug class, and does it more thoroughly (it also adds the X25519 kty/key_ops/ext checks that arm was missing).

However, #32827 does not touch the node:crypto createPrivateKey({format: "jwk"}) path, which has the same silent-repair problem: it built the EVP_PKEY from d alone and never compared it to the supplied x.

I have narrowed this PR to just the node:crypto half (KeyObject::getKeyObjectHandleFromJwk). The two PRs now touch disjoint files and are complementary, not duplicates. The title and body are updated to match.

claude

Thanks for updating the title/description — the diff now matches its stated scope and the validation logic looks correct to me, but I'll leave the final sign-off to a human since this is a crypto key-import trust boundary.

Extended reasoning...

Overview

This PR adds an 11-line guard to KeyObject::getKeyObjectHandleFromJwk in src/jsc/bindings/node/crypto/KeyObject.cpp: after building the private EVP_PKEY from an OKP JWK's d, it base64url-decodes the supplied x, fetches the key's derived public bytes via EVPKeyPointer::rawPublicKey(), and rejects with ERR_CRYPTO_INVALID_JWK if x is the wrong length or differs (constant-time CRYPTO_memcmp). Two new negative tests per OKP curve in test/js/node/crypto/crypto.key-objects.test.ts cover the mismatched-x and wrong-length-x cases; the existing round-trip tests cover the happy path.

My earlier review flagged that the title/description claimed a WebCrypto fix that wasn't in the diff. The author has since retitled to node:crypto: ..., rewritten the repro around createPrivateKey, and added a "Relationship to #32827" section delegating the WebCrypto half — that concern is fully resolved and the inline comment is marked resolved.

Security risks

The change is strictly a tightening of input validation at a key-import trust boundary, matching Node.js behavior and RFC 8037 §2. It uses existing helpers (decodeJwkString, rawPublicKey()) and a constant-time compare. The size check precedes the memcmp, so there is no out-of-bounds read. I see no way this introduces a new vulnerability; if anything it closes a minor one (silent repair of inconsistent JWKs).

Level of scrutiny

This is node:crypto key-import code — explicitly security-sensitive — so per policy I'm deferring rather than auto-approving even though the change is small, mechanical, and well-tested. There is also an open sibling PR (#32827) covering the WebCrypto half of the same bug class; a human can confirm the two remain non-overlapping as stated.

Other factors

No CODEOWNERS entry covers this path. No bugs were surfaced by the bug-hunt on this revision. No outstanding unresolved reviewer comments.

robobun · 2026-06-28T02:24:44Z

CI has now gone red three times on this PR, each time on infrastructure or unrelated flake, and never on anything this diff touches. Build 65830 has since finished, so updating this comment for accuracy: the finished build has two failed test files, not zero. Breakdown of the finished build 65830 (255 jobs passed):

test/js/sql/tls-sql.test.ts (darwin 14 aarch64): the postgres_tls service container failed to build because Docker Hub returned 429 Too Many Requests (toomanyrequests: You have reached your unauthenticated pull rate limit) while pulling postgres:15.13. No assertion ran. Purely external.
test/js/bun/spawn/spawn-pipe-leak.test.ts (windows 11 aarch64): an RSS growth threshold in a Bun.spawn pipe leak test, expect(pct).toBeLessThan(0.8) observed 1.6. That test already has a dedicated deflake commit in its history (0a92d64, "Deflake test/js/bun/spawn/spawn-pipe-leak.test.ts"), so it is a known flaky RSS assertion. It never calls createPrivateKey and cannot reach the changed code.
The only other failed jobs are two darwin 26 aarch64 - test-bun shards, both of which died on buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' before running a single test. This is the third and fourth occurrence of that exact error on this PR (it also killed both darwin 26 shards on build 65761). That agent pool currently has an artifact transfer problem.
5 jobs ended expired (Buildkite never assigned them an agent) and 22 downstream jobs were waiting_failed as a consequence.

For the earlier builds: 65761 (identical diff) hard-failed only on the same darwin artifact download timeout, and 65717 only on unrelated lanes.

test/js/node/crypto/crypto.key-objects.test.ts, which holds the new tests, does not appear in any failure or flake annotation on any of the three builds, and the changed function is only reachable from an OKP JWK private key import. The new tests fail on main and pass with the change (verified locally on a debug ASAN build).

I already pushed one empty commit to re-roll CI and do not want to keep spamming the history with more, so I will stop here. Retrying the failed and expired jobs on build 65830 in the Buildkite UI, or kicking off a fresh build, should be all that is needed to get this green.

robobun · 2026-09-16T14:55:33Z

Stale PR review: keep open, rework.

The fix is wanted. Bun reports Node v26.3.0, and Node v26.3.0 throws ERR_CRYPTO_INVALID_JWK ("Invalid JWK OKP key") when the x of an Ed25519 or X25519 private JWK does not match d (nodejs/node#62499). Bun 1.4.3 and main accept a zeroed, 8-byte, or empty x in createPrivateKey and crypto.sign, and export({ format: "jwk" }) returns a different x. No other open or merged PR covers the OKP arm. The AKP arm of the same function already has this check (src/jsc/bindings/node/crypto/KeyObject.cpp:1277-1288).

The current diff is not the Node v26 behavior yet:

The error message is the default "Invalid JWK data". Node says "Invalid JWK OKP key".
The check runs only when the caller asks for a private key (keyType != CryptoKeyType::Public). In Node the JWK decides: a string d means private material on every path. With a zeroed x, Node v26.3.0 throws from createPublicKey, crypto.sign, and crypto.verify too. Bun 1.4.3 imports the wrong x in createPublicKey, and crypto.verify returns false.
xBuf has no JSC::ensureStillAliveHere, which the AKP arm has for pubBuf.

Wanted shape: rebase onto main (the head is 2212 commits behind, and build 65830 is red), mirror the AKP arm in case Kty::Okp (run the check whenever d is a string, and throw with "Invalid JWK OKP key"), and extend the test to createPublicKey, crypto.sign, and crypto.verify, with the short and empty x cases.

One compatibility note for the PR body: code that passes a placeholder x starts to throw, the same as on Node 26. @libp2p/crypto did this before 5.1.18 (libp2p/js-libp2p#3491).

…with AKP An OKP (Ed25519, X25519) JWK that carries "d" is now a private key at every entry point. The import builds the key from "d" and rejects the JWK with ERR_CRYPTO_INVALID_JWK ("Invalid JWK OKP key") when "x" is not the public key of "d". Before, the caller's mode picked the member: createPublicKey and verify built the key from "x" and never read "d", and createPrivateKey and sign built it from "d" and never compared "x". One JWK was two different keys. The OKP arm of KeyObject::getKeyObjectHandleFromJwk now shares the body of the AKP arm, as Node's ImportJWKEdKey and ImportJWKPqcKey share one shape. The arm reads the name, the public member and the private member once each before it validates them, and it reports each malformed member with the same message. The check for a public-only JWK at a private-key consumer moves after the import, as in Node. A public-only JWK with a malformed public member now reports the malformed key, for AKP too.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the moved "JWK does not contain private key material" check: for sign()/createSign()/privateEncrypt()/privateDecrypt() with a public-only OKP JWK, Bun now throws ERR_CRYPTO_INVALID_JWK where Node throws ERR_INVALID_ARG_TYPE from the operation. The diff comments this as a deliberate divergence, so I did not file it, but a maintainer should confirm that divergence is acceptable.

Extended reasoning...

The commit merges the OKP JWK import arm into the AKP raw-key arm in src/jsc/bindings/node/crypto/KeyObject.cpp, adding the x-vs-d consistency check and relying on BoringSSL for raw-key length validation, and adds tests under test/js/node/crypto and the vendored test/js/node/test tree. It touches crypto key import, a security-sensitive trust boundary, and two verified findings are posted inline, so approval is not appropriate. The one additional candidate examined was the ERR_CRYPTO_INVALID_JWK vs ERR_INVALID_ARG_TYPE ordering change for private-key consumers given a public-only JWK, which the code explicitly documents as an intentional divergence from Node.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

🟣 src/jsc/bindings/node/crypto/KeyObject.cpp — A caller importing an EC private JWK whose d string fails to decode gets a null dereference instead of a thrown error. KeyObject.cpp:1367 calls decodeJwkString for key.d with no RETURN_IF_EXCEPTION, then :1368 reads dBuf->span() even when decodeJwkString returned null with an exception pending. The sibling calls at :1356-1359 and the rewritten OKP/AKP arm at :1265-1273 all check. Fix: add RETURN_IF_EXCEPTION(scope, {}) after every decodeJwkString call in getKeyObjectHandleFromJwk, including this EC d decode.

Why this was flagged

Trigger: createPrivateKey/sign with {format:'jwk', key:{kty:'EC', crv:'P-256', x, y, d}} where decoding d throws inside constructFromEncoding at KeyObject.cpp:1135 (allocation failure for a very long d string, or any future throwing path) or where the decoded value is not an ArrayBufferView so :1139 throws ERR_INVALID_ARG_TYPE and returns null. The EC arm at KeyObject.cpp:1367 ignores the pending exception and :1368 dereferences the null dBuf, crashing the process instead of surfacing the error; with BUN_JSC_validateExceptionChecks=1 this is an unchecked-exception violation. The base branch has the identical lines, so this is pre-existing, but it sits in the same function this PR rewrote, every other decodeJwkString call in the function is checked, and the repository's rule is to fix the whole class in the same PR.

Verification: pre-existing — the EC arm is byte-identical at base (bc7a813, line 1414) and HEAD (line 1367). /home/claude/bun/src/jsc/bindings/node/crypto/KeyObject.cpp:1367 auto* dBuf = decodeJwkString(...) is followed at :1368 by dBuf->span() with no RETURN_IF_EXCEPTION. decodeJwkString returns null whenever constructFromEncoding throws (:1136), e.g. on OOM, so :1368 dereferences null.

claude · 2026-10-03T03:29:08Z

        if (jwkType == CryptoKeyType::Private) {
            auto derivedPub = key.rawPublicKey();
            auto expected = pubBuf->span();
            if (!derivedPub || derivedPub.size() != expected.size()
                || CRYPTO_memcmp(derivedPub.get(), expected.data(), expected.size()) != 0) {
-                ERR::CRYPTO_INVALID_JWK(scope, globalObject, "Invalid JWK AKP key"_s);
+                ERR::CRYPTO_INVALID_JWK(scope, globalObject, invalidKey);
                return {};
            }


🔴 Apps that use @ libp2p/crypto below 5.1.18 on Bun lose Ed25519 key handling after merge, which works on the base branch. Those versions call createPrivateKey with a JWK carrying a valid d and x: '' to derive the public key; KeyObject.cpp:1300 now rejects any x whose decoded length differs from the derived key, so the call throws ERR_CRYPTO_INVALID_JWK "Invalid JWK OKP key". This follows from the stated purpose (Node 26 parity), and the description names the package. Fix: the call is the maintainers' — if the breakage is accepted, label the PR breaking and release-note it; otherwise the derived-key check must stay while a documented, Node-matching lenience for a placeholder x is agreed upstream first.

Why this was flagged

The trigger is a private OKP JWK whose x is an empty string or otherwise not the public key of d, passed to createPrivateKey, createPublicKey, sign, verify or any other node:crypto entry point that imports a JWK. @ libp2p/crypto 4.1.9 and 5.1.7 through 5.1.17 build exactly this JWK ({ crv: 'Ed25519', x: '', d, kty: 'OKP' }) inside derivePublicKey and then export the key to read x. In the new shared arm KeyObject.cpp:1263 decodes x unconditionally, KeyObject.cpp:1298-1301 derives the public key from d and compares it with CRYPTO_memcmp, and the size mismatch (32 versus 0) throws ERR_CRYPTO_INVALID_JWK "Invalid JWK OKP key" at KeyObject.cpp:1302. On the base branch the old OKP arm read only d for private consumers and never looked at the content of x, so the same call succeeded and returned the key. No safeguard mitigates this: the PR description lists it as a downside but there is no release note or breaking label visible in the change. Node 26.3.0 is claimed to reject the same input, so the behaviour is the stated purpose.

Verification: In src/jsc/bindings/node/crypto/KeyObject.cpp the merged OKP arm now always decodes x (line ~1263), then for a string d builds the key from d and compares (lines ~1296-1304). For x: '', expected.size() is 0 vs derivedPub.size() 32, so createPrivateKey throws ERR_CRYPTO_INVALID_JWK "Invalid JWK OKP key". On base x was never decoded or compared for a private consumer, so the same input succeeded.

This is correct, and the decision belongs to a maintainer, not to me. The break is the stated purpose: the same call throws on Node v26.3.0. I checked the versions myself: @libp2p/crypto 4.1.9, 5.1.7, 5.1.12, 5.1.13 and 5.1.17 pass x: '' next to d, and 5.1.18 does not.

I added the breaking label and put the choice first in the Notes of the PR body. The one alternative is a Bun-only exception that treats an empty x next to a string d as absent. Node has no such exception, so this PR does not add one. I leave this thread open for a maintainer to decide.

robobun · 2026-10-03T03:38:20Z

The head moved. It is now 5a165e3, on main bc7a813. This is the rework that the review comment of 2026-09-16 asked for, in this PR.

What changed since 7f06207

cd53fd7: case Kty::Okp now shares the body of the AKP arm. A string d makes the JWK a private key at every entry point, and x must be the public key of d. The error is Node's: ERR_CRYPTO_INVALID_JWK, "Invalid JWK OKP key".
5a165e3: two comments shortened to one line each. This commit also removes three changes under test/js/node/test/ that cd53fd7 had added. Its message names only the comments. I pushed it before the message was right.

How I reproduced it. The script is in the Notes of the PR body. It gives one JWK the d of key A and the x of key B.

Node v26.3.0: ERR_CRYPTO_INVALID_JWK four times.
main: A, B, false, true.
The earlier head 7f06207: ERR_CRYPTO_INVALID_JWK, B, ERR_CRYPTO_INVALID_JWK, true.
cd53fd7: ERR_CRYPTO_INVALID_JWK four times.

The test runs and measurements in the PR body were made on cd53fd7. The head differs from it by 7 comment lines in KeyObject.cpp and by the removed test changes. I have not built the head itself. CI has: see below.

Two decisions for a maintainer

This PR is breaking, and I labelled it so. createPrivateKey with x: '' next to d works on main and throws here, as on Node v26.3.0. @libp2p/crypto below 5.1.18 makes that call. Node has no exception for a placeholder x. A Bun-only exception for an empty x is possible and is not in this PR.
The shared arm also moves one AKP check order to Node's (36 of 1,352 probed AKP cells). A cloned OKP arm would leave AKP untouched at about 1 KB more.

Answers to the review

Vendored tests: correct, and fixed in 5a165e3. test/js/node/test/parallel/CLAUDE.md says those tests are not modified. The file I had added is upstream v26.10.0's, byte for byte. But it and the isBoringSSL export do not exist at v26.3.0, and test/common/crypto.js was last synced to v26.3.0. Nothing under test/js/node/test/ changes now.
The EC d decode without an exception check: real, and older than this PR. That one line is all of node:crypto: fix null deref when worker.terminate() lands during an EC JWK private key import #37443, which is open, so I did not repeat it here.
A public-only OKP JWK at sign: Node v26.3.0 fails in the operation with ERR_OSSL_NOT_A_PRIVATE_KEY for Ed25519 (I ran it), not with ERR_INVALID_ARG_TYPE. This PR throws ERR_CRYPTO_INVALID_JWK at import, as the AKP arm already did. The PR body lists it.

CI on this head. Build 123115 built 5a165e3 and ran the suites: 180 jobs passed and 1 failed. No file of this PR appears in a failure or in a retried test.

The failed job is debian 13 x64-asan - test-bun, on test/js/bun/spawn/spawn.test.ts: "an idle reader stopped at the highwater mark does not keep the process alive". That test has no crypto in it.
The same test fails or passes only on retry in 35 of the 60 most recent finished builds of the repository. 34 of those are other branches. The two builds of main that I checked have no ASAN test job.
I do not push an empty commit to run CI again.

github-actions Bot added the claude label Jun 27, 2026

claude Bot reviewed Jun 27, 2026

View reviewed changes

robobun force-pushed the farm/a27f804b/okp-jwk-validate-x branch from 0414b01 to 56740cc Compare June 27, 2026 22:46

robobun changed the title ~~webcrypto: validate OKP private JWK x matches d on import~~ node:crypto: validate OKP private JWK x matches d in createPrivateKey Jun 27, 2026

claude Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread src/jsc/bindings/node/crypto/KeyObject.cpp Outdated

claude Bot reviewed Jun 28, 2026

View reviewed changes

robobun mentioned this pull request Jul 8, 2026

node:crypto: validate EC JWK private scalar matches public point in createPrivateKey #33752

Open

robobun force-pushed the farm/a27f804b/okp-jwk-validate-x branch from 7f06207 to cd53fd7 Compare October 3, 2026 02:17

github-actions Bot reviewed Oct 3, 2026

View reviewed changes

Comment thread src/jsc/bindings/node/crypto/KeyObject.cpp Outdated

github-actions Bot reviewed Oct 3, 2026

View reviewed changes

Comment thread src/jsc/bindings/node/crypto/KeyObject.cpp Outdated

robobun changed the title ~~node:crypto: validate OKP private JWK x matches d in createPrivateKey~~ node:crypto: import OKP JWKs like Node 26, in one raw-key arm shared with AKP Oct 3, 2026

claude Bot reviewed Oct 3, 2026

View reviewed changes

Comment thread test/js/node/test/parallel/test-crypto-jwk-raw-validation.js Outdated

node:crypto: shorten two comments in the raw-key JWK arm

5a165e3

claude Bot reviewed Oct 3, 2026

View reviewed changes

robobun added breaking node:crypto labels Oct 3, 2026

Conversation

robobun commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Downsides

Uh oh!

coderabbitai Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

robobun commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 27, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jun 27, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

robobun Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

robobun commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jun 27, 2026 •

edited

Loading

coderabbitai Bot commented Jun 27, 2026 •

edited

Loading

robobun commented Jun 27, 2026 •

edited

Loading

robobun commented Jun 28, 2026 •

edited

Loading

robobun commented Oct 3, 2026 •

edited

Loading