Fixed improper input validation on metrics and client-api

src/api-docs/schemas/metric.js

@@ -72,9 +72,6 @@ export default {
     MetricsData: {
         type: 'object',
         properties: {
-            page: {
-                type: 'integer'
-            },
             data: {
                 type: 'array',
                 items: data

src/middleware/validators.js

@@ -5,11 +5,11 @@ import { getComponents } from '../services/component';
 import { getEnvironments } from '../services/environment';
 
 export async function checkConfig(req, res, next) {
-    const config = await getConfig({ domain: req.domain, key: req.query.key.toString() }, true);
+    const config = await getConfig({ domain: req.domain, key: String(req.query.key) }, true);
 
     if (!config) {
         return res.status(404).send({ 
-            error: `Unable to load a key ${req.query.key.toString()}` });
+            error: `Unable to load a key ${String(req.query.key)}` });
     }
 
     req.config = config;

src/routers/client-api.js

@@ -62,7 +62,7 @@ router.get('/criteria/snapshot_check/:version', appAuth, async (req, res) => {
 });
 
 router.post('/criteria/switchers_check', appAuth, [
-    check('switchers', 'Switcher Key is required').isLength({ min: 1 })
+    check('switchers', 'Switcher Key is required').isArray().isLength({ min: 1 })
 ], validate, async (req, res) => {
     try {
         const configsFound = await getConfigs({ domain: req.domain, components: req.componentId });

src/routers/metric.js

@@ -1,16 +1,11 @@
 import express from 'express';
-import { Metric } from '../models/metric';
-import { getConfig } from '../services/config';
 import { check, query } from 'express-validator';
 import { auth } from '../middleware/auth';
-import { verifyOwnership } from '../helpers';
-import { ActionTypes, RouterTypes } from '../models/permission';
 import { responseException } from '../exceptions';
 import { 
-    aggreagateReasons, 
-    aggregateComponents, 
-    aggregateSwitchers, 
-    buildMetricsFilter 
+    deleteMetrics,
+    getData,
+    getStatistics
 } from '../services/metric';
 import { validate } from '../middleware/validators';
 
@@ -24,34 +19,17 @@ router.get('/metric/data/', auth, [
     check('page', 'page is required as query parameter').isLength({ min: 1 })
 ], validate, async (req, res) => {
     try {
-        let { args } = buildMetricsFilter(req);
-        const page = req.query.page.toString();
-
+        const page = String(req.query.page);
         if (isNaN(page)) {
             throw new Error('Page value should be a number');
         }
 
-        if (req.query.key) { 
-            const config = await getConfig({ domain: req.query.domainid, key: req.query.key });
-            if (config) {
-                args.config = config._id;
-            } else {
-                return res.send();
-            }
+        const data = await getData(req, page);
+        if (!data) {
+            return res.send();
         }
 
-        const skip = parseInt((process.env.METRICS_MAX_PAGE * parseInt(req.query.page)) - process.env.METRICS_MAX_PAGE);
-        const metrics = await Metric.find({ ...args }, 
-            'config component entry result reason message group environment date -_id', {
-                skip,
-                limit: parseInt(process.env.METRICS_MAX_PAGE)
-            }).sort(req.query.sortBy ? req.query.sortBy.replace(';', ' ') : 'date')
-            .populate({ path: 'config', select: 'key -_id' }).exec();
-
-        res.send({
-            page,
-            data: metrics
-        });
+        res.send({ data });
     } catch (e) {
         responseException(res, e, 500);
     }
@@ -70,35 +48,11 @@ router.get('/metric/statistics/', auth, [
     query('dateAfter').optional().isDate()
 ], validate, async (req, res) => {
     try {
-        const switcher = buildMetricsFilter(req);
-        const components = buildMetricsFilter(req);
-        const reasons = buildMetricsFilter(req);
-
-        const dateGroupPattern = req.query.dateGroupPattern ? 
-            req.query.dateGroupPattern : 'YYYY-MM';
-
-        if (req.query.key) { 
-            const config = await getConfig({ domain: req.query.domainid, key: req.query.key });
-            if (config) {
-                const switcherId = { config: config._id };
-                switcher.aggregate.match(switcherId);
-                components.aggregate.match(switcherId);
-                reasons.aggregate.match(switcherId);
-            } else {
-                return res.send();
-            }
+        const result = await getStatistics(req);
+        if (!result) {
+            return res.send();
         }
 
-        let result = {};
-        await Promise.all([
-            req.query.statistics.match(/(switchers)|(all)/) ?
-                aggregateSwitchers(switcher.aggregate, dateGroupPattern, result) : Promise.resolve(),
-            req.query.statistics.match(/(components)|(all)/) ?
-                aggregateComponents(components.aggregate, dateGroupPattern, result) : Promise.resolve(),
-            req.query.statistics.match(/(reasons)|(all)/) ?
-                aggreagateReasons(reasons.aggregate, result) : Promise.resolve()
-        ]);
-
         res.send(result);
     } catch (e) {
         responseException(res, e, 500);
@@ -111,14 +65,7 @@ router.delete('/metric', auth, [
     check('key', 'switcher key must be provided').isLength({ min: 1 })
 ], validate, async (req, res) => {
     try {
-        let config = await getConfig({ domain: req.query.domainid, key: req.query.key });
-        if (!config) {
-            return res.status(404).send();
-        }
-
-        config = await verifyOwnership(req.admin, config, config.domain, ActionTypes.DELETE, RouterTypes.ADMIN);
-
-        await Metric.deleteMany({ domain: config.domain, config: config._id });
+        await deleteMetrics(req);
         res.send({ message: 'Switcher metrics deleted' });
     } catch (e) {
         responseException(res, e, 500);

src/services/metric.js

@@ -1,9 +1,73 @@
 import moment from 'moment';
 import { ObjectId } from 'mongodb';
+import { NotFoundError } from '../exceptions';
+import { verifyOwnership } from '../helpers';
 import { EnvType } from '../models/environment';
 import { Metric } from '../models/metric';
+import { ActionTypes, RouterTypes } from '../models/permission';
+import { getConfig } from './config';
 
-export async function aggreagateReasons(aggregate, result) {
+export async function getData(req, page) {
+    const { args } = buildMetricsFilter(req);
+
+    if (req.query.key) { 
+        const config = await getConfig({ domain: req.query.domainid, key: req.query.key });
+        if (!config) return undefined;
+        args.config = config._id;
+    }
+
+    const skip = parseInt((process.env.METRICS_MAX_PAGE * parseInt(page)) - process.env.METRICS_MAX_PAGE);
+    return Metric.find({ ...args }, 
+        'config component entry result reason message group environment date -_id', {
+            skip,
+            limit: parseInt(process.env.METRICS_MAX_PAGE)
+        }).sort(req.query.sortBy ? String(req.query.sortBy).replace(';', ' ') : 'date')
+        .populate({ path: 'config', select: 'key -_id' }).exec();
+}
+
+export async function getStatistics(req) {
+    const switcher = buildMetricsFilter(req);
+    const components = buildMetricsFilter(req);
+    const reasons = buildMetricsFilter(req);
+
+    const dateGroupPattern = req.query.dateGroupPattern ? 
+        req.query.dateGroupPattern : 'YYYY-MM';
+
+    if (req.query.key) { 
+        const config = await getConfig({ domain: req.query.domainid, key: req.query.key });
+        if (!config) return undefined;
+
+        const switcherId = { config: config._id };
+        switcher.aggregate.match(switcherId);
+        components.aggregate.match(switcherId);
+        reasons.aggregate.match(switcherId);
+    }
+
+    let result = {};
+    const options = String(req.query.statistics);
+    await Promise.all([
+        options.match(/(switchers)|(all)/) ?
+            aggregateSwitchers(switcher.aggregate, dateGroupPattern, result) : Promise.resolve(),
+        options.match(/(components)|(all)/) ?
+            aggregateComponents(components.aggregate, dateGroupPattern, result) : Promise.resolve(),
+        options.match(/(reasons)|(all)/) ?
+            aggreagateReasons(reasons.aggregate, result) : Promise.resolve()
+    ]);
+
+    return result;
+}
+
+export async function deleteMetrics(req) {
+    let config = await getConfig({ domain: req.query.domainid, key: req.query.key });
+    if (!config) {
+        throw new NotFoundError('Switcher not found');
+    }
+
+    config = await verifyOwnership(req.admin, config, config.domain, ActionTypes.DELETE, RouterTypes.ADMIN);
+    await Metric.deleteMany({ domain: config.domain, config: config._id });
+}
+
+async function aggreagateReasons(aggregate, result) {
     aggregate.group({
         _id: { reason: '$reason' },
         count: { $sum: 1 }
@@ -21,7 +85,7 @@ export async function aggreagateReasons(aggregate, result) {
     result.reasons = reasonsAggregated;
 }
 
-export async function aggregateComponents(aggregate, dateGroupPattern, result) {
+async function aggregateComponents(aggregate, dateGroupPattern, result) {
     aggregate.group({
         _id: { component: '$component', result: '$result', date: '$date' },
         count: { $sum: 1 }
@@ -63,7 +127,7 @@ export async function aggregateComponents(aggregate, dateGroupPattern, result) {
     result.components = buildStatistics(componentsAggregated, 'component');
 }
 
-export async function aggregateSwitchers(aggregate, dateGroupPattern, result) {
+async function aggregateSwitchers(aggregate, dateGroupPattern, result) {
     aggregate.group({
         _id: { config: '$config', result: '$result', date: '$date' },
         count: { $sum: 1 }
@@ -111,42 +175,42 @@ export async function aggregateSwitchers(aggregate, dateGroupPattern, result) {
     result.switchers = buildStatistics(switchersAggregated, 'switcher');
 }
 
-export function buildMetricsFilter(req) {
+function buildMetricsFilter(req) {
     let aggregate = Metric.aggregate();
     let args = {};
 
     args.domain = req.query.domainid;
-    aggregate.match({ domain: new ObjectId(req.query.domainid.toString()) });
+    aggregate.match({ domain: new ObjectId(String(req.query.domainid)) });
 
     if (req.query.environment) {
         args.environment = req.query.environment;
-        aggregate.match({ environment: req.query.environment.toString() });
+        aggregate.match({ environment: String(req.query.environment) });
     } else { 
         args.environment = EnvType.DEFAULT;
         aggregate.match({ environment: EnvType.DEFAULT });
     }
 
     if (req.query.result) { 
         args.result = req.query.result;
-        aggregate.match({ result: req.query.result.toString() === 'true' });
+        aggregate.match({ result: String(req.query.result) === 'true' });
     }
 
     if (req.query.component) { 
         args.component = req.query.component;
-        aggregate.match({ component: req.query.component.toString() });
+        aggregate.match({ component: String(req.query.component) });
     }
 
     if (req.query.group) { 
         args.group = req.query.group;
-        aggregate.match({ group: req.query.group.toString() });
+        aggregate.match({ group: String(req.query.group) });
     }
 
     if (req.query.dateBefore && !req.query.dateAfter) { 
         args.date = { $lte: new Date(req.query.dateBefore) };
-        aggregate.match({ date: { $lte: new Date(req.query.dateBefore.toString()) } });
+        aggregate.match({ date: { $lte: new Date(String(req.query.dateBefore)) } });
     } else if (req.query.dateAfter && !req.query.dateBefore) { 
         args.date = { $gte: new Date(req.query.dateAfter) };
-        aggregate.match({ date: { $gte: new Date(req.query.dateAfter.toString()) } });
+        aggregate.match({ date: { $gte: new Date(String(req.query.dateAfter)) } });
     } else if (req.query.dateAfter && req.query.dateBefore) {
         buildRangeDateFilter(req, args, aggregate);
     }

tests/client-api.test.js

@@ -780,6 +780,22 @@ describe('Testing criteria [REST] ', () => {
         expect(req.statusCode).toBe(200);
         expect(req.body.not_found).toEqual(['I_DO_NOT_EXIST']);
     });
+
+    test('CLIENT_SUITE - Should NOT return list of switchers - Invalid body attribute', async () => {
+        await request(app)
+            .post('/criteria/switchers_check')
+            .set('Authorization', `Bearer ${token}`)
+            .send({
+                switchers: 'TEST_CONFIG_KEY'
+            })
+            .expect(422);
+
+        await request(app)
+            .post('/criteria/switchers_check')
+            .set('Authorization', `Bearer ${token}`)
+            .send()
+            .expect(422);
+    });
 });
 
 describe('Testing domain [Adm-GraphQL] ', () => {

tests/metric.test.js

@@ -14,6 +14,7 @@ afterAll(async () => {
     await new Promise(resolve => setTimeout(resolve, 1000));
     await mongoose.disconnect();
 });
+
 describe('Fetch overall statistics', () => {
     beforeAll(setupDatabase);