Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid #18200

yrliou · 2023-04-21T23:28:06Z

Resolves brave/brave-browser#29798

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Switch to Solana devnet and visit https://pwgoom.csb.app/.
Click Sign Transaction (Invalid blockhash), wallet panel won't prompt and should see
[error] signTransactionWithInvalidBlockhash: {"code":-32603,"message":"Blockhash is invalid or can not be validated"} on the screen.
Click Sign All Transactions (Invalid blockhash), wallet panel won't prompt and should see
[error] signAllTransactionsWithInvalidBlockhash: {"code":-32603,"message":"Blockhash is invalid or can not be validated"} on the screen.
Click Sign Transaction and Sign All Transactions (multiple). In both cases, transaction signing prompt should be shown, and shouldn't see blockhash is invalid error. Note: It could fail after you approve for other reasons like insufficient funds or no record of a prior credit, which is fine.

There is also a test case provided in the hackerone report, but requires manual setup, please reach out to me for the zip of the repo and how to configure it.
(I did verify it locally, however, I think the tests I added in our test dApp should be enough. So maybe just test below on one platform to be extra safe.)
For testing the dApp in the hackerone report:

Get a fresh profile and setup wallet with two Solana accounts created. Alternatively, you can also use an existing wallet with two Solana accounts.
Get the repo copy from @yrliou and run yarn setup in that repo folder. (Node v18 is needed.)
In solana-cross-cluster-poc/src/cross-cluster.js, replace the address in accounts with address of your Solana Account 1 (around line 9), and replace address used asattackerPubkey with address of your Solana Account 2 (around line 77).
Save the change and run yarn start under the repo folder.
Back to browser, make sure Default Solana Wallet setting is set to Brave Wallet and set active Solana network to mainnet beta.
Visit http://localhost:9011/, connect to the wallet, click SEND FUNDS…, should see prompt for transaction signing. Approve it. Depends on whether you have funds available in your account, you’ll either see error later due to insufficient fund when the dApp trying to send transaction, or you'll see it successfully sends out a transaction and gives you the tx hash that you can query on explorer if you have funds enough to complete the transaction.
Switch active network to devnet, reload the page and click SEND FUNDS…, wait for error {"code":-32603,"message":"Blockhash is invalid or can not be validated"} to show up and you should not see any prompt for transaction signing.

yrliou · 2023-04-22T00:33:17Z

browser/brave_wallet/solana_provider_browsertest.cc

@@ -305,6 +292,13 @@ bool WaitForWalletBubble(content::WebContents* web_contents) {

  return tab_helper->IsShowingBubble();
 }
+
+void CloseWalletBubble(content::WebContents* web_contents) {


Had to add it to multiple Call* functions otherwise WaitForWalletBubble isn't working as what we expected because it was never dismissed after the previous request (we use NotifyXXXProcessed directly so UI doesn't really get dismissed).

yrliou · 2023-04-22T00:41:01Z

components/brave_wallet/browser/solana_requests.cc

+  params.Append(blockhash);
+
+  base::Value::Dict configuration;
+  configuration.Set("commitment", commitment ? *commitment : "processed");


Use processed as default so newer blockhash won't be invalid.

components/brave_wallet/browser/solana_requests_unittest.cc

browser/brave_wallet/solana_provider_browsertest.cc

mkarolin · 2023-04-22T04:03:07Z

components/resources/wallet_strings.grdp

@@ -9,6 +9,7 @@
  <message name="IDS_WALLET_EXPECTED_SINGLE_PARAMETER" desc="The text of the response for wallet_addEthereumChain call with empty parameters">Expected single, object parameter.</message>
  <message name="IDS_WALLET_CHAIN_EXISTS" desc="The text of the response for wallet_addEthereumChain call with existing chain id">This Chain ID is currently used</message>
  <message name="IDS_WALLET_INTERNAL_ERROR" desc="The text of the response for wallet_addEthereumChain call with internal error">An internal error has occurred</message>
