Skip to content
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,86 @@ describe('DeeplinkProtocolService', () => {
service.setupBridge(clientInfo);
expect(setupBridgeSpy).toHaveReturned();
});

it('should throw error when originatorInfo.url is "metamask"', () => {
const clientInfo: DappClient = {
clientId: 'client1',
originatorInfo: {
url: 'metamask',
title: 'Test',
platform: 'test',
dappId: 'dappId',
},
connected: false,
validUntil: Date.now(),
scheme: 'test',
};

expect(() => service.setupBridge(clientInfo)).toThrow(
'Connections from metamask origin are not allowed',
);
expect(service.bridgeByClientId[clientInfo.clientId]).toBeUndefined();
});

it('should throw error when originatorInfo.title is "metamask"', () => {
const clientInfo: DappClient = {
clientId: 'client1',
originatorInfo: {
url: 'https://example.com',
title: 'metamask',
platform: 'test',
dappId: 'dappId',
},
connected: false,
validUntil: Date.now(),
scheme: 'test',
};

expect(() => service.setupBridge(clientInfo)).toThrow(
'Connections from metamask origin are not allowed',
);
expect(service.bridgeByClientId[clientInfo.clientId]).toBeUndefined();
});

it('should allow connection when originatorInfo contains "metamask" as substring', () => {
const clientInfo: DappClient = {
clientId: 'client1',
originatorInfo: {
url: 'https://my-metamask-dapp.com',
title: 'My MetaMask App',
platform: 'test',
dappId: 'dappId',
},
connected: false,
validUntil: Date.now(),
scheme: 'test',
};

expect(() => service.setupBridge(clientInfo)).not.toThrow();
expect(service.bridgeByClientId[clientInfo.clientId]).toBeInstanceOf(
BackgroundBridge,
);
});

it('should allow connection when originatorInfo has valid url and title', () => {
const clientInfo: DappClient = {
clientId: 'client1',
originatorInfo: {
url: 'https://example.com',
title: 'Example Dapp',
platform: 'test',
dappId: 'dappId',
},
connected: false,
validUntil: Date.now(),
scheme: 'test',
};

expect(() => service.setupBridge(clientInfo)).not.toThrow();
expect(service.bridgeByClientId[clientInfo.clientId]).toBeInstanceOf(
BackgroundBridge,
);
});
Copy link

Choose a reason for hiding this comment

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

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)

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

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

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)

