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

@@ -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),
   };
 }

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);

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<string>(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' });

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';

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<string>(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<IRole | null> {
-    if (SystemRoles[roleName as keyof typeof SystemRoles]) {
+    if (systemRoleValues.has(roleName)) {
       throw new Error(`Cannot delete system role: ${roleName}`);
     }
     const Role = mongoose.models.Role;

packages/data-schemas/src/schema/user.ts

@@ -64,6 +64,7 @@ const userSchema = new Schema<IUser>(
     role: {
       type: String,
       default: SystemRoles.USER,
+      index: true,
     },
     googleId: {
       type: String,