Skip to content

webcrypto: convert JWK key_ops and oth as the Web Crypto IDL types them - #42102

Open
robobun wants to merge 10 commits into
mainfrom
robobun/52a188a1/jwk-key-ops-domstring
Open

robobun wants to merge 10 commits into
mainfrom
robobun/52a188a1/jwk-key-ops-domstring

Conversation

@robobun

@robobun robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • crypto.subtle.importKey("jwk", { kty: "oct", k, key_ops: ["encrypt", "fly"] }, "AES-GCM", true, ["encrypt"]) rejects with TypeError: value must be enumeration (string). Node v26.12.1 imports the key. A comparison with Node found this, not a user report.
  • The cause is convertDictionary<JsonWebKey> (src/jsc/bindings/webcrypto/JSJsonWebKey.cpp:150). It converts key_ops as sequence<KeyUsage>. The Web Crypto IDL has sequence<DOMString>, and RFC 7517 section 4.3 lets a JWK name other operations.
  • oth: [{}] is a TypeError too: the converter requires r, d and t. The IDL does not.

Fix

Background

  • importKey("jwk") and unwrapKey("jwk") share this converter. A caller that owns its importKey call can filter key_ops first. A call inside a library cannot: hono's jwk({ jwks_uri }) answers 401.
  • WebKit converts both members the old way, and Safari rejects these keys. Bun leaves WebKit here on purpose.
  • Considered a filter in the converter (4 files). It drops the strings before the duplicate check, so ["encrypt", "fly", "fly"] imports where Node's test wants a DataError.

Downsides

  • A JWK that Bun refused with a TypeError now imports, or fails with a DataError.
  • exportKey("jwk") runs 50 to 70 more instructions for each usage after the first (7,148 to 7,209 with two): it writes strings. importKey("jwk") runs 36 to 199 fewer.
  • Release .text: 512 bytes smaller.
Notes

What this PR is now. The PR of 2026-09-09 had two parts. Main covers one of them since #47559.

part main 3f3a9ff this PR
a repeated key_ops entry imports for AES, RSA, EC and OKP keys covered by #47559: jwkKeyOpsAllow in CryptoKey{AES,RSA,EC,OKP}::importJwk removed from here: CryptoAlgorithm::validateJwkKeyOps, its 15 call sites, and Node's message text
a key_ops entry that is not a KeyUsage is a TypeError not covered kept, in a smaller form: 17 source files, +73/-48, no new source file

The branch has main merged. The merge of bc55d56 takes main's tree as it is, and the commits after it hold the whole change. #33902 was an earlier PR of mine for the duplicate part alone. I closed it in favour of this one.

Where this comes from. No issue and no user report exists for it. gh search issues --repo oven-sh/bun finds no issue for key_ops and none for "value must be enumeration". The difference showed in a comparison of importKey("jwk") with Node v26.12.1. The grounds are:

  • the Web Crypto IDL: sequence<DOMString> key_ops, and RsaOtherPrimesInfo with no required member,
  • RFC 7517 section 4.3: "Other values MAY be used",
  • Node v26.12.1, and Node's own test/parallel/test-webcrypto-key-ops-duplicates.js, which imports ["sign", "future"].

WebKit has sequence<CryptoKeyUsage> key_ops and required on r, d and t (Source/WebCore/crypto/JsonWebKey.idl, RsaOtherPrimesInfo.idl). Its LayoutTests/crypto/subtle/import-key-malformed-parameters.html expects a rejection for key_ops: ["foo"]. So Safari rejects these keys on purpose, and Bun differs from WebKit on purpose after this PR. JsonWebKey.h says so at the member.

Who this can change, and who it cannot.

  • It changes a caller that passes a JWK on as it got it, with usages of its own. With hono 4.13.15, jwk({ jwks_uri }) and a JWKS key that has key_ops: ["verify", "urn:example:attest"]: HTTP 401 on main (the cause is the TypeError), HTTP 200 on Node v26.12.1 and with this PR. Azure Key Vault is one source of such values (import, export).
  • It does not change a caller of jose. importJWK passes key_ops on as the keyUsages argument, and a name that is not a KeyUsage is a TypeError there in Node too.
  • importKey is where such a JWK is known to arrive. unwrapKey("jwk") changes because it shares convertDictionary<JsonWebKey>. I know of no program that wraps a JWK with such an entry.

A caller that owns its importKey call gets Node's results on a released Bun with this wrapper. With it, the installed Bun 1.4.3 canary agrees with Node v26.12.1 on all 242 importKey cells of the census below.

async function importJwk(jwk, algorithm, extractable, usages) {
  const ops = typeof jwk.key_ops === "object" ? [...jwk.key_ops].map(op => `${op}`) : undefined;
  if (ops && new Set(ops).size !== ops.length) throw new DOMException("Duplicate key operation", "DataError");
  const key_ops = ops?.filter(op => usages.includes(op));
  return crypto.subtle.importKey("jwk", ops ? { ...jwk, key_ops } : jwk, algorithm, extractable, usages);
}

The wrapper cannot reach a call inside a library (the hono row). It also does not fit unwrapKey("jwk") when the wrapping key has unwrapKey and not decrypt. The one detour there unwraps the bytes as an extractable raw HMAC key, exports it, filters the JSON and imports it. That puts the key material into script, which an unwrap into a key that is not extractable must not do. Those two cases are why the converter changes.

Results. 11 algorithms (AES-GCM, AES-KW, AES-CBC, HMAC, ChaCha20-Poly1305, RSA-OAEP, RSASSA-PKCS1-v1_5, ECDSA, ECDH, Ed25519, X25519), 22 shapes of key_ops, through importKey("jwk") and unwrapKey("jwk"): 484 cells. Each cell is the usages of the key or the name of the error. "main" is a debug build with the src/ of 3f3a9ff, "this PR" a debug build of this branch.

cells that agree with Node v26.12.1
importKey, main 110 of 242
importKey, this PR 242 of 242
unwrapKey, main 66 of 242
unwrapKey, this PR 187 of 242

264 cells change between main and this PR. Each of them is the TypeError on main. 224 of them import now, and 40 are a DataError. The rows for AES-GCM with the usages ["encrypt", "decrypt"], through importKey:

key_ops Node v26.12.1 main this PR
["encrypt","decrypt","x-op"], ["x-op","encrypt","decrypt"] imports TypeError imports
["encrypt","decrypt","x-op","y-op"] imports TypeError imports
["encrypt","decrypt",7], [..., null], [..., true], [..., {}], [..., ""], [..., "ENCRYPT"] imports TypeError imports
["x-op"] DataError TypeError DataError
["encrypt","decrypt","x-op","x-op"] DataError TypeError DataError
["encrypt","decrypt","encrypt"], [] DataError DataError DataError
"encrypt", 5, {}, null (not a sequence) TypeError TypeError TypeError

oth, 6 keys and 10 shapes, 60 cells: 41 agree with Node on main and 59 with this PR. oth: [{}], [{ r }] and [null] import for a key that is not RSA, and are a DataError for an RSA key. The one cell that still differs is a private RSA JWK with a full entry (r, d, t). Node imports it as a multi-prime key. BoringSSL has no multi-prime RSA, and #47591 rejects every oth.

A repeated entry that is not a KeyUsage. ["encrypt","fly","fly"] is a DataError here. Node v26.12.1 rejects it, and Node v26.3.0 imported it: validateKeyOps now keeps a set of every entry. Node's test asserts all three cases (["sign","future"] imports, ["sign","sign"] and ["sign","future","future"] are a DataError). The file is in this PR as Node has it at v26.12.1. It exits 1 on main, at line 13.

Not changed here.

  • Message text. The rejected imports of AES, RSA, EC and OKP keys keep Bun's generic DataError message. node:crypto, webcrypto: assorted Node compatibility fixes #47559 lists Node's text (Duplicate key operation, Key operations and usage mismatch) as a difference it did not change.
  • The keyUsages argument. importKey("raw", bytes, "AES-GCM", true, [{ toString() { return "encrypt" } }]) is TypeError: value must be a string. Node v26.12.1 and Bun 1.2.23 convert the entry with ToString and import the key. Bun 1.3.0 does not. The cause is in the shared sequence conversion since Add new bindings generator; port SSLConfig #23169 (Bun::DefaultConversionContext, src/jsc/bindings/webcore/JSDOMConvertSequences.h), at 8 call sites (JSSubtleCrypto.cpp:407, :440, :522, :638, :662, :719, :782 and node/crypto/KeyObject.cpp:965). It is a different defect with a different cause, so it is not in this PR. After this PR one importKey("jwk") call converts a key_ops entry with ToString and a keyUsages entry without. I handed it on as separate work.
  • The 55 unwrapKey cells that differ from Node. They are in the parse step of unwrapKey, which this PR does not touch. In 44, key_ops is not a sequence: TypeError here, DataError in Node, the same as on main. In 11, an entry of key_ops in the wrapped JSON is an object: TypeError on main, an import here, DataError in Node. Node parses a wrapped JWK into objects with no prototype, so ToString throws for that entry.

How a usual key pays nothing for the duplicate check. hasDuplicateJwkKeyOps takes the JsonWebKey. usages has one bit for each distinct entry that names a KeyUsage. If key_ops has as many entries as usages has bits, no entry is repeated and none is foreign, and the check returns. Only a key_ops with a foreign or a repeated entry reaches the HashSet<String> scan in SubtleCrypto.cpp.

Cost, measured on release builds. Release builds (ThinLTO, linux-x64) of 3f3a9ff and of this branch (d350e24). The commit after d350e24 (fd64a85) names the two values of the duplicate check and shortens two comments in JsonWebKey.h. In a reduction of the check, clang 23 at -O2 emits the same 10 instructions for the old form and the new form (-march=nehalem and -march=haswell). Only the order of the popcount and the size load differs. The numbers are instructions in user mode on the main thread for one call: the slope between a loop of 100,000 calls and a loop of 300,000 (perf_event_open), with the smallest of 5 runs for each loop, and the two builds in turn. Single runs of one cell spread by 7 to 200 instructions, and a few runs are off by about 600. The smallest count of each loop removes that. The Node column is one run, for scale.

call main this PR Node v26.12.1
importKey("raw") AES-GCM, 1 usage 5,892 5,825
importKey("raw") AES-GCM, 2 usages 6,112 6,039 23,916
generateKey AES-GCM, 2 usages 9,323 9,330
importKey("jwk") AES-GCM, no key_ops 15,825 15,713 32,282
the same, 1 key_ops 16,921 16,885
the same, 2 key_ops 17,147 17,071 35,655
the same, 12 key_ops 19,974 19,775 44,138
the same, 2 key_ops and 1 entry that is not a KeyUsage rejects 17,938 36,174
importKey("jwk") HMAC, 2 key_ops 19,860 19,819 42,727
importKey("jwk") ECDSA public, 1 key_ops 28,852 28,762
importKey("jwk") RSA-OAEP public, 1 key_ops 24,944 24,880
unwrapKey("jwk") AES-GCM, 2 key_ops 27,500 27,459
exportKey("jwk") AES-GCM, 1 usage 6,957 6,966
exportKey("jwk") AES-GCM, 2 usages 7,148 7,209 19,418
exportKey("jwk") AES-GCM, 4 usages 7,514 7,720
importKey("raw") and the first read of key.usages, 4 usages 8,894 8,900

