webcrypto: reject JWK with duplicate key_ops on importKey/unwrapKey - #33902
webcrypto: reject JWK with duplicate key_ops on importKey/unwrapKey#33902robobun wants to merge 3 commits into
Conversation
RFC 7517 section 4.3 requires "Duplicate key operation values MUST NOT be present in the array", and WebCrypto importKey specifies that a key_ops member invalid per RFC 7517 is a DataError. normalizeJsonWebKey folded key_ops straight into a bitmap, which made duplicates invisible to the per-key-class importers that only compare bitmasks. normalizeJsonWebKey now detects a bit that is already set while building the bitmap and reports failure; both call sites (importKey and the JWK branch of unwrapKey) reject the promise with DataError on that result. The usages argument to importKey itself is unchanged and still permits duplicates, matching the spec and Node.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesJWK key_ops validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:27 AM PT - Jul 10th, 2026
❌ @robobun, your commit e9e7fbf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33902That installs a local version of the PR into your bun-33902 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small spec-compliance fix that mirrors the adjacent JWK error paths, with thorough test coverage.
Extended reasoning...
Overview
This PR tightens JWK validation in SubtleCrypto.cpp: normalizeJsonWebKey now returns false when key_ops contains duplicate entries, and both call sites (toKeyData for importKey, and the JWK branch inside the unwrapKey decrypt callback) reject with DataError on that result. Tests in test/js/web/crypto/web-crypto.test.ts cover all five key classes, the unwrapKey path, non-duplicate control cases, and confirm the usages argument itself still permits duplicates.
Security risks
None introduced. This is a fail-closed tightening of input validation per RFC 7517 §4.3 and the WebCrypto spec, aligning Bun with Node.js and Chrome. It rejects previously-accepted malformed JWKs rather than accepting anything new. No changes to key material handling, TLS, or crypto primitives.
Level of scrutiny
Low-to-medium. The C++ change is ~15 lines and mechanically follows established patterns: the new unwrapKey error branch is byte-for-byte identical in shape to the two adjacent error branches (!jwkObject and scope.exception()) — same m_pendingPromises.remove(index) + promise->reject(...) + return sequence. The toKeyData change reuses the existing std::nullopt-means-rejected contract that the importKey caller already handles. The bitmap loop is trivially correct since CryptoKeyUsage is a closed WebIDL enum and toCryptoKeyUsageBitmap(usage) maps each value to exactly one bit.
Other factors
The change addresses an existing "Maybe we shouldn't silently bypass duplicated usages?" comment at exactly this location. Test coverage is comprehensive with positive and negative controls, and the PR description shows the tests fail on system Bun and pass on the debug build. No CODEOWNERS match this path. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — small, spec-mandated validation tightening with thorough test coverage.
Extended reasoning...
Overview
This PR changes normalizeJsonWebKey in src/jsc/bindings/webcrypto/SubtleCrypto.cpp to detect duplicate entries in a JWK's key_ops array and return false, and updates its two call sites (the toKeyData JWK branch used by importKey, and the JWK branch inside the unwrapKey decrypt callback) to reject with DataError when that happens. It adds ~130 lines of tests to test/js/web/crypto/web-crypto.test.ts covering all five key classes, the unwrapKey path, non-duplicate controls, and that the usages argument still accepts duplicates.
Security risks
None. This is validation tightening only: previously-accepted malformed input (duplicate key_ops) is now rejected per RFC 7517 §4.3 and the WebCrypto spec, matching Node.js and Chrome. No new code paths accept input; no key material handling changes.
Level of scrutiny
Low-to-medium. While this touches WebCrypto, the change is mechanical and narrowly scoped: key_ops is already a Vector<CryptoKeyUsage> (enum values validated by WebIDL conversion), so each element maps to exactly one bit and result & bit is a correct duplicate check. Both new error paths mirror the adjacent error branches exactly — including the m_pendingPromises.remove(index) cleanup in unwrapKey that prevents the DeferredPromise leak covered by an existing test in the same file.
Other factors
The bug-hunting system found no issues. The PR description includes fails-on-main / passes-on-PR evidence for both debug+ASAN and release builds. Test coverage is comprehensive with positive and negative controls. No CODEOWNERS entry covers this path, and there are no outstanding human reviewer comments.
|
The diff is green; remaining CI failures are unrelated flake.
Neither has any code path in common with |
1 participant
What
crypto.subtle.importKey("jwk", ...)andcrypto.subtle.unwrapKey(..., "jwk", ...)accepted JWKs whosekey_opsarray contains duplicate entries. RFC 7517 section 4.3 states "Duplicate key operation values MUST NOT be present in the array", and the WebCrypto spec requires aDataErrorwhenkey_opsis invalid per RFC 7517. Node.js and Chrome both reject these.Reproduction
The same gap held for every key class (oct/AES, oct/HMAC, EC, OKP, RSA) and for the
unwrapKeyJWK path.Cause
normalizeJsonWebKeyinSubtleCrypto.cppcollapsedkey_opsdirectly into a usage bitmap viatoCryptoKeyUsageBitmap, so by the time the per-class importers (CryptoKeyAES,CryptoKeyEC,CryptoKeyHMAC,CryptoKeyOKP,CryptoKeyRSA) ran their(keyData.usages & usages) != usagescheck, duplicates were already merged into a single bit and structurally undetectable. The source carried a "Maybe we shouldn't silently bypass duplicated usages?" note at exactly this spot.Fix
normalizeJsonWebKeynow builds the bitmap itself and returnsfalsethe first time a bit repeats. Both call sites (thetoKeyDataJWK branch used byimportKey, and the JWK branch inside theunwrapKeydecrypt callback) reject the promise withDataErroron that result, matching the surrounding error handling for the adjacent JWK parse failures.The
usagesargument toimportKey/unwrapKeyitself is untouched and still permits duplicates, which matches the spec and Node.Verification
New tests in
test/js/web/crypto/web-crypto.test.tscoverimportKeyacross all five key classes, theunwrapKeypath, the non-duplicate control cases, and that theusagesargument still accepts duplicates.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file