From b9e0fa48c6c2c53f71299bc6cd45b10f6abbc29e Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 26 Mar 2026 17:05:01 -0700 Subject: [PATCH] fix: validate permissions in create, RoleConflictError, rollback safety, cache consistency - Add permissions type/array validation in createRoleHandler - Introduce RoleConflictError class replacing fragile string-prefix matching - Wrap rollback in !role null path with try/catch for correct 404 response - Wrap deleteRoleByName cache.set in try/catch matching createRoleByName - Narrow updateRoleHandler body type to { name?, description? } - Add tests: non-string description in create, rollback failure logging, permissions array rejection, description max-length assertion fix --- packages/api/src/admin/roles.spec.ts | 63 ++++++++++++++++++++-- packages/api/src/admin/roles.ts | 26 +++++---- packages/data-schemas/src/index.ts | 1 + packages/data-schemas/src/methods/index.ts | 3 +- packages/data-schemas/src/methods/role.ts | 23 +++++--- 5 files changed, 95 insertions(+), 21 deletions(-) diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 519c1c528e..5fe7fda091 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -6,6 +6,8 @@ import type { ServerRequest } from '~/types/http'; import type { AdminRolesDeps } from './roles'; import { createAdminRolesHandlers } from './roles'; +const { RoleConflictError } = jest.requireActual('@librechat/data-schemas'); + jest.mock('@librechat/data-schemas', () => ({ ...jest.requireActual('@librechat/data-schemas'), logger: { error: jest.fn() }, @@ -209,7 +211,9 @@ describe('createAdminRolesHandlers', () => { it('returns 409 when role already exists', async () => { const deps = createDeps({ - createRoleByName: jest.fn().mockRejectedValue(new Error('Role "editor" already exists')), + createRoleByName: jest + .fn() + .mockRejectedValue(new RoleConflictError('Role "editor" already exists')), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ body: { name: 'editor' } }); @@ -224,7 +228,9 @@ describe('createAdminRolesHandlers', () => { const deps = createDeps({ createRoleByName: jest .fn() - .mockRejectedValue(new Error('Cannot create role with reserved system name: ADMIN')), + .mockRejectedValue( + new RoleConflictError('Cannot create role with reserved system name: ADMIN'), + ), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ body: { name: 'ADMIN' } }); @@ -263,6 +269,34 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(500); }); + + it('returns 400 when description is not a string', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + body: { name: 'editor', description: 123 }, + }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'description must be a string' }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 400 when permissions is an array', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + body: { name: 'editor', permissions: [1, 2, 3] }, + }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'permissions must be an object' }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); }); describe('updateRole', () => { @@ -522,11 +556,30 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor'); }); - it('returns 400 when description exceeds max length', async () => { + it('logs rollback failure and still returns 500', async () => { const deps = createDeps({ - getRoleByName: jest.fn().mockResolvedValue(mockRole()), + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateUsersByRole: jest + .fn() + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('rollback failed')), + updateRoleByName: jest.fn().mockRejectedValue(new Error('rename failed')), }); const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'new-name' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(deps.updateUsersByRole).toHaveBeenCalledTimes(2); + }); + + it('returns 400 when description exceeds max length', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor' }, body: { description: 'a'.repeat(2001) }, @@ -538,7 +591,7 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'description must not exceed 2000 characters', }); - expect(deps.updateRoleByName).not.toHaveBeenCalled(); + expect(deps.getRoleByName).not.toHaveBeenCalled(); }); it('returns 500 on unexpected error', async () => { diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 834b948fc8..0477d9a7d7 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -1,5 +1,5 @@ import { SystemRoles } from 'librechat-data-provider'; -import { logger, isValidObjectIdString } from '@librechat/data-schemas'; +import { logger, isValidObjectIdString, RoleConflictError } from '@librechat/data-schemas'; import type { IRole, IUser, AdminMember } from '@librechat/data-schemas'; import type { FilterQuery } from 'mongoose'; import type { Response } from 'express'; @@ -102,6 +102,12 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { .status(400) .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` }); } + if ( + permissions !== undefined && + (typeof permissions !== 'object' || Array.isArray(permissions)) + ) { + return res.status(400).json({ error: 'permissions must be an object' }); + } const role = await createRoleByName({ name: name.trim(), description: description ?? '', @@ -110,18 +116,16 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(201).json({ role }); } catch (error) { logger.error('[adminRoles] createRole error:', error); - const is409 = - error instanceof Error && - (error.message.startsWith('Role "') || - error.message.startsWith('Cannot create role with reserved')); - const message = is409 && error instanceof Error ? error.message : 'Failed to create role'; - return res.status(is409 ? 409 : 500).json({ error: message }); + if (error instanceof RoleConflictError) { + return res.status(409).json({ error: error.message }); + } + return res.status(500).json({ error: 'Failed to create role' }); } } async function updateRoleHandler(req: ServerRequest, res: Response) { const { name } = req.params as RoleNameParams; - const body = req.body as Partial; + const body = req.body as { name?: string; description?: string }; let isRename = false; let trimmedName = ''; let migrationRan = false; @@ -188,7 +192,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { const role = await updateRoleByName(name, updates); if (!role) { if (migrationRan) { - await updateUsersByRole(trimmedName, name); + try { + await updateUsersByRole(trimmedName, name); + } catch (rollbackError) { + logger.error('[adminRoles] rollback failed (role not found path):', rollbackError); + } } return res.status(404).json({ error: 'Role not found' }); } diff --git a/packages/data-schemas/src/index.ts b/packages/data-schemas/src/index.ts index cd683c937c..d673db1f5c 100644 --- a/packages/data-schemas/src/index.ts +++ b/packages/data-schemas/src/index.ts @@ -7,6 +7,7 @@ export * from './utils'; export { createModels } from './models'; export { createMethods, + RoleConflictError, DEFAULT_REFRESH_TOKEN_EXPIRY, DEFAULT_SESSION_EXPIRY, tokenValues, diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index 4202cac0eb..11025fd47d 100644 --- a/packages/data-schemas/src/methods/index.ts +++ b/packages/data-schemas/src/methods/index.ts @@ -1,6 +1,7 @@ import { createSessionMethods, DEFAULT_REFRESH_TOKEN_EXPIRY, type SessionMethods } from './session'; import { createTokenMethods, type TokenMethods } from './token'; -import { createRoleMethods, type RoleMethods, type RoleDeps } from './role'; +import { createRoleMethods, RoleConflictError, type RoleMethods, type RoleDeps } from './role'; +export { RoleConflictError }; import { createUserMethods, DEFAULT_SESSION_EXPIRY, type UserMethods } from './user'; export { DEFAULT_REFRESH_TOKEN_EXPIRY, DEFAULT_SESSION_EXPIRY }; diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 2ca7e745ae..fcaef95ebe 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -9,6 +9,13 @@ import type { Model } from 'mongoose'; import type { IRole, IUser } from '~/types'; import logger from '~/config/winston'; +export class RoleConflictError extends Error { + constructor(message: string) { + super(message); + this.name = 'RoleConflictError'; + } +} + export interface RoleDeps { /** Returns a cache store for the given key. Injected from getLogStores. */ getCache?: (key: string) => { @@ -350,12 +357,12 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol throw new Error('Role name is required'); } if (SystemRoles[name.trim() as keyof typeof SystemRoles]) { - throw new Error(`Cannot create role with reserved system name: ${name}`); + throw new RoleConflictError(`Cannot create role with reserved system name: ${name}`); } const Role = mongoose.models.Role; const existing = await Role.findOne({ name: name.trim() }).lean(); if (existing) { - throw new Error(`Role "${name.trim()}" already exists`); + throw new RoleConflictError(`Role "${name.trim()}" already exists`); } let role; try { @@ -371,7 +378,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol * the same user-facing message as the application-level duplicate check. */ if (err && typeof err === 'object' && 'code' in err && err.code === 11000) { - throw new Error(`Role "${name.trim()}" already exists`); + throw new RoleConflictError(`Role "${name.trim()}" already exists`); } throw err; } @@ -400,9 +407,13 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } await getUserModel().updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); const deleted = await Role.findOneAndDelete({ name: roleName }).lean(); - const cache = deps.getCache?.(CacheKeys.ROLES); - if (cache) { - await cache.set(roleName, null); + try { + const cache = deps.getCache?.(CacheKeys.ROLES); + if (cache) { + await cache.set(roleName, null); + } + } catch (cacheError) { + logger.error(`[deleteRoleByName] cache invalidation failed for "${roleName}":`, cacheError); } return deleted as IRole | null; }