API signTransaction accepts arbitrary blobs #912

aristidesstaffieri · 2023-07-25T16:10:06Z

This extends signTransaction to be able to do sign arbitrary blobs encoded as b64.
It splits our singing workflows into either tx or blob and splits the review screen as well.

Adds new signBlob which is an equivalent to signTransaction, which signs arbitrary base64 data blobs.

…blob

piyalbasu · 2023-07-26T20:35:30Z

extension/src/background/messageListener/freighterApiMessageListener.ts

+    // try to build a tx xdr, if you cannot then assume the user wants to sign an arbitrary blob
+    let encodedBlob = ""
+    try {
+      const transaction = SDK.TransactionBuilder.fromXDR(


Yeah, I like your idea of splitting this up into a new API method called signBlob. I think signing a blob is a different enough, and perhaps more of a super-user feature, that we can just have its own dedicated method for it rather than splitting it here on the try/catch

Added in f3865d3

100% agreed: signing arbitrary data is very dangerous

@Shaptic can you expand your thoughts on this? I'm trying to think through ways to make this safer. I think we can safely decode the b64 blob and display it in the signing window/make the window clearly show that this is not XDR, but can you think of anything else we can do to make this safer for end users?

I think a warning around "Do not sign random blobs unless you know what you're signing/doing, trust their source, etc." would be really helpful! As in "we have no idea what this blob represents so be aware of that"

@piyalbasu I added these kind of off the cuff, any thoughts or additions for this warning?

It also shows the blob you are signing underneath

Nice - I think this is a great start. We can run this by Bruno to see if he wants to adjust this copy at all

piyalbasu · 2023-07-26T20:44:10Z

extension/src/popup/views/SignTransaction/index.tsx

+  blob: BlobToSign
+}
+
+const SignBlobBody = ({ blob }: SignBlobBodyProps) => {


what are the differences between this and SignTxBody? It seems like there's some repeated logic here. Is it possible to either combine these into one function or split out the reusable stuff? (for ex: selecting the correct account, checking password, showing warning messages, etc.)

yeah I think so, looking at it more I think we can probably define something like isBlob because the main difference is you don't show some elements for blobs and change some copy slightly.

I refactored this in 440302a to move common state/dom up to the parent, there is still some repetition in the markup but it seemed better to me to have this bit of repetition for now vs trying to make one common render with more complicated logic to switch between blob or tx mode, I suspect that the render will only get more complicated as we expand on how we display common blobs. What do you think of this approach? @piyalbasu

aristidesstaffieri · 2023-07-28T15:56:23Z

@piyalbasu one detail here is that blob must be b64 encoded right now, I can't think of any problems about this right away but wanted to bring it up.

extension/src/popup/views/SignTransaction/index.tsx

piyalbasu · 2023-07-28T16:41:10Z

extension/src/popup/views/SignTransaction/index.tsx

+  handleApprove: (signAndClose: () => Promise<void>) => () => Promise<void>
+}
+
+const SignBlobBody = ({ publicKey, allAccounts, isDropdownOpen, handleApprove, isConfirming, accountNotFound, rejectAndClose, currentAccount, setIsDropdownOpen, setIsPasswordRequired, isPasswordRequired, blob, isExperimentalModeEnabled, t, dispatch, hwStatus, isHardwareWallet, setStartedHwSign }: SignBlobBodyProps) => {


what do you think about moving SignBlobBody and SignTxBody to their own components files? Might make this file a little easier to digest

Also, if you move these to their own files - you won't need to pass t and dispatch, you can just re-instantiate them with useTranslation and useDispatch respectively

yup, can do on both 👍

I split out the body components in their own files, moved t and dispatch into the body components and formatted the files manually, all in 5f96ea1

* API signTransaction accepts arbitrary blobs (#912) * adds signBlob workflows and allows for signTransaction to take a b64 blob * cleans up SignTransaction and splits blob and tx signing review * cleans up SignTransaction and splits blob and tx signing review * Added translations * add domain to blob signing, split blob and tx signing body components * Added translations * refactors SignTransaction to abstract common state to parent * adds new api, signBlob * splits out sign blob and sign tx bodies * restore var name in signTransaction * adds blob warning and blob preview to sign blob flow * Added translations * fixes data blob styling and placement * Added translations * manually format changes * fixes grammar in signing warning * Added translations * Feature/wal 509 remove domain connection (#911) * update slack notify as old version was erroring * add UI to remove connected domains * Added translations * add a separate service for saveAllowList and add an empty state * clean up allowList value that UI receives * fixes bad references to request/response keys in the listener layer * renames args, drops unused args, and splits Request interfaces for tx and blobs * renames forgotten reference in response handler * makes options optional for signBlob * bump freighter-api version for next release (#925) * set current account after accountMap has been generated (#927) * set current account after accountMap has been generated * de-dupe accountsMap logic --------- Co-authored-by: aristides <aristides.staffieri@stellar.org>

adds signBlob workflows and allows for signTransaction to take a b64 …

f7662a4

…blob

aristidesstaffieri added the wip not for merging yet label Jul 25, 2023

aristidesstaffieri self-assigned this Jul 25, 2023

aristidesstaffieri added 5 commits July 26, 2023 11:48

cleans up SignTransaction and splits blob and tx signing review

e61d061

cleans up SignTransaction and splits blob and tx signing review

20b3485

Added translations

d3e3903

add domain to blob signing, split blob and tx signing body components

606e398

Added translations

7ac3c98

aristidesstaffieri changed the title ~~[WIP] API signTransaction accepts arbitrary blobs~~ API signTransaction accepts arbitrary blobs Jul 26, 2023

aristidesstaffieri requested a review from piyalbasu July 26, 2023 18:51

aristidesstaffieri removed the wip not for merging yet label Jul 26, 2023

aristidesstaffieri marked this pull request as ready for review July 26, 2023 18:51

piyalbasu reviewed Jul 26, 2023

View reviewed changes

aristidesstaffieri mentioned this pull request Jul 27, 2023

p10 auth stellar/soroban-react-atomic-swap#4

Merged

refactors SignTransaction to abstract common state to parent

cddc615

aristidesstaffieri force-pushed the feature/sign-arbitrary-blobs branch from e6ae94f to cddc615 Compare July 28, 2023 15:51

aristidesstaffieri requested a review from piyalbasu July 28, 2023 16:26

adds new api, signBlob

f037bdc

aristidesstaffieri force-pushed the feature/sign-arbitrary-blobs branch from f3865d3 to f037bdc Compare July 28, 2023 16:33

piyalbasu reviewed Jul 28, 2023

View reviewed changes

extension/src/popup/views/SignTransaction/index.tsx Outdated Show resolved Hide resolved

piyalbasu reviewed Jul 28, 2023

View reviewed changes

aristidesstaffieri force-pushed the feature/sign-arbitrary-blobs branch from 69a41d3 to 5f96ea1 Compare July 28, 2023 17:08

splits out sign blob and sign tx bodies

440302a

aristidesstaffieri force-pushed the feature/sign-arbitrary-blobs branch from 5f96ea1 to 440302a Compare July 28, 2023 17:16

restore var name in signTransaction

aa487f9

aristidesstaffieri mentioned this pull request Jul 28, 2023

adds signBlob to support upcoming Freighter feature Creit-Tech/Stellar-Wallets-Kit#1

Merged

aristidesstaffieri added 2 commits July 31, 2023 13:53

adds blob warning and blob preview to sign blob flow

4dd4d0e

Added translations

2ea1d73

aristidesstaffieri added 3 commits July 31, 2023 14:43

fixes data blob styling and placement

0e92315

Added translations

f968503

manually format changes

d93eac8

aristidesstaffieri changed the base branch from master to release/5.2.6 July 31, 2023 20:49

aristidesstaffieri requested a review from piyalbasu July 31, 2023 20:49

piyalbasu approved these changes Jul 31, 2023

View reviewed changes

aristidesstaffieri added 2 commits July 31, 2023 16:05

fixes grammar in signing warning

e188bff

Added translations

3fef213

piyalbasu merged commit 4f8e48f into release/5.2.6 Aug 1, 2023
1 check passed

piyalbasu deleted the feature/sign-arbitrary-blobs branch August 1, 2023 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API signTransaction accepts arbitrary blobs #912

API signTransaction accepts arbitrary blobs #912

aristidesstaffieri commented Jul 25, 2023 •

edited

Loading

piyalbasu Jul 26, 2023

aristidesstaffieri Jul 28, 2023

Shaptic Jul 28, 2023

aristidesstaffieri Jul 28, 2023

Shaptic Jul 28, 2023

aristidesstaffieri Jul 31, 2023

aristidesstaffieri Jul 31, 2023

piyalbasu Jul 31, 2023

piyalbasu Jul 26, 2023

aristidesstaffieri Jul 26, 2023

aristidesstaffieri Jul 28, 2023 •

edited

Loading

aristidesstaffieri commented Jul 28, 2023

piyalbasu Jul 28, 2023

piyalbasu Jul 28, 2023

aristidesstaffieri Jul 28, 2023

aristidesstaffieri Jul 28, 2023

API signTransaction accepts arbitrary blobs #912

API signTransaction accepts arbitrary blobs #912

Conversation

aristidesstaffieri commented Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aristidesstaffieri Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

aristidesstaffieri commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aristidesstaffieri commented Jul 25, 2023 •

edited

Loading

aristidesstaffieri Jul 28, 2023 •

edited

Loading