From b5909511907ff6a6d9097fd633125c47eee977a6 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 27 Mar 2026 08:24:02 -0700 Subject: [PATCH] fix: harden role guards, add User.role index, validate names, improve tests - Add index on User.role field for efficient member queries at scale - Replace fragile SystemRoles key lookup with value-based Set check (6 sites) - Elevate rename rollback failure logging to CRITICAL (matches removeRoleMember) - Guard removeRoleMember against non-ADMIN system roles (403 for USER) - Fix parsePagination limit=0 gotcha: use parseInt + NaN check instead of || - Add control character and reserved path segment validation to role names - Simplify validateRoleName: remove redundant casts and dead conditions - Add JSDoc to deleteRoleByName documenting non-atomic window - Split mixed value+type import in methods/index.ts per AGENTS.md - Add 9 new tests: permissions assertion, combined rename+desc, createRole with permissions, pagination edge cases, control char/reserved name rejection, system role removeRoleMember guard --- packages/api/src/admin/pagination.ts | 9 +- packages/api/src/admin/roles.spec.ts | 129 ++++++++++++++++++++- packages/api/src/admin/roles.ts | 44 ++++--- packages/data-schemas/src/methods/index.ts | 3 +- packages/data-schemas/src/methods/role.ts | 17 ++- packages/data-schemas/src/schema/user.ts | 1 + 6 files changed, 179 insertions(+), 24 deletions(-) diff --git a/packages/api/src/admin/pagination.ts b/packages/api/src/admin/pagination.ts index 836ee9e700..69003f0418 100644 --- a/packages/api/src/admin/pagination.ts +++ b/packages/api/src/admin/pagination.ts @@ -5,8 +5,13 @@ export function parsePagination(query: { limit?: string; offset?: string }): { limit: number; offset: number; } { + const rawLimit = parseInt(query.limit ?? '', 10); + const rawOffset = parseInt(query.offset ?? '', 10); return { - limit: Math.min(Math.max(Number(query.limit) || DEFAULT_PAGE_LIMIT, 1), MAX_PAGE_LIMIT), - offset: Math.max(Number(query.offset) || 0, 0), + limit: Math.min( + Math.max(Number.isNaN(rawLimit) ? DEFAULT_PAGE_LIMIT : rawLimit, 1), + MAX_PAGE_LIMIT, + ), + offset: Math.max(Number.isNaN(rawOffset) ? 0 : rawOffset, 0), }; } diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index da0ff18a63..5b5f6869d2 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -117,6 +117,46 @@ describe('createAdminRolesHandlers', () => { expect(deps.listRoles).toHaveBeenCalledWith({ limit: 200, offset: 0 }); }); + it('clamps negative offset to 0', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ query: { offset: '-5' } }); + + await handlers.listRoles(req, res); + + expect(deps.listRoles).toHaveBeenCalledWith({ limit: 50, offset: 0 }); + }); + + it('treats non-numeric limit as default', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ query: { limit: 'abc' } }); + + await handlers.listRoles(req, res); + + expect(deps.listRoles).toHaveBeenCalledWith({ limit: 50, offset: 0 }); + }); + + it('clamps limit=0 to 1', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ query: { limit: '0' } }); + + await handlers.listRoles(req, res); + + expect(deps.listRoles).toHaveBeenCalledWith({ limit: 1, offset: 0 }); + }); + + it('truncates float offset to integer', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ query: { offset: '1.7' } }); + + await handlers.listRoles(req, res); + + expect(deps.listRoles).toHaveBeenCalledWith({ limit: 50, offset: 1 }); + }); + it('returns 500 on error', async () => { const deps = createDeps({ listRoles: jest.fn().mockRejectedValue(new Error('db down')) }); const handlers = createAdminRolesHandlers(deps); @@ -187,6 +227,25 @@ describe('createAdminRolesHandlers', () => { }); }); + it('passes provided permissions to createRoleByName', async () => { + const perms = { chat: { read: true, write: false } } as unknown as IRole['permissions']; + const role = mockRole({ permissions: perms }); + const deps = createDeps({ createRoleByName: jest.fn().mockResolvedValue(role) }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + body: { name: 'editor', permissions: perms }, + }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(201); + expect(json).toHaveBeenCalledWith({ role }); + expect(deps.createRoleByName).toHaveBeenCalledWith({ + name: 'editor', + permissions: perms, + }); + }); + it('returns 400 when name is missing', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); @@ -210,6 +269,30 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'name is required' }); }); + it('returns 400 when name contains control characters', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ body: { name: 'bad\x00name' } }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'name contains invalid characters' }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 400 when name is a reserved path segment', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ body: { name: 'members' } }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'name is a reserved path segment' }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); + it('returns 400 when name exceeds max length', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); @@ -428,6 +511,29 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateUsersByRole).not.toHaveBeenCalled(); }); + it('renames and updates description in a single request', async () => { + const role = mockRole({ name: 'senior-editor', description: 'Updated desc' }); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateRoleByName: jest.fn().mockResolvedValue(role), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'senior-editor', description: 'Updated desc' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(json).toHaveBeenCalledWith({ role }); + expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'senior-editor'); + expect(deps.updateRoleByName).toHaveBeenCalledWith('editor', { + name: 'senior-editor', + description: 'Updated desc', + }); + }); + it('returns 403 when renaming a system role', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); @@ -696,10 +802,13 @@ describe('createAdminRolesHandlers', () => { }); describe('updateRolePermissions', () => { - it('updates permissions and returns 200', async () => { + it('updates permissions and returns 200 with updated role', async () => { const role = mockRole(); + const updatedRole = mockRole({ + permissions: { chat: { read: true, write: true } } as IRole['permissions'], + }); const deps = createDeps({ - getRoleByName: jest.fn().mockResolvedValue(role), + getRoleByName: jest.fn().mockResolvedValueOnce(role).mockResolvedValueOnce(updatedRole), }); const handlers = createAdminRolesHandlers(deps); const perms = { chat: { read: true, write: true } }; @@ -712,7 +821,7 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateAccessPermissions).toHaveBeenCalledWith('editor', perms, role); expect(status).toHaveBeenCalledWith(200); - expect(json).toHaveBeenCalledWith({ role: expect.objectContaining({ name: 'editor' }) }); + expect(json).toHaveBeenCalledWith({ role: updatedRole }); }); it('returns 400 when permissions is missing', async () => { @@ -1075,6 +1184,20 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ success: true }); }); + it('returns 403 when removing from a non-ADMIN system role', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: SystemRoles.USER, userId: validUserId }, + }); + + await handlers.removeRoleMember(req, res); + + expect(status).toHaveBeenCalledWith(403); + expect(json).toHaveBeenCalledWith({ error: 'Cannot remove members from a system role' }); + expect(deps.getRoleByName).not.toHaveBeenCalled(); + }); + it('returns 400 for invalid ObjectId', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 6b704041ac..cbed2db961 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -6,8 +6,11 @@ import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; import { parsePagination } from './pagination'; +const systemRoleValues = new Set(Object.values(SystemRoles)); const MAX_NAME_LENGTH = 500; const MAX_DESCRIPTION_LENGTH = 2000; +const CONTROL_CHAR_RE = /\p{Cc}/u; +const RESERVED_ROLE_NAMES = new Set(['members', 'permissions']); function validateNameParam(name: string): string | null { if (!name || typeof name !== 'string') { @@ -20,18 +23,21 @@ function validateNameParam(name: string): string | null { } function validateRoleName(name: unknown, required: boolean): string | null { - if (required) { - if (!name || typeof name !== 'string' || !(name as string).trim()) { - return 'name is required'; - } - } else if (name !== undefined) { - if (!name || typeof name !== 'string' || !(name as string).trim()) { - return 'name must be a non-empty string'; - } + if (name === undefined) { + return required ? 'name is required' : null; } - if (name !== undefined && typeof name === 'string' && name.trim().length > MAX_NAME_LENGTH) { + if (typeof name !== 'string' || !name.trim()) { + return required ? 'name is required' : 'name must be a non-empty string'; + } + if (name.trim().length > MAX_NAME_LENGTH) { return `name must not exceed ${MAX_NAME_LENGTH} characters`; } + if (CONTROL_CHAR_RE.test(name)) { + return 'name contains invalid characters'; + } + if (RESERVED_ROLE_NAMES.has(name.trim().toLowerCase())) { + return 'name is a reserved path segment'; + } return null; } @@ -176,7 +182,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { try { await updateUsersByRole(newName, currentName); } catch (rollbackError) { - logger.error('[adminRoles] rollback failed (role not found path):', rollbackError); + logger.error( + `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`, + rollbackError, + ); } } return role; @@ -184,7 +193,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { try { await updateUsersByRole(newName, currentName); } catch (rollbackError) { - logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError); + logger.error( + `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`, + rollbackError, + ); } throw error; } @@ -210,10 +222,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { const trimmedName = body.name?.trim() ?? ''; const isRename = trimmedName !== '' && trimmedName !== name; - if (isRename && SystemRoles[name as keyof typeof SystemRoles]) { + if (isRename && systemRoleValues.has(name)) { return res.status(403).json({ error: 'Cannot rename system role' }); } - if (isRename && SystemRoles[trimmedName as keyof typeof SystemRoles]) { + if (isRename && systemRoleValues.has(trimmedName)) { return res.status(403).json({ error: 'Cannot use a reserved system role name' }); } @@ -310,7 +322,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { if (paramError) { return res.status(400).json({ error: paramError }); } - if (SystemRoles[name as keyof typeof SystemRoles]) { + if (systemRoleValues.has(name)) { return res.status(403).json({ error: 'Cannot delete system role' }); } @@ -405,6 +417,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(400).json({ error: 'Invalid user ID format' }); } + if (systemRoleValues.has(name) && name !== SystemRoles.ADMIN) { + return res.status(403).json({ error: 'Cannot remove members from a system role' }); + } + const existing = await getRoleByName(name); if (!existing) { return res.status(404).json({ error: 'Role not found' }); diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index 88c8a59115..830d88ff4c 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, RoleConflictError, type RoleMethods, type RoleDeps } from './role'; +import { createRoleMethods, RoleConflictError } from './role'; +import type { RoleMethods, RoleDeps } from './role'; import { createUserMethods, DEFAULT_SESSION_EXPIRY, type UserMethods } from './user'; import { createKeyMethods, type KeyMethods } from './key'; import { createFileMethods, type FileMethods } from './file'; diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 7ac3778739..4f17b651a9 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -9,6 +9,8 @@ import type { Model } from 'mongoose'; import type { IRole, IUser } from '~/types'; import logger from '~/config/winston'; +const systemRoleValues = new Set(Object.values(SystemRoles)); + export class RoleConflictError extends Error { constructor(message: string) { super(message); @@ -96,7 +98,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } const role = await query.lean().exec(); - if (!role && SystemRoles[roleName as keyof typeof SystemRoles]) { + if (!role && systemRoleValues.has(roleName)) { const newRole = await new Role(roleDefaults[roleName as keyof typeof roleDefaults]).save(); if (cache) { await cache.set(roleName, newRole); @@ -380,7 +382,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol throw new Error('Role name is required'); } const trimmed = name.trim(); - if (SystemRoles[trimmed as keyof typeof SystemRoles]) { + if (systemRoleValues.has(trimmed)) { throw new RoleConflictError(`Cannot create role with reserved system name: ${name}`); } const Role = mongoose.models.Role; @@ -414,9 +416,16 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol return role.toObject() as IRole; } - /** Guards against deleting system roles. Reassigns affected users back to USER. */ + /** + * Guards against deleting system roles. Reassigns affected users back to USER. + * + * Note: the user reassignment (`updateMany`) runs before `findOneAndDelete`. + * Without a MongoDB transaction these two operations are non-atomic — if the + * delete fails after the reassignment, users will already have been moved to + * USER while the role document still exists. + */ async function deleteRoleByName(roleName: string): Promise { - if (SystemRoles[roleName as keyof typeof SystemRoles]) { + if (systemRoleValues.has(roleName)) { throw new Error(`Cannot delete system role: ${roleName}`); } const Role = mongoose.models.Role; diff --git a/packages/data-schemas/src/schema/user.ts b/packages/data-schemas/src/schema/user.ts index 92680415bd..d39a813a6c 100644 --- a/packages/data-schemas/src/schema/user.ts +++ b/packages/data-schemas/src/schema/user.ts @@ -64,6 +64,7 @@ const userSchema = new Schema( role: { type: String, default: SystemRoles.USER, + index: true, }, googleId: { type: String,