-
Notifications
You must be signed in to change notification settings - Fork 100
Updated Handler for appencryptionrequest, added z_functions ported from React Native Verus #228
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
base: generic-request-changes
Are you sure you want to change the base?
Updated Handler for appencryptionrequest, added z_functions ported from React Native Verus #228
Conversation
… decrypt from react-native-zcash
Add validation logic to prevent spoofing attacks in AppEncryptionRequest flow: - If requestSignerID is in wallet: appOrDelegatedID can be any identity (user is delegating) - If requestSignerID is NOT in wallet: appOrDelegatedID must equal requestSignerID or be absent (app can only claim to be itself) New helper functions: - isIdentityInWallet: checks if identity's primaryaddresses include wallet address - validateRequestIdentities: enforces the above rules, returns validated appID - validateResponseSignerID: validates user's responding identity is in wallet Handler does the following: - Validates identities before key derivation - Builds full AppEncryptionResponseDetails (ivk, evk, address, optionally spending_key) - Encrypts response to app's z-address - Pushes result to response array for envelope construction
-- fixed typo in zGet validate response signer in now the main handler of appEncryptionHandler -- changed the top level import on zfunctions from react native zcash (old) to react native verus (new)
… on miketout changes -- added checks in appEncryptionRequestValidator -- requests are handled in appEncryptionRequestHandler -- returns encrypted response and handledIndices
-- returned encrypted results directly -- code clean up
…stVDXFObject function - using the ESK for deriving channel keys instead of seed - removed the app or delegated id check that was in appEncryptionRequestValidator because that would go on envelope level.
michaeltout
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments
.yarn/releases/yarn-classic.cjs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file, we do not use yarn classic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, will push when make all changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo this change, we should point to the VerusCoin repo and pointing to biz's repo was temporary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see it pointing to any repo except for the ./m2/repository
| const keys = derivationResult.result; | ||
|
|
||
| // build response details | ||
| const responseDetails = AppEncryptionResponseDetails.fromJson({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using the fromJson function, and use the constructor while passing in and formatting the data as needed by the constructor
| // encrypt response to app's z-address | ||
| const encryptTo = request.encryptToZAddress; | ||
|
|
||
| if (!encryptTo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to handle encryptToZAddress here, that'll be handled when processing the entire response at the very end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I got this confused with encryptResponseToAddress on the generic request, I'm actually not sure what to do to handle the encryptToZAddress on the appEncryptionRequestHandler, it still might need to be handled in a different way than what you're doing though, because to return an encrypted AppEncryptionResponse, you probably need to return a DataDescriptorOrdinalVDXFObject instead of an AppEncryptionResponseOrdinalVDXFObject. AppEncryptionResponseOrdinalVDXFObject would only be if the response was unencrypted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into the code it seems this encryptToZAddress is not actually optional, I will make it optional with a flag and then you can check something like hasEncryptToZAddress to see if it is present or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waiting on this
| // extract envelope-level info | ||
| const systemID = request.signature.systemID.toIAddress(); | ||
| const requestSignerID = request.signature.identityID.toIAddress(); | ||
| const appOrDelegatedID = getAppOrDelegatedID(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not do any verification of the appOrDelegatedID in this request, that will be verified when the request comes in, before processing occurs. You can remove things like validateRequestIdentities
.yarnrc.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to this file should be removed
shim.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add shim.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to this file should be removed, there is no reason to change the packages or yarn lock files
| VrpcProvider.initEndpoint(coinObj.system_id, coinObj.vrpc_endpoints[0]); | ||
|
|
||
| // process the request | ||
| const encryptedResponse = await processAppEncryptionRequest({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, processAppEncryptionRequest returns hex, this will cause a runtime error when hex is passed into the data field of AppEncryptionResponseOrdinalVDXFObject, as it is expecting AppEncryptionResponseDetails
- Use AppEncryptionResponseDetails constructor instead of fromJson()
- Convert string keys to proper types (Buffer, SaplingExtendedViewingKey,
SaplingPaymentAddress, SaplingExtendedSpendingKey)
- Remove validateRequestIdentities and isIdentityInWallet functions
- Validation now handled at envelope level before handler is called
- Fix response wrapping to prevent runtime error
- Unencrypted: return AppEncryptionResponseOrdinalVDXFObject with
AppEncryptionResponseDetails object (not hex string)
- Encrypted: return DataDescriptorOrdinalVDXFObject with
DataDescriptor containing encrypted data
- Rename encryptToZAddress to encryptResponseToAddress
- Add hasEncryptResponseToAddress() helper with method/property fallback
- Add getEncryptResponseToAddress() helper for address extraction
No description provided.