From 1a5b6647cfe27b3667e069d6382f5837660725ff Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Sat, 6 Aug 2022 12:30:03 -0700 Subject: [PATCH 1/2] Fixed improper input validation on metrics and client-api --- src/middleware/validators.js | 4 +- src/routers/client-api.js | 2 +- src/routers/metric.js | 77 +++++--------------------------- src/services/metric.js | 86 +++++++++++++++++++++++++++++++----- tests/client-api.test.js | 16 +++++++ tests/metric.test.js | 1 + 6 files changed, 107 insertions(+), 79 deletions(-) diff --git a/src/middleware/validators.js b/src/middleware/validators.js index 7da7775..eb56e3b 100644 --- a/src/middleware/validators.js +++ b/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; diff --git a/src/routers/client-api.js b/src/routers/client-api.js index bc1be93..f432786 100644 --- a/src/routers/client-api.js +++ b/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 }); diff --git a/src/routers/metric.js b/src/routers/metric.js index 2f072c5..3a7f6c2 100644 --- a/src/routers/metric.js +++ b/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({ page, 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); diff --git a/src/services/metric.js b/src/services/metric.js index 7321df4..dea3a03 100644 --- a/src/services/metric.js +++ b/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,16 +175,16 @@ 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 }); @@ -128,25 +192,25 @@ export function buildMetricsFilter(req) { 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); } diff --git a/tests/client-api.test.js b/tests/client-api.test.js index 8959cb3..debb047 100644 --- a/tests/client-api.test.js +++ b/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] ', () => { diff --git a/tests/metric.test.js b/tests/metric.test.js index 5869c9a..8eaed37 100644 --- a/tests/metric.test.js +++ b/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); From a78745ea47a9a93662594ae7d4c946629853dbeb Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Sat, 6 Aug 2022 12:48:58 -0700 Subject: [PATCH 2/2] Fixed resource to not be vulnerable to reflected XSS --- src/api-docs/schemas/metric.js | 3 --- src/routers/metric.js | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/api-docs/schemas/metric.js b/src/api-docs/schemas/metric.js index ac8c541..0d74751 100644 --- a/src/api-docs/schemas/metric.js +++ b/src/api-docs/schemas/metric.js @@ -72,9 +72,6 @@ export default { MetricsData: { type: 'object', properties: { - page: { - type: 'integer' - }, data: { type: 'array', items: data diff --git a/src/routers/metric.js b/src/routers/metric.js index 3a7f6c2..5d74ff1 100644 --- a/src/routers/metric.js +++ b/src/routers/metric.js @@ -29,7 +29,7 @@ router.get('/metric/data/', auth, [ return res.send(); } - res.send({ page, data }); + res.send({ data }); } catch (e) { responseException(res, e, 500); }