Skip to content

crypto: fix unhandled error in Hash._transform#63261

Open
haramj wants to merge 1 commit into
nodejs:mainfrom
haramj:haramj-patch-260512
Open

crypto: fix unhandled error in Hash._transform#63261
haramj wants to merge 1 commit into
nodejs:mainfrom
haramj:haramj-patch-260512

Conversation

@haramj
Copy link
Copy Markdown
Member

@haramj haramj commented May 12, 2026

Description

This PR fixes a bug where errors occurring during Hash.prototype.write() were silently swallowed instead of being surfaced.

Problem

In lib/internal/crypto/hash.js, the _transform implementation did not check the return value of this[kHandle].update(). While the synchronous update() method correctly throws ERR_CRYPTO_HASH_UPDATE_FAILED on failure, the streaming interface continued execution even if the underlying ncrypto binding returned false. This resulted in incorrect hash outputs without any error notification to the user.

Solution

Updated Hash.prototype._transform to check the return value of the internal update() call. If it returns false, the error is passed to the stream callback, triggering an 'error' event as expected for a stream.Transform instance.

Validation

  • Added a regression test test/parallel/test-crypto-hash-stream-error.js.
  • The test mocks the internal handle using kHandle to simulate an update failure and verifies that an 'error' event with ERR_CRYPTO_HASH_UPDATE_FAILED is emitted.
  • Verified that the test fails without this change and passes with it.
  • Verified that existing crypto tests (test/parallel/test-crypto-hash.js) still pass.

Checklist

  • make lint-js passes
  • make test-parallel (relevant to crypto) passes
  • Commit message follows the Node.js guidelines

Fixes: #63258

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels May 12, 2026
@haramj haramj force-pushed the haramj-patch-260512 branch from 868c0c8 to 931b4bc Compare May 12, 2026 05:46
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.04%. Comparing base (78bbee3) to head (931b4bc).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63261      +/-   ##
==========================================
- Coverage   90.04%   90.04%   -0.01%     
==========================================
  Files         713      713              
  Lines      225003   225004       +1     
  Branches    42536    42539       +3     
==========================================
- Hits       202606   202603       -3     
- Misses      14177    14183       +6     
+ Partials     8220     8218       -2     
Files with missing lines Coverage Δ
lib/internal/crypto/hash.js 99.01% <100.00%> (+<0.01%) ⬆️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crypto.Hash.write swallows errors

2 participants