fix(settings): fixed two IPFS gateway issues #19700

HowardBraham · 2023-06-21T05:22:37Z

Explanation

adds back in two bugfixes that were originally in feat(srp): add a quiz to the SRP reveal #19283
- fixes [Bug]: IPFS Gateway input not validated and seeing the encoded value on Advanced Settings #16871
- fixes [Bug]: When editing the IPFS Gateway URL - land on MetaMask encountered an error page #18140
achieves 100% code coverage for /ui/pages/settings/security-tab
removes the npm package valid-url, which has not been updated in 10 years

github-actions · 2023-06-21T05:22:53Z

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.

codecov · 2023-06-21T06:01:56Z

Codecov Report

Merging #19700 (777c5bf) into develop (e02f597) will increase coverage by 0.33%.
The diff coverage is 81.94%.

❗ Current head 777c5bf differs from pull request most recent head a82362e. Consider uploading reports for the commit a82362e to get more accurate results

@@             Coverage Diff             @@
##           develop   #19700      +/-   ##
===========================================
+ Coverage    68.84%   69.17%   +0.33%     
===========================================
  Files          993      994       +1     
  Lines        38258    38271      +13     
  Branches     10248    10251       +3     
===========================================
+ Hits         26338    26472     +134     
+ Misses       11920    11799     -121

Files Changed	Coverage Δ
ui/pages/keychains/reveal-seed.js	`98.75% <ø> (+1.22%)`	⬆️
...c-method-middleware/handlers/add-ethereum-chain.js	`9.15% <21.43%> (+0.49%)`	⬆️
app/scripts/lib/util.ts	`90.62% <92.31%> (+34.55%)`	⬆️
...ttings/networks-tab/networks-form/networks-form.js	`81.82% <100.00%> (-0.06%)`	⬇️
...es/settings/security-tab/security-tab.component.js	`100.00% <100.00%> (+22.83%)`	⬆️

... and 6 files with indirect coverage changes

socket-security · 2023-07-18T19:45:58Z

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: [email protected]

NidhiKJha

Valid-url removal is really nice. Though I can still enter IPFS with spaces. We need to validate that?

256262783-ecfb30b2-cc4a-4a3d-80eb-e4853af45734.mov

HowardBraham · 2023-07-27T01:47:05Z

Valid-url removal is really nice. Though I can still enter IPFS with spaces. We need to validate that?

This is a tricky situation, because a URL actually can have a %20 space in it. Is it done a lot? No.

How do we handle this in other places?

Gudahtt · 2023-07-28T15:01:52Z

Spaces are allowed in the path, but not the origin. We need to retain some amount of validation here to protect users from accidentally entering invalid URLs.

Elsewhere we validate using the URL constructor. Might that work here?

HowardBraham · 2023-07-28T21:40:22Z

@Gudahtt it's actually already validating through URL

Try new URL('https://a b'), it doesn't produce an error

I'm not actually sure if https://a%20b.com/ is a valid URL, I can't find any clear documentation

HowardBraham · 2023-07-31T22:37:20Z

@Gudahtt @NidhiKJha update... you cannot have a domain name with an encoded space in it because % is not a valid character. But... URL behaves differently in Node and browser!

Node new URL('https://a b.com') gives an error
Browser new URL('https://a b.com') is fine

Options:

Validate with another package, maybe is-url-http or url
Validate with a regex

Any preferences?

HowardBraham · 2023-08-03T07:50:29Z

@Gudahtt @NidhiKJha I fixed the URL validation this way:

const url = new URL(urlString);

if (url.hostname !== decodeURIComponent(url.hostname)) {
  return null; // will happen if there's a %, a space, or other invalid character in the hostname
}

It also now incorporates the changes from #20172

Hopefully should be good to approve and merge now

app/scripts/lib/util.ts

legobeat · 2023-08-03T13:08:57Z

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

@@ -83,27 +83,14 @@ async function addEthereumChainHandler(
    );
  }

-  const isLocalhost = (strUrl) => {


Looking this function properly for the first time, it was introduced in #14272.

I find these magic values problematic. What about foo.localhost, localhost.localdomain, self-mapped hostnames, or other loopback addresses like ::1 (i see this in particular regularly tripping people up) or the rest of 127.0.0.0/8? Sure it's usually possible to work around but seems arbitrary and could be a potential cause of hair-pulling in places. So this may need to be revised (possibly even resulting in new user-facing-preference(s)?). Heck, if someone wants to run a block explorer over file:// or some protocol we're not having in mind right now, why not let them?

While the changes in this PR still look like an improvement and resolving that aspect is probably out of scope here, moving this function out to util.ts invites use from other parts of the code, so perhaps it can be moved back and unexported for now?

Yeah I see what you mean... it's better, but not good enough to put in util. I moved the new version of it back to add-ethereum-chain.js.

metamaskbot · 2023-08-03T22:45:44Z

Builds ready [5050bc2]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1515 ± 58 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	106	189	127	19	9
		domContentLoaded	1361	1848	1514	120	58
		load	1361	1848	1515	120	58
		domInteractive	1361	1848	1514	120	58

Bundle size diffs [🚀 Bundle size reduced!]

background: -60 Bytes (-0.00%)
ui: -120 Bytes (-0.00%)
common: -1 KiB (-0.03%)

metamaskbot · 2023-08-05T06:41:21Z

Builds ready [777c5bf]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1472 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	106	191	131	24	12
		domContentLoaded	1325	1828	1472	103	50
		load	1325	1828	1472	103	50
		domInteractive	1325	1828	1472	103	50

Bundle size diffs [🚀 Bundle size reduced!]

background: -60 Bytes (-0.00%)
ui: -47 Bytes (-0.00%)
common: -1 KiB (-0.03%)

- adds back in two bugfixes that were originally in #19283 - fixes #16871 - fixes #18140 - achieves 100% code coverage for /ui/pages/settings/security-tab - removes the npm package `valid-url`, which has not been updated in 10 years

metamaskbot · 2023-08-08T04:30:47Z

Builds ready [a82362e]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1437 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	107	156	122	12	6
		domContentLoaded	1296	1670	1437	94	45
		load	1296	1670	1437	94	45
		domInteractive	1296	1670	1437	94	45

Bundle size diffs [🚀 Bundle size reduced!]

background: -60 Bytes (-0.00%)
ui: -47 Bytes (-0.00%)
common: -1 KiB (-0.03%)

app/scripts/lib/util.ts

HowardBraham force-pushed the fix/two-ipfs-issues branch 2 times, most recently from 5158666 to e684692 Compare June 21, 2023 05:32

This comment was marked as outdated.

Sign in to view

HowardBraham added the team-accounts label Jul 18, 2023

HowardBraham force-pushed the fix/two-ipfs-issues branch 2 times, most recently from bc924d8 to 74e3b25 Compare July 18, 2023 19:45

This comment was marked as outdated.

Sign in to view

HowardBraham force-pushed the fix/two-ipfs-issues branch from 4bba7de to 22193b8 Compare July 18, 2023 20:53

HowardBraham marked this pull request as ready for review July 18, 2023 20:55

HowardBraham requested a review from a team as a code owner July 18, 2023 20:55

HowardBraham force-pushed the fix/two-ipfs-issues branch from 22193b8 to b7713c5 Compare July 18, 2023 20:56

This comment was marked as outdated.

Sign in to view

plasmacorral added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Jul 18, 2023

HowardBraham assigned seaona Jul 19, 2023

HowardBraham requested a review from seaona July 19, 2023 00:52

HowardBraham mentioned this pull request Jul 25, 2023

Added IPFS toggle #20172

Merged

8 tasks

NidhiKJha reviewed Jul 26, 2023

View reviewed changes

HowardBraham force-pushed the fix/two-ipfs-issues branch 2 times, most recently from 3484147 to fa9aca7 Compare July 31, 2023 23:00

AlexJupiter added the type-bug label Aug 1, 2023

HowardBraham force-pushed the fix/two-ipfs-issues branch 2 times, most recently from 2c264d6 to fd24646 Compare August 3, 2023 04:42

This comment was marked as outdated.

Sign in to view

legobeat reviewed Aug 3, 2023

View reviewed changes

app/scripts/lib/util.ts Outdated Show resolved Hide resolved

legobeat reviewed Aug 3, 2023

View reviewed changes

HowardBraham force-pushed the fix/two-ipfs-issues branch from d978752 to 338c8ee Compare August 3, 2023 21:51

NidhiKJha previously approved these changes Aug 4, 2023

View reviewed changes

HowardBraham dismissed NidhiKJha’s stale review via 777c5bf August 5, 2023 06:04

HowardBraham force-pushed the fix/two-ipfs-issues branch from 5050bc2 to 777c5bf Compare August 5, 2023 06:04

HowardBraham added 6 commits August 7, 2023 20:54

fix(settings): fixed two IPFS gateway issues

9eee7dc

- adds back in two bugfixes that were originally in #19283 - fixes #16871 - fixes #18140 - achieves 100% code coverage for /ui/pages/settings/security-tab - removes the npm package `valid-url`, which has not been updated in 10 years

changes after #20172 was merged

2a25c2a

improved URL validation (specifically spaces)

0950d36

better Jest coverage

aaddb36

response to legobeat review

703b3da

fixing lint and Jest

a82362e

HowardBraham force-pushed the fix/two-ipfs-issues branch from 777c5bf to a82362e Compare August 8, 2023 03:54

NidhiKJha approved these changes Aug 14, 2023

View reviewed changes

HowardBraham requested review from legobeat and Gudahtt August 15, 2023 06:39

HowardBraham self-assigned this Aug 17, 2023

montelaidev reviewed Aug 22, 2023

View reviewed changes

app/scripts/lib/util.ts Show resolved Hide resolved

montelaidev approved these changes Aug 23, 2023

View reviewed changes

HowardBraham merged commit d3d30fd into develop Aug 23, 2023

HowardBraham deleted the fix/two-ipfs-issues branch August 23, 2023 05:13

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 23, 2023

github-actions bot locked and limited conversation to collaborators Aug 23, 2023

metamaskbot added the release-11.1.0 Issue or pull request that will be included in release 11.1.0 label Aug 23, 2023

Gudahtt removed the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(settings): fixed two IPFS gateway issues #19700

fix(settings): fixed two IPFS gateway issues #19700

HowardBraham commented Jun 21, 2023

github-actions bot commented Jun 21, 2023

codecov bot commented Jun 21, 2023 •

edited

Loading

This comment was marked as outdated.

socket-security bot commented Jul 18, 2023

This comment was marked as outdated.

This comment was marked as outdated.

NidhiKJha left a comment •

edited

Loading

HowardBraham commented Jul 27, 2023

Gudahtt commented Jul 28, 2023 •

edited

Loading

HowardBraham commented Jul 28, 2023

HowardBraham commented Jul 31, 2023

HowardBraham commented Aug 3, 2023

This comment was marked as outdated.

legobeat Aug 3, 2023 •

edited

Loading

HowardBraham Aug 3, 2023

metamaskbot commented Aug 3, 2023

metamaskbot commented Aug 5, 2023

metamaskbot commented Aug 8, 2023

fix(settings): fixed two IPFS gateway issues #19700

fix(settings): fixed two IPFS gateway issues #19700

Conversation

HowardBraham commented Jun 21, 2023

Explanation

github-actions bot commented Jun 21, 2023

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

This comment was marked as outdated.

socket-security bot commented Jul 18, 2023

This comment was marked as outdated.

This comment was marked as outdated.

NidhiKJha left a comment • edited Loading

Choose a reason for hiding this comment

HowardBraham commented Jul 27, 2023

Gudahtt commented Jul 28, 2023 • edited Loading

HowardBraham commented Jul 28, 2023

HowardBraham commented Jul 31, 2023

HowardBraham commented Aug 3, 2023

This comment was marked as outdated.

legobeat Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

HowardBraham Aug 3, 2023

Choose a reason for hiding this comment

metamaskbot commented Aug 3, 2023

metamaskbot commented Aug 5, 2023

metamaskbot commented Aug 8, 2023

codecov bot commented Jun 21, 2023 •

edited

Loading

NidhiKJha left a comment •

edited

Loading

Gudahtt commented Jul 28, 2023 •

edited

Loading

legobeat Aug 3, 2023 •

edited

Loading