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

RN Cipher #1358

Merged
merged 1 commit into from
Jul 31, 2024
Merged

RN Cipher #1358

merged 1 commit into from
Jul 31, 2024

Conversation

fan-zhang-sv
Copy link
Contributor

@fan-zhang-sv fan-zhang-sv commented Jul 26, 2024

Summary

Reimplemented Cipher for RN using @noble/curves

How did you test your changes?

Manually + Unit Test

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jul 26, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

"@noble/hashes": "^1.4.0",
"clsx": "^1.2.1",
"eventemitter3": "^5.0.1",
"expo-standard-web-crypto": "^1.8.1",
Copy link
Contributor Author

@fan-zhang-sv fan-zhang-sv Jul 26, 2024

Choose a reason for hiding this comment

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

this is a tiny library to polyfill the crypto.getRandomValues from "expo-crypto" on RN, which is needed in "@noble/curves" to provide randomness. On Node env, this does nothing

Copy link
Contributor

Choose a reason for hiding this comment

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

this could be moved to optional dependencies instead since nothing besides React Native needs it

Copy link
Contributor

Choose a reason for hiding this comment

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

or ideally polyfilling Web Crypto should be done not by a library but a React Native application. It's not a library's job to polyfill functionality imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx @talentlessguy, agreed and we will omit the polyfill here!

also would like to know your view on the approach we took on RN Cipher if possible. Generally we will ask consumer to provide randomness and use @noble/curves to achieve diffie hellman, do you have any concern or maybe you know any potential hiccup in this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps there's no guarantee that provided randomness is random enough and isn't using something like Math.random()

@@ -14,7 +14,14 @@ export type EncryptedData = {
cipherText: ArrayBuffer;
};

export type MobileEncryptedData = {
iv: Uint8Array;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON.stringify cannot handle ArrayBuffer, using Uint8Array to transmit the data

@fan-zhang-sv
Copy link
Contributor Author

image
i tried!

@fan-zhang-sv fan-zhang-sv merged commit e497878 into master Jul 31, 2024
8 checks passed
@fan-zhang-sv fan-zhang-sv deleted the felix/rn-cipher branch July 31, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants