Skip to content

Commit

Permalink
feat: add Network URL non-ascii -> punycode warning (#12813)
Browse files Browse the repository at this point in the history
## **Description**

Similar to the extension, we want to warn the user if the URL has
non-ASCII characters. Inline alerts are not supported on mobile, so we
display the new alert as a banner.

Notes: 
- existing Network URL logic removes the path with hideKeyFromUrl
- punycode is deprecated, however, the url library does not seem to
parse the url into its punycode encoded version in react-native. It is
tricky as the url does parse into its punycode version in node.js, jest
tests

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2365
Related to: MetaMask/metamask-extension#29490
(fixes isValidASCIIURL to include path check in extension)


## **Manual testing steps**

Test switching to a custom network

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<img width="320"
src="https://github.com/user-attachments/assets/51c455ab-4f2e-4663-8bd1-305cfab6f3dd">

### **After**

<img width="320"
src="https://github.com/user-attachments/assets/8df714b1-6f9e-4915-8b6c-a5944c2d43c0">

## **Pre-merge author checklist**

- [ ] 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.

## **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.
  • Loading branch information
digiwand authored Jan 14, 2025
1 parent a4650ba commit 52a481d
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,37 @@ describe('NetworkVerificationInfo', () => {

expect(getByText('10')).toBeTruthy();
});

it('should not render Network URL warning banner when the custom rpc url has all ascii characters', () => {
(useSelector as jest.Mock).mockReturnValue(true);
const { getByText } = render(
<NetworkVerificationInfo
customNetworkInformation={mockNetworkInfo}
onReject={() => undefined}
onConfirm={() => undefined}
/>,
);

expect(() =>
getByText('Attackers sometimes mimic sites by making small changes to the site address. Make sure you\'re interacting with the intended Network URL before you continue. Punycode version: https://xn--ifura-dig.io/gnosis')
).toThrow('Unable to find an element with text');
});

describe('when the custom rpc url has non-ascii characters', () => {
it('should render Network URL warning banner and display punycode encoded version', () => {
(useSelector as jest.Mock).mockReturnValue(true);
const { getByText } = render(
<NetworkVerificationInfo
customNetworkInformation={{
...mockNetworkInfo,
rpcUrl: 'https://iոfura.io/gnosis',
}}
onReject={() => undefined}
onConfirm={() => undefined}
/>,
);

expect(getByText('Attackers sometimes mimic sites by making small changes to the site address. Make sure you\'re interacting with the intended Network URL before you continue. Punycode version: https://xn--ifura-dig.io/gnosis')).toBeTruthy();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { toggleUseSafeChainsListValidation } from '../../../util/networks/engine
import { NetworkApprovalBottomSheetSelectorsIDs } from '../../../../e2e/selectors/Network/NetworkApprovalBottomSheet.selectors';
import hideKeyFromUrl from '../../../util/hideKeyFromUrl';
import { convertHexToDecimal } from '@metamask/controller-utils';
import { isValidASCIIURL, toPunycodeURL } from '../../../util/url';

interface Alert {
alertError: string;
Expand Down Expand Up @@ -83,6 +84,8 @@ const NetworkVerificationInfo = ({
const showReviewDefaultRpcUrlChangesModal = () =>
setShowReviewDefaultRpcUrlChanges(!showReviewDefaultRpcUrlChanges);

const customRpcUrl = customNetworkInformation.rpcUrl;

const goToLearnMore = () => {
Linking.openURL(
'https://support.metamask.io/networks-and-sidechains/managing-networks/verifying-custom-network-information/',
Expand Down Expand Up @@ -237,7 +240,7 @@ const NetworkVerificationInfo = ({
</Text>
)}
<Text style={styles.textSection}>
{hideKeyFromUrl(customNetworkInformation.rpcUrl)}
{hideKeyFromUrl(customRpcUrl)}
</Text>

<Accordion
Expand Down Expand Up @@ -291,6 +294,25 @@ const NetworkVerificationInfo = ({
return null;
};

const renderBannerNetworkUrlNonAsciiDetected = () => {
if (!customRpcUrl || isValidASCIIURL(customRpcUrl)) { return null; }
const punycodeUrl = toPunycodeURL(customRpcUrl);

return (
<View style={styles.alertBar}>
<Banner
severity={BannerAlertSeverity.Warning}
variant={BannerVariant.Alert}
description={
strings('networks.network_rpc_url_warning_punycode') +
'\n' +
punycodeUrl
}
/>
</View>
);
};

const renderCustomNetworkBanner = () => (
<View style={styles.alertBar}>
<Banner
Expand Down Expand Up @@ -332,7 +354,7 @@ const NetworkVerificationInfo = ({
{strings('networks.current_label')}
</Text>
<Text style={styles.textSection}>
{customNetworkInformation.rpcUrl}
{customRpcUrl}
</Text>
<Text variant={TextVariant.BodyMDBold}>
{strings('networks.new_label')}
Expand Down Expand Up @@ -442,6 +464,7 @@ const NetworkVerificationInfo = ({
/>
{renderAlerts()}
{renderBanner()}
{renderBannerNetworkUrlNonAsciiDetected()}
{isMultichainVersion1Enabled &&
isCustomNetwork &&
renderCustomNetworkBanner()}
Expand Down
45 changes: 45 additions & 0 deletions app/util/url/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import AppConstants from '../../core/AppConstants';

/**
* "Use require('punycode/') to import userland modules rather than core modules."
* {@see {@link https://github.com/mathiasbynens/punycode.js?tab=readme-ov-file#installation}
*/
import { toASCII } from 'punycode/';

const hostnameRegex = /^(?:[a-zA-Z][a-zA-Z0-9+.-]*:\/\/)?(?:www\.)?([^/?:]+)(?::\d+)?/;

export function isPortfolioUrl(url: string) {
try {
const currentUrl = new URL(url);
Expand All @@ -24,6 +32,43 @@ export function isBridgeUrl(url: string) {
}
}

/**
* This method does not use the URL library because it does not support punycode encoding in react native.
* It compares the original hostname to a punycode version of the hostname.
*/
export const isValidASCIIURL = (urlString?: string) => {
if (!urlString || urlString.length === 0) {
return false;
}

try {
const originalHostname = urlString.match(hostnameRegex);
const punycodeHostname = toASCII(originalHostname?.[1] || '');
return originalHostname?.[1] === punycodeHostname;
} catch (exp: unknown) {
console.error(`Failed to detect if URL hostname contains non-ASCII characters: ${urlString}. Error: ${exp}`);
return false;
}
};

function removePathTrailingSlash(path: string) {
return path.endsWith('/') ? path.slice(0, -1) : path;
}

/**
* Note: We use the punycode library here because the URL library in react native doesn't support punycode encoding.
* We do have the 'react-native-url-polyfill' package which supports the URL library, but it doesn't support punycode encoding.
* The URL library is supported in node.js which allows tests to pass, but behavior differs in react-native runtime.
*/
export const toPunycodeURL = (urlString: string) => {
try {
const url = new URL(urlString);
const punycodeUrl = toASCII(url.href);
const isWithoutEndSlash = url.pathname === '/' && !urlString.endsWith('/');

return isWithoutEndSlash ? punycodeUrl.slice(0, -1) : punycodeUrl;
} catch (err: unknown) {
console.error(`Failed to convert URL to Punycode: ${err}`);
return urlString;
}
};
54 changes: 53 additions & 1 deletion app/util/url/url.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isPortfolioUrl, isBridgeUrl } from './index';
import { isPortfolioUrl, isBridgeUrl, isValidASCIIURL, toPunycodeURL } from './index';
import AppConstants from '../../core/AppConstants';

describe('URL Check Functions', () => {
Expand Down Expand Up @@ -60,4 +60,56 @@ describe('URL Check Functions', () => {
expect(isBridgeUrl(url)).toBe(false);
});
});

describe('isValidASCIIURL', () => {
it('returns true for URL containing only ASCII characters in its hostname', () => {
expect(isValidASCIIURL('https://www.google.com')).toEqual(true);
});

it('returns true for URL with both its hostname and path containing ASCII characters', () => {
expect(
isValidASCIIURL('https://infura.io/gnosis?x=xn--ifura-dig.io'),
).toStrictEqual(true);
});

it('returns true for URL with its hostname containing ASCII characters and its path containing non-ASCII characters', () => {
expect(isValidASCIIURL('https://infura.io/gnosis?x=iոfura.io')).toStrictEqual(true);
expect(isValidASCIIURL('infura.io:7777/gnosis?x=iոfura.io')).toStrictEqual(true);
});

it('returns false for URL with its hostname containing non-ASCII characters', () => {
expect(isValidASCIIURL('https://iոfura.io/gnosis')).toStrictEqual(false);
expect(isValidASCIIURL('iոfura.io:7777/gnosis?x=test')).toStrictEqual(false);
});

it('returns false for empty string', () => {
expect(isValidASCIIURL('')).toStrictEqual(false);
});
});

describe('toPunycodeURL', () => {
it('returns punycode version of URL', () => {
expect(toPunycodeURL('https://iոfura.io/gnosis')).toStrictEqual(
'https://xn--ifura-dig.io/gnosis',
);
expect(toPunycodeURL('https://iոfura.io')).toStrictEqual(
'https://xn--ifura-dig.io',
);
expect(toPunycodeURL('https://iոfura.io/')).toStrictEqual(
'https://xn--ifura-dig.io/',
);
expect(
toPunycodeURL('https://iոfura.io/gnosis:5050?test=iոfura&foo=bar'),
).toStrictEqual(
'https://xn--ifura-dig.io/gnosis:5050?test=i%D5%B8fura&foo=bar',
);

expect(toPunycodeURL('https://www.google.com')).toStrictEqual(
'https://www.google.com',
);
expect(toPunycodeURL('https://opensea.io/language=français')).toStrictEqual(
'https://opensea.io/language=fran%C3%A7ais',
);
});
});
});
1 change: 1 addition & 0 deletions locales/languages/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -1814,6 +1814,7 @@
"network_chain_id": "Chain ID",
"network_rpc_url": "Network URL",
"network_rpc_url_label": "Network RPC URL",
"network_rpc_url_warning_punycode": "Attackers sometimes mimic sites by making small changes to the site address. Make sure you're interacting with the intended Network URL before you continue. Punycode version:",
"new_default_network_url": "New default network URL",
"current_label": "Current",
"new_label": "New",
Expand Down

0 comments on commit 52a481d

Please sign in to comment.