Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [7.61.4]

### Fixed

- chore: robust implementation dapp connection ([#24222](https://github.com/MetaMask/metamask-mobile/pull/24222))

## [7.61.3]

### Fixed
Expand Down Expand Up @@ -9661,7 +9667,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#957](https://github.com/MetaMask/metamask-mobile/pull/957): fix timeouts (#957)
- [#954](https://github.com/MetaMask/metamask-mobile/pull/954): Bugfix: onboarding navigation (#954)

[Unreleased]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.3...HEAD
[Unreleased]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.4...HEAD
[7.61.4]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.3...v7.61.4
[7.61.3]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.2...v7.61.3
[7.61.2]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.1...v7.61.2
[7.61.1]: https://github.com/MetaMask/metamask-mobile/compare/v7.61.0...v7.61.1
Expand Down
4 changes: 2 additions & 2 deletions android/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ android {
applicationId "io.metamask"
minSdkVersion rootProject.ext.minSdkVersion
targetSdkVersion rootProject.ext.targetSdkVersion
versionName "7.61.3"
versionCode 3327
versionName "7.61.4"
versionCode 3335
testBuildType System.getProperty('testBuildType', 'debug')
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
manifestPlaceholders.MM_BRANCH_KEY_TEST = "$System.env.MM_BRANCH_KEY_TEST"
Expand Down
7 changes: 7 additions & 0 deletions app/core/RPCMethods/eth_sendTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { rpcErrors } from '@metamask/rpc-errors';
import ppomUtil, { PPOMRequest } from '../../lib/ppom/ppom-util';
import { updateConfirmationMetric } from '../redux/slices/confirmationMetrics';
import { store } from '../../store';
import { INTERNAL_ORIGINS } from '../../constants/transaction';

/**
* A JavaScript object that is not `null`, a function, or an array.
Expand Down Expand Up @@ -112,6 +113,12 @@ async function eth_sendTransaction({
from: req.params[0].from,
chainId: nChainId,
});
// Prevent external transactions from using internal origins
if (INTERNAL_ORIGINS.includes(hostname)) {
throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
}

const { result, transactionMeta } = await sendTransaction(req.params[0], {
deviceConfirmedOn: WalletDevice.MM_MOBILE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
getDefaultCaip25CaveatValue,
getPermittedAccounts,
} from '../../Permissions';
import { INTERNAL_ORIGINS } from '../../../constants/transaction';
import { rpcErrors } from '@metamask/rpc-errors';
import { areAddressesEqual, toFormattedAddress } from '../../../util/address';

export default class DeeplinkProtocolService {
Expand Down Expand Up @@ -492,7 +494,6 @@ export default class DeeplinkProtocolService {

return;
}

await SDKConnect.getInstance().addDappConnection({
id: clientInfo.clientId,
lastAuthorized: Date.now(),
Expand Down Expand Up @@ -533,6 +534,16 @@ export default class DeeplinkProtocolService {
params: any;
};

// Prevent external transactions from using internal origins
// This is an external connection (SDK), so block any internal origin
if (requestObject.method === 'eth_sendTransaction') {
if (INTERNAL_ORIGINS.includes(params.url)) {
throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
}
}

// Handle custom rpc method
const processedRpc = await handleCustomRpcCalls({
batchRPCManager: this.batchRPCManager,
Expand Down
12 changes: 12 additions & 0 deletions app/core/SDKConnect/handlers/setupBridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { Connection } from '../Connection';
import DevLogger from '../utils/DevLogger';
import handleSendMessage from './handleSendMessage';
import { ImageSourcePropType } from 'react-native';
import { INTERNAL_ORIGINS } from '../../../constants/transaction';
import { rpcErrors } from '@metamask/rpc-errors';

export const setupBridge = ({
originatorInfo,
Expand Down Expand Up @@ -55,6 +57,16 @@ export const setupBridge = ({
DevLogger.log(
`getRpcMethodMiddleware origin=${connection.origin} url=${originatorInfo.url} `,
);
// Prevent external connections from using internal origins
// This is an external connection (SDK), so block any internal origin
if (
INTERNAL_ORIGINS.includes(originatorInfo.url) ||
INTERNAL_ORIGINS.includes(originatorInfo.title)
) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing truthiness guards may block legitimate connections

The new INTERNAL_ORIGINS.includes() check lacks truthiness guards, unlike the existing check at lines 29-34. The INTERNAL_ORIGINS array contains process.env.MM_FOX_CODE, which may be undefined in some environments. If originatorInfo.url or originatorInfo.title is also undefined (possible since these come from external SDK types with optional fields), then INTERNAL_ORIGINS.includes(undefined) would return true, incorrectly blocking legitimate connections. The check should follow the same pattern as lines 29-34 by adding truthiness guards like originatorInfo.url && INTERNAL_ORIGINS.includes(originatorInfo.url).

Fix in Cursor Fix in Web

throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
}
return getRpcMethodMiddleware({
hostname: connection.origin,
channelId: connection.channelId,
Expand Down
16 changes: 10 additions & 6 deletions app/core/SDKConnectV2/services/connection-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import logger from './logger';
import { ACTIONS, PREFIXES } from '../../../constants/deeplinks';
import { decompressPayloadB64 } from '../utils/compression-utils';
import { whenStoreReady } from '../utils/when-store-ready';
import { ORIGIN_METAMASK } from '@metamask/controller-utils';
import { rpcErrors } from '@metamask/rpc-errors';
import { INTERNAL_ORIGINS } from '../../../constants/transaction';

/**
* The ConnectionRegistry is the central service responsible for managing the
Expand Down Expand Up @@ -111,15 +112,18 @@ export class ConnectionRegistry {

try {
const connReq = this.parseConnectionRequest(url);
// Prevent external connections from using internal origins
// This is an external connection (SDK V2), so block any internal origin
if (
(connReq.metadata.dapp.url &&
connReq.metadata.dapp.url === ORIGIN_METAMASK) ||
(connReq.metadata.dapp.name &&
connReq.metadata.dapp.name === ORIGIN_METAMASK)
INTERNAL_ORIGINS.includes(connReq.metadata.dapp.url) ||
INTERNAL_ORIGINS.includes(connReq.metadata.dapp.name)
) {
throw new Error('Connections from metamask origin are not allowed');
throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined values may incorrectly match in origin check

The INTERNAL_ORIGINS array includes process.env.MM_FOX_CODE, which can be undefined when the environment variable is not set. The new checks use INTERNAL_ORIGINS.includes(value) without truthiness guards. If a dapp's metadata.dapp.url or metadata.dapp.name is undefined AND MM_FOX_CODE is also undefined, then INTERNAL_ORIGINS.includes(undefined) returns true, incorrectly blocking legitimate external connections. The previous code pattern included explicit truthiness checks (e.g., connReq.metadata.dapp.url && ...) that prevented this issue.

Additional Locations (1)

Fix in Cursor Fix in Web

}
connInfo = this.toConnectionInfo(connReq);

this.hostapp.showConnectionLoading(connInfo);
conn = await Connection.create(
connInfo,
Expand Down
9 changes: 9 additions & 0 deletions app/core/WalletConnect/WalletConnect2Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { addTransaction } from '../../util/transaction-controller';
import BackgroundBridge from '../BackgroundBridge/BackgroundBridge';
import { Minimizer } from '../NativeModules';
import { getPermittedAccounts, getPermittedChains } from '../Permissions';
import { INTERNAL_ORIGINS } from '../../constants/transaction';
import getRpcMethodMiddleware, {
getRpcMethodMiddlewareHooks,
} from '../RPCMethods/RPCMethodMiddleware';
Expand Down Expand Up @@ -754,6 +755,14 @@ class WalletConnect2Session {
origin: string,
) {
try {
// Prevent external transactions from using internal origins
// This is an external connection (WalletConnect), so block any internal origin
if (INTERNAL_ORIGINS.includes(origin)) {
throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
}

const networkClientId = getNetworkClientIdForCaipChainId(caip2ChainId);
const trx = await addTransaction(methodParams[0], {
deviceConfirmedOn: WalletDevice.MM_MOBILE,
Expand Down
42 changes: 22 additions & 20 deletions app/core/WalletConnect/WalletConnectV2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1446,21 +1446,23 @@ describe('WC2Manager', () => {
});
});

describe.skip('Origin Rejection', () => {
describe('Origin Rejection', () => {
let rejectSessionSpy: jest.SpyInstance;

beforeEach(() => {
const web3Wallet = (manager as unknown as { web3Wallet: IWalletKit })
.web3Wallet;
rejectSessionSpy = jest.spyOn(web3Wallet, 'rejectSession');
rejectSessionSpy = jest
.spyOn(web3Wallet, 'rejectSession')
.mockResolvedValue(undefined);
});

it('should reject session proposal with "metamask" origin', async () => {
const proposal = {
id: 999,
params: {
id: 999,
pairingTopic: 'test-pairing',
pairingTopic: 'test-pairing-999',
proposer: {
publicKey: 'test-public-key',
metadata: {
Expand All @@ -1470,16 +1472,16 @@ describe('WC2Manager', () => {
icons: [],
},
},
requiredNamespaces: {},
optionalNamespaces: {},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
requiredNamespaces: {},
optionalNamespaces: {},
},
verifyContext: {
verified: {
verifyUrl: 'https://example.com',
verifyUrl: 'metamask',
validation: 'VALID' as const,
origin: 'https://example.com',
origin: 'metamask',
},
},
};
Expand All @@ -1502,7 +1504,7 @@ describe('WC2Manager', () => {
id: 997,
params: {
id: 997,
pairingTopic: 'test-pairing',
pairingTopic: 'test-pairing-997',
proposer: {
publicKey: 'test-public-key',
metadata: {
Expand All @@ -1512,6 +1514,8 @@ describe('WC2Manager', () => {
icons: [],
},
},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
requiredNamespaces: {
eip155: {
chains: ['eip155:1'],
Expand All @@ -1520,14 +1524,12 @@ describe('WC2Manager', () => {
},
},
optionalNamespaces: {},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
},
verifyContext: {
verified: {
verifyUrl: 'https://example.com',
verifyUrl: 'https://metamask.example.com',
validation: 'VALID' as const,
origin: 'https://example.com',
origin: 'https://metamask.example.com',
},
},
};
Expand All @@ -1550,7 +1552,7 @@ describe('WC2Manager', () => {
id: 996,
params: {
id: 996,
pairingTopic: 'test-pairing',
pairingTopic: 'test-pairing-996',
proposer: {
publicKey: 'test-public-key',
metadata: {
Expand All @@ -1560,6 +1562,8 @@ describe('WC2Manager', () => {
icons: [],
},
},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
requiredNamespaces: {
eip155: {
chains: ['eip155:1'],
Expand All @@ -1568,8 +1572,6 @@ describe('WC2Manager', () => {
},
},
optionalNamespaces: {},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
},
verifyContext: {
verified: {
Expand Down Expand Up @@ -1598,7 +1600,7 @@ describe('WC2Manager', () => {
id: 995,
params: {
id: 995,
pairingTopic: 'test-pairing',
pairingTopic: 'test-pairing-995',
proposer: {
publicKey: 'test-public-key',
metadata: {
Expand All @@ -1608,6 +1610,8 @@ describe('WC2Manager', () => {
icons: [],
},
},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
requiredNamespaces: {
eip155: {
chains: ['eip155:1'],
Expand All @@ -1616,14 +1620,12 @@ describe('WC2Manager', () => {
},
},
optionalNamespaces: {},
expiryTimestamp: Date.now() + 300000,
relays: [{ protocol: 'irn' }],
},
verifyContext: {
verified: {
verifyUrl: 'https://example.com',
verifyUrl: 'https://metamask.io',
validation: 'VALID' as const,
origin: 'https://example.com',
origin: 'https://metamask.io',
},
},
};
Expand Down
14 changes: 14 additions & 0 deletions app/core/WalletConnect/WalletConnectV2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { IWalletKit, WalletKit, WalletKitTypes } from '@reown/walletkit';
import { Core } from '@walletconnect/core';
import { SessionTypes } from '@walletconnect/types';
import { getSdkError } from '@walletconnect/utils';
import { rpcErrors } from '@metamask/rpc-errors';

import { updateWC2Metadata } from '../../../app/actions/sdk';
import { selectEvmChainId } from '../../selectors/networkController';
Expand All @@ -23,6 +24,7 @@ import {
getPermittedAccounts,
updatePermittedChains,
} from '../Permissions';
import { INTERNAL_ORIGINS } from '../../constants/transaction';
import DevLogger from '../SDKConnect/utils/DevLogger';
import getAllUrlParams from '../SDKConnect/utils/getAllUrlParams.util';
import { wait, waitForKeychainUnlocked } from '../SDKConnect/utils/wait.util';
Expand Down Expand Up @@ -468,6 +470,18 @@ export class WC2Manager {
const walletChainIdDecimal = parseInt(walletChainIdHex, 16);

try {
// Prevent external connections from using internal origins
// This is an external connection (WalletConnect), so block any internal origin
if (INTERNAL_ORIGINS.includes(origin)) {
await this.web3Wallet.rejectSession({
id: proposal.id,
reason: getSdkError('USER_REJECTED_METHODS'),
});
throw rpcErrors.invalidParams({
message: 'External transactions cannot use internal origins',
});
}

// Create a modified CAIP-25 caveat value that includes the current chain
const caveatValue = getDefaultCaip25CaveatValue();

Expand Down
8 changes: 4 additions & 4 deletions bitrise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3574,16 +3574,16 @@ app:
PROJECT_LOCATION_IOS: ios
- opts:
is_expand: false
VERSION_NAME: 7.61.3
VERSION_NAME: 7.61.4
- opts:
is_expand: false
VERSION_NUMBER: 3327
VERSION_NUMBER: 3335
- opts:
is_expand: false
FLASK_VERSION_NAME: 7.61.3
FLASK_VERSION_NAME: 7.61.4
- opts:
is_expand: false
FLASK_VERSION_NUMBER: 3327
FLASK_VERSION_NUMBER: 3335
- opts:
is_expand: false
ANDROID_APK_LINK: ''
Expand Down
Loading
Loading