Skip to content

Commit

Permalink
Patches ReDoS vulnerability found in formatInput and containsValue (#483
Browse files Browse the repository at this point in the history 
)

* Patches ReDoS vulnerability found in formatInput and containsValue

* chore: linting test suite

* chore: bump deps
  • Loading branch information
@petruki
petruki authored Mar 29, 2024
1 parent f5c60db commit 8bb7fc3
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 43 deletions.
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"axios": "^1.6.8",
"bcryptjs": "^2.4.3",
"cors": "^2.8.5",
"express": "^4.19.1",
"express": "^4.19.2",
"express-basic-auth": "^1.2.1",
"express-rate-limit": "^7.2.0",
"express-validator": "^7.0.1",
Expand All @@ -41,7 +41,7 @@
"jsonwebtoken": "^9.0.2",
"moment": "^2.30.1",
"mongodb": "^6.5.0",
"mongoose": "^8.2.2",
"mongoose": "^8.2.4",
"pino": "^8.19.0",
"pino-pretty": "^11.0.0",
"swagger-ui-express": "^5.0.0",
Expand Down
16 changes: 12 additions & 4 deletions src/helpers/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import { getDomainById } from '../services/domain';
import { getEnvironments } from '../services/environment';
import { getTeams } from '../services/team';
import { verifyPermissions, verifyPermissionsCascade } from './permission';
import { some } from 'lodash';

const PATTERN_ALPHANUMERIC_SPACE = /^[a-zA-Z0-9_\- ]*$/;
const PATTERN_ALPHANUMERIC = /^[a-zA-Z0-9_-]*$/;

export async function checkEnvironmentStatusRemoval(domainId, environmentName, strategy = false) {
const environment = await getEnvironments({ domain: domainId }, ['_id', 'name']);
Expand Down Expand Up @@ -41,7 +45,10 @@ export function parseJSON(str) {
}

export function containsValue(arr, value) {
return arr?.filter(item => item.match(value)).length > 0;
return some(arr, item => {
const regex = new RegExp(value);
return regex.test(item);
});
}

export function formatInput(input,
Expand All @@ -54,12 +61,13 @@ export function formatInput(input,

let regexStr;
if (options.autoUnderscore) {
regexStr = /^[a-zA-Z0-9_\- ]*$/;
regexStr = PATTERN_ALPHANUMERIC_SPACE;
} else {
regexStr = options.allowSpace ? /^[a-zA-Z0-9_\- ]*$/ : /^[a-zA-Z0-9_-]*$/;
regexStr = options.allowSpace ? PATTERN_ALPHANUMERIC_SPACE : PATTERN_ALPHANUMERIC;
}

if (!input.match(regexStr)) {
const regex = new RegExp(regexStr);
if (!regex.test(input)) {
throw new Error('Invalid input format. Use only alphanumeric digits.');
}

Expand Down
32 changes: 16 additions & 16 deletions tests/client-api.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -940,12 +940,12 @@ describe('Testing domain [Adm-GraphQL] ', () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminMasterAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"UPDATE","DELETE"`, RouterTypes.GROUP));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"UPDATE","DELETE"', RouterTypes.GROUP));

const exptected = '[{"action":"UPDATE","result":"ok"},{"action":"DELETE","result":"ok"}]';
expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("Group Test");
expect(JSON.parse(req.text).data.permission[0].name).toBe('Group Test');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
});

Expand All @@ -956,60 +956,60 @@ describe('Testing domain [Adm-GraphQL] ', () => {
await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminMasterAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"UPDATE","DELETE"`, RouterTypes.GROUP));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"UPDATE","DELETE"', RouterTypes.GROUP));

expect(cacheSpy.callCount).toBe(0);

const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminMasterAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"UPDATE","DELETE"`, RouterTypes.GROUP));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"UPDATE","DELETE"', RouterTypes.GROUP));

const exptected = '[{"action":"UPDATE","result":"ok"},{"action":"DELETE","result":"ok"}]';
expect(req.statusCode).toBe(200);
expect(cacheSpy.callCount).toBe(1);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("Group Test");
expect(JSON.parse(req.text).data.permission[0].name).toBe('Group Test');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
});

test('CLIENT_SUITE - Should return list of Groups permissions - by environment', async () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"CREATE"`, RouterTypes.GROUP, EnvType.DEFAULT));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"CREATE"', RouterTypes.GROUP, EnvType.DEFAULT));

const exptected = '[{"action":"CREATE","result":"ok"}]';
expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("Group Test");
expect(JSON.parse(req.text).data.permission[0].name).toBe('Group Test');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
});

