From 7c8590f1261d74914120cb11c430b9c05abeb354 Mon Sep 17 00:00:00 2001 From: Daniel McNally Date: Thu, 30 Jan 2020 00:06:32 -0500 Subject: [PATCH] feat(config): property type and value checking This adds assertion logic to the `Config` class to validate that any properties specified are of the expected type and, for certain properties, that the value is valid. This may prevent crashes or other unexpected behavior in xud due to invalid configuration, and will also log more instructive errors when a specified property is invalid. Closes #1222. --- lib/Config.ts | 47 +++++++++++++++++++++++++++++++++++----- lib/Xud.ts | 14 +++++++++++- test/jest/Config.spec.ts | 22 +++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/lib/Config.ts b/lib/Config.ts index 7b585c3e9..26251c442 100644 --- a/lib/Config.ts +++ b/lib/Config.ts @@ -1,14 +1,47 @@ +import assert from 'assert'; +import { promises as fs } from 'fs'; import os from 'os'; import path from 'path'; import toml from 'toml'; -import { deepMerge } from './utils/utils'; -import { promises as fs } from 'fs'; +import { XuNetwork } from './constants/enums'; import { LndClientConfig } from './lndclient/types'; -import { RaidenClientConfig } from './raidenclient/types'; import { Level } from './Logger'; -import { XuNetwork } from './constants/enums'; -import { PoolConfig } from './p2p/types'; import { OrderBookThresholds } from './orderbook/types'; +import { PoolConfig } from './p2p/types'; +import { RaidenClientConfig } from './raidenclient/types'; +import { deepMerge } from './utils/utils'; + +const propAssertions = { + port: (val: number) => assert(val >= 0 && val <= 65535, 'port must be between 0 and 65535'), + cltvdelta: (val: number) => assert(val > 0, 'cltvdelta must be a positive number'), + discoverminutes: (val: number) => assert(val > 0, 'discoverminutes must be a positive number'), + minQuantity: (val: number) => assert(val >= 0, 'minQuantity must be 0 or a positive number'), +}; + +function validateConfig(propVal: any, defaultVal: any, propKey?: string, prefix?: string) { + const actualType = typeof propVal; + const expectedType = typeof defaultVal; + if (actualType === 'undefined') { + return; // this is an unspecified property that will use the default value + } + if (expectedType === 'undefined') { + return; // this is a superfluous property that we ignore for now + } + assert.equal(actualType, expectedType, `${prefix || ''}${propKey} is type ${actualType} but should be ${expectedType}`); + + if (actualType === 'object') { + // if this is an object, we recurse + for (const nestedPropKey in propVal) { + const nestedPrefix = propKey ? `${prefix || ''}${propKey}.` : undefined; + validateConfig(propVal[nestedPropKey], defaultVal[nestedPropKey], nestedPropKey, nestedPrefix); + } + } else { + if (propKey && propKey in propAssertions) { + // shortcoming in typescript 3.6.4 `in` keyword type guard requires manual cast to any below + (propAssertions as any)[propKey](propVal); + } + } +} class Config { public p2p: PoolConfig; @@ -176,6 +209,8 @@ class Config { const configProps = await Config.readConfigProps(configPath); if (configProps) { + validateConfig(configProps, this); + // set the network and xudir props up front because they influence default config values if (configProps.network && (!args || !args.network)) { this.network = configProps.network; @@ -208,6 +243,8 @@ class Config { } if (args) { + validateConfig(args, this); + // override our config file with command line arguments deepMerge(this, args); } diff --git a/lib/Xud.ts b/lib/Xud.ts index 1145e1368..578429336 100644 --- a/lib/Xud.ts +++ b/lib/Xud.ts @@ -16,6 +16,7 @@ import Service from './service/Service'; import SwapClientManager from './swaps/SwapClientManager'; import Swaps from './swaps/Swaps'; import { UnitConverter } from './utils/UnitConverter'; +import { AssertionError } from 'assert'; const version: string = require('../package.json').version; @@ -64,7 +65,18 @@ class Xud extends EventEmitter { * @param args optional arguments to override configuration parameters. */ public start = async (args?: { [argName: string]: any }) => { - const configFileLoaded = await this.config.load(args); + let configFileLoaded: boolean; + try { + configFileLoaded = await this.config.load(args); + } catch (err) { + if (err instanceof AssertionError) { + console.error(err.message); + process.exit(1); + } else { + throw err; + } + } + const loggers = Logger.createLoggers(this.config.loglevel, this.config.logpath, this.config.instanceid, this.config.logdateformat); this.logger = loggers.global; if (configFileLoaded) { diff --git a/test/jest/Config.spec.ts b/test/jest/Config.spec.ts index 5ee16a00b..326255a4a 100644 --- a/test/jest/Config.spec.ts +++ b/test/jest/Config.spec.ts @@ -64,4 +64,26 @@ describe('Config', () => { expect(config.p2p.port).toEqual(MAINNET_P2P_PORT); expect(config.network).toEqual(XuNetwork.TestNet); }); + + test('it throws an error when a property is assigned the wrong type', async () => { + await expect(config.load({ initdb: 23 })).rejects.toThrow('initdb is type number but should be boolean'); + }); + + test('it throws an error when a nested property is assigned the wrong type', async () => { + await expect(config.load({ p2p: { listen: 'no' } })).rejects.toThrow('p2p.listen is type string but should be boolean'); + }); + + test('it throws an error when a port property is assigned an invalid value', async () => { + await expect(config.load({ p2p: { port: 999999 } })).rejects.toThrow('port must be between 0 and 65535'); + }); + + test('it throws an error when a cltvdelta property is assigned a negative value', async () => { + await expect(config.load({ lnd: { BTC: { cltvdelta: -1 } } })).rejects.toThrow('cltvdelta must be a positive number'); + }); + + test('it uses the default value when a prperty is assigned an undefined value', async () => { + const defaultInitDb = config.initdb; + await config.load({ initdb: undefined }); + expect(config.initdb).toEqual(defaultInitDb); + }); });