refactor: remove crypto-js which is now deprecated #5866

jeanregisser · 2024-08-30T16:17:14Z

Description

This removes crypto-js which is now discontinued:

Active development of CryptoJS has been discontinued. This library is no longer maintained.

Nowadays, NodeJS and modern browsers have a native Crypto module. The latest version of CryptoJS already uses the native Crypto module for random number generation, since Math.random() is not crypto-safe. Further development of CryptoJS would result in it only being a wrapper of native Crypto. Therefore, development and maintenance has been discontinued, it is time to go for the native crypto module.

Bonus, because it now uses the native crypto module (via react-native-quick-crypto) AES encryption/decryption should be faster. Though the gains are probably not noticeable given the small use we have.

Note: there's an important change for tests too, they now all use real encryption.

I feel this is less surprising than what we used to do.
I was able to refactor tests that expected mocked encryption without too much trouble

Test plan

Updated tests (ensuring backward compat with CryptoJS encrypted strings)
Manually tested existing keychain items encrypted by CryptoJS can still be decrypted in Valora

Related issues

Part of RET-1185

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-08-30T16:26:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (737cfd5) to head (cd2209b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5866    +/-   ##
========================================
  Coverage   87.60%   87.60%            
========================================
  Files         735      736     +1     
  Lines       31590    31616    +26     
  Branches     5675     5675            
========================================
+ Hits        27673    27696    +23     
+ Misses       3873     3697   -176     
- Partials       44      223   +179

Files with missing lines	Coverage Δ
src/backup/utils.ts	`92.64% <100.00%> (-0.11%)`	⬇️
src/utils/aes.ts	`100.00% <100.00%> (ø)`
src/web3/KeychainAccounts.ts	`97.26% <100.00%> (-0.02%)`	⬇️

... and 75 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 737cfd5...cd2209b. Read the comment docs.

jeanregisser · 2024-08-30T16:26:25Z

src/utils/aes.ts

+// This file was copied from https://github.com/RaisinTen/aes-crypto-js/blob/2978af8e004d47539d767e751def003fe134b6e2/index.js
+// and modified slightly for TS compatibility.


I found this while looking for a recommended migration path in brix/crypto-js#468

I chose to copy the implementation instead of importing it from NPM as it's critical to the correct behavior of Valora.
And wanted to avoid yet another unmaintained package in the future 😄

I'm sure this is all probably totally fine, but something about checking in code that operates at this low-level of abstraction makes me nervous 😅 This is all totally backwards compatible with data that's already been stored and encrypted by our previous library, right? That is, you can encrypt data the old way, decrypt it the new way, and it will decrypt as we'd expect?

Yes, I carefully added tests for backward compatibility and ensured everything was fine by manually testing too.
Well of course there's always room for subtle bugs, but these should be minimized now.

jeanregisser · 2024-08-30T17:16:36Z

src/viem/getLockableWallet.test.ts

-// Use real encryption
-jest.unmock('crypto-js')


Real encryption is now the default, since __mocks__/crypto-js.ts was removed, which was mocking encryption and decryption.

jophish

Looks fantastic! Glad it was an easy migration, and happy to get rid of this :)

jophish · 2024-08-30T17:19:20Z

package.json

@@ -124,7 +124,6 @@
    "bignumber.js": "^9.1.2",
    "clevertap-react-native": "^2.2.1",
    "country-data": "^0.0.31",
-    "crypto-js": "^4.2.0",


jophish · 2024-08-30T17:24:21Z

src/utils/aes.ts

+// This file was copied from https://github.com/RaisinTen/aes-crypto-js/blob/2978af8e004d47539d767e751def003fe134b6e2/index.js
+// and modified slightly for TS compatibility.


I'm sure this is all probably totally fine, but something about checking in code that operates at this low-level of abstraction makes me nervous 😅 This is all totally backwards compatible with data that's already been stored and encrypted by our previous library, right? That is, you can encrypt data the old way, decrypt it the new way, and it will decrypt as we'd expect?

jophish · 2024-08-30T17:25:11Z

src/pincode/authentication.test.ts