Fix in Cursor Fix in Web

});
describe('sendMessage', () => {
it('should handle sending messages correctly', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { KeyringController } from '@metamask/keyring-controller';
import { NetworkController } from '@metamask/network-controller';
import { PermissionController } from '@metamask/permission-controller';
import { OriginatorInfo } from '@metamask/sdk-communication-layer';
import {
ORIGIN_METAMASK,
toChecksumHexAddress,
} from '@metamask/controller-utils';
import { Linking } from 'react-native';
import { PROTOCOLS } from '../../../constants/deeplinks';
import AppConstants from '../../../core/AppConstants';
Expand All @@ -23,7 +27,6 @@ import handleCustomRpcCalls from '../handlers/handleCustomRpcCalls';
import DevLogger from '../utils/DevLogger';
import { wait, waitForKeychainUnlocked } from '../utils/wait.util';
import { AccountsController } from '@metamask/accounts-controller';
import { toChecksumHexAddress } from '@metamask/controller-utils';
import {
Caip25CaveatType,
Caip25EndowmentPermissionName,
Expand Down Expand Up @@ -100,6 +103,15 @@ export default class DeeplinkProtocolService {
return;
}

if (
(clientInfo.originatorInfo.url &&
clientInfo.originatorInfo.url === ORIGIN_METAMASK) ||
(clientInfo.originatorInfo.title &&
clientInfo.originatorInfo.title === ORIGIN_METAMASK)
) {
throw new Error('Connections from metamask origin are not allowed');
}

const defaultBridgeParams = getDefaultBridgeParams(clientInfo);

const bridge = new BackgroundBridge({
Expand Down
39 changes: 39 additions & 0 deletions app/core/SDKConnect/handlers/setupBridge.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,43 @@ describe('setupBridge', () => {
}),
);
});

it('should throw error when originatorInfo.url is "metamask"', () => {
connection.backgroundBridge = undefined;
originatorInfo.url = 'metamask';

expect(() => setupBridge({ originatorInfo, connection })).toThrow(
'Connections from metamask origin are not allowed',
);
expect(BackgroundBridge).not.toHaveBeenCalled();
});

it('should throw error when originatorInfo.title is "metamask"', () => {
connection.backgroundBridge = undefined;
originatorInfo.url = 'https://example.com';
originatorInfo.title = 'metamask';

expect(() => setupBridge({ originatorInfo, connection })).toThrow(
'Connections from metamask origin are not allowed',
);
expect(BackgroundBridge).not.toHaveBeenCalled();
});

it('should allow connection when originatorInfo contains "metamask" as substring', () => {
connection.backgroundBridge = undefined;
originatorInfo.url = 'https://my-metamask-dapp.com';
originatorInfo.title = 'My MetaMask App';

expect(() => setupBridge({ originatorInfo, connection })).not.toThrow();
expect(BackgroundBridge).toHaveBeenCalled();
});

it('should allow connection when originatorInfo has valid url and title', () => {
connection.backgroundBridge = undefined;
originatorInfo.url = 'https://example.com';
originatorInfo.title = 'Example Dapp';

expect(() => setupBridge({ originatorInfo, connection })).not.toThrow();
expect(BackgroundBridge).toHaveBeenCalled();
});
});
8 changes: 8 additions & 0 deletions app/core/SDKConnect/handlers/setupBridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import getRpcMethodMiddleware, {
} from '../../RPCMethods/RPCMethodMiddleware';

import { OriginatorInfo } from '@metamask/sdk-communication-layer';
import { ORIGIN_METAMASK } from '@metamask/controller-utils';
import Logger from '../../../util/Logger';
import { Connection } from '../Connection';
import DevLogger from '../utils/DevLogger';
Expand All @@ -22,6 +23,13 @@ export const setupBridge = ({
DevLogger.log(`setupBridge:: backgroundBridge already exists`);
return connection.backgroundBridge;
}

if (
(originatorInfo.url && originatorInfo.url === ORIGIN_METAMASK) ||
(originatorInfo.title && originatorInfo.title === ORIGIN_METAMASK)
) {
throw new Error('Connections from metamask origin are not allowed');
}
const backgroundBridge = new BackgroundBridge({
webview: null,
isMMSDK: true,
Expand Down
56 changes: 56 additions & 0 deletions app/core/SDKConnectV2/services/connection-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,62 @@ describe('ConnectionRegistry', () => {
expect(mockHostApp.hideConnectionLoading).toHaveBeenCalledTimes(1);
expect(mockStore.save).not.toHaveBeenCalled();
});

it('blocks connection requests with `metamask` as origin', async () => {
registry = new ConnectionRegistry(
RELAY_URL,
mockKeyManager,
mockHostApp,
mockStore,
);

const blockedRequest = {
...mockConnectionRequest,
metadata: {
...mockConnectionRequest.metadata,
dapp: {
...mockConnectionRequest.metadata.dapp,
url: 'metamask',
},
},
};

const blockedDeeplink = `metamask://connect/mwp?p=${encodeURIComponent(
JSON.stringify(blockedRequest),
)}`;

await registry.handleConnectDeeplink(blockedDeeplink);

expect(mockHostApp.showConnectionError).toHaveBeenCalledTimes(1);
expect(Connection.create).not.toHaveBeenCalled();
expect(mockStore.save).not.toHaveBeenCalled();
});

it('blocks connection requests with `metamask` as dapp name', async () => {
registry = new ConnectionRegistry(
RELAY_URL,
mockKeyManager,
mockHostApp,
mockStore,
);

const blockedRequest = {
...mockConnectionRequest,
metadata: {
...mockConnectionRequest.metadata,
dapp: {
...mockConnectionRequest.metadata.dapp,
name: 'metamask',
},
},
};

const blockedDeeplink = `metamask://connect/mwp?p=${encodeURIComponent(
JSON.stringify(blockedRequest),
)}`;

await registry.handleConnectDeeplink(blockedDeeplink);
});
Copy link

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

});

describe('disconnect', () => {
Expand Down
9 changes: 9 additions & 0 deletions app/core/SDKConnectV2/services/connection-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ACTIONS, PREFIXES } from '../../../constants/deeplinks';
import { decompressPayloadB64 } from '../utils/compression-utils';
import { whenStoreReady } from '../utils/when-store-ready';
import Engine from '../../Engine';
import { ORIGIN_METAMASK } from '@metamask/controller-utils';

/**
* The ConnectionRegistry is the central service responsible for managing the
Expand Down Expand Up @@ -169,6 +170,14 @@ export class ConnectionRegistry {

try {
const connReq = this.parseConnectionRequest(url);
if (
(connReq.metadata.dapp.url &&
connReq.metadata.dapp.url === ORIGIN_METAMASK) ||
(connReq.metadata.dapp.name &&
connReq.metadata.dapp.name === ORIGIN_METAMASK)
) {
throw new Error('Connections from metamask origin are not allowed');
}
connInfo = this.toConnectionInfo(connReq);
this.hostapp.showConnectionLoading(connInfo);
conn = await Connection.create(
Expand Down
Loading
Loading