chore: improve SDK and WalletConnect connection reliability

[jiexi]

Description

Improve SDK and WalletConnect connection reliability

Changelog

CHANGELOG entry: null

Related issues

Fixes:

Manual testing steps

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]

Screenshots/Recordings

Before

After

Pre-merge author checklist

• I’ve followed MetaMask Contributor Docs and MetaMask Mobile 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.

---

Note

Blocks connections when origin/url/title equals 'metamask' (not substrings) in SDK/WalletConnect flows and adds tests.

• Security/Origin Validation:
  • SDK Deeplink: In DeeplinkProtocolService.setupBridge, reject clients whose originatorInfo.url or title equals ORIGIN_METAMASK; keep normal bridge setup otherwise.
  • SDK Handler: In handlers/setupBridge, add the same rejection before creating BackgroundBridge.
  • SDKConnectV2: In services/connection-registry.handleConnectDeeplink, block requests when metadata.dapp.url or name equals ORIGIN_METAMASK.
  • WalletConnect v2:
    • During init, skip restoring sessions with peer.metadata.url === ORIGIN_METAMASK.
    • On session_proposal, reject when metadata.url === ORIGIN_METAMASK.
• Tests:
  • Add unit tests covering rejection for exact metamask origins and allowing substrings/valid URLs in DeeplinkProtocolService.test.ts, handlers/setupBridge.test.ts, and connection-registry.test.ts (WC2 origin rejection tests added but skipped).
• Misc:
  • Import ORIGIN_METAMASK where needed; minor import consolidation with toChecksumHexAddress.

^{Written by Cursor Bugbot for commit 95cb249. This will update automatically on new commits. Configure here.}

[github-actions]

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]

Test has no assertions and always passes

The test 'blocks connection requests with \metamask` as dapp name'has no assertions after callinghandleConnectDeeplink. Unlike the similar test above it (lines 604-632) which properly asserts showConnectionError, Connection.createnot called, andsave` not called, this test executes the code but never verifies the expected behavior. This means the test will always pass regardless of whether the security check actually works, providing no regression protection for the dapp name blocking feature.

 

[cursor]

Test names use prohibited "should" prefix (Bugbot Rules)

The unit testing guidelines explicitly state "NEVER use 'should' in test names - this is a hard rule with zero exceptions." The newly added tests use names like 'should throw error when originatorInfo.url is "metamask"', 'should allow connection when originatorInfo contains "metamask" as substring', and 'should reject session proposal with "metamask" origin'. These violate the naming convention and should use action-oriented descriptions instead.

Additional Locations (2)

• app/core/SDKConnect/handlers/setupBridge.test.ts#L126-L164
• app/core/WalletConnect/WalletConnectV2.test.ts#L1457-L1592

 

[cursor]

WalletConnect origin check misses name/title validation

The WalletConnectV2.ts origin validation only checks if url === ORIGIN_METAMASK, but the other three implementations in this PR (DeeplinkProtocolService.ts, setupBridge.ts, connection-registry.ts) consistently check both url AND title/name fields. This inconsistency means a malicious dapp could set metadata.name to 'metamask' while using a different URL, which would be blocked by SDK Connect but allowed through WalletConnect, creating a security gap.

Additional Locations (1)

• app/core/WalletConnect/WalletConnectV2.ts#L124-L130

 

[cursor]

Skipped tests leave origin rejection feature untested

The describe.skip('Origin Rejection', ...) prevents all origin rejection tests from running. This means the new security feature that blocks connections from metamask origin in WalletConnectV2.ts (lines 445-452) has no test coverage. The skip modifier causes Jest to ignore this entire test suite, leaving the feature unverified.

 

[cursor]

Test names use "should" violating guidelines (Bugbot Rules)

Multiple new tests use "should" in their names, violating the unit testing guidelines rule: "NEVER use 'should' in test names - this is a hard rule with zero exceptions". Examples include should throw error when originatorInfo.url is "metamask", should allow connection when originatorInfo contains "metamask" as substring, and should reject session proposal with "metamask" origin. Tests should use action-oriented descriptions like throws error when originatorInfo.url is "metamask".

Additional Locations (2)

• app/core/SDKConnect/handlers/setupBridge.test.ts#L126-L164
• app/core/WalletConnect/WalletConnectV2.test.ts#L1457-L1640

 

[github-actions]

🔍 Smart E2E Test Selection

• Selected E2E tags: SmokeCore, SmokeMultiChainPermissions, SmokeConfirmationsRedesigned
• Risk Level: high
• AI Confidence: 75%

click to see 🤖 AI reasoning details

This PR implements a security fix that blocks connections from external sources claiming to be from the "metamask" origin (ORIGIN_METAMASK). The changes span multiple critical connection handling modules:

1. DeeplinkProtocolService.ts - Adds validation to reject deeplink connections where originatorInfo.url or title equals "metamask"
2. setupBridge.ts - Blocks bridge setup for connections claiming metamask origin
3. connection-registry.ts (SDKConnectV2) - Blocks connection requests with metamask as dapp.url or dapp.name
4. WalletConnectV2.ts - Rejects WalletConnect session proposals with "metamask" URL and skips invalid sessions during init

This is a security-focused change that prevents potential origin spoofing attacks. The changes are in critical app/core/ paths that handle all external dApp connections.

Tag Selection Rationale:

• SmokeCore: Essential because these changes affect core wallet connectivity, deeplink handling, and SDK/WalletConnect connection protocols - fundamental to how the app communicates with external dApps
• SmokeMultiChainPermissions: The changes involve permission-related code (PermissionController imports, CAIP-25 permission handling) and affect how connections are authorized
• SmokeConfirmationsRedesigned: WalletConnect and SDK connections ultimately lead to transaction confirmations; ensuring the connection flow works correctly is critical for confirmation flows

The unit tests added are comprehensive, but E2E testing is important to verify that legitimate dApp connections still work correctly while malicious origin spoofing is blocked.

View GitHub Actions results

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[abretonc7s]

I have tested and it works on sdk but bugbots are legits and should be addressed first.