+  <message name="IDS_WALLET_INVALID_BLOCKHASH_ERROR" desc="The text of the response for Solana signTransaction and signAllTransactions with invalid blockhash or unable to validate due to network or RPC error">Blockhash is invalid or unable to be validated</message>


Suggestion: Blockhash is invalid or can not be validated

Thanks, updated as suggested.

mkarolin

strings ++ (with a suggestion)

…nvalid

yrliou · 2023-04-22T05:30:53Z

macOS had a known test failure: P3ARotationSchedulerTest.ConstellationRotation.

wknapik · 2023-04-22T15:21:34Z

Force-merged on request https://bravesoftware.slack.com/archives/C7VLGSR55/p1682175822196279?thread_ts=1682072580.417689&cid=C7VLGSR55

kdenhartog · 2023-04-24T01:08:22Z

Screen.Recording.2023-04-24.at.1.06.07.PM.mov

@yrliou from what I'm seeing locally, this is breaking proper transaction signing now. Is this what you saw when you tested the happy path case as well?

yrliou · 2023-04-24T03:58:45Z

@kdenhartog I didn't use their dApp to test the happy path but used our own dApp's normal cases which works fine, can test theirs tomorrow.

yrliou · 2023-04-24T04:58:43Z

@kdenhartog I just tested it locally on master, signing works on mainnet and failed due to blockhash invalid error on devnet.

kdenhartog · 2023-04-24T09:34:48Z

Ok must just be a local failure for me - thanks for double checking. I just tested on the PR build rather than master so that's probably the issue for me. I'll go ahead and close out the H1 issue - thanks for fixing so quickly!

yrliou · 2023-04-24T18:09:26Z

@kdenhartog PR build should work too, not sure if it's environment or the some mistakes during testing, feel free to reach out to me if it's broken for you on latest nightly.

