From cd8e488f8e8c1e5452f354e3a0fb9b0f50ecf564 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 11 Mar 2024 10:35:52 -0400 Subject: [PATCH 01/15] Add checks for malicious input --- .../address-book-controller/src/AddressBookController.ts | 1 + packages/ens-controller/src/EnsController.ts | 2 ++ packages/name-controller/src/NameController.ts | 7 +++++++ 3 files changed, 10 insertions(+) diff --git a/packages/address-book-controller/src/AddressBookController.ts b/packages/address-book-controller/src/AddressBookController.ts index 830e642f4d9..e78bb6f7dcd 100644 --- a/packages/address-book-controller/src/AddressBookController.ts +++ b/packages/address-book-controller/src/AddressBookController.ts @@ -110,6 +110,7 @@ export class AddressBookController extends BaseControllerV1< delete(chainId: Hex, address: string) { address = toChecksumHexAddress(address); if ( + [chainId, address].includes('__proto__') || !isValidHexAddress(address) || !this.state.addressBook[chainId] || !this.state.addressBook[chainId][address] diff --git a/packages/ens-controller/src/EnsController.ts b/packages/ens-controller/src/EnsController.ts index 76013ad6a9f..1a3c4f88bf8 100644 --- a/packages/ens-controller/src/EnsController.ts +++ b/packages/ens-controller/src/EnsController.ts @@ -194,6 +194,8 @@ export class EnsController extends BaseController< delete(chainId: Hex, ensName: string): boolean { const normalizedEnsName = normalizeEnsName(ensName); if ( + // @ts-expect-error suppressing to perform runtime check + chainId === '__proto__' || !normalizedEnsName || !this.state.ensEntries[chainId] || !this.state.ensEntries[chainId][normalizedEnsName] diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index 197015dd894..5a92822c95b 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -441,6 +441,13 @@ export class NameController extends BaseController< const normalizedValue = this.#normalizeValue(value, type); const normalizedVariation = this.#normalizeVariation(variationKey, type); + if ( + normalizedValue === '__proto__' || + normalizedVariation === '__proto__' + ) { + return; + } + this.update((state) => { const typeEntries = state.names[type] || {}; state.names[type] = typeEntries; From 08880ac824fa679e5f84a09e0e47bb48879af026 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 11 Mar 2024 10:48:26 -0400 Subject: [PATCH 02/15] Adjust jest coverage thresholds --- packages/name-controller/jest.config.js | 6 +++--- packages/name-controller/src/NameController.ts | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/name-controller/jest.config.js b/packages/name-controller/jest.config.js index ca084133399..5c18f6bd511 100644 --- a/packages/name-controller/jest.config.js +++ b/packages/name-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 100, + branches: 99.15, functions: 100, - lines: 100, - statements: 100, + lines: 99.72, + statements: 99.72, }, }, }); diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index 5a92822c95b..45a65e4f8fd 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -441,10 +441,7 @@ export class NameController extends BaseController< const normalizedValue = this.#normalizeValue(value, type); const normalizedVariation = this.#normalizeVariation(variationKey, type); - if ( - normalizedValue === '__proto__' || - normalizedVariation === '__proto__' - ) { + if ([normalizedValue, normalizedVariation].includes('__proto__')) { return; } From b1767ca6f27ea5c79718e5b2be732d0646ef753a Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 12 Mar 2024 20:40:07 -0400 Subject: [PATCH 03/15] Define `isSafeDynamicKey` method and `PROTOTYPE_POLLUTION_BLOCKLIST` --- packages/controller-utils/jest.config.js | 8 ++++---- packages/controller-utils/src/util.ts | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index e77cd278680..032f4d14e19 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 68.05, - functions: 80.55, - lines: 69.82, - statements: 70.17, + branches: 77.65, + functions: 81.25, + lines: 86.87, + statements: 86.74, }, }, diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index ed2503cd087..ee56c666133 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -16,6 +16,23 @@ import { MAX_SAFE_CHAIN_ID } from './constants'; const TIMEOUT_ERROR = new Error('timeout'); +const PROTOTYPE_POLLUTION_BLOCKLIST = [ + '__proto__', + 'constructor', + 'defineProperty', +] as const; + +/** + * Checks whether a dynamic string used as an object key is a dangerous string + * that makes the object vulnerable to a prototype pollution attack. + * + * @param key - The dynamic key to check for safety. + * @returns Whether the given dyanmic key is safe + */ +export function isSafeDynamicKey(key: string): boolean { + return PROTOTYPE_POLLUTION_BLOCKLIST.every((badInput) => key !== badInput); +} + /** * Checks whether the given number primitive chain ID is safe. * Because some cryptographic libraries we use expect the chain ID to be a From d85f0f5ec61d7092baeb9d6057eddf86b8697d52 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 12 Mar 2024 20:40:27 -0400 Subject: [PATCH 04/15] Rewrite prototype pollution checks to utilize `isSafeDynamicKey` --- .../address-book-controller/src/AddressBookController.ts | 3 ++- packages/ens-controller/src/EnsController.ts | 4 ++-- packages/name-controller/src/NameController.ts | 7 ++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/address-book-controller/src/AddressBookController.ts b/packages/address-book-controller/src/AddressBookController.ts index e78bb6f7dcd..e4cb4e87f9b 100644 --- a/packages/address-book-controller/src/AddressBookController.ts +++ b/packages/address-book-controller/src/AddressBookController.ts @@ -7,6 +7,7 @@ import { toHex, } from '@metamask/controller-utils'; import type { Hex } from '@metamask/utils'; +import { isSafeDynamicKey } from '../../controller-utils/src/util'; /** * @type ContactEntry @@ -110,7 +111,7 @@ export class AddressBookController extends BaseControllerV1< delete(chainId: Hex, address: string) { address = toChecksumHexAddress(address); if ( - [chainId, address].includes('__proto__') || + ![chainId, address].every((key) => isSafeDynamicKey(key)) || !isValidHexAddress(address) || !this.state.addressBook[chainId] || !this.state.addressBook[chainId][address] diff --git a/packages/ens-controller/src/EnsController.ts b/packages/ens-controller/src/EnsController.ts index 1a3c4f88bf8..79939eb72c2 100644 --- a/packages/ens-controller/src/EnsController.ts +++ b/packages/ens-controller/src/EnsController.ts @@ -18,6 +18,7 @@ import type { NetworkState } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createProjectLogger } from '@metamask/utils'; import { toASCII } from 'punycode/'; +import { isSafeDynamicKey } from '../../controller-utils/src/util'; const log = createProjectLogger('ens-controller'); @@ -194,8 +195,7 @@ export class EnsController extends BaseController< delete(chainId: Hex, ensName: string): boolean { const normalizedEnsName = normalizeEnsName(ensName); if ( - // @ts-expect-error suppressing to perform runtime check - chainId === '__proto__' || + !isSafeDynamicKey(chainId) || !normalizedEnsName || !this.state.ensEntries[chainId] || !this.state.ensEntries[chainId][normalizedEnsName] diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index 45a65e4f8fd..dd4abeffe70 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -12,6 +12,7 @@ import type { NameProviderSourceResult, } from './types'; import { NameType } from './types'; +import { isSafeDynamicKey } from '../../controller-utils/src/util'; export const FALLBACK_VARIATION = '*'; export const PROPOSED_NAME_EXPIRE_DURATION = 60 * 60 * 24; // 24 hours @@ -441,7 +442,11 @@ export class NameController extends BaseController< const normalizedValue = this.#normalizeValue(value, type); const normalizedVariation = this.#normalizeVariation(variationKey, type); - if ([normalizedValue, normalizedVariation].includes('__proto__')) { + if ( + ![normalizedValue, normalizedVariation].every((key) => + isSafeDynamicKey(key), + ) + ) { return; } From d978fad87b19bc149f02353618f8ecacd01d075b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 12 Mar 2024 20:51:18 -0400 Subject: [PATCH 05/15] Add `@metamask/controller-utils` as dep to name-controller --- packages/name-controller/package.json | 1 + yarn.lock | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/name-controller/package.json b/packages/name-controller/package.json index a8ecafea8dd..fdcd8a36a05 100644 --- a/packages/name-controller/package.json +++ b/packages/name-controller/package.json @@ -43,6 +43,7 @@ }, "dependencies": { "@metamask/base-controller": "^5.0.1", + "@metamask/controller-utils": "^9.0.1", "@metamask/utils": "^8.3.0", "async-mutex": "^0.2.6" }, diff --git a/yarn.lock b/yarn.lock index 4b7282d1e1b..42b688a3033 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2540,6 +2540,7 @@ __metadata: dependencies: "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^5.0.1 + "@metamask/controller-utils": ^9.0.1 "@metamask/utils": ^8.3.0 "@types/jest": ^27.4.1 async-mutex: ^0.2.6 From 4d59ca1257390014641a3b954ee94475c9d3330f Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 12 Mar 2024 20:51:23 -0400 Subject: [PATCH 06/15] Fix import statements from controller-utils --- packages/address-book-controller/src/AddressBookController.ts | 2 +- packages/ens-controller/src/EnsController.ts | 2 +- packages/name-controller/src/NameController.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/address-book-controller/src/AddressBookController.ts b/packages/address-book-controller/src/AddressBookController.ts index e4cb4e87f9b..1101dffc94a 100644 --- a/packages/address-book-controller/src/AddressBookController.ts +++ b/packages/address-book-controller/src/AddressBookController.ts @@ -3,11 +3,11 @@ import { BaseControllerV1 } from '@metamask/base-controller'; import { normalizeEnsName, isValidHexAddress, + isSafeDynamicKey, toChecksumHexAddress, toHex, } from '@metamask/controller-utils'; import type { Hex } from '@metamask/utils'; -import { isSafeDynamicKey } from '../../controller-utils/src/util'; /** * @type ContactEntry diff --git a/packages/ens-controller/src/EnsController.ts b/packages/ens-controller/src/EnsController.ts index 79939eb72c2..898c1697c9a 100644 --- a/packages/ens-controller/src/EnsController.ts +++ b/packages/ens-controller/src/EnsController.ts @@ -9,6 +9,7 @@ import type { ChainId } from '@metamask/controller-utils'; import { normalizeEnsName, isValidHexAddress, + isSafeDynamicKey, toChecksumHexAddress, CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP, convertHexToDecimal, @@ -18,7 +19,6 @@ import type { NetworkState } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createProjectLogger } from '@metamask/utils'; import { toASCII } from 'punycode/'; -import { isSafeDynamicKey } from '../../controller-utils/src/util'; const log = createProjectLogger('ens-controller'); diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index dd4abeffe70..e2434d7095e 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -4,6 +4,7 @@ import type { RestrictedControllerMessenger, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; +import { isSafeDynamicKey } from '@metamask/controller-utils'; import type { NameProvider, @@ -12,7 +13,6 @@ import type { NameProviderSourceResult, } from './types'; import { NameType } from './types'; -import { isSafeDynamicKey } from '../../controller-utils/src/util'; export const FALLBACK_VARIATION = '*'; export const PROPOSED_NAME_EXPIRE_DURATION = 60 * 60 * 24; // 24 hours From a2a561a894ed3d17e7fae1b03830e3d18ddb93cd Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 13 Mar 2024 12:00:58 -0400 Subject: [PATCH 07/15] Remove 'defineProperty' from blocklist --- packages/controller-utils/src/util.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index ee56c666133..7f4cd0f0368 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -16,18 +16,14 @@ import { MAX_SAFE_CHAIN_ID } from './constants'; const TIMEOUT_ERROR = new Error('timeout'); -const PROTOTYPE_POLLUTION_BLOCKLIST = [ - '__proto__', - 'constructor', - 'defineProperty', -] as const; +const PROTOTYPE_POLLUTION_BLOCKLIST = ['__proto__', 'constructor'] as const; /** - * Checks whether a dynamic string used as an object key is a dangerous string - * that makes the object vulnerable to a prototype pollution attack. + * Checks whether a dynamic string used as an object property key + * could be used in a prototype pollution attack. * * @param key - The dynamic key to check for safety. - * @returns Whether the given dyanmic key is safe + * @returns Whether the given dyanmic key is safe to use. */ export function isSafeDynamicKey(key: string): boolean { return PROTOTYPE_POLLUTION_BLOCKLIST.every((badInput) => key !== badInput); From a145237447a84bf3fa534ad0bc54cd7b22601a37 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 14 Mar 2024 13:01:19 -0400 Subject: [PATCH 08/15] test: Write unit tests for `isSafeDynamicKey` validation --- .../src/AddressBookController.test.ts | 7 +++ .../ens-controller/src/EnsController.test.ts | 10 ++++ packages/name-controller/jest.config.js | 6 +- .../src/NameController.test.ts | 58 +++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/packages/address-book-controller/src/AddressBookController.test.ts b/packages/address-book-controller/src/AddressBookController.test.ts index baaf512347d..3d8c42915e0 100644 --- a/packages/address-book-controller/src/AddressBookController.test.ts +++ b/packages/address-book-controller/src/AddressBookController.test.ts @@ -294,6 +294,13 @@ describe('AddressBookController', () => { ).toBe(true); }); + it('should return false to indicate an address book entry has NOT been deleted due to unsafe input', () => { + const controller = new AddressBookController(); + // @ts-expect-error Suppressing error to test runtime behavior + expect(controller.delete('__proto__', '0x01')).toBe(false); + expect(controller.delete(toHex(1), 'constructor')).toBe(false); + }); + it('should return false to indicate an address book entry has NOT been deleted', () => { const controller = new AddressBookController(); controller.set('0x32Be343B94f860124dC4fEe278FDCBD38C102D88', '0x00'); diff --git a/packages/ens-controller/src/EnsController.test.ts b/packages/ens-controller/src/EnsController.test.ts index 735945b26c0..9a49dfe2caf 100644 --- a/packages/ens-controller/src/EnsController.test.ts +++ b/packages/ens-controller/src/EnsController.test.ts @@ -359,6 +359,16 @@ describe('EnsController', () => { expect(controller.state.ensEntries['0x1']).toBeUndefined(); }); + it('should return false if an ENS entry was NOT deleted due to unsafe input', () => { + const messenger = getMessenger(); + const controller = new EnsController({ + messenger, + }); + // @ts-expect-error Suppressing error to test runtime behavior + expect(controller.delete('__proto__', 'bar')).toBe(false); + expect(controller.delete(toHex(2), 'constructor')).toBe(false); + }); + it('should return false if an ENS entry was NOT deleted', () => { const messenger = getMessenger(); const controller = new EnsController({ diff --git a/packages/name-controller/jest.config.js b/packages/name-controller/jest.config.js index 5c18f6bd511..ca084133399 100644 --- a/packages/name-controller/jest.config.js +++ b/packages/name-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 99.15, + branches: 100, functions: 100, - lines: 99.72, - statements: 99.72, + lines: 100, + statements: 100, }, }, }); diff --git a/packages/name-controller/src/NameController.test.ts b/packages/name-controller/src/NameController.test.ts index 9159e256ee5..d05d3fa80a4 100644 --- a/packages/name-controller/src/NameController.test.ts +++ b/packages/name-controller/src/NameController.test.ts @@ -458,6 +458,64 @@ describe('NameController', () => { }); }); + it('does not update if passed unsafe input', () => { + const provider1 = createMockProvider(1); + + const controller = new NameController({ + ...CONTROLLER_ARGS_MOCK, + providers: [provider1], + state: { + names: { + [NameType.ETHEREUM_ADDRESS]: { + [VALUE_MOCK]: { + [CHAIN_ID_MOCK]: { + name: null, + sourceId: null, + origin: null, + proposedNames: { + [SOURCE_ID_MOCK]: { + proposedNames: [PROPOSED_NAME_MOCK, PROPOSED_NAME_2_MOCK], + lastRequestTime: null, + updateDelay: null, + }, + }, + }, + }, + }, + }, + }, + }); + + controller.setName({ + value: '__proto__', + type: NameType.ETHEREUM_ADDRESS, + name: NAME_MOCK, + sourceId: `${SOURCE_ID_MOCK}1`, + variation: CHAIN_ID_MOCK, + }); + + expect(controller.state.names).toStrictEqual< + NameControllerState['names'] + >({ + [NameType.ETHEREUM_ADDRESS]: { + [VALUE_MOCK]: { + [CHAIN_ID_MOCK]: { + name: null, + sourceId: null, + origin: null, + proposedNames: { + [SOURCE_ID_MOCK]: { + proposedNames: [PROPOSED_NAME_MOCK, PROPOSED_NAME_2_MOCK], + lastRequestTime: null, + updateDelay: null, + }, + }, + }, + }, + }, + }); + }); + it('does not throw if variation is fallback and type is Ethereum address', () => { const controller = new NameController(CONTROLLER_ARGS_MOCK); From 64f0b6d06519e7494ff4af4459a5544d6a0ecb32 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 14 Mar 2024 13:13:33 -0400 Subject: [PATCH 09/15] Apply suggestion for readability Co-authored-by: Elliot Winkel --- packages/name-controller/src/NameController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index e2434d7095e..c4d534d955b 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -443,8 +443,8 @@ export class NameController extends BaseController< const normalizedVariation = this.#normalizeVariation(variationKey, type); if ( - ![normalizedValue, normalizedVariation].every((key) => - isSafeDynamicKey(key), + [normalizedValue, normalizedVariation].some( + (key) => !isSafeDynamicKey(key), ) ) { return; From 4d634eaf77f83b1608997cda7041a86b135647e7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 14 Mar 2024 16:22:32 -0400 Subject: [PATCH 10/15] Update `isSafeDynamicKey` and JSDoc for readability, add `prototype` to blocklist --- packages/controller-utils/jest.config.js | 2 +- packages/controller-utils/src/util.ts | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index 032f4d14e19..9b2fe680d53 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -19,7 +19,7 @@ module.exports = merge(baseConfig, { global: { branches: 77.65, functions: 81.25, - lines: 86.87, + lines: 86.53, statements: 86.74, }, }, diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 7f4cd0f0368..769e82995fe 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -16,17 +16,23 @@ import { MAX_SAFE_CHAIN_ID } from './constants'; const TIMEOUT_ERROR = new Error('timeout'); -const PROTOTYPE_POLLUTION_BLOCKLIST = ['__proto__', 'constructor'] as const; +const PROTOTYPE_POLLUTION_BLOCKLIST = [ + '__proto__', + 'constructor', + 'prototype', +] as const; /** - * Checks whether a dynamic string used as an object property key - * could be used in a prototype pollution attack. + * Checks whether a dynamic property key could be used in + * a [prototype pollution attack](https://portswigger.net/web-security/prototype-pollution). * - * @param key - The dynamic key to check for safety. - * @returns Whether the given dyanmic key is safe to use. + * @param key - The dynamic key to validate. + * @returns Whether the given dynamic key is safe to use. */ export function isSafeDynamicKey(key: string): boolean { - return PROTOTYPE_POLLUTION_BLOCKLIST.every((badInput) => key !== badInput); + return !PROTOTYPE_POLLUTION_BLOCKLIST.some( + (blockedKey) => key === blockedKey, + ); } /** From 3a342157cada9b1802cd28c4da299f00e0400c90 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 15 Mar 2024 17:08:03 -0400 Subject: [PATCH 11/15] Remove wildcard export for `util`, add `PROTOTYPE_POLLUTION_BLOCKLIST` as non-package level export --- packages/controller-utils/src/index.ts | 30 +++++++++++++++++++++++++- packages/controller-utils/src/util.ts | 2 +- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/controller-utils/src/index.ts b/packages/controller-utils/src/index.ts index 14a22e8e00e..d0ad340bb8c 100644 --- a/packages/controller-utils/src/index.ts +++ b/packages/controller-utils/src/index.ts @@ -1,4 +1,32 @@ export * from './constants'; -export * from './util'; +export type { NonEmptyArray } from './util'; +export { + isSafeDynamicKey, + isSafeChainId, + BNToHex, + fractionBN, + gweiDecToWEIBN, + weiHexToGweiDec, + getBuyURL, + hexToBN, + hexToText, + fromHex, + toHex, + safelyExecute, + safelyExecuteWithTimeout, + toChecksumHexAddress, + isValidHexAddress, + isSmartContractCode, + successfulFetch, + handleFetch, + fetchWithErrorHandling, + timeoutFetch, + normalizeEnsName, + query, + convertHexToDecimal, + isPlainObject, + isNonEmptyArray, + isValidJson, +} from './util'; export * from './types'; export * from './siwe'; diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 769e82995fe..2b402aa58e8 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -16,7 +16,7 @@ import { MAX_SAFE_CHAIN_ID } from './constants'; const TIMEOUT_ERROR = new Error('timeout'); -const PROTOTYPE_POLLUTION_BLOCKLIST = [ +export const PROTOTYPE_POLLUTION_BLOCKLIST = [ '__proto__', 'constructor', 'prototype', From 1d09bee105100d6c4ad840368e96f51bc99d0b66 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 15 Mar 2024 17:08:34 -0400 Subject: [PATCH 12/15] Add runtime check for input type to `isSafeDynamicKey` --- packages/controller-utils/src/util.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 2b402aa58e8..12eaf3db2aa 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -30,8 +30,9 @@ export const PROTOTYPE_POLLUTION_BLOCKLIST = [ * @returns Whether the given dynamic key is safe to use. */ export function isSafeDynamicKey(key: string): boolean { - return !PROTOTYPE_POLLUTION_BLOCKLIST.some( - (blockedKey) => key === blockedKey, + return ( + typeof key === 'string' && + !PROTOTYPE_POLLUTION_BLOCKLIST.some((blockedKey) => key === blockedKey) ); } From 03333afd8517922f14cf0da4dc42b06f45e1991c Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 15 Mar 2024 17:08:56 -0400 Subject: [PATCH 13/15] test: Write unit tests for `isSafeDyanmicKey` --- packages/controller-utils/jest.config.js | 8 ++++---- packages/controller-utils/src/util.test.ts | 11 ++++++++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index 9b2fe680d53..1d042ba6d40 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 77.65, - functions: 81.25, - lines: 86.53, - statements: 86.74, + branches: 78.12, + functions: 85.41, + lines: 87.3, + statements: 86.5, }, }, diff --git a/packages/controller-utils/src/util.test.ts b/packages/controller-utils/src/util.test.ts index bbf993af5cc..00108328088 100644 --- a/packages/controller-utils/src/util.test.ts +++ b/packages/controller-utils/src/util.test.ts @@ -11,12 +11,21 @@ const SOME_API = 'https://someapi.com'; const SOME_FAILING_API = 'https://somefailingapi.com'; describe('util', () => { + it('isSafeDynamicKey', () => { + expect(util.isSafeDynamicKey(util.toHex(MAX_SAFE_CHAIN_ID))).toBe(true); + expect(util.isSafeDynamicKey('')).toBe(true); + for (const badKey of util.PROTOTYPE_POLLUTION_BLOCKLIST) { + expect(util.isSafeDynamicKey(badKey)).toBe(false); + } + // @ts-expect-error - ensure that non-string input return false. + expect(util.isSafeDynamicKey(null)).toBe(false); + }); it('isSafeChainId', () => { expect(util.isSafeChainId(util.toHex(MAX_SAFE_CHAIN_ID + 1))).toBe(false); expect(util.isSafeChainId(util.toHex(MAX_SAFE_CHAIN_ID))).toBe(true); expect(util.isSafeChainId(util.toHex(0))).toBe(false); expect(util.isSafeChainId('0xinvalid')).toBe(false); - // @ts-expect-error - ensure that string args return false. + // @ts-expect-error - ensure that non-string args return false. expect(util.isSafeChainId('test')).toBe(false); }); From bad6ed416f82659d9cf6e545f30a6e6d55124c1d Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 15 Mar 2024 15:05:04 -0700 Subject: [PATCH 14/15] sort package-level exports list in alphabetic order --- packages/controller-utils/src/index.ts | 34 +++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/controller-utils/src/index.ts b/packages/controller-utils/src/index.ts index d0ad340bb8c..265872e6204 100644 --- a/packages/controller-utils/src/index.ts +++ b/packages/controller-utils/src/index.ts @@ -1,32 +1,32 @@ export * from './constants'; export type { NonEmptyArray } from './util'; export { - isSafeDynamicKey, - isSafeChainId, BNToHex, + convertHexToDecimal, + fetchWithErrorHandling, fractionBN, - gweiDecToWEIBN, - weiHexToGweiDec, + fromHex, getBuyURL, + gweiDecToWEIBN, + handleFetch, hexToBN, hexToText, - fromHex, - toHex, + isNonEmptyArray, + isPlainObject, + isSafeChainId, + isSafeDynamicKey, + isSmartContractCode, + isValidJson, + isValidHexAddress, + normalizeEnsName, + query, safelyExecute, safelyExecuteWithTimeout, - toChecksumHexAddress, - isValidHexAddress, - isSmartContractCode, successfulFetch, - handleFetch, - fetchWithErrorHandling, timeoutFetch, - normalizeEnsName, - query, - convertHexToDecimal, - isPlainObject, - isNonEmptyArray, - isValidJson, + toChecksumHexAddress, + toHex, + weiHexToGweiDec, } from './util'; export * from './types'; export * from './siwe'; From f6c3c47e67d0402ca4abbd43fec5ce6dc7e6b118 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 20 Mar 2024 19:14:30 -0400 Subject: [PATCH 15/15] Fix lockfile --- packages/name-controller/package.json | 2 +- yarn.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/name-controller/package.json b/packages/name-controller/package.json index 9910620b5c6..cc0889cf8b5 100644 --- a/packages/name-controller/package.json +++ b/packages/name-controller/package.json @@ -43,7 +43,7 @@ }, "dependencies": { "@metamask/base-controller": "^5.0.1", - "@metamask/controller-utils": "^9.0.1", + "@metamask/controller-utils": "^9.0.2", "@metamask/utils": "^8.3.0", "async-mutex": "^0.2.6" }, diff --git a/yarn.lock b/yarn.lock index 0e35707225d..bba2431f2fc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2540,7 +2540,7 @@ __metadata: dependencies: "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^5.0.1 - "@metamask/controller-utils": ^9.0.1 + "@metamask/controller-utils": ^9.0.2 "@metamask/utils": ^8.3.0 "@types/jest": ^27.4.1 async-mutex: ^0.2.6