Skip to content

fix(solana): decode a dApp transaction before asking the user to approve it - #152

Open
BitHighlander wants to merge 1 commit into
developfrom
fix/solana-decode-before-approval
Open

BitHighlander wants to merge 1 commit into
developfrom
fix/solana-decode-before-approval

Conversation

@BitHighlander

Copy link
Copy Markdown
Collaborator

Found while driving SoltoshiDICE with a funded mainnet wallet: approving a solana_signTransaction showed TO: N/A / AMOUNT: N/A over a real transfer. Diagnosis in HANDOFF_solana_tx_unreviewable_in_client.md; I verified its three load-bearing claims against the code before writing this.

Two bugs

  1. Nothing to render. buildEvent() never set unsignedTx, and RequestDetailsCard reads unsignedTx.payment for its To and Amount rows, so both printed the literal "N/A".
  2. Worse: the ordering. Approval was collected at solanaHandler.ts:880, and the vault's decode, risk bar and blind-signing policy all run later, inside the signing gate. The user approved a blind screen before any readable review existed anywhere in the system.

This PR

  • solana_signTransaction and solana_signAndSendTransaction decode first, attach the result to the event, and then ask for approval. Sign-and-send matters most, since the dApp broadcasts right after.
  • The card gets a Solana branch: instruction count and version, one row per instruction (program, instruction name, decoded args), an unresolved-ALT warning, and a blind-signing warning.
  • A decode failure renders a red refusal banner with the error, never a friendly summary from a partial parse.
  • The generic To/Amount table is never used for Solana again. An empty table reads as "nothing is being moved", which is the fake-data failure this repo's rules forbid.
  • Raw tab falls back to transaction.request when unsignedTx is absent, so a blind event still shows its bytes.

Requires the vault PR

Decoding goes through a new POST /solana/decode-transaction (keepkey/keepkey-vault, separate PR), which runs the same buildSolanaDecodedInfo the signing gate runs — one decoder, so the two screens cannot drift. No device, no signing, not a signing route.

Verified live against a patched vault:

  • a real v0 transaction decodes: 4 instructions — Compute Budget setComputeUnitLimit(120000), setComputeUnitPrice(1000), SPL Token transferChecked(amount=2000, decimals=6), Memo v2
  • garbage bytes return SolanaTxParseError: … with requiresBlindSigningConsent: true, not a summary
  • no bearer token gives 401; a missing raw_tx gives a 400 validation error
  • pnpm type-check passes (15/15), pnpm build succeeds, and the built background bundle contains the decode call

Not covered: no automated test asserts the decode-before-approval ordering. The handler imports too much to unit test as it stands. It needs the browser check below.

Please test

  1. Reload the unpacked extension from dist/.
  2. Trigger a Solana transaction from a dApp.
  3. The approval card should list instructions instead of "N/A".
  4. Stop the vault and trigger another: expect the red "could not decode" banner, not an empty table.

🤖 Generated with Claude Code

…ove it

The approval card read unsignedTx.payment, and buildEvent() never set
unsignedTx for a dApp-supplied Solana transaction, so a real transfer
rendered as TO: N/A / AMOUNT: N/A. Worse, approval was collected BEFORE
the vault's decode ran inside its signing gate, so the user approved a
blind screen before any readable review existed anywhere.

- vault: POST /solana/decode-transaction (separate PR) runs the same
  decoder as the signing gate, with no device and no signing
- solana_signTransaction and solana_signAndSendTransaction now decode
  first and attach the result to the event, then ask for approval
- the card gets a Solana branch: instruction list, blind-signing warning,
  unresolved-ALT warning, and a red refusal banner on a decode failure —
  never a friendly summary from a partial parse
- the generic To/Amount table is never used for Solana again
- Raw tab falls back to the request when unsignedTx is absent

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant