update ens name validation

package.json

@@ -75,6 +75,7 @@
   "dependencies": {
     "await-semaphore": "^0.1.3",
     "eth-contract-metadata": "^1.9.1",
+    "eth-ens-namehash": "^2.0.8",
     "eth-json-rpc-infura": "^4.0.1",
     "eth-keyring-controller": "^5.0.1",
     "eth-method-registry": "1.1.0",

src/third-party/EnsController.ts

@@ -1,5 +1,6 @@
-import BaseController, { BaseConfig, BaseState } from '../BaseController';
 import { isValidAddress, toChecksumAddress } from 'ethereumjs-util';
+import BaseController, { BaseConfig, BaseState } from '../BaseController';
+import { normalizeEnsName } from '../util';
 
 /**
  * @type EnsEntry
@@ -8,7 +9,7 @@ import { isValidAddress, toChecksumAddress } from 'ethereumjs-util';
  *
  * @property chainId - Id of the associated chain
  * @property ensName - The ENS name
- * @property address - Hex address with the ENS name
+ * @property address - Hex address with the ENS name, or null
  */
 export interface EnsEntry {
 	chainId: string;
@@ -29,7 +30,7 @@ export interface EnsState extends BaseState {
 
 /**
  * Controller that manages a list ENS names and their resolved addresses
- * by chainId
+ * by chainId. A null address indicates an unresolved ENS name.
  */
 export class EnsController extends BaseController<BaseConfig, EnsState> {
 	/**
@@ -59,18 +60,25 @@ export class EnsController extends BaseController<BaseConfig, EnsState> {
 	}
 
 	/**
-	 * Remove a contract entry by address
+	 * Delete an ENS entry.
 	 *
 	 * @param chainId - Parent chain of the ENS entry to delete
 	 * @param ensName - Name of the ENS entry to delete
+	 *
+	 * @returns - Boolean indicating if the entry was deleted
 	 */
-	delete(chainId: string, ensName: string) {
-		if (!this.state.ensEntries[chainId] || !this.state.ensEntries[chainId][ensName]) {
+	delete(chainId: string, ensName: string): boolean {
+		const normalizedEnsName = normalizeEnsName(ensName);
+		if (
+			!normalizedEnsName ||
+			!this.state.ensEntries[chainId] ||
+			!this.state.ensEntries[chainId][normalizedEnsName]
+		) {
 			return false;
 		}
 
 		const ensEntries = Object.assign({}, this.state.ensEntries);
-		delete ensEntries[chainId][ensName];
+		delete ensEntries[chainId][normalizedEnsName];
 
 		if (Object.keys(ensEntries[chainId]).length === 0) {
 			delete ensEntries[chainId];
@@ -80,13 +88,30 @@ export class EnsController extends BaseController<BaseConfig, EnsState> {
 		return true;
 	}
 
+	/**
+	 * Retrieve a DNS entry.
+	 *
+	 * @param chainId - Parent chain of the ENS entry to retrieve
+	 * @param ensName - Name of the ENS entry to retrieve
+	 *
+	 * @returns - The EnsEntry or null if it does not exist
+	 */
+	get(chainId: string, ensName: string): EnsEntry | null {
+		const normalizedEnsName = normalizeEnsName(ensName);
+
+		return !!normalizedEnsName && this.state.ensEntries[chainId]
+			? this.state.ensEntries[chainId][normalizedEnsName] || null
+			: null;
+	}
+
 	/**
 	 * Add or update an ENS entry by chainId and ensName
 	 *
 	 * @param chainId - Id of the associated chain
 	 * @param ensName - The ENS name
 	 * @param address - Associated address to add or update
-	 * @returns - Boolean indicating whether the entry was set
+	 *
+	 * @returns - Boolean indicating if the entry was set
 	 */
 	set(chainId: string, ensName: string, address: string | null): boolean {
 		if (
@@ -98,10 +123,15 @@ export class EnsController extends BaseController<BaseConfig, EnsState> {
 			throw new Error(`Invalid ENS entry: { chainId:${chainId}, ensName:${ensName}, address:${address}}`);
 		}
 
+		const normalizedEnsName = normalizeEnsName(ensName);
+		if (!normalizedEnsName) {
+			throw new Error(`Invalid ENS name: ${ensName}`);
+		}
+
 		const normalizedAddress = address ? toChecksumAddress(address) : null;
 		const subState = this.state.ensEntries[chainId];
 
-		if (subState && subState[ensName] && subState[ensName].address === normalizedAddress) {
+		if (subState && subState[normalizedEnsName] && subState[normalizedEnsName].address === normalizedAddress) {
 			return false;
 		}
 
@@ -110,10 +140,10 @@ export class EnsController extends BaseController<BaseConfig, EnsState> {
 				...this.state.ensEntries,
 				[chainId]: {
 					...this.state.ensEntries[chainId],
-					[ensName]: {
+					[normalizedEnsName]: {
 						address: normalizedAddress,
 						chainId,
-						ensName
+						ensName: normalizedEnsName
 					}
 				}
 			}

src/user/AddressBookController.ts

@@ -1,5 +1,5 @@
 import { isValidAddress, toChecksumAddress } from 'ethereumjs-util';
-import { isValidEnsName } from '../util';
+import { normalizeEnsName } from '../util';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 
 /**
@@ -112,18 +112,26 @@ export class AddressBookController extends BaseController<BaseConfig, AddressBoo
 			return false;
 		}
 
+		const entry = {
+			address,
+			chainId,
+			isEns: false,
+			memo,
+			name
+		};
+
+		const ensName = normalizeEnsName(name);
+		if (ensName) {
+			entry.name = ensName;
+			entry.isEns = true;
+		}
+
 		this.update({
 			addressBook: {
 				...this.state.addressBook,
 				[chainId]: {
 					...this.state.addressBook[chainId],
-					[address]: {
-						address,
-						chainId,
-						isEns: isValidEnsName(name),
-						memo,
-						name
-					}
+					[address]: entry
 				}
 			}
 		});

src/util.ts

@@ -7,6 +7,7 @@ import { Token } from './assets/TokenRatesController';
 const sigUtil = require('eth-sig-util');
 const jsonschema = require('jsonschema');
 const { BN, stripHexPrefix } = require('ethereumjs-util');
+const ensNamehash = require('eth-ens-namehash');
 const hexRe = /^[0-9A-Fa-f]+$/g;
 
 const NORMALIZERS: { [param in keyof Transaction]: any } = {
@@ -320,9 +321,26 @@ export async function timeoutFetch(url: string, options?: RequestInit, timeout:
 	]);
 }
 
-export function isValidEnsName(ensName: string) {
-	const regex = /^.{7,}\.(eth|test)$/;
-	return regex.test(ensName);
+/**
+ * Normalizes the given ENS name.
+ *
+ * @param {string} ensName - The ENS name
+ *
+ * @returns - the normalized ENS name string
+ */
+export function normalizeEnsName(ensName: string): string | null {
+	if (ensName && typeof ensName === 'string') {
+		try {
+			const normalized = ensNamehash.normalize(ensName.trim());
+			// change 7 in regex to 3 when shorter ENS domains are live
+			if (normalized.match(/^(([\w\d]+)\.)*[\w\d]{7,}\.(eth|test)$/)) {
+				return normalized;
+			}
+		} catch (_) {
+			// do nothing
+		}
+	}
+	return null;
 }
 
 export default {
@@ -333,7 +351,6 @@ export default {
 	hexToBN,
 	hexToText,
 	isSmartContractCode,
-	isValidEnsName,
 	normalizeTransaction,
 	safelyExecute,
 	timeoutFetch,

tests/EnsController.test.ts

@@ -5,8 +5,8 @@ import EnsController from '../src/third-party/EnsController';
 const address1 = '0x32Be343B94f860124dC4fEe278FDCBD38C102D88';
 const address2 = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d';
 const address3 = '0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359';
-const name1 = 'foo.eth';
-const name2 = 'bar.eth';
+const name1 = 'foobarb.eth';
+const name2 = 'bazbarb.eth';
 
 const address1Checksum = toChecksumAddress(address1);
 const address2Checksum = toChecksumAddress(address2);
@@ -149,10 +149,48 @@ describe('EnsController', () => {
 		});
 	});
 
-	it('should throw on attempt to set invalid ENS entry', () => {
+	it('should get ENS entry by chainId and ensName', () => {
+		const controller = new EnsController();
+		expect(controller.set('1', name1, address1)).toBeTruthy();
+		expect(controller.get('1', name1)).toEqual({
+			address: address1Checksum,
+			chainId: '1',
+			ensName: name1
+		});
+	});
+
+	it('should return null when getting nonexistent name', () => {
+		const controller = new EnsController();
+		expect(controller.set('1', name1, address1)).toBeTruthy();
+		expect(controller.get('1', name2)).toEqual(null);
+	});
+
+	it('should return null when getting nonexistent chainId', () => {
+		const controller = new EnsController();
+		expect(controller.set('1', name1, address1)).toBeTruthy();
+		expect(controller.get('2', name1)).toEqual(null);
+	});
+
+	it('should throw on attempt to set invalid ENS entry: chainId', () => {
+		const controller = new EnsController();
+		expect(() => {
+			controller.set('a', name1, address1);
+		}).toThrowError();
+		expect(controller.state).toEqual({ ensEntries: {} });
+	});
+
+	it('should throw on attempt to set invalid ENS entry: ENS name', () => {
+		const controller = new EnsController();
+		expect(() => {
+			controller.set('1', 'foo.eth', address1);
+		}).toThrowError();
+		expect(controller.state).toEqual({ ensEntries: {} });
+	});
+
+	it('should throw on attempt to set invalid ENS entry: address', () => {
 		const controller = new EnsController();
 		expect(() => {
-			controller.set('1', '1337', 'foo');
+			controller.set('1', name1, 'foo');
 		}).toThrowError();
 		expect(controller.state).toEqual({ ensEntries: {} });
 	});

tests/util.test.ts

@@ -454,14 +454,56 @@ describe('util', () => {
 			expect(error.message).toBe('timeout');
 		});
 	});
-	describe('isValidEnsName', () => {
-		it('should return if the ens name is valid by current standards', async () => {
-			const valid = util.isValidEnsName('metamask.eth');
-			expect(valid).toBeTruthy();
-		});
-		it('should return if the ens name is invalid by current standards', async () => {
-			const invalid = util.isValidEnsName('me.eth');
-			expect(invalid).toBeFalsy();
+
+	describe('normalizeEnsName', () => {
+		it('should normalize with valid 2LD', async () => {
+			const valid = util.normalizeEnsName('metamask.eth');
+			expect(valid).toEqual('metamask.eth');
+		});
+
+		it('should normalize with valid 2LD and "test" TLD', async () => {
+			const valid = util.normalizeEnsName('metamask.eth');
+			expect(valid).toEqual('metamask.eth');
+		});
+
+		it('should normalize with valid 2LD and 3LD', async () => {
+			const valid = util.normalizeEnsName('a.metamask.eth');
+			expect(valid).toEqual('a.metamask.eth');
+		});
+
+		it('should return null with invalid 2LD', async () => {
+			const invalid = util.normalizeEnsName('me.eth');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with valid 2LD and invalid 3LD', async () => {
+			const invalid = util.normalizeEnsName('@foo.metamask.eth');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with invalid 2LD and valid 3LD', async () => {
+			const invalid = util.normalizeEnsName('foo.barbaz.eth');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with invalid TLD', async () => {
+			const invalid = util.normalizeEnsName('a.metamask.com');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with repeated periods', async () => {
+			const invalid = util.normalizeEnsName('foo.metamask..eth');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with repeated periods', async () => {
+			const invalid = util.normalizeEnsName('foo..metamask.eth');
+			expect(invalid).toEqual(null);
+		});
+
+		it('should return null with empty string', async () => {
+			const invalid = util.normalizeEnsName('');
+			expect(invalid).toEqual(null);
 		});
 	});
 });