feat: add Network URL non-ascii -> punycode warning #12813
Conversation
|
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. |
|
| console.error(exp); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
@digiwand : why is the check for valid ASCII includes only the host part of the URL ?
There was a problem hiding this comment.
hey @jpuri, good question. I copied this method verbatim from the metamask-extension code. I think you created the one in the metamask-extension.
will double-check this method with consideration of @NicholasEllul's comment above https://github.com/MetaMask/metamask-mobile/pull/12813/files#r1905686031
There was a problem hiding this comment.
please see #12813 (comment)
app/util/url/index.ts
Outdated
| @@ -24,6 +26,35 @@ export function isBridgeUrl(url: string) { | |||
| } | |||
| } | |||
|
|
|||
| export const isValidASCIIURL = (urlString?: string) => { | |||
| try { | |||
| return urlString?.includes(punycode.toASCII(new URL(urlString).host)); | |||
There was a problem hiding this comment.
This check is able to be bypassed when a URL string such as https://iոfura.io/gnosis?x=xn--ifura-dig.io is provided. Perhaps we can add this as a test case.
We will need to ensure we are comparing only to the host itself.
There was a problem hiding this comment.
hey @NicholasEllul and @jpuri, thanks for flagging this! This helper method was copied over from the extension. I updated the extension code and the code in this PR to include the path in its check. can I get another look at this please?
related extension PR: MetaMask/metamask-extension#29490
digiwand
added
Run Smoke E2E
|
app/util/url/index.ts
Outdated
| const urlPunycodeString = punycode.toASCII(new URL(urlString).href); | ||
| return urlPunycodeString?.includes(urlString); | ||
| } catch (exp: unknown) { | ||
| console.error(exp); |
There was a problem hiding this comment.
Minor, does this warrant an error log if it's handled?
And if we still want a log, should we add some context for clarity such as:
console.log('URL contains non-ASCII characters', urlString, urlPunycodeString)
There was a problem hiding this comment.
unsure if the punycode library could throw an error. Just in case, keeping the console.error and updating phrasing
updated console error to
console.error(`Failed to detect if URL contains non-ASCII characters: ${urlString}`);
app/util/url/index.ts
Outdated
| const pathname = | ||
| url.pathname === '/' && !urlString.endsWith('/') ? '' : url.pathname; | ||
|
|
||
| return `${protocol}//${punycodeHostname}${port}${pathname}${search}${hash}`; |
There was a problem hiding this comment.
Why do we need to build this manually and only convert the hostname to ASCII?
Could we pass the entire href in one go as we do in isValidASCIIURL?
There was a problem hiding this comment.
oh I originally proposed href then took it out to handle the pathname === '/' logic. Taking a second look at this we can keep the href and reword the logic. Thanks for the callout! updated 4c48863
There was a problem hiding this comment.
I notice the following notice in the punycode README: https://github.com/mathiasbynens/punycode.js?tab=readme-ov-file#installation
⚠️ Note that userland modules don't hide core modules. For example, require('punycode') still imports the deprecated core module even if you executed npm install punycode. Use require('punycode/') to import userland modules rather than core modules.
We should double check to ensure we are using the punycode built into node which is soft-deprecated. I notice other files like this one import punycode as the following:
app/util/url/index.ts
Outdated
| const urlPunycodeString = punycode.toASCII(new URL(urlString).href); | ||
| return urlPunycodeString?.includes(urlString); |
There was a problem hiding this comment.
From a security POV I believe this satisfies things! However, on the UX side of things there is another edge case.
The toASCII function is intended to only process domains or email addresses. This means the behaviour when processing full hrefs may result in undefined behaviour.
E.g if the url string in this case is https://opensea.io/language=français, the toASCII function will incorrectly turn it into https://example.xn--com/lang=franais-opb.
Potential solution:
By default in nodejs, if you call new URL(urlString) and later call url.hostname or url.href it should automatically punycode the hostname in whatever the URL object returns.
With this in mind could we do something like
hasPunycodeHostname = urlString !== (new URL(urlString).href);
...Im not super familiar with mobile's runtime, so this may or may not be possible in this environment, but if it is, it could help with that decoding.
There was a problem hiding this comment.
Ah I just noticed your note in the PR description:
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
So looks like this may not be possible
There was a problem hiding this comment.
yeah, the react-native runtime URL library didn't seem to support the punycode encoding
E.g if the url string in this case is https://opensea.io/language=français, the toASCII function will incorrectly turn it into https://example.xn--com/lang=franais-opb.
This is the reason why I was told we are displaying the given input rather than the punycode version. We show the punycode different in the warning. If only we could mimic the behavior of the browser search bar. An attempt at this would be out-of-scope
There was a problem hiding this comment.
@digiwand to avoid false flagging URLs that contain non-ascii characters in the path/query params, what if we switch to doing something like this:
const isValidASCIIURL = (urlString?: string) => {
if (!urlString) { return false; }
try {
const { hostname: originalHostname = new URL(urlString);
const punycodeHostname = toASCII(originalHostname);
return originalHostname === punycodeHostname
} catch (exp: unknown) {
console.error(`Failed to detect if URL contains non-ASCII characters: ${urlString}. Error: ${exp}`);
return false;
}
};This would ensure that we only fail on cases where the hostname itself contains a non-ascii character. This works because react-natives behaviour of not punycode encoding hostnames when accessed through the URL object.
However, we would need to add a test case that would fail if react native ever caught this change in the future
|
|
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:
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/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
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist