Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: isValidASCIIURL should include path in check #29490

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Jan 7, 2025

Description

When the change network confirmation is shown, If there are non-ASCII characters in the URL, we should display a warning

example input: https://infura.io/gnosis?x=iոfura.io
new URL(input).href → 'https://infura.io/gnosis?x=i%D5%B8fura.io'

Related to: https://github.com/MetaMask/MetaMask-planning/issues/2365

Related issues

Fixes: #29489
Related to: https://github.com/MetaMask/MetaMask-planning/issues/2365

Manual testing steps

  1. from the console, call
 window.ethereum.request({
      "method": "wallet_addEthereumChain",
      "params": [
        {
          "chainId": "0x64",
          "chainName": "Gnosis",
          "rpcUrls": [
            "https://infura.io/gnosis?x=iոfura.io"
          ],
          "iconUrls": [
            "https://xdaichain.com/fake/example/url/xdai.svg",
            "https://xdaichain.com/fake/example/url/xdai.png"
          ],
          "nativeCurrency": {
            "name": "xDAI",
            "symbol": "xDAI",
            "decimals": 18
          },
          "blockExplorerUrls": [
            "https://blockscout.com/poa/xdai/"
          ]
        }
      ]
    });
2. observe expected error

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jan 7, 2025
@digiwand digiwand marked this pull request as ready for review January 7, 2025 23:23
@digiwand digiwand requested a review from a team as a code owner January 7, 2025 23:23
@metamaskbot
Copy link
Collaborator

Builds ready [376fcd1]
Page Load Metrics (1657 ± 75 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14242101165815574
domContentLoaded14082086163215273
load14172105165715675
domInteractive16178433718
backgroundConnect992262311
firstReactRender1577512411
getState46214178
initialActions0563126
loadScripts9711566121213364
setupStore619931
uiStartup16822427187717886

? undefined
: t('networkUrlErrorWarning', [toPunycodeURL(customRpcUrl)]),
[t('networkURL')]:
!customRpcUrl || isValidASCIIURL(customRpcUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: isValidASCIIURL takes care of scenario when customRpcUrl is not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want to show the error if there is no URL passed so we need to check !customRpcUrl here

jpuri
jpuri previously approved these changes Jan 8, 2025
matthewwalsh0
matthewwalsh0 previously approved these changes Jan 8, 2025
@digiwand digiwand dismissed stale reviews from matthewwalsh0 and jpuri via 1f62cba January 8, 2025 19:10
@metamaskbot
Copy link
Collaborator

Builds ready [1f62cba]
Page Load Metrics (1676 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30218681608317152
domContentLoaded1470181316409847
load14921872167610852
domInteractive24119382311
backgroundConnect1087372211
firstReactRender1670372210
getState589162010
initialActions01000
loadScripts1061137512309043
setupStore6511095
uiStartup16772453192517182

@metamaskbot
Copy link
Collaborator

Builds ready [36f4134]
Page Load Metrics (1485 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1321166914918441
domContentLoaded1316164914678039
load1320166014858742
domInteractive247042189
backgroundConnect76318167
firstReactRender1596282411
getState44510115
initialActions01000
loadScripts949123310847335
setupStore660212210
uiStartup15361961170411957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Network punycode warning does not check non-ASCII in path
5 participants