Add validation for /conversionRates endpoint

related to #587
Giveth · Aug 29, 2021 · 6db20ed · 6db20ed
1 parent f1fe886
commit 6db20ed
Show file tree

Hide file tree

Showing 7 changed files with 161 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);