Skip to content

Bun.CryptoHasher: accept a TypedArray of byteLength bytes in digest(hashInto) - #45085

Open
robobun wants to merge 3 commits into
mainfrom
robobun/8c37116f/cryptohasher-digest-into-byte-length
Open

robobun wants to merge 3 commits into
mainfrom
robobun/8c37116f/cryptohasher-digest-into-byte-length

Conversation

@robobun

@robobun robobun commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • new Bun.CryptoHasher("sha256").digest(new Uint8Array(32)) throws TypeError: TypedArray must be at least 64 bytes. It is the example of docs/runtime/hashing.mdx.
  • 16 of the 19 algorithms refuse an array of their own 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

  • Compare with the digest length (byteLength), as the static hash() does.
  • Each finalizer writes the digest length, so the write stays inside the array. SHAKE fills the array, as before.
  • Verified: test/js/bun/util/bun-cryptohasher.test.ts (35 new tests, 29 fail without the fix), five other crypto test files.
  • Self-reviewed: 7 concerns raised, 6 addressed. Not done here: a runner for the docs code fences.

Background

Downsides

  • digest(TypedArray) makes 1 more call: EVP_MD_CTX_size, HMAC_size, or a field read. digest() and digest(encoding) are unchanged.
  • A consumed HMAC with an array under 64 bytes now reports the consumed error, not the size error.
  • SHAKE (extendable output): the instance form now also fills an array of byteLength to 63 bytes. The static form writes byteLength bytes. No test pins that difference.
Notes

Before and after, for an array of exactly byteLength bytes:

                 1.4.3 (release)                                this branch (debug, ASAN)
sha256 32  digest(out): throws: TypedArray must be at least 64 bytes    fills it, b94d27b9
sha1   20  digest(out): throws: TypedArray must be at least 64 bytes    fills it, 2aae6c35
md5    16  digest(out): throws: TypedArray must be at least 64 bytes    fills it, 5eb63bbb
sha384 48  digest(out): throws: TypedArray must be at least 64 bytes    fills it, fdbd8e75
sha512 64  digest(out): fills it, 309ecc48                              fills it, 309ecc48

What each kind writes into a slice of bytes_len >= size bytes:

  • EVP: EVP_DigestFinal_ex writes md_size bytes. EVP::size() returns the same md_size through EVP_MD_CTX_size.
  • HMAC: HMAC_Final writes EVP_MD_size(ctx->md) bytes. HMAC::size() returns the same value through HMAC_size.
  • RustCrypto, fixed length: out[..DIGEST_LENGTH]. digest_length is that constant.
  • RustCrypto, SHAKE: finalize_xof_reset_into(out) writes out.len() bytes, the length of the slice.

Other results on this branch:

  • An array that is one byte short throws 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.
  • An empty or detached array gets the same error.
  • A longer array: the digest is at the start and the rest is untouched, for every fixed-length algorithm and for HMAC. The tests check an array of byteLength + 8 bytes for each of them.
  • shake128 into 20 and into 100 bytes writes 20 and 100 bytes. The static form writes 16. For SHAKE the tests check only the first byteLength bytes of a longer array, so they do not make the rest a contract.
  • A Uint32Array(8) takes a SHA-256 digest: the check counts bytes. A subarray(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:

Reach and order:

  • No user reported this. A run of the example of the docs page found it. No test in this repository runs the code fences of the docs. The new test named after the page is a copy of the example.
  • git merge-tree of 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.
  • A call that worked before this PR (an array of 64 bytes or more) writes the same bytes as before.

Design notes:

  • The four sibling sites already compare with the digest length: the static CryptoHasher::hash_to_bytes, CryptoHasherZig::hash_by_name_inner_to_bytes, and StaticCryptoHasher::hash_to_bytes and digest_to_bytes (Bun.SHA256 and the other classes).
  • EVP::r#final, EVP::hash and HMAC::r#final are safe functions that rely on the caller for the length of the destination. This PR does not change them. Their callers are CryptoHasher and the node:crypto hash, 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).

… 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.
@robobun

robobun commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:34 AM PT - Oct 13th, 2026

✅ @robobun, your commit ac41ac5f9f8d160f7af100da2df41fbf38acb6e1 passed in Build #125028! 🎉


🧪   To try this PR locally:

bunx bun-pr 45085

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

bun-45085 --bun

@robobun

robobun commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the fix and its tests are on the branch.

Reproduction: new Bun.CryptoHasher("sha256").update("hello world").digest(new Uint8Array(32)) throws TypedArray must be at least 64 bytes on 1.4.3 and on main. On this branch it writes the 32 bytes.

Test proof: USE_SYSTEM_BUN=1 bun test test/js/bun/util/bun-cryptohasher.test.ts fails 29 of the 35 new tests. bun bd test test/js/bun/util/bun-cryptohasher.test.ts passes all 139.

…r SHAKE

A longer TypedArray is checked past byteLength only for fixed-length
digests. The HMAC test moves above the key loop.
@robobun
robobun marked this pull request as ready for review October 13, 2026 06:44
@robobun
robobun requested a review from alii as a code owner October 13, 2026 06:44
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 073864ca-32f0-4d6f-903c-065153d5409d
📥 Commits

Reviewing files that changed from the base of the PR and between 7583003 and ac41ac5.

📒 Files selected for processing (4)
  • docs/runtime/hashing.mdx
  • packages/bun-types/bun.d.ts
  • src/runtime/crypto/CryptoHasher.rs
  • test/js/bun/util/bun-cryptohasher.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

CryptoHasher.digest() now validates output buffers against the digest’s byte length instead of EVP_MAX_MD_SIZE. Tests and documentation specify the minimum buffer size and cover exact-size, undersized, and oversized buffers.

Changes

Hasher digest output sizing

Layer / File(s) Summary
Digest byte-length and output-buffer validation
src/runtime/crypto/CryptoHasher.rs, test/js/bun/util/bun-cryptohasher.test.ts, docs/runtime/hashing.mdx, packages/bun-types/bun.d.ts
The hasher reports its digest length, and output-buffer validation uses that length. Tests cover HMAC and CryptoHasher.digest(hashInto) behavior, including typed arrays and views. Documentation states the minimum buffer size.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to ac41a

The output-buffer sizing change appears ready to merge after normal checks.

Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title is concise, specific, and accurately describes the primary change to Bun.CryptoHasher.digest(hashInto).
Description check Passed The description clearly explains the problem, fix, behavior changes, verification steps, test results, and known limitations. It uses alternative headings instead of the template headings, but it cove…
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.
  • 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

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

@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.

Comment on lines +726 to 733
// 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) };

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.

🟡 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct. It is the open question of this PR (the last bullet of Downsides).

Each option changes something:

  • Write only byteLength bytes 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 byteLength to 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.

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