webcrypto: reject HMAC importKey with length 0

[robobun]

Problem

crypto.subtle.importKey with HMAC and an explicit length: 0 in HmacImportParams returns a usable key whose algorithm.length is derived from the key data, instead of rejecting. The spec requires DataError when the length member is present and zero.

const k = await crypto.subtle.importKey(
  "raw", new Uint8Array(20),
  { name: "HMAC", hash: "SHA-256", length: 0 },
  true, ["sign"],
);
// before: resolves, k.algorithm.length === 160
// after:  rejects with DataError

Node v26.3.0, Deno, and browsers all reject with DataError here.

Cause

CryptoAlgorithmHMAC::importKey passes hmacParameters.length.value_or(0) to CryptoKeyHMAC::importRaw / importJwk, collapsing "length is absent" and "length is 0" into the same value. importRaw then treats 0 as "unspecified" via a falsy check and falls back to the key-data length.

Fix

Reject a present-and-zero length inside the raw and jwk branches of CryptoAlgorithmHMAC::importKey, after the format dispatch. That placement keeps the spec's error precedence: an unsupported format (pkcs8, spki) still rejects with NotSupportedError regardless of length.

The check has to live here rather than in CryptoKeyHMAC::importRaw: SerializedScriptValue.cpp calls importRaw(0, ...) for structured-clone deserialization, where 0 must keep meaning "unspecified".

The length: 1..159 cases already throw DataError, which is what the spec says; Node's NotSupportedError there is Node deviating, so those rows are left as-is.

Verification

$ bun bd test test/js/web/crypto/web-crypto.test.ts -t "HMAC importKey length"
(pass) Web Crypto > HMAC importKey length > rejects length:0 with DataError and accepts an absent length (raw)
(pass) Web Crypto > HMAC importKey length > rejects length:0 with DataError and accepts an absent length (jwk)
(pass) Web Crypto > HMAC importKey length > rejects the pkcs8 format with NotSupportedError regardless of length
(pass) Web Crypto > HMAC importKey length > rejects the spki format with NotSupportedError regardless of length

The two length:0 tests fail on main with zero: { length: 160 } instead of zero: { error: "DataError" }. The format-precedence tests pass on main and guard against a guard placed before the format switch.

[coderabbitai]

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 19 minutes and 43 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4bf258e2-33c3-41ff-b3d0-21d5ea5ee315

📥 Commits

Reviewing files that changed from the base of the PR and between 8706328 and 34b2fbc.

📒 Files selected for processing (2)

• src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp
• test/js/web/crypto/web-crypto.test.ts

---

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

[robobun]

^{Updated 10:11 PM PT - Jun 27th, 2026}

❌ @robobun, your commit 34b2fbc has some failures in Build #65821 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 32899

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

bun-32899 --bun

[claude]

LGTM — straightforward spec-compliance guard mirroring the existing generateKey pattern; the error-precedence nit for pkcs8/spki is take-it-or-leave-it.

Extended reasoning...

Overview

This PR adds a single 4-line early-return guard to CryptoAlgorithmHMAC::importKey in src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp, rejecting with DataError when HmacImportParams.length is present and zero. It also adds two regression tests in test/js/web/crypto/web-crypto.test.ts covering the raw and jwk formats, asserting that absent length still works, length:0 rejects with DataError, and adjacent non-zero lengths behave as before.

The guard is a near-verbatim copy of the existing length && !length.value() check in generateKey ~20 lines above (which throws OperationError per its own spec step), so it follows an established pattern in the same file.

Security risks

None. The change makes input validation stricter, turning a previously-accepted nonsensical parameter (length: 0) into a rejection. It does not relax any check, touch key material handling, or affect the sign/verify paths. No new data flows are introduced.

Level of scrutiny

Low. This is a small, mechanical spec-compliance fix in WebKit-derived WebCrypto bindings: one conditional plus an exception callback, with the exact std::optional present-and-zero idiom already used elsewhere in the function's sibling. The behavior aligns Bun with Node, Deno, and browsers per the PR description. Tests demonstrate the before/after delta cleanly and pin surrounding behavior (absent / 1 / 159 / 160) to guard against regressions.

Other factors

