Skip to content
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

fix: normalize transaction parameters before PPOM validation #8774

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Feb 28, 2024

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: please use "team-confirmations" label 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
Copy link

sonarcloud bot commented Feb 28, 2024

@seaona seaona added QA Passed A successful QA run through has been done Run Smoke E2E Triggers smoke e2e on Bitrise labels Mar 1, 2024
Copy link
Contributor

github-actions bot commented Mar 1, 2024

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
Copy link
Contributor

seaona commented Mar 1, 2024

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

Copy link
Contributor

@dbrans dbrans left a comment

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 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Mar 4, 2024
@matthewwalsh0 matthewwalsh0 merged commit 2c629fa into main Mar 4, 2024
40 of 47 checks passed
@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 A successful QA run through has been done release-7.19.0 Issue or pull request that will be included in release 7.19.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-confirmations-system DEPRECATED: please use "team-confirmations" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants