Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented May 22, 2024

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
Copy link
Member

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.
#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
Copy link
Contributor Author

dhensby commented May 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jsbn] Node 21 introduced navigator global object which has changed jsbn behaviour
2 participants