feat: validate that id is number or we throw our our source code (#6860)

Previously, we were not validating that the ID was a number, which
sometimes resulted in returning our database queries (source code) to
the frontend. Now, we have validation middleware.

src/lib/features/segment/admin-segment.e2e.test.ts

@@ -194,6 +194,16 @@ test('should create segments', async () => {
     expect(segments.map((s) => s.name)).toEqual(['a', 'b', 'c']);
 });
 
+test('should return 400 for non numeeric segment ID', async () => {
+    const { body } = await app.request
+        .get(`${SEGMENTS_BASE_PATH}/stringName`)
+        .expect(400);
+    expect(body).toMatchObject({
+        message:
+            'Request validation failed: your request body or params contain invalid data: ID should be an integer',
+    });
+});
+
 test('should update segments', async () => {
     await app.createSegment({
         name: 'a',

src/lib/features/segment/segment-controller.ts

@@ -41,6 +41,7 @@ import {
 
 import { anonymiseKeys, extractUserIdFromUser } from '../../util';
 import { BadDataError } from '../../error';
+import idNumberMiddleware from '../../middleware/id-number-middleware';
 
 type IUpdateFeatureStrategySegmentsRequest = IAuthRequest<
     {},
@@ -159,6 +160,7 @@ export class SegmentsController extends Controller {
                         200: createResponseSchema('segmentStrategiesSchema'),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 
@@ -180,6 +182,7 @@ export class SegmentsController extends Controller {
                         ...getStandardResponses(401, 403, 409),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 
@@ -200,6 +203,7 @@ export class SegmentsController extends Controller {
                         ...getStandardResponses(400, 401, 403, 409, 415),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 
@@ -219,6 +223,7 @@ export class SegmentsController extends Controller {
                         ...getStandardResponses(404),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 

src/lib/middleware/id-number-middleware.test.ts

@@ -0,0 +1,32 @@
+import express from 'express';
+import supertest from 'supertest';
+import idNumberMiddleware from './id-number-middleware';
+
+describe('idNumberMiddleware', () => {
+    it('should pass when id is a valid integer', async () => {
+        const app = express();
+        app.use('/:id', idNumberMiddleware());
+        app.get('/:id', (req, res) => {
+            res.status(200).send('Valid ID');
+        });
+
+        await supertest(app)
+            .get('/123')
+            .expect(200)
+            .expect((res) => {
+                expect(res.text).toBe('Valid ID');
+            });
+    });
+    it('should throw BadDataError when id is not a valid integer', async () => {
+        const app = express();
+        app.use('/:id', idNumberMiddleware());
+        app.get('/:id', (req, res) => {
+            res.status(200).send('This should not be executed');
+        });
+
+        const { body } = await supertest(app).get('/abc').expect(400);
+        expect(body).toMatchObject({
+            details: [{ message: 'ID should be an integer' }],
+        });
+    });
+});

src/lib/middleware/id-number-middleware.ts

@@ -0,0 +1,16 @@
+import { BadDataError } from '../error';
+
+const idNumberMiddleware = (): any => {
+    return async (req, res, next) => {
+        const { id } = req.params;
+        if (!Number.isInteger(Number(id))) {
+            res.status(400).send(
+                new BadDataError('ID should be an integer').toJSON(),
+            );
+            return;
+        }
+        next();
+    };
+};
+
+export default idNumberMiddleware;

src/lib/routes/admin-api/user-admin.ts

@@ -51,12 +51,13 @@ import {
     type AdminCountSchema,
     adminCountSchema,
 } from '../../openapi/spec/admin-count-schema';
-import { BadDataError, ForbiddenError } from '../../error';
+import { ForbiddenError } from '../../error';
 import {
     createUserResponseSchema,
     type CreateUserResponseSchema,
 } from '../../openapi/spec/create-user-response-schema';
 import type { IRoleWithPermissions } from '../../types/stores/access-store';
+import idNumberMiddleware from '../../middleware/id-number-middleware';
 
 export default class UserAdminController extends Controller {
     private flagResolver: IFlagResolver;
@@ -354,6 +355,7 @@ export default class UserAdminController extends Controller {
                         ...getStandardResponses(400, 401, 404),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 
@@ -375,6 +377,7 @@ export default class UserAdminController extends Controller {
                         ...getStandardResponses(400, 401, 403, 404),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
 
@@ -395,6 +398,7 @@ export default class UserAdminController extends Controller {
                         ...getStandardResponses(401, 403, 404),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
     }
@@ -505,9 +509,6 @@ export default class UserAdminController extends Controller {
 
     async getUser(req: Request, res: Response<UserSchema>): Promise<void> {
         const { id } = req.params;
-        if (!Number.isInteger(Number(id))) {
-            throw new BadDataError('User id should be an integer');
-        }
         const user = await this.userService.getUser(Number(id));
 
         this.openApiService.respondWithValidation(
@@ -607,9 +608,7 @@ export default class UserAdminController extends Controller {
         const { user, params, body } = req;
         const { id } = params;
         const { name, email, rootRole } = body;
-        if (!Number.isInteger(Number(id))) {
-            throw new BadDataError('User id should be an integer');
-        }
+
         await this.throwIfScimUser({ id: Number(id) });
         const normalizedRootRole = Number.isInteger(Number(rootRole))
             ? Number(rootRole)
@@ -639,9 +638,7 @@ export default class UserAdminController extends Controller {
     async deleteUser(req: IAuthRequest, res: Response): Promise<void> {
         const { user, params } = req;
         const { id } = params;
-        if (!Number.isInteger(Number(id))) {
-            throw new BadDataError('User id should be an integer');
-        }
+
         await this.throwIfScimUser({ id: Number(id) });
 
         await this.userService.deleteUser(+id, user);

src/lib/routes/admin-api/user/pat.ts

@@ -26,6 +26,7 @@ import {
     createPatSchema,
 } from '../../../openapi/spec/create-pat-schema';
 import { ForbiddenError, NotFoundError } from '../../../error';
+import idNumberMiddleware from '../../../middleware/id-number-middleware';
 
 export default class PatController extends Controller {
     private patService: PatService;
@@ -109,6 +110,7 @@ export default class PatController extends Controller {
                         ...getStandardResponses(401, 403, 404),
                     },
                 }),
+                idNumberMiddleware(),
             ],
         });
     }