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

Switch from CryptoJS to WebCrypto API #286

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

Conversation

arjunyel
Copy link
Contributor

From CryptoJS readme:

Active development of CryptoJS has been discontinued. This library is no longer maintained.

Removed CryptoJS and replaced it with the WebCrypto API. Also added tests to make sure it still works with a polyfill.

@arjunyel
Copy link
Contributor Author

I have no idea why globalThis.crypto.getRandomValues is not found? bun run test passes on my machine, does it on yours?

@sergiodxa
Copy link
Owner

sergiodxa commented Nov 30, 2023

That's because tests still run in Node and Node doesn't have WebCrypto API globally available, you have to import it from the crypto package.

This is also the reason Remix Utils uses CryptoJS instead of the WebCrypto API, because of Node.

And since most Remix app runs on Node, and not Cloudflare, Deno, or Bun, this will be a breaking change for those apps, that will now need to supply the Node implementation of WebCrypto.

@arjunyel
Copy link
Contributor Author

arjunyel commented Nov 30, 2023

Should be fully supported in Node 20 which is now LTS :D It's available on globalThis so node LTS users will have no breaking change besides some methods becoming async. Also CryptoJS is no longer supported so could have security issues. I can't speak for other Remix projects but mine in personally on Cloudflare.

src/helpers/base64.ts Outdated Show resolved Hide resolved
@sergiodxa
Copy link
Owner

Should be fully supported in Node 20 which is now LTS :D It's available on globalThis so node LTS users will have no breaking change besides some methods becoming async. Also CryptoJS is no longer supported so could have security issues. I can't speak for other Remix projects but mine in personally on Cloudflare.

Remix apps requires Node v18, I'm not sure why but requiring but Remix Utils keeps the same minimum required Node version as the Remix version it supports, since Remix Utils v7 requires Remix v2 it therefore should work with Node v18.

@arjunyel
Copy link
Contributor Author

I agree this would be a breaking change for Node 18 users, they can use --experimental-global-webcrypto or the polyfill method from the docs. However CryptoJS is not maintained so the transition to WebCrypto has to happen eventually

@enZane
Copy link

enZane commented Aug 5, 2024

Any news on this? I don't want to open another thread :p
In a couple of months node lts will be v22

@sergiodxa
Copy link
Owner

As long as Remix supports v18 this will not change

@arjunyel
Copy link
Contributor Author

arjunyel commented Aug 6, 2024

As long as Remix supports v18 this will not change

Your Remix auth library uses web crypto so curious why different for this library :) Node 18 users are able to use the Web Crypto polyfill so they wouldn't be left out

@enZane
Copy link

enZane commented Aug 6, 2024

And what about a way to provide a sharable interface, so this honeypot can be integrated with whatver remix supported runtime

Currently, I faced a problem with cloudflare workers because crypto-js is not edge ready:(

If that makes sense I can try to draft a PR for that instead of a more aggressive transition

@bitofbreeze
Copy link

bitofbreeze commented Aug 9, 2024

Everyone could be happy by making an intermediary package that conditionally exports the needed functions from cryptojs when runtime is node, and otherwise web crypto api. Here's an example where you can see a different file being exported depending on node or workerd runtime. Here are some other possible values you can specify for the runtime. Will just need to make a thin wrapper API to make the functions from each package have the same signature.

@arjunyel
Copy link
Contributor Author

arjunyel commented Aug 9, 2024

Everyone could be happy by making an intermediary package that conditionally exports the needed functions from cryptojs when runtime is node, and otherwise web crypto api. Here's an example where you can see a different file being exported depending on node or workerd runtime. Here are some other possible values you can specify for the runtime. Will just need to make a thin wrapper API to make the functions from each package have the same signature.

I don't think that's necessary, instead older node can just use a WebCrypto polyfill

@bitofbreeze
Copy link

bitofbreeze commented Aug 9, 2024

@arjunyel But then you're adding unnecessary bundle size to all runtimes, not just where it's needed. To make things easier, I found an existing package that we can use to do what we're describing by the way (uses webcrypto polyfill when in node, otherwise actual webcrypto—best of both worlds)

@arjunyel
Copy link
Contributor Author

arjunyel commented Aug 9, 2024

@arjunyel But then you're adding unnecessary bundle size to all runtimes, not just where it's needed. To make things easier, I found an existing package that we can use to do what we're describing by the way (uses webcrypto polyfill when in node, otherwise actual webcrypto—best of both worlds)

No the user would be responsible for passing in the polyfill so it won't effect everyone. That's how I have it set up on this PR :)

@bitofbreeze
Copy link

@arjunyel Sure but I guess that is @sergiodxa's concern "this will be a breaking change for those apps, that will now need to supply the Node implementation of WebCrypto", so if you would use that dependency instead, I think it would cover the concern.

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.

4 participants