From 6db20edcac7992f95f07edaff41257edcf2cff56 Mon Sep 17 00:00:00 2001 From: Mohammad Ranjbar Z Date: Sun, 29 Aug 2021 17:18:00 +0430 Subject: [PATCH] Add validation for /conversionRates endpoint related to #587 --- src/services/conversionRates/coingecko.js | 19 ++++--- .../conversionRates/conversionRates.hooks.js | 23 ++++++++ .../conversionRates.service.js | 54 +++++++++++-------- .../conversionRates.service.test.js | 34 ++++++++++-- src/utils/errorMessages.js | 2 + src/utils/tokenHelper.js | 23 ++++++++ src/utils/tokenHelper.test.js | 43 ++++++++++++++- 7 files changed, 161 insertions(+), 37 deletions(-) diff --git a/src/services/conversionRates/coingecko.js b/src/services/conversionRates/coingecko.js index 127b2859..aeb31490 100644 --- a/src/services/conversionRates/coingecko.js +++ b/src/services/conversionRates/coingecko.js @@ -11,16 +11,17 @@ const fetchCoingecko = async (timestampMS, coingeckoId, toSymbol) => { // for values below 72 hours sometime coingecko return empty values so I had to increate the range const timestampFrom = timestampTo - 3600 * 72; - let bestPrice = 1; + let bestPrice = 0; let resp; + const url = `https://api.coingecko.com/api/v3/coins/${coingeckoId}/market_chart/range?vs_currency=${toSymbol}&from=${timestampFrom}&to=${timestampTo}`; try { - resp = JSON.parse( - await rp( - `https://api.coingecko.com/api/v3/coins/${coingeckoId}/market_chart/range?vs_currency=${toSymbol}&from=${timestampFrom}&to=${timestampTo}`, - ), - ); + resp = JSON.parse(await rp(url)); + logger.info('coingecko response', { + url, + resp, + }); } catch (e) { - logger.error(`coingecko fetch (id:${coingeckoId}, toSymbol:${toSymbol})`, e); + logger.error(`coingecko fetch error, url:${url}`, e); Sentry.captureException(new Error(`Error requesting to coingecko: ${e.message}`)); return undefined; } @@ -32,13 +33,11 @@ const fetchCoingecko = async (timestampMS, coingeckoId, toSymbol) => { resp.prices.forEach(cur => { const [time, price] = cur; difference = Math.abs(timestampMS - time); - if (difference < bestDifference) { + if (difference < bestDifference && price) { bestDifference = difference; bestPrice = price; } }); - } else { - bestPrice = 1; } return bestPrice; }; diff --git a/src/services/conversionRates/conversionRates.hooks.js b/src/services/conversionRates/conversionRates.hooks.js index 7fd1a77f..04f32c67 100644 --- a/src/services/conversionRates/conversionRates.hooks.js +++ b/src/services/conversionRates/conversionRates.hooks.js @@ -1,8 +1,11 @@ const { disallow } = require('feathers-hooks-common'); const config = require('config'); +const errors = require('@feathersjs/errors'); const { rateLimit } = require('../../utils/rateLimit'); const onlyInternal = require('../../hooks/onlyInternal'); +const { errorMessages } = require('../../utils/errorMessages'); +const { isSymbolInTokenWhitelist } = require('../../utils/tokenHelper'); const { getConversionRates, getHourlyCryptoConversion, @@ -37,11 +40,31 @@ const findConversionRates = () => async context => { return context; }); }; +const validateSymbols = () => async context => { + const { params } = context; + const { to, symbol } = params.query; + if (symbol && !isSymbolInTokenWhitelist(symbol)) { + throw new errors.BadRequest(errorMessages.SENT_SYMBOL_IS_NOT_IN_TOKEN_WITHE_LIST); + } + if (to && !Array.isArray(to) && !isSymbolInTokenWhitelist(to)) { + throw new errors.BadRequest(errorMessages.SENT_TO_IS_NOT_IN_TOKEN_WITHE_LIST); + } + if (to && Array.isArray(to)) { + // to can be string or array of strings + to.forEach(toSymbol => { + if (!isSymbolInTokenWhitelist(toSymbol)) { + throw new errors.BadRequest(errorMessages.SENT_TO_IS_NOT_IN_TOKEN_WITHE_LIST); + } + }); + } + return context; +}; module.exports = { before: { all: [], find: [ + validateSymbols(), rateLimit({ threshold: config.rateLimit.threshold, ttl: config.rateLimit.ttlSeconds, diff --git a/src/services/conversionRates/conversionRates.service.js b/src/services/conversionRates/conversionRates.service.js index 72869625..7c7f1328 100644 --- a/src/services/conversionRates/conversionRates.service.js +++ b/src/services/conversionRates/conversionRates.service.js @@ -2,6 +2,7 @@ const createService = require('feathers-mongoose'); const createModel = require('../../models/conversionRates.model'); const hooks = require('./conversionRates.hooks'); +const { getValidSymbols } = require('../../utils/tokenHelper'); const { defaultFeatherMongooseOptions } = require('../serviceCommons'); module.exports = function conversionRates() { @@ -19,29 +20,36 @@ module.exports = function conversionRates() { service.docs = { operations: { find: { - 'parameters[0]': { - name: 'date', - in: 'query', - description: 'timestamp for instance: 1624951936000', - }, - 'parameters[1]': { - name: 'symbol', - in: 'query', - - default: 'ETH', - }, - 'parameters[2]': { - name: 'to', - in: 'query', - - default: 'USD', - }, - 'parameters[3]': { - name: 'interval', - in: 'query', - - description: 'could be hourly', - }, + parameters: [ + { + name: 'date', + in: 'query', + description: 'timestamp for instance: 1624951936000', + }, + { + name: 'symbol', + in: 'query', + schema: { + type: 'string', + default: 'ETH', + enum: getValidSymbols(), + }, + }, + { + name: 'to', + in: 'query', + schema: { + type: 'string', + enum: getValidSymbols(), + }, + description: 'could be string or array of string', + }, + { + name: 'interval', + in: 'query', + description: 'could be hourly', + }, + ], }, update: false, patch: false, diff --git a/src/services/conversionRates/conversionRates.service.test.js b/src/services/conversionRates/conversionRates.service.test.js index 5bf23f98..04e6424c 100644 --- a/src/services/conversionRates/conversionRates.service.test.js +++ b/src/services/conversionRates/conversionRates.service.test.js @@ -1,6 +1,7 @@ const request = require('supertest'); const config = require('config'); const { assert } = require('chai'); +const { errorMessages } = require('../../utils/errorMessages'); const { getFeatherAppInstance } = require('../../app'); const app = getFeatherAppInstance(); @@ -18,6 +19,22 @@ function getConversionRatesTestCases() { assert.exists(response.body.rates); }); + it('should get 400 when sending invalid token symbols ', async () => { + const response = await request(baseUrl) + .get(relativeUrl) + .query({ symbol: 'FAKE_SYMBOL' }); + assert.equal(response.statusCode, 400); + assert.exists(response.body.message, errorMessages.SENT_SYMBOL_IS_NOT_IN_TOKEN_WITHE_LIST); + }); + + it('should get 400 when sending invalid to token symbols ', async () => { + const response = await request(baseUrl) + .get(relativeUrl) + .query({ symbol: 'ETH', to: 'FAKE_SYMBOL' }); + assert.equal(response.statusCode, 400); + assert.exists(response.body.message, errorMessages.SENT_TO_IS_NOT_IN_TOKEN_WITHE_LIST); + }); + it('should get equal values for BTC and WBTC', async () => { const wbtcSumbol = 'WBTC'; const response = await request(baseUrl) @@ -99,15 +116,26 @@ function getConversionRatesTestCases() { it('should multiple hourly get successful result', async () => { const usdSymbol = 'USD'; - const eurSymbol = 'EUR'; + const ethSymbol = 'ETH'; const hourlyInterval = 'hourly'; const response = await request(baseUrl) .get(relativeUrl) - .query({ interval: hourlyInterval, from: btcSymbol, to: [usdSymbol, eurSymbol] }); + .query({ interval: hourlyInterval, from: btcSymbol, to: [usdSymbol, ethSymbol] }); assert.equal(response.statusCode, 200); assert.exists(response.body.rates); assert.exists(response.body.rates[usdSymbol]); - assert.exists(response.body.rates[eurSymbol]); + assert.exists(response.body.rates[ethSymbol]); + }); + + it('should multiple hourly get error when one of toSymbols is not invalid', async () => { + const fakeSymbol = 'FAKE_SYMBOL'; + const ethSymbol = 'ETH'; + const hourlyInterval = 'hourly'; + const response = await request(baseUrl) + .get(relativeUrl) + .query({ interval: hourlyInterval, from: btcSymbol, to: [fakeSymbol, ethSymbol] }); + assert.equal(response.statusCode, 400); + assert.exists(response.body.message, errorMessages.SENT_TO_IS_NOT_IN_TOKEN_WITHE_LIST); }); } diff --git a/src/utils/errorMessages.js b/src/utils/errorMessages.js index bc6e7a7f..fc57b06d 100644 --- a/src/utils/errorMessages.js +++ b/src/utils/errorMessages.js @@ -7,6 +7,8 @@ const errorMessages = { 'Just campaignOwner and admin can unArchive campaign', ARCHIVED_CAMPAIGNS_STATUS_JUST_CAN_UPDATE_TO_ACTIVE: 'Archived campaigns status can change just to Active', + SENT_SYMBOL_IS_NOT_IN_TOKEN_WITHE_LIST: 'Sent symbol is not in token whitelist', + SENT_TO_IS_NOT_IN_TOKEN_WITHE_LIST: 'Sent toSymbol is not in token whitelist', }; module.exports = { errorMessages }; diff --git a/src/utils/tokenHelper.js b/src/utils/tokenHelper.js index 5b8426fc..f928974f 100644 --- a/src/utils/tokenHelper.js +++ b/src/utils/tokenHelper.js @@ -41,9 +41,32 @@ function getTokenBySymbol(symbol) { return tokensBySymbols[symbol] || { symbol }; } +const isSymbolInTokenWhitelist = symbol => { + return Boolean( + getWhiteListTokens().find(token => token.symbol === symbol) || + // for example we dont have BTC as symbol but it is rateEqSymbol for the WBTC token in our config + getWhiteListTokens().find(token => token.rateEqSymbol === symbol), + ); +}; + +const getValidSymbols = () => { + const symbols = []; + getWhiteListTokens().forEach(token => { + if (!symbols.includes(token.symbol)) { + symbols.push(token.symbol); + } + if (token.rateEqSymbol && !symbols.includes(token.rateEqSymbol)) { + symbols.push(token.rateEqSymbol); + } + }); + return symbols; +}; + module.exports = { getTokenBySymbol, getWhiteListTokens, getTokenByAddress, getTokenByForeignAddress, + isSymbolInTokenWhitelist, + getValidSymbols, }; diff --git a/src/utils/tokenHelper.test.js b/src/utils/tokenHelper.test.js index 037121dc..5638274a 100644 --- a/src/utils/tokenHelper.test.js +++ b/src/utils/tokenHelper.test.js @@ -1,6 +1,12 @@ const { expect, assert } = require('chai'); const config = require('config'); -const { getTokenBySymbol, getWhiteListTokens } = require('./tokenHelper'); + +const { + getTokenBySymbol, + getValidSymbols, + getWhiteListTokens, + isSymbolInTokenWhitelist, +} = require('./tokenHelper'); const tokens = config.get('tokenWhitelist'); @@ -23,6 +29,41 @@ function getWhiteListTokensTestCases() { expect(getWhiteListTokens()).to.be.deep.equal(tokens); }); } +function getValidSymbolsTestCases() { + it('should return correct whiteList tokens', () => { + assert.sameDeepMembers(getValidSymbols(), [ + 'ETH', + 'SAI', + 'DAI', + 'PAN', + 'WBTC', + 'BTC', + 'USDC', + 'ANT', + 'XDAI', + 'USD', + ]) + + // expect().to.be.deep.equal(); + }); +} + +function isSymbolInTokenWhitelistTestCases() { + it('should return true for DAI token', () => { + assert.isTrue(isSymbolInTokenWhitelist('DAI')); + }); + it('should return true for WBTC token', () => { + assert.isTrue(isSymbolInTokenWhitelist('WBTC')); + }); + it('should return true for PAN token', () => { + assert.isTrue(isSymbolInTokenWhitelist('PAN')); + }); + it('should return false for NODE token', () => { + assert.isFalse(isSymbolInTokenWhitelist('NODE')); + }); +} describe('test getTokenBySymbol() function', getTokenBySymbolTestCases); describe('test getWhiteListTokens() function', getWhiteListTokensTestCases); +describe('test isSymbolInTokenWhitelist() function', isSymbolInTokenWhitelistTestCases); +describe('test getValidSymbols() function', getValidSymbolsTestCases);