+  const originalAes = jest.requireActual('src/utils/aes')
+
+  return {
+    ...originalAes,


Why do we need to return the original import here? It seems like the only two functions in that file are aesEncrypt and aesDecrypt, which we're mocking anyways.

This is in case it changes in the future and more things are exported.
Less likely that we'd need to touch the mock.

…rypto-js

### Description Cloud Account Backup broke in #5866. That PR changed `global.crypto` to be polyfilled by `react-native-quick-crypto` and the version of `@toruslabs/eccrypto` pulled by our Cloud Account Backup, wasn't working out-of-the box with it. However it got fixed in torusresearch/eccrypto#40 and landed in `@toruslabs/eccrypto >= 5.0.2`. So this PR upgrades the `@toruslabs` libs so it pulls the latest version of `@toruslabs/[email protected]` which works with `react-native-quick-crypto`. It's using new major versions. Scanning through the [release notes](https://github.com/torusresearch/torus.js/releases), the main impact for us was the way to import the module. But let me know if there's an important detail I've missed. Additionally, I found there was a bug in `react-native-quick-crypto`, `getAuthTag` was returning an `ArrayBuffer` instead of a `Buffer` leading to an incorrect encoding in the final encrypted string. I contributed a fix upstream (margelo/react-native-quick-crypto#443) and created a patch to use until it's merged. And made a slight change to the wallet code so we can decrypt data if the `authTag` was serialized as an `ArrayBuffer` (e.g. `"1,2,3,4,..."`) instead of the expected base64. This replaces #5934 ### Test plan - Tests pass - Manually tested: - Restore from Cloud Account Backup works - Setup Cloud Account Backup works - Delete Cloud Account Backup works ### Related issues See Slack: [thread 1](https://valora-app.slack.com/archives/C04B61SJ6DS/p1725642273567639), [thread 2](https://valora-app.slack.com/archives/C025X4WJT09/p1725666426992419). ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

jeanregisser commented Aug 30, 2024

View reviewed changes

jophish approved these changes Aug 30, 2024

View reviewed changes

jeanregisser force-pushed the jeanregisser/use-rn-quick-crypto branch from 3ffb507 to 4290d87 Compare September 2, 2024 10:33

Base automatically changed from jeanregisser/use-rn-quick-crypto to main September 2, 2024 12:00

jeanregisser force-pushed the jeanregisser/remove-crypto-js branch from aead8c2 to de8507c Compare September 2, 2024 12:16

jeanregisser enabled auto-merge September 2, 2024 12:22

refactor: remove crypto-js which is now deprecated

aa68a40

jeanregisser force-pushed the jeanregisser/remove-crypto-js branch from de8507c to aa68a40 Compare September 2, 2024 13:38

jeanregisser added 2 commits September 3, 2024 16:39

Fix e2e, because an unexpected Buffer impl was being used

73484cd

Merge remote-tracking branch 'origin/main' into jeanregisser/remove-c…

cd2209b

…rypto-js

jeanregisser added this pull request to the merge queue Sep 3, 2024

Merged via the queue into main with commit d4171f3 Sep 3, 2024
15 checks passed

jeanregisser deleted the jeanregisser/remove-crypto-js branch September 3, 2024 15:20

jeanregisser mentioned this pull request Sep 9, 2024

fix: restore Cloud Account Backup #5941

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove crypto-js which is now deprecated #5866

refactor: remove crypto-js which is now deprecated #5866

jeanregisser commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

jeanregisser Aug 30, 2024 •

edited

Loading

jophish Aug 30, 2024

jeanregisser Sep 2, 2024

jeanregisser Aug 30, 2024 •

edited

Loading

jophish left a comment

jophish Aug 30, 2024

jophish Aug 30, 2024

jophish Aug 30, 2024

jeanregisser Sep 2, 2024 •

edited

Loading

		// This file was copied from https://github.com/RaisinTen/aes-crypto-js/blob/2978af8e004d47539d767e751def003fe134b6e2/index.js
		// and modified slightly for TS compatibility.

refactor: remove crypto-js which is now deprecated #5866

refactor: remove crypto-js which is now deprecated #5866

Conversation

jeanregisser commented Aug 30, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

jeanregisser Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

jophish Aug 30, 2024

Choose a reason for hiding this comment

jeanregisser Sep 2, 2024

Choose a reason for hiding this comment

jeanregisser Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

jophish left a comment

Choose a reason for hiding this comment

jophish Aug 30, 2024

Choose a reason for hiding this comment

jophish Aug 30, 2024

Choose a reason for hiding this comment

jophish Aug 30, 2024

Choose a reason for hiding this comment

jeanregisser Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

jeanregisser commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

jeanregisser Aug 30, 2024 •

edited

Loading

jeanregisser Aug 30, 2024 •

edited

Loading

jeanregisser Sep 2, 2024 •

edited

Loading