unjs / unenv

🕊️ Convert javaScript code to be runtime agnostic
MIT License
358 stars 19 forks source link

feat: support more node crypto functions #111

Closed AwesomeDude091 closed 1 year ago

AwesomeDude091 commented 1 year ago

Hopefully a fix for #110 Resolves #91

AwesomeDude091 commented 1 year ago

Are there any more changes required @pi0 ?

Romitou commented 1 year ago

I can confirm that it solves all the problems encountered with issue #110 on my side.

pi0 commented 1 year ago

Thanks for the pull request dear @AwesomeDude091. This PR is great however we probably need to use more of webCrypto API as node:crypto is only available for Node.js environments and compatibility layer of cloudflare pages for node. I will check it through asap.

Hebilicious commented 1 year ago

@AwesomeDude091 @Romitou I would be surprised if this solves your issues with cloudflare once it's deployed. Building locally will work because you are using a node.js environment for the build, however, what you're doing here isn't a polyfill, you're just using "node:crypto" in unenv (which should break in workerd as "node:crypto" won't be available).

If you want to polyfill all "node:crypto" method, you would basically need to do a real polyfill of node:crypto, similar to https://github.com/browserify/crypto-browserify

What I can suggest instead is that instead of trying to polyfill nuxt-auth with cloudflare, you use another library that doesn't require node-only internals at runtime, such as https://github.com/Hebilicious/authjs-nuxt (very similar to nuxt-auth but edge-compatible), or Lucia https://github.com/pilcrowOnPaper/lucia or even Hanko https://github.com/nuxt-modules/hanko

pi0 commented 1 year ago

Using edge friendly libs is always the best indeed however the aim of unenv is to add such compatibility layer we just have to make it better and less node dependent (PS: Cloudflare also has some optional node compatibility layer)

Hebilicious commented 1 year ago

@pi0 we could go with an incremental approach for the crypto polyfills, starting with the functions needed for nuxt-auth seems less daunting. We also need to make sure that we don't polyfill things that are available in crypto global

Cloudflare node compat doesn't include node:crypto, but we could add a cloudflare-node-compat preset to unenv to avoid polyfill wha they already polyfill.

AwesomeDude091 commented 1 year ago

Update:

Nearly all Cloudflare Issues have been resolved with the current build (testing on a local wrangler). It builds successfully without warnings, however the worker fails on initalization with error Uncaught TypeError: Cannot read properties of undefined (reading 'split') at do835gcosb7.js:20707:33, however this seems to be more of a general issue with cloudflare and nuxt or anything using service workers. The issues are outlined here: https://github.com/cloudflare/workers-sdk/issues/3144 and here https://github.com/unjs/nitro/issues/1105

Any ideas on how to tackle that 'split' issue?

P.S. For testing I build unenv locally, paste it into my node_modules folder of my test application (overwritting the current released build) and then build and wrangler execute

AwesomeDude091 commented 1 year ago

I isolated the line that is causing issues in the bundledWorker: var [Bv, Mv] = ge.versions.node.split(".").map((F2) => parseInt(F2, 10));

var me = "undefined" != typeof globalThis ? globalThis : "undefined" != typeof self ? self : "undefined" != typeof global ? global : {};
me.process = me.process || oe;
var ge = me.process;

Do you guys @pi0 @Hebilicious have any ideas?

Hebilicious commented 1 year ago

I isolated the line that is causing issues in the bundledWorker: var [Bv, Mv] = ge.versions.node.split(".").map((F2) => parseInt(F2, 10));

var me = "undefined" != typeof globalThis ? globalThis : "undefined" != typeof self ? self : "undefined" != typeof global ? global : {};
me.process = me.process || oe;
var ge = me.process;

Do you guys @pi0 @Hebilicious have any ideas?

Read my comment above about using node internals in unenv. This approach won't work, you need to remove all imports from "node:crypto" and polyfill them. You can keep the type imports as they won't be included in the final bundle. Also I would recommend that you turn-off the minification when you're testing, this will make your life easier.

AwesomeDude091 commented 1 year ago

I need some help here, it is not working out

AwesomeDude091 commented 1 year ago

I cannot figure out the port for generateKey, generateKeySync, generateKeyPair, generateKeyPairSync

AwesomeDude091 commented 1 year ago

I cannot figure out the port for generateKey, generateKeySync, generateKeyPair, generateKeyPairSync

Any help with this would be much appreciated

AwesomeDude091 commented 1 year ago

TODOs for a nearly complete Crypto Polyfill

AwesomeDude091 commented 1 year ago

@pi0 @Hebilicious Does the above list check out, are the previous new commits okay and is there any chance that you could give me a pointer as to what to do for the generateKey and generateKeyPair logic, especially the difference between sync and not sync?

pi0 commented 1 year ago

I will be working on this PR/Issue later this week once had time.

In general overview for changes:

pi0 commented 1 year ago

@Hebilicious Thanks for the review. I will handle the next steps on this. Kindly please NEVER request for change in other PRs in unjs repos unless you are an explicit maintainer of it (read more about roles). You can always ask questions about general project directions from maintainers.

Should we proxy all of browserify-crypto and augment it with the correct types ?

Not necessarily for all. Only for ones are possible to be purely implemented in JS with Web APIs. For those that are not possible or not implemented yet, unenv simply throws an Not Implemented error. But this often helps having these stubs to fix lots of build errors with incompatible Node.js libraries that simply import things and in practice don't essentially require them.

Should unenv have an explicit opt-in behavior for these browserify crypto polyfills ? This would be consistent with something like https://github.com/cyco130/esbuild-plugin-polyfill-node

Unenv only provides presets and polyfill (today). It is up to the consumer (nitro cloudflare presets for instance) to apply opt-in/out behavior which has to be opt-out i guess since cloudflare (and only with a flag) allows some of Node apis.

What happens if only one import is needed ? Does everything gets added to the final bundle ?

Exports are tree-shakable in general unless a lib is using require with untreeshakable syntax or import * from.

AwesomeDude091 commented 1 year ago

@pi0 I have started the implementation of generateKeyPair hope this is the right direction...

pi0 commented 1 year ago

Hi dear @AwesomeDude091 thanks again for working on all this and so sorry for all troubles.

121 adds all needed stubs so builds won't fail.

For cloudflare (and other worker runtimes) we shall leverage Node.js compatibility

If you think some of utils could be actually implemented and possible without external dependencies such as browserify-crypto and purely based on webcrypto/subtle, PR more than welcome on case by case basis.

Best luck ❤️