• The one finding from bug-hunting is a true nit: placing the guard before the switch(format) means pkcs8/spki + length:0 now yields DataError instead of NotSupportedError. That input is nonsensical (HMAC doesn't support those formats), the call fails either way, and no WPT covers it. Not a blocker.
• No CODEOWNERS entry covers src/jsc/bindings/webcrypto/.
• No prior human review comments to address; CodeRabbit was rate-limited and posted no findings.

[claude]

LGTM — the error-precedence nit is addressed (guard now lives inside the Raw/Jwk cases) and the new pkcs8/spki tests pin it.

Extended reasoning...

Overview

Adds an 8-line guard to CryptoAlgorithmHMAC::importKey so that an explicit length: 0 in HmacImportParams rejects with DataError for the raw and jwk formats, matching the WebCrypto spec and Node/Deno/browser behavior. Previously the .value_or(0) collapse made length: 0 indistinguishable from an absent length, so the call succeeded with a derived length. ~58 lines of tests cover absent/0/1/159/160 for raw, absent/0 for jwk, and pin NotSupportedError precedence for pkcs8/spki regardless of length.

Security risks

None. This strictly tightens input validation on a nonsensical parameter combination that every other engine already rejects. No crypto primitives, key material handling, or memory management are touched.

Level of scrutiny

Low. The C++ change is two identical 4-line early-returns that mirror the existing generateKey pattern in the same file (same hmacParameters.length && !hmacParameters.length.value() shape, just DataError instead of OperationError per spec). The placement inside the per-format case arms keeps the spec's error ordering intact, and the author explained why the check can't move into CryptoKeyHMAC::importRaw (structured-clone calls it with 0 meaning "unspecified").

Other factors

• My earlier nit about the guard running before the format switch was fully addressed in 92a9c65; the inline thread is resolved and the new it.each(['pkcs8','spki']) test guards against regressing it.
• No CODEOWNERS cover these paths.
• The bug-hunting pass found nothing.
• The CI failures reported by robobun are all build-rust jobs (plus one musl build-cpp) in scripts/build/ci.ts — this PR touches no Rust, so those look like infra flakes rather than anything caused here.

[claude]

LGTM — the error-precedence nit is addressed and the new tests pin both the fix and the format ordering.

Extended reasoning...

Overview

Adds a length == 0 → DataError guard to the Raw and Jwk branches of CryptoAlgorithmHMAC::importKey (8 lines of C++), plus a new describe block in web-crypto.test.ts covering absent/zero/short/exact length for raw and jwk, and asserting that pkcs8/spki still reject with NotSupportedError regardless of length.

My earlier review flagged that the guard was placed before the format switch, which would have flipped the spec's error precedence for unsupported formats. The author moved it into the per-format cases in 92a9c65 and added tests that lock in the precedence — this fully resolves the nit.

Security risks

None. The change tightens input validation on an edge case (HmacImportParams.length === 0) that previously fell through to deriving the length from key data. No new key material handling, no crypto primitive changes, no parsing of attacker-controlled buffers beyond what already exists.

Level of scrutiny

Low. This is a narrow spec-compliance fix matching browser/Node/Deno behavior. The guard pattern is copied verbatim from generateKey a few lines up in the same file, and the placement now mirrors how sibling algorithm files structure per-format checks. The author's note about not pushing the check into CryptoKeyHMAC::importRaw (because SerializedScriptValue.cpp relies on 0 meaning unspecified there) is correct and shows the right call sites were considered.

Other factors

No CODEOWNERS cover this path. The bug-hunting pass found nothing. Tests are thorough — they distinguish absent vs. explicit-zero, cover both supported formats, and regression-test the error-precedence ordering for unsupported formats. The robobun CI comment references failures on an earlier commit (aada231) before the ci: retrigger commit; the code change itself is sound regardless.

[robobun]

CI status, for whoever picks this up:

The only lane that fails is darwin 26 aarch64 - test-bun. It has failed the same way on all three builds for this PR (65689, 65758, 65821), before running a single test:

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

The darwin-aarch64-build-bun step it depends on passes every time. The test agent just cannot pull that artifact within 120s, a failure mode that is not reachable from this diff (a guard in CryptoAlgorithmHMAC::importKey plus a test file).

Build 65758 ran the full matrix against this exact code: 280 lanes passed and that one darwin lane was the only failure. On build 65821 six more test lanes expired waiting for an agent, and the most recent main build (65790) failed with its pipeline step timing out, so the agent pool looks unhealthy in general right now, independent of this PR.

I retried once with an empty commit and it hit the same timeout, so I'll stop re-pushing rather than pile up retrigger commits. The change is ready to review.

[robobun]

Closing: this change is already on main.

#34838 (50bb3bd) added the length: 0 check to CryptoAlgorithmHMAC::importKey (src/jsc/bindings/webcrypto/CryptoAlgorithmHMAC.cpp:112-116). The check runs before the format switch, so it covers both the raw and jwk paths that this PR patched. test/js/node/test/parallel/test-webcrypto-export-import.js asserts that length: 0 rejects with DataError and the message HmacImportParams.length cannot be 0.

Verified on main at e3b6d63 (source) and f42e980 (runtime):

• importKey("raw", new Uint8Array(20), { name: "HMAC", hash: "SHA-256", length: 0 }, true, ["sign"]) rejects with DataError.
• The same call with "jwk" and { kty: "oct", k: "AAAAAAAAAAAAAAAAAAAAAAAAAAA", alg: "HS256" } rejects with DataError.
• An absent length and length: 160 resolve with algorithm.length === 160.

Two expectations in this PR's tests differ from main on purpose. Main matches Node v26.3.0 in both cases:

• raw with length: 1 or length: 159 rejects with NotSupportedError ("Unsupported HmacImportParams.length"), not DataError. Node rejects any length that is not a multiple of 8 this way.
• spki and pkcs8 with length: 0 reject with DataError, not NotSupportedError. Node validates HmacImportParams.length in its WebIDL converter, before the format dispatch, and main keeps that order.