From 2a5ffb2ea860f73a4820e58318602fe0b08125be Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Fri, 5 May 2023 22:14:10 -0700 Subject: [PATCH 1/3] Closes #397 - Relay HTTPS settings --- .env-cmdrc-template | 1 + .github/workflows/master.yml | 1 + .github/workflows/release.yml | 1 + config/.env.dev | 1 + docker-compose.yml | 1 + src/app.js | 1 + src/client/resolvers.js | 3 + src/middleware/validators.js | 4 +- src/routers/config.js | 4 +- src/services/config.js | 17 ++++- tests/config.test.js | 84 ++++++++++++++++++++-- tests/relay.test.js | 130 ++++++++++++++++++++++++++-------- 12 files changed, 207 insertions(+), 41 deletions(-) diff --git a/.env-cmdrc-template b/.env-cmdrc-template index 63fdc53..3ca75bf 100644 --- a/.env-cmdrc-template +++ b/.env-cmdrc-template @@ -12,6 +12,7 @@ "JWT_CLIENT_TOKEN_EXP_TIME": "5m", "JWT_ADMIN_TOKEN_RENEW_INTERVAL": "5m", "MAX_STRATEGY_OPERATION": 100, + "RELAY_BYPASS_HTTPS": true, "HISTORY_ACTIVATED": true, "METRICS_ACTIVATED": true, "METRICS_MAX_PAGE": 50, diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index a3b7f12..831cb1e 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -47,6 +47,7 @@ jobs: GOOGLE_RECAPTCHA_SECRET: ${{ secrets.GOOGLE_RECAPTCHA_SECRET }} GOOGLE_SKIP_AUTH: false MAX_STRATEGY_OPERATION: 100 + RELAY_BYPASS_HTTPS: true METRICS_ACTIVATED: true METRICS_MAX_PAGE: 50 SWITCHER_API_ENABLE: false diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1459ddc..ae21458 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -41,6 +41,7 @@ jobs: GOOGLE_RECAPTCHA_SECRET: ${{ secrets.GOOGLE_RECAPTCHA_SECRET }} GOOGLE_SKIP_AUTH: false MAX_STRATEGY_OPERATION: 100 + RELAY_BYPASS_HTTPS: true METRICS_ACTIVATED: true METRICS_MAX_PAGE: 50 SWITCHER_API_ENABLE: false diff --git a/config/.env.dev b/config/.env.dev index ffd481e..c25bebf 100644 --- a/config/.env.dev +++ b/config/.env.dev @@ -5,6 +5,7 @@ RESOURCE_SECRET=[CHANGE_IT] JWT_CLIENT_TOKEN_EXP_TIME=5m JWT_ADMIN_TOKEN_RENEW_INTERVAL=10m MAX_STRATEGY_OPERATION=100 +RELAY_BYPASS_HTTPS=true REGEX_MAX_TIMEOUT=3000 REGEX_MAX_BLACLIST=50 HISTORY_ACTIVATED=true diff --git a/docker-compose.yml b/docker-compose.yml index 47ad5b4..01383bb 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -43,6 +43,7 @@ services: - JWT_CLIENT_TOKEN_EXP_TIME=${JWT_CLIENT_TOKEN_EXP_TIME} - JWT_ADMIN_TOKEN_RENEW_INTERVAL=${JWT_ADMIN_TOKEN_RENEW_INTERVAL} - MAX_STRATEGY_OPERATION=${MAX_STRATEGY_OPERATION} + - RELAY_BYPASS_HTTPS=${RELAY_BYPASS_HTTPS} - HISTORY_ACTIVATED=${HISTORY_ACTIVATED} - METRICS_ACTIVATED=${METRICS_ACTIVATED} - METRICS_MAX_PAGE=${METRICS_MAX_PAGE} diff --git a/src/app.js b/src/app.js index 3cc3688..65977e6 100644 --- a/src/app.js +++ b/src/app.js @@ -96,6 +96,7 @@ app.get('/check', (_req, res) => { db_state: mongoose.connection.readyState, switcherapi: process.env.SWITCHER_API_ENABLE, switcherapi_logger: process.env.SWITCHER_API_LOGGER, + relay_bypass_https: process.env.RELAY_BYPASS_HTTPS, history: process.env.HISTORY_ACTIVATED, metrics: process.env.METRICS_ACTIVATED, max_metrics_pages: process.env.METRICS_MAX_PAGE, diff --git a/src/client/resolvers.js b/src/client/resolvers.js index 600a4a0..d072ced 100644 --- a/src/client/resolvers.js +++ b/src/client/resolvers.js @@ -9,6 +9,7 @@ import { verifyOwnership } from '../helpers'; import { resolveNotification, resolveValidation } from './relay/index'; import Component from '../models/component'; import Logger from '../helpers/logger'; +import { validateRelay } from '../services/config'; export const resolveConfigByKey = async (domain, key) => Config.findOne({ domain, key }, null, { lean: true }); @@ -171,6 +172,8 @@ async function checkConfigStrategies(configId, strategyFilter) { async function resolveRelay(config, environment, entry, response) { try { + validateRelay(config.relay); + if (config.relay && config.relay.activated[environment]) { if (config.relay.type === RelayTypes.NOTIFICATION) { resolveNotification(config.relay.endpoint[environment], config.relay.method, entry, diff --git a/src/middleware/validators.js b/src/middleware/validators.js index eb56e3b..d1f27ea 100644 --- a/src/middleware/validators.js +++ b/src/middleware/validators.js @@ -1,5 +1,5 @@ import { validationResult } from 'express-validator'; -import { BadRequestError } from '../exceptions'; +import { BadRequestError, responseException } from '../exceptions'; import { getConfig } from '../services/config'; import { getComponents } from '../services/component'; import { getEnvironments } from '../services/environment'; @@ -48,7 +48,7 @@ export function verifyInputUpdateParameters(allowedUpdates) { const isValidOperation = updates.every((update) => allowedUpdates.includes(update)); if (!isValidOperation) { - return res.status(400).send({ error: 'Invalid update parameters' }); + return responseException(res, new Error('Invalid update parameters'), 400); } req.updates = updates; diff --git a/src/routers/config.js b/src/routers/config.js index dbbd897..c8704b8 100644 --- a/src/routers/config.js +++ b/src/routers/config.js @@ -132,7 +132,9 @@ router.patch('/config/:id', auth, [ router.patch('/config/updateRelay/:id', auth, [ check('id').isMongoId() -], validate, async (req, res) => { +], validate, verifyInputUpdateParameters([ + 'type', 'description', 'activated', 'endpoint', 'method', 'auth_prefix', 'auth_token' +]), async (req, res) => { try { let config = await Services.updateConfigRelay(req.params.id, req.body, req.admin); res.send(config); diff --git a/src/services/config.js b/src/services/config.js index 4261d99..cacadb5 100644 --- a/src/services/config.js +++ b/src/services/config.js @@ -130,6 +130,8 @@ export async function updateConfig(id, args, admin) { export async function updateConfigRelay(id, args, admin) { let config = await getConfigById(id); + validateRelay(args); + config = await verifyOwnership(admin, config, config.domain, ActionTypes.UPDATE, RouterTypes.CONFIG); config.updatedBy = admin.email; @@ -283,4 +285,17 @@ export async function verifyRelay(id, code, admin) { } return 'failed'; -} \ No newline at end of file +} + +export function validateRelay(relay) { + const bypass = process.env.RELAY_BYPASS_HTTPS === 'true' || false; + + if (bypass) + return; + + const foundNotHttps = Object.values(relay.endpoint) + .filter(endpoint => !endpoint.toLowerCase().startsWith('https')); + + if (foundNotHttps.length) + throw new BadRequestError('HTTPS required'); +}; \ No newline at end of file diff --git a/tests/config.test.js b/tests/config.test.js index 1f6998f..ad34be3 100644 --- a/tests/config.test.js +++ b/tests/config.test.js @@ -766,6 +766,64 @@ describe('Testing component association', () => { }); }); +describe('Testing relay verification', () => { + beforeAll(async () => { + await setupDatabase(); + process.env.RELAY_BYPASS_HTTPS = false; + }); + + afterAll(() => { + process.env.RELAY_BYPASS_HTTPS = true; + }); + + const bodyRelay = { + type: 'VALIDATION', + activated: { + default: true + }, + endpoint: { + default: {} + }, + method: 'POST' + }; + + test('CONFIG_SUITE - Should NOT configure new Relay - Not HTTPS', async () => { + // Given HTTP + bodyRelay.endpoint.default = 'http://localhost:3001'; + + // Test + const response = await request(app) + .patch(`/config/updateRelay/${configId1}`) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send(bodyRelay).expect(400); + + expect(response.body.error).toEqual('HTTPS required'); + }); + + test('CONFIG_SUITE - Should configure new Relay', async () => { + // Given HTTPS + bodyRelay.endpoint.default = 'https://localhost:3001'; + + // Test + await request(app) + .patch(`/config/updateRelay/${configId1}`) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send(bodyRelay).expect(200); + }); + + test('CONFIG_SUITE - Should configure new Relay - All capital HTTPS', async () => { + // Given HTTPS + bodyRelay.endpoint.default = 'HTTPS://localhost:3001'; + + // Test + await request(app) + .patch(`/config/updateRelay/${configId1}`) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send(bodyRelay).expect(200); + }); + +}); + describe('Testing relay association', () => { beforeAll(setupDatabase); @@ -794,6 +852,7 @@ describe('Testing relay association', () => { // DB validation - document updated const config = await Config.findById(configId1).lean().exec(); expect(config.relay.verified).toEqual(false); + expect(config.relay.verification_code).toEqual(undefined); expect(config.relay.activated['default']).toEqual(true); expect(config.relay.endpoint['default']).toBe('http://localhost:3001'); expect(config.relay.auth_token['default']).toEqual('123'); @@ -860,7 +919,7 @@ describe('Testing relay association', () => { }); test('CONFIG_SUITE - Should configure new Relay on new envrironment', async () => { - //given + // Given // Creating development Environment... await request(app) .post('/environment/create') @@ -870,7 +929,7 @@ describe('Testing relay association', () => { domain: domainId }).expect(201); - //test + // Test await request(app) .patch(`/config/updateRelay/${configId1}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -898,7 +957,7 @@ describe('Testing relay association', () => { }); test('CONFIG_SUITE - Should remove configured Relay when reseting environment', async () => { - //test + // Test await request(app) .patch('/config/removeStatus/' + configId1) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -918,7 +977,7 @@ describe('Testing relay association', () => { }); test('CONFIG_SUITE - Should remove Relay from an environment', async () => { - //given - adding development relay to be removed later on + // Given - adding development relay to be removed later on await request(app) .patch(`/config/updateRelay/${configId1}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -940,7 +999,7 @@ describe('Testing relay association', () => { expect(config.relay.endpoint['development']).toBe('http://localhost:7000'); expect(config.relay.auth_token['development']).toEqual('abcd'); - //test + // Test await request(app) .patch(`/config/removeRelay/${configId1}/development`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -1060,6 +1119,7 @@ describe('Testing relay association', () => { }); describe('Testing disable metrics', () => { + beforeAll(setupDatabase); test('CONFIG_SUITE - Should disable metrics for production environment', async () => { await request(app) @@ -1090,7 +1150,17 @@ describe('Testing disable metrics', () => { }); test('CONFIG_SUITE - Should reset disabled metric flag when reseting environment', async () => { - //given + // Given + // Creating development Environment... + await request(app) + .post('/environment/create') + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send({ + name: 'development', + domain: domainId + }).expect(201); + + // Update to true await request(app) .patch(`/config/${configId1}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -1104,7 +1174,7 @@ describe('Testing disable metrics', () => { let config = await Config.findById(configId1).lean().exec(); expect(config.disable_metrics['development']).toEqual(true); - //test + // Test await request(app) .patch('/config/removeStatus/' + configId1) .set('Authorization', `Bearer ${adminMasterAccountToken}`) diff --git a/tests/relay.test.js b/tests/relay.test.js index 3a71841..91c7f68 100644 --- a/tests/relay.test.js +++ b/tests/relay.test.js @@ -73,20 +73,20 @@ describe('Testing Switcher Relay', () => { afterAll(setupDatabase); test('RELAY_SUITE - Should return success when validating relay using GET method', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'get'); - //given + // Given const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.GET, RelayTypes.VALIDATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -109,20 +109,20 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should return success when validating relay using POST method', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'post'); - //given + // Given const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.POST, RelayTypes.VALIDATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -145,20 +145,20 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should return success when notifying relay using GET method', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'get'); - //given - altough it's not considered after invoking the relay + // Given - altough it's not considered after invoking the relay const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.GET, RelayTypes.NOTIFICATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -181,20 +181,20 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should return success when notifying relay using POST method', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'post'); - //given - altough it's not considered after invoking the relay + // Given - altough it's not considered after invoking the relay const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.POST, RelayTypes.NOTIFICATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -217,14 +217,14 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should return success when validating relay using GET method - no input', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'get'); - //given + // Given const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -234,7 +234,7 @@ describe('Testing Switcher Relay', () => { await changeStrategy(configStrategyUSERId, undefined, false, EnvType.DEFAULT); await changeStrategy(configStrategyCIDRId, undefined, false, EnvType.DEFAULT); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -247,14 +247,14 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should return success when validating relay using POST method - no input', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'post'); - //given + // Given const mockRelayService = { data: { result: true, reason: 'Success' } }; axiosStub.returns(Promise.resolve(mockRelayService)); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) @@ -264,7 +264,7 @@ describe('Testing Switcher Relay', () => { await changeStrategy(configStrategyUSERId, undefined, false, EnvType.DEFAULT); await changeStrategy(configStrategyCIDRId, undefined, false, EnvType.DEFAULT); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -277,17 +277,17 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should NOT return success when validating relay using GET method - Service exception', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'get'); axiosStub.throwsException(); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.GET, RelayTypes.VALIDATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -300,17 +300,17 @@ describe('Testing Switcher Relay', () => { }); test('RELAY_SUITE - Should NOT return success when validating relay using POST method - Service exception', async () => { - //mock + // Mock axiosStub = sinon.stub(axios, 'post'); axiosStub.throwsException(); - //setup Switcher + // Setup Switcher await request(app) .patch(`/config/updateRelay/${configId}`) .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send(bodyRelay(RelayMethods.POST, RelayTypes.VALIDATION)).expect(200); - //test + // Test const req = await request(app) .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) .set('Authorization', `Bearer ${token}`) @@ -322,4 +322,74 @@ describe('Testing Switcher Relay', () => { expect(req.body.result).toBe(false); }); +}); + +describe('Testing Switcher Relay Validation', () => { + + const bodyRelay = (endpoint) => { + return { + type: RelayTypes.VALIDATION, + activated: { + default: true + }, + endpoint: { + default: endpoint + }, + method: RelayMethods.GET + }; + }; + + let token; + + beforeAll(async () => { + const response = await request(app) + .post('/criteria/auth') + .set('switcher-api-key', `${apiKey}`) + .send({ + domain: domainDocument.name, + component: component1.name, + environment: EnvType.DEFAULT + }).expect(200); + + token = response.body.token; + }); + + afterAll(() => { + process.env.RELAY_BYPASS_HTTPS = true; + }); + + test('RELAY_SUITE - Should return Relay could not be reached - Relay HTTPS required', async () => { + // Given + // HTTPS not required + process.env.RELAY_BYPASS_HTTPS = true; + + // Setup Switcher + await request(app) + .patch(`/config/updateRelay/${configId}`) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send(bodyRelay('http://localhost:3001')).expect(200); + + // HTTPS required + process.env.RELAY_BYPASS_HTTPS = false; + + // Test + const req = await request(app) + .post(`/criteria?key=${keyConfig}&showReason=true&showStrategy=true`) + .set('Authorization', `Bearer ${token}`) + .send({ + entry: [ + { + strategy: StrategiesType.VALUE, + input: 'USER_1' + }, + { + strategy: StrategiesType.NETWORK, + input: '10.0.0.3' + } + ]}); + + expect(req.statusCode).toBe(200); + expect(req.body.reason).toEqual('Relay service could not be reached'); + }); + }); \ No newline at end of file From cddecaf0d77eebe55e86e4df75abcee43ee412ce Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Fri, 5 May 2023 22:18:31 -0700 Subject: [PATCH 2/3] Fixed code smell --- src/services/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/config.js b/src/services/config.js index cacadb5..a697ae0 100644 --- a/src/services/config.js +++ b/src/services/config.js @@ -298,4 +298,4 @@ export function validateRelay(relay) { if (foundNotHttps.length) throw new BadRequestError('HTTPS required'); -}; \ No newline at end of file +} \ No newline at end of file From ac5f085ea75e51df29c0b9c86d062daf8ebf939f Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Fri, 5 May 2023 22:58:31 -0700 Subject: [PATCH 3/3] Fixed code smell --- src/client/resolvers.js | 10 +++++----- src/services/config.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/client/resolvers.js b/src/client/resolvers.js index d072ced..6a45844 100644 --- a/src/client/resolvers.js +++ b/src/client/resolvers.js @@ -172,9 +172,9 @@ async function checkConfigStrategies(configId, strategyFilter) { async function resolveRelay(config, environment, entry, response) { try { - validateRelay(config.relay); - - if (config.relay && config.relay.activated[environment]) { + if (config.relay?.activated[environment]) { + validateRelay(config.relay); + if (config.relay.type === RelayTypes.NOTIFICATION) { resolveNotification(config.relay.endpoint[environment], config.relay.method, entry, config.relay.auth_prefix, config.relay.auth_token[environment]); @@ -197,7 +197,7 @@ async function resolveRelay(config, environment, entry, response) { } function isMetricDisabled(config, environment) { - if (config.disable_metrics && config.disable_metrics[environment]) { + if (config.disable_metrics) { return config.disable_metrics[environment]; } return false; @@ -228,7 +228,7 @@ async function checkStrategy(entry, strategies, environment) { } async function checkStrategyInput(entry, { strategy, operation, values }) { - if (entry && entry.length) { + if (entry?.length) { const strategyEntry = entry.filter(e => e.strategy === strategy); if (strategyEntry.length == 0 || !(await processOperation(strategy, operation, strategyEntry[0].input, values))) { throw new Error(`Strategy '${strategy}' does not agree`); diff --git a/src/services/config.js b/src/services/config.js index a697ae0..8a4f201 100644 --- a/src/services/config.js +++ b/src/services/config.js @@ -247,7 +247,7 @@ export async function removeRelay(id, env, admin) { config = await verifyOwnership(admin, config, config.domain, ActionTypes.DELETE, RouterTypes.CONFIG); config.updatedBy = admin.email; - if (config.relay.activated && config.relay.activated.get(env) != undefined) { + if (config.relay.activated?.get(env) != undefined) { if (config.relay.activated.size > 1) { config.relay.activated.delete(env); config.relay.endpoint.delete(env);