Skip to content

Patches ReDoS vulnerability found in formatInput and containsValue #483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Patches ReDoS vulnerability found in formatInput and containsValue #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
681c760
Patches ReDoS vulnerability found in formatInput and containsValue
petruki Mar 29, 2024
03dbcb6
chore: linting test suite
petruki Mar 29, 2024
0e66857
chore: bump deps
petruki Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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'));
});

});