webcrypto: reject importKey("jwk") with a missing or wrong kty with DataError - #32901
webcrypto: reject importKey("jwk") with a missing or wrong kty with DataError#32901robobun wants to merge 4 commits into
Conversation
The W3C WebCrypto spec defines JsonWebKey.kty as an optional dictionary member (not `required`). Each algorithm's Import Key operation is then responsible for checking 'If the kty field of jwk is not <expected>, then throw a DataError'. Bun inherited WebKit's IDL which marks kty as `required`, so the WebIDL dictionary conversion threw a TypeError before the per-algorithm kty check ever ran. Code distinguishing DataError (bad key material) from TypeError (programmer error) took the wrong branch. Drop the required-member TypeError so a missing kty falls through to the existing per-algorithm kty check, which already surfaces DataError for every algorithm (AES, HMAC, RSA, EC, OKP).
|
Warning Review limit reached
More reviews will be available in 49 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 12:42 AM PT - Jun 28th, 2026
❌ @robobun, your commit 7b24a67 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32901That installs a local version of the PR into your bun-32901 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
CryptoKeyOKP::importJwkInternal only compared keyData.kty against "OKP"
in the Ed25519 arm; the X25519 arm checked crv only. With kty no longer
required at the WebIDL layer, an X25519 JWK with a missing (or wrong)
kty would import successfully instead of rejecting with DataError.
Hoist the kty check above the curve switch so it covers both curves,
and drop required from kty in JsonWebKey.idl to keep the IDL in sync
with the hand-edited converter.
The unwrapKey tests that used {foo:"bar"} to exercise the dictionary
conversion error path no longer do (that object converts cleanly now),
so switch those fixtures to an invalid key_ops enum value and keep the
{foo:"bar"} case as coverage for the kty-less DataError path.
robobun
changed the title
|
The review caught a real gap: af209fb addresses all three review comments:
Existing OKP JWK coverage still passes: |
There was a problem hiding this comment.
Thanks — all three of my earlier points are addressed (hoisted OKP kty check + X25519 test rows, .idl synced, leak/regression fixtures swapped to key_ops:["bogus"]). I didn't find anything else; deferring to a human only because this loosens the WebIDL-level guard on a crypto key-import path.
Extended reasoning...
Overview
This PR changes crypto.subtle.importKey("jwk", ...) to reject a missing kty with DataError (a DOMException) instead of TypeError, matching the W3C spec, Node.js, and Chrome. It does so by removing the throwRequiredMemberTypeError branch for kty in the generated JsonWebKey dictionary converter (JSJsonWebKey.cpp), dropping required from JsonWebKey.idl, and — to close the gap that removal exposed — hoisting if (keyData.kty != "OKP"_s) return nullptr; above the curve switch in CryptoKeyOKP::importJwkInternal so X25519 now validates kty too. Tests add an it.each over six algorithms (including X25519) for missing kty, a wrong-kty X25519 case, and rework the adjacent unwrapKey regression tests so the dictionary-conversion-throws / m_pendingPromises leak path is still exercised via an invalid key_ops enum value.
Prior feedback resolved
In my earlier review I flagged (🔴) that removing the dictionary-level guard let an X25519 JWK with no kty import successfully, (🟡) that the .idl was now out of sync, and (🟡) that the {foo:"bar"} leak fixture no longer reached the scope.exception() branch it was guarding. All three are now addressed exactly as suggested, and the bug-hunting pass on the updated diff found nothing new. I re-checked that every CryptoKey*::importJwk (AES, HMAC, RSA, EC, OKP) has a kty != "<expected>" comparison, so no other algorithm relied on the removed WebIDL guard.
Security risks
This is in the WebCrypto key-import path, so it is security-adjacent. The net effect is strictly tightening or error-type-only: every input that was previously rejected is still rejected (just with DataError instead of TypeError), and X25519 now additionally rejects wrong/missing kty where it previously accepted a wrong kty. No cryptographic operation, key material handling, or accept-set is widened. I don't see an exploit vector here.
Level of scrutiny
Medium. The diff is small (~5 lines of production code plus tests) and mechanically follows the spec text, but it removes a blanket input-validation guard in favor of per-algorithm checks, which is the kind of change where a missed branch silently accepts malformed input. I already caught one such branch (X25519) in the first round and it's now fixed and tested; a second pair of eyes on the per-algorithm coverage seems prudent given the area.
Other factors
The one CI failure (test/js/node/tls/node-tls-connect.test.ts) is unrelated to this change. No CODEOWNERS entry covers src/jsc/bindings/webcrypto/. Test coverage for the new behavior is thorough and includes a Node/Chrome-parity assertion in the PR description.
There was a problem hiding this comment.
LGTM — all three review points from the earlier pass are addressed in af209fb, and every JWK import path now has its own kty check.
Extended reasoning...
Overview
Four files: drop the required-member TypeError for kty in JSJsonWebKey.cpp (3 lines), hoist the keyData.kty != "OKP" check above the curve switch in CryptoKeyOKP.cpp so it covers X25519 as well as Ed25519 (5 lines moved), drop required from JsonWebKey.idl (1 word), and add/adjust tests in web-crypto.test.ts. The net behavior change is (a) missing kty now rejects with DataError instead of TypeError — matching the W3C spec, Node.js, and Chrome — and (b) X25519 JWK import with a wrong kty (e.g. "EC") now rejects instead of being silently accepted, which fixes a pre-existing validation gap.
Security risks
This is WebCrypto input validation, not key material or cipher logic. The change is net-tightening: a previously-accepted malformed X25519 JWK is now rejected, and the missing-kty case still rejects (just with a different, spec-correct error type). I re-grepped every importJwk path (CryptoKeyAES, CryptoKeyHMAC, CryptoKeyRSA, CryptoKeyEC, CryptoKeyOKP) and each now compares keyData.kty against its expected literal, so removing the dictionary-level guard leaves no algorithm uncovered. No new attack surface.
Level of scrutiny
Moderate — crypto-adjacent but a small, mechanical, spec-aligned validation change. The production diff is ~10 lines and the logic is a straightforward string comparison whose failure path (return nullptr → exceptionCallback(DataError)) is the existing, well-exercised one for every other invalid JWK field.
Other factors
My earlier review flagged the X25519 gap, the IDL drift, and the lost scope.exception() regression coverage; af209fb addresses all three exactly as suggested, the inline comments are resolved, and the bug-hunting re-run on the current revision found nothing. Test coverage is solid: an it.each over six algorithms for missing kty, a dedicated wrong-kty X25519 case that fails on current Bun, and the unwrapKey leak regression test was migrated to a fixture (key_ops: ["bogus"]) that still throws inside the dictionary converter. The author also reports the existing OKP/WebCrypto suites (Deno, Node parallel tests, x25519-derive-bits) still pass.
|
Status for a maintainer: the change is complete and reviewed; the red CI is infrastructure, not this diff. Across all three Buildkite runs of this PR (65692, 65751, 66043), no crypto test ever failed, and For context: none of the 15 most recently finished builds on this pipeline passed, on any branch, each failing on a different set of external-service or infrastructure tests. I am not going to keep pushing empty commits to re-run CI. Locally, |
|
Closing: both parts of this change landed separately.
Verified on current main (bdb7382): |
1 participant
What does this PR do?
crypto.subtle.importKey("jwk", ...)with a JWK object missing thektymember threwTypeErrorinstead ofDataError.TypeErrorDataErrorThe W3C WebCrypto spec defines
JsonWebKey.ktyas an optionalDOMString. Each algorithm's Import Key operation is then responsible for "If thektyfield of jwk is not<expected>, then throw aDataError."Bun inherited WebKit's IDL which marks
ktyasrequired, so the generated WebIDL dictionary conversion threwTypeError: Member JsonWebKey.kty is required and must be an instance of DOMStringbefore the per-algorithm check ever ran.DataErroris aDOMExceptionandTypeErroris a JSError, so code that distinguishes "bad key material" (DataError: reject the key, try the next one in the JWKS) from "programmer error" (TypeError: crash) takes the wrong branch on every malformed JWK from an untrusted source.Two changes:
JSJsonWebKey.cpp: drop the required-memberTypeErrorforkty. A missingktyleavesresult.ktyas a nullString, which fails each algorithm'skeyData.kty != "<expected>"check and surfacesDataErrorvia the normalexceptionCallback(DataError, ...)path.JsonWebKey.idllosesrequiredto stay in sync.CryptoKeyOKP.cpp: hoist thekeyData.kty != "OKP"check above the curve switch inimportJwkInternal. It previously lived only in theEd25519arm; theX25519arm checked onlycrv, so with the WebIDL guard gone an X25519 JWK with a missingktywould have imported successfully instead of rejecting. This also fixes the pre-existing case where a wrongkty(for example"EC") was silently accepted for X25519.How did you verify your code works?
In
test/js/web/crypto/web-crypto.test.ts:it.eachover AES-GCM, HMAC, RSA-OAEP, ECDSA, Ed25519, X25519: missingktyrejects withDataError. The OKP rows use a well-formed 32-bytexso the missingktyis the only reason the import is rejected.wrong kty: X25519:{ kty: "EC", crv: "X25519", x }rejects withDataError. On current Bun this imports successfully, so this test fails without theCryptoKeyOKP.cppchange on its own.unwrapKeytests that used{foo:"bar"}to exercise the JWK dictionary-conversion exception path no longer do (that object converts cleanly now thatktyis optional), so those fixtures switch to an invalidkey_opsenum value. The{foo:"bar"}case is kept as coverage for the kty-lessDataErrorpath.No regressions in the existing OKP JWK import coverage:
test/js/deno/crypto/webcrypto.test.ts,test/js/bun/crypto/x25519-derive-bits.test.ts, and Node'stest-webcrypto-derivebits-cfrg.js,test-webcrypto-derivekey-cfrg.js,test-webcrypto-sign-verify.js, andtest-webcrypto-wrap-unwrap.jsall pass.