Repository navigation
Conversation
… length The instance digest() compared the array with the 64-byte scratch buffer. It now compares with the digest length of the hasher, as the static hash() does.
|
Updated 12:34 AM PT - Oct 13th, 2026
✅ @robobun, your commit ac41ac5f9f8d160f7af100da2df41fbf38acb6e1 passed in 🧪 To try this PR locally: bunx bun-pr 45085That installs a local version of the PR into your bun-45085 --bun |
|
Status: the fix and its tests are on the branch. Reproduction: Test proof: |
…r SHAKE A longer TypedArray is checked past byteLength only for fixed-length digests. The HMAC test moves above the key loop.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesHasher digest output sizing
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| // SAFETY: `bytes_len >= size` checked above, and `final_` writes `size` | ||
| // bytes, or all `bytes_len` of them for a SHAKE hasher. No digest is | ||
| // empty, so the view is not detached and `output_buf.ptr` is the | ||
| // JSC-owned writable backing store, outliving this frame. Build the | ||
| // `&mut` directly from the raw `*mut u8` field — never via | ||
| // `&[u8].as_ptr()` (Stacked-Borrows UB). | ||
| output_digest_slice = | ||
| unsafe { core::slice::from_raw_parts_mut(output_buf.ptr, bytes_len) }; |
There was a problem hiding this comment.
🟡 nit (optional): users of shake128/shake256 who now pass a 16..63-byte array to digest(hashInto) get the whole array overwritten, while Bun.CryptoHasher.hash(alg, input, arr) with the same array writes only byteLength bytes. The instance path hands the full bytes_len slice to the XOF at src/runtime/crypto/CryptoHasher.rs:733, and impl_xof squeezes out.len() bytes; the static path at src/runtime/crypto/CryptoHasher.rs:999 slices to digest_length_comptime. The new .d.ts text ("at least byteLength bytes") and the SAFETY comment describe this only as an aside. Fix: make the two entry points agree for SHAKE (either both squeeze the full array or both write byteLength), or document the whole-array squeeze in bun.d.ts and docs and pin it with a test that asserts the trailing bytes.
Why this was flagged
A user calls new Bun.CryptoHasher("shake128").update(x).digest(new Uint8Array(32)). On the base branch this threw "TypedArray must be at least 64 bytes"; after the change src/runtime/crypto/CryptoHasher.rs:717-733 accepts it because size is 16 and builds the slice over all 32 bytes. CryptoHasherZig::final_ at src/runtime/crypto/CryptoHasher.rs:1082 forwards the whole slice and impl_xof at src/runtime/crypto/CryptoHasher.rs:860 calls finalize_xof_reset_into(self, out), writing all 32 bytes. The sibling static path at src/runtime/crypto/CryptoHasher.rs:999 builds the slice with digest_length_comptime (16) and leaves the other 16 bytes untouched. So the same algorithm, input and array give different buffer contents from the two public entry points. The base branch had the same divergence only for arrays of 64 bytes or more; this change makes it reachable for every array from byteLength up to 63 bytes. The new test at test/js/bun/util/bun-cryptohasher.test.ts:582 explicitly skips asserting the trailing bytes for shake, so nothing pins either behaviour.
Verification: The test at test/js/bun/util/bun-cryptohasher.test.ts:586-591 deliberately skips the trailing-bytes check for shake*. src/runtime/crypto/CryptoHasher.rs:717-733 checks bytes_len < size then builds from_raw_parts_mut(output_buf.ptr, bytes_len) over the whole array; impl_xof::final_ at 857-861 squeezes out.len() bytes. The static sibling at 998-1000 slices to digest_length_comptime.
There was a problem hiding this comment.
Correct. It is the open question of this PR (the last bullet of Downsides).
Each option changes something:
- Write only
byteLengthbytes in the instance form. The two forms then agree, but a call that works today changes: on 1.4.3,new Bun.CryptoHasher("shake128").update("x").digest(new Uint8Array(100))writes 100 bytes, and the static form writes 16. - Keep the fill. No shipped call changes, but the fill is now also reachable for an array of
byteLengthto 63 bytes.
This PR keeps the fill, because it is a bug fix and the other option changes a shipped result. It does not document the fill and no test pins it, so the instance form can still be cut to byteLength without a broken contract. That decision needs a maintainer, so this thread stays open.
Problem
new Bun.CryptoHasher("sha256").digest(new Uint8Array(32))throwsTypeError: TypedArray must be at least 64 bytes. It is the example ofdocs/runtime/hashing.mdx.byteLength. No user reported it.CryptoHasher::digest_to_bytes(src/runtime/crypto/CryptoHasher.rs:714) compares the array with its 64-byte stack buffer.Fix
byteLength), as the statichash()does.test/js/bun/util/bun-cryptohasher.test.ts(35 new tests, 29 fail without the fix), five other crypto test files.Background
EVP_DigestFinal_exandHMAC_Finaldo not read the length of the destination. This check is the only guard, as on the static path.Downsides
digest(TypedArray)makes 1 more call:EVP_MD_CTX_size,HMAC_size, or a field read.digest()anddigest(encoding)are unchanged.byteLengthto 63 bytes. The static form writesbyteLengthbytes. No test pins that difference.Notes
Before and after, for an array of exactly
byteLengthbytes:What each kind writes into a slice of
bytes_len >= sizebytes:EVP:EVP_DigestFinal_exwritesmd_sizebytes.EVP::size()returns the samemd_sizethroughEVP_MD_CTX_size.HMAC:HMAC_FinalwritesEVP_MD_size(ctx->md)bytes.HMAC::size()returns the same value throughHMAC_size.out[..DIGEST_LENGTH].digest_lengthis that constant.finalize_xof_reset_into(out)writesout.len()bytes, the length of the slice.Other results on this branch:
TypedArray must be at least <byteLength> bytes(ERR_INVALID_ARG_TYPE), the message of the static form. The hasher keeps its data. An HMAC is not consumed.byteLength + 8bytes for each of them.shake128into 20 and into 100 bytes writes 20 and 100 bytes. The static form writes 16. For SHAKE the tests check only the firstbyteLengthbytes of a longer array, so they do not make the rest a contract.Uint32Array(8)takes a SHA-256 digest: the check counts bytes. Asubarray(8, 40)of a 48-byte array gets the digest at bytes 8 to 40, and the 8 bytes on each side stay untouched.Docs and types:
docs/runtime/hashing.mdxnow runs as printed in page order:digest()resets the hasher, so the block callsupdate()beforedigest(arr). docs, bun-types: correct 20 examples that do not run as printed #44480 makes the same correction for the block above it.digest(hashInto)and the page state the minimum length.Reach and order:
git merge-treeof this branch with Decide a CryptoHasher failure from the BoringSSL return value, not the error queue #44940 gives no conflict in either order. For that,byte_length()sits after the getter, and the HMAC test sits above the key loop.Design notes:
CryptoHasher::hash_to_bytes,CryptoHasherZig::hash_by_name_inner_to_bytes, andStaticCryptoHasher::hash_to_bytesanddigest_to_bytes(Bun.SHA256and the other classes).EVP::r#final,EVP::hashandHMAC::r#finalare safe functions that rely on the caller for the length of the destination. This PR does not change them. Their callers areCryptoHasherand thenode:cryptohash, which passes at least 64 bytes.Suites run with the debug build:
test/js/bun/util/bun-cryptohasher.test.ts(139 pass),test/js/node/crypto/crypto-hmac-algorithm.test.ts,test/js/node/crypto/node-crypto.test.js,test/js/node/crypto/crypto.test.ts,test/regression/issue/36577.test.ts,test/regression/issue/17793.test.ts(627 pass).test/integration/bun-types/bun-types.test.ts(22 pass).