Skip to content

fix: normalize transaction parameters before PPOM validation#8774

Merged
matthewwalsh0 merged 2 commits into
mainfrom
fix/transaction-data-hex-padding
Mar 4, 2024
Merged

fix: normalize transaction parameters before PPOM validation#8774
matthewwalsh0 merged 2 commits into
mainfrom
fix/transaction-data-hex-padding

Conversation

@matthewwalsh0

@matthewwalsh0 matthewwalsh0 commented Feb 28, 2024

Copy link
Copy Markdown
Member

Description

Update the TransactionController to pad the data property to an even length to ensure the client UIs receive a consistent data value.

Normalize transaction parameters before validating with the PPOM to ensure the data can be parsed correctly, and all parameters are in a consistent format.

See the core branch diff for a clearer view of the patch changes.

Related issues

Fixes: #2152 #2153

Manual testing steps

  1. Update test dApp locally to remove leading zero in malicious approval data.
  2. Verify associated button still displays approval confirmation.
  3. Verify associated button still displays deceptive request warning.

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner February 28, 2024 16:58
@matthewwalsh0 matthewwalsh0 added the team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead label Feb 28, 2024
@matthewwalsh0 matthewwalsh0 changed the title Normalize transaction parameters before PPOM validation fix: normalize transaction parameters before PPOM validation Feb 28, 2024
@sonarqubecloud

Copy link
Copy Markdown

@seaona seaona added QA Passed QA testing has been completed and passed Run Smoke E2E labels Mar 1, 2024
@github-actions

github-actions Bot commented Mar 1, 2024

Copy link
Copy Markdown
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 49e5309
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cc6e29d9-6072-4423-a553-a1d6ba60c615

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@seaona

seaona commented Mar 1, 2024

Copy link
Copy Markdown
Member

This looks good from QA:

  • 🟢 hex data is normalized
  • 🟢 ppom validation works with odd hex data, since it's normalized (ie approve without a 0)
  • Send flow with Edit hex data cannot be initiated from inside the wallet - so no change here

I've triggered a Bitrise e2e build here: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/961e97dd-afd4-4ed1-962f-6487f418d0f0

odd-hex-data-mobile.mp4

@dbrans dbrans left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fantastic work, @matthewwalsh0!

@matthewwalsh0 matthewwalsh0 merged commit 2c629fa into main Mar 4, 2024
@matthewwalsh0 matthewwalsh0 deleted the fix/transaction-data-hex-padding branch March 4, 2024 14:33
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 4, 2024
@metamaskbot metamaskbot added the release-7.19.0 Issue or pull request that will be included in release 7.19.0 label Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed QA testing has been completed and passed release-7.19.0 Issue or pull request that will be included in release 7.19.0 team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants