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
This commit is contained in:
Dustin Healy 2026-03-27 08:36:06 -07:00
parent b590951190
commit 1fb205ef54
2 changed files with 21 additions and 10 deletions

View file

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

View file

@ -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)) {