From 1fb205ef54b9984c3ebed14a8ac722ed87aba411 Mon Sep 17 00:00:00 2001
From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com>
Date: Fri, 27 Mar 2026 08:36:06 -0700
Subject: [PATCH] fix: exact-case reserved name check, consistent validation,
 cleaner createRole

- Remove .toLowerCase() from reserved name check so only exact matches
  (members, permissions) are rejected, not legitimate names like "Members"
- Extract trimmed const in validateRoleName for consistent validation
- Add control char check to validateNameParam for parity with body validation
- Build createRole roleData conditionally to avoid passing description: undefined
- Expand deleteRoleByName JSDoc documenting self-healing design and no-op trade-off
---
 packages/api/src/admin/roles.ts           | 19 +++++++++++++------
 packages/data-schemas/src/methods/role.ts | 12 ++++++++----
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts
index cbed2db961..f26b910d90 100644
--- a/packages/api/src/admin/roles.ts
+++ b/packages/api/src/admin/roles.ts
@@ -19,6 +19,9 @@ function validateNameParam(name: string): string | null {
   if (name.length > MAX_NAME_LENGTH) {
     return `name must not exceed ${MAX_NAME_LENGTH} characters`;
   }
+  if (CONTROL_CHAR_RE.test(name)) {
+    return 'name contains invalid characters';
+  }
   return null;
 }
 
@@ -29,13 +32,14 @@ function validateRoleName(name: unknown, required: boolean): string | null {
   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) {
+  const trimmed = name.trim();
+  if (trimmed.length > MAX_NAME_LENGTH) {
     return `name must not exceed ${MAX_NAME_LENGTH} characters`;
   }
-  if (CONTROL_CHAR_RE.test(name)) {
+  if (CONTROL_CHAR_RE.test(trimmed)) {
     return 'name contains invalid characters';
   }
-  if (RESERVED_ROLE_NAMES.has(name.trim().toLowerCase())) {
+  if (RESERVED_ROLE_NAMES.has(trimmed)) {
     return 'name is a reserved path segment';
   }
   return null;
@@ -154,11 +158,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
       ) {
         return res.status(400).json({ error: 'permissions must be an object' });
       }
-      const role = await createRoleByName({
+      const roleData: Partial<IRole> = {
         name: (name as string).trim(),
-        description,
         permissions: permissions ?? {},
-      });
+      };
+      if (description !== undefined) {
+        roleData.description = description;
+      }
+      const role = await createRoleByName(roleData);
       return res.status(201).json({ role });
     } catch (error) {
       logger.error('[adminRoles] createRole error:', error);
diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts
index 4f17b651a9..08fb92606e 100644
--- a/packages/data-schemas/src/methods/role.ts
+++ b/packages/data-schemas/src/methods/role.ts
@@ -419,10 +419,14 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
   /**
    * 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.
+   * No existence pre-check is performed: for a nonexistent role the `updateMany`
+   * is a harmless no-op and `findOneAndDelete` returns null. This is intentional
+   * so that calling delete after a partial failure still cleans up orphaned user
+   * references and cache entries (self-healing).
+   *
+   * Without a MongoDB transaction the two writes 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 (systemRoleValues.has(roleName)) {