crypto: changes to async handling by Renegade334 · Pull Request #59841 · nodejs/node

Renegade334 · 2025-09-10T14:16:35Z

crypto: avoid calls to `promise.catch()`

Changes promise.catch() to try/catch in async crypto functions.

This avoids explicit calls to the user-mutable %Promise.prototype%.catch, and by association, implicit calls to the user-mutable %Promise.prototype%.then.

crypto: use async functions for non-stub Promise-returning functions

There are certain functions in the crypto internals which simply wrap a call to a Promise-returning function, and these stubs can be non-async, as there's no need to re-wrap the return value in another Promise.

However, there are some functions which are using this pattern, but are not quite so "stubby", and perform meaningful code execution before the wrapped call. This is an issue because any errors raised in the function body will be thrown synchronously, not wrapped in a rejected Promise.

Declaring these functions as async is the easiest way of handling these cases.

crypto: use `return await` when returning Promises from async functions

As discussed in #59699.

When returning a Promise p from an async function, it is wrapped in an outer Promise by the language, in a way that involves an implicit call to p.then().

return await avoids this behaviour in most conditions, as await resolves a builtin Promise without a call to .then(). A normative change being considered to ECMA-262 would bring "most" to "all" in the future.

This avoids explicit calls to the user-mutable `%Promise.prototype%.catch`, and by association, implicit calls to the user-mutable `%Promise.prototype%.then`.

These were intended to mimic simple async functions, but exceptions thrown in the function body would be returned synchronously, not wrapped in a rejected Promise.

This offers _some_ resistance to `%Promise.prototype%` pollution. Refs: nodejs#59699

nodejs-github-bot · 2025-09-10T14:16:40Z

Review requested:

@nodejs/crypto

codecov · 2025-09-10T15:47:56Z

Codecov Report

❌ Patch coverage is 85.71429% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.25%. Comparing base (861d624) to head (40f03d4).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/crypto/aes.js	69.23%	4 Missing ⚠️
lib/internal/crypto/mac.js	60.00%	4 Missing ⚠️
lib/internal/crypto/cfrg.js	71.42%	2 Missing ⚠️
lib/internal/crypto/chacha20_poly1305.js	77.77%	2 Missing ⚠️
lib/internal/crypto/ec.js	77.77%	2 Missing ⚠️
lib/internal/crypto/ml_dsa.js	71.42%	2 Missing ⚠️
lib/internal/crypto/ml_kem.js	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59841      +/-   ##
==========================================
- Coverage   88.26%   88.25%   -0.02%     
==========================================
  Files         701      701              
  Lines      206774   206819      +45     
  Branches    39778    39788      +10     
==========================================
+ Hits       182514   182524      +10     
- Misses      16298    16319      +21     
- Partials     7962     7976      +14

Files with missing lines	Coverage Δ
lib/internal/crypto/hash.js	`98.93% <100.00%> (ø)`
lib/internal/crypto/rsa.js	`94.10% <100.00%> (+0.04%)`	⬆️
lib/internal/crypto/webcrypto.js	`96.37% <100.00%> (+0.03%)`	⬆️
lib/internal/crypto/cfrg.js	`96.25% <71.42%> (-0.51%)`	⬇️
lib/internal/crypto/chacha20_poly1305.js	`93.78% <77.77%> (-0.99%)`	⬇️
lib/internal/crypto/ec.js	`96.21% <77.77%> (-0.61%)`	⬇️
lib/internal/crypto/ml_dsa.js	`93.71% <71.42%> (-0.58%)`	⬇️
lib/internal/crypto/ml_kem.js	`88.65% <60.00%> (-0.58%)`	⬇️
lib/internal/crypto/aes.js	`87.63% <69.23%> (-0.49%)`	⬇️
lib/internal/crypto/mac.js	`91.12% <60.00%> (-1.44%)`	⬇️

... and 26 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.

jasnell · 2025-09-10T16:38:27Z

lib/internal/crypto/webcrypto.js

+      return await require('internal/crypto/diffiehellman')
        .ecdhDeriveBits(algorithm, baseKey, length);


Nit: These have always been a bit weird. It would probably be better to separate these lines:

Suggested change

return await require('internal/crypto/diffiehellman')

.ecdhDeriveBits(algorithm, baseKey, length);

{

const { ecdhDeriveBits } = require('internal/crypto/diffiehellman');

return await ecdhDeriveBits(algorithm, baseKey, length);

}

Optional tho

Think I'll leave for a follow-up.

lib/internal/crypto/cfrg.js

lib/internal/crypto/ml_dsa.js

nodejs-github-bot · 2025-09-10T18:36:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69174/

nodejs-github-bot · 2025-09-12T14:21:52Z

Landed in e511344...8869210

This avoids explicit calls to the user-mutable `%Promise.prototype%.catch`, and by association, implicit calls to the user-mutable `%Promise.prototype%.then`. PR-URL: #59841 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

These were intended to mimic simple async functions, but exceptions thrown in the function body would be returned synchronously, not wrapped in a rejected Promise. PR-URL: #59841 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>