fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0 with patch #24138

digiwand · 2024-04-19T19:18:15Z

Description

Resolves @spruceid/siwe-parser 1.1.3 → 2.1.0 The updated @spruceid/siwe-parser allows http and https schemas 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_sign message 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 a personal_sign message looks like a SIWE but doesn't parse to a SIWE message: #24128

Issue 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

Go to the metamask test-dapp or run the test-dapp locally
When testing locally, optionally update the from addresses used by the buttons
Test Sign-in With Ethereum buttons
Observe MM

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
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.

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.

github-actions · 2024-04-19T19:18:28Z

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.

pedronfigueiredo · 2024-04-22T09:52:16Z

Can we not simply update the package to 9.1.0 instead of resolving it?

…"9.1.0"" This reverts commit 266757a.

digiwand · 2024-04-22T11:03:46Z

hi @pedronfigueiredo, great question!

"@metamask/controller-utils": "^9.1.0", was updated as an npm dependency in this repo (PR). The problem was other packages were resolving to v8.0.4 which caused the older version of the siwe-parser to be used. I realized on second thought, and after your message, that we needed to update @metamask/message-manager which would update the controller-utils version to 9.1.0. Updated the PR with this change de1c630.

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

legobeat · 2024-04-22T12:46:09Z

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 @metamask/message-manager is desired as well, is there a reason to not want to do both here? As in, the resolutions force + the update to @metamask/message-manager@^8.0.1?

digiwand · 2024-04-22T12:57:52Z

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

package.json

This reverts commit d0a540d.

package.json

following @spruceid/siwe-parser 1.1.3 -> 2.1.0 resolution

legobeat · 2024-04-23T00:57:09Z

@metamaskbot update-policies

metamaskbot · 2024-04-23T01:06:38Z

Policies updated

…a-in-url

@digiwand

…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.

…a-in-url

digiwand · 2024-04-24T12:28:20Z

@metamaskbot update-policies

metamaskbot · 2024-04-24T12:38:38Z

Policies updated

…a-in-url

metamaskbot · 2024-04-24T13:20:01Z

