-
Notifications
You must be signed in to change notification settings - Fork 89
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 Node.js crypto to Web Crypto #1661
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
Oh well, |
Hmm, maybe I should do something so |
I tried to fix the typing issue for the |
This PR would make #1595 more or less obsolete, but there might be more things to consider before we can truly say that. |
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). |
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? |
It's going to be a separate export. They're going to have to import from 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. |
Pull Request
What does this PR do?