From 974b9a1a67955168f37182979b012d501f982a61 Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Sat, 30 Jul 2022 14:37:17 -0700 Subject: [PATCH 1/2] Added Pino for logging - added input sanitization --- package.json | 4 +++- sonar-project.properties | 2 +- src/api-docs/schemas/environment.js | 14 +++++++++++++- src/api-docs/swagger-info.js | 2 +- src/client/resolvers.js | 3 ++- src/exceptions/index.js | 12 ++++-------- src/helpers/logger.js | 22 ++++++++++++++++++++++ src/routers/config.js | 6 ++++-- src/routers/environment.js | 6 ++++-- src/routers/permission.js | 12 +++++++----- src/routers/team.js | 5 +++-- src/services/slack.js | 7 ++++--- tests/config.test.js | 2 +- tests/permission.test.js | 4 ++-- tests/relay.test.js | 2 -- tests/team.test.js | 2 +- 16 files changed, 72 insertions(+), 33 deletions(-) create mode 100644 src/helpers/logger.js diff --git a/package.json b/package.json index 9d2745e..0b20bf4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "switcher-api", - "version": "1.2.5", + "version": "1.2.6", "description": "Feature Flag/Toggle API", "main": "start.js", "author": { @@ -41,6 +41,8 @@ "moment": "^2.29.4", "mongodb": "^4.7.0", "mongoose": "^6.4.0", + "pino": "^8.3.1", + "pino-pretty": "^8.1.0", "swagger-ui-express": "^4.4.0", "switcher-client": "^3.1.1", "validator": "^13.7.0" diff --git a/sonar-project.properties b/sonar-project.properties index 5e183ef..5213e20 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -1,7 +1,7 @@ sonar.projectKey=switcherapi_switcher-api sonar.projectName=switcher-api sonar.organization=switcherapi -sonar.projectVersion=1.2.5 +sonar.projectVersion=1.2.6 sonar.links.homepage=https://github.com/switcherapi/switcher-api sonar.testExecutionReportPaths=test-report.xml diff --git a/src/api-docs/schemas/environment.js b/src/api-docs/schemas/environment.js index ead91f9..1b4d227 100644 --- a/src/api-docs/schemas/environment.js +++ b/src/api-docs/schemas/environment.js @@ -30,5 +30,17 @@ const environment = { export default { Environment: environment, - EnvironmentCreateRequest: environment + EnvironmentCreateRequest: { + type: 'object', + properties: { + name: { + type: 'string' + }, + domain: { + type: 'string', + description: 'The domain ID parent of the environment', + format: 'uuid' + } + } + } }; \ No newline at end of file diff --git a/src/api-docs/swagger-info.js b/src/api-docs/swagger-info.js index e302ce0..9604389 100644 --- a/src/api-docs/swagger-info.js +++ b/src/api-docs/swagger-info.js @@ -1,6 +1,6 @@ export default { title: 'Switcher API', - version: 'v1.2.5', + version: 'v1.2.6', description: 'Switcher API is a Feature Flag API focused on toggling features over different environments and applications.', contact: { name: 'Roger Floriano (petruki)', diff --git a/src/client/resolvers.js b/src/client/resolvers.js index 9a8da8b..002f241 100644 --- a/src/client/resolvers.js +++ b/src/client/resolvers.js @@ -8,6 +8,7 @@ import { ActionTypes, RouterTypes } from '../models/permission'; import { verifyOwnership } from '../helpers'; import { resolveNotification, resolveValidation } from './relay/index'; import Component from '../models/component'; +import Logger from '../helpers/logger'; export const resolveConfigByKey = async (domain, key) => Config.findOne({ domain, key }, null, { lean: true }); @@ -187,7 +188,7 @@ async function resolveRelay(config, environment, entry, response) { if (config.relay.type === RelayTypes.VALIDATION) { response.result = false; response.reason = 'Relay service could not be reached'; - response.message = e.message; + Logger.error(response.reason, e); } } } diff --git a/src/exceptions/index.js b/src/exceptions/index.js index 2ea6f7f..b7b0601 100644 --- a/src/exceptions/index.js +++ b/src/exceptions/index.js @@ -1,4 +1,5 @@ import { Switcher } from 'switcher-client'; +import Logger from '../helpers/logger'; export class NotFoundError extends Error { constructor(message) { @@ -37,19 +38,14 @@ export class FeatureUnavailableError extends Error { export function responseException(res, err, code, feature = undefined) { if (process.env.SWITCHER_API_LOGGER == 'true') { - console.error(`Error (${err.constructor.name}): ${err.message} - Code: ${err.code}`); + Logger.httpError(err.constructor.name, err.code, err.message, err) if (feature) { - console.error('\n### Switcher API Logger ###\n' + - JSON.stringify(Switcher.getLogger(feature), undefined, 2)); + Logger.info(`Feature [${feature}]`, { log: Switcher.getLogger(feature) }); } } if (err.code) { - try { - return res.status(err.code).send({ error: err.message }); - } catch (e) { - console.error(err); - } + return res.status(err.code).send({ error: err.message }); } res.status(code).send({ error: err.message }); } \ No newline at end of file diff --git a/src/helpers/logger.js b/src/helpers/logger.js new file mode 100644 index 0000000..671d7b8 --- /dev/null +++ b/src/helpers/logger.js @@ -0,0 +1,22 @@ +import pino from 'pino'; + +const logger = pino({ + transport: { + target: 'pino-pretty' + } +}); + +export default class Logger { + + static info(message, obj) { + logger.info(obj, message); + } + + static error(message, err) { + logger.error(err, message); + } + + static httpError(name, code, message, err) { + logger.error(err, `${name} [${code}]: ${message}`); + } +} \ No newline at end of file diff --git a/src/routers/config.js b/src/routers/config.js index 714a254..39bd5d2 100644 --- a/src/routers/config.js +++ b/src/routers/config.js @@ -1,5 +1,5 @@ import express from 'express'; -import { check } from 'express-validator'; +import { check, query } from 'express-validator'; import { relayOptions } from '../models/config'; import History from '../models/history'; import { auth } from '../middleware/auth'; @@ -31,7 +31,9 @@ router.post('/config/create', // GET /config?group=ID&limit=10&skip=20 // GET /config?group=ID&sortBy=createdAt:desc // GET /config?group=ID -router.get('/config', auth, async (req, res) => { +router.get('/config', auth, [ + query('group').isMongoId() +], validate, async (req, res) => { try { const groupConfig = await getGroupConfigById(req.query.group); await groupConfig.populate({ diff --git a/src/routers/environment.js b/src/routers/environment.js index 8a59b84..b534271 100644 --- a/src/routers/environment.js +++ b/src/routers/environment.js @@ -2,13 +2,15 @@ import express from 'express'; import { check, query } from 'express-validator'; import { auth } from '../middleware/auth'; import { responseException } from '../exceptions'; -import { validate } from '../middleware/validators'; +import { validate, verifyInputUpdateParameters } from '../middleware/validators'; import * as Services from '../services/environment'; import { SwitcherKeys } from '../external/switcher-api-facade'; const router = new express.Router(); -router.post('/environment/create', auth, async (req, res) => { +router.post('/environment/create', auth, verifyInputUpdateParameters([ + 'name', 'domain' +]), async (req, res) => { try { const environment = await Services.createEnvironment(req.body, req.admin); res.status(201).send(environment); diff --git a/src/routers/permission.js b/src/routers/permission.js index 4fa8771..6309c5b 100644 --- a/src/routers/permission.js +++ b/src/routers/permission.js @@ -98,8 +98,9 @@ router.delete('/permission/:id', auth, [ } }); -router.patch('/permission/value/add/:id', auth, verifyInputUpdateParameters(['value']), [ - check('id').isMongoId() +router.patch('/permission/value/add/:id', auth, verifyInputUpdateParameters(['id', 'value']), [ + check('id').isMongoId(), + check('value').isString() ], validate, async (req, res) => { try { const permission = await Services.addValue(req.body, req.params.id, req.admin); @@ -109,9 +110,10 @@ router.patch('/permission/value/add/:id', auth, verifyInputUpdateParameters(['va } }); -router.patch('/permission/value/remove/:id', auth, [ - check('id').isMongoId() -], validate, verifyInputUpdateParameters(['value']), async (req, res) => { +router.patch('/permission/value/remove/:id', auth, verifyInputUpdateParameters(['id', 'value']), [ + check('id').isMongoId(), + check('value').isString() +], validate, async (req, res) => { try { const permission = await Services.removeValue(req.body, req.params.id, req.admin); res.send(permission); diff --git a/src/routers/team.js b/src/routers/team.js index 49b5b05..5ba4e79 100644 --- a/src/routers/team.js +++ b/src/routers/team.js @@ -11,8 +11,9 @@ import { SwitcherKeys } from '../external/switcher-api-facade'; const router = new express.Router(); -router.post('/team/create', auth, [ - check('name').isLength({ min: 2, max: 50 }) +router.post('/team/create', auth, verifyInputUpdateParameters(['name', 'domain']), [ + check('name').isLength({ min: 2, max: 50 }), + check('domain').isMongoId() ], validate, async (req, res) => { try { const team = await Services.createTeam(req.body, req.admin, req.query.defaultActions); diff --git a/src/services/slack.js b/src/services/slack.js index 4d63835..47ea109 100644 --- a/src/services/slack.js +++ b/src/services/slack.js @@ -8,6 +8,7 @@ import { getDomainById } from './domain'; import { getEnvironment } from './environment'; import { getGroupConfig } from './group-config'; import { containsValue } from '../helpers'; +import Logger from '../helpers/logger'; /** * Validates if ticket already exists, if so, return it. @@ -78,9 +79,9 @@ export async function checkAvailability(admin, feature) { SwitcherKeys.SLACK_UPDATE ]); - if (process.env.SWITCHER_API_LOGGER == 'true') - console.log('\n### Switcher API Logger ###\n' + - JSON.stringify(Switcher.getLogger(feature), undefined, 2)); + if (process.env.SWITCHER_API_LOGGER == 'true') { + Logger.info(`checkAvailability [${feature}]`, { log: Switcher.getLogger(feature) }); + } return result; } diff --git a/tests/config.test.js b/tests/config.test.js index f7c684f..8c1ea9c 100644 --- a/tests/config.test.js +++ b/tests/config.test.js @@ -135,7 +135,7 @@ describe('Testing fetch configuration info', () => { await request(app) .get('/config?group=INVALID_ID_VALUE') .set('Authorization', `Bearer ${adminMasterAccountToken}`) - .send().expect(500); + .send().expect(422); }); test('CONFIG_SUITE - Should get Config information by Id', async () => { diff --git a/tests/permission.test.js b/tests/permission.test.js index fff2a73..c7fa5e6 100644 --- a/tests/permission.test.js +++ b/tests/permission.test.js @@ -314,7 +314,7 @@ describe('Updating permission values tests', () => { await request(app) .patch('/permission/value/add/' + permission1Id) .set('Authorization', `Bearer ${adminMasterAccountToken}`) - .send().expect(400); + .send().expect(422); }); test('PERMISSION_SUITE - Should NOT add a value - Value already joined', async () => { @@ -375,7 +375,7 @@ describe('Updating permission values tests', () => { await request(app) .patch('/permission/value/remove/' + permission1Id) .set('Authorization', `Bearer ${adminMasterAccountToken}`) - .send().expect(400); + .send().expect(422); }); test('PERMISSION_SUITE - Should NOT remove a value - Invalid parameter', async () => { diff --git a/tests/relay.test.js b/tests/relay.test.js index 12bf778..ab3192e 100644 --- a/tests/relay.test.js +++ b/tests/relay.test.js @@ -296,7 +296,6 @@ describe('Testing Switcher Relay', () => { axiosStub.restore(); expect(req.statusCode).toBe(200); expect(req.body.reason).toEqual('Relay service could not be reached'); - expect(req.body.message).toEqual('Failed to reach http://localhost:3001 via GET'); expect(req.body.result).toBe(false); }); @@ -320,7 +319,6 @@ describe('Testing Switcher Relay', () => { axiosStub.restore(); expect(req.statusCode).toBe(200); expect(req.body.reason).toEqual('Relay service could not be reached'); - expect(req.body.message).toEqual('Failed to reach http://localhost:3001 via POST'); expect(req.body.result).toBe(false); }); diff --git a/tests/team.test.js b/tests/team.test.js index 25fca5e..bca800e 100644 --- a/tests/team.test.js +++ b/tests/team.test.js @@ -98,7 +98,7 @@ describe('Insertion tests', () => { .send({ name: 'My Team', domain: 'INVALID_ID' - }).expect(404); + }).expect(422); }); test('TEAM_SUITE - Should NOT create a new Team - Name is missing', async () => { From af0917a994bf715bb35ba565c10a2abe116dada7 Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Sat, 30 Jul 2022 14:41:25 -0700 Subject: [PATCH 2/2] Fixed code smell --- src/exceptions/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/exceptions/index.js b/src/exceptions/index.js index b7b0601..6c0d95e 100644 --- a/src/exceptions/index.js +++ b/src/exceptions/index.js @@ -38,7 +38,7 @@ export class FeatureUnavailableError extends Error { export function responseException(res, err, code, feature = undefined) { if (process.env.SWITCHER_API_LOGGER == 'true') { - Logger.httpError(err.constructor.name, err.code, err.message, err) + Logger.httpError(err.constructor.name, err.code, err.message, err); if (feature) { Logger.info(`Feature [${feature}]`, { log: Switcher.getLogger(feature) }); }