The installed Bun 1.4.3 canary (367d939) is an official build with another build configuration. For the six rows that it can run and that have a Node number it gives 5,620, 15,277, 16,335, 19,056, 19,068 and 6,915.

  • The import rows go down. toKeyData and SubtleCrypto::importKey each copied the JsonWebKey, and unwrapKey copied it once. These are moves now. JsonWebKey::JsonWebKey(const JsonWebKey&) (905 bytes) is not in the binary any more.
  • The export rows go up by 50 to 70 instructions for each usage after the first. exportKey and wrapKey write key_ops as a vector of strings (toJwkKeyOps), where main has a vector of enum values. A single-step trace of one exportKey("jwk") call with 4 usages has 6,297 instructions on main and 6,495 here. The difference is in toJwkKeyOps, convertEnumerationToString and the destructor of the vector. The number of allocations is the same. An export that pays nothing needs a second member in JsonWebKey: strings for the import, enum values for the export. I did not build that.
  • The KeyUsage lookup. parseEnumerationFromString<CryptoKeyUsage> has two callers. In the first version of this change the optimizer inlined it into both, and the scan of the 12 names became an out-of-line std::__find_if. Every conversion of a KeyUsage paid for it: on the earlier base, +245 instructions for an AES JWK with 2 key_ops, +1,136 with 12, and +300 for generateKey. NEVER_INLINE keeps the scan in one function, as on main. nm bun-profile | grep '__find_if.*CryptoKeyUsage' prints nothing for main and for this branch.
  • Size. .text is 110,632,729 bytes on main and 110,632,217 here (-512). .rodata is 26,768,364 bytes in both. The stripped bun is 140,474,656 bytes in both. sizeof(JsonWebKey) is 200 bytes before and after. CI's size check of fd64a85 (build 136478) against main's canary build 136408 shows +0.0 KB on 7 of the 12 targets and 1.5 KB to 5.5 KB less on the other 5 (Android x64, FreeBSD x64 and aarch64, Windows x64 and aarch64).
  • exportKey("jwk") of 9 fixed keys (AES-GCM, AES-KW, HMAC, ChaCha20-Poly1305, RSA-OAEP, ECDSA, ECDH with no usages, Ed25519, X25519) and one wrapKey("jwk") give the same bytes on both builds.
  • bench/snippets/webcrypto-keys.mjs is new. It has rows for importKey, unwrapKey, exportKey and generateKey. bench/ had no row for a key operation.

Tests.

  • test/js/web/crypto/ on a debug build of this branch: 134 pass. With the src/ of 3f3a9ff the four new tests fail and the other 113 of web-crypto.test.ts pass.
  • The test for each key type has one row for each place that checks key_ops (CryptoKey{AES,RSA,EC}::importJwk, the two branches of CryptoKeyOKP::importJwk, and the HMAC, ChaCha20-Poly1305 and ML-DSA importers). Each row also exports its key, so each of the seven places that write key_ops has a row.
  • test/js/node/test/parallel/test-webcrypto-* (40 files, the new one included) and test-crypto-key-objects-to-crypto-key.js: all exit 0.
  • test/js/deno/crypto/webcrypto.test.ts, test/js/node/crypto/crypto.key-objects.test.ts, crypto-pqc.test.ts, sign-jwk-ieee-p1363.test.ts, test/js/bun/jsc/common-strings.test.ts: 166 pass, 27 skip, 1 todo.
  • Two older tests forced a TypeError from the dictionary conversion with key_ops: ["bogus"]. That value converts now, so they use key_ops: "sign", which is not a sequence and still throws there.
  • Self-review: of the 17 concerns, two disagreed about oth. One asked to leave it out until RSA import rejects a present oth. node:crypto, webcrypto: a third batch of Node compatibility fixes #47591 does that now, so oth is in.

The account of 2026-09-09 (against the main of that day). Its validateJwkKeyOps part is not in the PR any more. Its sentence that such a JWK "fails only in bun" is wrong: Safari rejects it too.

Problem

  • importKey("jwk", { kty: "oct", k, key_ops: ["encrypt", "fly"] }, "AES-GCM", true, ["encrypt"]) rejects with TypeError: value must be enumeration (string). Node 26 and Chromium 148 import the key. A JWK with a JOSE-side or vendor key_ops value fails only in bun.
  • Cause: JSJsonWebKey.cpp converts key_ops as sequence<KeyUsage>. The spec IDL says sequence<DOMString>, and RFC 7517 section 4.3 allows other values.
  • The inverse also held: key_ops: ["encrypt", "encrypt"] imported for AES, RSA, EC and OKP keys. RFC 7517 forbids duplicates. Node and Chromium reject them.

