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

Drop support for old nodejs versions (req v14) #49

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jimmywarting
Copy link

@jimmywarting jimmywarting commented Sep 15, 2023

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

Removed some dependencies that are no longer needed.
This will require new nodejs versions.
the functionality stays the same.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Auth0 Community post
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@@ -1,18 +1,23 @@
var bufferEqual = require('buffer-equal-constant-time');
var Buffer = require('safe-buffer').Buffer;
var Buffer = require('buffer').Buffer;
var crypto = require('crypto');
var formatEcdsa = require('ecdsa-sig-formatter');
Copy link
Collaborator

@panva panva Sep 15, 2023

Choose a reason for hiding this comment

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

Node crypto's own dsaEncoding option valued ieee-p1363 can be used for ECDSA to completely remove the ecdsa-sig-formatter dependency. This was added in nodejs/node#29292 and is available in all nodejs versions above v13.2.0 and v12.16.0.

At the same time the node crypto now offers one-shot sign and verify methods which can be used both in both a blocking and non-blocking manner.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, can you show a code example of how to solve it...
maybe just do a little diff suggestion and then i can just apply the changes ;)

```diff
+
-
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Sry, i'm too lost right now. don't work much with cryptografy. will leave this fully up to you guys to solve.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's really just a lift and shift from the old api to the new and for ecdsa having a dedicated signer/verifier with the dsaEncoding option.

@tiAshEluntz
Copy link

Any update on this PR?
I am having some issue with buffer-equal-constant-time, should I create a small PR only to remove the dependency?

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