Builds ready [a4d6d9a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1115 ± 680 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	213	124	44	21
		domContentLoaded	9	79	30	19	9
		load	48	3813	1115	1417	680
		domInteractive	9	79	30	19	9

Bundle size diffs [🚀 Bundle size reduced!]

background: -22.13 KiB (-0.61%)
ui: 0 Bytes (0.00%)
common: -10.85 KiB (-0.18%)

codecov · 2024-04-24T13:52:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.49%. Comparing base (085e808) to head (a4d6d9a).

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.
📢 Have feedback on the report? Share it here.

fix: add resolution "@metamask/controller-utils@npm:^8.0.4": "9.1.0"

266757a

digiwand requested a review from a team as a code owner April 19, 2024 19:18

digiwand added the team-confirmations-secure-ux-PR PRs from the confirmations team label Apr 19, 2024

jpuri previously approved these changes Apr 22, 2024

View reviewed changes

Revert "fix: add resolution "@metamask/controller-utils@npm:^8.0.4": …

28b80a3

…"9.1.0"" This reverts commit 266757a.

digiwand dismissed jpuri’s stale review via de1c630 April 22, 2024 10:48

digiwand changed the title ~~fix: add resolution "@metamask/controller-utils@npm:^8.0.4": "9.1.0"~~ fix: add npm resolution @metamask/message-manager ^7.3.0 → ^v8.0.1 which updates @metamask/controller-utils ^8.0.4 → 9.1.0 Apr 22, 2024

pedronfigueiredo previously approved these changes Apr 22, 2024

View reviewed changes

digiwand changed the title ~~fix: add npm resolution @metamask/message-manager ^7.3.0 → ^v8.0.1 which updates @metamask/controller-utils ^8.0.4 → 9.1.0~~ fix: add npm resolution @metamask/controller-utils ^8.0.4 → 9.1.0 Apr 22, 2024

digiwand requested a review from jpuri April 22, 2024 12:57

digiwand mentioned this pull request Apr 22, 2024

[Bug]: Sign-in With Ethereum (SIWE) buttons are not using EIP-55 compliant from addresses MetaMask/test-dapp#326

Closed

NicholasEllul reviewed Apr 22, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

digiwand added 3 commits April 22, 2024 22:35

fix: upgrade @metamask/message-manager ^7.3.0 -> v8.0.1

338b7e5

Revert "fix: upgrade @metamask/message-manager ^7.3.0 -> v8.0.1"

885e901

This reverts commit d0a540d.

fix: add resolution "@spruceid/siwe-parser@npm:1.1.3": "2.1.0"

3fc4ff3

digiwand dismissed pedronfigueiredo’s stale review via 3fc4ff3 April 22, 2024 19:28

digiwand force-pushed the fix-siwe-parser-resolution-to-allow-schema-in-url branch from 4b0c5ae to 3fc4ff3 Compare April 22, 2024 19:28

digiwand changed the title ~~fix: add npm resolution @metamask/controller-utils ^8.0.4 → 9.1.0~~ fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0 Apr 22, 2024

digiwand commented Apr 22, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

yarn lavamoat:auto

e7612d7

following @spruceid/siwe-parser 1.1.3 -> 2.1.0 resolution

digiwand requested review from a team as code owners April 22, 2024 20:37

Update LavaMoat policies

2e23765

Merge branch 'develop' into fix-siwe-parser-resolution-to-allow-schem…

ea3a236

…a-in-url

NicholasEllul mentioned this pull request Apr 23, 2024

fix: remove requirement for EIP-55 address when parsing SIWE message #24204

Merged

7 tasks

digiwand changed the title ~~fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0~~ fix: add npm resolution @spruceid/siwe-parser 1.1.3 → 2.1.0 with patch Apr 23, 2024

digiwand added 2 commits April 24, 2024 00:39

Merge branch 'develop' into fix-siwe-parser-resolution-to-allow-schem…

789b433

…a-in-url

yarn lavamoat:auto

540c6b2

Update LavaMoat policies

babf1a0

Merge branch 'develop' into fix-siwe-parser-resolution-to-allow-schem…

a4d6d9a

…a-in-url

digiwand requested review from legobeat, pedronfigueiredo and NicholasEllul April 24, 2024 12:47

pedronfigueiredo approved these changes Apr 24, 2024

View reviewed changes

jpuri approved these changes Apr 24, 2024

View reviewed changes

digiwand merged commit 4b55476 into develop Apr 24, 2024
68 checks passed

digiwand deleted the fix-siwe-parser-resolution-to-allow-schema-in-url branch April 24, 2024 14:08

github-actions bot locked and limited conversation to collaborators Apr 24, 2024

metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

digiwand commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 19, 2024

pedronfigueiredo commented Apr 22, 2024

digiwand commented Apr 22, 2024 •

edited

Loading

legobeat commented Apr 22, 2024

digiwand commented Apr 22, 2024

legobeat commented Apr 23, 2024

metamaskbot commented Apr 23, 2024

digiwand commented Apr 24, 2024

metamaskbot commented Apr 24, 2024

metamaskbot commented Apr 24, 2024

codecov bot commented Apr 24, 2024

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

digiwand commented Apr 19, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Apr 19, 2024

pedronfigueiredo commented Apr 22, 2024

digiwand commented Apr 22, 2024 • edited Loading

legobeat commented Apr 22, 2024

digiwand commented Apr 22, 2024

legobeat commented Apr 23, 2024

metamaskbot commented Apr 23, 2024

digiwand commented Apr 24, 2024

metamaskbot commented Apr 24, 2024

metamaskbot commented Apr 24, 2024

codecov bot commented Apr 24, 2024

Codecov Report

digiwand commented Apr 19, 2024 •

edited

Loading

digiwand commented Apr 22, 2024 •

edited

Loading