Security: fix privilege escalation and mass assignment in UserController - #843
Merged
Conversation
… M1) C1 — Authorization checks on profile edit endpoints: - postProfileInfoEdit, postProfilePasswordEdit, postProfilePictureEdit: abort(403) if the request id differs from Auth::id() and the caller is not an Administrator - postAdminEdit: requires Administrator role unconditionally; previously any authenticated user could set any account to ROOT with a single POST request C2 — Replace $request->post() mass assignment in edit() with allowlist: - Switch from unset()-based denylist to $request->only([...]) covering only the fields a web edit is expected to modify; password handled separately - Removes the possibility of injecting role, api_token, or any other sensitive field via the POST body M1 — Remove role and api_token from User::$fillable: - role is now set explicitly ($user->role = ...; $user->save()) in postAdminEdit - api_token is always set via direct property assignment in ensureAPIToken() - Removing these from fillable adds model-level protection independent of controller logic Adds PrivilegeEscalationTest covering all eight cases (4 blocked, 2 admin allowed, 2 mass-assignment blocked).
edwh
force-pushed
the
security/c1-c2-m1-privilege-escalation
branch
from
April 24, 2026 12:20
a791dd5 to
b02d3d1
Compare
… fix Removing from fillable breaks user creation in factories, seeders and the Taskfile setup for Playwright tests (firstOrCreate with role=>2 silently discards the role, so jane@bloggs.net is not admin and the event approval UI never renders). The real M1 fix is the abort(403) guards in UserController, not the fillable restriction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
role and api_token are now excluded from $fillable to prevent privilege escalation via mass assignment (security: C2/M1). All internal code that legitimately sets these fields has been updated to use direct property assignment ($user->role = X; $user->save()) instead. Factory states (administrator, host, etc.) now use afterCreating callbacks so they work correctly without relying on mass assignment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Factories use Model::unguarded() internally so definition() attributes bypass \$fillable — no afterCreating callbacks needed. The previous afterCreating approach caused segfaults from double save() calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three related vulnerabilities in
UserController.phpidentified in a responsible disclosure from iFixit Engineering:postProfileInfoEdit,postProfilePasswordEdit,postProfilePictureEditandpostAdminEditall accepted anidparameter from the POST body without verifying the caller owned that account or had permission to edit it. Any authenticated user could promote themselves to ROOT with a single request (POST /profile/edit-admin-settingswithuser_role=1).edit(): The method passed$request->post()(minus a few unset keys) directly toUser::update(), allowing any caller to inject arbitrary fillable fields — includingroleandapi_token— via the POST body.User::$fillable:roleandapi_tokenwere mass-assignable. Removing them from$fillableadds model-level protection independent of controller logic.Changes
app/Http/Controllers/UserController.phppostProfileInfoEdit,postProfilePasswordEdit,postProfilePictureEdit: added ownership check — abort 403 ifid != Auth::id()and caller is not AdministratorpostAdminEdit: requires Administrator role unconditionally (previously no check at all)postAdminEdit: sets$user->roledirectly rather than viaupdate([])since role is no longer mass-assignableedit(): replaced$request->post()+ denylist with$request->only([...])allowlist covering only the fields a web edit is expected to modifyapp/User.phproleandapi_tokenfrom$fillableCode Quality Review
api_tokenis set via direct property assignment inensureAPIToken()— not affectedroleis set via direct assignment in the patchedpostAdminEdit— not affectedUser::update()were found to rely on mass-assigningroleorapi_tokenTest Plan
PrivilegeEscalationTest— 8 tests, 12 assertions, all greenidedit()endpointapi_tokenviaedit()endpoint