meilisearch / meilisearch-js

JavaScript client for the Meilisearch API
https://www.meilisearch.com
MIT License
732 stars 86 forks source link

Switch from Node.js crypto to Web Crypto #1661

Open flevi29 opened 4 months ago

flevi29 commented 4 months ago

Pull Request

What does this PR do?

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.00%. Comparing base (6cd64e9) to head (4fc66e8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1661 +/- ## ========================================== + Coverage 97.43% 98.00% +0.56% ========================================== Files 22 20 -2 Lines 858 850 -8 Branches 93 94 +1 ========================================== - Hits 836 833 -3 + Misses 21 16 -5 Partials 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

flevi29 commented 4 months ago

Oh well, crypto global is only available since Node.js 19, bummer, will try to fix this later. https://developer.mozilla.org/en-US/docs/Web/API/Crypto#browser_compatibility

flevi29 commented 4 months ago

Hmm, maybe I should do something so @types/node only applies to webcrypto import on fix, I'll convert it to draft for now.

flevi29 commented 4 months ago

I tried to fix the typing issue for the node:crypto import properly, but that opened up another can of worms, found some more bugs/issues, too many changes for this PR, so I just @ts-expect-error it for now.

flevi29 commented 4 months ago

This PR would make #1595 more or less obsolete, but there might be more things to consider before we can truly say that. EDIT: I'm not sure what I was thinking when I wrote the last part of my comment, but yeah, it does make it obsolete, now there's only one client, JS client.

brunoocasali commented 4 months ago

Hi @flevi29 so the idea of not allowing the customer to use the generateTenantToken in the web version is a business decision, not a technical one. Let me give you the context:

In order to generate the token, the user will have to expose a real key (usually a key with more permissions like the masterKey). If they do that using the front-end integration, our users will be exposing that info incorrectly. To prevent the users from making those mistakes, we only allowed them to generate the keys using this integration in the backend (node).

flevi29 commented 4 months ago

I see, Web Crypto can only be used in secure contexts, not sure if that changes anything, but I see why we need to separate this part of the code.

I still think it's the right move to use Web Crypto, because of WinterCG and the Minimum Common API that Node.js and Deno for example subscribe to otherwise.

I will try and separate tokens into a separate export, related to #1611.

brunoocasali commented 4 months ago

I see, Web Crypto can only be used in secure contexts, not sure if that changes anything, but I see why we need to separate this part of the code.

I still think it's the right move to use Web Crypto, because of WinterCG and the Minimum Common API that Node.js and Deno for example subscribe to otherwise.

I will try and separate tokens into a separate export, related to #1611.

but how can we protect the user to make those mistakes?

flevi29 commented 4 months ago

It's going to be a separate export. They're going to have to import from "meilisearch/tokens", or whatever we name it, otherwise it won't be bundled into their code in any way.

We will warn people to use it only on backend and any other way only if they really know what they're doing, use it at their own risk, and explain the risks involved. Maybe even console log some big warnings when they use it, although I can see this becoming a little annoying.

If they really wanted to they could implement this on their own on the front end via Web Crypto, as you can see it's not a complex change, there's only so much we can do for users to be "protected" from their own mistakes, and even so there are protections in place, for example they can only use Web Crypto in a browser in a secure context (HTTPS), although I know this might not be entirely foolproof.

Anyhow not having to worry about two separate JS clients is a worthwhile change in my opinion.