Fix

  • JsonWebKey.key_ops becomes std::optional<Vector<String>>. normalizeJsonWebKey builds the usages bitmap from the entries that name a KeyUsage and skips the rest.
  • A new CryptoAlgorithm::validateJwkKeyOps rejects duplicate strings, then key_ops that miss a requested usage, with Node's Duplicate key operation and Key operations and usage mismatch messages. Every jwk import branch calls it after the use check. HMAC, ChaCha20-Poly1305 and AKP had this inline. The other key classes gain it.
  • Verified: test/js/web/crypto/web-crypto.test.ts (three new tests, stock bun fails all three), the Node test-webcrypto-export-import* tests, and the key_ops blocks upstream Node main adds for EC, RSA and CFRG.
  • Self-reviewed: 2 concerns raised, 1 addressed (one helper with Node's messages). The other also folds use and ext into it, a wider refactor.

Background

  • JsonWebKey (src/jsc/bindings/webcrypto/JsonWebKey.h) is the C++ struct behind the WebIDL JsonWebKey dictionary. importKey("jwk") and unwrapKey(..., "jwk") both convert into it. Each CryptoAlgorithm*::importKey jwk branch checks its members, then calls CryptoKey*::importJwk.
  • JWK key_ops shares its strings with the WebCrypto KeyUsage enum, but a JWK is a JOSE object and can name operations WebCrypto does not define.

Notes of that account.

Behavior matrix for importKey("jwk", { kty: "oct", k, key_ops }, "AES-GCM", true, ["encrypt"]):

key_ops bun before bun after node 26.3 Chromium 148
["encrypt","fly"] TypeError imported imported imported
["encrypt","deriveBits","x-custom"] TypeError imported imported imported
[1,"encrypt"] TypeError imported imported imported
["encrypt",null] TypeError imported imported imported
["fly"] TypeError DataError DataError DataError
["encrypt","encrypt"] imported DataError DataError DataError
["encrypt","fly","fly"] TypeError DataError imported DataError
"encrypt" (not a sequence) TypeError TypeError TypeError TypeError

Export sites map usages() to strings with the new toJwkKeyOps helper in JsonWebKey.cpp.

Upstream WebKit still converts key_ops as sequence<CryptoKeyUsage>, so this diverges from it on purpose.

The one cell where Node and Chromium differ is a repeated unknown value. Node's validateKeyOps skips unknown values in its duplicate scan. Chromium's GetWebCryptoUsagesFromJwkKeyOps tracks them in a set and rejects. RFC 7517 section 4.3 ("Duplicate key operation values MUST NOT be present in the array") does not limit the rule to registered values, so this follows Chromium. Node's reading would be a one-line change in hasDuplicateJwkKeyOps.

Upstream Node main (newer than v26.3.0) asserts { name: 'DataError', message: 'Duplicate key operation' } for EC, RSA and CFRG JWKs in test-webcrypto-export-import-{ec,rsa,cfrg}.js, after a SyntaxError for an unsupported usage. Those blocks pass on this branch. The vendored copies in test/js/node/test/parallel/ do not have them yet.

For AES the key_ops check runs in the algorithm's jwk branch before CryptoKeyAES::importJwk, which still holds the kty, use, alg and ext checks with the generic message. So for an AES JWK that is wrong in two ways at once, the key_ops message wins where Node reports kty or use first. Both are DataError.

CryptoKey{AES,RSA,EC,OKP}::importJwk keep their own "key_ops covers the usages" test from WebKit. It cannot fire after validateJwkKeyOps, and it stays as the guard for a caller that skips the branch checks.

Two existing tests used key_ops: ["bogus"] to force a dictionary-conversion TypeError in the unwrapKey leak regression test. That input is valid now, so they use key_ops: "sign" (not a sequence), which still throws during conversion.

Other suites run locally: test-webcrypto-export-import{,-cfrg,-ec,-rsa,-ml-dsa,-ml-kem}.js, test-webcrypto-wrap-unwrap.js, test-webcrypto-encrypt-decrypt-{aes,chacha20-poly1305}.js, test-webcrypto-cryptokey-hidden-slots.js, test-webcrypto-internal-slots.mjs, test-webcrypto-sign-verify-{hmac,ml-dsa}.js, test-webcrypto-encap-decap-ml-kem.js, test-webcrypto-derivekey.js, test/js/deno/crypto/webcrypto.test.ts, web-crypto-sha3.test.ts, x25519-derive-bits.test.ts.

#33902 (July, conflicting) covered only the duplicate half. It rejected in normalizeJsonWebKey before algorithm dispatch, which puts the duplicate error ahead of the use check that main now orders like Node. This PR supersedes it.


[human-review] gate passed · iteration 1 · 20 files touched

fails on main (without fix)
ASAN without fix: 4 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/crypto/web-crypto.test.ts
bun test v1.4.4 (367d939d9)

test/js/web/crypto/web-crypto.test.ts:
(pass) crypto.subtle setter should not throw [2.71ms]
(pass) getRandomValues and randomUUID have the lengths of the IDL [1.06ms]
(pass) Web Crypto > keeps event loop alive [301.57ms]
(pass) Web Crypto > has globals [2.89ms]
(pass) Web Crypto > should encrypt and decrypt [13.64ms]
(pass) Web Crypto > should verify and sign [22.53ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are not valid JSON [12.63ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are valid JSON but not a valid JWK [8.72ms]
(pass) Web Crypto > unwrapKey JWK error handling > settles when JsonWebKey dictionary conversion itself throws [44.26ms]
(pass) Web Crypto > unwrapKey JWK error handling > does not leak DeferredPromise in m_pendingPromises on JWK parse errors [2149.05ms]
(pass) oversized inputs > rejects >2 GiB inputs instead of aborting [607.56ms]
(pass) oversized outputs > rejects an output over
... (truncated)

release without fix: 23 FAILED
bun test v1.4.3-canary.1 (367d939d9)

test/js/web/crypto/web-crypto.test.ts:
(pass) crypto.subtle setter should not throw [0.13ms]
15 |   expect(() => (globalThis.crypto.subtle = 123)).not.toThrow();
16 |   expect(globalThis.crypto.subtle).toBe(subtle);
17 | });
18 | 
19 | it("getRandomValues and randomUUID have the lengths of the IDL", () => {
20 |   expect(crypto.getRandomValues.length).toBe(1);
                                             ^
error: expect(received).toBe(expected)

Expected: 1
Received: 0

      at <anonymous> (/workspace/bun/test/js/web/crypto/web-crypto.test.ts:20:41)
(fail) getRandomValues and randomUUID have the lengths of the IDL [4.91ms]
(pass) Web Crypto > keeps event loop alive [25.93ms]
(pass) Web Crypto > has globals [0.22ms]
(pass) Web Crypto > should encrypt and decrypt [12.26ms]
(pass) Web Crypto > should verify and sign [4.03ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are not valid JSON [1.80ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are valid JSON but not a valid JWK [2.36ms]
(pass) Web Crypto > unwrapKey JWK error handling > settles when JsonWebKey dictiona
... (truncated)
passes on PR (with fix)
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/web/crypto/web-crypto.test.ts
bun test v1.4.4 (367d939d9)

test/js/web/crypto/web-crypto.test.ts:
(pass) crypto.subtle setter should not throw [2.56ms]
(pass) getRandomValues and randomUUID have the lengths of the IDL [0.97ms]
(pass) Web Crypto > keeps event loop alive [356.22ms]
(pass) Web Crypto > has globals [3.08ms]
(pass) Web Crypto > should encrypt and decrypt [13.13ms]
(pass) Web Crypto > should verify and sign [24.29ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are not valid JSON [12.21ms]
(pass) Web Crypto > unwrapKey JWK error handling > rejects when wrapped bytes are valid JSON but not a valid JWK [9.15ms]
(pass) Web Crypto > unwrapKey JWK error handling > settles when JsonWebKey dictionary conversion itself throws [9.91ms]
(pass) Web Crypto > unwrapKey JWK error handling > does not leak DeferredPromise in m_pendingPromises on JWK parse errors [2103.41ms]
(pass) oversized inputs > rejects >2 GiB inputs instead of aborting [292.43ms]
(pass) oversized outputs > rejects an output over 
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 2246ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/351] gen cpp.rs (cppbind)
[2/351] gen the table of built-ins
builtins table: operands without uses: none
builtins table: 2101 host entries have a result that is not described; a program in which a value may be anything holds what any host class needs: 0 modules, 15 functions
[3/293] cxx obj/unified/UnifiedSource-src_jsc_modules-0.cpp.o
[4/293] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-1.cpp.o
[5/293] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_crypto-1.cpp.o
[6/293] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-2.cpp.o
[7/293] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-0.cpp.o
[8/293] cxx obj/src/jsc/bindings/image_wic_shim.cpp.o
[9/293] cxx obj/src/jsc/bindings/image_coregraphics_shim.cpp.o
[10/293] cxx obj/src/jsc/bindings/MimallocOverrideDarwin.cpp.o
[11/293] cxx obj/src/jsc/bindings/highway_png.cpp.o
[12/293] cxx obj/src/jsc/bindings/highway_json.cpp.o
[13/293] cxx obj/src/jsc/bindings/JSBuffer.cpp.o
[14/293] cxx obj/src/jsc/bindings/BunSignalActions.
... (truncated)
diff hotspot
bench/snippets/webcrypto-keys.mjs                  |  84 ++++++++++++
 .../bindings/webcrypto/CryptoAlgorithmAKPShared.h  |   2 +-
 .../webcrypto/CryptoAlgorithmChaCha20Poly1305.cpp  |   4 +-
 src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyAES.cpp        |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyAKP.cpp        |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyEC.cpp         |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyHMAC.cpp       |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyOKP.cpp        |   2 +-
 src/jsc/bindings/webcrypto/CryptoKeyRSA.cpp        |   2 +-
 src/jsc/bindings/webcrypto/JSCryptoKeyUsage.cpp    |   7 +-
 src/jsc/bindings/webcrypto/JSCryptoKeyUsage.h      |   1 +
 src/jsc/bindings/webcrypto/JSJsonWebKey.cpp        |   8 +-
 .../bindings/webcrypto/JSRsaOtherPrimesInfo.cpp    |   9 --
 src/jsc/bindings/webcrypto/JsonWebKey.h            |  29 +++--
 src/jsc/bindings/webcrypto/JsonWebKey.idl          |   2 +-
 src/jsc/bindings/webcrypto/RsaOtherPrimesInfo.idl  |   6 +-
 src/jsc/bindings/webcrypto/SubtleCrypto.cpp        |  38 +++++-
 .../parallel/test-webcrypto-key-ops-duplicates.js  |  20 +++
 test/js/web/crypto/web-crypto.test.ts              | 145 +++++++++++++++++++--
 20 files changed, 312 insertions(+), 57 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file
file                                                      reads  edits  tests
bench/snippets/webcrypto-keys.mjs                             0      0     15
src/jsc/bindings/webcrypto/CryptoAlgorithmAKPShared.h         0      0     15
…/bindings/webcrypto/CryptoAlgorithmChaCha20Poly1305.cpp      0      0     15
src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp            0      0     15
src/jsc/bindings/webcrypto/CryptoKeyAES.cpp                   1      0     15
src/jsc/bindings/webcrypto/CryptoKeyAKP.cpp                   0      0     15
src/jsc/bindings/webcrypto/CryptoKeyEC.cpp                    1      0     15
src/jsc/bindings/webcrypto/CryptoKeyHMAC.cpp                  0      0     15
src/jsc/bindings/webcrypto/CryptoKeyOKP.cpp                   1      0     15
src/jsc/bindings/webcrypto/CryptoKeyRSA.cpp                   1      0     15
src/jsc/bindings/webcrypto/JSCryptoKeyUsage.cpp               1      1     15
src/jsc/bindings/webcrypto/JSCryptoKeyUsage.h                 1      1     15
src/jsc/bindings/webcrypto/JSJsonWebKey.cpp                   1      1     16
src/jsc/bindings/webcrypto/JSRsaOtherPrimesInfo.cpp           0      0     15
src/jsc/bindings/webcrypto/JsonWebKey.h                       3      5     15
src/jsc/bindings/webcrypto/JsonWebKey.idl                     0      0     15
(+ 4 more files)

…erywhere

JsonWebKey.key_ops is sequence<DOMString> in the WebCrypto IDL. RFC 7517
section 4.3 allows key_ops values that are not WebCrypto usages, and the
jwk import steps only require key_ops to contain the requested usages.
The dictionary converted the member as sequence<KeyUsage>, so a JWK with
a foreign or non-string key_ops entry failed with a TypeError before the
import algorithm ran. Node and Chromium import such keys.

key_ops is now a Vector<String>. normalizeJsonWebKey builds the usages
bitmap from the entries that name a KeyUsage and skips the rest.
hasDuplicateJwkKeyOps compares the strings, so it also catches repeated
unknown values, and the AES, RSA, EC and OKP importers now call it too.
Before, only HMAC, ChaCha20-Poly1305 and AKP rejected duplicates.
Add CryptoAlgorithm::validateJwkKeyOps, which rejects duplicate key_ops
entries and key_ops that do not cover the requested usages with Node's
"Duplicate key operation" and "Key operations and usage mismatch"
DataError messages. Every jwk import branch calls it after the use check.
HMAC, ChaCha20-Poly1305 and AKP already reported these messages inline;
AES, RSA, EC and OKP reported the generic DataError. Upstream Node tests
assert the messages for EC, RSA and CFRG keys.
@robobun

robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:32 AM PT - Nov 1st, 2026

❌ @robobun, your commit fd64a85 has 41 failures in Build #136478 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 42102

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

bun-42102 --bun

@robobun

robobun commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduction (main 3f3a9ff, compare with node):

const S = crypto.subtle;
const jwk = { kty: "oct", k: "AAECAwQFBgcICQoLDA0ODw" };
const run = members =>
  S.importKey("jwk", { ...jwk, ...members }, "AES-GCM", true, ["encrypt"]).then(
    k => "imported " + JSON.stringify(k.usages),
    e => e.name + ": " + e.message,
  );
for (const key_ops of [["encrypt","fly"], ["encrypt","deriveBits","x-custom"], [1,"encrypt"], ["encrypt",null], ["fly"], ["encrypt","fly","fly"], ["encrypt","encrypt"]]) {
  console.log(JSON.stringify(key_ops).padEnd(36), await run({ key_ops }));
}
console.log("oth: [{}]".padEnd(36), await run({ oth: [{}] }));
input main 3f3a9ff this branch Node v26.12.1
["encrypt","fly"], ["encrypt","deriveBits","x-custom"] TypeError: value must be enumeration (string) imported ["encrypt"] imported ["encrypt"]
[1,"encrypt"], ["encrypt",null] TypeError: value must be a string imported ["encrypt"] imported ["encrypt"]
["fly"] TypeError: value must be enumeration (string) DataError DataError: Key operations and usage mismatch
["encrypt","fly","fly"] TypeError: value must be enumeration (string) DataError DataError: Duplicate key operation
["encrypt","encrypt"] DataError (since #47559) DataError DataError: Duplicate key operation
oth: [{}] TypeError: Member RsaOtherPrimesInfo.d is required and must be an instance of DOMString imported ["encrypt"] imported ["encrypt"]

The DataError of Bun has the message Data provided to an operation does not meet requirements in each row. This PR does not change message text.

CI. The red tests on this PR are in the AOT suite: 40 files under test/aot/ and test/bundler/compile-aot-size.test.ts (build #136478 is finished: 124 jobs passed and 64 failed). This PR does not touch them. 40 of the 41 are also red in the final builds of merged PRs: #47615 (136388), #47601 (136323) and #47596 (136295). The other one, test/aot/three-layers-agree.test.ts, also fails in some runs of other branches, where it passes on a retry (builds 136506, 136494 and 136492). No test outside that suite is red on Linux, Windows or macOS. I read the logs of all 64 failed jobs to check that. In CI, test/js/web/crypto/web-crypto.test.ts passes with 117 tests (macOS x64 is one of the lanes where I read its output). On a debug build of fd64a85, test/js/web/crypto/ passes (134 tests), and the 40 test-webcrypto-* files and test-crypto-key-objects-to-crypto-key.js exit 0.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between d350e24 and fd64a85.

Files selected for processing: 1
  • src/jsc/bindings/webcrypto/JsonWebKey.h

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

WebCrypto JWK key_ops now uses string sequences, preserves unknown entries during normalization, detects repeated strings, and converts recognized usages for export. RSA other-prime dictionary fields are optional. Tests and benchmarks cover these changes.

Changes

JWK key operation handling

Layer / File(s) Summary
JWK representation and normalization
src/jsc/bindings/webcrypto/JsonWebKey.idl, src/jsc/bindings/webcrypto/JSJsonWebKey.cpp, src/jsc/bindings/webcrypto/JsonWebKey.h, src/jsc/bindings/webcrypto/JSCryptoKeyUsage.*, src/jsc/bindings/webcrypto/SubtleCrypto.cpp
key_ops uses strings instead of enum values. Normalization retains the entries and sets usage bits for recognized values. Duplicate detection checks repeated strings.
Algorithm import checks and export serialization
src/jsc/bindings/webcrypto/CryptoAlgorithmAKPShared.h, src/jsc/bindings/webcrypto/CryptoAlgorithmChaCha20Poly1305.cpp, src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp, src/jsc/bindings/webcrypto/CryptoKey*.cpp, src/jsc/bindings/webcrypto/SubtleCrypto.cpp
Algorithm imports pass the full JWK to duplicate detection. Key exports convert usage bitmaps to JWK operation strings. Import and unwrap paths move converted JWK data.
JWK behavior tests and benchmarks
test/js/web/crypto/web-crypto.test.ts, test/js/node/test/parallel/test-webcrypto-key-ops-duplicates.js, bench/snippets/webcrypto-keys.mjs
Tests cover unknown and duplicate operations, conversion, import, unwrap, and export behavior across key types. The benchmark snippet covers key import, unwrap, export, and generation.

RSA other-prime dictionary conversion

Layer / File(s) Summary
Optional RSA other-prime fields
src/jsc/bindings/webcrypto/RsaOtherPrimesInfo.idl, src/jsc/bindings/webcrypto/JSRsaOtherPrimesInfo.cpp, test/js/web/crypto/web-crypto.test.ts
The r, d, and t fields are no longer required. Tests cover missing fields during AES and RSA imports.

Suggested reviewers: cirospaciari, dylan-conway

Merge Risk: Minimal · up to fd64a

No concrete merge-blocking issue is established for the JWK conversion changes. Merge readiness remains subject to normal build and test checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main changes: converting JWK key_ops and oth to match Web Crypto IDL types.
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.
Description check Passed The description is thorough and follows the repository template. It has a 'What does this PR do?' equivalent (Problem, Fix, Background, Downsides sections) and a 'How did you verify your code works?' …
Full details: Description check

Explanation

The description is thorough and follows the repository template. It has a 'What does this PR do?' equivalent (Problem, Fix, Background, Downsides sections) and a 'How did you verify your code works?' equivalent (the Verified bullets, test results, the fail-on-main and pass-on-PR gate evidence, and the Node test file). It states the behavior change clearly: key_ops entries that are not KeyUsage values are now accepted when requested usages are present, and oth entries convert without r, d, or t. It also covers the trade-offs, including the export cost, the intentional divergence from WebKit, and the remaining unwrapKey differences. The main weakness is length and density. Much of the detail sits in a collapsed Notes section, so a reviewer must read a lot to find the core rationale. This does not make the description incomplete.

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@robobun

robobun commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the automated review reported no actionable findings, so there is nothing to change from it. The claude-find-issues check failed inside its own action after 0.5 s (is_error: true before it read the diff), which is unrelated to this change. Buildkite #113400 is running. I will follow up on its result.

@robobun

robobun commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the edited automated review still has no actionable findings. The fail-before/pass-after gate passed on ASAN and release builds. Buildkite #113400 is at 170 of 181 jobs passed with none failed so far.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it retypes a WebCrypto IDL member and touches key-import validation across every algorithm — deliberately diverging from upstream WebKit — a human look would still be worthwhile.

What was reviewed:

  • Grepped case CryptoKeyFormat::Jwk — all 15 JWK import branches now call validateJwkKeyOps; all 7 exportJwk() sites use toJwkKeyOps(), none missed.
  • HashSet<String> in hasDuplicateJwkKeyOps: IDLDOMString conversion never yields a null WTF::String, and empty "" is a valid entry (WTF uses null/HashTableDeletedValue as the sentinel slots), so key_ops: ["", ""] is handled without tripping a hash-table assertion.
  • The two updated leak-regression tests: key_ops: "sign" still throws inside convert<IDLSequence<...>> in convertDictionary<JsonWebKey>, so the same DeferredPromise conversion-exception branch is exercised.
  • parseEnumerationFromString<CryptoKeyUsage> follows the existing BufferEncodingType specialization pattern; the base template is already declared in JSDOMConvertEnumeration.h.
Extended reasoning...

Overview

This PR retypes JsonWebKey.key_ops from sequence<CryptoKeyUsage> to sequence<DOMString> so JWKs carrying JOSE-defined or vendor key_ops values (permitted by RFC 7517 §4.3 and by the actual W3C WebCrypto IDL) no longer TypeError during dictionary conversion. It extracts a shared static CryptoAlgorithm::validateJwkKeyOps() (duplicate check via HashSet<String>, then usages-cover check, with Node's exact DataError messages and order) and wires it into every algorithm's JWK import branch — the AES family, ChaCha20-Poly1305, HMAC, ECDH/ECDSA, Ed25519/X25519, RSA-OAEP/PSS/RSASSA, and AKP. normalizeJsonWebKey() now builds the usages bitmap by parsing each string entry and dropping unrecognized ones. All exportJwk() sites convert back through a new toJwkKeyOps() helper. Three new tests cover the full behavior matrix; two existing DeferredPromise-leak regression tests swap their trigger from ["bogus"] to a non-sequence value since the old input no longer throws at the conversion layer.

Security risks

WebCrypto key import is security-relevant surface. The change relaxes validation in one direction (foreign key_ops entries are now tolerated) and tightens it in another (duplicate entries now reject for every algorithm, not just HMAC/ChaCha/AKP). The relaxation is safe: key_ops is advisory metadata about intended operations, and the actual capability restriction on the imported CryptoKey comes from the caller's usages argument, which the new helper still enforces ((jwk.usages & usages) != usages → DataError). No cryptographic primitive, key material handling, or capability grant changed. The HashSet<String> duplicate scan is not exposed to hash-table sentinel hazards because IDLDOMString conversion always produces a non-null WTF::String and empty strings are valid entries.

Level of scrutiny

Medium-high. The mechanical fan-out (15 import branches, 7 export sites) is verified complete by grep, and the shared helper is placed at the right layer (CryptoAlgorithm, the base every importer already inherits from). The tests are thorough — one combined .toEqual per case, exact error class + message asserted, both known and unknown duplicate values covered per algorithm, and the unwrapKey path exercised. However, the change deliberately diverges from upstream WebKit's IDL, picks Chromium's reading over Node's on one edge (duplicate unknown values reject), and for the AES family places the key_ops check ahead of the kty/use/alg checks that live inside CryptoKeyAES::importJwk — a documented but real error-precedence difference from Node. Those are design calls a maintainer should sign off on rather than an automated review.

Other factors

No CODEOWNERS entry covers src/jsc/bindings/webcrypto/. The PR description notes the vendored Node parallel tests don't yet carry upstream's new EC/RSA/CFRG key_ops assertions, and that a wider refactor folding use/ext into the shared helper was considered and deferred — both reasonable scoping decisions worth a human acknowledging. The two edited pre-existing tests still exercise the same convertDictionary<JsonWebKey> throw path (sequence conversion of a non-iterable), so the leak guard they protect is intact.

@robobun

robobun commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

The review found no bugs and leaves three design calls for a maintainer. For whoever picks this up, here is where each one stands:

  1. key_ops as sequence<DOMString> instead of WebKit's sequence<CryptoKeyUsage>: this is the W3C IDL, and Node, Chromium and Firefox all read it that way. WebKit is the outlier and has no pending fix. I do not see a reading under which the old behavior is correct.
  2. A repeated unknown value (["encrypt","fly","fly"]) rejects, as in Chromium and RFC 7517 section 4.3. Node skips unknown values in its duplicate scan and imports. If Node parity is preferred here, it is a one-line change in hasDuplicateJwkKeyOps (scan only the recognized values) plus one test row.
  3. For the AES family the key_ops check runs in the algorithm's jwk branch, before the kty/use/alg checks that still live in CryptoKeyAES::importJwk. A JWK that is wrong in two ways gets the key_ops message where Node reports kty or use first. Both are DataError. Matching Node's order for AES means hoisting those member checks into the branch (as HMAC and ChaCha20-Poly1305 already do), which I left out to keep this change about key_ops. I can add it here if wanted.

Buildkite #113400 is at 180 of 181 jobs passed.

main (bc55d56) rejects a repeated key_ops entry for AES, RSA, EC and
OKP keys in CryptoKey*::importJwk, through jwkKeyOpsAllow. This branch
did the same in the algorithm branches, with validateJwkKeyOps. The
merge takes main's tree as it is. The next commit adds the part that
main does not have: key_ops entries that are not a KeyUsage.
JsonWebKey.key_ops is sequence<DOMString> in the WebCrypto IDL, and
RFC 7517 section 4.3 lets a JWK name operations that WebCrypto does not
define. The dictionary conversion read it as sequence<KeyUsage>, so
importKey and unwrapKey threw a TypeError for such a key.

key_ops is now a Vector<String> through JsonWebKey. normalizeJsonWebKey
builds the usages bitmap from the entries that name a KeyUsage. The
checks that main has (jwkKeyOpsAllow and the three inline ones) see the
strings, so a repeated entry is a DataError whether it names a KeyUsage
or not, as in Node v26.12.1.

A key_ops without a foreign entry costs one popcount compare in the
duplicate check. exportKey builds its key_ops from the usages bitmap.
The three copies of the JsonWebKey on import are now moves.
parseEnumerationFromString<CryptoKeyUsage> has two callers. Inlined into
both, the scan of the 12-entry table became an out-of-line
std::__find_if in release builds, and every conversion of a KeyUsage
paid a call for it. NEVER_INLINE keeps the scan folded into the one
function, as it is on main.
RsaOtherPrimesInfo has no required member in the Web Crypto IDL. The
converter threw a TypeError for an entry without r, d or t, for every
key type. Now the entry converts. An RSA import rejects every oth with
a DataError, and the other key types do not read it.
The row for each key type now exports its key, so each of the seven
places that build key_ops from the usages has a row. The
ChaCha20-Poly1305 row asks for its four usages.

bench: add rows for importKey, unwrapKey, exportKey and generateKey.
bench/ had none for a key operation.
Comment thread src/jsc/bindings/webcrypto/JsonWebKey.h Resolved
Comment thread src/jsc/bindings/webcrypto/JsonWebKey.h Resolved
@robobun robobun changed the title webcrypto: read JWK key_ops as plain strings, reject duplicates for every key class webcrypto: convert JWK key_ops and oth as the Web Crypto IDL types them Nov 1, 2026
@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

This branch now has main (3f3a9ff) merged, and the PR is smaller: 17 source files, +73/-48. It was 31 files.

What main covers. #47559 rejects a repeated key_ops entry for AES, RSA, EC and OKP keys (jwkKeyOpsAllow). This PR did that with validateJwkKeyOps and with Node's message text. Both are gone from here.

What main still lacks. A key_ops entry that is not a KeyUsage. On main, importKey("jwk", { kty: "oct", k, key_ops: ["encrypt", "fly"] }, "AES-GCM", true, ["encrypt"]) is still TypeError: value must be enumeration (string), for every key type and for unwrapKey("jwk"). Node v26.12.1 imports the key.

What this push has.

  • JsonWebKey.key_ops is a Vector<String>, as in the Web Crypto IDL. The checks of main see the strings. So a repeated entry is a DataError whether or not it names a KeyUsage, which Node's test-webcrypto-key-ops-duplicates.js asserts. That file is added as Node has it.
  • oth has the same cause and is in: an entry converts without r, d or t. This needs node:crypto, webcrypto: a third batch of Node compatibility fixes #47591, which rejects every oth for an RSA key.
  • A review of the first version found that it made each conversion of a KeyUsage slower in release builds, also for calls that have no JWK. That is repaired. The body has the instruction counts for main and for this branch.

Against Node v26.12.1. 11 algorithms and 22 shapes of key_ops: importKey agrees on 242 of 242 cells (main: 110), and unwrapKey on 187 of 242 (main: 66). The 55 that are left are in the parse step of unwrapKey. The body lists them.

Two questions for a maintainer. @dylan-conway, this is next to your #47559 and #47591.

  1. The retype leaves WebKit on purpose: WebKit has sequence<CryptoKeyUsage>, and Safari rejects these keys. No user asked for the change. A comparison with Node found it. Is the retype wanted?
  2. exportKey("jwk") runs 50 to 70 more instructions for each usage after the first (7,148 to 7,209 with two usages), because it writes a vector of strings. importKey("jwk") runs 36 to 199 fewer. An export that pays nothing needs one member in JsonWebKey for each direction. I did not build that. Is the cost acceptable?

Not in this PR. importKey("raw", bytes, "AES-GCM", true, [{ toString() { return "encrypt" } }]) is a TypeError since Bun 1.3.0. Node and Bun 1.2.23 import the key. That is the keyUsages argument, and it has another cause (the sequence conversion since #23169). It is handed on as separate work.

Verified on a debug build of d350e24:

  • test/js/web/crypto/: 134 pass. The four new tests fail with the src/ of 3f3a9ff.
  • test/js/node/test/parallel/test-webcrypto-* (40 files) and test-crypto-key-objects-to-crypto-key.js: all exit 0. The new Node test exits 1 on main.
  • test/js/deno/crypto/webcrypto.test.ts, crypto.key-objects.test.ts, crypto-pqc.test.ts, sign-jwk-ieee-p1363.test.ts, common-strings.test.ts: 166 pass, 27 skip, 1 todo.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Beyond the inline finding, I also checked a few things in the C++ side and found nothing wrong: toJwkKeyOps iterates 1 << bit in CryptoKeyUsage declaration order, which matches both the CryptoKeyUsage* bit constants in CryptoKeyUsage.h and the order the old CryptoKey::usages() produced, so exported key_ops are unchanged; the size() == popcount(usages) fast path in hasDuplicateJwkKeyOps is sound because normalizeJsonWebKey only sets a bit for entries that parse as a KeyUsage, so equality implies no repeats and no foreign entries; the three new WTF::moves are on an rvalue variant and locals that are not read afterwards; and dropping required from RsaOtherPrimesInfo cannot change RSA import behavior since CryptoKeyRSA::importJwk rejects any oth before looking at its members.

Extended reasoning...

The change rewires JsonWebKey.key_ops from an enum sequence to DOMString in the WebCrypto bindings, adds string-based duplicate detection, and makes every exporter derive key_ops from the usages bitmap; it touches crypto import/export paths but no auth or data-exposure surface. One inline finding (a Bun-authored test placed under the vendored Node test directory) is already posted; the ruled-out items above are the enum/bitmap ordering, the popcount fast path, the move sites, and the RsaOtherPrimesInfo IDL relaxation.

hasDuplicateJwkKeyOps had a comment that explained why equal counts mean
no repeated entry. The two values now have names that say it, and the
comment is gone. key_ops and usages each have a one-line comment in
place of the shared paragraph. No behavior change.
@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-up, in fd64a85:

  • The two multi-line comments in JsonWebKey.h are gone. key_ops and usages each have one line. The duplicate check names its two values (distinctKeyUsageCount, everyEntryIsADistinctKeyUsage), so it needs no comment. The behavior does not change.
  • The finding about test-webcrypto-key-ops-duplicates.js does not hold. The file is a Node.js test, copied without a change: nodejs/node at v26.12.1. The git blob is 4a666a1caa476b4bc244723ee3dba9c7f00feb01 in nodejs/node and in this PR. It stays in test/js/node/test/parallel/.
  • The other automated reviews have no actionable finding.

On a debug build of this commit, test/js/web/crypto/ passes (134 tests), and the 40 test-webcrypto-* files and test-crypto-key-objects-to-crypto-key.js exit 0.

@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

The automated review of fd64a85 has no actionable finding, so there is nothing to change. All three review threads have a reply and are resolved.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

The second automated review of fd64a85 also found no issue. No review thread is open, and no change is pending from a review.

@robobun

robobun commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

The edited automated summary for fd64a85 still has no actionable finding. The CI build for that commit is almost complete (183 of 188 jobs), and its red tests are only AOT-suite files. There are 41 of them (an earlier version of this comment said 29, from a truncated list). The first comment has the details. 40 of them are also red on merged PRs, the other one fails in some runs of other branches, and this PR touches none of them.

@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

The edited automated summary still has no actionable finding. Its one remark is about the length of the description: the four visible sections hold what decides the merge, and the collapsed Notes hold the measurements and the test lists, so the Notes are optional reading.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants