feat: Replace CryptoJS with Web Crypto
#2501
Conversation
|
🚀 Thanks for opening this pull request! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #2501 +/- ##
==========================================
- Coverage 99.88% 99.79% -0.10%
==========================================
Files 64 64
Lines 6220 6251 +31
Branches 1476 1478 +2
==========================================
+ Hits 6213 6238 +25
- Misses 7 13 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dplewis
changed the title
crypto-js with webcryptoCryptoJS with Web Crypto
|
@mtrezza Do you want to plan a Version 7.0.0 release? |
|
The thing is that we wouldn't be able to upgrade the JS SDK in Parse Server until December 2025 if we merged this. So we can't really merge this so easily with our current branch setup. We could tag this PR for merge on next major release. |
|
What does the server have to do with this? |
|
The Parse JS SDK is a dependency of Parse Sever. |
|
The server has a lot of dependencies, because the SDK breaks doesn’t mean the server breaks. In the case of this PR. You can’t log in on the server, there is no local storage like the browser, there is nothing to encrypt, so this doesn’t break the server. |
|
The difference is that the Parse JS SDK is a fully exposed API in Cloud Code, not merely a "hidden" dependency (which we want to change). The "hidden" dependencies are tested indirectly through the Parse Server API tests. The fully exposed JS SDK API may not be completely tested in Parse Server because not all of its APIs apply there, as you mentioned. Because of that risk, major version increments in tandem are safer. If we decide to go ahead and merge a major version upgrade of the JS SDK, we'd need to make sure that even ineffective APIs of the JS SDK do not break anything, for example methods that wouldn't make sense to be called in Cloud Code, but fail gracefully or have no effect at all. I agree that we could do the upgrade, but it requires some extra care. And it would need to be a dedicated major release, meaning we cannot combine it with other breaking changes, as we usually would do to minimize the number of major releases. Are there any other PRs that should go into a 7.0.0 release? |
|
For version 7.0.0
|
Signed-off-by: Diamond Lewis <[email protected]>
|
Which one of these are breaking and are there interdependencies? If only this crypto PR is breaking, then we can do a 7.0.0 release without waiting for the other ones. Since this is a "special" major release, it would make it also easier to control. |
📝 WalkthroughWalkthroughEncryption was migrated from CryptoJS to the Web Crypto API (and Node.js crypto.webcrypto). CryptoController APIs were changed to support async controllers; encryption/decryption are now async. Related core types, Parse user storage APIs, docs, tests, and packaging were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Parse
participant CoreManager
participant CryptoController
participant Storage
Note over App,Parse: Store flow (async)
App->>Parse: set secret / sign up
Parse->>CoreManager: getCryptoController()
CoreManager->>CryptoController: encrypt(json, secret) [async]
CryptoController-->>CoreManager: Promise<encryptedData>
CoreManager-->>Parse: encryptedData
Parse->>Storage: setItemAsync(encryptedData)
Note over App,Parse: Retrieve flow (async)
App->>Parse: currentUserAsync()
Parse->>Storage: getItemAsync()
Parse->>CoreManager: getCryptoController()
CoreManager->>CryptoController: decrypt(encryptedData, secret) [async]
CryptoController-->>CoreManager: Promise<json>
CoreManager-->>Parse: json
Parse-->>App: user object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Potential focal points for review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
[pre_merge_check_pass] The pull request description follows the repository template and includes all required sections. The Issue section contains a link to the related GitHub issue (#2494) and explains the problem being addressed (deprecated CryptoJS). The Approach section provides a detailed description of the changes being made, explicitly identifying multiple breaking changes and their implications. The Tasks section shows that both test additions and documentation updates have been completed. The description also includes a comprehensive migration guide for developers affected by the breaking changes, explaining two paths forward for users with previously encrypted data. [pre_merge_check_pass] The code changes successfully address the core objectives from linked issue #2494. The implementation replaces deprecated CryptoJS and react-native-crypto-js with the native Web Crypto API, enabling isomorphic encryption across Node.js and browsers without external dependencies. The [pre_merge_check_pass] The changes are focused on the stated objective of replacing CryptoJS with Web Crypto API. All modifications—including updates to the CryptoController implementation, API changes (removal of ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/CoreManager.ts (1)
32-42: Excellent discriminated union design for supporting both sync and async crypto controllers.The type definition properly:
- Uses
asyncproperty for type discrimination- Provides clear return types (
stringvsPromise<string>)- Updates parameter names for better clarity (
json,parseSecret)Consider making the
decryptmethod parameter naming consistent between variants - both should useparseSecretinstead ofsecretKeyfor the second parameter to match theencryptmethod.type CryptoController = | { async: 0; encrypt: (json: any, parseSecret: any) => string; - decrypt: (encryptedJSON: string, secretKey: any) => string; + decrypt: (encryptedJSON: string, parseSecret: any) => string; } | { async: 1; encrypt: (json: any, parseSecret: any) => Promise<string>; - decrypt: (encryptedJSON: string, secretKey: any) => Promise<string>; + decrypt: (encryptedJSON: string, parseSecret: any) => Promise<string>; };README.md (1)
118-147: Excellent documentation for the new encryption functionality.The documentation clearly explains:
- How to enable encryption using
Parse.secret- Web Crypto API usage and polyfill requirements
- Custom CryptoController implementation pattern
The custom controller example correctly demonstrates the new async interface with
async: 1and Promise-returning methods.Add language specifiers to the code blocks to address linting warnings:
-``` +```js // Set your key to enable encryption, this key will be passed to the CryptoController Parse.secret = 'MY_SECRET_KEY'; // or Parse.CoreManager.set('ENCRYPTED_KEY', 'MY_SECRET_KEY');-
+js
const CustomCryptoController = {</blockquote></details> <details> <summary>src/ParseUser.ts (1)</summary><blockquote> `1081-1117`: **Clean refactor to async/await pattern.** The conversion from promise chains to async/await improves readability and maintains the same functionality. The null checks and error handling are properly preserved. Consider extracting the user data processing logic (lines 1099-1113) into a helper function to reduce duplication with the synchronous `currentUser` method: ```diff + _processUserData(userData: any): ParseUser { + userData = JSON.parse(userData); + if (!userData.className) { + userData.className = '_User'; + } + if (userData._id) { + if (userData.objectId !== userData._id) { + userData.objectId = userData._id; + } + delete userData._id; + } + if (userData._sessionToken) { + userData.sessionToken = userData._sessionToken; + delete userData._sessionToken; + } + return ParseObject.fromJSON(userData) as ParseUser; + }, async currentUserAsync(): Promise<ParseUser | null> { // ... existing code ... - userData = JSON.parse(userData); - if (!userData.className) { - userData.className = '_User'; - } - if (userData._id) { - if (userData.objectId !== userData._id) { - userData.objectId = userData._id; - } - delete userData._id; - } - if (userData._sessionToken) { - userData.sessionToken = userData._sessionToken; - delete userData._sessionToken; - } - const current = ParseObject.fromJSON(userData) as ParseUser; + const current = DefaultController._processUserData(userData); currentUserCache = current; current._synchronizeAllAuthData(); return Promise.resolve(current); },types/Parse.d.ts (1)
53-70: Fix parameter naming inconsistency.The second parameter of the decrypt method is named
secretKeyin the type definition butparseSecretin the implementation. They should be consistent.setCryptoController(controller: { async: 0; encrypt: (json: any, parseSecret: any) => string; - decrypt: (encryptedJSON: string, secretKey: any) => string; + decrypt: (encryptedJSON: string, parseSecret: any) => string; } | { async: 1; encrypt: (json: any, parseSecret: any) => Promise<string>; - decrypt: (encryptedJSON: string, secretKey: any) => Promise<string>; + decrypt: (encryptedJSON: string, parseSecret: any) => Promise<string>; }): void; getCryptoController(): { async: 0; encrypt: (json: any, parseSecret: any) => string; - decrypt: (encryptedJSON: string, secretKey: any) => string; + decrypt: (encryptedJSON: string, parseSecret: any) => string; } | { async: 1; encrypt: (json: any, parseSecret: any) => Promise<string>; - decrypt: (encryptedJSON: string, secretKey: any) => Promise<string>; + decrypt: (encryptedJSON: string, parseSecret: any) => Promise<string>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.nvmrc(1 hunks)README.md(3 hunks)integration/test/ParseDistTest.js(1 hunks)integration/test/ParseReactNativeTest.js(1 hunks)integration/test/ParseUserTest.js(1 hunks)package.json(0 hunks)src/CoreManager.ts(1 hunks)src/CryptoController.ts(1 hunks)src/Parse.ts(1 hunks)src/ParseUser.ts(3 hunks)src/__tests__/Parse-test.js(2 hunks)src/__tests__/ParseUser-test.js(3 hunks)src/__tests__/browser-test.js(1 hunks)src/__tests__/react-native-test.js(3 hunks)types/CoreManager.d.ts(1 hunks)types/CryptoController.d.ts(1 hunks)types/Parse.d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/__tests__/browser-test.js (3)
src/__tests__/Parse-test.js (1)
require(13-13)src/__tests__/RESTController-test.js (1)
require(11-11)src/__tests__/react-native-test.js (1)
require(30-30)
src/__tests__/Parse-test.js (3)
src/__tests__/RESTController-test.js (1)
require(11-11)src/__tests__/browser-test.js (1)
require(16-16)src/__tests__/react-native-test.js (1)
require(30-30)
integration/test/ParseDistTest.js (3)
integration/test/cloud/main.js (1)
user(17-17)src/ParseUser.ts (1)
current(602-608)src/ParseGeoPoint.ts (1)
current(195-205)
src/CoreManager.ts (2)
integration/test/ParseReactNativeTest.js (1)
CryptoController(5-5)src/__tests__/ParseUser-test.js (1)
CryptoController(35-35)
src/ParseUser.ts (2)
src/ParseConfig.ts (2)
current(63-66)current(137-167)src/ParseSession.ts (1)
current(58-69)
src/__tests__/react-native-test.js (5)
src/__tests__/Parse-test.js (1)
require(13-13)src/__tests__/RESTController-test.js (1)
require(11-11)integration/test/ParseReactNativeTest.js (2)
require(4-4)CryptoController(5-5)src/__tests__/browser-test.js (1)
require(16-16)src/__tests__/ParseUser-test.js (1)
CryptoController(35-35)
🪛 markdownlint-cli2 (0.17.2)
README.md
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Link and image reference definitions should be needed
Unused link or image reference definition: "react-native-webview-crypto"
(MD053, link-image-reference-definitions)
175-175: Link and image reference definitions should be needed
Unused link or image reference definition: "types-parse"
(MD053, link-image-reference-definitions)
🔇 Additional comments (21)
.nvmrc (1)
1-1: LGTM - Node.js version update supports Web Crypto API migration.The Node.js version bump from 20.15.0 to 20.19.0 aligns well with the migration from CryptoJS to Web Crypto API, ensuring better support for native crypto modules and asynchronous cryptographic operations.
integration/test/ParseReactNativeTest.js (1)
54-57: LGTM - Correctly updated to asynchronous crypto operations.The change from synchronous to asynchronous decryption using
awaitproperly adapts to the new Web Crypto API-based CryptoController interface. This maintains the test's functionality while aligning with the SDK's migration from CryptoJS to Web Crypto API.src/__tests__/browser-test.js (1)
16-18: LGTM - Essential setup for Web Crypto API testing.Adding
TextEncoderandTextDecoderto the global scope is necessary for Web Crypto API operations in the test environment. This change is consistent with similar setups in other test files and ensures proper string-to-ArrayBuffer conversions during cryptographic operations.src/__tests__/Parse-test.js (2)
13-15: LGTM - Essential setup for Web Crypto API testing.Adding
TextEncoderandTextDecoderto the global scope is necessary for Web Crypto API operations in the test environment. This follows the same pattern established in other test files and ensures proper cryptographic functionality.
255-255: LGTM - Proper console output management in tests.Adding the console.log spy helps manage console output during the IndexedDB storage test, which is a standard testing practice for cleaner test output.
integration/test/ParseUserTest.js (1)
1158-1161: LGTM - Correctly updated to asynchronous crypto operations.The change from synchronous to asynchronous decryption using
awaitproperly adapts to the new Web Crypto API-based CryptoController interface. This maintains the test's functionality while ensuring compatibility with the SDK's migration from CryptoJS to Web Crypto API.integration/test/ParseDistTest.js (1)
87-101: LGTM! Good test coverage for the new async encryption functionality.The test correctly demonstrates the new encryption workflow:
- Uses
Parse.secretto enable encryption- Leverages
Parse.User.currentAsync()for async user retrieval when encryption is enabled- Properly cleans up the secret key after testing
This validates the end-to-end encryption behavior in a browser environment.
types/CryptoController.d.ts (1)
1-6: Type definitions correctly reflect the async CryptoController interface.The updated interface properly:
- Adds the
async: numberdiscriminator property- Changes methods to return
Promise<string>for async operations- Updates parameter names to match the implementation (
json,parseSecret)src/Parse.ts (1)
252-252: Good documentation improvement.Changing from
@propertyto@memberis more appropriate since this field has both getter and setter methods.src/__tests__/ParseUser-test.js (5)
779-792: LGTM! Correct migration to async methods.The changes properly replace synchronous calls with their async counterparts to support the new asynchronous crypto controller.
796-796: LGTM! Proper async/await usage for decrypt.The decrypt method is correctly awaited since the crypto controller is now asynchronous.
806-806: Good practice: Proper test cleanup.Storing and restoring the original CryptoController ensures test isolation and prevents side effects on other tests.
Also applies to: 856-856
830-830: LGTM! Correct discriminated union type implementation.The
async: 0property correctly identifies this as a synchronous crypto controller, matching the discriminated union type definition.
860-870: Good test coverage for error handling.This test ensures that attempting to use synchronous methods with async encryption properly throws an error, preventing runtime issues.
src/__tests__/react-native-test.js (2)
30-40: LGTM! Proper Web Crypto API setup for React Native.The setup correctly configures TextEncoder/TextDecoder and the global crypto object with Web Crypto API's subtle interface, which is required for the new encryption implementation.
56-60: LGTM! Test correctly validates async encryption.The test properly validates that the CryptoController uses the Web Crypto API's
subtle.encryptmethod.types/CoreManager.d.ts (1)
31-39: Excellent type design with discriminated union.The discriminated union type elegantly supports both synchronous and asynchronous crypto controllers, enabling backward compatibility while supporting the new Web Crypto API. The updated parameter names (
json,parseSecret,encryptedJSON) are more descriptive than the previous generic names.src/ParseUser.ts (2)
1002-1015: LGTM! Proper async conversion of updateUserOnDisk.The method correctly handles async encryption and storage operations. The configuration key change from
'ENCRYPTED_USER'to'ENCRYPTED_KEY'is consistently applied.
1044-1050: Good error handling for async crypto with sync calls.The error check prevents runtime issues by throwing a descriptive error when attempting synchronous access with async encryption enabled.
src/CryptoController.ts (2)
20-35: LGTM! Secure key derivation implementation.The key derivation uses industry-standard PBKDF2 with appropriate parameters (250,000 iterations, SHA-256, AES-GCM-256).
1-13: Environment detection and global APIs are covered by Node.js ≥18The
package.jsonalready enforcesengines.node: "18 || 19 || 20 || 22", and Node.js 18+ provides bothcrypto.webcryptoand globalbtoa/atobout of the box. No additional polyfills are needed—keeping the existing browser vs. Node.js branch logic is sufficient.
|
@mtrezza I fix the conflicts this is ready for review |
There was a problem hiding this comment.
LGTM, what of the following is a breaking change? Could you rewrite this into individual points? I'm a bit lost with the "Breaking Change" tag
- Remove
Parse.enableEncryptedUser(),Parse.encryptedUser,Parse.isEncryptedUseras they are not needed Breaking Change - Remove default support for React-Native, developers will now have to polyfill webcrypto Breaking Change
- CryptoJS is syncronous while Web Crypto is asyncronous meaning some functions won't be available when this feature is turned on, like when using AsyncStorage. Breaking Change
- Parse.User.current
- Parse.User.isCurrent
- Update Readme with instructions to create a custom crypto controller
- Remove dependency on
crypto-jsandreact-native-crypto-js - Developers will no longer have to install the optional dependency
crypto-jsalong side parse
Due to the encryption and decryption method being different, developers who used this feature before won't be able to decrypt users that are already logged in but not cached on disk. I'm not sure how many developers were using this feature as it was undocumented. There are two ways for devs to solve this.
- Forcefully logging out users (deleting sessions). This will clean up the local storage.
- Follow the readme instructions and use the old crypto-js CryptoController as your custom controller. This will require the developers to install those packages.
mtrezza
added
the
state:breaking
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
README.md (3)
33-33: TOC list item syntax is correct.The table of contents entry uses proper markdown list syntax with a hyphen.
128-128: Reference link syntax is correct.The markdown reference link syntax for
[react-native-webview-crypto]is properly formatted.
170-175: Reference links are properly used.All reference links in the bottom section are correctly used in the documentation above, so no cleanup is needed.
src/CryptoController.ts (2)
15-28: Buffer conversion functions are correctly implemented.The base64 conversion functions properly handle both browser and Node.js environments. The previous bug with
charCodeAt(null)has been fixed to usecharCodeAt(0).
65-74: Decrypt method should return parsed JSON object.The decrypt method currently returns a decoded string, but based on the interface and usage patterns, it should return the parsed JSON object.
async decrypt(encryptedJSON: string, parseSecret: any): Promise<string> { const buffer = base64ToBuffer(encryptedJSON); const salt = buffer.slice(0, 16); const iv = buffer.slice(16, 16 + 12); const data = buffer.slice(16 + 12); const key = await importKey(parseSecret); const aesKey = await deriveKey(key, salt, ['decrypt']); const decrypted = await webcrypto.subtle.decrypt({ name: 'AES-GCM', iv }, aesKey, data); - return decoder.decode(decrypted); + const jsonString = decoder.decode(decrypted); + return JSON.parse(jsonString); },
🧹 Nitpick comments (2)
README.md (2)
123-126: Add language specifier to code block.The code block should specify the language for proper syntax highlighting.
-``` +```js // Set your key to enable encryption, this key will be passed to the CryptoController Parse.secret = 'MY_SECRET_KEY'; // or Parse.CoreManager.set('ENCRYPTED_KEY', 'MY_SECRET_KEY'); -``` +```
132-146: Add language specifier to code block.The code block should specify the language for better readability and syntax highlighting.
-``` +```js const CustomCryptoController = { async: 1, async encrypt(json: any, parseSecret: any): Promise<string> { const encryptedJSON = await customEncrypt(json); return encryptedJSON; }, async decrypt(encryptedJSON: string, parseSecret: any): Promise<string> { const json = await customDecrypt(encryptedJSON); return JSON.stringify(json); }, }; // Must be called before Parse.initialize Parse.CoreManager.setCryptoController(CustomCryptoController); -``` +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)src/CryptoController.ts(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (Node 20, 20.15.1)
- GitHub Check: build (Node 18, 18.20.4)
- GitHub Check: build (Node 22, 22.4.1)
🔇 Additional comments (4)
src/CryptoController.ts (4)
1-13: LGTM! Cross-platform Web Crypto API setup.The conditional logic properly handles Web Crypto API access across browser and Node.js environments, with appropriate fallbacks for TextEncoder/TextDecoder.
30-31: LGTM! Secure key import implementation.The key import uses PBKDF2 which is appropriate for password-based key derivation.
33-45: Excellent security parameters for key derivation.The PBKDF2 configuration uses strong security parameters:
- 250,000 iterations (exceeds OWASP recommendations)
- SHA-256 hash function
- AES-GCM with 256-bit key length
This provides robust protection against brute force attacks.
47-63: LGTM! Secure encryption implementation.The encryption method follows cryptographic best practices:
- Uses cryptographically secure random values for salt and IV
- Proper salt and IV sizes (16 bytes salt, 12 bytes IV for GCM)
- Correctly concatenates salt + IV + ciphertext
- Uses AES-GCM which provides authenticated encryption
|
@mtrezza can we get this merged? V7 was released I thought it would be apart of it |
|
@dplewis Would love to merge this as well; to make this easy to understand for developers, could you take a look at my questions at #2501 (review)? |
3 participants
Pull Request
Issue
Active development of CryptoJS has been discontinued. Nowadays, NodeJS and modern browsers have a native
cryptomodule. This allows the SDK to have isomorphic encryption.Closes: #2494
Approach
Parse.enableEncryptedUser(),Parse.encryptedUser,Parse.isEncryptedUseras they are not needed Breaking Changecrypto-jsandreact-native-crypto-jscrypto-jsalong side parseDue to the encryption and decryption method being different, developers who used this feature before won't be able to decrypt users that are already logged in but not cached on disk. I'm not sure how many developers were using this feature as it was undocumented. There are two ways for devs to solve this.
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests