-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore: fix signature verification sorting issue cp-7.60.0 #23220
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
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
a17758d to
e3b5d81
Compare
e3b5d81 to
10c9026
Compare
Cal-L
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add unit test to cover cases that this intends to fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23220 +/- ##
==========================================
+ Coverage 78.56% 78.59% +0.02%
==========================================
Files 3947 3947
Lines 102433 102423 -10
Branches 20311 20303 -8
==========================================
+ Hits 80481 80497 +16
+ Misses 16407 16385 -22
+ Partials 5545 5541 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MarioAslau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
056ba77 to
bbfc297
Compare
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThe changes are in a critical deeplink signature verification utility ( What Changed:
Why This is High Risk:
Why These Tags:
The changes are well-tested (comprehensive test updates included), but the security-critical nature and wide impact across multiple features warrant thorough E2E validation of all deeplink-dependent flows. |
|
Cal-L
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



Related issues
Fixes: #23222
.sortworks on React Native.sig_paramsbeing empty, so that we can still sign links without any parameters that need to be signedDescription
Reason for change:
Deep link signature verification was failing for marketing links (e.g.,
https://link.metamask.io/perps?sig_params=...&utm_...). The Mobile app'scanonicalizefunction had diverged from the Extension's implementation, causing signature mismatches.The root cause: React Native's
URLSearchParams.sort()is not implemented — it throws an error. This meant parameters weren't being sorted correctly, so the canonical URL didn't match what was signed.Improvement/solution:
Updated the
canonicalizefunction to:URLSearchParams.sort()(which is broken in React Native)sig_paramsexplicitly — whensig_params='', only includesig_paramsitself in the canonical URL (allows appending UTMs after signing)getAll()/append()instead ofget()/set()to preserve multiple values for the same parameter (matches Extension behavior)encodeURIComponent()for consistent URL encodingThese changes align Mobile with the Extension's canonicalization logic, ensuring signed deep links verify correctly.
Changelog
CHANGELOG entry: Fixed deep link signature verification failing for marketing links with UTM parameters
Manual testing steps
Screenshots/Recordings
Before
Signed marketing links showed the "unsigned link" warning modal due to signature verification failure.
After
Signed marketing links verify correctly and open without the warning modal.
Pre-merge author checklist
Pre-merge reviewer checklist
Labels to add:
team-mobile-platform