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(node:crypto): properly call web crypto methods #122

Merged
merged 4 commits into from
Aug 8, 2023
Merged

Conversation

AaronDewes
Copy link
Contributor

@AaronDewes AaronDewes commented Aug 7, 2023

The latest change broke some Nuxt deployments to Deno deploy for me. Deno's webcrypto is somewhat different, and ...globalThis.crypto does not work in Deno. This replaces it with a custom getter that retrieves the correct value instead. This adds some overhead, but is required for Deno compatibility.

grafik

@@ -5,10 +5,16 @@ import type nodeCrypto from "node:crypto";
export const CryptoKey =
globalThis.CryptoKey as unknown as typeof nodeCrypto.webcrypto.CryptoKey;

export const webcrypto: Crypto & typeof nodeCrypto.webcrypto = {
CryptoKey,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can simply bind CryptoKey in L5 to avoid proxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tried that, and I don't think I fully understand what you mean to be honest. Can you show me a code example so I can try locally?

Copy link
Member

Choose a reason for hiding this comment

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

const CryptoKey = globalThis.CryptoKey.bind(globalThis.CryptoKey). Please try it on your repo (if you have a reproduction that would be amazing too!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using Nuxt with Deno Deploy, so the issue is somewhat nested in the dependency chain, but I'll try to create a minimal sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/AaronDewes/unenv-bug-demo There's also some more explanation of the bug in this repo. I hope this shows the issue well enough. Nitro is probably not needed, but was the easiest for me to set up as a demo.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for nice reproduction! Have made some few refactors to make sure node webcrypto is not affecting us also simplified proxy (by using main crypto as target, proxy handles calls with the right context)

@pi0 pi0 changed the title Add deno compatibility to the webCrypto node implementation fix(node:crypto): properly call web crypto methods Aug 8, 2023
@pi0 pi0 merged commit 2fcf9b8 into unjs:main Aug 8, 2023
2 checks passed
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.

2 participants