chore: merge release to main #24226
chore: merge release to main #24226
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| )}`; | ||
|
|
||
| await registry.handleConnectDeeplink(blockedDeeplink); | ||
| }); |
There was a problem hiding this comment.
Test missing assertions for blocked metamask dapp name
The test blocks connection requests with 'metamask' as dapp name lacks any expect assertions. Unlike the preceding test for blocking metamask as origin (which includes three assertions), this test only calls handleConnectDeeplink without verifying the behavior. The test will always pass regardless of whether the blocking logic works correctly.
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->
Dapp connection implementaion robustness
<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
<!--
If this PR is not End-User-Facing and should not show up in the
CHANGELOG, you can choose to either:
1. Write `CHANGELOG entry: null`
2. Label with `no-changelog`
If this PR is End-User-Facing, please write a short User-Facing
description in the past tense like:
`CHANGELOG entry: Added a new tab for users to see their NFTs`
`CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker`
(This helps the Release Engineer do their job more quickly and
accurately)
-->
CHANGELOG entry:
Fixes:
```gherkin
Feature: my feature name
Scenario: user [verb for user action]
Given [describe expected initial app state]
When user [verb for user action]
Then [describe expected outcome]
```
<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->
<!-- [screenshots/recordings] -->
<!-- [screenshots/recordings] -->
- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] 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.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> Strengthens origin validation to prevent external dapps from using
internal MetaMask origins.
>
> - Enforces `INTERNAL_ORIGINS` checks and throws
`rpcErrors.invalidParams` in `eth_sendTransaction`, SDK deeplink
`processDappRpcRequest`, SDK `setupBridge`, SDKConnectV2
`handleConnectDeeplink`, WC2 `onSessionProposal`, and WC2 session
`handleSendTransaction`.
> - Normalizes use of origin/hostname where needed and persists origin
metadata for UI as before.
> - Updates WC2 tests: stubs `rejectSession` to resolve, adds
comprehensive origin-rejection cases (blocks plain `metamask`, allows
valid URLs including subdomains and `metamask.io`).
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
bd6a404. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
| expect(service.bridgeByClientId[clientInfo.clientId]).toBeInstanceOf( | ||
| BackgroundBridge, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test names use prohibited "should" prefix (Bugbot Rules)
Multiple new tests use "should" in their names (e.g., 'should throw error when originatorInfo.url is "metamask"'). This violates the Unit Testing Guidelines rule: "NEVER use 'should' in test names - this is a hard rule with zero exceptions." Test names like 'throws error when originatorInfo.url equals metamask' would be compliant.
Additional Locations (2)
| throw rpcErrors.invalidParams({ | ||
| message: 'External transactions cannot use internal origins', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Undefined values may incorrectly match INTERNAL_ORIGINS array
The INTERNAL_ORIGINS array includes process.env.MM_FOX_CODE, which is undefined when the env var is not set. The new INTERNAL_ORIGINS.includes() checks don't guard against undefined input values, unlike the earlier === ORIGIN_METAMASK checks which use truthiness guards (originatorInfo.url &&). If both the env var is unset and originatorInfo.url/title or params.url is undefined (e.g., from a malformed dapp request), includes(undefined) would return true, incorrectly blocking legitimate connections.
Additional Locations (2)
| handled(); | ||
| if (params.channelId && INTERNAL_ORIGINS.includes(params.channelId)) { | ||
| throw new Error('External transactions cannot use internal origins'); | ||
| } |
There was a problem hiding this comment.
Security check validates wrong field in deeplink handlers
The security check validates params.channelId against INTERNAL_ORIGINS, but channelId is a session identifier (typically a UUID), not an origin URL. The error message says "External transactions cannot use internal origins" which is misleading since channelId is not an origin. Compare with other files in this PR that correctly check origin fields: eth_sendTransaction.ts checks hostname, DeeplinkProtocolService.ts checks params.url, setupBridge.ts checks originatorInfo.url. The actual dApp origin information in these handlers is in params.originatorInfo (parsed later), not channelId. This check provides incomplete protection against internal origin spoofing.
Additional Locations (1)
weitingsun
added
the
skip-sonar-cloud
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR implements critical security hardening to prevent external connections (WalletConnect, SDK, deeplinks) from spoofing internal MetaMask origins. The changes span multiple core modules:
Tag Selection Rationale:
This is a high-risk change because it modifies security-critical code paths that handle external dApp connections and transaction processing. While the changes are defensive (blocking malicious origins), any regression could either break legitimate dApp connections or fail to properly block malicious ones. |
| )}`; | ||
|
|
||
| await registry.handleConnectDeeplink(blockedDeeplink); | ||
| }); |
There was a problem hiding this comment.
Test missing assertions for dapp name blocking (Bugbot Rules)
The test 'blocks connection requests with \metamask` as dapp name'has no assertions after callinghandleConnectDeeplink. The preceding test (lines 604-631) for blocking by URL includes three assertions (showConnectionError, Connection.createnot called,mockStore.save` not called), but this test simply ends after the Act phase. This violates the mandatory AAA (Arrange, Act, Assert) pattern from the unit testing guidelines and means the test won't detect regressions if the dapp name blocking logic breaks.
| .mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| it('should reject session proposal with "metamask" origin', async () => { |
There was a problem hiding this comment.
Tests use prohibited "should" in names (Bugbot Rules)
Multiple new tests violate the unit testing guidelines rule: "NEVER use 'should' in test names - this is a hard rule with zero exceptions." Tests like 'should throw error when originatorInfo.url is "metamask"', 'should reject session proposal with "metamask" origin', and similar patterns across WalletConnectV2.test.ts, DeeplinkProtocolService.test.ts, and setupBridge.test.ts all use "should" at the start of their names. These need to be rewritten with action-oriented descriptions (e.g., 'throws error when originatorInfo.url is metamask').
Additional Locations (2)
|
Description
Merge 7.61.3, 7.61.4 and 7.61.5 back to main
Changelog
CHANGELOG entry:null
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Strengthens origin validation for external requests and expands hardware auto-signing.
eth_sendTransactionand connections using internal origins via checks ineth_sendTransaction, WalletConnect (session proposal/init and sendTransaction), SDKConnect (bridge setup, deeplink protocol, V2 connection registry), and deeplink handlers; surfacesinvalidParams/errorschannelIdhandling in deeplink flows and guards against internal-origin channel IDsRootRPCMethodsUI(in addition to Ledger)Written by Cursor Bugbot for commit c2d4bd0. This will update automatically on new commits. Configure here.