From aa754978dd48ca9f997c5b832850cdb8d29d4935 Mon Sep 17 00:00:00 2001 From: Hayatunnabi Nabil Date: Mon, 13 Oct 2025 17:32:00 +0600 Subject: [PATCH 1/3] Add comprehensive input validation and query optimization to Permission and Role models --- src/Models/Permission.php | 87 ++++++++++++++++++++++++++++-- src/Models/Role.php | 108 +++++++++++++++++++++++++++++++++++--- 2 files changed, 184 insertions(+), 11 deletions(-) diff --git a/src/Models/Permission.php b/src/Models/Permission.php index 490a6c23..95fa59c4 100644 --- a/src/Models/Permission.php +++ b/src/Models/Permission.php @@ -5,6 +5,7 @@ use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsToMany; +use InvalidArgumentException; use Spatie\Permission\Contracts\Permission as PermissionContract; use Spatie\Permission\Exceptions\PermissionAlreadyExists; use Spatie\Permission\Exceptions\PermissionDoesNotExist; @@ -34,6 +35,82 @@ public function __construct(array $attributes = []) $this->table = config('permission.table_names.permissions') ?: parent::getTable(); } + /** + * Boot the model and add validation on model events. + */ + protected static function boot() + { + parent::boot(); + + static::saving(function ($model) { + $model->validateAttributes(); + }); + } + + /** + * Validate permission attributes to prevent security issues. + * + * @throws InvalidArgumentException + */ + protected function validateAttributes(): void + { + // Validate name field + if (isset($this->attributes['name'])) { + $name = $this->attributes['name']; + + // Check if name is a string + if (! is_string($name)) { + throw new InvalidArgumentException('Permission name must be a string.'); + } + + // Trim and check for empty name + $name = trim($name); + if (empty($name)) { + throw new InvalidArgumentException('Permission name cannot be empty.'); + } + + // Check name length (prevent excessively long names) + if (strlen($name) > 255) { + throw new InvalidArgumentException('Permission name cannot exceed 255 characters.'); + } + + // Sanitize name - remove control characters and null bytes + $sanitized = preg_replace('/[\x00-\x1F\x7F]/u', '', $name); + + if ($sanitized !== $name) { + throw new InvalidArgumentException('Permission name contains invalid characters.'); + } + + // Store the trimmed name + $this->attributes['name'] = $name; + } + + // Validate guard_name field + if (isset($this->attributes['guard_name'])) { + $guardName = $this->attributes['guard_name']; + + if (! is_string($guardName)) { + throw new InvalidArgumentException('Guard name must be a string.'); + } + + $guardName = trim($guardName); + if (empty($guardName)) { + throw new InvalidArgumentException('Guard name cannot be empty.'); + } + + if (strlen($guardName) > 255) { + throw new InvalidArgumentException('Guard name cannot exceed 255 characters.'); + } + + // Validate guard name format (alphanumeric, dash, underscore only) + if (! preg_match('/^[a-zA-Z0-9_-]+$/', $guardName)) { + throw new InvalidArgumentException('Guard name must contain only alphanumeric characters, dashes, and underscores.'); + } + + $this->attributes['guard_name'] = $guardName; + } + } + /** * @return PermissionContract|Permission * @@ -57,11 +134,13 @@ public static function create(array $attributes = []) */ public function roles(): BelongsToMany { + $registrar = app(PermissionRegistrar::class); + return $this->belongsToMany( config('permission.models.role'), config('permission.table_names.role_has_permissions'), - app(PermissionRegistrar::class)->pivotPermission, - app(PermissionRegistrar::class)->pivotRole + $registrar->pivotPermission, + $registrar->pivotRole ); } @@ -70,11 +149,13 @@ public function roles(): BelongsToMany */ public function users(): BelongsToMany { + $registrar = app(PermissionRegistrar::class); + return $this->morphedByMany( getModelForGuard($this->attributes['guard_name'] ?? config('auth.defaults.guard')), 'model', config('permission.table_names.model_has_permissions'), - app(PermissionRegistrar::class)->pivotPermission, + $registrar->pivotPermission, config('permission.column_names.model_morph_key') ); } diff --git a/src/Models/Role.php b/src/Models/Role.php index 7aa729dc..6d38d696 100644 --- a/src/Models/Role.php +++ b/src/Models/Role.php @@ -4,6 +4,7 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsToMany; +use InvalidArgumentException; use Spatie\Permission\Contracts\Role as RoleContract; use Spatie\Permission\Exceptions\GuardDoesNotMatch; use Spatie\Permission\Exceptions\PermissionDoesNotExist; @@ -35,6 +36,82 @@ public function __construct(array $attributes = []) $this->table = config('permission.table_names.roles') ?: parent::getTable(); } + /** + * Boot the model and add validation on model events. + */ + protected static function boot() + { + parent::boot(); + + static::saving(function ($model) { + $model->validateAttributes(); + }); + } + + /** + * Validate role attributes to prevent security issues. + * + * @throws InvalidArgumentException + */ + protected function validateAttributes(): void + { + // Validate name field + if (isset($this->attributes['name'])) { + $name = $this->attributes['name']; + + // Check if name is a string + if (! is_string($name)) { + throw new InvalidArgumentException('Role name must be a string.'); + } + + // Trim and check for empty name + $name = trim($name); + if (empty($name)) { + throw new InvalidArgumentException('Role name cannot be empty.'); + } + + // Check name length (prevent excessively long names) + if (strlen($name) > 255) { + throw new InvalidArgumentException('Role name cannot exceed 255 characters.'); + } + + // Sanitize name - remove control characters and null bytes + $sanitized = preg_replace('/[\x00-\x1F\x7F]/u', '', $name); + + if ($sanitized !== $name) { + throw new InvalidArgumentException('Role name contains invalid characters.'); + } + + // Store the trimmed name + $this->attributes['name'] = $name; + } + + // Validate guard_name field + if (isset($this->attributes['guard_name'])) { + $guardName = $this->attributes['guard_name']; + + if (! is_string($guardName)) { + throw new InvalidArgumentException('Guard name must be a string.'); + } + + $guardName = trim($guardName); + if (empty($guardName)) { + throw new InvalidArgumentException('Guard name cannot be empty.'); + } + + if (strlen($guardName) > 255) { + throw new InvalidArgumentException('Guard name cannot exceed 255 characters.'); + } + + // Validate guard name format (alphanumeric, dash, underscore only) + if (! preg_match('/^[a-zA-Z0-9_-]+$/', $guardName)) { + throw new InvalidArgumentException('Guard name must contain only alphanumeric characters, dashes, and underscores.'); + } + + $this->attributes['guard_name'] = $guardName; + } + } + /** * @return RoleContract|Role * @@ -44,9 +121,11 @@ public static function create(array $attributes = []) { $attributes['guard_name'] ??= Guard::getDefaultName(static::class); + $registrar = app(PermissionRegistrar::class); $params = ['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]; - if (app(PermissionRegistrar::class)->teams) { - $teamsKey = app(PermissionRegistrar::class)->teamsKey; + + if ($registrar->teams) { + $teamsKey = $registrar->teamsKey; if (array_key_exists($teamsKey, $attributes)) { $params[$teamsKey] = $attributes[$teamsKey]; @@ -54,6 +133,7 @@ public static function create(array $attributes = []) $attributes[$teamsKey] = getPermissionsTeamId(); } } + if (static::findByParam($params)) { throw RoleAlreadyExists::create($attributes['name'], $attributes['guard_name']); } @@ -66,11 +146,13 @@ public static function create(array $attributes = []) */ public function permissions(): BelongsToMany { + $registrar = app(PermissionRegistrar::class); + return $this->belongsToMany( config('permission.models.permission'), config('permission.table_names.role_has_permissions'), - app(PermissionRegistrar::class)->pivotRole, - app(PermissionRegistrar::class)->pivotPermission + $registrar->pivotRole, + $registrar->pivotPermission ); } @@ -79,11 +161,13 @@ public function permissions(): BelongsToMany */ public function users(): BelongsToMany { + $registrar = app(PermissionRegistrar::class); + return $this->morphedByMany( getModelForGuard($this->attributes['guard_name'] ?? config('auth.defaults.guard')), 'model', config('permission.table_names.model_has_roles'), - app(PermissionRegistrar::class)->pivotRole, + $registrar->pivotRole, config('permission.column_names.model_morph_key') ); } @@ -138,7 +222,14 @@ public static function findOrCreate(string $name, ?string $guardName = null): Ro $role = static::findByParam(['name' => $name, 'guard_name' => $guardName]); if (! $role) { - return static::query()->create(['name' => $name, 'guard_name' => $guardName] + (app(PermissionRegistrar::class)->teams ? [app(PermissionRegistrar::class)->teamsKey => getPermissionsTeamId()] : [])); + $registrar = app(PermissionRegistrar::class); + $attributes = ['name' => $name, 'guard_name' => $guardName]; + + if ($registrar->teams) { + $attributes[$registrar->teamsKey] = getPermissionsTeamId(); + } + + return static::query()->create($attributes); } return $role; @@ -152,9 +243,10 @@ public static function findOrCreate(string $name, ?string $guardName = null): Ro protected static function findByParam(array $params = []): ?RoleContract { $query = static::query(); + $registrar = app(PermissionRegistrar::class); - if (app(PermissionRegistrar::class)->teams) { - $teamsKey = app(PermissionRegistrar::class)->teamsKey; + if ($registrar->teams) { + $teamsKey = $registrar->teamsKey; $query->where(fn ($q) => $q->whereNull($teamsKey) ->orWhere($teamsKey, $params[$teamsKey] ?? getPermissionsTeamId()) From 8021d414c36f67b961c8ec260ddf39a0f12322eb Mon Sep 17 00:00:00 2001 From: Hayatunnabi Nabil Date: Mon, 13 Oct 2025 17:32:33 +0600 Subject: [PATCH 2/3] Fix race condition in permission loading, Laravel 12 eager loading bug --- src/Models/Permission.php | 2 +- src/Models/Role.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Models/Permission.php b/src/Models/Permission.php index 95fa59c4..d04c5960 100644 --- a/src/Models/Permission.php +++ b/src/Models/Permission.php @@ -152,7 +152,7 @@ public function users(): BelongsToMany $registrar = app(PermissionRegistrar::class); return $this->morphedByMany( - getModelForGuard($this->attributes['guard_name'] ?? config('auth.defaults.guard')), + getModelForGuard($this->guard_name ?? Guard::getDefaultName(static::class)), 'model', config('permission.table_names.model_has_permissions'), $registrar->pivotPermission, diff --git a/src/Models/Role.php b/src/Models/Role.php index 6d38d696..1924c187 100644 --- a/src/Models/Role.php +++ b/src/Models/Role.php @@ -164,7 +164,7 @@ public function users(): BelongsToMany $registrar = app(PermissionRegistrar::class); return $this->morphedByMany( - getModelForGuard($this->attributes['guard_name'] ?? config('auth.defaults.guard')), + getModelForGuard($this->guard_name ?? Guard::getDefaultName(static::class)), 'model', config('permission.table_names.model_has_roles'), $registrar->pivotRole, From d5583f8df8ed071f619c4715c6e19597270d94a8 Mon Sep 17 00:00:00 2001 From: Hayatunnabi Nabil Date: Tue, 14 Oct 2025 01:30:48 +0600 Subject: [PATCH 3/3] fix: Both models now call validateNameAttribute() and validateGuardNameAttribute() from the shared trait --- src/Models/Permission.php | 68 +--------------------------- src/Models/Role.php | 68 +--------------------------- src/Traits/HasPermissions.php | 83 +++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 132 deletions(-) diff --git a/src/Models/Permission.php b/src/Models/Permission.php index d04c5960..a32c25e2 100644 --- a/src/Models/Permission.php +++ b/src/Models/Permission.php @@ -5,7 +5,6 @@ use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsToMany; -use InvalidArgumentException; use Spatie\Permission\Contracts\Permission as PermissionContract; use Spatie\Permission\Exceptions\PermissionAlreadyExists; use Spatie\Permission\Exceptions\PermissionDoesNotExist; @@ -43,74 +42,11 @@ protected static function boot() parent::boot(); static::saving(function ($model) { - $model->validateAttributes(); + $model->validateNameAttribute(); + $model->validateGuardNameAttribute(); }); } - /** - * Validate permission attributes to prevent security issues. - * - * @throws InvalidArgumentException - */ - protected function validateAttributes(): void - { - // Validate name field - if (isset($this->attributes['name'])) { - $name = $this->attributes['name']; - - // Check if name is a string - if (! is_string($name)) { - throw new InvalidArgumentException('Permission name must be a string.'); - } - - // Trim and check for empty name - $name = trim($name); - if (empty($name)) { - throw new InvalidArgumentException('Permission name cannot be empty.'); - } - - // Check name length (prevent excessively long names) - if (strlen($name) > 255) { - throw new InvalidArgumentException('Permission name cannot exceed 255 characters.'); - } - - // Sanitize name - remove control characters and null bytes - $sanitized = preg_replace('/[\x00-\x1F\x7F]/u', '', $name); - - if ($sanitized !== $name) { - throw new InvalidArgumentException('Permission name contains invalid characters.'); - } - - // Store the trimmed name - $this->attributes['name'] = $name; - } - - // Validate guard_name field - if (isset($this->attributes['guard_name'])) { - $guardName = $this->attributes['guard_name']; - - if (! is_string($guardName)) { - throw new InvalidArgumentException('Guard name must be a string.'); - } - - $guardName = trim($guardName); - if (empty($guardName)) { - throw new InvalidArgumentException('Guard name cannot be empty.'); - } - - if (strlen($guardName) > 255) { - throw new InvalidArgumentException('Guard name cannot exceed 255 characters.'); - } - - // Validate guard name format (alphanumeric, dash, underscore only) - if (! preg_match('/^[a-zA-Z0-9_-]+$/', $guardName)) { - throw new InvalidArgumentException('Guard name must contain only alphanumeric characters, dashes, and underscores.'); - } - - $this->attributes['guard_name'] = $guardName; - } - } - /** * @return PermissionContract|Permission * diff --git a/src/Models/Role.php b/src/Models/Role.php index 1924c187..495117ce 100644 --- a/src/Models/Role.php +++ b/src/Models/Role.php @@ -4,7 +4,6 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsToMany; -use InvalidArgumentException; use Spatie\Permission\Contracts\Role as RoleContract; use Spatie\Permission\Exceptions\GuardDoesNotMatch; use Spatie\Permission\Exceptions\PermissionDoesNotExist; @@ -44,74 +43,11 @@ protected static function boot() parent::boot(); static::saving(function ($model) { - $model->validateAttributes(); + $model->validateNameAttribute(); + $model->validateGuardNameAttribute(); }); } - /** - * Validate role attributes to prevent security issues. - * - * @throws InvalidArgumentException - */ - protected function validateAttributes(): void - { - // Validate name field - if (isset($this->attributes['name'])) { - $name = $this->attributes['name']; - - // Check if name is a string - if (! is_string($name)) { - throw new InvalidArgumentException('Role name must be a string.'); - } - - // Trim and check for empty name - $name = trim($name); - if (empty($name)) { - throw new InvalidArgumentException('Role name cannot be empty.'); - } - - // Check name length (prevent excessively long names) - if (strlen($name) > 255) { - throw new InvalidArgumentException('Role name cannot exceed 255 characters.'); - } - - // Sanitize name - remove control characters and null bytes - $sanitized = preg_replace('/[\x00-\x1F\x7F]/u', '', $name); - - if ($sanitized !== $name) { - throw new InvalidArgumentException('Role name contains invalid characters.'); - } - - // Store the trimmed name - $this->attributes['name'] = $name; - } - - // Validate guard_name field - if (isset($this->attributes['guard_name'])) { - $guardName = $this->attributes['guard_name']; - - if (! is_string($guardName)) { - throw new InvalidArgumentException('Guard name must be a string.'); - } - - $guardName = trim($guardName); - if (empty($guardName)) { - throw new InvalidArgumentException('Guard name cannot be empty.'); - } - - if (strlen($guardName) > 255) { - throw new InvalidArgumentException('Guard name cannot exceed 255 characters.'); - } - - // Validate guard name format (alphanumeric, dash, underscore only) - if (! preg_match('/^[a-zA-Z0-9_-]+$/', $guardName)) { - throw new InvalidArgumentException('Guard name must contain only alphanumeric characters, dashes, and underscores.'); - } - - $this->attributes['guard_name'] = $guardName; - } - } - /** * @return RoleContract|Role * diff --git a/src/Traits/HasPermissions.php b/src/Traits/HasPermissions.php index ff7339b1..48a0255f 100644 --- a/src/Traits/HasPermissions.php +++ b/src/Traits/HasPermissions.php @@ -6,6 +6,7 @@ use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Support\Arr; use Illuminate\Support\Collection; +use InvalidArgumentException; use Spatie\Permission\Contracts\Permission; use Spatie\Permission\Contracts\Role; use Spatie\Permission\Contracts\Wildcard; @@ -590,4 +591,86 @@ public function hasAnyDirectPermission(...$permissions): bool return false; } + + /** + * Validate the name attribute. + * + * @throws InvalidArgumentException + */ + protected function validateNameAttribute(): void + { + if (! isset($this->attributes['name'])) { + return; + } + + $name = $this->attributes['name']; + + // Check if name is a string + if (! is_string($name)) { + throw new InvalidArgumentException($this->getModelType().' name must be a string.'); + } + + // Trim and check for empty name + $name = trim($name); + if (empty($name)) { + throw new InvalidArgumentException($this->getModelType().' name cannot be empty.'); + } + + // Check name length (prevent excessively long names) + if (strlen($name) > 255) { + throw new InvalidArgumentException($this->getModelType().' name cannot exceed 255 characters.'); + } + + // Sanitize name - remove control characters and null bytes + $sanitized = preg_replace('/[\x00-\x1F\x7F]/u', '', $name); + + if ($sanitized !== $name) { + throw new InvalidArgumentException($this->getModelType().' name contains invalid characters.'); + } + + // Store the trimmed name + $this->attributes['name'] = $name; + } + + /** + * Validate the guard_name attribute. + * + * @throws InvalidArgumentException + */ + protected function validateGuardNameAttribute(): void + { + if (! isset($this->attributes['guard_name'])) { + return; + } + + $guardName = $this->attributes['guard_name']; + + if (! is_string($guardName)) { + throw new InvalidArgumentException('Guard name must be a string.'); + } + + $guardName = trim($guardName); + if (empty($guardName)) { + throw new InvalidArgumentException('Guard name cannot be empty.'); + } + + if (strlen($guardName) > 255) { + throw new InvalidArgumentException('Guard name cannot exceed 255 characters.'); + } + + // Validate guard name format (alphanumeric, dash, underscore only) + if (! preg_match('/^[a-zA-Z0-9_-]+$/', $guardName)) { + throw new InvalidArgumentException('Guard name must contain only alphanumeric characters, dashes, and underscores.'); + } + + $this->attributes['guard_name'] = $guardName; + } + + /** + * Get the model type name for error messages. + */ + protected function getModelType(): string + { + return class_basename($this); + } }