test('CLIENT_SUITE - Should return list of Groups permissions - Unauthorized access', async () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"UPDATE","DELETE"`, RouterTypes.GROUP));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"UPDATE","DELETE"', RouterTypes.GROUP));

const exptected = '[{"action":"UPDATE","result":"nok"},{"action":"DELETE","result":"nok"}]';
expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("Group Test");
expect(JSON.parse(req.text).data.permission[0].name).toBe('Group Test');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
});

test('CLIENT_SUITE - Should return list of Configs permissions', async () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminMasterAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, groupConfigId, `"UPDATE","DELETE"`, RouterTypes.CONFIG));
.send(graphqlUtils.permissionsQuery(domainId, groupConfigId, '"UPDATE","DELETE"', RouterTypes.CONFIG));

const exptected = '[{"action":"UPDATE","result":"ok"},{"action":"DELETE","result":"ok"}]';
expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("TEST_CONFIG_KEY");
expect(JSON.parse(req.text).data.permission[1].name).toBe("TEST_CONFIG_KEY_PRD_QA");
expect(JSON.parse(req.text).data.permission[0].name).toBe('TEST_CONFIG_KEY');
expect(JSON.parse(req.text).data.permission[1].name).toBe('TEST_CONFIG_KEY_PRD_QA');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
expect(JSON.parse(req.text).data.permission[1].permissions).toMatchObject(JSON.parse(exptected));
});
Expand All @@ -1018,13 +1018,13 @@ describe('Testing domain [Adm-GraphQL] ', () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, groupConfigId, `"UPDATE","DELETE"`, RouterTypes.CONFIG));
.send(graphqlUtils.permissionsQuery(domainId, groupConfigId, '"UPDATE","DELETE"', RouterTypes.CONFIG));

const exptected = '[{"action":"UPDATE","result":"nok"},{"action":"DELETE","result":"nok"}]';
expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
expect(JSON.parse(req.text).data.permission[0].name).toBe("TEST_CONFIG_KEY");
expect(JSON.parse(req.text).data.permission[1].name).toBe("TEST_CONFIG_KEY_PRD_QA");
expect(JSON.parse(req.text).data.permission[0].name).toBe('TEST_CONFIG_KEY');
expect(JSON.parse(req.text).data.permission[1].name).toBe('TEST_CONFIG_KEY_PRD_QA');
expect(JSON.parse(req.text).data.permission[0].permissions).toMatchObject(JSON.parse(exptected));
expect(JSON.parse(req.text).data.permission[1].permissions).toMatchObject(JSON.parse(exptected));
});
Expand All @@ -1033,7 +1033,7 @@ describe('Testing domain [Adm-GraphQL] ', () => {
const req = await request(app)
.post('/adm-graphql')
.set('Authorization', `Bearer ${adminAccountToken}`)
.send(graphqlUtils.permissionsQuery(domainId, undefined, `"UPDATE","DELETE"`, RouterTypes.DOMAIN));
.send(graphqlUtils.permissionsQuery(domainId, undefined, '"UPDATE","DELETE"', RouterTypes.DOMAIN));

expect(req.statusCode).toBe(200);
expect(JSON.parse(req.text)).not.toBe(null);
Expand Down
18 changes: 9 additions & 9 deletions tests/unit-test/cache.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { permissionCache } from "../../src/helpers/cache";
import { EnvType } from "../../src/models/environment";
import { ActionTypes, RouterTypes } from "../../src/models/permission";
import { permissionCache } from '../../src/helpers/cache';
import { EnvType } from '../../src/models/environment';
import { ActionTypes, RouterTypes } from '../../src/models/permission';

describe("Test permissionCache", () => {
describe('Test permissionCache', () => {

beforeEach(() => {
permissionCache.cache.clear();
});

it("UNIT_CACHE - Should set and get cache", () => {
it('UNIT_CACHE - Should set and get cache', () => {
const cacheKey = permissionCache.permissionKey(
'adminId',
'domainId',
Expand All @@ -23,7 +23,7 @@ describe("Test permissionCache", () => {
expect(result).toEqual('value');
});

it("UNIT_CACHE - Should reload cache", () => {
it('UNIT_CACHE - Should reload cache', () => {
const cacheKey = permissionCache.permissionKey(
'adminId',
'domainId',
Expand All @@ -38,7 +38,7 @@ describe("Test permissionCache", () => {
expect(result).toBeUndefined();
});

it("UNIT_CACHE - Should NOT reload cache", () => {
it('UNIT_CACHE - Should NOT reload cache', () => {
const cacheKey = permissionCache.permissionKey(
'adminId',
'domainId',
Expand All @@ -53,7 +53,7 @@ describe("Test permissionCache", () => {
expect(result).toEqual('value');
});

it("UNIT_CACHE - Should NOT reload cache - empty router/action", () => {
it('UNIT_CACHE - Should NOT reload cache - empty router/action', () => {
const cacheKey = permissionCache.permissionKey(
'adminId',
'domainId',
Expand All @@ -68,7 +68,7 @@ describe("Test permissionCache", () => {
expect(result).toEqual('value');
});

it("UNIT_CACHE - Should NOT get from cache - different environment", () => {
it('UNIT_CACHE - Should NOT get from cache - different environment', () => {
const cacheKey = permissionCache.permissionKey(
'adminId',
'domainId',
Expand Down
3 changes: 1 addition & 2 deletions tests/unit-test/switcher-api-facade.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// eslint-disable-next-line no-unused-vars
import app from '../../src/app';
import '../../src/app';
import mongoose from 'mongoose';
import {
checkDomain,
Expand Down
2 changes: 1 addition & 1 deletion tests/unit-test/try-match.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ describe('Test tryMatch', () => {

const evilRE = '^(([a-z])+.)+[A-Z]([a-z])+$';
const evilInput1 = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
const evilInput2 = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
const evilInput2 = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb';

beforeEach(() => {
TimedMatch.setMaxBlackListed(50);
Expand Down
17 changes: 8 additions & 9 deletions tests/unit-test/verify-ownership.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// eslint-disable-next-line no-unused-vars
import app from '../../src/app';
import '../../src/app';
import mongoose from 'mongoose';
import GroupConfig from '../../src/models/group-config';
import { Team } from '../../src/models/team';
Expand Down Expand Up @@ -313,7 +312,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.READ,
RouterTypes.GROUP);
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));
});

test('UNIT_TEAM_PERMISSION_SUITE - Should NOT allow access - Permission not found', async () => {
Expand All @@ -324,7 +323,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.CREATE,
RouterTypes.GROUP);
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));
});

test('UNIT_TEAM_PERMISSION_SUITE - Should NOT allow access - Permission does not match', async () => {
Expand All @@ -335,7 +334,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.READ,
RouterTypes.CONFIG);
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));
});

test('UNIT_TEAM_PERMISSION_SUITE - Should NOT allow access - Member does not belong to a team', async () => {
Expand All @@ -346,7 +345,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.READ,
RouterTypes.DOMAIN);
}).rejects.toThrow(new PermissionError(`It was not possible to find any team that allows you to proceed with this operation`));
}).rejects.toThrow(new PermissionError('It was not possible to find any team that allows you to proceed with this operation'));
});

test('UNIT_TEAM_PERMISSION_SUITE - Should NOT allow access - Team not active', async () => {
Expand All @@ -359,7 +358,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.READ,
RouterTypes.GROUP);
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));

// tearDown
await changeTeamStatus(team1Id, true);
Expand All @@ -380,7 +379,7 @@ describe('Error tests', () => {
domainDocument,
ActionTypes.READ,
RouterTypes.GROUP);
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));
});

test('UNIT_TEAM_PERMISSION_SUITE - Should NOT allow access - Member does not have permission to environment', async () => {
Expand All @@ -397,7 +396,7 @@ describe('Error tests', () => {
RouterTypes.CONFIG,
false,
'default');
}).rejects.toThrow(new PermissionError(`Action forbidden`));
}).rejects.toThrow(new PermissionError('Action forbidden'));
});

});

0 comments on commit 8bb7fc3

Please sign in to comment.