search

digitalbazaar / forge

A native implementation of TLS in Javascript and tools to write crypto-based and network-heavy webapps
https://digitalbazaar.com/
Other
5.05k stars 779 forks source link

fix: jsbn use util.isNodejs to determine node runtime #1082

Open dhensby opened 4 months ago

dhensby commented 4 months ago

fixes #1081

A simple fix to avoid using the navigator global for determining if we are running node, instead use the forge util.isNodejs value.

TBH - I have no idea if the assumptions around performance in different browsers still holds since 2005 (almost 20 years ago) - but this at least keeps the node performance where it was.

davidlehn commented 4 months ago

Did this come up because of an error or performance issue? Or did you just happen to notice it? I refreshed a PR to test up though 22 and tests pass without this patch. Not sure if that path is tested though. https://github.com/digitalbazaar/forge/pull/976

There is also navigator use in util.estimateCores and in lib/random.js. Not sure if those paths would be hit in Node.js use. A lot of code here needs refactoring to catch up with the last few decades of JS advancements.

As far as a minimal change that likely won't break anything, I think this patch will work. Anyone else watching have thoughts?

dhensby commented 4 months ago

This came up in a test we have in one of our projects which compares the internals of a BigInteger to make sure our internal conversion of JWK to rsa keys performs as expected.

In Node 22 (and 21 from their changelog) there was a discrepancy because of this change.

I haven't done any testing of the actual performance implications, but it makes sense to continue to use 32 bit internals for node IMO.

I had found another slight improvement to JSBN off the back of this (when the number is clamped, it should truncate the data array, otherwise you can find times the array has vestigial elements at the end - this can easily be tested by providing a hex number with leading zeros), but I've left that out as it's a lot more "opinionated" change than just restoring the logic path for Node.

Looking at the other uses of navigator you point out, they are sufficiently narrowed/guarded to ensure they aren't a problem in Node (from what I can see).