fix(SDKConnectV2): harden INTERNAL_ORIGINS check and restrict dapp.url to http(s) by adonesky1 · Pull Request #26833 · MetaMask/metamask-mobile

adonesky1 · 2026-03-02T20:24:17Z

Description

The new URL(metadata.dapp.url) validation added in the parent branch ensures dapp.url is always a parseable URL. However, the production INTERNAL_ORIGINS array contains plain strings ('metamask', 'MetaMask Mobile', process.env.MM_FOX_CODE), so the existing INTERNAL_ORIGINS.includes(dapp.url) check became dead code — no validated URL can equal those strings. The test masked this by mocking INTERNAL_ORIGINS with 'https://internal.metamask.io', a value absent from the production array.

This PR:

Restricts dapp.url to http:/https: schemes in isConnectionRequest() — this is the primary security boundary. It prevents a malicious dApp from sending metamask://... or other non-web schemes as its URL, which could match internal origin strings.
Retains the original INTERNAL_ORIGINS.includes() check as documented defense-in-depth — currently redundant but will activate if the upstream URL validation is ever relaxed.
Adds cross-referencing security comments in both locations so future developers understand the invariant linking them.
Removes the INTERNAL_ORIGINS mock from the test file so tests exercise the real production values instead of a synthetic URL that hid the dead code.

Changelog

CHANGELOG entry: null

Related issues

Refs: parent branch jl/make-mmc-parsing-more-robust

Manual testing steps

Feature: SDKConnectV2 internal origin blocking

  Scenario: dApp with non-http scheme is rejected at validation
    Given a dApp connection request with url "metamask://evil.com"
    When isConnectionRequest validates the request
    Then it returns false

  Scenario: dApp with internal origin name is blocked
    Given a dApp connection request with dapp.name "metamask"
    When ConnectionRegistry handles the deeplink
    Then it throws "External transactions cannot use internal origins"

Screenshots/Recordings

N/A — no UI changes.

Before

N/A

After

N/A

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

Medium Risk
Tightens SDKConnectV2 connection-request validation and could reject previously accepted dApps that supplied non-HTTP(S) dapp.url values; it also touches internal-origin security checks on the deeplink connection path.

Overview
Hardens SDKConnectV2 connection request validation by requiring metadata.dapp.url to be a parseable URL and to use only http:/https: schemes, rejecting custom/non-web schemes (e.g. metamask://, ftp://, file://).

Clarifies and reinforces internal-origin blocking in ConnectionRegistry.handleConnectDeeplink() as a documented defense-in-depth check, and updates tests to stop mocking INTERNAL_ORIGINS and instead explicitly bypass isConnectionRequest() to exercise the internal-origin rejection path.

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

github-actions · 2026-03-02T20:24:29Z

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.

…heck The new `new URL()` validation in `isConnectionRequest` ensures `dapp.url` is always a parseable URL. This made the direct-string `INTERNAL_ORIGINS.includes(dapp.url)` check dead code because production INTERNAL_ORIGINS contains non-URL values ('metamask', 'MetaMask Mobile'). The test masked this by injecting 'https://internal.metamask.io' into the mock. - Extract URL hostname and compare case-insensitively against INTERNAL_ORIGINS (e.g. `https://metamask/` → hostname `metamask`) - Restrict `dapp.url` to http/https schemes as defense-in-depth - Remove fake 'https://internal.metamask.io' from test mock - Add scheme-validation tests for isConnectionRequest Made-with: Cursor

…-in-depth Revert hostname extraction logic in favor of the original plain-string INTERNAL_ORIGINS comparison. The URL validation in isConnectionRequest() is the primary security boundary; this check is a documented safety net. Both sites now carry security comments linking them together. Made-with: Cursor

adonesky1 · 2026-03-02T20:59:16Z

-jest.mock('../../../constants/transaction', () => ({
-  INTERNAL_ORIGINS: ['metamask', 'MetaMask Mobile', 'https://internal.metamask.io'],
-}));


We should use the actual internal origin values for the test

Mock isConnectionRequest to bypass upstream URL validation, exercising the INTERNAL_ORIGINS check for dapp.url that is otherwise unreachable. Made-with: Cursor

The try/catch rejects plain strings; the protocol check blocks custom schemes. The previous comment conflated both layers. Made-with: Cursor

jiexi · 2026-03-02T21:08:42Z

+      const connReqModule = require('../types/connection-request');
+      const spy = jest
+        .spyOn(connReqModule, 'isConnectionRequest')
+        .mockReturnValueOnce(true);
+


does a jest.mock() work here out of curiosity?

I think jest.mock can only be applied at the top level of the test and therefore get applied to all tests?

yeah that sound vaguely right. I know it gets hoisted. ehh okay this is fine

github-actions · 2026-03-02T21:09:13Z

🔍 Smart E2E Test Selection

⏭️ Smart E2E selection skipped - base branch is not main (base: jl/make-mmc-parsing-more-robust)

All E2E tests pre-selected.

View GitHub Actions results

adonesky1 self-assigned this Mar 2, 2026

metamaskbot added the team-wallet-integrations Wallet Integrations team label Mar 2, 2026

github-actions Bot added the size-S label Mar 2, 2026

adonesky1 force-pushed the fix/internal-origins-hostname-check branch from 0a8262e to 346e230 Compare March 2, 2026 20:43

adonesky1 changed the title ~~fix(SDKConnectV2): use hostname comparison for INTERNAL_ORIGINS URL check~~ fix(SDKConnectV2): harden INTERNAL_ORIGINS check and restrict dapp.url to http(s) Mar 2, 2026

adonesky1 commented Mar 2, 2026

View reviewed changes

adonesky1 added 2 commits March 2, 2026 15:03

test(SDKConnectV2): restore dapp url defense-in-depth test

6a824bd

Mock isConnectionRequest to bypass upstream URL validation, exercising the INTERNAL_ORIGINS check for dapp.url that is otherwise unreachable. Made-with: Cursor

docs(SDKConnectV2): clarify security comment on URL validation

84a9794

The try/catch rejects plain strings; the protocol check blocks custom schemes. The previous comment conflated both layers. Made-with: Cursor

jiexi reviewed Mar 2, 2026

View reviewed changes

jiexi approved these changes Mar 2, 2026

View reviewed changes

adonesky1 marked this pull request as ready for review March 2, 2026 21:14

adonesky1 requested a review from a team as a code owner March 2, 2026 21:14

adonesky1 merged commit 6dc4da4 into jl/make-mmc-parsing-more-robust Mar 2, 2026
39 checks passed

adonesky1 deleted the fix/internal-origins-hostname-check branch March 2, 2026 21:14

github-actions Bot locked and limited conversation to collaborators Mar 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(SDKConnectV2): harden INTERNAL_ORIGINS check and restrict dapp.url to http(s)#26833

fix(SDKConnectV2): harden INTERNAL_ORIGINS check and restrict dapp.url to http(s)#26833
adonesky1 merged 4 commits into
jl/make-mmc-parsing-more-robustfrom
fix/internal-origins-hostname-check

adonesky1 commented Mar 2, 2026 •

edited by cursor Bot

Loading

Uh oh!

github-actions Bot commented Mar 2, 2026

Uh oh!

adonesky1 Mar 2, 2026

Uh oh!

jiexi Mar 2, 2026

Uh oh!

adonesky1 Mar 2, 2026

Uh oh!

jiexi Mar 2, 2026

Uh oh!

github-actions Bot commented Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

adonesky1 commented Mar 2, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions Bot commented Mar 2, 2026

Uh oh!

adonesky1 Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

jiexi Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

adonesky1 Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

jiexi Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 2, 2026

🔍 Smart E2E Test Selection

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adonesky1 commented Mar 2, 2026 •

edited by cursor Bot

Loading