Andreas/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-04-04 14:57:20 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

🛂 fix: Validate types Query Param in People Picker Access Middleware (#12276)

Browse source 
* 🛂 fix: Validate `types` query param in people picker access middleware

checkPeoplePickerAccess only inspected `req.query.type` (singular),
allowing callers to bypass type-specific permission checks by using
the `types` (plural) parameter accepted by the controller. Now both
`type` and `types` are collected and each requested principal type is
validated against the caller's role permissions.

* 🛂 refactor: Hoist valid types constant, improve logging, and add edge-case tests

- Hoist VALID_PRINCIPAL_TYPES to module-level Set to avoid per-request allocation
- Include both `type` and `types` in error log for debuggability
- Restore detailed JSDoc documenting per-type permission requirements
- Add missing .json() assertion on partial-denial test
- Add edge-case tests: all-invalid types, empty string types, PrincipalType.PUBLIC

* 🏷️ fix: Align TPrincipalSearchParams with actual controller API

The stale type used `type` (singular) but the controller and all callers
use `types` (plural array). Aligns with PrincipalSearchParams in
types/queries.ts.
This commit is contained in:
Danny Avila 2026-03-17 02:46:11 -04:00 • committed by GitHub
parent 68435cdcd0
commit 2f09d29c71
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 209 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

167
api/server/middleware/checkPeoplePickerAccess.spec.js
View file

@ -173,6 +173,171 @@ describe('checkPeoplePickerAccess', () => {
expect(next).not.toHaveBeenCalled();
});
it('should deny access when using types param to bypass type-specific check', async () => {
req.query.types = PrincipalType.GROUP;
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: 'Forbidden',
message: 'Insufficient permissions to search for groups',
});
expect(next).not.toHaveBeenCalled();
});
it('should deny access when types contains any unpermitted type', async () => {
req.query.types = `${PrincipalType.USER},${PrincipalType.ROLE}`;
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: 'Forbidden',
message: 'Insufficient permissions to search for roles',
});
expect(next).not.toHaveBeenCalled();
});
it('should allow access when all requested types are permitted', async () => {
req.query.types = `${PrincipalType.USER},${PrincipalType.GROUP}`;
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: true,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(next).toHaveBeenCalled();
expect(res.status).not.toHaveBeenCalled();
});
it('should validate types when provided as array (Express qs parsing)', async () => {
req.query.types = [PrincipalType.GROUP, PrincipalType.ROLE];
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: true,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: 'Forbidden',
message: 'Insufficient permissions to search for groups',
});
expect(next).not.toHaveBeenCalled();
});
it('should enforce permissions for combined type and types params', async () => {
req.query.type = PrincipalType.USER;
req.query.types = PrincipalType.GROUP;
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: 'Forbidden',
message: 'Insufficient permissions to search for groups',
});
expect(next).not.toHaveBeenCalled();
});
it('should treat all-invalid types values as mixed search', async () => {
req.query.types = 'foobar';
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(next).toHaveBeenCalled();
expect(res.status).not.toHaveBeenCalled();
});
it('should deny when types is empty string and user has no permissions', async () => {
req.query.types = '';
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: false,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith({
error: 'Forbidden',
message: 'Insufficient permissions to search for users, groups, or roles',
});
expect(next).not.toHaveBeenCalled();
});
it('should treat types=public as mixed search since PUBLIC is not a searchable principal type', async () => {
req.query.types = PrincipalType.PUBLIC;
getRoleByName.mockResolvedValue({
permissions: {
[PermissionTypes.PEOPLE_PICKER]: {
[Permissions.VIEW_USERS]: true,
[Permissions.VIEW_GROUPS]: false,
[Permissions.VIEW_ROLES]: false,
},
},
});
await checkPeoplePickerAccess(req, res, next);
expect(next).toHaveBeenCalled();
expect(res.status).not.toHaveBeenCalled();
});
it('should allow mixed search when user has at least one permission', async () => {
// No type specified = mixed search
req.query.type = undefined;
@ -222,7 +387,7 @@ describe('checkPeoplePickerAccess', () => {
await checkPeoplePickerAccess(req, res, next);
expect(logger.error).toHaveBeenCalledWith(
'[checkPeoplePickerAccess][user123] checkPeoplePickerAccess error for req.query.type = undefined',
'[checkPeoplePickerAccess][user123] error for type=undefined, types=undefined',
error,
);
expect(res.status).toHaveBeenCalledWith(500);