…nvalid (#18200) * Add Solana IsBlockhashValid JSON-RPC support * Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid

srirambv · 2023-04-25T05:14:50Z

Verification passed on

Brave	1.52.69 Chromium: 113.0.5672.53 (Official Build) nightly (64-bit)
Revision	`12f5dac35d12e8f4e72d7dd11df557ef93bc046f-refs/branch-heads/5672@{#703}`
OS	Windows 11 Version 22H2 (Build 22621.1555)

Verified steps from PR using https://pwgoom.csb.app
Verified clicking on Sign Transaction (Invalid blockhash) doesn't show the panel for approval but instead shows [error] signTransactionWithInvalidBlockhash: {"code":-32603,"message":"Blockhash is invalid or can not be validated"}
Verified clicking on Sign All Transaction (Invalid blockhash) doesn't show the panel for approval but instead shows [error] signAllTransactionsWithInvalidBlockhash: {"code":-32603,"message":"Blockhash is invalid or can not be validated"}
Verified Sign Transaction (Invlalid blockhash) and Sign All Transactions (Invalid blockhash) fails when both Solana Mainnet and Solana Devnet are set as active network

18200-TestDapp.mp4

Verification passed on

Brave	1.52.69 Chromium: 113.0.5672.53 (Official Build) nightly (64-bit)
Revision	`12f5dac35d12e8f4e72d7dd11df557ef93bc046f-refs/branch-heads/5672@{#703}`
OS	Linux

Verified steps from PR of using the PoC setup
Verified when no funds in Mainnet and attempt to send funds shows Result: Error: failed to send transaction: Transaction simulation failed: Attempt to debit an account but found no record of a prior credit. error message
Verified when switching to Devnet and attempt to send funds shows Result: Error: Blockhash is invalid or can not be validated

18200.mp4

Verified Sign Transaction and Sign All Transactions (multiple) works as expected.

18200-Sign.Transactions.mp4

…nvalid (uplift to 1.51.x) (#18217) Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid (#18200) * Add Solana IsBlockhashValid JSON-RPC support * Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid

yrliou self-assigned this Apr 21, 2023

yrliou requested review from a team as code owners April 21, 2023 23:28

github-actions bot added feature/web3/wallet feature/web3/wallet/core labels Apr 21, 2023

yrliou force-pushed the validate_blockhash_from_dapps branch from e35f306 to c154ac6 Compare April 22, 2023 00:16

yrliou commented Apr 22, 2023

View reviewed changes

darkdh reviewed Apr 22, 2023

View reviewed changes

components/brave_wallet/browser/solana_requests_unittest.cc Outdated Show resolved Hide resolved

browser/brave_wallet/solana_provider_browsertest.cc Show resolved Hide resolved

yrliou force-pushed the validate_blockhash_from_dapps branch 3 times, most recently from d3f45e4 to 218e798 Compare April 22, 2023 02:44

darkdh approved these changes Apr 22, 2023

View reviewed changes

Add Solana IsBlockhashValid JSON-RPC support

ac70214

yrliou force-pushed the validate_blockhash_from_dapps branch 2 times, most recently from 7c910fd to 21bcc78 Compare April 22, 2023 03:57

mkarolin reviewed Apr 22, 2023

View reviewed changes

mkarolin approved these changes Apr 22, 2023

View reviewed changes

Reject Solana SignTransaction/SignAllTransactions when blockhash is i…

bf6ffdf

…nvalid

yrliou force-pushed the validate_blockhash_from_dapps branch from 21bcc78 to bf6ffdf Compare April 22, 2023 04:21

StephenHeaps approved these changes Apr 22, 2023

View reviewed changes

wknapik merged commit 297c1d4 into master Apr 22, 2023

wknapik deleted the validate_blockhash_from_dapps branch April 22, 2023 15:21

github-actions bot added this to the 1.52.x - Nightly milestone Apr 22, 2023

kdenhartog restored the validate_blockhash_from_dapps branch April 24, 2023 00:41

kdenhartog deleted the validate_blockhash_from_dapps branch April 24, 2023 00:42

yrliou added a commit that referenced this pull request Apr 24, 2023

Reject Solana SignTransaction/SignAllTransactions when blockhash is i…

85a39fd

…nvalid (#18200) * Add Solana IsBlockhashValid JSON-RPC support * Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid

yrliou mentioned this pull request Apr 24, 2023

Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid (uplift to 1.51.x) #18217

Merged

7 tasks

srirambv mentioned this pull request May 2, 2023

[hackerone]: blind cross chain signing should be prevented brave/brave-browser#29798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid #18200

Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid #18200

yrliou commented Apr 21, 2023 •

edited

Loading

yrliou Apr 22, 2023

yrliou Apr 22, 2023

mkarolin Apr 22, 2023

yrliou Apr 22, 2023

mkarolin left a comment

yrliou commented Apr 22, 2023

wknapik commented Apr 22, 2023

kdenhartog commented Apr 24, 2023

yrliou commented Apr 24, 2023

yrliou commented Apr 24, 2023 •

edited

Loading

kdenhartog commented Apr 24, 2023

yrliou commented Apr 24, 2023

srirambv commented Apr 25, 2023 •

edited

Loading

Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid #18200

Reject Solana SignTransaction/SignAllTransactions when blockhash is invalid #18200

Conversation

yrliou commented Apr 21, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

yrliou Apr 22, 2023

Choose a reason for hiding this comment

yrliou Apr 22, 2023

Choose a reason for hiding this comment

mkarolin Apr 22, 2023

Choose a reason for hiding this comment

yrliou Apr 22, 2023

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

yrliou commented Apr 22, 2023

wknapik commented Apr 22, 2023

kdenhartog commented Apr 24, 2023

yrliou commented Apr 24, 2023

yrliou commented Apr 24, 2023 • edited Loading

kdenhartog commented Apr 24, 2023

yrliou commented Apr 24, 2023

srirambv commented Apr 25, 2023 • edited Loading

yrliou commented Apr 21, 2023 •

edited

Loading

yrliou commented Apr 24, 2023 •

edited

Loading

srirambv commented Apr 25, 2023 •

edited

Loading