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

Remove the dependency on node:crypto module #216

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

aron
Copy link
Contributor

@aron aron commented Mar 7, 2024

Fixes #210

This replaces the use of the node:crypto module with functions from the Web Crypto API which are supported in both Node >=15, browsers, Deno and Cloudflare Workers (hopefully also React Native).

The only issue was base64 decoding the string into an ArrayBuffer for use by the crypto libraries. In node this is done with Buffer.from(string, 'base64') but there is no equivalent in the browser. The atob() and btoa() functions use latin1 rather than utf8 which made them incompatible.

After evaluating a bunch of third party modules I settled on @hexagon/base64 which is actively maintained and supports a breadth of environments.

@mattt
Copy link
Contributor

mattt commented Mar 7, 2024

Thanks for taking this on, @aron!

The only issue was base64 decoding the string into an ArrayBuffer for use by the crypto libraries. In node this is done with Buffer.from(string, 'base64') but there is no equivalent in the browser. The atob() and btoa() functions use latin1 rather than utf8 which made them incompatible.

After evaluating a bunch of third party modules I settled on @hexagon/base64 which is actively maintained and supports a breadth of environments.

If at all possible, I think we should avoid adding any third-party dependencies.

For our use case, could we inline something like what MDN has in their Base64 article?

@aron aron force-pushed the remove-node-crypto branch from 6b4c91c to 7340315 Compare March 7, 2024 11:54
@aron
Copy link
Contributor Author

aron commented Mar 7, 2024

If at all possible, I think we should avoid adding any third-party dependencies.

We could, though we have the same issue for the eventsource-parser that fixes the streaming. I'm open to vendorizing these scripts with attribution to avoid the npm dependencies if preferred? Both are pretty stable implementations that I don't think will change much over time.

@mattt
Copy link
Contributor

mattt commented Mar 7, 2024

Yes, I'm happy with us vendoring or inlining code with relevant attribution to provide cross-platform polyfills. We can tailor this code specifically to our use case to minimize bloat. The most important thing is good test coverage to ensure correctness and compatibility.

We use base64 for webhook validation as well as data URI encoding, so we'll need something that can handle strings and bytes. The MDN reference code seems to support both with minimal code, so I'm inclined toward that. @hexagon/base64 includes validation and uses lookup tables, which seem unnecessary for our specific use case.

@aron
Copy link
Contributor Author

aron commented Mar 7, 2024

I did a bunch of research on this when looking into cross-browser support for creating the data-uri's for the image uploads. I can dig it out if needed. My TLDR; was that the two MDN examples aren't going to cut it, particularly performance wise for encoding files.

The btoa and atob functions are tagged as legacy in Node and very slow, which concerned me for use with file uploads. They also have the double encoding issue mentioned in the doc.

The FileReader approach only works in the browser at the moment, so we'd need something else for Node anyway. And, despite being async various benchmarks I looked at showed it was also slower than the plain JS implementations.

There was a good implementation I found that had solid benchmarks, but I couldn't find it today. Will have another look and revisit it. I'd prefer to vendor something that already has an existing test suite.

@aron
Copy link
Contributor Author

aron commented Mar 7, 2024

Went back through my notes and looked at the benchmarks. I think the tradeoffs are mainly around encoding string data where non-native implementations like https://gist.github.com/jonleighton/958841 and @hexagon/base64 appear to be faster.

For our use cases we're dealing with binary data and secrets at the moment so I think we'll be fine.

@aron aron force-pushed the remove-node-crypto branch from 903ef45 to 2ecebbf Compare March 7, 2024 14:44
@aron aron merged commit cbfcc62 into main Mar 7, 2024
10 checks passed
@aron aron deleted the remove-node-crypto branch March 7, 2024 18:03
@guivr
Copy link

guivr commented Mar 15, 2024

node:crypto is still not available in React Native

IMG_2848

@vishalvibes
Copy link

vishalvibes commented Mar 17, 2024

Hey @aron 👋, I am still facing the same issue, in Next.js

line 93 still has node:crypto 👇

Screenshot 2024-03-17 at 11 58 17 PM

@aron
Copy link
Contributor Author

aron commented Mar 18, 2024

@vishalvibes would you please open an issue with a simple example of reproducing the issue plus details of your setup including Node version. Current versions of Vercel run node 18 which is fully supported so I don't see any immediate issues here.

@vishalvibes
Copy link

Sure @aron
I just opened an issue 👉 #225

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.

Node standard library module "node:crypto".
5 participants