fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0 with patch#24138
fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0 with patch#24138
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. |
|
Can we not simply update the package to 9.1.0 instead of resolving it? |
…"9.1.0"" This reverts commit 266757a.
|
hi @pedronfigueiredo, great question!
edit: the last @metamask/message-manager dependency update ^7.3.0 → ^v8.0.1 is still not resolving the dependencies properly. Other dependencies were resolved with older versions of @metamask/message-manager. Due to the interconnected dependencies (keyring-controller, signature-controller, etc.), and this being a time-sensitive change (ref: Internal thread), I reverted back to the initial @metamask/controller-util resolution. af06b3e |
@digiwand If the update of |
|
hello @legobeat, the reason is I wanted to introduce minimal changes for the fix. We could update in another PR though I'm not sure there is a particular reason to do so now. Our team is currently deprecating older signature pages and introducing new ones. I think updating packages like these should come following these changes |
This reverts commit d0a540d8963c2284f9f7d321db8d028e7241586d.
4b0c5ae to
3fc4ff3
Compare
following @spruceid/siwe-parser 1.1.3 -> 2.1.0 resolution
|
@metamaskbot update-policies |
|
Policies updated |
…24204) Base branch is #24138 ## **Description** Upon testing the new `@spruceid/siwe-parser` changes, @digiwand noticed that message parsing would fail when using the [MetaMask Test dApp](https://metamask.github.io/test-dapp/). During this investigation, it was identified that the parsing failed due to the [isEIP55Address](https://github.com/spruceid/siwe/blob/4290e1fb3702c97ffee16112492216ad4c4654c1/packages/siwe-parser/lib/abnf.ts#L362-L364) check. This is due to the fact that an EIP-55 address is case sensitive, but MetaMask lowercases the wallet address before returning it to an `eth_accounts` call. This means that when dApps attempt to request a SIWE message (using this lowecase address), the operation would fail to parse. This change removes that check, and provides the consistent experience we have already been giving prior to the `@spruceid/siwe-parser` v2 upgrade. ## **Related issues** Adds to #24138 ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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.
|
@metamaskbot update-policies |
|
Policies updated |
Builds ready [a4d6d9a]
Page Load Metrics (1115 ± 680 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24138 +/- ##
========================================
Coverage 67.49% 67.49%
========================================
Files 1260 1260
Lines 49271 49271
Branches 12838 12838
========================================
Hits 33255 33255
Misses 16016 16016 ☔ View full report in Codecov by Sentry. |
Description
Resolves @spruceid/siwe-parser 1.1.3 → 2.1.0 The updated @spruceid/siwe-parser allows
httpandhttpsschemas in a transaction message's URL.Additional changes are included in the @spruceid/siwe-parser upgrade from v1.1.3 → v2.1.0 that we did not want to include - hence the patch. The update we are currently excluding is the requirement of EIP-55 complaint addresses. This means that if a dapp attempts to sign a
personal_signmessage with a from address that is not EIP-55 compliant, SIWE will not be detected. This is what we want as EIP-4361 enforces EIP-55 addresses. There's a separate ticket that'll warn users if apersonal_signmessage looks like a SIWE but doesn't parse to a SIWE message: #24128Issue to enforce SIWE addresses to be EIP-55 complaint: https://github.com/MetaMask/MetaMask-planning/issues/2430
Related Links (Thanks @NicholasEllul for these):
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2256
Relates To: https://github.com/MetaMask/MetaMask-planning/issues/2278
Related to: #24107
Related to: MetaMask/core#4153
Related to: MetaMask/core#4141
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist