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.

Sign up for GitHub

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(settings): fixed two IPFS gateway issues #19700

Merged
merged 6 commits into from
Aug 23, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9eee7dc
fix(settings): fixed two IPFS gateway issues
HowardBraham Jul 18, 2023
2a25c2a
changes after #20172 was merged
HowardBraham Aug 2, 2023
0950d36
improved URL validation (specifically spaces)
HowardBraham Aug 3, 2023
aaddb36
better Jest coverage
HowardBraham Aug 3, 2023
703b3da
response to legobeat review
HowardBraham Aug 3, 2023
a82362e
fixing lint and Jest
HowardBraham Aug 3, 2023
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
36 changes: 17 additions & 19 deletions app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { ethErrors, errorCodes } from 'eth-rpc-errors';
import validUrl from 'valid-url';
import { omit } from 'lodash';
import { ApprovalType } from '@metamask/controller-utils';
import { errorCodes, ethErrors } from 'eth-rpc-errors';
import { omit } from 'lodash';
import {
MESSAGE_TYPE,
UNKNOWN_TICKER_SYMBOL,
} from '../../../../../shared/constants/app';
import { MetaMetricsNetworkEventSource } from '../../../../../shared/constants/metametrics';
import {
isPrefixedFormattedHexString,
isSafeChainId,
} from '../../../../../shared/modules/network.utils';
import { MetaMetricsNetworkEventSource } from '../../../../../shared/constants/metametrics';
import { getValidUrl } from '../../util';

const addEthereumChain = {
methodNames: [MESSAGE_TYPE.ADD_ETHEREUM_CHAIN],
Expand Down Expand Up @@ -83,27 +83,25 @@ async function addEthereumChainHandler(
);
}

const isLocalhost = (strUrl) => {
try {
Copy link
Contributor

@legobeat legobeat Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const url = new URL(strUrl);
return url.hostname === 'localhost' || url.hostname === '127.0.0.1';
} catch (error) {
return false;
}
};
function isLocalhostOrHttps(urlString) {
const url = getValidUrl(urlString);

return (
url !== null &&
(url.hostname === 'localhost' ||
url.hostname === '127.0.0.1' ||
url.protocol === 'https:')
);
}

const firstValidRPCUrl = Array.isArray(rpcUrls)
? rpcUrls.find(
(rpcUrl) => isLocalhost(rpcUrl) || validUrl.isHttpsUri(rpcUrl),
)
? rpcUrls.find((rpcUrl) => isLocalhostOrHttps(rpcUrl))
: null;

const firstValidBlockExplorerUrl =
blockExplorerUrls !== null && Array.isArray(blockExplorerUrls)
? blockExplorerUrls.find(
(blockExplorerUrl) =>
isLocalhost(blockExplorerUrl) ||
validUrl.isHttpsUri(blockExplorerUrl),
? blockExplorerUrls.find((blockExplorerUrl) =>
isLocalhostOrHttps(blockExplorerUrl),
)
: null;

Expand Down
52 changes: 44 additions & 8 deletions app/scripts/lib/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
import { isPrefixedFormattedHexString } from '../../../shared/modules/network.utils';
import {
ENVIRONMENT_TYPE_POPUP,
ENVIRONMENT_TYPE_NOTIFICATION,
ENVIRONMENT_TYPE_FULLSCREEN,
ENVIRONMENT_TYPE_BACKGROUND,
PLATFORM_FIREFOX,
PLATFORM_OPERA,
ENVIRONMENT_TYPE_FULLSCREEN,
ENVIRONMENT_TYPE_NOTIFICATION,
ENVIRONMENT_TYPE_POPUP,
PLATFORM_CHROME,
PLATFORM_EDGE,
PLATFORM_FIREFOX,
PLATFORM_OPERA,
} from '../../../shared/constants/app';
import {
TransactionEnvelopeType,
TransactionStatus,
TransactionType,
TransactionEnvelopeType,
} from '../../../shared/constants/transaction';
import { isPrefixedFormattedHexString } from '../../../shared/modules/network.utils';
import {
addUrlProtocolPrefix,
deferredPromise,
formatTxMetaForRpcResult,
getEnvironmentType,
getPlatform,
formatTxMetaForRpcResult,
getValidUrl,
isWebUrl,
} from './util';

describe('app utils', () => {
Expand Down Expand Up @@ -73,6 +76,39 @@ describe('app utils', () => {
});
});

describe('URL utils', () => {
it('should test addUrlProtocolPrefix', () => {
expect(addUrlProtocolPrefix('http://example.com')).toStrictEqual(
'http://example.com',
);
expect(addUrlProtocolPrefix('https://example.com')).toStrictEqual(
'https://example.com',
);
expect(addUrlProtocolPrefix('example.com')).toStrictEqual(
'https://example.com',
);
expect(addUrlProtocolPrefix('exa mple.com')).toStrictEqual(null);
});

it('should test isWebUrl', () => {
expect(isWebUrl('http://example.com')).toStrictEqual(true);
expect(isWebUrl('https://example.com')).toStrictEqual(true);
expect(isWebUrl('https://exa mple.com')).toStrictEqual(false);
expect(isWebUrl('')).toStrictEqual(false);
});

it('should test getValidUrl', () => {
expect(getValidUrl('http://example.com').toString()).toStrictEqual(
'http://example.com/',
);
expect(getValidUrl('https://example.com').toString()).toStrictEqual(
'https://example.com/',
);
expect(getValidUrl('https://exa%20mple.com')).toStrictEqual(null);
expect(getValidUrl('')).toStrictEqual(null);
});
});

describe('isPrefixedFormattedHexString', () => {
it('should return true for valid hex strings', () => {
expect(isPrefixedFormattedHexString('0x1')).toStrictEqual(true);
Expand Down
67 changes: 50 additions & 17 deletions app/scripts/lib/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
import urlLib from 'url';
import { AccessList } from '@ethereumjs/tx';
import BN from 'bn.js';
import { memoize } from 'lodash';
import { AccessList } from '@ethereumjs/tx';
import { CHAIN_IDS, TEST_CHAINS } from '../../../shared/constants/network';

import {
ENVIRONMENT_TYPE_POPUP,
ENVIRONMENT_TYPE_NOTIFICATION,
ENVIRONMENT_TYPE_FULLSCREEN,
ENVIRONMENT_TYPE_BACKGROUND,
PLATFORM_FIREFOX,
PLATFORM_OPERA,
ENVIRONMENT_TYPE_FULLSCREEN,
ENVIRONMENT_TYPE_NOTIFICATION,
ENVIRONMENT_TYPE_POPUP,
PLATFORM_BRAVE,
PLATFORM_CHROME,
PLATFORM_EDGE,
PLATFORM_BRAVE,
PLATFORM_FIREFOX,
PLATFORM_OPERA,
} from '../../../shared/constants/app';
import { stripHexPrefix } from '../../../shared/modules/hexstring-utils';
import { CHAIN_IDS, TEST_CHAINS } from '../../../shared/constants/network';
import {
TransactionEnvelopeType,
TransactionMeta,
} from '../../../shared/constants/transaction';
import { stripHexPrefix } from '../../../shared/modules/hexstring-utils';

/**
* @see {@link getEnvironmentType}
Expand Down Expand Up @@ -143,13 +143,13 @@ function checkAlarmExists(alarmList: { name: string }[], alarmName: string) {
}

export {
getPlatform,
getEnvironmentType,
hexToBn,
BnMultiplyByFraction,
addHexPrefix,
getChainType,
checkAlarmExists,
getChainType,
getEnvironmentType,
getPlatform,
hexToBn,
};

// Taken from https://stackoverflow.com/a/1349426/3696652
Expand Down Expand Up @@ -235,10 +235,43 @@ export function previousValueComparator<A>(
}

export function addUrlProtocolPrefix(urlString: string) {
if (!urlString.match(/(^http:\/\/)|(^https:\/\/)/u)) {
return `https://${urlString}`;
let trimmed = urlString.trim();

if (trimmed.length && !urlLib.parse(trimmed).protocol) {
trimmed = `https://${trimmed}`;
}
return urlString;

if (getValidUrl(trimmed) !== null) {
montelaidev marked this conversation as resolved.
Show resolved Hide resolved
return trimmed;
}

return null;
}

export function getValidUrl(urlString: string): URL | null {
try {
const url = new URL(urlString);

if (url.hostname.length === 0 || url.pathname.length === 0) {
return null;
}

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

return url;
} catch (error) {
return null;
}
}

export function isWebUrl(urlString: string): boolean {
const url = getValidUrl(urlString);

return (
url !== null && (url.protocol === 'https:' || url.protocol === 'http:')
);
}

interface FormattedTransactionMeta {
Expand Down
5 changes: 2 additions & 3 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ module.exports = {
'<rootDir>/app/scripts/controllers/sign.ts',
'<rootDir>/app/scripts/controllers/decrypt-message.ts',
'<rootDir>/app/scripts/flask/**/*.js',
'<rootDir>/app/scripts/lib/**/*.js',
'<rootDir>/app/scripts/lib/**/*.(js|ts)',
'<rootDir>/app/scripts/lib/createRPCMethodTrackingMiddleware.js',
'<rootDir>/app/scripts/migrations/*.js',
'<rootDir>/app/scripts/migrations/*.ts',
Expand Down Expand Up @@ -42,8 +42,7 @@ module.exports = {
'<rootDir>/app/scripts/controllers/sign.test.ts',
'<rootDir>/app/scripts/controllers/decrypt-message.test.ts',
'<rootDir>/app/scripts/flask/**/*.test.js',
'<rootDir>/app/scripts/lib/**/*.test.js',
'<rootDir>/app/scripts/lib/**/*.test.ts',
'<rootDir>/app/scripts/lib/**/*.test.(js|ts)',
'<rootDir>/app/scripts/lib/createRPCMethodTrackingMiddleware.test.js',
'<rootDir>/app/scripts/migrations/*.test.(js|ts)',
'<rootDir>/app/scripts/platforms/*.test.js',
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,6 @@
"single-call-balance-checker-abi": "^1.0.0",
"unicode-confusables": "^0.1.1",
"uuid": "^8.3.2",
"valid-url": "^1.0.9",
"web3-stream-provider": "^4.0.0",
"zxcvbn": "^4.4.2"
},
Expand Down
2 changes: 2 additions & 0 deletions test/data/mock-state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@
"unapprovedTypedMessagesCount": 0,
"useTokenDetection": true,
"useCurrencyRateCheck": true,
"useNftDetection": true,
"openSeaEnabled": true,
"advancedGasFee": {
"maxBaseFee": "75",
"priorityFee": "2"
Expand Down
2 changes: 1 addition & 1 deletion ui/pages/keychains/reveal-seed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ const RevealSeedPage = () => {

const renderPasswordPromptContent = () => {
return (
<form onSubmit={(event) => handleSubmit(event)}>
<form onSubmit={handleSubmit}>
<Label htmlFor="password-box">{t('enterPasswordContinue')}</Label>
<TextField
inputProps={{
Expand Down
Loading
Loading